pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedSSL SNI
24+ messages / 9 participants
[nested] [flat]
* SSL SNI
@ 2021-02-15 14:09 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 4 replies; 24+ messages in thread
From: Peter Eisentraut @ 2021-02-15 14:09 UTC (permalink / raw)
To: pgsql-hackers
A customer asked about including Server Name Indication (SNI) into the
SSL connection from the client, so they can use an SSL-aware proxy to
route connections. There was a thread a few years ago where this was
briefly discussed but no patch appeared.[0] I whipped up a quick patch
and it did seem to do the job, so I figured I'd share it here.
The question I had was whether this should be an optional behavior, or
conversely a behavior that can be turned off, or whether it should just
be turned on all the time.
Technically, it seems pretty harmless. It adds another field to the TLS
handshake, and if the server is not interested in it, it just gets ignored.
The Wikipedia page[1] discusses some privacy concerns in the context of
web browsing, but it seems there is no principled solution to those.
The relevant RFC[2] "recommends" that SNI is used for all applicable TLS
connections.
[0]:
https://www.postgresql.org/message-id/flat/CAPPwrB_tsOw8MtVaA_DFyOFRY2ohNdvMnLoA_JRr3yB67Rggmg%40mai...
[1]: https://en.wikipedia.org/wiki/Server_Name_Indication
[2]: https://tools.ietf.org/html/rfc6066#section-3
From 3e4aae8b01a05fe78017659bb3be1942a7f1ccf5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Feb 2021 14:11:27 +0100
Subject: [PATCH] Set SNI for SSL connections from the client
This allows an SNI-aware proxy to route connections.
---
src/interfaces/libpq/fe-secure-openssl.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 5b4a4157d5..6e439679e5 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1066,6 +1066,25 @@ initialize_SSL(PGconn *conn)
SSL_CTX_free(SSL_context);
SSL_context = NULL;
+ /*
+ * Set Server Name Indication (SNI), but not if it's a literal IP address.
+ * (RFC 6066)
+ */
+ if (!((conn->pghost[0] >= '0' && conn->pghost[0] <= '9') || strchr(conn->pghost, ':')))
+ {
+ if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
--
2.30.0
Attachments:
[text/plain] 0001-Set-SNI-for-SSL-connections-from-the-client.patch (1.4K, ../../7289d5eb-62a5-a732-c3b9-438cee2cb709@enterprisedb.com/2-0001-Set-SNI-for-SSL-connections-from-the-client.patch)
download | inline diff:
From 3e4aae8b01a05fe78017659bb3be1942a7f1ccf5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Feb 2021 14:11:27 +0100
Subject: [PATCH] Set SNI for SSL connections from the client
This allows an SNI-aware proxy to route connections.
---
src/interfaces/libpq/fe-secure-openssl.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 5b4a4157d5..6e439679e5 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1066,6 +1066,25 @@ initialize_SSL(PGconn *conn)
SSL_CTX_free(SSL_context);
SSL_context = NULL;
+ /*
+ * Set Server Name Indication (SNI), but not if it's a literal IP address.
+ * (RFC 6066)
+ */
+ if (!((conn->pghost[0] >= '0' && conn->pghost[0] <= '9') || strchr(conn->pghost, ':')))
+ {
+ if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
--
2.30.0
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-15 14:28 Matthias van de Meent <boekewurm+postgres@gmail.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
3 siblings, 1 reply; 24+ messages in thread
From: Matthias van de Meent @ 2021-02-15 14:28 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: pgsql-hackers
On Mon, 15 Feb 2021 at 15:09, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> A customer asked about including Server Name Indication (SNI) into the
> SSL connection from the client, so they can use an SSL-aware proxy to
> route connections. There was a thread a few years ago where this was
> briefly discussed but no patch appeared.[0] I whipped up a quick patch
> and it did seem to do the job, so I figured I'd share it here.
The same topic of SSL-aware proxying based on SNI was mentioned in a
more recent thread here [0]. The state of that patch is unclear,
though. Other than that, this feature seems useful.
+ /*
+ * Set Server Name Indication (SNI), but not if it's a literal IP address.
+ * (RFC 6066)
+ */
+ if (!((conn->pghost[0] >= '0' && conn->pghost[0] <= '9') ||
strchr(conn->pghost, ':')))
'1one.example.com' is a valid hostname, but would fail this trivial
test, and thus would not have SNI enabled on its connection.
With regards,
Matthias van de Meent
[0] https://www.postgresql.org/message-id/flat/37846a5e-bb5e-0c4f-3ee8-54fb4bd02fab%40gmx.de
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-15 17:40 Jesse Zhang <sbjesse@gmail.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
3 siblings, 1 reply; 24+ messages in thread
From: Jesse Zhang @ 2021-02-15 17:40 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: pgsql-hackers
Hi Peter,
I imagine this also (finally) opens up the possibility for the server
to present a different certificate for each hostname based on SNI.
This eliminates the requirement for wildcard certs where the cluster
is running on a host with multiple (typically two to three) hostnames
and the clients check the hostname against SAN in the cert
(sslmode=verify-full). Am I right? Is that feature on anybody's
roadmap?
Cheers,
Jesse
On Mon, Feb 15, 2021 at 6:09 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> A customer asked about including Server Name Indication (SNI) into the
> SSL connection from the client, so they can use an SSL-aware proxy to
> route connections. There was a thread a few years ago where this was
> briefly discussed but no patch appeared.[0] I whipped up a quick patch
> and it did seem to do the job, so I figured I'd share it here.
>
> The question I had was whether this should be an optional behavior, or
> conversely a behavior that can be turned off, or whether it should just
> be turned on all the time.
>
> Technically, it seems pretty harmless. It adds another field to the TLS
> handshake, and if the server is not interested in it, it just gets ignored.
>
> The Wikipedia page[1] discusses some privacy concerns in the context of
> web browsing, but it seems there is no principled solution to those.
> The relevant RFC[2] "recommends" that SNI is used for all applicable TLS
> connections.
>
>
> [0]:
> https://www.postgresql.org/message-id/flat/CAPPwrB_tsOw8MtVaA_DFyOFRY2ohNdvMnLoA_JRr3yB67Rggmg%40mai...
> [1]: https://en.wikipedia.org/wiki/Server_Name_Indication
> [2]: https://tools.ietf.org/html/rfc6066#section-3
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-15 19:24 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Jesse Zhang <sbjesse@gmail.com>
0 siblings, 0 replies; 24+ messages in thread
From: Peter Eisentraut @ 2021-02-15 19:24 UTC (permalink / raw)
To: Jesse Zhang <sbjesse@gmail.com>; +Cc: pgsql-hackers
On 2021-02-15 18:40, Jesse Zhang wrote:
> I imagine this also (finally) opens up the possibility for the server
> to present a different certificate for each hostname based on SNI.
> This eliminates the requirement for wildcard certs where the cluster
> is running on a host with multiple (typically two to three) hostnames
> and the clients check the hostname against SAN in the cert
> (sslmode=verify-full). Am I right? Is that feature on anybody's
> roadmap?
This would be the client side of that. But I don't know of anyone
planning to work on the server side.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-16 23:01 Jacob Champion <pchampion@vmware.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
3 siblings, 1 reply; 24+ messages in thread
From: Jacob Champion @ 2021-02-16 23:01 UTC (permalink / raw)
To: peter.eisentraut@enterprisedb.com <peter.eisentraut@enterprisedb.com>; pgsql-hackers
On Mon, 2021-02-15 at 15:09 +0100, Peter Eisentraut wrote:
> The question I had was whether this should be an optional behavior, or
> conversely a behavior that can be turned off, or whether it should just
> be turned on all the time.
Personally I think there should be a toggle, so that any users for whom
hostnames are potentially sensitive don't have to make that information
available on the wire. Opt-in, to avoid having any new information
disclosure after a version upgrade?
> The Wikipedia page[1] discusses some privacy concerns in the context of
> web browsing, but it seems there is no principled solution to those.
I think Encrypted Client Hello is the new-and-improved Encrypted SNI,
and it's on the very bleeding edge. You'd need to load a public key
into the client using some out-of-band communication -- e.g. browsers
would use DNS-over-TLS, but it might not make sense for a Postgres
client to use that same system.
NSS will probably be receiving any final implementation before OpenSSL,
if I had to guess, since Mozilla is driving pieces of the
implementation.
--Jacob
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-25 15:58 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Matthias van de Meent <boekewurm+postgres@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-02-25 15:58 UTC (permalink / raw)
To: Matthias van de Meent <boekewurm+postgres@gmail.com>; +Cc: pgsql-hackers
On 15.02.21 15:28, Matthias van de Meent wrote:
> + /*
> + * Set Server Name Indication (SNI), but not if it's a literal IP address.
> + * (RFC 6066)
> + */
> + if (!((conn->pghost[0] >= '0' && conn->pghost[0] <= '9') ||
> strchr(conn->pghost, ':')))
>
> '1one.example.com' is a valid hostname, but would fail this trivial
> test, and thus would not have SNI enabled on its connection.
Here is an updated patch that fixes this. If there are other ideas for
how to tell apart literal IP addresses from host names that are less ad
hoc, I would welcome them.
From bef8152b6f4ad2ed2ccd8158088a35b2cf625491 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 25 Feb 2021 16:40:32 +0100
Subject: [PATCH v2] Set SNI for SSL connections from the client
This allows an SNI-aware proxy to route connections.
Discussion: https://www.postgresql.org/message-id/flat/7289d5eb-62a5-a732-c3b9-438cee2cb709%40enterprisedb.com
---
src/interfaces/libpq/fe-secure-openssl.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0fa10a23b4..889213c994 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1076,6 +1076,26 @@ initialize_SSL(PGconn *conn)
SSL_CTX_free(SSL_context);
SSL_context = NULL;
+ /*
+ * Set Server Name Indication (SNI), but not if it's a literal IP address.
+ * (RFC 6066)
+ */
+ if (!(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
+ strchr(conn->pghost, ':')))
+ {
+ if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
--
2.30.1
Attachments:
[text/plain] v2-0001-Set-SNI-for-SSL-connections-from-the-client.patch (1.5K, ../../be696d28-36e8-739b-04a9-48f1c374d6a8@enterprisedb.com/2-v2-0001-Set-SNI-for-SSL-connections-from-the-client.patch)
download | inline diff:
From bef8152b6f4ad2ed2ccd8158088a35b2cf625491 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 25 Feb 2021 16:40:32 +0100
Subject: [PATCH v2] Set SNI for SSL connections from the client
This allows an SNI-aware proxy to route connections.
Discussion: https://www.postgresql.org/message-id/flat/7289d5eb-62a5-a732-c3b9-438cee2cb709%40enterprisedb.com
---
src/interfaces/libpq/fe-secure-openssl.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0fa10a23b4..889213c994 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1076,6 +1076,26 @@ initialize_SSL(PGconn *conn)
SSL_CTX_free(SSL_context);
SSL_context = NULL;
+ /*
+ * Set Server Name Indication (SNI), but not if it's a literal IP address.
+ * (RFC 6066)
+ */
+ if (!(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
+ strchr(conn->pghost, ':')))
+ {
+ if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
--
2.30.1
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-25 16:00 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Jacob Champion <pchampion@vmware.com>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-02-25 16:00 UTC (permalink / raw)
To: Jacob Champion <pchampion@vmware.com>; pgsql-hackers
On 17.02.21 00:01, Jacob Champion wrote:
> On Mon, 2021-02-15 at 15:09 +0100, Peter Eisentraut wrote:
>> The question I had was whether this should be an optional behavior, or
>> conversely a behavior that can be turned off, or whether it should just
>> be turned on all the time.
> Personally I think there should be a toggle, so that any users for whom
> hostnames are potentially sensitive don't have to make that information
> available on the wire. Opt-in, to avoid having any new information
> disclosure after a version upgrade?
Just as additional data points, it has come to my attention that both
the Go driver ("lib/pq") and the JDBC environment already send SNI
automatically. (In the case of JDBC this is done by the Java system
libraries, not the JDBC driver implementation.)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-25 18:36 Jacob Champion <pchampion@vmware.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 24+ messages in thread
From: Jacob Champion @ 2021-02-25 18:36 UTC (permalink / raw)
To: peter.eisentraut@enterprisedb.com <peter.eisentraut@enterprisedb.com>; pgsql-hackers
On Thu, 2021-02-25 at 17:00 +0100, Peter Eisentraut wrote:
> Just as additional data points, it has come to my attention that both
> the Go driver ("lib/pq") and the JDBC environment already send SNI
> automatically. (In the case of JDBC this is done by the Java system
> libraries, not the JDBC driver implementation.)
For the Go case it's only for sslmode=verify-full, and only because the
Go standard library implementation does it for you automatically if you
request the builtin server hostname validation. (I checked both lib/pq
and its de facto replacement, jackc/pgx.) So it may not be something
that was done on purpose by the driver implementation.
--Jacob
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-26 02:40 Greg Stark <stark@mit.edu>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 24+ messages in thread
From: Greg Stark @ 2021-02-26 02:40 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>; pgsql-hackers
Hate to be that guy but....
This still doesn't seem like it is IPv6-ready. Is there any harm in
having SNI with an IPv6 address there if it gets through?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-26 07:05 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Greg Stark <stark@mit.edu>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-02-26 07:05 UTC (permalink / raw)
To: Greg Stark <stark@mit.edu>; +Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>; pgsql-hackers
On 26.02.21 03:40, Greg Stark wrote:
> This still doesn't seem like it is IPv6-ready.
Do you mean the IPv6 detection code is not correct? What is the problem?
> Is there any harm in> having SNI with an IPv6 address there if it
gets through?
I doubt it.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-26 17:55 Stephen Frost <sfrost@snowman.net>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
3 siblings, 0 replies; 24+ messages in thread
From: Stephen Frost @ 2021-02-26 17:55 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: pgsql-hackers
Greetings,
* Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:
> A customer asked about including Server Name Indication (SNI) into the SSL
> connection from the client, so they can use an SSL-aware proxy to route
> connections. There was a thread a few years ago where this was briefly
> discussed but no patch appeared.[0] I whipped up a quick patch and it did
> seem to do the job, so I figured I'd share it here.
This doesn't actually result in the ability to do that SSL connection
proxying though, does it? At the least, whatever the proxy is would
have to be taught how to send back an 'S' to the client, and send an 'S'
to the server chosen after the client sends along the TLS ClientHello w/
SNI set, and then pass the traffic between afterwards.
Perhaps it's worth doing this to allow proxy developers to do that, but
this isn't enough to make it actually work without the proxy actually
knowing PG and being able to be configured to do the right thing for the
PG protocol. I would think that, ideally, we'd have some proxy author
who would be willing to actually implement this and test that it all
works with this patch applied, and then make sure to explain that
proxies will need to be adapted to be able to work. Simply including
this and then putting in the release notes that we now provide SNI as
part of the SSL connection would likely lead people to believe that
it'll 'just work'. Perhaps to manage expectations we'd want to say
something like:
- libpq will now include Server Name Indication as part of the
PostgreSQL SSL startup process; proxies will need to understand the
PostgreSQL protocol in order to be able to leverage this to perform
routing.
Or something along those lines, I would think. Of course, such a proxy
would need to also understand how to tell a client that, for example,
GSSAPI encryption isn't available if a 'G' came first from the client,
and what to do if a plaintext connection was requested.
> The question I had was whether this should be an optional behavior, or
> conversely a behavior that can be turned off, or whether it should just be
> turned on all the time.
Certainly seems like something that we should support turning off, at
least. As mentioned elsewhere, knowing the system that's being
connected to is certainly interesting to attackers.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (818B, ../../20210226175516.GZ20766@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-02-26 22:27 Greg Stark <stark@mit.edu>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 24+ messages in thread
From: Greg Stark @ 2021-02-26 22:27 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>; pgsql-hackers
> Do you mean the IPv6 detection code is not correct? What is the problem?
This bit, will recognize ipv4 addresses but not ipv6 addresses:
+ /*
+ * Set Server Name Indication (SNI), but not if it's a literal IP address.
+ * (RFC 6066)
+ */
+ if (!(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
+ strchr(conn->pghost, ':')))
+ {
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-03-18 08:31 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Greg Stark <stark@mit.edu>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-03-18 08:31 UTC (permalink / raw)
To: Greg Stark <stark@mit.edu>; +Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>; pgsql-hackers
On 26.02.21 23:27, Greg Stark wrote:
>> Do you mean the IPv6 detection code is not correct? What is the problem?
>
> This bit, will recognize ipv4 addresses but not ipv6 addresses:
>
> + /*
> + * Set Server Name Indication (SNI), but not if it's a literal IP address.
> + * (RFC 6066)
> + */
> + if (!(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
> + strchr(conn->pghost, ':')))
> + {
The colon should recognize an IPv6 address, unless I'm not thinking
straight.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-03-18 08:48 Magnus Hagander <magnus@hagander.net>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 0 replies; 24+ messages in thread
From: Magnus Hagander @ 2021-03-18 08:48 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Greg Stark <stark@mit.edu>; Matthias van de Meent <boekewurm+postgres@gmail.com>; pgsql-hackers
On Thu, Mar 18, 2021 at 9:31 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 26.02.21 23:27, Greg Stark wrote:
> >> Do you mean the IPv6 detection code is not correct? What is the problem?
> >
> > This bit, will recognize ipv4 addresses but not ipv6 addresses:
> >
> > + /*
> > + * Set Server Name Indication (SNI), but not if it's a literal IP address.
> > + * (RFC 6066)
> > + */
> > + if (!(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
> > + strchr(conn->pghost, ':')))
> > + {
>
> The colon should recognize an IPv6 address, unless I'm not thinking
> straight.
Yeah, it should.
One could argue you should also check that it's got only valid ipv6
characters in it, but since the colon isn't allowed in a hostname this
shouldn't be a problem. (And we cannot have a <host>:<port> stored in
conn->pghost).
My guess is Greg missed the second part of it that checks for a colon
-- so maybe expand on that a bit in the comment, and on the fact that
we already know the port can't be part of it.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-03-18 11:27 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Jacob Champion <pchampion@vmware.com>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-03-18 11:27 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Jacob Champion <pchampion@vmware.com>
On 25.02.21 19:36, Jacob Champion wrote:
> On Thu, 2021-02-25 at 17:00 +0100, Peter Eisentraut wrote:
>> Just as additional data points, it has come to my attention that both
>> the Go driver ("lib/pq") and the JDBC environment already send SNI
>> automatically. (In the case of JDBC this is done by the Java system
>> libraries, not the JDBC driver implementation.)
>
> For the Go case it's only for sslmode=verify-full, and only because the
> Go standard library implementation does it for you automatically if you
> request the builtin server hostname validation. (I checked both lib/pq
> and its de facto replacement, jackc/pgx.) So it may not be something
> that was done on purpose by the driver implementation.
Here is a new patch with an option to turn it off, and some
documentation added.
From 50bc10d11018883ad7a555e90231b6ef16e3fdf6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 18 Mar 2021 12:25:13 +0100
Subject: [PATCH v3] libpq: Set Server Name Indication (SNI) for SSL
connections
By default, have libpq set the TLS extension "Server Name Indication" (SNI).
This allows an SNI-aware SSL proxy to route connections. (This
requires a proxy that is aware of the PostgreSQL protocol, not just
any SSL proxy.)
In the future, this could also allow the server to use different SSL
certificates for different host specifications. (That would require
new server functionality. This would be the client-side functionality
for that.)
Since SNI makes the host name appear in cleartext in the network
traffic, this might be undesirable in some cases. Therefore, also add
a libpq connection option "sslsni" to turn it off.
Discussion: https://www.postgresql.org/message-id/flat/7289d5eb-62a5-a732-c3b9-438cee2cb709%40enterprisedb.com
---
.../postgres_fdw/expected/postgres_fdw.out | 2 +-
doc/src/sgml/libpq.sgml | 31 +++++++++++++++++++
src/interfaces/libpq/fe-connect.c | 6 ++++
src/interfaces/libpq/fe-secure-openssl.c | 22 +++++++++++++
src/interfaces/libpq/libpq-int.h | 1 +
5 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 0649b6b81c..d0405cf4a7 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8946,7 +8946,7 @@ DO $d$
END;
$d$;
ERROR: invalid option "password"
-HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size
+HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size
CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')"
PL/pgSQL function inline_code_block line 3 at EXECUTE
-- If we add a password for our user mapping instead, we should get a different
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index be674fbaa9..dbfc7add14 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1777,6 +1777,27 @@ <title>Parameter Key Words</title>
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-sslsni" xreflabel="sslsni">
+ <term><literal>sslsni</literal><indexterm><primary>Server Name Indication</primary></indexterm></term>
+ <listitem>
+ <para>
+ By default, libpq sets the TLS extension <quote>Server Name
+ Indication</quote> (SNI) on SSL-enabled connections. See <ulink
+ url="https://tools.ietf.org/html/rfc6066#section-3">RFC 6066</ulink>
+ for details. By setting this parameter to 0, this is turned off.
+ </para>
+
+ <para>
+ The Server Name Indication can be used by SSL-aware proxies to route
+ connections without having to decrypt the SSL stream. (Note that this
+ requires a proxy that is aware of the PostgreSQL protocol handshake,
+ not just any SSL proxy.) However, SNI makes the destination host name
+ appear in cleartext in the network traffic, so it might be undesirable
+ in some cases.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-requirepeer" xreflabel="requirepeer">
<term><literal>requirepeer</literal></term>
<listitem>
@@ -7757,6 +7778,16 @@ <title>Environment Variables</title>
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGSSLSNI</envar></primary>
+ </indexterm>
+ <envar>PGSSLSNI</envar> behaves the same as the <xref
+ linkend="libpq-connect-sslsni"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 53b354abb2..f1551e796c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -303,6 +303,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"SSL-Revocation-List-Dir", "", 64,
offsetof(struct pg_conn, sslcrldir)},
+ {"sslsni", "PGSSLSNI", "1", NULL,
+ "SSL-SNI", "", 1,
+ offsetof(struct pg_conn, sslsni)},
+
{"requirepeer", "PGREQUIREPEER", NULL, NULL,
"Require-Peer", "", 10,
offsetof(struct pg_conn, requirepeer)},
@@ -4094,6 +4098,8 @@ freePGconn(PGconn *conn)
free(conn->sslcrldir);
if (conn->sslcompression)
free(conn->sslcompression);
+ if (conn->sslsni)
+ free(conn->sslsni);
if (conn->requirepeer)
free(conn->requirepeer);
if (conn->ssl_min_protocol_version)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 9c2222c1d1..6f357dfbfe 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1082,6 +1082,28 @@ initialize_SSL(PGconn *conn)
SSL_CTX_free(SSL_context);
SSL_context = NULL;
+ /*
+ * Set Server Name Indication (SNI), if enabled by connection parameters.
+ * Per RFC 6066, do not set it if the host is a literal IP address (IPv4
+ * or IPv6).
+ */
+ if (conn->sslsni && conn->sslsni[0] &&
+ !(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
+ strchr(conn->pghost, ':')))
+ {
+ if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 6374ec657a..24bd1209a0 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -383,6 +383,7 @@ struct pg_conn
char *sslrootcert; /* root certificate filename */
char *sslcrl; /* certificate revocation list filename */
char *sslcrldir; /* certificate revocation list directory name */
+ char *sslsni; /* use SSL SNI extension (0 or 1) */
char *requirepeer; /* required peer credentials for local sockets */
char *gssencmode; /* GSS mode (require,prefer,disable) */
char *krbsrvname; /* Kerberos service name */
base-commit: ed62d3737c1b823f796d974060b1d0295a3dd831
--
2.30.2
Attachments:
[text/plain] v3-0001-libpq-Set-Server-Name-Indication-SNI-for-SSL-conn.patch (7.4K, ../../4eaa2b56-c3d0-67b6-4716-010d001c7601@enterprisedb.com/2-v3-0001-libpq-Set-Server-Name-Indication-SNI-for-SSL-conn.patch)
download | inline diff:
From 50bc10d11018883ad7a555e90231b6ef16e3fdf6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 18 Mar 2021 12:25:13 +0100
Subject: [PATCH v3] libpq: Set Server Name Indication (SNI) for SSL
connections
By default, have libpq set the TLS extension "Server Name Indication" (SNI).
This allows an SNI-aware SSL proxy to route connections. (This
requires a proxy that is aware of the PostgreSQL protocol, not just
any SSL proxy.)
In the future, this could also allow the server to use different SSL
certificates for different host specifications. (That would require
new server functionality. This would be the client-side functionality
for that.)
Since SNI makes the host name appear in cleartext in the network
traffic, this might be undesirable in some cases. Therefore, also add
a libpq connection option "sslsni" to turn it off.
Discussion: https://www.postgresql.org/message-id/flat/7289d5eb-62a5-a732-c3b9-438cee2cb709%40enterprisedb.com
---
.../postgres_fdw/expected/postgres_fdw.out | 2 +-
doc/src/sgml/libpq.sgml | 31 +++++++++++++++++++
src/interfaces/libpq/fe-connect.c | 6 ++++
src/interfaces/libpq/fe-secure-openssl.c | 22 +++++++++++++
src/interfaces/libpq/libpq-int.h | 1 +
5 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 0649b6b81c..d0405cf4a7 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8946,7 +8946,7 @@ DO $d$
END;
$d$;
ERROR: invalid option "password"
-HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size
+HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size
CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')"
PL/pgSQL function inline_code_block line 3 at EXECUTE
-- If we add a password for our user mapping instead, we should get a different
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index be674fbaa9..dbfc7add14 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1777,6 +1777,27 @@ <title>Parameter Key Words</title>
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-sslsni" xreflabel="sslsni">
+ <term><literal>sslsni</literal><indexterm><primary>Server Name Indication</primary></indexterm></term>
+ <listitem>
+ <para>
+ By default, libpq sets the TLS extension <quote>Server Name
+ Indication</quote> (SNI) on SSL-enabled connections. See <ulink
+ url="https://tools.ietf.org/html/rfc6066#section-3">RFC 6066</ulink>
+ for details. By setting this parameter to 0, this is turned off.
+ </para>
+
+ <para>
+ The Server Name Indication can be used by SSL-aware proxies to route
+ connections without having to decrypt the SSL stream. (Note that this
+ requires a proxy that is aware of the PostgreSQL protocol handshake,
+ not just any SSL proxy.) However, SNI makes the destination host name
+ appear in cleartext in the network traffic, so it might be undesirable
+ in some cases.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-requirepeer" xreflabel="requirepeer">
<term><literal>requirepeer</literal></term>
<listitem>
@@ -7757,6 +7778,16 @@ <title>Environment Variables</title>
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGSSLSNI</envar></primary>
+ </indexterm>
+ <envar>PGSSLSNI</envar> behaves the same as the <xref
+ linkend="libpq-connect-sslsni"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 53b354abb2..f1551e796c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -303,6 +303,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"SSL-Revocation-List-Dir", "", 64,
offsetof(struct pg_conn, sslcrldir)},
+ {"sslsni", "PGSSLSNI", "1", NULL,
+ "SSL-SNI", "", 1,
+ offsetof(struct pg_conn, sslsni)},
+
{"requirepeer", "PGREQUIREPEER", NULL, NULL,
"Require-Peer", "", 10,
offsetof(struct pg_conn, requirepeer)},
@@ -4094,6 +4098,8 @@ freePGconn(PGconn *conn)
free(conn->sslcrldir);
if (conn->sslcompression)
free(conn->sslcompression);
+ if (conn->sslsni)
+ free(conn->sslsni);
if (conn->requirepeer)
free(conn->requirepeer);
if (conn->ssl_min_protocol_version)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 9c2222c1d1..6f357dfbfe 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1082,6 +1082,28 @@ initialize_SSL(PGconn *conn)
SSL_CTX_free(SSL_context);
SSL_context = NULL;
+ /*
+ * Set Server Name Indication (SNI), if enabled by connection parameters.
+ * Per RFC 6066, do not set it if the host is a literal IP address (IPv4
+ * or IPv6).
+ */
+ if (conn->sslsni && conn->sslsni[0] &&
+ !(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
+ strchr(conn->pghost, ':')))
+ {
+ if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ }
+
/*
* Read the SSL key. If a key is specified, treat it as an engine:key
* combination if there is colon present - we don't support files with
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 6374ec657a..24bd1209a0 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -383,6 +383,7 @@ struct pg_conn
char *sslrootcert; /* root certificate filename */
char *sslcrl; /* certificate revocation list filename */
char *sslcrldir; /* certificate revocation list directory name */
+ char *sslsni; /* use SSL SNI extension (0 or 1) */
char *requirepeer; /* required peer credentials for local sockets */
char *gssencmode; /* GSS mode (require,prefer,disable) */
char *krbsrvname; /* Kerberos service name */
base-commit: ed62d3737c1b823f796d974060b1d0295a3dd831
--
2.30.2
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-04-07 13:32 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-04-07 13:32 UTC (permalink / raw)
To: pgsql-hackers
On 18.03.21 12:27, Peter Eisentraut wrote:
> On 25.02.21 19:36, Jacob Champion wrote:
>> On Thu, 2021-02-25 at 17:00 +0100, Peter Eisentraut wrote:
>>> Just as additional data points, it has come to my attention that both
>>> the Go driver ("lib/pq") and the JDBC environment already send SNI
>>> automatically. (In the case of JDBC this is done by the Java system
>>> libraries, not the JDBC driver implementation.)
>>
>> For the Go case it's only for sslmode=verify-full, and only because the
>> Go standard library implementation does it for you automatically if you
>> request the builtin server hostname validation. (I checked both lib/pq
>> and its de facto replacement, jackc/pgx.) So it may not be something
>> that was done on purpose by the driver implementation.
>
> Here is a new patch with an option to turn it off, and some
> documentation added.
Committed like that. (Default to on, but it's easy to change if there
are any further thoughts.)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-03 17:25 Jacob Champion <pchampion@vmware.com>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 24+ messages in thread
From: Jacob Champion @ 2021-06-03 17:25 UTC (permalink / raw)
To: peter.eisentraut@enterprisedb.com <peter.eisentraut@enterprisedb.com>; pgsql-hackers
On Wed, 2021-04-07 at 15:32 +0200, Peter Eisentraut wrote:
> Committed like that. (Default to on, but it's easy to change if there
> are any further thoughts.)
Hi Peter,
It looks like this code needs some guards for a NULL conn->pghost. For example when running
psql 'dbname=postgres sslmode=require hostaddr=127.0.0.1'
with no PGHOST in the environment, psql is currently segfaulting for
me.
--Jacob
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-03 17:41 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jacob Champion <pchampion@vmware.com>
0 siblings, 1 reply; 24+ messages in thread
From: Tom Lane @ 2021-06-03 17:41 UTC (permalink / raw)
To: Jacob Champion <pchampion@vmware.com>; +Cc: peter.eisentraut@enterprisedb.com <peter.eisentraut@enterprisedb.com>; pgsql-hackers
Jacob Champion <pchampion@vmware.com> writes:
> It looks like this code needs some guards for a NULL conn->pghost. For example when running
> psql 'dbname=postgres sslmode=require hostaddr=127.0.0.1'
> with no PGHOST in the environment, psql is currently segfaulting for
> me.
Duplicated here:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007f3adec47ec3 in __strspn_sse42 () from /lib64/libc.so.6
(gdb) bt
#0 0x00007f3adec47ec3 in __strspn_sse42 () from /lib64/libc.so.6
#1 0x00007f3adf6b7026 in initialize_SSL (conn=0xed4160)
at fe-secure-openssl.c:1090
#2 0x00007f3adf6b8755 in pgtls_open_client (conn=conn@entry=0xed4160)
at fe-secure-openssl.c:132
#3 0x00007f3adf6b3955 in pqsecure_open_client (conn=conn@entry=0xed4160)
at fe-secure.c:180
#4 0x00007f3adf6a4808 in PQconnectPoll (conn=conn@entry=0xed4160)
at fe-connect.c:3102
#5 0x00007f3adf6a5b31 in connectDBComplete (conn=conn@entry=0xed4160)
at fe-connect.c:2219
#6 0x00007f3adf6a8968 in PQconnectdbParams (keywords=keywords@entry=0xed40c0,
values=values@entry=0xed4110, expand_dbname=expand_dbname@entry=1)
at fe-connect.c:669
#7 0x0000000000404db2 in main (argc=<optimized out>, argv=0x7ffc58477208)
at startup.c:266
You don't seem to need the "sslmode=require" either, just an
SSL-enabled server.
regards, tom lane
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-03 17:52 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 24+ messages in thread
From: Tom Lane @ 2021-06-03 17:52 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Jacob Champion <pchampion@vmware.com>; pgsql-hackers
I wrote:
> Jacob Champion <pchampion@vmware.com> writes:
>> It looks like this code needs some guards for a NULL conn->pghost. For example when running
>> psql 'dbname=postgres sslmode=require hostaddr=127.0.0.1'
>> with no PGHOST in the environment, psql is currently segfaulting for
>> me.
> Duplicated here:
It looks like the immediate problem can be resolved by just adding
a check for conn->pghost not being NULL, since the comment above
says
* Per RFC 6066, do not set it if the host is a literal IP address (IPv4
* or IPv6).
and having only hostaddr certainly fits that case. But I didn't
check to see if any more problems arise later.
regards, tom lane
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-03 18:14 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 24+ messages in thread
From: Tom Lane @ 2021-06-03 18:14 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Jacob Champion <pchampion@vmware.com>; pgsql-hackers
I wrote:
> It looks like the immediate problem can be resolved by just adding
> a check for conn->pghost not being NULL,
... scratch that. There's another problem here, which is that this
code should not be looking at conn->pghost AT ALL. That will do the
wrong thing with a multi-element host list. The right thing to be
looking at is conn->connhost[conn->whichhost].host --- with a test
to make sure it's not NULL or an empty string. (I didn't stop to
study this code close enough to see if it'll ignore an empty
string without help.)
regards, tom lane
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-07 09:54 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2021-06-07 09:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jacob Champion <pchampion@vmware.com>; pgsql-hackers
On 03.06.21 20:14, Tom Lane wrote:
> I wrote:
>> It looks like the immediate problem can be resolved by just adding
>> a check for conn->pghost not being NULL,
>
> ... scratch that. There's another problem here, which is that this
> code should not be looking at conn->pghost AT ALL. That will do the
> wrong thing with a multi-element host list. The right thing to be
> looking at is conn->connhost[conn->whichhost].host --- with a test
> to make sure it's not NULL or an empty string. (I didn't stop to
> study this code close enough to see if it'll ignore an empty
> string without help.)
Patch attached. Empty host string was handled implicitly by the IP
detection expression, but I added an explicit check for sanity. (I
wasn't actually able to get an empty string to this point, but it's
clearly better to be prepared for it.)
From 095bb64fc735c54d0ebf17ba6c13e0de5af44a36 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 7 Jun 2021 11:50:01 +0200
Subject: [PATCH] libpq: Fix SNI host handling
Fix handling of NULL host name (possibly by using hostaddr). It
previously crashed. Also, we should look at connhost, not pghost, to
handle multi-host specifications.
Reported-by: Jacob Champion <pchampion@vmware.com>
Discussion: https://www.postgresql.org/message-id/504c276ab6eee000bb23d571ea9b0ced4250774e.camel@vmware.com
---
src/interfaces/libpq/fe-secure-openssl.c | 27 ++++++++++++++----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 00d43f3eff..36fec96ef1 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1087,20 +1087,25 @@ initialize_SSL(PGconn *conn)
* Per RFC 6066, do not set it if the host is a literal IP address (IPv4
* or IPv6).
*/
- if (conn->sslsni && conn->sslsni[0] &&
- !(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
- strchr(conn->pghost, ':')))
+ if (conn->sslsni && conn->sslsni[0])
{
- if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ const char *host = conn->connhost[conn->whichhost].host;
+
+ if (host && host[0] &&
+ !(strspn(host, "0123456789.") == strlen(host) ||
+ strchr(host, ':')))
{
- char *err = SSLerrmessage(ERR_get_error());
+ if (SSL_set_tlsext_host_name(conn->ssl, host) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
- err);
- SSLerrfree(err);
- SSL_CTX_free(SSL_context);
- return -1;
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
}
}
--
2.31.1
Attachments:
[text/plain] 0001-libpq-Fix-SNI-host-handling.patch (2.0K, ../../5df46438-c1b4-161c-4669-f6e6ab6797f0@enterprisedb.com/2-0001-libpq-Fix-SNI-host-handling.patch)
download | inline diff:
From 095bb64fc735c54d0ebf17ba6c13e0de5af44a36 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 7 Jun 2021 11:50:01 +0200
Subject: [PATCH] libpq: Fix SNI host handling
Fix handling of NULL host name (possibly by using hostaddr). It
previously crashed. Also, we should look at connhost, not pghost, to
handle multi-host specifications.
Reported-by: Jacob Champion <pchampion@vmware.com>
Discussion: https://www.postgresql.org/message-id/504c276ab6eee000bb23d571ea9b0ced4250774e.camel@vmware.com
---
src/interfaces/libpq/fe-secure-openssl.c | 27 ++++++++++++++----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 00d43f3eff..36fec96ef1 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1087,20 +1087,25 @@ initialize_SSL(PGconn *conn)
* Per RFC 6066, do not set it if the host is a literal IP address (IPv4
* or IPv6).
*/
- if (conn->sslsni && conn->sslsni[0] &&
- !(strspn(conn->pghost, "0123456789.") == strlen(conn->pghost) ||
- strchr(conn->pghost, ':')))
+ if (conn->sslsni && conn->sslsni[0])
{
- if (SSL_set_tlsext_host_name(conn->ssl, conn->pghost) != 1)
+ const char *host = conn->connhost[conn->whichhost].host;
+
+ if (host && host[0] &&
+ !(strspn(host, "0123456789.") == strlen(host) ||
+ strchr(host, ':')))
{
- char *err = SSLerrmessage(ERR_get_error());
+ if (SSL_set_tlsext_host_name(conn->ssl, host) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
- err);
- SSLerrfree(err);
- SSL_CTX_free(SSL_context);
- return -1;
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set SSL Server Name Indication (SNI): %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
}
}
--
2.31.1
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-07 15:34 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 24+ messages in thread
From: Tom Lane @ 2021-06-07 15:34 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Jacob Champion <pchampion@vmware.com>; pgsql-hackers
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> Patch attached. Empty host string was handled implicitly by the IP
> detection expression, but I added an explicit check for sanity. (I
> wasn't actually able to get an empty string to this point, but it's
> clearly better to be prepared for it.)
Yeah, I'd include the empty-string test just because it's standard
practice in this area of libpq. Whether those tests are actually
triggerable in every case is obscure, but ...
Patch looks sane by eyeball, though I didn't test it.
regards, tom lane
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-08 06:54 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2021-06-08 06:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; Jacob Champion <pchampion@vmware.com>; pgsql-hackers
On Mon, Jun 07, 2021 at 11:34:24AM -0400, Tom Lane wrote:
> Yeah, I'd include the empty-string test just because it's standard
> practice in this area of libpq. Whether those tests are actually
> triggerable in every case is obscure, but ...
Checking after a NULL string and an empty one is more libpq-ish.
> Patch looks sane by eyeball, though I didn't test it.
I did, and I could not break it.
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
It seems to me that there is no need to free SSL_context if
SSL_set_tlsext_host_name() fails here, except if you'd like to move
the check for the SNI above SSL_CTX_free() around L1082. There is no
harm as SSL_CTX_free() is a no-op on NULL input.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YL8ULDGZaTDw3Swa@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: SSL SNI
@ 2021-06-08 14:12 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 24+ messages in thread
From: Peter Eisentraut @ 2021-06-08 14:12 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jacob Champion <pchampion@vmware.com>; pgsql-hackers
On 08.06.21 08:54, Michael Paquier wrote:
> On Mon, Jun 07, 2021 at 11:34:24AM -0400, Tom Lane wrote:
>> Yeah, I'd include the empty-string test just because it's standard
>> practice in this area of libpq. Whether those tests are actually
>> triggerable in every case is obscure, but ...
>
> Checking after a NULL string and an empty one is more libpq-ish.
>
>> Patch looks sane by eyeball, though I didn't test it.
>
> I did, and I could not break it.
>
> + SSLerrfree(err);
> + SSL_CTX_free(SSL_context);
> + return -1;
> It seems to me that there is no need to free SSL_context if
> SSL_set_tlsext_host_name() fails here, except if you'd like to move
> the check for the SNI above SSL_CTX_free() around L1082. There is no
> harm as SSL_CTX_free() is a no-op on NULL input.
Good point. Committed that way.
^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2021-06-08 14:12 UTC | newest]
Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-15 14:09 SSL SNI Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-02-15 14:28 ` Matthias van de Meent <boekewurm+postgres@gmail.com>
2021-02-25 15:58 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-02-26 02:40 ` Greg Stark <stark@mit.edu>
2021-02-26 07:05 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-02-26 22:27 ` Greg Stark <stark@mit.edu>
2021-03-18 08:31 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-03-18 08:48 ` Magnus Hagander <magnus@hagander.net>
2021-02-15 17:40 ` Jesse Zhang <sbjesse@gmail.com>
2021-02-15 19:24 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-02-16 23:01 ` Jacob Champion <pchampion@vmware.com>
2021-02-25 16:00 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-02-25 18:36 ` Jacob Champion <pchampion@vmware.com>
2021-03-18 11:27 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-04-07 13:32 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-06-03 17:25 ` Jacob Champion <pchampion@vmware.com>
2021-06-03 17:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-03 17:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-03 18:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-07 09:54 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-06-07 15:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-08 06:54 ` Michael Paquier <michael@paquier.xyz>
2021-06-08 14:12 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2021-02-26 17:55 ` Stephen Frost <sfrost@snowman.net>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox