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