Horton

Thread

Commits

  1. doc: Add note to ssl_group config on X25519 and FIPS

  2. Avoid using the X25519 curve in ssl tests

  3. Add X25519 to the default set of curves

  4. SSL: Support ECDH key exchange

  1. ecdh support causes unnecessary roundtrips

    Andres Freund <andres@anarazel.de> — 2024-06-16T23:46:12Z

    Hi,

When connecting with a libpq based client, the TLS establishment ends up like
this in many configurations;

C->S: TLSv1 393 Client Hello
S->C: TLSv1.3 167 Hello Retry Request, Change Cipher Spec
C->S: TLSv1.3 432 Change Cipher Spec, Client Hello
S->C: TLSv1.3 1407 Server Hello, Application Data, Application Data, Application Data, Application Data
...

I.e. there are two clients hellos, because the server rejects the clients
"parameters".


This appears to be caused by ECDH support.  The difference between the two
ClientHellos is
-            Extension: key_share (len=38) x25519
+            Extension: key_share (len=71) secp256r1

I.e. the clients wanted to use x25519, but the server insists on secp256r1.


This turns out to be due to

commit 3164721462d547fa2d15e2a2f07eb086a3590fd5
Author: Peter Eisentraut <peter_e@gmx.net>
Date:   2013-12-07 15:11:44 -0500

    SSL: Support ECDH key exchange



I don't know if it's good that we're calling SSL_CTX_set_tmp_ecdh at all, but
if it is, shouldn't we at least do the same in libpq, so we don't introduce
unnecessary roundtrips?

I did confirm that doing the same thing on the client side removes the
additional roundtrip.



It seems kind of a shame that we have fewer roundtrips due to
sslnegotiation=direct, but do completely unnecessary roundtrips all the
time...


In a network with ~10ms latency I see an almost 30% increased
connections-per-second for a single client if I avoid the added roundtrip.

I think this could almost be considered a small bug...

Greetings,

Andres Freund

  2. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2024-06-17T10:00:30Z

    > On 17 Jun 2024, at 01:46, Andres Freund <andres@anarazel.de> wrote:

> When connecting with a libpq based client, the TLS establishment ends up like
> this in many configurations;
> 
> C->S: TLSv1 393 Client Hello
> S->C: TLSv1.3 167 Hello Retry Request, Change Cipher Spec
> C->S: TLSv1.3 432 Change Cipher Spec, Client Hello
> S->C: TLSv1.3 1407 Server Hello, Application Data, Application Data, Application Data, Application Data

I wonder if this is the result of us still using TLS 1.2 as the default minimum
protocol version.  In 1.2 the ClientHello doesn't contain the extensions for
cipher and curves, so the server must reply with a HRR (Hello Retry Request)
message asking the client to set protocol, curve and cipher.  The client will
respond with a new Client Hello using 1.3 with the extensions.

Using 1.3 as the default minimum has been on my personal TODO for a while,
maybe that should be revisited for 18.

> I.e. there are two clients hellos, because the server rejects the clients
> "parameters".

Or the client didn't send any.

> This appears to be caused by ECDH support.  The difference between the two
> ClientHellos is
> -            Extension: key_share (len=38) x25519
> +            Extension: key_share (len=71) secp256r1
> 
> I.e. the clients wanted to use x25519, but the server insists on secp256r1.

Somewhat related, Erica Zhang has an open patch to make the server-side curves
configuration take a list rather than a single curve [0], and modernizing the
API used as a side effect (SSL_CTX_set_tmp_ecdh is documented as obsoleted by
OpenSSL, but not deprecated with an API level).

> I don't know if it's good that we're calling SSL_CTX_set_tmp_ecdh at all,

To set the specified curve in ssl_ecdh_curve we have to don't we?

> but if it is, shouldn't we at least do the same in libpq, so we don't introduce
> unnecessary roundtrips?

If we don't set the curve in the client I believe OpenSSL will pass the set of
supported curves the client has, which then should allow the server to pick the
one it wants based on ssl_ecdh_curve, so ideally we shouldn't have to I think.

> I did confirm that doing the same thing on the client side removes the
> additional roundtrip.

