PROXY protocol support

PFA a simple patch that implements support for the PROXY protocol.

This is a protocol common and very light weight in proxies and load
balancers (haproxy is one common example, but also for example the AWS
cloud load balancers). Basically this protocol prefixes the normal
connection with a header and a specification of what the original host
was, allowing the server to unwrap that and get the correct client
address instead of just the proxy ip address. It is a one-way protocol
in that there is no response from the server, it's just purely a
prefix of the IP information.

Using this when PostgreSQL is behind a proxy allows us to keep using
pg_hba.conf rules based on the original ip address, as well as track
the original address in log messages and pg_stat_activity etc.

The implementation adds a parameter named proxy_servers which lists
the ips or ip+cidr mask to be trusted. Since a proxy can decide what
the origin is, and this is used for security decisions, it's very
important to not just trust any server, only those that are
intentionally used. By default, no servers are listed, and thus the
protocol is disabled.

When specified, and the connection on the normal port has the proxy
prefix on it, and the connection comes in from one of the addresses
listed as valid proxy servers, we will replace the actual IP address
of the client with the one specified in the proxy packet.

Currently there is no information about the proxy server in the
pg_stat_activity view, it's only available as a log message. But maybe
it should go in pg_stat_activity as well? Or in a separate
pg_stat_proxy view?

(In passing, I note that pq_discardbytes were in pqcomm.h, yet listed
as static in pqcomm.c -- but now made non-static)

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

Hi,

On Tue, 2 Mar 2021 at 14:43, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> PFA a simple patch that implements support for the PROXY protocol.

Nice. I didn't know I needed this. But in hindsight, I would've used
it quite a few times in the past if I could have.

> The implementation adds a parameter named proxy_servers which lists
> the ips or ip+cidr mask to be trusted. Since a proxy can decide what
> the origin is, and this is used for security decisions, it's very
> important to not just trust any server, only those that are
> intentionally used. By default, no servers are listed, and thus the
> protocol is disabled.

Might make sense to add special cases for 'samehost' and 'samenet', as
in hba rules, as proxy servers are commonly on the same machine or
share one of the same internal networks.

Despite the security issues, I'm sure people will soon try and set
proxy_servers='*' or 'all' if they think this setting works as
listen_addresses or as pg_hba. But I don't think I'd make these use
cases easier.

Tureba - Arthur Nascimento

On Tue, 2021-03-02 at 18:43 +0100, Magnus Hagander wrote:
> PFA a simple patch that implements support for the PROXY protocol.

I'm not all the way through the patch yet, but this part jumped out at
me:

