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

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

[Max Carrara]

The "Connection: upgrade" header is occasionally, but not 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 follows below.

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 ...

This would explain why certain reverse proxies (like Caddy) expect the
header to be included in the response.

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.

Caddy therefore does not deviate from the HTTP/1.1 and HTTP/2
specifications; neither to we. I therefore conclude that it's best to
just include the header in this specific response anyway, as no harm
is done by doing so.

This fixes bug #5217 [6] 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://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

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

[Thomas Lamprecht]

Am 28/02/2024 um 17:20 schrieb Max Carrara:
> Caddy therefore does not deviate from the HTTP/1.1 and HTTP/2
> specifications; neither to we. I therefore conclude that it's best to
> just include the header in this specific response anyway, as no harm
> is done by doing so.
> 

can be fine, but is there any discussion about this from Caddy's site?

As while we can take this in and PBS should work with Caddy in-between,
other HTTP/2 daemons that follow spec while not having that header
still won't work.
So, if there isn't any issue tracker entry for this at caddy's side it
might be worth opening one even if we apply this here, and if there's
an existing issue then referencing it here would be great.

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

[Max Carrara]

On 2/28/24 19:03, Thomas Lamprecht wrote:
> Am 28/02/2024 um 17:20 schrieb Max Carrara:
>> Caddy therefore does not deviate from the HTTP/1.1 and HTTP/2
>> specifications; neither to we. I therefore conclude that it's best to
>> just include the header in this specific response anyway, as no harm
>> is done by doing so.
>>
> 
> can be fine, but is there any discussion about this from Caddy's site?

I haven't found anything, no.

> 
> As while we can take this in and PBS should work with Caddy in-between,
> other HTTP/2 daemons that follow spec while not having that header
> still won't work.
> So, if there isn't any issue tracker entry for this at caddy's side it
> might be worth opening one even if we apply this here, and if there's
> an existing issue then referencing it here would be great.

I've opened a more detailed issue over on Caddy's GitHub where I've
described pretty much everything I had discovered [0]. Most of the
information there regarding the RFC specs and whatnot are already in the
commit message of the patch.

I'd say we wait a little bit and see what the Caddy folks' opinions are on
this before we apply this patch in case some additional things / infos turn
up that might be of relevance.

In either case I'll send in a v2 with an updated commit message once there's
been some progress in that regard. Will reference the issue in the message
as well (and might reword some things, now that I've read it again).

[0]: https://github.com/caddyserver/caddy/issues/6134