The roundtrip went away because the client was set to use secp256r1?  I wonder
if that made OpenSSL override the min protocol version and switch to a TLS1.3
ClientHello since it otherwise couldn't announce the curve.  If you force the
client min protocol to 1.3 in an unpatched client, do you see the same speedup?

--
Daniel Gustafsson

[0] tencent_1B368B07F4B5886F9822981189DA736CF209@qq.com

  3. Re: ecdh support causes unnecessary roundtrips

    Andres Freund <andres@anarazel.de> — 2024-06-17T17:01:33Z

    Hi,

On 2024-06-17 12:00:30 +0200, Daniel Gustafsson wrote:
> > On 17 Jun 2024, at 01:46, Andres Freund <andres@anarazel.de> wrote:
>
> > When connecting with a libpq based client, the TLS establishment ends up like
> > this in many configurations;
> >
> > C->S: TLSv1 393 Client Hello
> > S->C: TLSv1.3 167 Hello Retry Request, Change Cipher Spec
> > C->S: TLSv1.3 432 Change Cipher Spec, Client Hello
> > S->C: TLSv1.3 1407 Server Hello, Application Data, Application Data, Application Data, Application Data
>
> I wonder if this is the result of us still using TLS 1.2 as the default minimum
> protocol version.  In 1.2 the ClientHello doesn't contain the extensions for
> cipher and curves, so the server must reply with a HRR (Hello Retry Request)
> message asking the client to set protocol, curve and cipher.  The client will
> respond with a new Client Hello using 1.3 with the extensions.

I'm pretty sure it's not that, given that
a) removing the server side SSL_CTX_set_tmp_ecdh() call
b) adding a client side SSL_CTX_set_tmp_ecdh() call
avoid the roundtrip.

I've now also confirmed that ssl_min_protocol_version=TLSv1.3 doesn't change
anything (relevant, of course the indicated version support changes).


> > This appears to be caused by ECDH support.  The difference between the two
> > ClientHellos is
> > -            Extension: key_share (len=38) x25519
> > +            Extension: key_share (len=71) secp256r1
> >
> > I.e. the clients wanted to use x25519, but the server insists on secp256r1.
>
> Somewhat related, Erica Zhang has an open patch to make the server-side curves
> configuration take a list rather than a single curve [0], and modernizing the
> API used as a side effect (SSL_CTX_set_tmp_ecdh is documented as obsoleted by
> OpenSSL, but not deprecated with an API level).

That does seem nicer. Fun coincidence in timing.


> > I don't know if it's good that we're calling SSL_CTX_set_tmp_ecdh at all,
>
> To set the specified curve in ssl_ecdh_curve we have to don't we?

Sure, but it's not obvious to me why we actually want to override openssl's
defaults here. There's not even a parameter to opt out of forcing a specific
choice on the server side.


> > but if it is, shouldn't we at least do the same in libpq, so we don't introduce
> > unnecessary roundtrips?
>
> If we don't set the curve in the client I believe OpenSSL will pass the set of
> supported curves the client has, which then should allow the server to pick the
> one it wants based on ssl_ecdh_curve, so ideally we shouldn't have to I think.

Afaict the client sends exactly one:

Transport Layer Security
    TLSv1.3 Record Layer: Handshake Protocol: Client Hello
        Content Type: Handshake (22)
        Version: TLS 1.0 (0x0301)
        Length: 320
        Handshake Protocol: Client Hello
...
            Extension: supported_versions (len=5) TLS 1.3, TLS 1.2
                Type: supported_versions (43)
                Length: 5
                Supported Versions length: 4
                Supported Version: TLS 1.3 (0x0304)
                Supported Version: TLS 1.2 (0x0303)
            Extension: psk_key_exchange_modes (len=2)
                Type: psk_key_exchange_modes (45)
                Length: 2
                PSK Key Exchange Modes Length: 1
                PSK Key Exchange Mode: PSK with (EC)DHE key establishment (psk_dhe_ke) (1)
            Extension: key_share (len=38) x25519
                Type: key_share (51)
                Length: 38
                Key Share extension
            Extension: compress_certificate (len=5)
                Type: compress_certificate (27)
...

Note key_share being set to x25519.


The HRR says:

