SSL SNI

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]/messages/by-id/37846a5e-bb5e-0c4f-3ee8-54fb4bd02fab@gmx.de. 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]: /messages/by-id/37846a5e-bb5e-0c4f-3ee8-54fb4bd02fab@gmx.de

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:

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.

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

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.

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

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

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?

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.

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

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, ':')))
+ {

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.

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/

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.

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

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

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

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

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

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

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

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

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.