> + if (memcmp(proxyheader.sig, "\x0d\x0a\x0d\x0a\x00\x0d\x0a\x51\x55\x49\x54\x0a", sizeof(proxyheader.sig)) != 0)
> + {
> + /*
> + * Data is there but it wasn't a proxy header. Also fall through to
> + * normal processing
> + */
> + pq_endmsgread();
> + return STATUS_OK;

From my reading, the spec explicitly disallows this sort of fallback
behavior:

> The receiver MUST be configured to only receive the protocol described in this
> specification and MUST not try to guess whether the protocol header is present
> or not. This means that the protocol explicitly prevents port sharing between
> public and private access.

You might say, "if we already trust the proxy server, why should we
care?" but I think the point is that you want to catch
misconfigurations where the middlebox is forwarding bare TCP without
adding a PROXY header of its own, which will "work" for innocent
clients but in reality is a ticking timebomb. If you've decided to
trust an intermediary to use PROXY connections, then you must _only_
accept PROXY connections from that intermediary. Does that seem like a
reasonable interpretation?

--Jacob

On Wed, Mar 3, 2021 at 1:50 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Tue, 2021-03-02 at 18:43 +0100, Magnus Hagander wrote:
> > PFA a simple patch that implements support for the PROXY protocol.
>
> I'm not all the way through the patch yet, but this part jumped out at
> me:
>
> > + if (memcmp(proxyheader.sig, "\x0d\x0a\x0d\x0a\x00\x0d\x0a\x51\x55\x49\x54\x0a", sizeof(proxyheader.sig)) != 0)
> > + {
> > + /*
> > + * Data is there but it wasn't a proxy header. Also fall through to
> > + * normal processing
> > + */
> > + pq_endmsgread();
> > + return STATUS_OK;
>
> From my reading, the spec explicitly disallows this sort of fallback
> behavior:
>
> > The receiver MUST be configured to only receive the protocol described in this
> > specification and MUST not try to guess whether the protocol header is present
> > or not. This means that the protocol explicitly prevents port sharing between
> > public and private access.
>
> You might say, "if we already trust the proxy server, why should we
> care?" but I think the point is that you want to catch
> misconfigurations where the middlebox is forwarding bare TCP without
> adding a PROXY header of its own, which will "work" for innocent
> clients but in reality is a ticking timebomb. If you've decided to
> trust an intermediary to use PROXY connections, then you must _only_
> accept PROXY connections from that intermediary. Does that seem like a
> reasonable interpretation?

I definitely missed that part of the spec. Ugh.

That said, I'm not sure it's *actually* an issue in the case of
PostgreSQL. Given that doing what you're suggesting, accidentally
passing connections without PROXY, will get caught in pg_hba.conf.

That said, I agree with your interpretation, and it's pretty easy to
change it to that. Basically we just have to do the IP check *before*
doing the PROXY protocol check. It makes testing a bit more difficult
though, but maybe worth it?

I've attached a POC that does that. Note that I have *not* updated the docs!

Another option would of course be to listen on a separate port for it,
which seems to be the "haproxy way". That would be slightly more code
(we'd still want to keep the code for validating the list of trusted
proxies I'd say), but maybe worth doing?

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

On Wed, Mar 3, 2021 at 10:00 AM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> On Wed, Mar 3, 2021 at 1:50 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> >
> > On Tue, 2021-03-02 at 18:43 +0100, Magnus Hagander wrote:
> > > PFA a simple patch that implements support for the PROXY protocol.
> >
> > I'm not all the way through the patch yet, but this part jumped out at
> > me:
> >
> > > + if (memcmp(proxyheader.sig, "\x0d\x0a\x0d\x0a\x00\x0d\x0a\x51\x55\x49\x54\x0a", sizeof(proxyheader.sig)) != 0)
> > > + {
> > > + /*
> > > + * Data is there but it wasn't a proxy header. Also fall through to
> > > + * normal processing
> > > + */
> > > + pq_endmsgread();
> > > + return STATUS_OK;
> >
> > From my reading, the spec explicitly disallows this sort of fallback
> > behavior:
> >
> > > The receiver MUST be configured to only receive the protocol described in this
> > > specification and MUST not try to guess whether the protocol header is present
> > > or not. This means that the protocol explicitly prevents port sharing between
> > > public and private access.
> >
> > You might say, "if we already trust the proxy server, why should we
> > care?" but I think the point is that you want to catch
> > misconfigurations where the middlebox is forwarding bare TCP without
> > adding a PROXY header of its own, which will "work" for innocent
> > clients but in reality is a ticking timebomb. If you've decided to
> > trust an intermediary to use PROXY connections, then you must _only_
> > accept PROXY connections from that intermediary. Does that seem like a
> > reasonable interpretation?
>
> I definitely missed that part of the spec. Ugh.
>
> That said, I'm not sure it's *actually* an issue in the case of
> PostgreSQL. Given that doing what you're suggesting, accidentally
> passing connections without PROXY, will get caught in pg_hba.conf.
>
> That said, I agree with your interpretation, and it's pretty easy to
> change it to that. Basically we just have to do the IP check *before*
> doing the PROXY protocol check. It makes testing a bit more difficult
> though, but maybe worth it?
>
> I've attached a POC that does that. Note that I have *not* updated the docs!
>
> Another option would of course be to listen on a separate port for it,
> which seems to be the "haproxy way". That would be slightly more code
> (we'd still want to keep the code for validating the list of trusted
> proxies I'd say), but maybe worth doing?

In order to figure that out, I hacked up a poc on that. Once again
without updates to the docs, but shows approximately how much code
complexity it adds (not much).

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

+10 on this one!

Hosting a farm of read replicas and r/w endpoint behind an HAproxy makes
the powerful pg_hba purpose by hiding the real source address... which is
bad for some environments with strict conformance and audit requirements

Le mar. 2 mars 2021 à 12:43, Magnus Hagander <magnus(at)hagander(dot)net> a écrit :

> PFA a simple patch that implements support for the PROXY protocol.
>
> This is a protocol common and very light weight in proxies and load
> balancers (haproxy is one common example, but also for example the AWS
> cloud load balancers). Basically this protocol prefixes the normal
> connection with a header and a specification of what the original host
> was, allowing the server to unwrap that and get the correct client
> address instead of just the proxy ip address. It is a one-way protocol
> in that there is no response from the server, it's just purely a
> prefix of the IP information.
>
> Using this when PostgreSQL is behind a proxy allows us to keep using
> pg_hba.conf rules based on the original ip address, as well as track
> the original address in log messages and pg_stat_activity etc.
>
> The implementation adds a parameter named proxy_servers which lists
> the ips or ip+cidr mask to be trusted. Since a proxy can decide what
> the origin is, and this is used for security decisions, it's very
> important to not just trust any server, only those that are
> intentionally used. By default, no servers are listed, and thus the
> protocol is disabled.
>
> When specified, and the connection on the normal port has the proxy
> prefix on it, and the connection comes in from one of the addresses
> listed as valid proxy servers, we will replace the actual IP address
> of the client with the one specified in the proxy packet.
>
> Currently there is no information about the proxy server in the
> pg_stat_activity view, it's only available as a log message. But maybe
> it should go in pg_stat_activity as well? Or in a separate
> pg_stat_proxy view?
>
> (In passing, I note that pq_discardbytes were in pqcomm.h, yet listed
> as static in pqcomm.c -- but now made non-static)
>
> --
> Magnus Hagander
> Me: https://www.hagander.net/
> Work: https://www.redpill-linpro.com/
>

> PFA a simple patch that implements support for the PROXY protocol.
>
> This is a protocol common and very light weight in proxies and load
> balancers (haproxy is one common example, but also for example the AWS
> cloud load balancers). Basically this protocol prefixes the normal
> connection with a header and a specification of what the original host
> was, allowing the server to unwrap that and get the correct client
> address instead of just the proxy ip address. It is a one-way protocol
> in that there is no response from the server, it's just purely a
> prefix of the IP information.

Is there any formal specification for the "a protocol common and very
light weight in proxies"? I am asking because I was expecting that is
explained in your patch (hopefully in "Frontend/Backend Protocol"
chapter) but I couldn't find it in your patch.

Also we need a regression test for this feature.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
> Is there any formal specification for the "a protocol common and very
> light weight in proxies"?

See

https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

which is maintained by HAProxy Technologies.

--Jacob

On Wed, 2021-03-03 at 10:39 +0100, Magnus Hagander wrote:
> On Wed, Mar 3, 2021 at 10:00 AM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> > Another option would of course be to listen on a separate port for it,
> > which seems to be the "haproxy way". That would be slightly more code
> > (we'd still want to keep the code for validating the list of trusted
> > proxies I'd say), but maybe worth doing?
>
> In order to figure that out, I hacked up a poc on that. Once again
> without updates to the docs, but shows approximately how much code
> complexity it adds (not much).

From a configuration perspective, I like that the separate-port
approach can shift the burden of verifying trust to an external
firewall, and that it seems to match the behavior of other major server
software. But I don't have any insight into the relative security of
the two options in practice; hopefully someone else can chime in.

> memset((char *) &hints, 0, sizeof(hints));
> hints.ai_flags = AI_NUMERICHOST;
> hints.ai_family = AF_UNSPEC;
>
> ret = pg_getaddrinfo_all(tok, NULL, &hints, &gai_result);

Idle thought I had while setting up a local test rig: Are there any
compelling cases for allowing PROXY packets to arrive over Unix
sockets? (By which I mean, the proxy is running on the same machine as
Postgres, and connects to it using the .s.PGSQL socket file instead of
TCP.) Are there cases where you want some other software to interact
with the TCP stack instead of Postgres, but it'd still be nice to have
the original connection information available?

--Jacob

On 3/4/21 2:45 PM, Jacob Champion wrote:
> On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
>> Is there any formal specification for the "a protocol common and very
>> light weight in proxies"?
>
> See
>
> https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
>
> which is maintained by HAProxy Technologies.
>
> --Jacob
>

This looks like it would only need a few extra protocol messages to be
understood by the backend. It might be possible to implement that with
the loadable wire protocol extensions proposed here:

https://commitfest.postgresql.org/32/3018/

Regards, Jan

--
Jan Wieck
Principle Database Engineer
Amazon Web Services

On Thu, Mar 4, 2021 at 9:29 PM Jan Wieck <jan(at)wi3ck(dot)info> wrote:
>
> On 3/4/21 2:45 PM, Jacob Champion wrote:
> > On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
> >> Is there any formal specification for the "a protocol common and very
> >> light weight in proxies"?
> >
> > See
> >
> > https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
> >
> > which is maintained by HAProxy Technologies.
> >
> > --Jacob
> >
>
> This looks like it would only need a few extra protocol messages to be
> understood by the backend. It might be possible to implement that with
> the loadable wire protocol extensions proposed here:
>
> https://commitfest.postgresql.org/32/3018/

Actually the whole point of it is that it *doesn't* need any new
protocol messages. And that it *wraps* whatever is there, definitely
doesn't replace it. It should equally be wrapping whatever an
extension uses.

So while the base topic is not unrelated, I don't think there is any
overlap between these.

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

On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Wed, 2021-03-03 at 10:39 +0100, Magnus Hagander wrote:
> > On Wed, Mar 3, 2021 at 10:00 AM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> > > Another option would of course be to listen on a separate port for it,
> > > which seems to be the "haproxy way". That would be slightly more code
> > > (we'd still want to keep the code for validating the list of trusted
> > > proxies I'd say), but maybe worth doing?
> >
> > In order to figure that out, I hacked up a poc on that. Once again
> > without updates to the docs, but shows approximately how much code
> > complexity it adds (not much).
>
> From a configuration perspective, I like that the separate-port
> approach can shift the burden of verifying trust to an external
> firewall, and that it seems to match the behavior of other major server
> software. But I don't have any insight into the relative security of
> the two options in practice; hopefully someone else can chime in.

Yeah I think that and the argument that the spec explicitly says it
should be on it's own port is the advantage. The disadvantage is,
well, more ports and more configuration. But it does definitely make a
more clean separation of concerns.

> > memset((char *) &hints, 0, sizeof(hints));
> > hints.ai_flags = AI_NUMERICHOST;
> > hints.ai_family = AF_UNSPEC;
> >
> > ret = pg_getaddrinfo_all(tok, NULL, &hints, &gai_result);
>
> Idle thought I had while setting up a local test rig: Are there any
> compelling cases for allowing PROXY packets to arrive over Unix
> sockets? (By which I mean, the proxy is running on the same machine as
> Postgres, and connects to it using the .s.PGSQL socket file instead of
> TCP.) Are there cases where you want some other software to interact
> with the TCP stack instead of Postgres, but it'd still be nice to have
> the original connection information available?

I'm uncertain what that usecase would be for something like haproxy,
tbh. It can't do connection pooling, so adding it on the same machine
as postgres itself wouldn't really add anything, I think?

Iid think about the other end, if you had a proxy on a different
machine accepting unix connections and passing them on over
PROXY-over-tcp. But I doubt it's useful to know it was unix in that
case (since it still couldn't do peer or such for the auth) --
instead, that seems like an argument where it'd be better to proxy
without using PROXY and just letting the IP address be.

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

On Thu, Mar 4, 2021 at 8:45 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
> > Is there any formal specification for the "a protocol common and very
> > light weight in proxies"?
>
> See
>
> https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

Yeah, it's currently in one of the comments, but should probably be
added to the docs side as well.

And yes tests :) Probably not a regression test, but some level of tap
testing should definitely be added. We'll just have to find a way to
do that without making haproxy a dependency to run the tests :)

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

On 3/4/21 3:40 PM, Magnus Hagander wrote:
> On Thu, Mar 4, 2021 at 9:29 PM Jan Wieck <jan(at)wi3ck(dot)info> wrote:
>> This looks like it would only need a few extra protocol messages to be
>> understood by the backend. It might be possible to implement that with
>> the loadable wire protocol extensions proposed here:
>>
>> https://commitfest.postgresql.org/32/3018/
>
> Actually the whole point of it is that it *doesn't* need any new
> protocol messages. And that it *wraps* whatever is there, definitely
> doesn't replace it. It should equally be wrapping whatever an
> extension uses.
>
> So while the base topic is not unrelated, I don't think there is any
> overlap between these.

I might be missing something here, but isn't sending some extra,
informational *header*, which is understood by the backend, in essence a
protocol extension?

Regards, Jan

--
Jan Wieck
Principle Database Engineer
Amazon Web Services

On Thu, Mar 4, 2021 at 10:01 PM Jan Wieck <jan(at)wi3ck(dot)info> wrote:
>
> On 3/4/21 3:40 PM, Magnus Hagander wrote:
> > On Thu, Mar 4, 2021 at 9:29 PM Jan Wieck <jan(at)wi3ck(dot)info> wrote:
> >> This looks like it would only need a few extra protocol messages to be
> >> understood by the backend. It might be possible to implement that with
> >> the loadable wire protocol extensions proposed here:
> >>
> >> https://commitfest.postgresql.org/32/3018/
> >
> > Actually the whole point of it is that it *doesn't* need any new
> > protocol messages. And that it *wraps* whatever is there, definitely
> > doesn't replace it. It should equally be wrapping whatever an
> > extension uses.
> >
> > So while the base topic is not unrelated, I don't think there is any
> > overlap between these.
>
> I might be missing something here, but isn't sending some extra,
> informational *header*, which is understood by the backend, in essence a
> protocol extension?

Bad choice of words, I guess.

The points being, there is a single packet sent ahead of the normal
stream. There are no new messages in "the postgresql protocol" or "the
febe protocol" or whatever we call it. And it doesn't change the
properties of any part of that protocol. And, importantly for the
simplicity, there is no negotiation and there are no packets going the
other way.

But sure, you can call it a protocol extension if you want. And yes,
it could probably be built on top of part of the ideas in that other
patch, but most of it would be useless (the abstraction of the listen
functionality into listen_have_free_slot/listen_add_socket would be
the big thing that could be used)

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

>> On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
>> > Is there any formal specification for the "a protocol common and very
>> > light weight in proxies"?
>>
>> See
>>
>> https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
>
> Yeah, it's currently in one of the comments, but should probably be
> added to the docs side as well.

It seems the protocol is HAproxy product specific and I think it would
be better to be mentioned in the docs.

> And yes tests :) Probably not a regression test, but some level of tap
> testing should definitely be added. We'll just have to find a way to
> do that without making haproxy a dependency to run the tests :)

Agreed.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
> On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> > Idle thought I had while setting up a local test rig: Are there any
> > compelling cases for allowing PROXY packets to arrive over Unix
> > sockets? (By which I mean, the proxy is running on the same machine as
> > Postgres, and connects to it using the .s.PGSQL socket file instead of
> > TCP.) Are there cases where you want some other software to interact
> > with the TCP stack instead of Postgres, but it'd still be nice to have
> > the original connection information available?
>
> I'm uncertain what that usecase would be for something like haproxy,
> tbh. It can't do connection pooling, so adding it on the same machine
> as postgres itself wouldn't really add anything, I think?

Yeah, I wasn't thinking HAproxy so much as some unspecified software
appliance that's performing Some Task before allowing a TCP client to
speak to Postgres. But it'd be better to hear from someone that has an
actual use case, instead of me spitballing.

> Iid think about the other end, if you had a proxy on a different
> machine accepting unix connections and passing them on over
> PROXY-over-tcp. But I doubt it's useful to know it was unix in that
> case (since it still couldn't do peer or such for the auth) --
> instead, that seems like an argument where it'd be better to proxy
> without using PROXY and just letting the IP address be.

You could potentially design a system that lets you proxy a "local all
all trust" setup from a different (trusted) machine, without having to
actually let people onto the machine that's running Postgres. That
would require some additional authentication on the PROXY connection
(i.e. something stronger than host-based auth) to actually be useful.

-- other notes --

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.

The original-host logging isn't working for me:

WARNING: pg_getnameinfo_all() failed: ai_family not supported
LOG: proxy connection from: host=??? port=???

and I think the culprit is this:

> /* Store a copy of the original address, for logging */
> memcpy(&raddr_save, &port->raddr, port->raddr.salen);

port->raddr.salen is the length of port->raddr.addr; we want the length
of the copy to be sizeof(port->raddr) here, no?

--Jacob

The current proposal seems to miss the case of transaction pooling
(and statement pooling) where the same established connection
multiplexes transactions / statements from multiple remote clients.

What we would need for that case would be a functionl

pg_set_remote_client_address( be_key, remote_ip, remote_hostname)

where only be_key and remote_ip are required, but any string (up to a
certain length) would be accepted as hostname.

It would be really nice if we could send this request at protocol level but
if that is hard to do then having a function would get us half way there.

the be_key in the function is the key from PGcancel, which is stored
by libpq when making the connection, and it is there, to make sure
that only the directly connecting proxy can successfully call the function.

Cheers
Hannu

On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
> > On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> > > Idle thought I had while setting up a local test rig: Are there any
> > > compelling cases for allowing PROXY packets to arrive over Unix
> > > sockets? (By which I mean, the proxy is running on the same machine as
> > > Postgres, and connects to it using the .s.PGSQL socket file instead of
> > > TCP.) Are there cases where you want some other software to interact
> > > with the TCP stack instead of Postgres, but it'd still be nice to have
> > > the original connection information available?
> >
> > I'm uncertain what that usecase would be for something like haproxy,
> > tbh. It can't do connection pooling, so adding it on the same machine
> > as postgres itself wouldn't really add anything, I think?
>
> Yeah, I wasn't thinking HAproxy so much as some unspecified software
> appliance that's performing Some Task before allowing a TCP client to
> speak to Postgres. But it'd be better to hear from someone that has an
> actual use case, instead of me spitballing.
>
> > Iid think about the other end, if you had a proxy on a different
> > machine accepting unix connections and passing them on over
> > PROXY-over-tcp. But I doubt it's useful to know it was unix in that
> > case (since it still couldn't do peer or such for the auth) --
> > instead, that seems like an argument where it'd be better to proxy
> > without using PROXY and just letting the IP address be.
>
> You could potentially design a system that lets you proxy a "local all
> all trust" setup from a different (trusted) machine, without having to
> actually let people onto the machine that's running Postgres. That
> would require some additional authentication on the PROXY connection
> (i.e. something stronger than host-based auth) to actually be useful.
>
> -- other notes --
>
> 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.
>
> The original-host logging isn't working for me:
>
> WARNING: pg_getnameinfo_all() failed: ai_family not supported
> LOG: proxy connection from: host=??? port=???
>
> and I think the culprit is this:
>
> > /* Store a copy of the original address, for logging */
> > memcpy(&raddr_save, &port->raddr, port->raddr.salen);
>
> port->raddr.salen is the length of port->raddr.addr; we want the length
> of the copy to be sizeof(port->raddr) here, no?
>
> --Jacob

On 5/3/21 0:21, Jacob Champion wrote:
> On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
>> On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>>> Idle thought I had while setting up a local test rig: Are there any
>>> compelling cases for allowing PROXY packets to arrive over Unix
>>> sockets? (By which I mean, the proxy is running on the same machine as
>>> Postgres, and connects to it using the .s.PGSQL socket file instead of
>>> TCP.) Are there cases where you want some other software to interact
>>> with the TCP stack instead of Postgres, but it'd still be nice to have
>>> the original connection information available?
>> I'm uncertain what that usecase would be for something like haproxy,
>> tbh. It can't do connection pooling, so adding it on the same machine
>> as postgres itself wouldn't really add anything, I think?
> Yeah, I wasn't thinking HAproxy so much as some unspecified software
> appliance that's performing Some Task before allowing a TCP client to
> speak to Postgres. But it'd be better to hear from someone that has an
> actual use case, instead of me spitballing.

    Here's a use case: Envoy's Postgres filter (see [1], [2]). Right now
is able to capture protocol-level metrics and send them to a metrics
collector (eg. Prometheus) while proxying the traffic. More capabilities
are being added as of today, and will eventually manage HBA too. It
would greatly benefit from this proposal, since it proxies the traffic
with, obviously, its IP, not the client's. It may be used (we do)
locally fronting Postgres, via UDS (so it can be easily trusted).

    Álvaro

[1]
https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/network_filters/postgres_proxy_filter
[2]
https://www.cncf.io/blog/2020/08/13/envoy-1-15-introduces-a-new-postgres-extension-with-monitoring-support/

--

Alvaro Hernandez

-----------
OnGres

On Fri, Mar 5, 2021 at 12:57 AM Hannu Krosing <hannuk(at)google(dot)com> wrote:
>
> The current proposal seems to miss the case of transaction pooling
> (and statement pooling) where the same established connection
> multiplexes transactions / statements from multiple remote clients.

Not at all.

The current proposal is there to implement the PROXY protocol. It
doesn't try to do anything with connection pooling at all.

Solving a similar problem for connection poolers would also definitely
be a useful thing, but it is entirely out of scope of this patch, and
is a completely separate implementation.

I'd definitely like to see that one solved as well, but let's look at
it on a different thread so we don't derail this one.

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

On Fri, Mar 5, 2021 at 1:33 AM Álvaro Hernández <aht(at)ongres(dot)com> wrote:
>
>
>
> On 5/3/21 0:21, Jacob Champion wrote:
> > On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
> >> On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> >>> Idle thought I had while setting up a local test rig: Are there any
> >>> compelling cases for allowing PROXY packets to arrive over Unix
> >>> sockets? (By which I mean, the proxy is running on the same machine as
> >>> Postgres, and connects to it using the .s.PGSQL socket file instead of
> >>> TCP.) Are there cases where you want some other software to interact
> >>> with the TCP stack instead of Postgres, but it'd still be nice to have
> >>> the original connection information available?
> >> I'm uncertain what that usecase would be for something like haproxy,
> >> tbh. It can't do connection pooling, so adding it on the same machine
> >> as postgres itself wouldn't really add anything, I think?
> > Yeah, I wasn't thinking HAproxy so much as some unspecified software
> > appliance that's performing Some Task before allowing a TCP client to
> > speak to Postgres. But it'd be better to hear from someone that has an
> > actual use case, instead of me spitballing.
>
> Here's a use case: Envoy's Postgres filter (see [1], [2]). Right now
> is able to capture protocol-level metrics and send them to a metrics
> collector (eg. Prometheus) while proxying the traffic. More capabilities
> are being added as of today, and will eventually manage HBA too. It
> would greatly benefit from this proposal, since it proxies the traffic
> with, obviously, its IP, not the client's. It may be used (we do)
> locally fronting Postgres, via UDS (so it can be easily trusted).

Yeah, Envoy is definitely a great example of a usecase for the proxy
protocol in general.

Specifically about the Unix socket though -- doesn't envoy normally
run on a different instance (or in a different container at least),
thus normally uses tcp between envoy and postgres? Or would it be a
reasonable usecase that you ran it locally on the postgres server,
having it speak IP to the clients but unix sockets to the postgres
backend? I guess maybe it is outside of the containerized world?

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

On Fri, Mar 5, 2021 at 12:08 AM Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> wrote:
>
> >> On Thu, 2021-03-04 at 10:42 +0900, Tatsuo Ishii wrote:
> >> > Is there any formal specification for the "a protocol common and very
> >> > light weight in proxies"?
> >>
> >> See
> >>
> >> https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
> >
> > Yeah, it's currently in one of the comments, but should probably be
> > added to the docs side as well.
>
> It seems the protocol is HAproxy product specific and I think it would
> be better to be mentioned in the docs.

It's definitely not HAProxy specific, it's more or less an industry
standard. It's just maintained by them. That said, yes, it should be
referenced in the docs.

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

On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
> > On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> > > Idle thought I had while setting up a local test rig: Are there any
> > > compelling cases for allowing PROXY packets to arrive over Unix
> > > sockets? (By which I mean, the proxy is running on the same machine as
> > > Postgres, and connects to it using the .s.PGSQL socket file instead of
> > > TCP.) Are there cases where you want some other software to interact
> > > with the TCP stack instead of Postgres, but it'd still be nice to have
> > > the original connection information available?
> >
> > I'm uncertain what that usecase would be for something like haproxy,
> > tbh. It can't do connection pooling, so adding it on the same machine
> > as postgres itself wouldn't really add anything, I think?
>
> Yeah, I wasn't thinking HAproxy so much as some unspecified software
> appliance that's performing Some Task before allowing a TCP client to
> speak to Postgres. But it'd be better to hear from someone that has an
> actual use case, instead of me spitballing.
>
> > Iid think about the other end, if you had a proxy on a different
> > machine accepting unix connections and passing them on over
> > PROXY-over-tcp. But I doubt it's useful to know it was unix in that
> > case (since it still couldn't do peer or such for the auth) --
> > instead, that seems like an argument where it'd be better to proxy
> > without using PROXY and just letting the IP address be.
>
> You could potentially design a system that lets you proxy a "local all
> all trust" setup from a different (trusted) machine, without having to
> actually let people onto the machine that's running Postgres. That
> would require some additional authentication on the PROXY connection
> (i.e. something stronger than host-based auth) to actually be useful.
>
> -- other notes --
>
> 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.

> The original-host logging isn't working for me:
>
> WARNING: pg_getnameinfo_all() failed: ai_family not supported
> LOG: proxy connection from: host=??? port=???
>
> and I think the culprit is this:
>
> > /* Store a copy of the original address, for logging */
> > memcpy(&raddr_save, &port->raddr, port->raddr.salen);
>
> port->raddr.salen is the length of port->raddr.addr; we want the length
> of the copy to be sizeof(port->raddr) here, no?

That's interesting -- it works perfectly fine here. What platform are
you testing on?

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. (I sent for sizeof(SockAddr) to make it
easier to read without having to look things up, but the net result is
the same)

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

On 5/3/21 10:03, Magnus Hagander wrote:
> On Fri, Mar 5, 2021 at 1:33 AM Álvaro Hernández <aht(at)ongres(dot)com> wrote:
>>
>>
>> On 5/3/21 0:21, Jacob Champion wrote:
>>> On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
>>>> On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>>>>> Idle thought I had while setting up a local test rig: Are there any
>>>>> compelling cases for allowing PROXY packets to arrive over Unix
>>>>> sockets? (By which I mean, the proxy is running on the same machine as
>>>>> Postgres, and connects to it using the .s.PGSQL socket file instead of
>>>>> TCP.) Are there cases where you want some other software to interact
>>>>> with the TCP stack instead of Postgres, but it'd still be nice to have
>>>>> the original connection information available?
>>>> I'm uncertain what that usecase would be for something like haproxy,
>>>> tbh. It can't do connection pooling, so adding it on the same machine
>>>> as postgres itself wouldn't really add anything, I think?
>>> Yeah, I wasn't thinking HAproxy so much as some unspecified software
>>> appliance that's performing Some Task before allowing a TCP client to
>>> speak to Postgres. But it'd be better to hear from someone that has an
>>> actual use case, instead of me spitballing.
>> Here's a use case: Envoy's Postgres filter (see [1], [2]). Right now
>> is able to capture protocol-level metrics and send them to a metrics
>> collector (eg. Prometheus) while proxying the traffic. More capabilities
>> are being added as of today, and will eventually manage HBA too. It
>> would greatly benefit from this proposal, since it proxies the traffic
>> with, obviously, its IP, not the client's. It may be used (we do)
>> locally fronting Postgres, via UDS (so it can be easily trusted).
> Yeah, Envoy is definitely a great example of a usecase for the proxy
> protocol in general.

    Actually Envoy already implements the Proxy protocol:
https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/listener_filters/proxy_protocol.html
But I believe it would need some further cooperation with the Postgres
filter, unless they can be chained directly. Still, Postgres needs to
understand it, which is what your patch would add (thanks!).

>
> Specifically about the Unix socket though -- doesn't envoy normally
> run on a different instance (or in a different container at least),
> thus normally uses tcp between envoy and postgres? Or would it be a
> reasonable usecase that you ran it locally on the postgres server,
> having it speak IP to the clients but unix sockets to the postgres
> backend? I guess maybe it is outside of the containerized world?
>

    This is exactly the architecture we use at StackGres [1][2]. We use
Envoy as a sidecar (so it runs on the same pod, server as Postgres) and
connects via UDS. But then exposes the connection to the outside clients
via TCP/IP. So in my opinion it is quite applicable to the container
world :)

    Álvaro

[1] https://stackgres.io
[2]
https://stackgres.io/doc/latest/intro/architecture/#stackgres-pod-architecture-diagram

--

Alvaro Hernandez

-----------
OnGres

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

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.

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

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/

On Sat, Mar 6, 2021 at 5:30 PM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> 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.

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.

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

On Tue, 2021-03-09 at 11:25 +0100, Magnus Hagander wrote:
> 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).

Yeah. The tests I'm writing for this and NSS have been the same way;
it's a real problem. I'm basically writing supplemental tests in Python
as the "daily driver", then trying to port whatever is easiest (not
much) into Perl, when I get time.

== More Notes ==

Some additional spec-compliance stuff:

> /* Lower 4 bits hold type of connection */
> if (proxyheader.fam == 0)
> {
> /* LOCAL connection, so we ignore the address included */
> }

(fam == 0) is the UNSPEC case, which isn't necessarily LOCAL. We have
to do something different for the LOCAL case:

> - \x0 : LOCAL : [...] The receiver must accept this connection as
> valid and must use the real connection endpoints and discard the
> protocol block including the family which is ignored.

So we should ignore the entire "protocol block" (by which I believe
they mean the protocol-and-address-family byte) in the case of LOCAL,
and just accept it with the original address info intact. That seems to
match the sample code in the back of the spec. The current behavior in
the patch will apply the PROXY behavior incorrectly if the sender sends
a LOCAL header with something other than UNSPEC -- which is strange
behavior but not explicitly prohibited as far as I can see.

We also need to reject all connections that aren't either LOCAL or
PROXY commands:

> - other values are unassigned and must not be emitted by senders.
> Receivers must drop connections presenting unexpected values here.

...and naturally it'd be Nice (tm) if the tests covered those corner
cases.

Over on the struct side:

> + struct
> + { /* for TCP/UDP over IPv4, len = 12 */
> + uint32 src_addr;
> + uint32 dst_addr;
> + uint16 src_port;
> + uint16 dst_port;
> + } ip4;
> ... snip ...
> + /* TCPv4 */
> + if (proxyaddrlen < 12)
> + {

Given the importance of these hardcoded lengths matching reality, is it
possible to add some static assertions to make sure that sizeof(<ipv4
block>) == 12 and so on? That would also save any poor souls who are
using compilers with nonstandard struct-packing behavior.

--Jacob

On Tue, Mar 9, 2021 at 11:25 AM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> On Sat, Mar 6, 2021 at 5:30 PM Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> >
> > 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.
>
> 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/

On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Tue, 2021-03-09 at 11:25 +0100, Magnus Hagander wrote:
> > 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).
>
> Yeah. The tests I'm writing for this and NSS have been the same way;
> it's a real problem. I'm basically writing supplemental tests in Python
> as the "daily driver", then trying to port whatever is easiest (not
> much) into Perl, when I get time.
>
> == More Notes ==
>
> Some additional spec-compliance stuff:
>
> > /* Lower 4 bits hold type of connection */
> > if (proxyheader.fam == 0)
> > {
> > /* LOCAL connection, so we ignore the address included */
> > }
>
> (fam == 0) is the UNSPEC case, which isn't necessarily LOCAL. We have
> to do something different for the LOCAL case:

Oh ugh. yeah, and the comment is wrong too -- it got the "command"
confused with "connection family". Too many copy/paste I think.

> > - \x0 : LOCAL : [...] The receiver must accept this connection as
> > valid and must use the real connection endpoints and discard the
> > protocol block including the family which is ignored.
>
> So we should ignore the entire "protocol block" (by which I believe
> they mean the protocol-and-address-family byte) in the case of LOCAL,
> and just accept it with the original address info intact. That seems to
> match the sample code in the back of the spec. The current behavior in
> the patch will apply the PROXY behavior incorrectly if the sender sends
> a LOCAL header with something other than UNSPEC -- which is strange
> behavior but not explicitly prohibited as far as I can see.

Yeah, I think we do the right thing in the "right usecase".

> We also need to reject all connections that aren't either LOCAL or
> PROXY commands:

Indeed.

> > - other values are unassigned and must not be emitted by senders.
> > Receivers must drop connections presenting unexpected values here.
>
> ...and naturally it'd be Nice (tm) if the tests covered those corner
> cases.

I think that's covered in the attached update.

> Over on the struct side:
>
> > + struct
> > + { /* for TCP/UDP over IPv4, len = 12 */
> > + uint32 src_addr;
> > + uint32 dst_addr;
> > + uint16 src_port;
> > + uint16 dst_port;
> > + } ip4;
> > ... snip ...
> > + /* TCPv4 */
> > + if (proxyaddrlen < 12)
> > + {
>
> Given the importance of these hardcoded lengths matching reality, is it
> possible to add some static assertions to make sure that sizeof(<ipv4
> block>) == 12 and so on? That would also save any poor souls who are
> using compilers with nonstandard struct-packing behavior.

Yeah, probably makes sense. Added.

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

Hi Magnus,

I'm only just starting to page this back into my head, so this is by no
means a full review of the v7 changes -- just stuff I've noticed over
the last day or so of poking around.

On Tue, 2021-06-29 at 11:48 +0200, Magnus Hagander wrote:
> On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> > On Tue, 2021-03-09 at 11:25 +0100, Magnus Hagander wrote:
> > > - \x0 : LOCAL : [...] The receiver must accept this connection as
> > > valid and must use the real connection endpoints and discard the
> > > protocol block including the family which is ignored.
> >
> > So we should ignore the entire "protocol block" (by which I believe
> > they mean the protocol-and-address-family byte) in the case of LOCAL,
> > and just accept it with the original address info intact. That seems to
> > match the sample code in the back of the spec. The current behavior in
> > the patch will apply the PROXY behavior incorrectly if the sender sends
> > a LOCAL header with something other than UNSPEC -- which is strange
> > behavior but not explicitly prohibited as far as I can see.
>
> Yeah, I think we do the right thing in the "right usecase".

The current implementation is, I think, stricter than the spec asks
for. We're supposed to ignore the family for LOCAL cases, and it's not
clear to me whether we're supposed to also ignore the entire "fam"
family-and-protocol byte (the phrase "protocol block" is not actually
defined in the spec).

It's probably not a big deal in practice, but it could mess with
interoperability for lazier proxy implementations. I think I'll ask the
HAProxy folks for some clarification tomorrow.

> + <term><varname>proxy_servers</varname> (<type>string</type>)
> + <indexterm>
> + <primary><varname>proxy_servers</varname> configuration parameter</primary>
> + </indexterm>
> + </term>
> + <listitem>
> + <para>
> + A comma separated list of one or more host names, cidr specifications or the
> + literal <literal>unix</literal>, indicating which proxy servers to trust when
> + connecting on the port specified in <xref linkend="guc-proxy-port" />.

The documentation mentions that host names are valid in proxy_servers,
but check_proxy_servers() uses the AI_NUMERICHOST hint with
getaddrinfo(), so host names get rejected.

> + GUC_check_errdetail("Invalid IP addrress %s", tok);

s/addrress/address/

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).

--Jacob

On Fri, Jul 9, 2021 at 1:42 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> Hi Magnus,
>
> I'm only just starting to page this back into my head, so this is by no
> means a full review of the v7 changes -- just stuff I've noticed over
> the last day or so of poking around.
>
> On Tue, 2021-06-29 at 11:48 +0200, Magnus Hagander wrote:
> > On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
> > > On Tue, 2021-03-09 at 11:25 +0100, Magnus Hagander wrote:
> > > > - \x0 : LOCAL : [...] The receiver must accept this connection as
> > > > valid and must use the real connection endpoints and discard the
> > > > protocol block including the family which is ignored.
> > >
> > > So we should ignore the entire "protocol block" (by which I believe
> > > they mean the protocol-and-address-family byte) in the case of LOCAL,
> > > and just accept it with the original address info intact. That seems to
> > > match the sample code in the back of the spec. The current behavior in
> > > the patch will apply the PROXY behavior incorrectly if the sender sends
> > > a LOCAL header with something other than UNSPEC -- which is strange
> > > behavior but not explicitly prohibited as far as I can see.
> >
> > Yeah, I think we do the right thing in the "right usecase".
>
> The current implementation is, I think, stricter than the spec asks
> for. We're supposed to ignore the family for LOCAL cases, and it's not
> clear to me whether we're supposed to also ignore the entire "fam"
> family-and-protocol byte (the phrase "protocol block" is not actually
> defined in the spec).
>
> It's probably not a big deal in practice, but it could mess with
> interoperability for lazier proxy implementations. I think I'll ask the
> HAProxy folks for some clarification tomorrow.

Thanks!

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..

> > + <term><varname>proxy_servers</varname> (<type>string</type>)
> > + <indexterm>
> > + <primary><varname>proxy_servers</varname> configuration parameter</primary>
> > + </indexterm>
> > + </term>
> > + <listitem>
> > + <para>
> > + A comma separated list of one or more host names, cidr specifications or the
> > + literal <literal>unix</literal>, indicating which proxy servers to trust when
> > + connecting on the port specified in <xref linkend="guc-proxy-port" />.
>
> The documentation mentions that host names are valid in proxy_servers,
> but check_proxy_servers() uses the AI_NUMERICHOST hint with
> getaddrinfo(), so host names get rejected.

Ah, good point. Should say "ip addresses".

>
> > + GUC_check_errdetail("Invalid IP addrress %s", tok);
>
> s/addrress/address/

Oops.

> 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?

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.

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

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.

> > 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. :)

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?

> 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.

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.

--Jacob

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/

On Tue, 2021-09-07 at 12:24 +0200, Magnus Hagander wrote:
> 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? :)

Not yet :( I've bumped the thread; in the meantime I still think the
stricter operation is fine, since in the worst case you just make it
less strict in the future.

> (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)

No worries!

> > > 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.

I guess I'm in the same boat. I'm probably not the right person to
weigh in.

> > 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!

I still owe you that overall review. Hoping to get to it this week.

> > 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.

Hm. I guess I'm overly optimistic that "properly securing your
database" is not such an uncommon case, but... :)

--Jacob

On Wed, 2021-09-08 at 18:51 +0000, Jacob Champion wrote:
> I still owe you that overall review. Hoping to get to it this week.

And here it is. I focused on things other than UnwrapProxyConnection()
for this round, since I think that piece is looking solid.

> + if (port->isProxy)
> + {
> + ereport(LOG,
> + (errcode_for_socket_access(),
> + errmsg("Ident authentication cannot be used over PROXY connections")));

What are the rules on COMMERROR vs LOG when dealing with authentication
code? I always thought COMMERROR was required, but I see now that LOG
(among others) is suppressed to the client during authentication.

> #ifdef USE_SSL
> /* No SSL when disabled or on Unix sockets */
> - if (!LoadedSSL || IS_AF_UNIX(port->laddr.addr.ss_family))
> + if (!LoadedSSL || (IS_AF_UNIX(port->laddr.addr.ss_family) && !port->isProxy))
> SSLok = 'N';
> else
> SSLok = 'S'; /* Support for SSL */
> @@ -2087,7 +2414,7 @@ retry1:
>
> #ifdef ENABLE_GSS
> /* No GSSAPI encryption when on Unix socket */
> - if (!IS_AF_UNIX(port->laddr.addr.ss_family))
> + if (!IS_AF_UNIX(port->laddr.addr.ss_family) || port->isProxy)
> GSSok = 'G';

Now that we have port->daddr, could these checks be simplified to just
IS_AF_UNIX(port->daddr...)? Or is there a corner case I'm missing for
the port->isProxy case?

> + * Note: AuthenticationTimeout is applied here while waiting for the
> + * startup packet, and then again in InitPostgres for the duration of any
> + * authentication operations. So a hostile client could tie up the
> + * process for nearly twice AuthenticationTimeout before we kick him off.

This comment needs to be adjusted after the move; waiting for the
startup packet comes later, and it looks like we can now tie up 3x the
timeout for the proxy case.

> + /* Check if this is a proxy connection and if so unwrap the proxying */
> + if (port->isProxy)
> + {
> + enable_timeout_after(STARTUP_PACKET_TIMEOUT, AuthenticationTimeout * 1000);
> + if (UnwrapProxyConnection(port) != STATUS_OK)
> + proc_exit(0);

I think the timeout here could comfortably be substantially less than
the overall authentication timeout, since the proxy should send its
header immediately even if the client takes its time with the startup
packet. The spec suggests allowing 3 seconds minimum to cover a
retransmission. Maybe something to tune in the future?

> + /* Also listen to the PROXY port on this address, if configured */
> + if (ProxyPortNumber)
> + {
> + if (strcmp(curhost, "*") == 0)
> + socket = StreamServerPort(AF_UNSPEC, NULL,
> + (unsigned short) ProxyPortNumber,
> + NULL,
> + ListenSocket, MAXLISTEN);

Sorry if you already talked about this upthread somewhere, but it looks
like another downside of treating "proxy mode" as a server-wide on/off
switch is that it cuts the effective MAXLISTEN in half, from 64 to 32,
since we're opening two sockets for every address. If I've understood
that correctly, it might be worth mentioning in the docs.

> - if (!success && elemlist != NIL)
> + if (socket == NULL && elemlist != NIL)
> ereport(FATAL,
> (errmsg("could not create any TCP/IP sockets")));

With this change in PostmasterMain, it looks like `success` is no
longer a useful variable. But I'm not convinced that this is the
correct logic -- this is just checking to see if the last socket
creation succeeded, as opposed to seeing if any of them succeeded. Is
that what you intended?

> +plan tests => 25;
> +
> +my $node = get_new_node('node');

The TAP test will need to be rebased over the changes in 201a76183e.

--Jacob

On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Wed, 2021-09-08 at 18:51 +0000, Jacob Champion wrote:
> > I still owe you that overall review. Hoping to get to it this week.
>
> And here it is. I focused on things other than UnwrapProxyConnection()
> for this round, since I think that piece is looking solid.

Thanks!

> > + if (port->isProxy)
> > + {
> > + ereport(LOG,
> > + (errcode_for_socket_access(),
> > + errmsg("Ident authentication cannot be used over PROXY connections")));
>
> What are the rules on COMMERROR vs LOG when dealing with authentication
> code? I always thought COMMERROR was required, but I see now that LOG
> (among others) is suppressed to the client during authentication.

I honestly don't know :) In this case, LOG is what's used for all the
other message in errors in ident_inet(), so I picked it for
consistency.

> > #ifdef USE_SSL
> > /* No SSL when disabled or on Unix sockets */
> > - if (!LoadedSSL || IS_AF_UNIX(port->laddr.addr.ss_family))
> > + if (!LoadedSSL || (IS_AF_UNIX(port->laddr.addr.ss_family) && !port->isProxy))
> > SSLok = 'N';
> > else
> > SSLok = 'S'; /* Support for SSL */
> > @@ -2087,7 +2414,7 @@ retry1:
> >
> > #ifdef ENABLE_GSS
> > /* No GSSAPI encryption when on Unix socket */
> > - if (!IS_AF_UNIX(port->laddr.addr.ss_family))
> > + if (!IS_AF_UNIX(port->laddr.addr.ss_family) || port->isProxy)
> > GSSok = 'G';
>
> Now that we have port->daddr, could these checks be simplified to just
> IS_AF_UNIX(port->daddr...)? Or is there a corner case I'm missing for
> the port->isProxy case?

Yeah, I think they could.

> > + * Note: AuthenticationTimeout is applied here while waiting for the
> > + * startup packet, and then again in InitPostgres for the duration of any
> > + * authentication operations. So a hostile client could tie up the
> > + * process for nearly twice AuthenticationTimeout before we kick him off.
>
> This comment needs to be adjusted after the move; waiting for the
> startup packet comes later, and it looks like we can now tie up 3x the
> timeout for the proxy case.

Good point.

> > + /* Check if this is a proxy connection and if so unwrap the proxying */
> > + if (port->isProxy)
> > + {
> > + enable_timeout_after(STARTUP_PACKET_TIMEOUT, AuthenticationTimeout * 1000);
> > + if (UnwrapProxyConnection(port) != STATUS_OK)
> > + proc_exit(0);
>
> I think the timeout here could comfortably be substantially less than
> the overall authentication timeout, since the proxy should send its
> header immediately even if the client takes its time with the startup
> packet. The spec suggests allowing 3 seconds minimum to cover a
> retransmission. Maybe something to tune in the future?

Maybe. I'll leave it with a new comment for now about us diong it, and
that we may want to consider igt in the future.

>
> > + /* Also listen to the PROXY port on this address, if configured */
> > + if (ProxyPortNumber)
> > + {
> > + if (strcmp(curhost, "*") == 0)
> > + socket = StreamServerPort(AF_UNSPEC, NULL,
> > + (unsigned short) ProxyPortNumber,
> > + NULL,
> > + ListenSocket, MAXLISTEN);
>
> Sorry if you already talked about this upthread somewhere, but it looks
> like another downside of treating "proxy mode" as a server-wide on/off
> switch is that it cuts the effective MAXLISTEN in half, from 64 to 32,
> since we're opening two sockets for every address. If I've understood
> that correctly, it might be worth mentioning in the docs.

Correct. I don't see a way to avoid that without complicating things
(as long as we want the ports to be separate), but I also don't see it
as something that's reality to be an issue in reality.

I would agree with documenting it, but I can't actually find us
documenting the MAXLISTEN value anywhere. Do we?

> > - if (!success && elemlist != NIL)
> > + if (socket == NULL && elemlist != NIL)
> > ereport(FATAL,
> > (errmsg("could not create any TCP/IP sockets")));
>
> With this change in PostmasterMain, it looks like `success` is no
> longer a useful variable. But I'm not convinced that this is the
> correct logic -- this is just checking to see if the last socket
> creation succeeded, as opposed to seeing if any of them succeeded. Is
> that what you intended?

Eh, no, that's clearly a code-moving-bug.

I think the reasonable thing is to succeed if we create either a
regular socket *or* a proxy one, but FATAL out if you configured
either of them but we failed co create any.

> > +plan tests => 25;
> > +
> > +my $node = get_new_node('node');
>
> The TAP test will need to be rebased over the changes in 201a76183e.

Done, and adjustments above according to your comments, along with a
small docs fix "a proxy connection is done" -> "a proxy connection is
made".

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

> On 28 Sep 2021, at 15:23, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:

>> The TAP test will need to be rebased over the changes in 201a76183e.
>
> Done

And now the TAP test will need to be rebased over the changes in
b3b4d8e68ae83f432f43f035c7eb481ef93e1583.

--
Daniel Gustafsson https://vmware.com/

On Wed, Nov 3, 2021 at 2:36 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 28 Sep 2021, at 15:23, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> > On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion <pchampion(at)vmware(dot)com>
> wrote:
>
> >> The TAP test will need to be rebased over the changes in 201a76183e.
> >
> > Done
>
> And now the TAP test will need to be rebased over the changes in
> b3b4d8e68ae83f432f43f035c7eb481ef93e1583.
>

Thanks for the pointer, PFA a rebase.

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

On Thu, 2021-11-04 at 12:03 +0100, Magnus Hagander wrote:
> Thanks for the pointer, PFA a rebase.

I think the Unix socket handling needs the same "success" fix that you
applied to the TCP socket handling above it:

> @@ -1328,9 +1364,23 @@ PostmasterMain(int argc, char *argv[])
> ereport(WARNING,
> (errmsg("could not create Unix-domain socket in directory \"%s\"",
> socketdir)));
> +
> + if (ProxyPortNumber)
> + {
> + socket = StreamServerPort(AF_UNIX, NULL,
> + (unsigned short) ProxyPortNumber,
> + socketdir,
> + ListenSocket, MAXLISTEN);
> + if (socket)
> + socket->isProxy = true;
> + else
> + ereport(WARNING,
> + (errmsg("could not create Unix-domain PROXY socket for \"%s\"",
> + socketdir)));
> + }
> }
>
> - if (!success && elemlist != NIL)
> + if (socket == NULL && elemlist != NIL)
> ereport(FATAL,
> (errmsg("could not create any Unix-domain sockets")));

Other than that, I can find nothing else to improve, and I think this
is ready for more eyes than mine. :)

--

To tie off some loose ends from upthread:

I didn't find any MAXLISTEN documentation either, so I guess it's only
a documentation issue if someone runs into it, heh.

I was not able to find any other cases (besides ident) where using
daddr instead of laddr would break things. I am going a bit snow-blind
on the patch, though, and there's a lot of auth code.

I never did hear back from the PROXY spec maintainer on how strict to
be with LOCAL; another contributor did chime in but only to add that
they didn't know the answer. That conversation is at [1], in case
someone picks it up in the future.

A summary of possible improvements talked about upthread, for a future
v2:

- SQL functions to get the laddr info (scoped to superusers, somehow),
if there's a use case for them

- Setting up PROXY Unix socket permissions separately from the "main"
socket

- Allowing PROXY-only communication (disable the "main" port)

Thanks,
--Jacob

[1] https://www.mail-archive.com/haproxy(at)formilux(dot)org/msg40899.html

On Tue, Nov 16, 2021 at 12:03 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Thu, 2021-11-04 at 12:03 +0100, Magnus Hagander wrote:
> > Thanks for the pointer, PFA a rebase.
>
> I think the Unix socket handling needs the same "success" fix that you
> applied to the TCP socket handling above it:
>
> > @@ -1328,9 +1364,23 @@ PostmasterMain(int argc, char *argv[])
> > ereport(WARNING,
> > (errmsg("could not create Unix-domain socket in directory \"%s\"",
> > socketdir)));
> > +
> > + if (ProxyPortNumber)
> > + {
> > + socket = StreamServerPort(AF_UNIX, NULL,
> > + (unsigned short) ProxyPortNumber,
> > + socketdir,
> > + ListenSocket, MAXLISTEN);
> > + if (socket)
> > + socket->isProxy = true;
> > + else
> > + ereport(WARNING,
> > + (errmsg("could not create Unix-domain PROXY socket for \"%s\"",
> > + socketdir)));
> > + }
> > }
> >
> > - if (!success && elemlist != NIL)
> > + if (socket == NULL && elemlist != NIL)
> > ereport(FATAL,
> > (errmsg("could not create any Unix-domain sockets")));
>
> Other than that, I can find nothing else to improve, and I think this
> is ready for more eyes than mine. :)

Here's another rebase on top of the AF_UNIX patch.

> To tie off some loose ends from upthread:
>
> I didn't find any MAXLISTEN documentation either, so I guess it's only
> a documentation issue if someone runs into it, heh.
>
> I was not able to find any other cases (besides ident) where using
> daddr instead of laddr would break things. I am going a bit snow-blind
> on the patch, though, and there's a lot of auth code.

Yeah, that's definitely a good reason for more eyes on it.

> A summary of possible improvements talked about upthread, for a future
> v2:
>
> - SQL functions to get the laddr info (scoped to superusers, somehow),
> if there's a use case for them
>
> - Setting up PROXY Unix socket permissions separately from the "main"
> socket
>
> - Allowing PROXY-only communication (disable the "main" port)

These all seem useful, but I'm liking the idea of putting them in a
v2, to avoid expanding the scope too much.

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

A general question on this feature: AFAICT, you can only send the proxy
header once at the beginning of the connection. So this wouldn't be of
use for PostgreSQL-protocol connection poolers (pgbouncer, pgpool),
where the same server connection can be used for clients from different
source addresses. Do I understand that correctly?

On Wed, Mar 9, 2022 at 5:23 PM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> A general question on this feature: AFAICT, you can only send the proxy
> header once at the beginning of the connection. So this wouldn't be of
> use for PostgreSQL-protocol connection poolers (pgbouncer, pgpool),
> where the same server connection can be used for clients from different
> source addresses. Do I understand that correctly?

Correct. It's only sent at connection startup, so if you're re-using
the connection it would also re-use the IP address of the first one.

For reusing the connection for multiple clients, you'd want something
different, like a "priviliged mode in the tunnel" that the pooler can
handle.

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

Hi,

I've been able to test the patch. Here is a recap of the experimentation.

# Setup

All tests have been done witch 3 VMs (PostgreSQL, HAproxy, psql client) on
Debian 11 communicating over private network.
* PostgreSQL have been built with proxy_protocol_11.patch applied on master branch (465ab24296).
* psql client is from postgresql-client-13 from Debian 11 repository.
* HAproxy version used is 2.5.5-1~bpo11+1 installed from https://haproxy.debian.net

# Configuration

PostgresSQL has been configured to listen only on its private IP. To enable
proxy protocol support `proxy_port` has been configured to `5431` and
`proxy_servers` to `10.0.0.0/24`. `log_connections` has been turned on to make
sure the correct IP address is logged. `log_min_duration_statement` has been
configured to 0 to log all queries. Finally `log_destination` has been
configured to `csvlog`.

pg_hba.conf is like this:

local all all trust
host all all 127.0.0.1/32 trust
host all all ::1/128 trust
local replication all trust
host replication all 127.0.0.1/32 trust
host replication all ::1/128 trust
host all all 10.0.0.208/32 md5

Where 10.0.0.208 is the IP host the psql client's VM.

HAproxy has two frontends, one for proxy protocol (port 5431) and one for
regular TCP traffic. The configuration looks like this:

listen postgresql
bind 10.0.0.222:5432
server pg 10.0.0.253:5432 check

listen postgresql_proxy
bind 10.0.0.222:5431
server pg 10.0.0.253:5431 send-proxy-v2

Where 10.0.0.222 is the IP of HAproxy's VM and 10.0.0.253 is the IP of
PostgreSQL's VM.

# Tests

* from psql's vm to haproxy on port 5432 (no proxy protocol)
--> connection denied by pg_hba.conf, as expected

* from psql's vm to postgresql's VM on port 5432 (no proxy protocol)
--> connection success with psql's vm ip in logfile and pg_stat_activity

* from psql's vm to postgresql's VM on port 5431 (proxy protocol)
--> unable to open a connection, as expected

* from psql's vm to haproxy on port 5431 (proxy protocol)
--> connection success with psql's vm ip in logfile and pg_stat_activity

I've also tested without proxy protocol enable (and pg_hba.conf updated
accordingly), PostgreSQL behave as expected.

# Conclusion

From my point of view the documentation is clear enough and the feature works
as expected.

On Sat, Apr 2, 2022 at 12:17 AM wilfried roset <wilfried(dot)roset(at)gmail(dot)com>
wrote:

> Hi,
>
> I've been able to test the patch. Here is a recap of the experimentation.
>
> # Setup
>
> All tests have been done witch 3 VMs (PostgreSQL, HAproxy, psql client) on
> Debian 11 communicating over private network.
> * PostgreSQL have been built with proxy_protocol_11.patch applied on
> master branch (465ab24296).
> * psql client is from postgresql-client-13 from Debian 11 repository.
> * HAproxy version used is 2.5.5-1~bpo11+1 installed from
> https://haproxy.debian.net
>
> # Configuration
>
> PostgresSQL has been configured to listen only on its private IP. To enable
> proxy protocol support `proxy_port` has been configured to `5431` and
> `proxy_servers` to `10.0.0.0/24` <http://10.0.0.0/24>. `log_connections`
> has been turned on to make
> sure the correct IP address is logged. `log_min_duration_statement` has
> been
> configured to 0 to log all queries. Finally `log_destination` has been
> configured to `csvlog`.
>
> pg_hba.conf is like this:
>
> local all all trust
> host all all 127.0.0.1/32 trust
> host all all ::1/128 trust
> local replication all trust
> host replication all 127.0.0.1/32 trust
> host replication all ::1/128 trust
> host all all 10.0.0.208/32 md5
>
> Where 10.0.0.208 is the IP host the psql client's VM.
>
> HAproxy has two frontends, one for proxy protocol (port 5431) and one for
> regular TCP traffic. The configuration looks like this:
>
> listen postgresql
> bind 10.0.0.222:5432
> server pg 10.0.0.253:5432 check
>
> listen postgresql_proxy
> bind 10.0.0.222:5431
> server pg 10.0.0.253:5431 send-proxy-v2
>
> Where 10.0.0.222 is the IP of HAproxy's VM and 10.0.0.253 is the IP of
> PostgreSQL's VM.
>
> # Tests
>
> * from psql's vm to haproxy on port 5432 (no proxy protocol)
> --> connection denied by pg_hba.conf, as expected
>
> * from psql's vm to postgresql's VM on port 5432 (no proxy protocol)
> --> connection success with psql's vm ip in logfile and pg_stat_activity
>
> * from psql's vm to postgresql's VM on port 5431 (proxy protocol)
> --> unable to open a connection, as expected
>
> * from psql's vm to haproxy on port 5431 (proxy protocol)
> --> connection success with psql's vm ip in logfile and pg_stat_activity
>
> I've also tested without proxy protocol enable (and pg_hba.conf updated
> accordingly), PostgreSQL behave as expected.
>
> # Conclusion
>
> From my point of view the documentation is clear enough and the feature
> works
> as expected.

Hi!

Thanks for this review and testing!

I think it could do with at least noe more look-over at the source code
level as well at this point though since it's been sitting around for a
while, so it won't make it in for this deadline. But hopefully I can get it
in early in the next cycle!

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

This needs a rebase, but after that I expect it to be RfC.

--Jacob

The new status of this patch is: Waiting on Author

On 7/28/22 22:05, Jacob Champion wrote:
> This needs a rebase, but after that I expect it to be RfC.
>
> --Jacob
>
> The new status of this patch is: Waiting on Author

Hello folks,

Thank you all for this awesome work!

I'm looking for this feature for years now. Last year, I've tried to
rebase the patch without success. Unfortunately, this is out of my league.

Magnus, please let me know if I can help.

Have a nice day,
Julien