Extension: key_share (len=2) secp256r1
    Type: key_share (51)
    Length: 2
    Key Share extension
        Selected Group: secp256r1 (23)


> > I did confirm that doing the same thing on the client side removes the
> > additional roundtrip.
>
> The roundtrip went away because the client was set to use secp256r1?

Yes. Or if I change the server to not set the ecdh curve.


> I wonder if that made OpenSSL override the min protocol version and switch
> to a TLS1.3 ClientHello since it otherwise couldn't announce the curve.

The client seems to announce the curve in the initial ClientHello even with
1.3 as the minimum version.

What *does* make the HRR go away is setting ssl_max_protocol_version=TLSv1.2
on the client side.


https://wiki.openssl.org/index.php/TLS1.3 says:

> In practice most clients will use X25519 or P-256 for their initial
> key_share. For maximum performance it is recommended that servers are
> configured to support at least those two groups and clients use one of those
> two for its initial key_share. This is the default case (OpenSSL clients
> will use X25519).

We're not allowing both groups and the client defaults to X25519, hence
the HRR.


> If you force the client min protocol to 1.3 in an unpatched client, do you
> see the same speedup?

Nope.

Greetings,

Andres Freund

  4. Re: ecdh support causes unnecessary roundtrips

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-06-17T17:19:23Z

    On Mon, Jun 17, 2024 at 10:01 AM Andres Freund <andres@anarazel.de> wrote:
> On 2024-06-17 12:00:30 +0200, Daniel Gustafsson wrote:
> > To set the specified curve in ssl_ecdh_curve we have to don't we?
>
> Sure, but it's not obvious to me why we actually want to override openssl's
> defaults here. There's not even a parameter to opt out of forcing a specific
> choice on the server side.

I had exactly the same question in the context of the other thread, and found

    https://www.openssl.org/blog/blog/2022/10/21/tls-groups-configuration/index.html

My initial takeaway was that our default is more restrictive than it
should be, but the OpenSSL default is more permissive than what they
recommend in practice, due to denial of service concerns:

> A general recommendation is to limit the groups to those that meet the
> required security level and that all the potential TLS clients support.

--Jacob

  5. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2024-06-17T17:29:47Z

    > On 17 Jun 2024, at 19:01, Andres Freund <andres@anarazel.de> wrote:
> On 2024-06-17 12:00:30 +0200, Daniel Gustafsson wrote:
>>> On 17 Jun 2024, at 01:46, Andres Freund <andres@anarazel.de> wrote:

>>> I don't know if it's good that we're calling SSL_CTX_set_tmp_ecdh at all,
>> 
>> To set the specified curve in ssl_ecdh_curve we have to don't we?
> 
> Sure, but it's not obvious to me why we actually want to override openssl's
> defaults here. There's not even a parameter to opt out of forcing a specific
> choice on the server side.

I agree that the GUC is a bit rough around the edges, maybe leavint it blank or
something should be defined as "OpenSSL defaults".  Let's bring that to Erica's
patch for allowing a list of curves.

>>> I did confirm that doing the same thing on the client side removes the
>>> additional roundtrip.
>> 
>> The roundtrip went away because the client was set to use secp256r1?
> 
> Yes. Or if I change the server to not set the ecdh curve.

Configuring the server to use x25519 instead of secp256r1 should achieve the
same thing.

>> I wonder if that made OpenSSL override the min protocol version and switch
>> to a TLS1.3 ClientHello since it otherwise couldn't announce the curve.
> 
> The client seems to announce the curve in the initial ClientHello even with
> 1.3 as the minimum version.

With 1.3 it should announce it in ClientHello, do you mean that it's announced
when 1.2 is the minimum version as well?  It does make sense since a 1.2 server
is defined to disregard all extensions.

> What *does* make the HRR go away is setting ssl_max_protocol_version=TLSv1.2
> on the client side.

Makes sense, that would remove the curve and there is no change required.

> https://wiki.openssl.org/index.php/TLS1.3 says:
> 
>> In practice most clients will use X25519 or P-256 for their initial
>> key_share. For maximum performance it is recommended that servers are
>> configured to support at least those two groups and clients use one of those
>> two for its initial key_share. This is the default case (OpenSSL clients
>> will use X25519).
> 
> We're not allowing both groups and the client defaults to X25519, hence
> the HRR.

