[pbs-devel] applied: [PATCH v2 proxmox-backup] fix #5217: api: send missing header when upgrading to HTTP/2

[Thomas Lamprecht]

Am 01/03/2024 um 14:49 schrieb Max Carrara:
> The "Connection: upgrade" header is strictly expected to be included
> in the response sent by the server when an upgrade to a different
> protocol is requested by the client.
> 
> A detailed explanation as well as additional context follows below.
> 
> Background
> ----------
> 
> Neither RFC 9110 (HTTP Semantics) [0] or RFC 7540 (HTTP/2) [1]
> *explicitly state* that the "Connection: upgrade" header must be
> included *in the server's response* when a client requests an upgrade
> to a different protocol. For clients, however, it is specified [2]:
> 
>> A sender of Upgrade MUST also send an "Upgrade" connection option in
>> the Connection header field (Section 7.6.1) to inform intermediaries
>> not to forward this field.
> 
> Yet, the example for a response provided in RFC 9110 [3] does include
> the header:
> 
>> HTTP/1.1 101 Switching Protocols
>> Connection: upgrade
>> Upgrade: websocket
>>
>> [... data stream switches to websocket with an appropriate response
>> (as defined by new protocol) to the "GET /hello" request ...]
> 
> The example in RFC 7540 [4] also includes the header:
> 
>> HTTP/1.1 101 Switching Protocols
>> Connection: Upgrade
>> Upgrade: h2c
>>
>> [ HTTP/2 connection ...
> 
> Additionally, RFC 9113 [5], which obsoletes RFC 7540 [1], mentions:
> 
>> The HTTP/1.1 Upgrade mechanism is deprecated and no longer specified
>> in this document. It was never widely deployed, with plaintext
>> HTTP/2 users choosing to use the prior-knowledge implementation
>> instead.
> 
> I therefore initially concluded that whether the "Connection: upgrade"
> header should / should not / must / must not be included in the
> server's response was unspecified.
> 
> Further Revelations
> -------------------
> 
> As per Thomas's suggestion [6], I opened a discussion over at Caddy's
> GitHub issue tracker [7]. This discussion revealed that RFC 7230 [8],
> which is obsoleted by RFC 9110 [1], does in fact specify that the
> header must be included [9], thus proving my initial conclusion to be
> incorrect:

Much thanks for opening that issue and digging deeper into this, now we
actually know that adding this header is not just a workaround for some
odd proxy implementation, but actually warranted in any way.

> 
>> When a header field aside from Connection is used to supply control
>> information for or about the current connection, the sender MUST
>> list the corresponding field-name within the Connection header
>> field. [...]
> 
> The discussion [7] also revealed that the WebSocket RFC 6455 [10]
> specifies the usage of the "Connection" header in more detail [11]:
> 
>> 3.  If the response lacks a |Connection| header field or the
>> |Connection| header field doesn't contain a token that is an ASCII
>> case-insensitive match for the value "Upgrade", the client MUST
>> _Fail the WebSocket Connection_.
> 
> Furthermore [12]:
> 
>> 5.  If the server chooses to accept the incoming connection, it
>> MUST reply with a valid HTTP response indicating the following.
>>
>> [...]
>>
>>     3.  A |Connection| header field with value "Upgrade".
> 
> Although we're using the upgrade mechanism for HTTP/2, the WebSocket
> RFC [10] specifies its usage more clearly and most importantly, in an
> explicit manner.
> 
> Final Conclusion
> ----------------
> 
> The "Connection: upgrade" header must therefore definitely be included
> as per RFC 7230 section 6.1 [8], even if the newer RFC 9110 [1] does
> not specify this explicitly anymore.
> 
> Finally, this fixes bug #5217 [13] and allows PBS to be deployed
> behind Caddy. Also tested with nginx, which still works as expected.
> 
> [0]: https://datatracker.ietf.org/doc/html/rfc9110
> [1]: https://datatracker.ietf.org/doc/html/rfc7540
> [2]: https://datatracker.ietf.org/doc/html/rfc9110#section-7.8-14
> [3]: https://datatracker.ietf.org/doc/html/rfc9110#section-7.8-13
> [4]: https://datatracker.ietf.org/doc/html/rfc7540#section-3.2
> [5]: https://datatracker.ietf.org/doc/html/rfc9113#appendix-B-2.3
> [6]: https://lists.proxmox.com/pipermail/pbs-devel/2024-February/007948.html
> [7]: https://github.com/caddyserver/caddy/issues/6134
> [8]: https://datatracker.ietf.org/doc/html/rfc7230
> [9]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1
> [10]: https://datatracker.ietf.org/doc/html/rfc6455
> [11]: https://datatracker.ietf.org/doc/html/rfc6455#section-4.1
> [12]: https://datatracker.ietf.org/doc/html/rfc6455#section-4.2.2
> [13]: https://bugzilla.proxmox.com/show_bug.cgi?id=5217
> 
> Signed-off-by: Max Carrara <m.carrara at proxmox.com>
> ---
>  src/api2/backup/mod.rs | 3 ++-
>  src/api2/reader/mod.rs | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!