Re: PROXY protocol support

On Wed, Jul 14, 2021 at 8:24 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Mon, 2021-07-12 at 18:28 +0200, Magnus Hagander wrote:
> > Yeah, I have no problem being stricter than necessary, unless that
> > actually causes any interop problems. It's a lot worse to not be
> > strict enough..
>
> Agreed. Haven't heard back from the HAProxy mailing list yet, so
> staying strict seems reasonable in the meantime. That could always be
> rolled back later.

Any further feedback from them now, two months later? :)

(Sorry, I was out on vacation for the end of the last CF, so didn't
get around to this one, but it seemed there'd be plenty of time in
this CF)

> > > I've been thinking more about your earlier comment:
> > >
> > > > 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).
> > >
> > > IMO these should return the "virtual" dst_addr/port, instead of
> > > exposing the physical connection information to the client. That way,
> > > if you intend for your proxy to be transparent, you're not advertising
> > > your network internals to connected clients. It also means that clients
> > > can reasonably expect to be able to reconnect to the addr:port that we
> > > give them, and prevents confusion if the proxy is using an address
> > > family that the client doesn't even support (e.g. the client is IPv4-
> > > only but the proxy connects via IPv6).
> >
> > That reasoning I think makes a lot of sense, especially with the
> > comment about being able to connect back to it.
> >
> > The question at that point extends to, would we also add extra
> > functions to get the data on the proxy connection itself? Maybe add a
> > inet_proxy_addr()/inet_proxy_port()? Better names?
>
> What's the intended use case? I have trouble viewing those as anything
> but information disclosure vectors, but I'm jaded. :)

"Covering all the bases"?

I'm not entirely sure what the point is of the *existing* functions
for that though, so I'm definitely not wedded to including it.

> If the goal is to give a last-ditch debugging tool to someone whose
> proxy isn't behaving properly -- though I'd hope the proxy in question
> has its own ways to debug its behavior -- maybe they could be locked
> behind one of the pg_monitor roles, so that they're only available to
> someone who could get that information anyway?

Yeah, agreed.

> > PFA a patch that fixes the above errors, and changes
> > inet_server_addr()/inet_server_port(). Does not yet add anything to
> > receive the actual local port in this case.
>
> Looking good in local testing. I'm going to reread the spec with fresh
> eyes and do a full review pass, but this is shaping up nicely IMO.

Thanks!

> Something that I haven't thought about very hard yet is proxy
> authentication, but I think the simple IP authentication will be enough
> for a first version. For the Unix socket case, it looks like anyone
> currently relying on peer auth will need to switch to a
> unix_socket_group/_permissions model. For now, that sounds like a
> reasonable v1 restriction, though I think not being able to set the
> proxy socket's permissions separately from the "normal" one might lead
> to some complications in more advanced setups.

Agreed in principle, but I think those are some quite uncommon
usecases, so definitely something we don't need to cover in a first
feature.

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