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