From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <m.carrara@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id EBBDF96D32
for <pbs-devel@lists.proxmox.com>; Fri, 1 Mar 2024 14:49:49 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id C3144324E7
for <pbs-devel@lists.proxmox.com>; Fri, 1 Mar 2024 14:49:19 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pbs-devel@lists.proxmox.com>; Fri, 1 Mar 2024 14:49:18 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0447048310
for <pbs-devel@lists.proxmox.com>; Fri, 1 Mar 2024 14:49:18 +0100 (CET)
From: Max Carrara <m.carrara@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Fri, 1 Mar 2024 14:49:06 +0100
Message-Id: <20240301134906.137579-1-m.carrara@proxmox.com>
X-Mailer: git-send-email 2.39.2
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.402 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
KAM_NUMSUBJECT 0.5 Subject ends in numbers excluding current years
POISEN_SPAM_PILL 0.1 Meta: its spam
POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes
POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
T_SCC_BODY_TEXT_LINE -0.01 -
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [ietf.org, mod.rs, proxmox.com]
Subject: [pbs-devel] [PATCH v2 proxmox-backup] fix #5217: api: send missing
header when upgrading to HTTP/2
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 01 Mar 2024 13:49:50 -0000
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