So this would be solved by the curve-list patch referenced above, especially if
allow it to have an opt-out to use OpenSSL defaults.

--
Daniel Gustafsson

  6. Re: ecdh support causes unnecessary roundtrips

    Andres Freund <andres@anarazel.de> — 2024-06-17T17:44:22Z

    Hi,

On 2024-06-17 19:29:47 +0200, Daniel Gustafsson wrote:
> >> I wonder if that made OpenSSL override the min protocol version and switch
> >> to a TLS1.3 ClientHello since it otherwise couldn't announce the curve.
> >
> > The client seems to announce the curve in the initial ClientHello even with
> > 1.3 as the minimum version.
>
> With 1.3 it should announce it in ClientHello, do you mean that it's announced
> when 1.2 is the minimum version as well?  It does make sense since a 1.2 server
> is defined to disregard all extensions.

Yes, it's announced even when 1.2 is the minimum:

            Extension: supported_versions (len=5) TLS 1.3, TLS 1.2
                Type: supported_versions (43)
                Length: 5
                Supported Versions length: 4
                Supported Version: TLS 1.3 (0x0304)
                Supported Version: TLS 1.2 (0x0303)
...
            Extension: key_share (len=38) x25519
                Type: key_share (51)
                Length: 38
                Key Share extension



> Let's bring that to Erica's patch for allowing a list of curves.

I'm kinda wondering if we ought to do something about this in the
backbranches. Forcing unnecessary roundtrips onto everyone for the next five
years due to an oversight on our part isn't great.  Once you're not local, the
roundtrip does measurably increase the "time to first query".

Greetings,

Andres Freund

  7. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2024-06-17T17:51:45Z

    > On 17 Jun 2024, at 19:44, Andres Freund <andres@anarazel.de> wrote:

>> Let's bring that to Erica's patch for allowing a list of curves.
> 
> I'm kinda wondering if we ought to do something about this in the
> backbranches. Forcing unnecessary roundtrips onto everyone for the next five
> years due to an oversight on our part isn't great.  Once you're not local, the
> roundtrip does measurably increase the "time to first query".

I don't disagree, but wouldn't it be the type of behavioural change which we
typically try to avoid in backbranches?  Changing the default of the ecdh GUC
would perhaps be doable?  (assuming that's a working solution to avoid the
roundtrip).  Amending the documentation is the one thing we certainly can do
but 99.9% of affected users won't know they are affected so won't look for that
section.

--
Daniel Gustafsson

  8. Re: ecdh support causes unnecessary roundtrips

    Andres Freund <andres@anarazel.de> — 2024-06-17T17:56:26Z

    Hi,

On 2024-06-17 19:51:45 +0200, Daniel Gustafsson wrote:
> > On 17 Jun 2024, at 19:44, Andres Freund <andres@anarazel.de> wrote:
> 
> >> Let's bring that to Erica's patch for allowing a list of curves.
> > 
> > I'm kinda wondering if we ought to do something about this in the
> > backbranches. Forcing unnecessary roundtrips onto everyone for the next five
> > years due to an oversight on our part isn't great.  Once you're not local, the
> > roundtrip does measurably increase the "time to first query".
> 
> I don't disagree, but wouldn't it be the type of behavioural change which we
> typically try to avoid in backbranches?

Yea, it's not great. Not sure what the right thing is here.


> Changing the default of the ecdh GUC would perhaps be doable?

I was wondering whether we could change the default so that it accepts both
x25519 and secp256r1. Unfortunately that seems to requires changing what we
use to set the parameter...


