Add support for compressing the body of responses with
`Content-Encoding: deflate` following [RFC9110]. Note that in this
context `deflate` is actually a "zlib" data format as defined in
[RFC1950].
To preserve the current behavior we prefer `Content-Encoding: gzip`
whenever `gzip` is listed as one of the encodings in the
`Accept-Encoding` header and the data should be compressed.
Friedrich Weber [Wed, 24 Jan 2024 11:38:56 +0000 (12:38 +0100)]
access control: avoid "uninitialized value" warning if using IP ranges
ALLOW_FROM/DENY_FROM accept any syntax understood by Net::IP. However,
if an IP range like "10.1.1.1-10.1.1.3" is configured, a confusing
Perl warning is printed to the syslog on a match:
Use of uninitialized value in concatenation (.) or string at [...]
The reason is that we use Net::IP::prefix to prepare a debug message,
but this returns undef if a range was specified. To avoid the warning,
use Net::IP::print to obtain a string representation instead.
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
set_min/max_proto_version is recommended upstream nowadays, and it seems to be
required for some reason if *only* TLS v1.3 is supposed to be enabled.
querying via get_options gives us the union of
- system-wide openssl defaults
- our internal SSL defaults
- flags configured by the user via /etc/default/pveproxy
note that by default only 1.2 and 1.3 are enabled in the first place, so
disabling either leaves a single version being set as min and max.
OpenSSL as packaged in Debian bookworm now ships a compat symlink for
the "combined" CA certificates file (CAfile) as managed by
update-ca-certificates. This symlink is in addition to the CApath
one that has been around for a file. The new symlink in turn gets
picked up by openssl-using code that uses the default values for the
trust store.
Every TLS context initialization now reads the full combined file,
even if no TLS is actually employed on a connection. We do such an
initialization for every proxied connection (where our HTTP server is
the client).
By specifying an explicit CA path (that is identical to the default
one), the old behaviour of looking up each CA certificate
individually iff needed is enabled again.
For an API endpoint where HTTP request handling is the bottle neck
(as opposed to the actual API handler), this improves performance of
proxied requests to be back in line with unproxied ones handled
directly by the unprivileged daemon. For all proxied requests, CPU
usage is decreased as well.
The default CAfile and CApath contain the same certificates, so there
should be no change in trusted certificates. Additionally,
certificate fingerprints are pinned in this context and verified
against the cache of pinned fingerprints.
Reported-by: Roland Kletzing <roland.kletzing@cybercon.de> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
avoid AnyEvent::AIO to fix CPU spinning if pure-perl lib is installed
when installing AnyEvent::AIO (by the package libanyevent-aio-perl),
the worker forks of our daemons using AnyEvent would consume 100% cpu
cycles while trying to do an epoll_wait which no one read from. It
was not really clear which part of the code set that fd up.
Reading the documentation of the related perl modules, it became
clear that the issue was with AnyEvent::IO. By default this uses
AnyEvent::AIO (if installed) which in turn uses IO::AIO which
explicitly says it uses pthreads and is not really fork compatible
(which we rely heavy upon).
It seems that IO::AIO sets up some fds with epoll in the END handler
of it's library (or earlier, but sends data to it in the END
handler), so that when using 'exit' instead of 'POSIX::_exit' (which
we do in PVE::Daemon) creates the observed behavior.
Interestingly we did not use any of AnyEvent::IO's functionality, so
we can safely remove it. Even if we would have used it in the past,
without AnyEvent::AIO the IO would not have been async anyway (the
pure perl impl doesn't do async IO). My best guess is that we wanted
to use it, but noticed that we can't, and forgot to remove the use
statement. (This is indicated by a comment that says aio_load is not
async unless IO::AIO is used)
This only occurs now, since bookworm is the first debian release to
package the library.
if we ever wanted to use AnyEvent::AIO, there are probably two other
ways that could fix it:
* replace our 'exit()' calls with 'POSIX::_exit()', which seems to
fix it, but other side effects are currently unknown
* use 'IO::AIO::reinit()' after forking, which also seems to fix it,
but perldoc says it 'is not an operation supported by any
standards, but happens to work on GNU/LINUX and some newer BSD
systems'
With this fix, one can safely install 'libanyevent-aio-perl' and
'libperl-languageserver-perl' (the only user of it AFAICS) on a
Proxmox VE or Proxmox Mail Gateway system.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 14 Apr 2023 14:38:13 +0000 (16:38 +0200)]
replace junior semicolon with actual one
commas can be used in two ways, quoting Perl Best Practices (PBP):
> The comma actually has two distinct roles in Perl. In a scalar
> context, it is (as those former C programmers expect) a sequencing
> operator: “do this, then do that”. But in a list context, such as
> the argument list of a print, the comma is a list separator, not
> technically an operator at all.
-- PBP, page 69
And the separating variant is called a "junior semicolon" by PBP.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 14 Apr 2023 14:09:07 +0000 (16:09 +0200)]
explicitly disallow tmpfilename parameter in query URL
This is an internal parameter and we pass the actual internal one
around via the $reqstate variable, so avoid confusion and return a
clear error if a POST request sets this query parameter.
Reported-by: Friedrich Weber <f.weber@proxmox.com> Suggested-by: Friedrich Weber <f.weber@proxmox.com> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
file upload: don't calculate MD5, log file name instead
Until now, we calculated the MD5 hash of any uploaded file during the
upload, regardless of whether the user chose to provide a hash sum
and algorithm. The hash was only logged in the syslog.
As the user can provide a hash algorithm and a checksum when
uploading a file, which gets automatically checked (after the
upload), this is not needed anymore. Instead, the file name is
logged.
Depending on the speed of the network and the cpu, upload speed or
CPU usage might improve: All tests were made by uploading a 3.6GB iso
from the PVE host to a local VM. First line is with md5, second
without.
125MB/s network
In this test, pveproxy worker used x % CPU during the upload. As you can see, the reduced CPU usage is noticable in slower networks.
~75% CPU: multipart upload complete (size: 3826831360B time: 30.764s rate: 118.63MiB/s md5sum: 8c651682056205967d530697c98d98c3)
~60% CPU: multipart upload complete (size: 3826831360B time: 30.763s rate: 118.64MiB/s filename: ubuntu-22.04.1-desktop-amd64.iso)
Friedrich Weber [Tue, 11 Apr 2023 12:19:47 +0000 (14:19 +0200)]
multipart upload: properly parse file parts without Content-Type
As reported in the forum, multipart requests are parsed incorrectly if
the file part header contains *only* Content-Disposition, but no other
fields (in particular, no Content-Type). As a result, uploaded files
are mangled: In most cases, an additional carriage return and line
feed (\r\n) is prepended to the file contents.
As an example, consider the following file part (with explicit \r\n
for clarity):
The current parsing code for file parts roughly works as follows:
1) Consume the Content-Disposition field including the trailing \r\n
2) Consume and ignore everything up to and including the next \r\n\r\n
3) Read the file contents
This works fine in the example above. However, it has a bug in case
Content-Disposition is the *only* header field:
Now, step 1 already consumes the first half of the \r\n\r\n sequence
that marks the end of the part headers. As a result, step 3 starts
reading the file at a wrong offset:
- If the remaining contents of the read buffer (currently sized 16KiB)
contain \r\n\r\n, step 2 consumes everything up to and including
this marker and step 3 starts reading file contents there. As a
result, the uploaded file is truncated at its beginning.
- Otherwise, step 2 is a noop and step 3 considers the remaining
second half of the \r\n\r\n marker to be part of the file contents.
As a result, the uploaded file is prepended with an extra \r\n.
To fix this, modify step 1 to *not* consume the trailing \r\n. This
keeps the \r\n\r\n marker intact, no matter whether additional header
fields are present or not.
Max Carrara [Fri, 3 Mar 2023 17:29:50 +0000 (18:29 +0100)]
fix #4494: redirect HTTP to HTTPS
Allow HTTP connections up until the request's header has been
parsed and processed. If no TLS handshake has been completed
beforehand, the server now responds with either a
'301 Moved Permanently' or a '308 Permanent Redirect' as noted in the
MDN web docs[1].
This is done after the header was parsed; for the redirect to work,
the `Host` header field of the request is used to create the
`Location` field of the response. This makes redirections independent
of how the server is accessed (e.g. via IP, localhost, FQDN, ...)
possible.
Upon redirection the client is immediately disconnected; otherwise,
they would have to wait for the connection to time out until
they may reconnect via TLS again.
In commit 0fbcbc2 ("fix #3990: multipart upload: rework to fix
uploading small files") a breaking change was added which now
requires the file's multipart part to have a `Content-Type` even
though the content type is never used. It is just included to consume
those bytes so phase 2 (dumping the file contents into the file) can
continue.
Avoid this overly strict and unused requirement.
Signed-off-by: John Hollowell <jhollowe@johnhollowell.com>
[ T: resolve merge conflict, add telling commit message ] Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
iso
```
would fail. These headers now also get ignored, as we don't use them.
Also, upload now works when the Content-Disposition header isn't the
first, i.e.:
```
--XVH95dt1-A3J8mWiLCmHCW4roSC7-gBntjATBy--
Content-Type: text/plain; charset=ISO-8859-1
Content-Disposition: form-data; name="content"
```
Fixed upload was tested using
* Curl
* GUI
* Apache HttpClient 5
Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com> Reviewed-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com> Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
Dominik Csapak [Mon, 7 Nov 2022 15:07:49 +0000 (16:07 +0100)]
upload: re-allow white space in filenames
Some fields (e.g. filename) can contain spaces, but our 'trim'
function, would only return the value until the first whitespace
character instead of removing leading/trailing white space. This lead
to passing the wrong filename to the API call (e.g. 'foo' instead of
'foo (1).iso'), which would then reject it because of the 'wrong'
extension.
Fix this by just using the battle proven trim from pve-common.
Fixes: 0fbcbc2 ("fix #3990: multipart upload: rework to fix uploading small files") Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
fix #3990: multipart upload: rework to fix uploading small files
== The problem
Upload of files smaller than ~16kb failed.
This was because the code assumed that it would be called
several times, and each time would do a certain action.
When the whole file was in the buffer, this failed because
the function would try parssing the first part in the payload and
then return, instead of parsing the rest of the available data.
== Why not just modifying the current code a bit?
The code had a lot of nested control statements and a
non-intuitive control flow (phase 0->2->1->1->1 and so on).
The way the phases and buffer content were checked made it
rather difficult to just fix a few lines.
== What was changed
* Part headers are parsed with a single regex line each,
which improves code readability.
* Parsing the content is done in order, so even if the whole data is in the buffer,
it can be read in one go. Files of arbitrary sizes can be uploaded.
Acknowledging the Content-Disposition header makes it possible for the
backend to tell the browser whether a file should be downloaded,
rather than displayed inline, and what it's default name should be.
Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
request: add missing early return to complete error check
While $self->error will immediately send out a 4xx or 5xx response
anyhow its still good to cover against possible side effects (e.g.,
from future code in that branch) on the server and return directly.
Note that this is mostly for completeness sake, we already have
another check that covers this one for relevant cases in commit 580d540ea907ba15f64379c5bb69ecf1a49a875f.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
requests: assert that theres no @ in the URLs authority
We don't expect any userinfo in the authority and t o avoid that this
allows some leverage in doing weird things later its better to error
out early on such requests.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
pass through streaming: only allow from privileged local pvedaemon
Ensures that no external request can control streaming on proxying
requests as safety net for when we'd have another issue in the
request handling part.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
basically only possible to trigger with chromium based browsers
(chrome, edge, opera) but besides those having the biggest usage
currently its not that nice in any way.
Users could inject headers in their response, which isn't really that
bad itself, as they won't really do anything at least for sane
browsers that don't allow setting third party cookies by default
(unlike again, chrome), in which case one can create huge cookies
that then trigger the max header size check on requests, DOS'ing an
user's access to a PVE interface if they can get them to visit a
malicious site (a clear cooki actione would allow visiting it again)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Reported-by: STAR Labs <info@starlabs.sg>
like the TLS <= 1.2 cipher list, but needs a different option since the
format and values are incompatible. AnyEvent doesn't yet handle this
directly like the cipher list, so set it directly on the context.
requires corresponding patch in pve-manager (which reads the config, and
passes relevant parts back to the API server).
fix #3807: don't attempt response on closed handle
if a client closes the connection while the API server is
waiting/stalling here, the handle will disappear, and sending a response
is no longer possible.
(this issue is only cosmetic, but if such clients are a regular
occurrence it might get quite noisy in the logs)
if the WS gets disconnected without any data having been sent first,
wbuf (and thus `length $wbuf`) is undef. the actual length of the buffer
is not relevant here anyway, just the fact that it's non-empty - so
avoid the undef warning by dropping the unnecessary comparison.
Stoiko Ivanov [Mon, 15 Nov 2021 20:50:43 +0000 (21:50 +0100)]
fix #3724: disable TLS renegotiation
The issue is probably not critical and best addressed by not running
the perl API servers in an exposed environment or when this needs to
be done by installing a reverse proxy in front of them.
The DOS potential of the perl daemons is limited more by the limited
number of parallel workers (and the memory constraints of starting
more of them), than by the CPU cycles wasted on TLS renegotiation.
Still disabling TLS renegotiation should show very little downside:
* it was removed in TLS 1.3 for security reasons
* it was the way nginx addressed this issue [1].
* we do not use client certificate authentication
Tested by running `openssl s_client -no_tls1_3 -connect 192.0.2.1:8006`
and issuing a `HEAD / HTTP/1.1\nR\n`
with and without the patch.
[1] 70bd187c4c386d82d6e4d180e0db84f361d1be02 at
https://github.com/nginx/nginx (although that code adapted to
the various changes in openssl API over the years) Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
for proxied requests, we usually tear down the proxy connection
immediately when closing the source connection. this is not the correct
course of action for bulk one-way data streams that are proxied, where
the source connection might be closed, but the proxy connection might
still have data in the write buffer that needs to be written out.
push_shutdown already handles this case (closing the socket/FH after it
has been fully drained).
one example for such a proxied data stream is the 'migrate' data for a
remote migration, which gets proxied over a websocket connection.
terminating the proxied connection early makes the target VM crash for
obvious reasons.
this major release still needs to have an incompatible client, the next
one can drop setting a protocol client-side, and the one after that can
remove the protocol handling on the server side.
projects / pve-http-server.git / log