From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 E5CB58538 for ; Fri, 3 Mar 2023 18:30:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CE2301A3F1 for ; Fri, 3 Mar 2023 18:30:03 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 3 Mar 2023 18:30:02 +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 03218461EF for ; Fri, 3 Mar 2023 18:30:02 +0100 (CET) From: Max Carrara To: pve-devel@lists.proxmox.com Date: Fri, 3 Mar 2023 18:29:47 +0100 Message-Id: <20230303172951.197711-1-m.carrara@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.043 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [PATCH v2 http-server 0/4] refactor HTTP request processing X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Mar 2023 17:30:03 -0000 This patch series refactors the request processing algorithm and also provides a patch for redirecting HTTP requests to HTTPS. The refactor provided in the first two patches is much simpler than the one made in v1[0], as it only moves the header processing, authentication handling, and further request handling into two separate subroutines, as suggested[1]. Contrary to v1[0], the behaviour remains *actually* identical. The third patch builds upon the refactor of the first two patches and implements the enhancement proposed in #4494[2], adding another subroutine which ensures that clients are connecting via TLS/SSL (if the server itself uses it). The fourth patch fixes some whitespace formatting issues scattered across the file and may be dropped if not desired. Testing ------- Each patch was tested separately on a VM which can not only be accessed via its IP, but also via a FQDN that a DNS in my virtual network is able to resolve. Using PVE (logging in, uploading ISOs, accessing the VNC console, creating VMs, etc.) worked as it did before each patch, regardless of whether the VM is accessed via its IP or its FQDN. The redirection from HTTP to HTTPS works consistently, irrespective of which actions are performed. Notes Regarding Security ------------------------ If a server uses TLS/SSL, such as pveproxy, every single request from clients which have not initiated and successfully completed a TLS handshake is answered with either a '301 Moved Permanently' or a '308 Permanent Redirect'. However: > The user agent MAY use the Location field value for automatic > redirection. The server's response content usually contains a > short hypertext note with a hyperlink to the new URI(s). - RFC9110, 15.4.2. [3] and 15.4.9. [4] The client doesn't have to follow the `Location` field that's provided by the server. They may therefore still send HTTP requests, which in this case will just be answered with more 301s or 308s. If such an HTTP request contains something like an authentication cookie, said cookie could be stolen (e.g. a man-in-the-middle attack). However, our authentication cookie[5] uses the Secure Attribute specified in RFC 6265[6], so any compliant browser will include that cookie only over HTTPS connections. This can be verified e.g. via Firefox: 1. Open the PVE web UI after applying patches 1-3 2. Open the Firefox Developer Tools using CTRL+SHIFT+I 3. Navigate to the "Network" tab 4. Inspect any secure request's header - you will find that the `Cookie` header field includes the `PVEAuthCookie` 5. Change `https://` of the domain of your PVE web UI to `http://` and hit Enter 6. Check the insecure request that you just forced Firefox to make in the network tab - the `Cookie` header might not even be included anymore, because the `PVEAuthCookie` isn't being used. Therefore, as long as the user's browser is compliant, there shouldn't be any security concerns in our case. Further Considerations ---------------------- Despite all the above, we do not support HSTS[7] and SameSite Cookies[8] yet, which we should implement eventually, since Firefox e.g. even emits a warning regarding SameSite Cookies in the developer console: > Cookie “PVEAuthCookie” does not have a proper “SameSite” attribute > value. Soon, cookies without the “SameSite” attribute or with an > invalid value will be treated as “Lax”. This means that the cookie > will no longer be sent in third-party contexts. If your application > depends on this cookie being available in such contexts, please add > the “SameSite=None“ attribute to it. To know more about the > “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite If required, I can implement both HSTS and SameSite cookies in a follow-up series (if this one happens to get applied) or in a v3. [0] https://lists.proxmox.com/pipermail/pve-devel/2023-February/055809.html [1] https://lists.proxmox.com/pipermail/pve-devel/2023-February/055950.html [2] https://bugzilla.proxmox.com/show_bug.cgi?id=4494 [3] https://httpwg.org/specs/rfc9110.html#status.301 [4] https://httpwg.org/specs/rfc9110.html#status.308 [5] https://git.proxmox.com/?p=pve-http-server.git;a=blob;f=src/PVE/APIServer/Formatter.pm;h=20455a02704e579c18f0455abb66b88eb04098f7;hb=HEAD#l95 [6] https://httpwg.org/specs/rfc6265.html#secure-attribute [7] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security [8] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite Max Carrara (4): anyevent: move header processing into separate subroutine anyevent: move auth and request handling into separate subroutine fix #4494: anyevent: redirect HTTP to HTTPS anyevent: fix whitespace src/PVE/APIServer/AnyEvent.pm | 451 ++++++++++++++++++++-------------- 1 file changed, 271 insertions(+), 180 deletions(-) -- 2.30.2