> (assuming that's a working solution to avoid the roundtrip).

It is.


> Amending the documentation is the one thing we certainly can do but 99.9% of
> affected users won't know they are affected so won't look for that section.

Yea. It's also possible that some other bindings changed their default to
match ours...

Greetings,

Andres Freund

  9. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-29T22:25:59Z

    > On 17 Jun 2024, at 19:56, Andres Freund <andres@anarazel.de> wrote:
> On 2024-06-17 19:51:45 +0200, Daniel Gustafsson wrote:

>> Changing the default of the ecdh GUC would perhaps be doable?
> 
> I was wondering whether we could change the default so that it accepts both
> x25519 and secp256r1. Unfortunately that seems to requires changing what we
> use to set the parameter...

Right.  The patch in https://commitfest.postgresql.org/48/5025/ does allow for
accepting both but that's a different discussion.

Changing, and backpatching, the default to at least keep new installations from
extra roundtrips doesn't seem that far off in terms of scope from what
860fe27ee1e2 backpatched.  Maybe it can be an option.

>> Amending the documentation is the one thing we certainly can do but 99.9% of
>> affected users won't know they are affected so won't look for that section.
> 
> Yea. It's also possible that some other bindings changed their default to
> match ours...

There is that possibility, though I think we would've heard something about
that by now if that had happened.

--
Daniel Gustafsson

  10. Re: ecdh support causes unnecessary roundtrips

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-04T19:13:34Z

    On Mon, Jul 29, 2024 at 3:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 17 Jun 2024, at 19:56, Andres Freund <andres@anarazel.de> wrote:
> > On 2024-06-17 19:51:45 +0200, Daniel Gustafsson wrote:
>
> >> Changing the default of the ecdh GUC would perhaps be doable?
> >
> > I was wondering whether we could change the default so that it accepts both
> > x25519 and secp256r1. Unfortunately that seems to requires changing what we
> > use to set the parameter...
>
> Right.  The patch in https://commitfest.postgresql.org/48/5025/ does allow for
> accepting both but that's a different discussion.

Just a reminder that, if we do want to change this for 18 onward, the
window is closing. Adding x25519 to the default groups seems like a
good idea to me, whether we can get something backpatched or not.

Thanks,
--Jacob

  11. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-04T19:19:47Z

    > On 4 Mar 2025, at 20:13, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Mon, Jul 29, 2024 at 3:26 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> On 17 Jun 2024, at 19:56, Andres Freund <andres@anarazel.de> wrote:
>>> On 2024-06-17 19:51:45 +0200, Daniel Gustafsson wrote:
>> 
>>>> Changing the default of the ecdh GUC would perhaps be doable?
>>> 
>>> I was wondering whether we could change the default so that it accepts both
>>> x25519 and secp256r1. Unfortunately that seems to requires changing what we
>>> use to set the parameter...
>> 
>> Right.  The patch in https://commitfest.postgresql.org/48/5025/ does allow for
>> accepting both but that's a different discussion.
> 
> Just a reminder that, if we do want to change this for 18 onward, the
> window is closing. Adding x25519 to the default groups seems like a
> good idea to me, whether we can get something backpatched or not.

Thanks for the reminder, this is sitting on my must-have TODO for 18 and I
agree that we should add x25519 to the default set.

--
Daniel Gustafsson

  12. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-05T00:04:57Z

    > On 4 Mar 2025, at 20:19, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 4 Mar 2025, at 20:13, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

>> Just a reminder that, if we do want to change this for 18 onward, the
>> window is closing. Adding x25519 to the default groups seems like a
>> good idea to me, whether we can get something backpatched or not.
> 
> Thanks for the reminder, this is sitting on my must-have TODO for 18 and I
> agree that we should add x25519 to the default set.

And to add some code for that proposal, the attached adds it to the default set.

--
Daniel Gustafsson

  13. Re: ecdh support causes unnecessary roundtrips

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-13T21:39:35Z

    On Tue, Mar 4, 2025 at 4:05 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 4 Mar 2025, at 20:19, Daniel Gustafsson <daniel@yesql.se> wrote:
> > Thanks for the reminder, this is sitting on my must-have TODO for 18 and I
> > agree that we should add x25519 to the default set.
>
> And to add some code for that proposal, the attached adds it to the default set.

In mundane-annoyances news, LibreSSL apparently does not accept
"x25519" and wants "X25519" instead. :D

--Jacob

  14. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-13T21:41:12Z

    > On 13 Mar 2025, at 22:39, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Tue, Mar 4, 2025 at 4:05 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> On 4 Mar 2025, at 20:19, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> Thanks for the reminder, this is sitting on my must-have TODO for 18 and I
>>> agree that we should add x25519 to the default set.
>> 
>> And to add some code for that proposal, the attached adds it to the default set.
> 
> In mundane-annoyances news, LibreSSL apparently does not accept
> "x25519" and wants "X25519" instead. :D

OpenSSL 3.4 also doesn't like it and AFAICT neither does the upcoming 3.5, just
haven't had the cycles yet to ship out a new patch with all the time-consuming
testing it requires =)

