Hi,
On Wed, Jan 24, 2024 at 12:03:06AM +0300, Maxim Dounin wrote:
> Hello!
>
> On Mon, Jan 22, 2024 at 07:48:01PM +0400, Roman Arutyunyan wrote:
>
> > Hi,
> >
> > On Mon, Jan 22, 2024 at 02:59:21PM +0300, Maxim Dounin wrote:
> > > Hello!
> > >
> > > On Mon, Jan 22, 2024 at 02:49:54PM +0400, Roman Arutyunyan wrote:
> > >
> > > > # HG changeset patch
> > > > # User Roman Arutyunyan <a...@nginx.com>
> > > > # Date 1705916128 -14400
> > > > # Mon Jan 22 13:35:28 2024 +0400
> > > > # Node ID 2f12c929527b2337c15ef99d3a4dc97819b61fbd
> > > > # Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
> > > > Avoiding mixed socket families in PROXY protocol v1 (ticket #2594).
>
> Also nitpicking: ticket #2010 might be a better choice.
>
> The #2594 is actually a duplicate (with a side issue noted that
> using long unix socket path might result in a PROXY protocol
> header without ports and CRLF) and should be closed as such.
>
> > > >
> > > > When using realip module, remote and local addreses 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
> > > >
> > > > The PROXY protocol v1 specification does not allow mixed families. The
> > > > change
> > > > will generate the unknown PROXY protocol header in this case:
> > > >
> > > > PROXY UNKNOWN
> > > >
> > > > 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.
> > >
> > > Nitpicking: double space in "from PROXY".
> >
> > Yes, thanks.
> >
> > > This change will essentially disable use of PROXY protocol in such
> > > configurations. While it is probably good enough from formal
> > > point of view, and better that what we have now, this might still
> > > be a surprise, especially when multiple address families are used
> > > on the original proxy server, and the configuration works for some
> > > of them, but not for others.
> > >
> > > Wouldn't it be better to remember if the PROXY protocol was used
> > > to set the address, and use $proxy_protocol_server_addr /
> > > $proxy_protocol_server_port in this case?
> > >
> > > Alternatively, we can use some dummy server address instead, so
> > > the client address will be still sent.
> >
> > Another alternative is duplicating client address in this case, see patch.
>
> I don't think it is a good idea. Using some meaningful real
> address might easily mislead users. I would rather use a clearly
> dummy address instead, such as INADDR_ANY with port 0.
>
> 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.
I suggest that we follow the first plan (INADDR_ANY etc).
> [...]
Updated patch attached.
--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <a...@nginx.com>
# Date 1706083568 -14400
# Wed Jan 24 12:06:08 2024 +0400
# Node ID 49e4bdc883520924cbed992959c36c1099fdcfcf
# Parent c5e01070a53b9f2e867f25481c6d4862aac68b17
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
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,10 @@ 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;
+ static ngx_sockaddr_t default_sockaddr;
if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
ngx_log_error(NGX_LOG_ALERT, c->log, 0,
@@ -312,11 +315,21 @@ 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 {
+ default_sockaddr.sockaddr.sa_family = c->sockaddr->sa_family;
+
+ local_sockaddr = &default_sockaddr.sockaddr;
+ local_socklen = sizeof(ngx_sockaddr_t);
+ }
+
+ 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);
}
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
