Re: PROXY protocol support

On Tue, Mar 9, 2021 at 11:25 AM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sat, Mar 6, 2021 at 5:30 PM Magnus Hagander <magnus@hagander.net> wrote:
> >
> > On Sat, Mar 6, 2021 at 4:17 PM Magnus Hagander <magnus@hagander.net> wrote:
> > >
> > > On Fri, Mar 5, 2021 at 8:11 PM Jacob Champion <pchampion@vmware.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@vmware.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.
>
> So cfbot didn't like thato ne one bit. Turns out that it's not a great
> idea to hardcode the username "mha" in the tests :)
>
> And also changed to only use unix sockets for the tests on linux, and
> tcp only on windows. Because that's how our tests are supposed to be.

PFA a rebase to make cfbot happy.

There's another set or review notes from Jacob on March 11, that I
will also address, but it's not included in this version.

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