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

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

[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:

> 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@proxmox.com>
---
 src/api2/backup/mod.rs | 3 ++-
 src/api2/reader/mod.rs | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 18fad745..013043dd 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -3,7 +3,7 @@
 use anyhow::{bail, format_err, Error};
 use futures::*;
 use hex::FromHex;
-use hyper::header::{HeaderValue, UPGRADE};
+use hyper::header::{HeaderValue, CONNECTION, UPGRADE};
 use hyper::http::request::Parts;
 use hyper::{Body, Request, Response, StatusCode};
 use serde::Deserialize;
@@ -318,6 +318,7 @@ fn upgrade_to_backup_protocol(
 
         let response = Response::builder()
             .status(StatusCode::SWITCHING_PROTOCOLS)
+            .header(CONNECTION, HeaderValue::from_static("upgrade"))
             .header(
                 UPGRADE,
                 HeaderValue::from_static(PROXMOX_BACKUP_PROTOCOL_ID_V1!()),
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index b1a5612b..42b42838 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -3,7 +3,7 @@
 use anyhow::{bail, format_err, Error};
 use futures::*;
 use hex::FromHex;
-use hyper::header::{self, HeaderValue, UPGRADE};
+use hyper::header::{self, HeaderValue, CONNECTION, UPGRADE};
 use hyper::http::request::Parts;
 use hyper::{Body, Request, Response, StatusCode};
 use serde::Deserialize;
@@ -209,6 +209,7 @@ fn upgrade_to_backup_reader_protocol(
 
         let response = Response::builder()
             .status(StatusCode::SWITCHING_PROTOCOLS)
+            .header(CONNECTION, HeaderValue::from_static("upgrade"))
             .header(
                 UPGRADE,
                 HeaderValue::from_static(PROXMOX_BACKUP_READER_PROTOCOL_ID_V1!()),
-- 
2.39.2

[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@proxmox.com>
> ---
>  src/api2/backup/mod.rs | 3 ++-
>  src/api2/reader/mod.rs | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!