On Wed, Jul 14, 2021 at 8:24 PM Jacob Champion <pchamp...@vmware.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/
