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