Re: PROXY protocol support

[Julien Riou]

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

Re: PROXY protocol support

[Jacob Champion]

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

--Jacob

The new status of this patch is: Waiting on Author

Re: PROXY protocol support

[Magnus Hagander]

On Sat, Apr 2, 2022 at 12:17 AM wilfried roset 
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
>   hostall all 127.0.0.1/32trust
>   hostall all ::1/128 trust
>   local   replication all trust
>   hostreplication all 127.0.0.1/32trust
>   hostreplication all ::1/128 trust
>   hostall 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/>

Re: PROXY protocol support

[wilfried roset]

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
  hostall all 127.0.0.1/32trust
  hostall all ::1/128 trust
  local   replication all trust
  hostreplication all 127.0.0.1/32trust
  hostreplication all ::1/128 trust
  hostall 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.

Re: PROXY protocol support

[Magnus Hagander]

On Wed, Mar 9, 2022 at 5:23 PM Peter Eisentraut
 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/

Re: PROXY protocol support

[Peter Eisentraut]

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?

Re: PROXY protocol support

[Magnus Hagander]

On Tue, Nov 16, 2021 at 12:03 AM Jacob Champion  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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 02f0489112..a3ff09b3ac 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7ed8c82a9d..e0847b6347 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,56 @@ include_dir 'conf.d'
   
  
 
+ 
+  proxy_port (integer)
+  
+   proxy_port configuration parameter
+  
+  
+  
+   
+The TCP port the server listens on for PROXY connections, disabled by
+default. If set to a number, PostgreSQL
+will listen on this port on the same addresses as for regular
+connections, but expect all connections to use the PROXY protocol to
+identify the client.  This parameter can only be set at server start.
+   
+   
+If a proxy connection is made over this port, and the proxy is listed
+in , the actual client address
+will be considered as the address of the client, instead of listing
+all connections as coming from the proxy server.
+   
+   
+ The http://www.haproxy.org/download/1.9/doc/proxy-protocol.txt";>PROXY
+ protocol is maintained by HAProxy,
+ and supported in many proxies and load
+ balancers. PostgreSQL supports version 2
+ of the protocol.
+   
+  
+ 
+
+ 
+  proxy_servers (string)
+  
+   proxy_servers configuration parameter
+  
+  
+  
+   
+A comma separated list of one or more ip addresses, cidr specifications or the
+literal unix, indicating which proxy servers to trust when
+connecting on the port specified in .
+   
+   
+If a proxy connection is made from an IP address not covered by this
+list, the connection will be rejected. By default no proxy is trusted
+and all proxy conne

Re: PROXY protocol support

[Jacob Champion]

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@formilux.org/msg40899.html

Re: PROXY protocol support

[Magnus Hagander]

On Wed, Nov 3, 2021 at 2:36 PM Daniel Gustafsson  wrote:

> > On 28 Sep 2021, at 15:23, Magnus Hagander  wrote:
> > On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion 
> 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/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 02f0489112..a3ff09b3ac 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index de77f14573..5211c1f7b1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,56 @@ include_dir 'conf.d'
   
  
 
+ 
+  proxy_port (integer)
+  
+   proxy_port configuration parameter
+  
+  
+  
+   
+The TCP port the server listens on for PROXY connections, disabled by
+default. If set to a number, PostgreSQL
+will listen on this port on the same addresses as for regular
+connections, but expect all connections to use the PROXY protocol to
+identify the client.  This parameter can only be set at server start.
+   
+   
+If a proxy connection is made over this port, and the proxy is listed
+in , the actual client address
+will be considered as the address of the client, instead of listing
+all connections as coming from the proxy server.
+   
+   
+ The http://www.haproxy.org/download/1.9/doc/proxy-protocol.txt";>PROXY
+ protocol is maintained by HAProxy,
+ and supported in many proxies and load
+ balancers. PostgreSQL supports version 2
+ of the protocol.
+   
+  
+ 
+
+ 
+  proxy_servers (string)
+  
+   proxy_servers configuration parameter
+  
+  
+  
+   
+A comma separated list of one or more ip addresses, cidr specifications or the
+literal unix, indicating which proxy servers to trust when
+connecting on the port specified in .
+   
+   
+If a proxy connection is made from an IP address not covered by this
+list, the connection will be rejected. By default no proxy is trusted
+and all proxy connections will be rejected.
+   
+  
+ 
+
  
   max_connections (integer)
   
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4b49dff2ff..74f38d4891 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -22238,7 +22238,12 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
 connection,
 or NULL if the current connection is via a
 Unix-domain socket.
