[pve-devel] applied series: [PATCH v2 http-server 0/4] refactor HTTP request processing

[Fabian Grünbichler]

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[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
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>