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

[Roman Arutyunyan]

Hi,

On Mon, Sep 05, 2022 at 03:52:49AM +0300, Maxim Dounin wrote:
> 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 known cases are these:

- https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-target-groups.html#proxy-protocol
- https://docs.microsoft.com/en-us/azure/private-link/private-link-service-overview#getting-connection-information-using-tcp-proxy-v2
- https://cloud.google.com/vpc/docs/configure-private-service-connect-producer#proxy-protocol

The data may need further parsing, but it can be done in njs or perl.

> > 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.

This is indeed a good idea and we have such plans as a further extenion of this
work.  One of the problems is however that the abovementioned TLV variables
are specified in internal documents of AWS/Azure/GCP which are not standards.
They can be changed anytime, while we have to maintain those variables in
nginx.  Also, raw variables give more flexibility in supporting less known TLVs.

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

This is an obvious one.  However we had exactly zero requests for this.

> 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.

In this case, if we have two variables, say $proxy_protocol_tlv_ssl_{sni, alpn},
we'll parse the entire TLV block twice - once per variable evaluation.

> 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.

This can be added, thanks.

> > 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/
> _______________________________________________
> nginx-devel mailing list -- nginx-devel at nginx.org
> To unsubscribe send an email to nginx-devel-leave at nginx.org