On Mon, Mar 11, 2024 at 04:44:15PM +0400, Roman Arutyunyan wrote:
> Hi,
>
> On Wed, Mar 06, 2024 at 06:50:26PM +0400, Sergey Kandaurov wrote:
> > On Thu, Feb 22, 2024 at 07:17:26PM +0400, Roman Arutyunyan wrote:
> > > Hi,
> > >
> > > On Thu, Feb 22, 2024 at 01:59:25AM +0000, J Carter wrote:
> > > > Hello Roman,
> > > >
> > > > On Wed, 21 Feb 2024 17:29:52 +0400
> > > > Roman Arutyunyan <a...@nginx.com> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > >
> >
> > > > > On Wed, Jan 24, 2024 at 12:03:06AM +0300, Maxim Dounin wrote:
> > > > > > [..]
> > > > > > Also, as suggested, using the server address as obtained via PROXY
> > > > > > protocol from the client might be a better solution as long as the
> > > > > > client address was set via PROXY protocol (regardless of whether
> > > > > > address families match or not), and what users expect from the
> > > > > > "proty_protocol on;" when chaining stream proxies in the first
> > > > > > place.
> > > >
> > > > > Checking whether the address used in PROXY writer is in fact the
> > > > > address
> > > > > that was passed in the PROXY header, is complicated. This will
> > > > > either require
> > > > > setting a flag when PROXY address is set by realip, which is ugly.
> > > > > Another approach is checking if the client address written to a PROXY
> > > > > header
> > > > > matches the client address in the received PROXY header. However
> > > > > since
> > > > > currently PROXY protocol addresses are stored as text, and not all
> > > > > addresses
> > > > > have unique text repersentations, this approach would require
> > > > > refactoring all
> > > > > PROXY protocol code + realip modules to switch from text to sockaddr.
> >
> > Checking for the realip context should do the trick, it is created
> > once the client address was successfully substituted. We could then
> > parse the text repr of server address in the PROXY header and use it
> > in PROXY writer. This will allow not to break API.
> > Something like this should work called from the stream proxy module,
> > it allows to preserve an original server address in the PROXY header
> > keeping both addresses unmutated
> > (which seems to be the best way to solve this problem).
> >
> > : extern ngx_module_t ngx_stream_realip_module;
> > :
> > : if (ngx_stream_get_module_ctx(s, ngx_stream_realip_module)) {
> > :
> > : local_sockaddr = c->local_sockaddr;
> > : local_socklen = c->local_socklen;
> > :
> > : if (ngx_parse_addr(c->pool, &addr,
> > c->proxy_protocol->dst_addr.data,
> > : c->proxy_protocol->dst_addr.len)
> > : != NGX_OK)
> > : {
> > : ngx_stream_proxy_finalize(s,
> > NGX_STREAM_INTERNAL_SERVER_ERROR);
> > : return;
> > : }
> > :
> > : ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->dst_port);
> > :
> > : c->local_sockaddr = addr.sockaddr;
> > : c->local_socklen = addr.socklen;
> > : }
> > :
> > : # call ngx_proxy_protocol_write(), other existing stuff
> > :
> > : if (ngx_stream_get_module_ctx(s, ngx_stream_realip_module)) {
> > : c->local_sockaddr = local_sockaddr;
> > : c->local_socklen = local_socklen;
> > : }
> >
> > Can't say I am excited from this code, though.
>
> Neither am I.
>
> > Another way is stick to the INADDR_ANY plan removing the server
> > address part from the PROXY header if not applicable.
> >
> > > > >
> > > > > I suggest that we follow the first plan (INADDR_ANY etc).
> >
> > I agree in general, but see below.
> >
> > > > >
> > > > > > [...]
> > > > >
> > > > > Updated patch attached.
> > > > >
> >
> > > # HG changeset patch
> > > # User Roman Arutyunyan <a...@nginx.com>
> > > # Date 1708522464 -14400
> > > # Wed Feb 21 17:34:24 2024 +0400
> > > # Node ID 2d9bb7b49d64576fa29a673133129f16de3cfbbe
> > > # Parent 44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
> > > Avoiding mixed socket families in PROXY protocol v1 (ticket #2010).
> > >
> > > When using realip module, remote and local addresses of a connection can
> > > belong
> > > to different address families. This previously resulted in generating
> > > PROXY
> > > protocol headers like this:
> > >
> > > PROXY TCP4 127.0.0.1 unix:/tmp/nginx1.sock 55544 0
> >
> > Well, UNIX-domain socket addresses are somewhat rather exsotic,
> > especially in the PROXY protocol. I'd use an example with IPv4/IPv6.
> >
> > >
> > > The PROXY protocol v1 specification does not allow mixed families. The
> > > change
> > > substitutes server address with zero address in this case:
> > >
> > > PROXY TCP4 127.0.0.1 0.0.0.0 55544 0
> > >
> > > As an alternative, "PROXY UNKNOWN" header could be used, which unlike this
> > > header does not contain any useful information about the client.
> > >
> > > Also, the above mentioned format for unix socket address is not specified
> > > in
> > > PROXY protocol v1 and is a by-product of internal nginx representation of
> > > it.
> > > The change eliminates such addresses from PROXY protocol headers as well.
> > >
> > > 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
> > > @@ -279,7 +279,13 @@ ngx_proxy_protocol_read_port(u_char *p,
> > > u_char *
> > > ngx_proxy_protocol_write(ngx_connection_t *c, u_char *buf, u_char *last)
> > > {
> > > - ngx_uint_t port, lport;
> > > + socklen_t local_socklen;
> > > + ngx_uint_t port, lport;
> > > + struct sockaddr *local_sockaddr;
> > > + struct sockaddr_in sin;
> > > +#if (NGX_HAVE_INET6)
> > > + struct sockaddr_in6 sin6;
> > > +#endif
> > >
> > > if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
> > > ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> > > @@ -312,11 +318,35 @@ ngx_proxy_protocol_write(ngx_connection_
> > >
> > > *buf++ = ' ';
> > >
> > > - buf += ngx_sock_ntop(c->local_sockaddr, c->local_socklen, buf, last
> > > - buf,
> > > - 0);
> > > + if (c->sockaddr->sa_family == c->local_sockaddr->sa_family) {
> > > + local_sockaddr = c->local_sockaddr;
> > > + local_socklen = c->local_socklen;
> > > +
> > > + } else {
> > > + switch (c->sockaddr->sa_family) {
> > > +
> > > +#if (NGX_HAVE_INET6)
> > > + case AF_INET6:
> > > + ngx_memzero(&sin6, sizeof(struct sockaddr_in6));
> > > + sin6.sin6_family = AF_INET6;
> > > + local_sockaddr = (struct sockaddr *) &sin6;
> > > + local_socklen = sizeof(struct sockaddr_in6);
> > > + break;
> > > +#endif
> > > +
> > > + default: /* AF_INET */
> > > + ngx_memzero(&sin, sizeof(struct sockaddr));
> >
> > Note that although wildcard adresses are typically implemented
> > using all-zeroes, this is not obligated by standards and hence
> > is an implementation detail. POSIX mandates just the following:
> >
> > : The <netinet/in.h> header shall define the following symbolic
> > : constant for use as a local address in the structure passed to bind():
> > :
> > : INADDR_ANY
> > : IPv4 wildcard address.
> >
> > (Note: it is "IPv4 local host address." in previous editions.)
> >
> > : The <netinet/in.h> header shall declare the following external
> > : variable:
> > :
> > : const struct in6_addr in6addr_any
> > :
> > : This variable is initialized by the system to contain the wildcard
> > : IPv6 address. The <netinet/in.h> header also defines the
> > : IN6ADDR_ANY_INIT macro. This macro must be constant at compile time
> > : and can be used to initialize a variable of type struct in6_addr to
> > : the IPv6 wildcard address.
> >
> > : RATIONALE
> > :
> > : The INADDR_ANY and INADDR_BROADCAST values are byte-order-neutral
> > : and thus their byte order is not specified. Many implementations
> > : have additional constants as extensions, such as INADDR_LOOPBACK,
> > : that are not byte-order-neutral. Traditionally, these constants are
> > : in host byte order, requiring the use of htonl() when using them in
> > : a sockaddr_in structure.
> >
> > So it seems quite legitimate to use a different binary representation.
> > To be on the safe side (and improve code readability), it makes sense
> > to consistently set wildcard addresses explicitly, using INADDR_ANY
> > and in6addr_any. It is not so important in this patch though, because
> > addresses do not cross the kernel, used solely for ngx_sock_ntop().
> >
> > > + sin.sin_family = AF_INET;
> > > + local_sockaddr = (struct sockaddr *) &sin;
> > > + local_socklen = sizeof(struct sockaddr_in);
> > > + break;
> > > + }
> > > + }
> > > +
> > > + buf += ngx_sock_ntop(local_sockaddr, local_socklen, buf, last - buf,
> > > 0);
> > >
> > > port = ngx_inet_get_port(c->sockaddr);
> > > - lport = ngx_inet_get_port(c->local_sockaddr);
> > > + lport = ngx_inet_get_port(local_sockaddr);
> > >
> > > return ngx_slprintf(buf, last, " %ui %ui" CRLF, port, lport);
> > > }
> >
> > Picking INADDR_ANY (and IPv6 equivalent) to substitute non-applicable
> > addresses due to mismatching address family may be unnecessarily
> > strict hiding addresses that can be represented in a different family.
> >
> > IPv4 address can be represented in the IPv4-mapped IPv6 address format.
> > IPv6 address can be represented in the IPv4 format if it's IPv4-mapped.
>
> What if it's not?
This is not a silver bullet.
If not, such address is replaced with INADDR_ANY (or IPv6 equivalent).
[..]
> I agree we can implement a few more workarounds and make it work.
> It all looks like we need a normal solution even if the patch will be bigger.
> Attached is a patch converting PROXY protocol addresses from text to binary
> representation. IMHO we should've done this long ago. Once it's done,
> the mixed family patch will be trivial, attached as well.
>
> --
> Roman Arutyunyan
> # HG changeset patch
> # User Roman Arutyunyan <a...@nginx.com>
> # Date 1709913161 -14400
> # Fri Mar 08 19:52:41 2024 +0400
> # Node ID 07c8c445746c6c74250bad7b42cd26dbb0ed6dd3
> # Parent 2ed3f57dca0a664340bca2236c7d614902db4180
> Storing PROXY protocol addresses as sockaddr.
>
> Previously they were stored as text. This was beneficial for v1 where the
Nitpicking on log description:
s/was/is/ ?
Using text format is still beneficial for v1.
> addresses do arrive as text, however counterproductive for v2, where they
- using emphasising "do" verb here looks unreasonable
- inconsistent comma before "where"
> arrive as binary.
>
> The change allows more flexibility when checking whether PROXY protocol
> address was set as client address. This will be needed in the followup
> changes.
>
> Also, PROXY protocol debugging is improved. Now TLVs are logged after
> parsing.
While logging certainly looks more convenient now, because all TLVs are
logged at once, this somewhat duplicates logging of $proxy_protocol_tlv_
variable evaluation, see ngx_proxy_protocol_get_tlv().
IMHO, it makes sense to remove it from there.
>
> 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
> @@ -67,9 +67,12 @@ typedef struct {
>
>
> static u_char *ngx_proxy_protocol_read_addr(ngx_connection_t *c, u_char *p,
> - u_char *last, ngx_str_t *addr);
> + u_char *last, ngx_addr_t *addr);
> static u_char *ngx_proxy_protocol_read_port(u_char *p, u_char *last,
> - in_port_t *port, u_char sep);
> + struct sockaddr *sa, u_char sep);
> +#if (NGX_DEBUG)
> +static void ngx_proxy_protocol_debug(ngx_connection_t *c, ngx_uint_t
> version);
I suggest to rename it to ngx_proxy_protocol_log(),
for consistency with ngx_ssl_handshake_log(),
and place below all callers including ngx_proxy_protocol_v2_read().
> +#endif
> static u_char *ngx_proxy_protocol_v2_read(ngx_connection_t *c, u_char *buf,
> u_char *last);
> static ngx_int_t ngx_proxy_protocol_lookup_tlv(ngx_connection_t *c,
> @@ -101,6 +104,7 @@ ngx_proxy_protocol_read(ngx_connection_t
> {
> size_t len;
> u_char *p;
> + ngx_addr_t addr;
> ngx_proxy_protocol_t *pp;
>
> static const u_char signature[] = "\r\n\r\n\0\r\nQUIT\n";
> @@ -141,22 +145,28 @@ ngx_proxy_protocol_read(ngx_connection_t
> return NULL;
> }
>
> - p = ngx_proxy_protocol_read_addr(c, p, last, &pp->src_addr);
> + p = ngx_proxy_protocol_read_addr(c, p, last, &addr);
> if (p == NULL) {
> goto invalid;
> }
>
> - p = ngx_proxy_protocol_read_addr(c, p, last, &pp->dst_addr);
> + pp->src_sockaddr = addr.sockaddr;
> + pp->src_socklen = addr.socklen;
> +
> + p = ngx_proxy_protocol_read_addr(c, p, last, &addr);
> if (p == NULL) {
> goto invalid;
> }
>
> - p = ngx_proxy_protocol_read_port(p, last, &pp->src_port, ' ');
> + pp->dst_sockaddr = addr.sockaddr;
> + pp->dst_socklen = addr.socklen;
> +
> + p = ngx_proxy_protocol_read_port(p, last, pp->src_sockaddr, ' ');
> if (p == NULL) {
> goto invalid;
> }
>
> - p = ngx_proxy_protocol_read_port(p, last, &pp->dst_port, CR);
> + p = ngx_proxy_protocol_read_port(p, last, pp->dst_sockaddr, CR);
> if (p == NULL) {
> goto invalid;
> }
> @@ -169,11 +179,11 @@ ngx_proxy_protocol_read(ngx_connection_t
> goto invalid;
> }
>
> - ngx_log_debug4(NGX_LOG_DEBUG_CORE, c->log, 0,
> - "PROXY protocol src: %V %d, dst: %V %d",
> - &pp->src_addr, pp->src_port, &pp->dst_addr, pp->dst_port);
> + c->proxy_protocol = pp;
>
> - c->proxy_protocol = pp;
> +#if (NGX_DEBUG)
> + ngx_proxy_protocol_debug(c, 1);
> +#endif
>
> return p;
>
> @@ -202,7 +212,7 @@ invalid:
>
> static u_char *
> ngx_proxy_protocol_read_addr(ngx_connection_t *c, u_char *p, u_char *last,
> - ngx_str_t *addr)
> + ngx_addr_t *addr)
> {
> size_t len;
> u_char ch, *pos;
> @@ -231,20 +241,16 @@ ngx_proxy_protocol_read_addr(ngx_connect
>
> len = p - pos - 1;
>
> - addr->data = ngx_pnalloc(c->pool, len);
> - if (addr->data == NULL) {
> + if (ngx_parse_addr(c->pool, addr, pos, len) != NGX_OK) {
> return NULL;
> }
>
> - ngx_memcpy(addr->data, pos, len);
> - addr->len = len;
> -
> return p;
> }
>
>
> static u_char *
> -ngx_proxy_protocol_read_port(u_char *p, u_char *last, in_port_t *port,
> +ngx_proxy_protocol_read_port(u_char *p, u_char *last, struct sockaddr *sa,
> u_char sep)
> {
> size_t len;
> @@ -270,11 +276,68 @@ ngx_proxy_protocol_read_port(u_char *p,
> return NULL;
> }
>
> - *port = (in_port_t) n;
> + ngx_inet_set_port(sa, n);
"n" still needs to be casted to "in_port_t".
This used to fix build on MSVC in the past.
See ngx_parse_addr_port(), which is very similar.
>
> return p;
> }
style: two blank lines here
>
> +#if (NGX_DEBUG)
> +
> +static void
> +ngx_proxy_protocol_debug(ngx_connection_t *c, ngx_uint_t version)
> +{
> + u_char *p;
> + size_t n, len;
> + ngx_str_t src_addr, dst_addr;
> + ngx_proxy_protocol_tlv_t *tlv;
> + u_char src_text[NGX_SOCKADDR_STRLEN];
> + u_char dst_text[NGX_SOCKADDR_STRLEN];
> +
> + if ((c->log->log_level & NGX_LOG_DEBUG_CORE) == 0) {
> + return;
> + }
> +
> + src_addr.data = src_text;
> + src_addr.len = ngx_sock_ntop(c->proxy_protocol->src_sockaddr,
> + c->proxy_protocol->src_socklen,
> + src_text, NGX_SOCKADDR_STRLEN, 1);
> +
> + dst_addr.data = dst_text;
> + dst_addr.len = ngx_sock_ntop(c->proxy_protocol->dst_sockaddr,
> + c->proxy_protocol->dst_socklen,
> + dst_text, NGX_SOCKADDR_STRLEN, 1);
> +
> + ngx_log_debug3(NGX_LOG_DEBUG_CORE, c->log, 0,
> + "PROXY protocol v%ui %V %V", version, &src_addr,
> &dst_addr);
> +
> + p = c->proxy_protocol->tlvs.data;
> + n = c->proxy_protocol->tlvs.len;
> +
> + for ( ;; ) {
> + if (n < sizeof(ngx_proxy_protocol_tlv_t)) {
> + break;
> + }
> +
> + tlv = (ngx_proxy_protocol_tlv_t *) p;
> + len = ngx_proxy_protocol_parse_uint16(tlv->len);
> +
> + p += sizeof(ngx_proxy_protocol_tlv_t);
> + n -= sizeof(ngx_proxy_protocol_tlv_t);
> +
> + if (n < len) {
> + break;
> + }
> +
> + ngx_log_debug3(NGX_LOG_DEBUG_CORE, c->log, 0,
> + "PROXY protocol tlv:%02xi %*xs",
> + (ngx_uint_t) tlv->type, len, p);
> +
> + p += len;
> + n -= len;
> + }
> +}
> +
> +#endif
and there
>
> u_char *
> ngx_proxy_protocol_write(ngx_connection_t *c, u_char *buf, u_char *last)
> @@ -327,13 +390,13 @@ ngx_proxy_protocol_v2_read(ngx_connectio
> {
> u_char *end;
> size_t len;
> - socklen_t socklen;
> ngx_uint_t version, command, family, transport;
> - ngx_sockaddr_t src_sockaddr, dst_sockaddr;
> + struct sockaddr_in *sin;
> ngx_proxy_protocol_t *pp;
> ngx_proxy_protocol_header_t *header;
> ngx_proxy_protocol_inet_addrs_t *in;
> #if (NGX_HAVE_INET6)
> + struct sockaddr_in6 *sin6;
> ngx_proxy_protocol_inet6_addrs_t *in6;
> #endif
>
> @@ -394,18 +457,29 @@ ngx_proxy_protocol_v2_read(ngx_connectio
>
> in = (ngx_proxy_protocol_inet_addrs_t *) buf;
>
> - src_sockaddr.sockaddr_in.sin_family = AF_INET;
> - src_sockaddr.sockaddr_in.sin_port = 0;
> - ngx_memcpy(&src_sockaddr.sockaddr_in.sin_addr, in->src_addr, 4);
> + sin = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in));
> + if (sin == NULL) {
> + return NULL;
> + }
> +
> + sin->sin_family = AF_INET;
> + ngx_memcpy(&sin->sin_addr, in->src_addr, 4);
> + sin->sin_port = htons(ngx_proxy_protocol_parse_uint16(in->src_port));
> +
> + pp->src_sockaddr = (struct sockaddr *) sin;
> + pp->src_socklen = sizeof(struct sockaddr_in);
>
> - dst_sockaddr.sockaddr_in.sin_family = AF_INET;
> - dst_sockaddr.sockaddr_in.sin_port = 0;
> - ngx_memcpy(&dst_sockaddr.sockaddr_in.sin_addr, in->dst_addr, 4);
> + sin = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in));
> + if (sin == NULL) {
> + return NULL;
> + }
>
> - pp->src_port = ngx_proxy_protocol_parse_uint16(in->src_port);
> - pp->dst_port = ngx_proxy_protocol_parse_uint16(in->dst_port);
> + sin->sin_family = AF_INET;
> + ngx_memcpy(&sin->sin_addr, in->dst_addr, 4);
> + sin->sin_port = htons(ngx_proxy_protocol_parse_uint16(in->dst_port));
>
> - socklen = sizeof(struct sockaddr_in);
> + pp->dst_sockaddr = (struct sockaddr *) sin;
> + pp->dst_socklen = sizeof(struct sockaddr_in);
>
> buf += sizeof(ngx_proxy_protocol_inet_addrs_t);
>
> @@ -421,18 +495,29 @@ ngx_proxy_protocol_v2_read(ngx_connectio
>
> in6 = (ngx_proxy_protocol_inet6_addrs_t *) buf;
>
> - src_sockaddr.sockaddr_in6.sin6_family = AF_INET6;
> - src_sockaddr.sockaddr_in6.sin6_port = 0;
> - ngx_memcpy(&src_sockaddr.sockaddr_in6.sin6_addr, in6->src_addr, 16);
> + sin6 = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in6));
> + if (sin6 == NULL) {
> + return NULL;
> + }
> +
> + sin6->sin6_family = AF_INET6;
> + ngx_memcpy(&sin6->sin6_addr, in6->src_addr, 16);
> + sin6->sin6_port =
> htons(ngx_proxy_protocol_parse_uint16(in6->src_port));
> +
> + pp->src_sockaddr = (struct sockaddr *) sin6;
> + pp->src_socklen = sizeof(struct sockaddr_in6);
>
> - dst_sockaddr.sockaddr_in6.sin6_family = AF_INET6;
> - dst_sockaddr.sockaddr_in6.sin6_port = 0;
> - ngx_memcpy(&dst_sockaddr.sockaddr_in6.sin6_addr, in6->dst_addr, 16);
> + sin6 = ngx_pcalloc(c->pool, sizeof(struct sockaddr_in6));
> + if (sin6 == NULL) {
> + return NULL;
> + }
>
> - pp->src_port = ngx_proxy_protocol_parse_uint16(in6->src_port);
> - pp->dst_port = ngx_proxy_protocol_parse_uint16(in6->dst_port);
> + sin6->sin6_family = AF_INET6;
> + ngx_memcpy(&sin6->sin6_addr, in6->dst_addr, 16);
> + sin6->sin6_port =
> htons(ngx_proxy_protocol_parse_uint16(in6->dst_port));
>
> - socklen = sizeof(struct sockaddr_in6);
> + pp->dst_sockaddr = (struct sockaddr *) sin6;
> + pp->dst_socklen = sizeof(struct sockaddr_in6);
>
> buf += sizeof(ngx_proxy_protocol_inet6_addrs_t);
>
> @@ -447,26 +532,6 @@ ngx_proxy_protocol_v2_read(ngx_connectio
> return end;
> }
>
> - pp->src_addr.data = ngx_pnalloc(c->pool, NGX_SOCKADDR_STRLEN);
> - if (pp->src_addr.data == NULL) {
> - return NULL;
> - }
> -
> - pp->src_addr.len = ngx_sock_ntop(&src_sockaddr.sockaddr, socklen,
> - pp->src_addr.data, NGX_SOCKADDR_STRLEN,
> 0);
> -
> - pp->dst_addr.data = ngx_pnalloc(c->pool, NGX_SOCKADDR_STRLEN);
> - if (pp->dst_addr.data == NULL) {
> - return NULL;
> - }
> -
> - pp->dst_addr.len = ngx_sock_ntop(&dst_sockaddr.sockaddr, socklen,
> - pp->dst_addr.data, NGX_SOCKADDR_STRLEN,
> 0);
> -
> - ngx_log_debug4(NGX_LOG_DEBUG_CORE, c->log, 0,
> - "PROXY protocol v2 src: %V %d, dst: %V %d",
> - &pp->src_addr, pp->src_port, &pp->dst_addr, pp->dst_port);
> -
> if (buf < end) {
> pp->tlvs.data = ngx_pnalloc(c->pool, end - buf);
> if (pp->tlvs.data == NULL) {
> @@ -479,6 +544,10 @@ ngx_proxy_protocol_v2_read(ngx_connectio
>
> c->proxy_protocol = pp;
>
> +#if (NGX_DEBUG)
> + ngx_proxy_protocol_debug(c, 2);
> +#endif
> +
> return end;
> }
>
> 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
> @@ -18,10 +18,12 @@
>
>
> struct ngx_proxy_protocol_s {
> - ngx_str_t src_addr;
> - ngx_str_t dst_addr;
> - in_port_t src_port;
> - in_port_t dst_port;
> + struct sockaddr *src_sockaddr;
> + struct sockaddr *dst_sockaddr;
> +
> + socklen_t src_socklen;
> + socklen_t dst_socklen;
> +
> ngx_str_t tlvs;
> };
>
> diff --git a/src/http/modules/ngx_http_realip_module.c
> b/src/http/modules/ngx_http_realip_module.c
> --- a/src/http/modules/ngx_http_realip_module.c
> +++ b/src/http/modules/ngx_http_realip_module.c
> @@ -152,6 +152,8 @@ ngx_http_realip_handler(ngx_http_request
> return NGX_DECLINED;
> }
>
> + c = r->connection;
> +
> switch (rlcf->type) {
>
> case NGX_HTTP_REALIP_XREALIP:
> @@ -179,14 +181,18 @@ ngx_http_realip_handler(ngx_http_request
>
> case NGX_HTTP_REALIP_PROXY:
>
> - if (r->connection->proxy_protocol == NULL) {
> + if (c->proxy_protocol == NULL) {
> return NGX_DECLINED;
> }
>
> - value = &r->connection->proxy_protocol->src_addr;
> - xfwd = NULL;
> + if (ngx_cidr_match(c->sockaddr, rlcf->from) != NGX_OK) {
> + return NGX_DECLINED;
> + }
>
> - break;
> + addr.sockaddr = c->proxy_protocol->src_sockaddr;
> + addr.socklen = c->proxy_protocol->src_socklen;
> +
> + return ngx_http_realip_set_addr(r, &addr);
>
> default: /* NGX_HTTP_REALIP_HEADER */
>
> @@ -225,8 +231,6 @@ ngx_http_realip_handler(ngx_http_request
>
> found:
>
> - c = r->connection;
> -
> addr.sockaddr = c->sockaddr;
> addr.socklen = c->socklen;
> /* addr.name = c->addr_text; */
> @@ -235,10 +239,6 @@ found:
> rlcf->recursive)
> != NGX_DECLINED)
> {
> - if (rlcf->type == NGX_HTTP_REALIP_PROXY) {
> - ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->src_port);
> - }
> -
> return ngx_http_realip_set_addr(r, &addr);
> }
>
> diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c
> --- a/src/http/ngx_http_variables.c
> +++ b/src/http/ngx_http_variables.c
> @@ -201,20 +201,16 @@ static ngx_http_variable_t ngx_http_cor
> { ngx_string("remote_port"), NULL, ngx_http_variable_remote_port, 0, 0,
> 0 },
>
> { ngx_string("proxy_protocol_addr"), NULL,
> - ngx_http_variable_proxy_protocol_addr,
> - offsetof(ngx_proxy_protocol_t, src_addr), 0, 0 },
> + ngx_http_variable_proxy_protocol_addr, 0, 0, 0 },
>
> { ngx_string("proxy_protocol_port"), NULL,
> - ngx_http_variable_proxy_protocol_port,
> - offsetof(ngx_proxy_protocol_t, src_port), 0, 0 },
> + ngx_http_variable_proxy_protocol_port, 0, 0, 0 },
>
> { ngx_string("proxy_protocol_server_addr"), NULL,
> - ngx_http_variable_proxy_protocol_addr,
> - offsetof(ngx_proxy_protocol_t, dst_addr), 0, 0 },
> + ngx_http_variable_proxy_protocol_addr, 1, 0, 0 },
>
> { ngx_string("proxy_protocol_server_port"), NULL,
> - ngx_http_variable_proxy_protocol_port,
> - offsetof(ngx_proxy_protocol_t, dst_port), 0, 0 },
> + ngx_http_variable_proxy_protocol_port, 1, 0, 0 },
>
> { ngx_string("proxy_protocol_tlv_"), NULL,
> ngx_http_variable_proxy_protocol_tlv,
> @@ -1340,7 +1336,8 @@ static ngx_int_t
> ngx_http_variable_proxy_protocol_addr(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data)
> {
> - ngx_str_t *addr;
> + u_char *p;
> + size_t n;
> ngx_proxy_protocol_t *pp;
>
> pp = r->connection->proxy_protocol;
> @@ -1349,13 +1346,25 @@ ngx_http_variable_proxy_protocol_addr(ng
> return NGX_OK;
> }
>
> - addr = (ngx_str_t *) ((char *) pp + data);
> -
> - v->len = addr->len;
> + p = ngx_pnalloc(r->connection->pool, NGX_SOCKADDR_STRLEN);
NGX_SOCKADDR_STRLEN includes UNIX-domain sockets as well, which means
~100 bytes memory over-allocation, with such addresses prohibited in
the PROXY protocol. It can be saved if allocate the actual size
returned from ngx_sock_ntop().
> + if (p == NULL) {
> + return NGX_ERROR;
> + }
> +
> + if (data) {
> + n = ngx_sock_ntop(pp->dst_sockaddr, pp->dst_socklen,
> + p, NGX_SOCKADDR_STRLEN, 0);
> +
> + } else {
> + n = ngx_sock_ntop(pp->src_sockaddr, pp->src_socklen,
> + p, NGX_SOCKADDR_STRLEN, 0);
> + }
> +
> + v->len = n;
> v->valid = 1;
> v->no_cacheable = 0;
> v->not_found = 0;
> - v->data = addr->data;
> + v->data = p;
>
> return NGX_OK;
> }
> @@ -1384,7 +1393,12 @@ ngx_http_variable_proxy_protocol_port(ng
> return NGX_ERROR;
> }
>
> - port = *(in_port_t *) ((char *) pp + data);
> + if (data) {
> + port = ngx_inet_get_port(pp->dst_sockaddr);
> +
> + } else {
> + port = ngx_inet_get_port(pp->src_sockaddr);
> + }
>
> if (port > 0 && port < 65536) {
> v->len = ngx_sprintf(v->data, "%ui", port) - v->data;
What I don't get is why we parse zero port as valid (see ngx_atoi()
call in ngx_proxy_protocol_read_port()) but reject evaluating it as
such in variable handlers. IMHO, it makes sense to remove this
restriction and write the port value unconditionally, including "0",
especially that after storing port as binary it cannot be invalid.
> diff --git a/src/mail/ngx_mail_auth_http_module.c
> b/src/mail/ngx_mail_auth_http_module.c
> --- a/src/mail/ngx_mail_auth_http_module.c
> +++ b/src/mail/ngx_mail_auth_http_module.c
> @@ -1249,11 +1249,11 @@ ngx_mail_auth_http_create_request(ngx_ma
>
> if (c->proxy_protocol) {
> len += sizeof("Proxy-Protocol-Addr: ") - 1
> - + c->proxy_protocol->src_addr.len + sizeof(CRLF) - 1
> + + NGX_SOCKADDR_STRLEN + sizeof(CRLF) - 1
See above about wasted memory.
> + sizeof("Proxy-Protocol-Port: ") - 1
> + sizeof("65535") - 1 + sizeof(CRLF) - 1
> + sizeof("Proxy-Protocol-Server-Addr: ") - 1
> - + c->proxy_protocol->dst_addr.len + sizeof(CRLF) - 1
> + + NGX_SOCKADDR_STRLEN + sizeof(CRLF) - 1
> + sizeof("Proxy-Protocol-Server-Port: ") - 1
> + sizeof("65535") - 1 + sizeof(CRLF) - 1;
> }
> @@ -1352,21 +1352,23 @@ ngx_mail_auth_http_create_request(ngx_ma
> if (c->proxy_protocol) {
> b->last = ngx_cpymem(b->last, "Proxy-Protocol-Addr: ",
> sizeof("Proxy-Protocol-Addr: ") - 1);
> - b->last = ngx_copy(b->last, c->proxy_protocol->src_addr.data,
> - c->proxy_protocol->src_addr.len);
> + b->last += ngx_sock_ntop(c->proxy_protocol->src_sockaddr,
> + c->proxy_protocol->src_socklen,
> + b->last, b->end - b->last, 0);
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_sprintf(b->last, "Proxy-Protocol-Port: %d" CRLF,
> - c->proxy_protocol->src_port);
> + (int)
> ngx_inet_get_port(c->proxy_protocol->src_sockaddr));
>
> b->last = ngx_cpymem(b->last, "Proxy-Protocol-Server-Addr: ",
> sizeof("Proxy-Protocol-Server-Addr: ") - 1);
> - b->last = ngx_copy(b->last, c->proxy_protocol->dst_addr.data,
> - c->proxy_protocol->dst_addr.len);
> + b->last += ngx_sock_ntop(c->proxy_protocol->dst_sockaddr,
> + c->proxy_protocol->dst_socklen,
> + b->last, b->end - b->last, 0);
> *b->last++ = CR; *b->last++ = LF;
>
> b->last = ngx_sprintf(b->last, "Proxy-Protocol-Server-Port: %d" CRLF,
> - c->proxy_protocol->dst_port);
> + (int)
> ngx_inet_get_port(c->proxy_protocol->dst_sockaddr));
> }
>
> if (s->auth_method == NGX_MAIL_AUTH_NONE) {
> diff --git a/src/mail/ngx_mail_realip_module.c
> b/src/mail/ngx_mail_realip_module.c
> --- a/src/mail/ngx_mail_realip_module.c
> +++ b/src/mail/ngx_mail_realip_module.c
> @@ -87,14 +87,8 @@ ngx_mail_realip_handler(ngx_mail_session
> return NGX_OK;
> }
>
> - if (ngx_parse_addr(c->pool, &addr, c->proxy_protocol->src_addr.data,
> - c->proxy_protocol->src_addr.len)
> - != NGX_OK)
> - {
> - return NGX_OK;
> - }
> -
> - ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->src_port);
> + addr.sockaddr = c->proxy_protocol->src_sockaddr;
> + addr.socklen = c->proxy_protocol->src_socklen;
>
> return ngx_mail_realip_set_addr(s, &addr);
> }
> diff --git a/src/stream/ngx_stream_realip_module.c
> b/src/stream/ngx_stream_realip_module.c
> --- a/src/stream/ngx_stream_realip_module.c
> +++ b/src/stream/ngx_stream_realip_module.c
> @@ -116,14 +116,8 @@ ngx_stream_realip_handler(ngx_stream_ses
> return NGX_DECLINED;
> }
>
> - if (ngx_parse_addr(c->pool, &addr, c->proxy_protocol->src_addr.data,
> - c->proxy_protocol->src_addr.len)
> - != NGX_OK)
> - {
> - return NGX_DECLINED;
> - }
> -
> - ngx_inet_set_port(addr.sockaddr, c->proxy_protocol->src_port);
> + addr.sockaddr = c->proxy_protocol->src_sockaddr;
> + addr.socklen = c->proxy_protocol->src_socklen;
>
> return ngx_stream_realip_set_addr(s, &addr);
> }
> diff --git a/src/stream/ngx_stream_variables.c
> b/src/stream/ngx_stream_variables.c
> --- a/src/stream/ngx_stream_variables.c
> +++ b/src/stream/ngx_stream_variables.c
> @@ -66,20 +66,16 @@ static ngx_stream_variable_t ngx_stream
> ngx_stream_variable_remote_port, 0, 0, 0 },
>
> { ngx_string("proxy_protocol_addr"), NULL,
> - ngx_stream_variable_proxy_protocol_addr,
> - offsetof(ngx_proxy_protocol_t, src_addr), 0, 0 },
> + ngx_stream_variable_proxy_protocol_addr, 0, 0, 0 },
>
> { ngx_string("proxy_protocol_port"), NULL,
> - ngx_stream_variable_proxy_protocol_port,
> - offsetof(ngx_proxy_protocol_t, src_port), 0, 0 },
> + ngx_stream_variable_proxy_protocol_port, 0, 0, 0 },
>
> { ngx_string("proxy_protocol_server_addr"), NULL,
> - ngx_stream_variable_proxy_protocol_addr,
> - offsetof(ngx_proxy_protocol_t, dst_addr), 0, 0 },
> + ngx_stream_variable_proxy_protocol_addr, 1, 0, 0 },
>
> { ngx_string("proxy_protocol_server_port"), NULL,
> - ngx_stream_variable_proxy_protocol_port,
> - offsetof(ngx_proxy_protocol_t, dst_port), 0, 0 },
> + ngx_stream_variable_proxy_protocol_port, 1, 0, 0 },
>
> { ngx_string("proxy_protocol_tlv_"), NULL,
> ngx_stream_variable_proxy_protocol_tlv,
> @@ -573,7 +569,8 @@ static ngx_int_t
> ngx_stream_variable_proxy_protocol_addr(ngx_stream_session_t *s,
> ngx_stream_variable_value_t *v, uintptr_t data)
> {
> - ngx_str_t *addr;
> + u_char *p;
> + size_t n;
> ngx_proxy_protocol_t *pp;
>
> pp = s->connection->proxy_protocol;
> @@ -582,13 +579,25 @@ ngx_stream_variable_proxy_protocol_addr(
> return NGX_OK;
> }
>
> - addr = (ngx_str_t *) ((char *) pp + data);
> + p = ngx_pnalloc(s->connection->pool, NGX_SOCKADDR_STRLEN);
> + if (p == NULL) {
> + return NGX_ERROR;
> + }
>
> - v->len = addr->len;
> + if (data) {
> + n = ngx_sock_ntop(pp->dst_sockaddr, pp->dst_socklen,
> + p, NGX_SOCKADDR_STRLEN, 0);
> +
> + } else {
> + n = ngx_sock_ntop(pp->src_sockaddr, pp->src_socklen,
> + p, NGX_SOCKADDR_STRLEN, 0);
> + }
> +
> + v->len = n;
> v->valid = 1;
> v->no_cacheable = 0;
> v->not_found = 0;
> - v->data = addr->data;
> + v->data = p;
>
> return NGX_OK;
> }
> @@ -617,7 +626,12 @@ ngx_stream_variable_proxy_protocol_port(
> return NGX_ERROR;
> }
>
> - port = *(in_port_t *) ((char *) pp + data);
> + if (data) {
> + port = ngx_inet_get_port(pp->dst_sockaddr);
> +
> + } else {
> + port = ngx_inet_get_port(pp->src_sockaddr);
> + }
>
> if (port > 0 && port < 65536) {
> v->len = ngx_sprintf(v->data, "%ui", port) - v->data;
> # HG changeset patch
> # User Roman Arutyunyan <a...@nginx.com>
> # Date 1710081227 -14400
> # Sun Mar 10 18:33:47 2024 +0400
> # Node ID 6957c271a15c789be51d042de6835ce02141fe58
> # Parent 07c8c445746c6c74250bad7b42cd26dbb0ed6dd3
> Pass PROXY protocol header from client to upstream (ticket #2010).
>
> When creating a PROXY protocol header in mail or stream, server address is
> taken from c->local_sockaddr, which is normally the real server address of the
> connection. If prior to that client address was substituted by the realip
> module, then client and server addresses can have different address families,
> which breaks PROXY protocol syntax. The change fixes this by using server
> address from client PROXY protocol header as well.
>
> 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
> @@ -375,11 +375,24 @@ ngx_proxy_protocol_write(ngx_connection_
>
> *buf++ = ' ';
>
The above ngx_connection_local_sockaddr() call is not needed now if
server address is obtained from the saved PROXY header, it can be put
under conditional.
> - buf += ngx_sock_ntop(c->local_sockaddr, c->local_socklen, buf, last -
> buf,
> - 0);
> + if (c->proxy_protocol
> + && ngx_cmp_sockaddr(c->proxy_protocol->src_sockaddr,
> + c->proxy_protocol->src_socklen,
> + c->sockaddr, c->socklen, 1)
> + == NGX_OK)
This relies on a weak assumption that matching client addresses
is the result of client address substitution by the realip module.
This allows to substitute server address in PROXY writer to a
client chosen value regardless of address replacement being allowed
with set_real_ip_from.
(That's why an explicit hint from realip might be beneficial.)
> + {
> + buf += ngx_sock_ntop(c->proxy_protocol->dst_sockaddr,
> + c->proxy_protocol->dst_socklen,
> + buf, last - buf, 0);
I'd add a blank line
> + lport = ngx_inet_get_port(c->proxy_protocol->dst_sockaddr);
> +
> + } else {
> + buf += ngx_sock_ntop(c->local_sockaddr, c->local_socklen,
> + buf, last - buf, 0);
As ngx_connection_local_sockaddr() calls ngx_sock_ntop() internally,
the result could be re-used here, to get rid of the 2nd explicit
ngx_sock_ntop() call (with all its implied overhead).
> + lport = ngx_inet_get_port(c->local_sockaddr);
> + }
>
> port = ngx_inet_get_port(c->sockaddr);
> - lport = ngx_inet_get_port(c->local_sockaddr);
>
> return ngx_slprintf(buf, last, " %ui %ui" CRLF, port, lport);
> }
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
