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

[Maxim Dounin]

Hello!

On Wed, Aug 31, 2022 at 07:52:15PM +0400, Roman Arutyunyan wrote:

> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # Date 1661436099 -14400
> #      Thu Aug 25 18:01:39 2022 +0400
> # Node ID 4b856f1dff939e4eb9c131e17af061cf2c38cfac
> # Parent  069a4813e8d6d7ec662d282a10f5f7062ebd817f
> Core: support for reading PROXY protocol v2 TLVs.

First of all, could you please provide details on the use case?  
I've seen requests for writing proxy protocol TLVs to upstream 
servers (see ticket #1639), but not yet seen any meaningful 
reading requests.

> The TLV values are available in HTTP and Stream variables
> $proxy_protocol_tlv_0xN, where N is a hexadecimal TLV type number with no
> leading zeroes.

I can't say I like the "hexadecimal TLV type number with no 
leading zeroes" approach, especially given that the specification 
uses leading zeroes in TLV types.  With leading zeros might be 
better, to match specification.

Also, it might worth the effort to actually add names for known 
types instead or in addition to numbers.

Another question is PP2_TYPE_SSL, which is itself a complex 
structure and a list of multiple subtypes.  Provided 

Given the above, not sure if the approach with early parsing and 
header-like list as in the patch is the good idea.  Just 
preserving TLVs as is and parsing them all during variable 
evaluation might be easier and more efficient.

Also, the idea of merging TLV values with identical types looks 
wrong to me, especially given that many TLSs are binary.  
Specification does not seem to define the behaviour here, 
unfortunately.  As far as I understand, HAProxy itself still 
doesn't implement PPv2 parsing, so there is not reference 
implementation either.  On the other hand, it should be easy 
enough to check all TLVs for duplicate by using a 256-bit bitmask 
and reject connections if there are any duplicates.

> 
> 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
> @@ -40,6 +40,12 @@ typedef struct {
>  } ngx_proxy_protocol_inet6_addrs_t;
>  
>  
> +typedef struct {
> +    u_char                                  type;
> +    u_char                                  len[2];
> +} ngx_proxy_protocol_tlv_t;
> +
> +
>  static u_char *ngx_proxy_protocol_read_addr(ngx_connection_t *c, u_char *p,
>      u_char *last, ngx_str_t *addr);
>  static u_char *ngx_proxy_protocol_read_port(u_char *p, u_char *last,
> @@ -273,8 +279,11 @@ ngx_proxy_protocol_v2_read(ngx_connectio
>      size_t                              len;
>      socklen_t                           socklen;
>      ngx_uint_t                          version, command, family, transport;
> +    ngx_list_t                         *pp_tlv;

tlvs?

See above though, it probably doesn't make sense to parse TLVs 
here.

>      ngx_sockaddr_t                      src_sockaddr, dst_sockaddr;
> +    ngx_table_elt_t                    *t;

Just in case, most ngx_table_elt_t variables are named "h", as in 
"header".

[...]

-- 
Maxim Dounin
http://mdounin.ru/