Обсуждение: ecdh support causes unnecessary roundtrips

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

> 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

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

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

> 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

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

> 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

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