Re: PROXY protocol support

On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion <pchampion@vmware.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/