Thread: SSL SNI
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%40mail.gmail.com [1]: https://en.wikipedia.org/wiki/Server_Name_Indication [2]: https://tools.ietf.org/html/rfc6066#section-3
Attachment
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
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%40mail.gmail.com > [1]: https://en.wikipedia.org/wiki/Server_Name_Indication > [2]: https://tools.ietf.org/html/rfc6066#section-3
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 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.
Attachment
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.)
Attachment
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
Attachment
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.