-   
+   
+   
+If the connection is a PROXY connection, this function returns the
+IP address used to connect to the proxy server.
+   
+   
   
 
   
@@ -22254,7 +22259,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
 connection,
 or NULL if the current connection is via a
 Unix-domain socket.
-   
+   
+   
+If the connection is a PROXY connection, this function returns the
+port used to connect to the proxy server.
+   
+
+   
   
 
   
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index a317aef1c9..f8c32ad492 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1696,6 +1696,14 @@ ident_inet(hbaPort *port)
 			   *la = NULL,
 hints;
 
+	if (port->isProxy)
+	{
+		ereport(LOG,
+(errcode_for_socket_access(),
+ errmsg("Ident authentication cannot be used over PROXY connections")));
+		return STATUS_ERROR;
+	}
+
 	/*
 	 * Might look a little weird to first convert it to text and then back to
 	 * sockaddr, but it's protocol independent.
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 89a5f901aa..a8d6c5fa4c 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -311,13 +311,13 @@ socket_close(int code, Datum arg)
  * Successfully opened sockets are added to the ListenSocket[] array (of
  * length MaxListen), at the first position that isn't PGINVALID_SOCKET.
  *
- * RETURNS: STATUS_OK or STATUS_ERROR
+

Re: PROXY protocol support

[Daniel Gustafsson]

> On 28 Sep 2021, at 15:23, Magnus Hagander  wrote:
> On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion  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/

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Sep 10, 2021 at 1:44 AM Jacob Champion  wrote:
>
> On Wed, 2021-09-08 at 18:51 +, 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 chec

Re: PROXY protocol support

[Jacob Champion]

On Wed, 2021-09-08 at 18:51 +, 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

Re: PROXY protocol support

[Jacob Champion]

On Tue, 2021-09-07 at 12:24 +0200, Magnus Hagander wrote:
> On Wed, Jul 14, 2021 at 8:24 PM Jacob Champion  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

Re: PROXY protocol support

[Magnus Hagander]

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

Re: PROXY protocol support

[Jacob Champion]

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

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Jul 9, 2021 at 1:42 AM Jacob Champion  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  
> > 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..


> > +  proxy_servers (string)
> > +  
> > +   proxy_servers configuration 
> > parameter
> > +  
> > +  
> > +  
> > +   
> > +A comma separated list of one or more host names, cidr 
> > specifications or the
> > +literal unix, indicating which proxy servers to 
> > trust when
> > +connecting on the port specified in  > />.
>
> 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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 02f0489112..a3ff09b3ac 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 381d8636ab..778a20a179 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,56 @@ include_dir 'conf.d'
 

Re: PROXY protocol support

[Jacob Champion]

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

> +  proxy_servers (string)
> +  
> +   proxy_servers configuration 
> parameter
> +  
> +  
> +  
> +   
> +A comma separated list of one or more host names, cidr 
> specifications or the
> +literal unix, indicating which proxy servers to 
> trust when
> +connecting on the port specified in  />.

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

Re: PROXY protocol support

[Magnus Hagander]

On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion  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( 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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 02f0489112..a3ff09b3ac 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6098f6b020..c8a7d2a3b7 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,56 @@ include_dir 'conf.d'
   
  
 
+ 
+  proxy_port (integer)
+  
+   proxy_port configuration parameter
+  
+  
+  
+   
+The TCP port the server listens on for PROXY connections, disabled by
+default. If set to a number, PostgreSQL
+will listen on this port on the same addresses as for regular
+connections, but expect all connections to use the PROXY protocol to
+identify the client.  This parameter can only be set at server start.
+   
+   
+If a proxy connection is done over this port, and the proxy is listed
+in , the actual client address
+will be considered as the address of the client, instead of listing
+all connections as coming from the proxy server.
+   
+   
+ The http://www.haproxy.org/download/1.9/doc/proxy-protocol.txt";>PROXY
+ protocol is maintained by HAProxy,
+ and supported in many proxies and load
+ balancers. PostgreSQL supports version 2
+ of the protocol.
+   
+ 

Re: PROXY protocol support

[Magnus Hagander]

On Tue, Mar 9, 2021 at 11:25 AM Magnus Hagander  wrote:
>
> On Sat, Mar 6, 2021 at 5:30 PM Magnus Hagander  wrote:
> >
> > On Sat, Mar 6, 2021 at 4:17 PM Magnus Hagander  wrote:
> > >
> > > On Fri, Mar 5, 2021 at 8:11 PM Jacob Champion  
> > > wrote:
> > > >
> > > > On Fri, 2021-03-05 at 10:22 +0100, Magnus Hagander wrote:
> > > > > On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion  
> > > > > 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 a

Re: PROXY protocol support

[Jacob Champion]

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() == 12 and so on? That would also save any poor souls who are
using compilers with nonstandard struct-packing behavior.

--Jacob

Re: PROXY protocol support

[Magnus Hagander]

On Sat, Mar 6, 2021 at 5:30 PM Magnus Hagander  wrote:
>
> On Sat, Mar 6, 2021 at 4:17 PM Magnus Hagander  wrote:
> >
> > On Fri, Mar 5, 2021 at 8:11 PM Jacob Champion  wrote:
> > >
> > > On Fri, 2021-03-05 at 10:22 +0100, Magnus Hagander wrote:
> > > > On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion  
> > > > 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).
> >

Re: PROXY protocol support

[Magnus Hagander]

On Sat, Mar 6, 2021 at 4:17 PM Magnus Hagander  wrote:
>
> On Fri, Mar 5, 2021 at 8:11 PM Jacob Champion  wrote:
> >
> > On Fri, 2021-03-05 at 10:22 +0100, Magnus Hagander wrote:
> > > On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion  
> > > 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 p

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Mar 5, 2021 at 8:11 PM Jacob Champion  wrote:
>
> On Fri, 2021-03-05 at 10:22 +0100, Magnus Hagander wrote:
> > On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion  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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml

Re: PROXY protocol support

[Jacob Champion]

On Fri, 2021-03-05 at 10:22 +0100, Magnus Hagander wrote:
> On Fri, Mar 5, 2021 at 12:21 AM Jacob Champion  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

Re: PROXY protocol support

[Álvaro Hernández]

On 5/3/21 10:03, Magnus Hagander wrote:
> On Fri, Mar 5, 2021 at 1:33 AM Álvaro Hernández  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  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

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Mar 5, 2021 at 12:21 AM 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  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/

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Mar 5, 2021 at 12:08 AM Tatsuo Ishii  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/

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Mar 5, 2021 at 1:33 AM Álvaro Hernández  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  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/

Re: PROXY protocol support

[Magnus Hagander]

On Fri, Mar 5, 2021 at 12:57 AM Hannu Krosing  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/

Re: PROXY protocol support

[Álvaro Hernández]

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

Re: PROXY protocol support

[Hannu Krosing]

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  wrote:
>
> On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
> > On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion  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

Re: PROXY protocol support

[Jacob Champion]

On Thu, 2021-03-04 at 21:45 +0100, Magnus Hagander wrote:
> On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion  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

Re: PROXY protocol support

[Tatsuo Ishii]

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

Re: PROXY protocol support

[Magnus Hagander]

On Thu, Mar 4, 2021 at 10:01 PM Jan Wieck  wrote:
>
> On 3/4/21 3:40 PM, Magnus Hagander wrote:
> > On Thu, Mar 4, 2021 at 9:29 PM Jan Wieck  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/

Re: PROXY protocol support

[Jan Wieck]

On 3/4/21 3:40 PM, Magnus Hagander wrote:

On Thu, Mar 4, 2021 at 9:29 PM Jan Wieck  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

Re: PROXY protocol support

[Magnus Hagander]

On Thu, Mar 4, 2021 at 8: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

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/

Re: PROXY protocol support

[Magnus Hagander]

On Thu, Mar 4, 2021 at 9:07 PM Jacob Champion  wrote:
>
> On Wed, 2021-03-03 at 10:39 +0100, Magnus Hagander wrote:
> > On Wed, Mar 3, 2021 at 10:00 AM Magnus Hagander  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/

Re: PROXY protocol support

[Magnus Hagander]

On Thu, Mar 4, 2021 at 9:29 PM Jan Wieck  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/

Re: PROXY protocol support

[Jan Wieck]

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

Re: PROXY protocol support

[Jacob Champion]

On Wed, 2021-03-03 at 10:39 +0100, Magnus Hagander wrote:
> On Wed, Mar 3, 2021 at 10:00 AM Magnus Hagander  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

Re: PROXY protocol support

[Jacob Champion]

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

Re: PROXY protocol support

[Tatsuo Ishii]

> 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

Re: PROXY protocol support

[Bruno Lavoie]

+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  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/
>

Re: PROXY protocol support

[Magnus Hagander]

On Wed, Mar 3, 2021 at 10:00 AM Magnus Hagander  wrote:
>
> On Wed, Mar 3, 2021 at 1:50 AM Jacob Champion  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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b420486a0a..d4f6fad5b0 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b5718fc136..fc7de25378 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,30 @@ include_dir 'conf.d'
   
  
 
+ 
+  proxy_servers (string)
+  
+   proxy_servers configuration parameter
+  
+  
+  
+   
+A comma separated list of one or more host names or cidr specifications
+of proxy servers to trust. If a connection using the PROXY protocol is made
+from one of these IP addresses, PostgreSQL will
+read the client IP address from the PROXY header and consider that the
+address of the client, instead of listing all connections as coming from
+the proxy server.
+   
+   
+If a proxy connection is made from an IP address not covered by this
+list, the connection will be rejected. By default no proxy is trusted
+and all proxy connections will be rejected.  This parameter can only
+be set at server start.
+   
+  
+ 
+
  
   max_connections (integer)
   
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 27a298f110..401f2d2464 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -53,6 +53,7 @@
  *		pq_getmessage	- get a message with length word from connection
  *		pq_getbyte		- get next byte from connection
  *		pq_peekbyte		- peek at next byte from connection
+ *		pq_peekbytes- peek at a known number of bytes from connection
  *		pq_putbyt

Re: PROXY protocol support

[Magnus Hagander]

On Wed, Mar 3, 2021 at 1:50 AM Jacob Champion  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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b420486a0a..d4f6fad5b0 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b5718fc136..fc7de25378 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,30 @@ include_dir 'conf.d'
   
  
 
+ 
+  proxy_servers (string)
+  
+   proxy_servers configuration parameter
+  
+  
+  
+   
+A comma separated list of one or more host names or cidr specifications
+of proxy servers to trust. If a connection using the PROXY protocol is made
+from one of these IP addresses, PostgreSQL will
+read the client IP address from the PROXY header and consider that the
+address of the client, instead of listing all connections as coming from
+the proxy server.
+   
+   
+If a proxy connection is made from an IP address not covered by this
+list, the connection will be rejected. By default no proxy is trusted
+and all proxy connections will be rejected.  This parameter can only
+be set at server start.
+   
+  
+ 
+
  
   max_connections (integer)
   
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 27a298f110..9163761cc2 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -53,6 +53,7 @@
  *		pq_getmessage	- get a message with length word from connection
  *		pq_getbyte		- get next byte from connection
  *		pq_peekbyte		- peek at next byte from connection
+ *		pq_peekbytes- peek at a known number of bytes from connection
  *		pq_putbytes		- send bytes to connection (not flushed until pq_flush)
  *		pq_flush		- flush pending output
  *		pq_flush_if_writable - flush pending output if writable without blocking
@@ -1039,6 +1040,27 @@ pq_peekbyte(void)
 	return (unsigned char) PqRecvBuffer[PqRecvPointer];
 }
 
+
+/* 
+ *		pq_peekbytes	

Re: PROXY protocol support

[Jacob Champion]

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

Re: PROXY protocol support

[Arthur Nascimento]

Hi,

On Tue, 2 Mar 2021 at 14:43, Magnus Hagander  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

PROXY protocol support

[Magnus Hagander]

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/
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b420486a0a..d4f6fad5b0 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -353,6 +353,15 @@ hostnogssenc  database  user
 
+  
+   If  is enabled and the
+   connection is made through a proxy server using the PROXY
+   protocol, the actual IP address of the client will be used
+   for matching. If a connection is made through a proxy server
+   not using the PROXY protocol, the IP address of the
+   proxy server will be used.
+  
+
   
These fields do not apply to local records.
   
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b5718fc136..fc7de25378 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -682,6 +682,30 @@ include_dir 'conf.d'
   
  
 
+ 
+  proxy_servers (string)
+  
+   proxy_servers configuration parameter
+  
+  
+  
+   
+A comma separated list of one or more host names or cidr specifications
+of proxy servers to trust. If a connection using the PROXY protocol is made
+from one of these IP addresses, PostgreSQL will
+read the client IP address from the PROXY header and consider that the
+address of the client, instead of listing all connections as coming from
+the proxy server.
+   
+   
+If a proxy connection is made from an IP address not covered by this
+list, the connection will be rejected. By default no proxy is trusted
+and all proxy connections will be rejected.  This parameter can only
+be set at server start.
+   
+  
+ 
+
  
   max_connections (integer)
   
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 27a298f110..9163761cc2 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -53,6 +53,7 @@
  *		pq_getmessage	- get a message with length word from connection
  *		pq_getbyte		- get next byte from connection
  *		pq_peekbyte		- peek at next byte from connection
+ *		pq_peekbytes- peek at a known number of bytes from connection
  *		pq_putbytes		- send bytes to connection (not flushed until pq_flush)
  *		pq_flush		- flush pending output
  *		pq_flush_if_writable - flush pending output if writable without blocking
@@ -1039,6 +1040,27 @@ pq_peekbyte(void)
 	return (unsigned char) PqRecvBuffer[PqRecvPointer];
 }
 
+
+/* 
+ *		pq_peekbytes		- peek at a known number of bytes from connection.
+ *			  Note! Does NOT wait for more data to arrive.
+ *
+ *		returns 0 if OK, EOF if trouble
+ * 
+ */
+int
+pq_peekbytes(char *s, size_t len)
+{
+	Assert(PqCommReadingMsg);
+
+	if (PqRecvLength - PqRecvPointer < len)
+		return EOF;
+
+	memcpy(s, PqRecvBuffer + PqRecvPointer, len);
+
+	return 0;
+}
+
 /* 
  *		pq_getbyte_if_available - get a single byte from connection,
  *			if available
@@ -1135,7 +1157,7 @@ pq_getbytes(char *s, size_t len)
  *		returns 0 if OK, EOF if trouble
  * --

Re: PROXY protocol support

[Bruno Lavoie]

+1 on this one...

MySQL and derivatives support it very well.. it is a  standard that can be
used with either haproxy or better, ProxySQL.

Would be nice to have it in core.

It is a show stopper for us to use proxying because of compliance and
tracability reasons.



Le dim. 19 mai 2019 11:36 AM, Julien Riou  a écrit :

> Hello,
>
> Nowadays, PostgreSQL is often used behind proxies. Some are PostgreSQL
> protocol aware (Pgpool, PgBouncer), some are pure TCP (HAProxy). From
> the database instance point of view, all clients come from the proxy.
>
> There are two major problems with this topology:
>
> * It neutralizes the host based authentication. Every client shares
> the same source. Either we allow this source or not but we cannot allow
> clients on a more fine-grained basis, or not by the IP address.
>
> * It makes debugging harder. If we have a DDL or a slow query logged, we
> cannot use the source to identify who is responsible.
>
> On one hand, we can move the authentication and logging mechanisms to
> PostgreSQL based proxies but they will never be as complete as
> PostgreSQL itself. And they don't have features like HTTP health checks
> to redirect trafic to nodes (health, role, whatever behind the URL). On
> the other hand, those features are not implemented at all because they
> don't know the PostgreSQL protocol, they simply forward requests.
>
> In the HTTP reverse proxies world, there's a "dirty hack" to identify
> the source IP address: add an HTTP header "X-Forwared-For" to the
> request. It's the destination duty to do whatever they want with this
> information. With this feature in mind, someone from HAProxy has
> implemented this mechanism at the protocol level. It's called the PROXY
> protocol.
>
> With this piece of logic at the beginning of the protocol, we could
> implement a totally transparent proxy and benefit from the great
> features of PostgreSQL regarding clients. Note that MariaDB support the
> PROXY protocol in MaxScale (proxy) and MariaDB Server in recent
> versions.
>
> My question is, what do you think of this feature? Is it worth to spend
> time implementing it in PostgreSQL or not?
>
> Links:
>  - http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
>  - https://mariadb.com/kb/en/library/proxy-protocol-support/
>
> Thanks,
> Julien
>
> PS: I've already sent this message to a wrong mailing list. Stephen
> Frost said it's implemented in pgbouncer but all I can find is an open
> issue: https://github.com/pgbouncer/pgbouncer/issues/241.
>
>
>

Re: PROXY protocol support

[Konstantin Knizhnik]

On 19.05.2019 18:36, Julien Riou wrote:

Hello,

Nowadays, PostgreSQL is often used behind proxies. Some are PostgreSQL
protocol aware (Pgpool, PgBouncer), some are pure TCP (HAProxy). From
the database instance point of view, all clients come from the proxy.

There are two major problems with this topology:

* It neutralizes the host based authentication. Every client shares
the same source. Either we allow this source or not but we cannot allow
clients on a more fine-grained basis, or not by the IP address.

* It makes debugging harder. If we have a DDL or a slow query logged, we
cannot use the source to identify who is responsible.

On one hand, we can move the authentication and logging mechanisms to
PostgreSQL based proxies but they will never be as complete as
PostgreSQL itself. And they don't have features like HTTP health checks
to redirect trafic to nodes (health, role, whatever behind the URL). On
the other hand, those features are not implemented at all because they
don't know the PostgreSQL protocol, they simply forward requests.

In the HTTP reverse proxies world, there's a "dirty hack" to identify
the source IP address: add an HTTP header "X-Forwared-For" to the
request. It's the destination duty to do whatever they want with this
information. With this feature in mind, someone from HAProxy has
implemented this mechanism at the protocol level. It's called the PROXY
protocol.

With this piece of logic at the beginning of the protocol, we could
implement a totally transparent proxy and benefit from the great
features of PostgreSQL regarding clients. Note that MariaDB support the
PROXY protocol in MaxScale (proxy) and MariaDB Server in recent
versions.

My question is, what do you think of this feature? Is it worth to spend
time implementing it in PostgreSQL or not?

Links:
  - http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
  - https://mariadb.com/kb/en/library/proxy-protocol-support/

Thanks,
Julien

PS: I've already sent this message to a wrong mailing list. Stephen
Frost said it's implemented in pgbouncer but all I can find is an open
issue: https://github.com/pgbouncer/pgbouncer/issues/241.




Hi,
From my point of view it will be better to support embedded connection 
pooler in Postgres.
In this case all mentioned problems can be more or less 
straightforwardly solved without inventing new protocol.
There is my prototype implementation of built-in connection pooler on 
commit-fest:

https://commitfest.postgresql.org/23/2067/


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: PROXY protocol support

[Julien Riou]

On May 19, 2019 5:59:04 PM GMT+02:00, Stephen Frost  wrote:
>Greetings,
>
>* Julien Riou (jul...@riou.xyz) wrote:
>> Nowadays, PostgreSQL is often used behind proxies. Some are
>PostgreSQL
>> protocol aware (Pgpool, PgBouncer), some are pure TCP (HAProxy). From
>> the database instance point of view, all clients come from the proxy.
>> 
>> There are two major problems with this topology:
>> 
>> * It neutralizes the host based authentication. Every client shares
>> the same source. Either we allow this source or not but we cannot
>allow
>> clients on a more fine-grained basis, or not by the IP address.
>
>You can instead have the IP-based checking done at the pooler.
>
>> * It makes debugging harder. If we have a DDL or a slow query logged,
>we
>> cannot use the source to identify who is responsible.
>
>Protocol-level poolers are able to do this, and pgbouncer does (see
>application_name_add_host).
>
>> On one hand, we can move the authentication and logging mechanisms to
>> PostgreSQL based proxies but they will never be as complete as
>> PostgreSQL itself. And they don't have features like HTTP health
>checks
>> to redirect trafic to nodes (health, role, whatever behind the URL).
>On
>> the other hand, those features are not implemented at all because
>they
>> don't know the PostgreSQL protocol, they simply forward requests.
>> 
>> In the HTTP reverse proxies world, there's a "dirty hack" to identify
>> the source IP address: add an HTTP header "X-Forwared-For" to the
>> request. It's the destination duty to do whatever they want with this
>> information. With this feature in mind, someone from HAProxy has
>> implemented this mechanism at the protocol level. It's called the
>PROXY
>> protocol.
>
>Someone from HAProxy could certainly implement something similar by
>having HAProxy understand PostgreSQL's protocol.
>
>> With this piece of logic at the beginning of the protocol, we could
>> implement a totally transparent proxy and benefit from the great
>> features of PostgreSQL regarding clients. Note that MariaDB support
>the
>> PROXY protocol in MaxScale (proxy) and MariaDB Server in recent
>> versions.
>
>pgbouncer is already a transparent proxy that understands the PG
>protocol, and, even better, it has support for transaction-level
>pooling
>(as well as connection-level), which is really critical for larger PG
>deployments as PG backend startup is (relatively) expensive.
>
>> PS: I've already sent this message to a wrong mailing list. Stephen
>> Frost said it's implemented in pgbouncer but all I can find is an
>open
>> issue: https://github.com/pgbouncer/pgbouncer/issues/241.
>
>That would be some *other* proxy system (Amazon's ELB) that apparently
>also doesn't understand the PG protocol and therefore doesn't have a
>feature similar to pgbouncer's application_name_add_host.
>
>I haven't looked very closely at if it'd be possible to interpret the
>PROXY protocol thing that Amazon's ELB can do without confusing it with
>a regular PG authentication startup and I'm not sure if we'd really
>want
>to wed ourselves to something like that.  Certainly, what pgbouncer
>does
>works quite well and is about as transparent to clients as possible.
>
>You'd almost certainly want something like pgbouncer after the ELB
>anyway to avoid having tons of connections to PG and avoid spinning up
>new backends constantly.
>
>Thanks,
>
>Stephen

It could be proprietary Amazon load balancers I don't have experience with, or 
simple HAProxy coupled with a Patroni HTTP API to tell if a backend is healthy 
or not.

The PgBouncer approach is interesting. I'm already using the application name 
as a workaround to identify containerized applications but didn't used it for 
setting the source IP.

If we take a look at the MariaDB implementation, they check for errors in the 
startup packet then run the PROXY protocol decoding then return a real error if 
it doesn't work. As our bouncers are all behind a pool of HAProxy, and if we 
consider PgBouncer as a trusted extension of PostgreSQL, maybe implementing it 
in PgBouncer first will be easier.

Thanks for your insightful comments.
Julien

Re: PROXY protocol support

[Stephen Frost]

Greetings,

* Julien Riou (jul...@riou.xyz) wrote:
> Nowadays, PostgreSQL is often used behind proxies. Some are PostgreSQL
> protocol aware (Pgpool, PgBouncer), some are pure TCP (HAProxy). From
> the database instance point of view, all clients come from the proxy.
> 
> There are two major problems with this topology:
> 
> * It neutralizes the host based authentication. Every client shares
> the same source. Either we allow this source or not but we cannot allow
> clients on a more fine-grained basis, or not by the IP address.

You can instead have the IP-based checking done at the pooler.

> * It makes debugging harder. If we have a DDL or a slow query logged, we
> cannot use the source to identify who is responsible.

Protocol-level poolers are able to do this, and pgbouncer does (see
application_name_add_host).

> On one hand, we can move the authentication and logging mechanisms to
> PostgreSQL based proxies but they will never be as complete as
> PostgreSQL itself. And they don't have features like HTTP health checks
> to redirect trafic to nodes (health, role, whatever behind the URL). On
> the other hand, those features are not implemented at all because they
> don't know the PostgreSQL protocol, they simply forward requests.
> 
> In the HTTP reverse proxies world, there's a "dirty hack" to identify
> the source IP address: add an HTTP header "X-Forwared-For" to the
> request. It's the destination duty to do whatever they want with this
> information. With this feature in mind, someone from HAProxy has
> implemented this mechanism at the protocol level. It's called the PROXY
> protocol.

Someone from HAProxy could certainly implement something similar by
having HAProxy understand PostgreSQL's protocol.

> With this piece of logic at the beginning of the protocol, we could
> implement a totally transparent proxy and benefit from the great
> features of PostgreSQL regarding clients. Note that MariaDB support the
> PROXY protocol in MaxScale (proxy) and MariaDB Server in recent
> versions.

pgbouncer is already a transparent proxy that understands the PG
protocol, and, even better, it has support for transaction-level pooling
(as well as connection-level), which is really critical for larger PG
deployments as PG backend startup is (relatively) expensive.

> PS: I've already sent this message to a wrong mailing list. Stephen
> Frost said it's implemented in pgbouncer but all I can find is an open
> issue: https://github.com/pgbouncer/pgbouncer/issues/241.

That would be some *other* proxy system (Amazon's ELB) that apparently
also doesn't understand the PG protocol and therefore doesn't have a
feature similar to pgbouncer's application_name_add_host.

I haven't looked very closely at if it'd be possible to interpret the
PROXY protocol thing that Amazon's ELB can do without confusing it with
a regular PG authentication startup and I'm not sure if we'd really want
to wed ourselves to something like that.  Certainly, what pgbouncer does
works quite well and is about as transparent to clients as possible.

You'd almost certainly want something like pgbouncer after the ELB
anyway to avoid having tons of connections to PG and avoid spinning up
new backends constantly.

Thanks,

Stephen


signature.asc
Description: PGP signature

PROXY protocol support

[Julien Riou]

Hello,

Nowadays, PostgreSQL is often used behind proxies. Some are PostgreSQL
protocol aware (Pgpool, PgBouncer), some are pure TCP (HAProxy). From
the database instance point of view, all clients come from the proxy.

There are two major problems with this topology:

* It neutralizes the host based authentication. Every client shares
the same source. Either we allow this source or not but we cannot allow
clients on a more fine-grained basis, or not by the IP address.

* It makes debugging harder. If we have a DDL or a slow query logged, we
cannot use the source to identify who is responsible.

On one hand, we can move the authentication and logging mechanisms to
PostgreSQL based proxies but they will never be as complete as
PostgreSQL itself. And they don't have features like HTTP health checks
to redirect trafic to nodes (health, role, whatever behind the URL). On
the other hand, those features are not implemented at all because they
don't know the PostgreSQL protocol, they simply forward requests.

In the HTTP reverse proxies world, there's a "dirty hack" to identify
the source IP address: add an HTTP header "X-Forwared-For" to the
request. It's the destination duty to do whatever they want with this
information. With this feature in mind, someone from HAProxy has
implemented this mechanism at the protocol level. It's called the PROXY
protocol.

With this piece of logic at the beginning of the protocol, we could
implement a totally transparent proxy and benefit from the great
features of PostgreSQL regarding clients. Note that MariaDB support the
PROXY protocol in MaxScale (proxy) and MariaDB Server in recent
versions.

My question is, what do you think of this feature? Is it worth to spend
time implementing it in PostgreSQL or not?

Links:
 - http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
 - https://mariadb.com/kb/en/library/proxy-protocol-support/

Thanks,
Julien

PS: I've already sent this message to a wrong mailing list. Stephen
Frost said it's implemented in pgbouncer but all I can find is an open
issue: https://github.com/pgbouncer/pgbouncer/issues/241.