Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin
November 01, 2022 10:16AM
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
Subject Author Views Posted

[PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 780 August 31, 2022 11:54AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 85 September 01, 2022 05:48AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 97 September 04, 2022 08:54PM

RE: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Eran Kornblau via nginx-devel 84 September 05, 2022 01:14AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 64 September 05, 2022 09:24AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 141 September 05, 2022 12:00PM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 75 September 09, 2022 11:48AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 66 September 12, 2022 05:32PM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 68 September 13, 2022 11:04AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 68 September 19, 2022 10:48PM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 69 September 27, 2022 05:42AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 75 October 10, 2022 09:22PM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 54 October 11, 2022 09:02AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 62 October 11, 2022 04:56PM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 56 October 31, 2022 08:08AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 62 November 01, 2022 10:16AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Roman Arutyunyan 60 November 02, 2022 09:08AM

Re: [PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin 59 November 02, 2022 09:54PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 211
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready