Re: PROXY protocol support

On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Wed, 2021-09-08 at 18:51 +0000, Jacob Champion wrote:
> > I still owe you that overall review. Hoping to get to it this week.
>
> And here it is. I focused on things other than UnwrapProxyConnection()
> for this round, since I think that piece is looking solid.

Thanks!

> > + if (port->isProxy)
> > + {
> > + ereport(LOG,
> > + (errcode_for_socket_access(),
> > + errmsg("Ident authentication cannot be used over PROXY connections")));
>
> What are the rules on COMMERROR vs LOG when dealing with authentication
> code? I always thought COMMERROR was required, but I see now that LOG
> (among others) is suppressed to the client during authentication.

I honestly don't know :) In this case, LOG is what's used for all the
other message in errors in ident_inet(), so I picked it for
consistency.

> > #ifdef USE_SSL
> > /* No SSL when disabled or on Unix sockets */
> > - if (!LoadedSSL || IS_AF_UNIX(port->laddr.addr.ss_family))
> > + if (!LoadedSSL || (IS_AF_UNIX(port->laddr.addr.ss_family) && !port->isProxy))
> > SSLok = 'N';
> > else
> > SSLok = 'S'; /* Support for SSL */
> > @@ -2087,7 +2414,7 @@ retry1:
> >
> > #ifdef ENABLE_GSS
> > /* No GSSAPI encryption when on Unix socket */
> > - if (!IS_AF_UNIX(port->laddr.addr.ss_family))
> > + if (!IS_AF_UNIX(port->laddr.addr.ss_family) || port->isProxy)
> > GSSok = 'G';
>
> Now that we have port->daddr, could these checks be simplified to just
> IS_AF_UNIX(port->daddr...)? Or is there a corner case I'm missing for
> the port->isProxy case?

Yeah, I think they could.

> > + * Note: AuthenticationTimeout is applied here while waiting for the
> > + * startup packet, and then again in InitPostgres for the duration of any
> > + * authentication operations. So a hostile client could tie up the
> > + * process for nearly twice AuthenticationTimeout before we kick him off.
>
> This comment needs to be adjusted after the move; waiting for the
> startup packet comes later, and it looks like we can now tie up 3x the
> timeout for the proxy case.

Good point.

> > + /* Check if this is a proxy connection and if so unwrap the proxying */
> > + if (port->isProxy)
> > + {
> > + enable_timeout_after(STARTUP_PACKET_TIMEOUT, AuthenticationTimeout * 1000);
> > + if (UnwrapProxyConnection(port) != STATUS_OK)
> > + proc_exit(0);
>
> I think the timeout here could comfortably be substantially less than
> the overall authentication timeout, since the proxy should send its
> header immediately even if the client takes its time with the startup
> packet. The spec suggests allowing 3 seconds minimum to cover a
> retransmission. Maybe something to tune in the future?

Maybe. I'll leave it with a new comment for now about us diong it, and
that we may want to consider igt in the future.

>
> > + /* Also listen to the PROXY port on this address, if configured */
> > + if (ProxyPortNumber)
> > + {
> > + if (strcmp(curhost, "*") == 0)
> > + socket = StreamServerPort(AF_UNSPEC, NULL,
> > + (unsigned short) ProxyPortNumber,
> > + NULL,
> > + ListenSocket, MAXLISTEN);
>
> Sorry if you already talked about this upthread somewhere, but it looks
> like another downside of treating "proxy mode" as a server-wide on/off
> switch is that it cuts the effective MAXLISTEN in half, from 64 to 32,
> since we're opening two sockets for every address. If I've understood
> that correctly, it might be worth mentioning in the docs.

Correct. I don't see a way to avoid that without complicating things
(as long as we want the ports to be separate), but I also don't see it
as something that's reality to be an issue in reality.

I would agree with documenting it, but I can't actually find us
documenting the MAXLISTEN value anywhere. Do we?

> > - if (!success && elemlist != NIL)
> > + if (socket == NULL && elemlist != NIL)
> > ereport(FATAL,
> > (errmsg("could not create any TCP/IP sockets")));
>
> With this change in PostmasterMain, it looks like `success` is no
> longer a useful variable. But I'm not convinced that this is the
> correct logic -- this is just checking to see if the last socket
> creation succeeded, as opposed to seeing if any of them succeeded. Is
> that what you intended?

Eh, no, that's clearly a code-moving-bug.

I think the reasonable thing is to succeed if we create either a
regular socket *or* a proxy one, but FATAL out if you configured
either of them but we failed co create any.

> > +plan tests => 25;
> > +
> > +my $node = get_new_node('node');
>
> The TAP test will need to be rebased over the changes in 201a76183e.

Done, and adjustments above according to your comments, along with a
small docs fix "a proxy connection is done" -> "a proxy connection is
made".

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/