Re: PROXY protocol support

On Sat, Mar 6, 2021 at 4:17 PM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> On Fri, Mar 5, 2021 at 8:11 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> >
> > 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:
> > > > 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.
>
> Curious. It doesn't show up on my debian.
>
> But either way -- it was clearly wrong :)
>
>
> > > (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?
>
> I didn't, I had some other hacks that were broken :) I've attached one
> now which includes those changes.
>
>
> > == More Notes ==
> >
> > (Stop me if I'm digging too far into a proof of concept patch.)
>
> Definitely not -- much appreciated, and just what was needed to take
> it from poc to a proper one!
>
>
> > > + 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.)
>
> Yeah, you're right. Fallout of too much moving around. I think inthe
> end that code should just be removed, in favor of the discard path as
> you mentinoed below.
>
>
> > 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.
>
> I used to have that check. I seem to have lost it in restructuring. Added back!
>
>
> > > + /* 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.
>
> Yup.
>
>
> > > + 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?
>
> That's a good point to discuss. I thought about it initially and
> figured it'd be even worse to actually copy over laddr since that
> woudl then suddenly have the IP address belonging to a different
> machine.. And then I forgot to enumerate the other cases.
>
> For ident, disabling the method seems reasonable.
>
> Another thing that shows up with added support for running the proxy
> protocol over Unix sockets, is that PostgreSQL refuses to do SSL over
> Unix sockets. So that check has to be updated to allow it over proxy
> connections. Same for GSSAPI.
>
> An interesting thing is what to do about
> inet_server_addr/inet_server_port. That sort of loops back up to the
> original question of where/how to expose the information about the
> proxy in general (since right now it just logs). Right now you can
> actually use inet_server_port() to see if the connection was proxied
> (as long as it was over tcp).
>
> Attached is an updated, which covers your comments, as well as adds
> unix socket support (per your question and Alvaros confirmed usecase).
> It allows proxy connections over unix sockets, but I saw no need to
> get into unix sockets over the proxy protocol (dealing with paths
> between machines etc).
>
> I changed the additional ListenSocket array to instead declare
> ListenSocket as an array of structs holding two fields. Seems cleaner,
> and especially should there be further extensions needed in the
> future.
>
> I've also added some trivial tests (man that took an ungodly amount of
> fighting perl -- it's clearly been a long time since I used perl
> properly). They probably need some more love but it's a start.
>
> And of course rebased.

Pfft, I was hoping for cfbot to pick it up and test it on a different
platform. Of course, for it to do that, I need to include the test
directory in the Makefile. Here's a new one which adds that, no other
changes.

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