--
Daniel Gustafsson

  15. Re: ecdh support causes unnecessary roundtrips

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-13T23:31:08Z

    On Thu, Mar 13, 2025 at 2:41 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> OpenSSL 3.4 also doesn't like it and AFAICT neither does the upcoming 3.5

Hm. FWIW, I have no issues locally with 3.4 or 3.5-alpha. Only with LibreSSL.

--Jacob

  16. Re: ecdh support causes unnecessary roundtrips

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-17T23:34:45Z

    On Thu, Mar 13, 2025 at 2:41 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> OpenSSL 3.4 also doesn't like it and AFAICT neither does the upcoming 3.5, just
> haven't had the cycles yet to ship out a new patch with all the time-consuming
> testing it requires =)

Here is a squash fix to change the capitalization -- this passes
locally for LibreSSL 3.4 to 3.8 and OpenSSL 1.1.1 to 3.5.

(Hmm, my LibreSSLs are a little out of date; it looks like I need to
add 3.9 and 4.0 to the suite.)

Thanks,
--Jacob

  17. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-18T09:45:41Z

    > On 18 Mar 2025, at 00:34, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Thu, Mar 13, 2025 at 2:41 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> OpenSSL 3.4 also doesn't like it and AFAICT neither does the upcoming 3.5, just
>> haven't had the cycles yet to ship out a new patch with all the time-consuming
>> testing it requires =)
> 
> Here is a squash fix to change the capitalization -- this passes
> locally for LibreSSL 3.4 to 3.8 and OpenSSL 1.1.1 to 3.5.
> 
> (Hmm, my LibreSSLs are a little out of date; it looks like I need to
> add 3.9 and 4.0 to the suite.)

Thanks for doing that, I'll try to get this in during a break in todays conference.

--
Daniel Gustafsson

  18. Re: ecdh support causes unnecessary roundtrips

    Andres Freund <andres@anarazel.de> — 2025-03-18T15:07:14Z

    Hi,

On 2025-03-18 10:45:41 +0100, Daniel Gustafsson wrote:
> Thanks for doing that, I'll try to get this in during a break in todays conference.

Thanks to both of you for fixing this!


I wonder how we could make it easier to find stuff like this and 274bbced853,
it's pretty painful right now. The SSLKEYLOGFILE stuff being discussed would
help, but would still require somebody breaking out wireshark or such.
Perhaps some added trace messages or such?

Greetings,

Andres

  19. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-18T15:25:37Z

    > On 18 Mar 2025, at 16:07, Andres Freund <andres@anarazel.de> wrote:
> On 2025-03-18 10:45:41 +0100, Daniel Gustafsson wrote:
>> Thanks for doing that, I'll try to get this in during a break in todays conference.
> 
> Thanks to both of you for fixing this!

No worries, this has now been committed.  Whether or not we can do anything for
backbranches is another discussion.

> I wonder how we could make it easier to find stuff like this and 274bbced853,
> it's pretty painful right now. The SSLKEYLOGFILE stuff being discussed would
> help, but would still require somebody breaking out wireshark or such.
> Perhaps some added trace messages or such?

Maybe, but I fear that most of the interesting tracepoints would be inside
OpenSSL rather than in libpq. Definitely worth investigatint though.

--
Daniel Gustafsson

  20. Re: ecdh support causes unnecessary roundtrips

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-09T19:41:33Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 18 Mar 2025, at 16:07, Andres Freund <andres@anarazel.de> wrote:
>> Thanks to both of you for fixing this!

> No worries, this has now been committed.  Whether or not we can do anything for
> backbranches is another discussion.

I happened to discover that daa02c6bd causes us to fail on FIPS-mode
platforms, because X25519 is not a permitted ECDH curve under FIPS.
SSL connection attempts fail with

