Re: PROXY protocol support

On Fri, 2021-03-05 at 10:22 +0100, Magnus Hagander wrote:
> On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> > A small nitpick on the current separate-port PoC is that I'm forced to
> > set up a "regular" TCP port, even if I only want the PROXY behavior.
>
> Yeah. I'm not sure there's a good way to avoid that without making
> configuations a lot more complex.

A generic solution would also solve the "I want to listen on more than
one port" problem, but that's probably not something to tackle at the
same time.

> > The original-host logging isn't working for me:
> >
> > [...]
>
> That's interesting -- it works perfectly fine here. What platform are
> you testing on?

Ubuntu 20.04.

> But yes, you are correct, it should do that. I guess it's a case of
> the salen actually ending up being uninitialized in the copy, and thus
> failing at a later stage.

That seems right; EAI_FAMILY can be returned for a mismatched addrlen.

> (I sent for sizeof(SockAddr) to make it
> easier to read without having to look things up, but the net result is
> the same)

Cool. Did you mean to attach a patch?

== More Notes ==

(Stop me if I'm digging too far into a proof of concept patch.)

> + proxyaddrlen = pg_ntoh16(proxyheader.len);
> +
> + if (proxyaddrlen > sizeof(proxyaddr))
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("oversized proxy packet")));
> + return STATUS_ERROR;
> + }

I think this is not quite right -- if there's additional data beyond
the IPv6 header size, that just means there are TLVs tacked onto the
header that we should ignore. (Or, eventually, use.)

Additionally, we need to check for underflow as well. A misbehaving
proxy might not send enough data to fill up the address block for the
address family in use.

> + /* If there is any more header data present, skip past it */
> + if (proxyaddrlen > sizeof(proxyaddr))
> + pq_discardbytes(proxyaddrlen - sizeof(proxyaddr));

This looks like dead code, given that we'll error out for the same
check above -- but once it's no longer dead code, the return value of
pq_discardbytes should be checked for EOF.

> + else if (proxyheader.fam == 0x11)
> + {
> + /* TCPv4 */
> + port->raddr.addr.ss_family = AF_INET;
> + port->raddr.salen = sizeof(struct sockaddr_in);
> + ((struct sockaddr_in *) &port->raddr.addr)->sin_addr.s_addr = proxyaddr.ip4.src_addr;
> + ((struct sockaddr_in *) &port->raddr.addr)->sin_port = proxyaddr.ip4.src_port;
> + }

I'm trying to reason through the fallout of setting raddr and not
laddr. I understand why we're not setting laddr -- several places in
the code rely on the laddr to actually refer to a machine-local address
-- but the fact that there is no actual connection from raddr to laddr
could cause shenanigans. For example, the ident auth protocol will just
break (and it might be nice to explicitly disable it for PROXY
connections). Are there any other situations where a "faked" raddr
could throw off Postgres internals?

--Jacob