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.
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.
Such format is supported in various nginx modules and may be enabled
by "listen ... ipv6only=off" to accept IPv4-mapped IPv6 addresses on a
wildcard listen socket.
For example, if nginx receives "PROXY TCP6" on "listen 127.0.0.1:8081"
and is configured to send PROXY protocol further in the stream proxy
chain, the server address will be updated to "::ffff:127.0.0.1 8081".
And opposite, receiving "PROXY TCP4" on "listen [::]:8082 ipv6only=off"
will update the PROXY header server part to "127.0.0.1 8082".
That said, something like this should work on top off your change:
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,12 +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)
{
- socklen_t local_socklen;
- ngx_uint_t port, lport;
- struct sockaddr *local_sockaddr;
- struct sockaddr_in sin;
+ socklen_t local_socklen;
+ ngx_uint_t port, lport;
+ ngx_sockaddr_t sa;
+ struct sockaddr *local_sockaddr;
#if (NGX_HAVE_INET6)
- struct sockaddr_in6 sin6;
+ in_addr_t addr4;
+ struct in6_addr *addr6;
#endif
if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
@@ -323,22 +324,62 @@ ngx_proxy_protocol_write(ngx_connection_
local_socklen = c->local_socklen;
} else {
+ local_sockaddr = &sa.sockaddr;
+
+ ngx_memzero(&sa, sizeof(ngx_sockaddr_t));
+ sa.sockaddr.sa_family = c->sockaddr->sa_family;
+ ngx_inet_set_port(&sa.sockaddr, ngx_inet_get_port(c->local_sockaddr));
+
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);
+
+ if (c->local_sockaddr->sa_family == AF_INET) {
+ addr4 = ntohl(((struct sockaddr_in *) c->local_sockaddr)
+ ->sin_addr.s_addr);
+
+ addr6 = &sa.sockaddr_in6.sin6_addr;
+ addr6->s6_addr[10] = 0xff;
+ addr6->s6_addr[11] = 0xff;
+ addr6->s6_addr[12] = addr4 >> 24;
+ addr6->s6_addr[13] = addr4 >> 16;
+ addr6->s6_addr[14] = addr4 >> 8;
+ addr6->s6_addr[15] = addr4;
+
+ } else {
+ sa.sockaddr_in6.sin6_addr = in6addr_any;
+ }
+
break;
#endif
default: /* AF_INET */
- ngx_memzero(&sin, sizeof(struct sockaddr));
- sin.sin_family = AF_INET;
- local_sockaddr = (struct sockaddr *) &sin;
local_socklen = sizeof(struct sockaddr_in);
+
+#if (NGX_HAVE_INET6)
+ if (c->local_sockaddr->sa_family == AF_INET6) {
+ addr6 = &((struct sockaddr_in6 *)
c->local_sockaddr)->sin6_addr;
+
+ if (IN6_IS_ADDR_V4MAPPED(addr6)) {
+ addr4 = addr6->s6_addr[12] << 24;
+ addr4 += addr6->s6_addr[13] << 16;
+ addr4 += addr6->s6_addr[14] << 8;
+ addr4 += addr6->s6_addr[15];
+
+ sa.sockaddr_in.sin_addr.s_addr = htonl(addr4);
+
+ } else {
+ sa.sockaddr_in.sin_addr.s_addr = INADDR_ANY;
+ }
+
+ } else
+#endif
+ {
+ sa.sockaddr_in.sin_addr.s_addr = INADDR_ANY;
+ }
+
break;
}
}
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