2026-02-09 14:29:05.214 EST postmaster[118237] FATAL:  could not set group names specified in ssl_groups: passed invalid argument
2026-02-09 14:29:05.214 EST postmaster[118237] HINT:  Ensure that each group name is spelled correctly and supported by the installed version of OpenSSL.

(This is with OpenSSL 3.0.9 on Fedora 38.)

The reason this eluded detection by the buildfarm is that
(a) our animals that run on FIPS-mode platforms aren't using
--enable-tap-tests, and (b) ssl_groups is not validated in any way
until we try to load it into an SSL context, ie at the moment of
actually receiving an SSL connection.

I don't object to X25519 being in the default setting, given that it
seems to be widely used.  But I think we had better (1) document that
you need to remove it if you want to run under FIPS, and (2) fix our
SSL-using regression tests to not use it.  I wonder also if we could
find a way to validate the ssl_groups setting in a check_hook.

			regards, tom lane

  21. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2026-02-09T19:56:13Z

    > On 9 Feb 2026, at 20:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 18 Mar 2025, at 16:07, Andres Freund <andres@anarazel.de> wrote:
>>> Thanks to both of you for fixing this!
> 
>> No worries, this has now been committed.  Whether or not we can do anything for
>> backbranches is another discussion.
> 
> I happened to discover that daa02c6bd causes us to fail on FIPS-mode
> platforms, because X25519 is not a permitted ECDH curve under FIPS.

Ugh, seems I missed testing this with a FIPS enabled setup..

> SSL connection attempts fail with
> 
> 2026-02-09 14:29:05.214 EST postmaster[118237] FATAL:  could not set group names specified in ssl_groups: passed invalid argument
> 2026-02-09 14:29:05.214 EST postmaster[118237] HINT:  Ensure that each group name is spelled correctly and supported by the installed version of OpenSSL.
> 
> (This is with OpenSSL 3.0.9 on Fedora 38.)
> 
> The reason this eluded detection by the buildfarm is that
> (a) our animals that run on FIPS-mode platforms aren't using
> --enable-tap-tests, and (b) ssl_groups is not validated in any way
> until we try to load it into an SSL context, ie at the moment of
> actually receiving an SSL connection.
> 
> I don't object to X25519 being in the default setting, given that it
> seems to be widely used.  But I think we had better (1) document that
> you need to remove it if you want to run under FIPS, and (2) fix our
> SSL-using regression tests to not use it.  I wonder also if we could
> find a way to validate the ssl_groups setting in a check_hook.

Maybe we can create a lightweight throw-away context in a check hook and ensure
the settings work?  Are you hacking on it or do you want me to pick it up?

--
Daniel Gustafsson

  22. Re: ecdh support causes unnecessary roundtrips

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-09T20:01:09Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 9 Feb 2026, at 20:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't object to X25519 being in the default setting, given that it
>> seems to be widely used.  But I think we had better (1) document that
>> you need to remove it if you want to run under FIPS, and (2) fix our
>> SSL-using regression tests to not use it.  I wonder also if we could
>> find a way to validate the ssl_groups setting in a check_hook.

> Maybe we can create a lightweight throw-away context in a check hook and ensure
> the settings work?

Yeah, I was envisioning something like that.  The main trick would be
to ensure that we can't error out, but given that we'd mostly be
calling OpenSSL code, ensuring that there's no ereport(ERROR)
shouldn't be too hard.

But I'd counsel getting the easy bits (1) and (2) out of the way
first.

> Are you hacking on it or do you want me to pick it up?

I was not planning to work on that.

			regards, tom lane

  23. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2026-02-13T22:32:18Z

    >> Maybe we can create a lightweight throw-away context in a check hook and ensure
>> the settings work?
> 
> Yeah, I was envisioning something like that.  The main trick would be
> to ensure that we can't error out, but given that we'd mostly be
> calling OpenSSL code, ensuring that there's no ereport(ERROR)
> shouldn't be too hard.

This is sort being added as already as part of the SNI patchset, so I'll see if
I can steal something from there in case that seems to miss the v19 train.

> But I'd counsel getting the easy bits (1) and (2) out of the way
> first.

Absolutely, the attached is what I had planned for addressing this.

--
Daniel Gustafsson

  24. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2026-02-20T12:33:16Z

    When testing before pushing I ran the ssl tests, but my colleague Nazir Bilal
Yavuz found that two additional test suites need FIPS adjustment.  I will apply
the attached shortly to fix those as well. Sorry for missing these when testing.

--
Daniel Gustafsson

  25. Re: ecdh support causes unnecessary roundtrips

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-20T14:58:28Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> When testing before pushing I ran the ssl tests, but my colleague Nazir Bilal
> Yavuz found that two additional test suites need FIPS adjustment.  I will apply
> the attached shortly to fix those as well. Sorry for missing these when testing.

I think this is whack-a-mole.  What I was thinking of proposing
was to conditionally alter the wired-in default, along the lines
of

 #ifdef USE_SSL
+#ifdef PG_FIPS_COMPLIANT
+#define DEFAULT_SSL_GROUPS "prime256v1"
+#else
 #define DEFAULT_SSL_GROUPS "X25519:prime256v1"
+#endif
 #else
 #define DEFAULT_SSL_GROUPS "none"
 #endif

and then people wanting to test on FIPS platforms could just add
-DPG_FIPS_COMPLIANT to their build recipes.

			regards, tom lane

  26. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2026-02-20T15:51:03Z

    > On 20 Feb 2026, at 15:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> and then people wanting to test on FIPS platforms could just add
> -DPG_FIPS_COMPLIANT to their build recipes.

I don't think we will gain much testing that way.  My proposal is to ensure
that the tests always pass with FIPS enabled coupled with a patch, which Bilal
is currently working on, to switch one of the CI jobs to use a FIPS enabled
OpenSSL so that we get ongoing testing of such configurations.

--
Daniel Gustafsson

  27. Re: ecdh support causes unnecessary roundtrips

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-20T16:07:42Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 20 Feb 2026, at 15:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> and then people wanting to test on FIPS platforms could just add
>> -DPG_FIPS_COMPLIANT to their build recipes.

> I don't think we will gain much testing that way.  My proposal is to ensure
> that the tests always pass with FIPS enabled coupled with a patch, which Bilal
> is currently working on, to switch one of the CI jobs to use a FIPS enabled
> OpenSSL so that we get ongoing testing of such configurations.

My concern about the fix you suggest is that we won't be testing the
same thing that people in the field will be using.  Admittedly, any
discrepancy would probably be OpenSSL's bug not ours, but that doesn't
make it a good thing.  I'd rather test the normal configuration
normally and make people who want to run the test on a FIPS platform
do something different.

			regards, tom lane

  28. Re: ecdh support causes unnecessary roundtrips

    Daniel Gustafsson <daniel@yesql.se> — 2026-02-20T17:53:19Z

    > On 20 Feb 2026, at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> My concern about the fix you suggest is that we won't be testing the
> same thing that people in the field will be using.

Yes and no.  Folks can configure this (and other ssl_* settings) in lots of
different way which are all disjoint from our default.

> I'd rather test the normal configuration
> normally and make people who want to run the test on a FIPS platform
> do something different.

How about a function in Cluster.pm which returns whether the underlying OpenSSL
is using FIPS or not, and if it does we adjust the config to make it not fail
on an unallowed group?  That way we can have a CI job that runs with FIPS and
the adjusted test config, and the rest - along with the Buildfarm - runs the
default config.

--
Daniel Gustafsson

  29. Re: ecdh support causes unnecessary roundtrips

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-02-20T19:17:42Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 20 Feb 2026, at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'd rather test the normal configuration
>> normally and make people who want to run the test on a FIPS platform
>> do something different.

> How about a function in Cluster.pm which returns whether the underlying OpenSSL
> is using FIPS or not, and if it does we adjust the config to make it not fail
> on an unallowed group?  That way we can have a CI job that runs with FIPS and
> the adjusted test config, and the rest - along with the Buildfarm - runs the
> default config.

If we can get that information easily, then sure.

BTW, I think we should be back-patching these fixes into v18, since
the testing hazard exists there too.  But maybe wait till after the
re-release is finished.

			regards, tom lane