[pve-devel] applied series: [PATCH v2 http-server 0/4] refactor HTTP request processing
f.gruenbichler at proxmox.com
Tue Mar 7 11:20:43 CET 2023
with some style follow-ups and slight rewording of the commit titles.
On March 3, 2023 6:29 pm, Max Carrara wrote:
> 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, as it only moves the header processing,
> authentication handling, and further request handling into two
> separate subroutines, as suggested. Contrary to v1, the
> behaviour remains *actually* identical.
> The third patch builds upon the refactor of the first two patches
> and implements the enhancement proposed in #4494, 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.
> 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'.
>> 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.  and 15.4.9. 
> 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 uses the Secure Attribute specified
> in RFC 6265, 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 and SameSite Cookies
> 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.
>  https://lists.proxmox.com/pipermail/pve-devel/2023-February/055809.html
>  https://lists.proxmox.com/pipermail/pve-devel/2023-February/055950.html
>  https://bugzilla.proxmox.com/show_bug.cgi?id=4494
>  https://httpwg.org/specs/rfc9110.html#status.301
>  https://httpwg.org/specs/rfc9110.html#status.308
>  https://git.proxmox.com/?p=pve-http-server.git;a=blob;f=src/PVE/APIServer/Formatter.pm;h=20455a02704e579c18f0455abb66b88eb04098f7;hb=HEAD#l95
>  https://httpwg.org/specs/rfc6265.html#secure-attribute
>  https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security
>  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(-)
> pve-devel mailing list
> pve-devel at lists.proxmox.com
More information about the pve-devel