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

[Maxim Dounin]

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 at 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/