Re: ecdh support causes unnecessary roundtrips

Hi,

On 2024-06-17 12:00:30 +0200, Daniel Gustafsson wrote:
> > On 17 Jun 2024, at 01:46, Andres Freund <andres(at)anarazel(dot)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