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

* [pve-devel] [PATCH v2 http-server 0/4] refactor HTTP request processing
@ 2023-03-03 17:29 Max Carrara
  2023-03-03 17:29 ` [pve-devel] [PATCH v2 http-server 1/4] anyevent: move header processing into separate subroutine Max Carrara
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Max Carrara @ 2023-03-03 17:29 UTC (permalink / raw)
  To: pve-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread