Horton

Thread

Commits

  1. libpq: Fix SNI host handling

  2. libpq: Set Server Name Indication (SNI) for SSL connections

  1. SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-02-15T14:09:47Z

    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

  2. Re: SSL SNI

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2021-02-15T14:28:23Z

    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

  3. Re: SSL SNI

    Jesse Zhang <sbjesse@gmail.com> — 2021-02-15T17:40:10Z

    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

  4. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-02-15T19:24:56Z

    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.

  5. Re: SSL SNI

    Jacob Champion <pchampion@vmware.com> — 2021-02-16T23:01:36Z

    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

  6. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-02-25T15:58:28Z

    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.

  7. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-02-25T16:00:25Z

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

  8. Re: SSL SNI

    Jacob Champion <pchampion@vmware.com> — 2021-02-25T18:36:22Z

    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

  9. Re: SSL SNI

    Greg Stark <stark@mit.edu> — 2021-02-26T02:40:03Z

    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?

  10. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-02-26T07:05:12Z

    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.

  11. Re: SSL SNI

    Stephen Frost <sfrost@snowman.net> — 2021-02-26T17:55:16Z

    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

  12. Re: SSL SNI

    Greg Stark <stark@mit.edu> — 2021-02-26T22:27:42Z

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

  13. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-18T08:31:24Z

    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.

  14. Re: SSL SNI

    Magnus Hagander <magnus@hagander.net> — 2021-03-18T08:48:57Z

    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/

  15. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-18T11:27:02Z

    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.

  16. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-04-07T13:32:55Z

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

  17. Re: SSL SNI

    Jacob Champion <pchampion@vmware.com> — 2021-06-03T17:25:24Z

    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

  18. Re: SSL SNI

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-03T17:41:48Z

    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

  19. Re: SSL SNI

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-03T17:52:41Z

    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

  20. Re: SSL SNI

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-03T18:14:41Z

    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

  21. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-07T09:54:31Z

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

  22. Re: SSL SNI

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-07T15:34:24Z

    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

  23. Re: SSL SNI

    Michael Paquier <michael@paquier.xyz> — 2021-06-08T06:54:36Z

    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

  24. Re: SSL SNI

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-08T14:12:44Z

    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.