Re: PROXY protocol support - Mailing list pgsql-hackers

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.

> +       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.

>  #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?

> +    * 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.

> +       /* 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?

> +                       /* 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.

> -               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?

> +plan tests => 25;
> +
> +my $node = get_new_node('node');

The TAP test will need to be rebased over the changes in 201a76183e.

--Jacob