Hello!
On Mon, Oct 31, 2022 at 04:07:00PM +0400, Roman Arutyunyan wrote:
> While testing the feature, it became clear that 107 bytes as maximum PROXY
> protocol header size may not be enough. This limit came from v1 and stayed
> unchanged when v2 was added. With this limit, there are only 79 bytes left for
> TLVs in case of IPv4 and 55 bytes in case of IPv6.
>
> Attached is a patch that increases buffer size up to 65K while reading PROXY
> protocl header. Writing is not changed since only v1 is supported.
[...]
> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1667216033 -14400
> # Mon Oct 31 15:33:53 2022 +0400
> # Node ID 8c99314f90eccc2ad5aaf4b3de5368e964c4ffe0
> # Parent 81b4326daac70d6de70abbc3fe36d4f6e3da54a2
> Increased maximum read PROXY protocol header size.
>
> Maximum size for reading the PROXY protocol header is increased to 65536 to
> accommodate a bigger number of TLVs, which are supported since cca4c8a715de.
>
> Maximum size for writing the PROXY protocol header is not changed since only
> version 1 is currently supported.
>
> diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
> --- a/src/core/ngx_proxy_protocol.c
> +++ b/src/core/ngx_proxy_protocol.c
> @@ -281,7 +281,7 @@ ngx_proxy_protocol_write(ngx_connection_
> {
> ngx_uint_t port, lport;
>
> - if (last - buf < NGX_PROXY_PROTOCOL_MAX_HEADER) {
> + if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
> return NULL;
> }
>
> diff --git a/src/core/ngx_proxy_protocol.h b/src/core/ngx_proxy_protocol.h
> --- a/src/core/ngx_proxy_protocol.h
> +++ b/src/core/ngx_proxy_protocol.h
> @@ -13,7 +13,8 @@
> #include <ngx_core.h>
>
>
> -#define NGX_PROXY_PROTOCOL_MAX_HEADER 107
> +#define NGX_PROXY_PROTOCOL_V1_MAX_HEADER 107
> +#define NGX_PROXY_PROTOCOL_MAX_HEADER 65536
>
>
> struct ngx_proxy_protocol_s {
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -641,7 +641,7 @@ ngx_http_alloc_request(ngx_connection_t
> static void
> ngx_http_ssl_handshake(ngx_event_t *rev)
> {
> - u_char *p, buf[NGX_PROXY_PROTOCOL_MAX_HEADER + 1];
> + u_char *p;
> size_t size;
> ssize_t n;
> ngx_err_t err;
> @@ -651,6 +651,7 @@ ngx_http_ssl_handshake(ngx_event_t *rev)
> ngx_http_ssl_srv_conf_t *sscf;
> ngx_http_core_loc_conf_t *clcf;
> ngx_http_core_srv_conf_t *cscf;
> + static u_char buf[NGX_PROXY_PROTOCOL_MAX_HEADER + 1];
>
I'm somewhat sceptical about the idea of allocation of up to 3
global 64k buffers, especially given that the protocol requires
atomic sending of the header, which means it cannot reliably
exceed 1500 bytes in most configurations.
Further, the maximum size of the proxy protocol header in non-SSL
case is still limited by client_header_buffer_size, which is 1024
bytes by default. Assuming there will be headers which does not
fit into 1k buffer, it is not clear why these will magically work
in case of SSL, but will require client_header_buffer_size tuning
otherwise.
Might be some dynamically allocated buffer, sized after
client_header_buffer_size, like in non-SSL case, would be a better
idea?
(It also might make sense to avoid assuming atomicity, which
clearly cannot be guaranteed if the header is larger than MTU.
This will also require dynamically allocated per-connection
buffers.)
Alternatively, we can consider using some larger-than-now on-stack
buffers, something like 4k should be acceptable given we use such
buffers in other places anyway. It should be enough for most
proxy protocol headers.
[...]
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
