Re: Add support to TLS 1.3 cipher suites and curves lists

> On 18 Sep 2024, at 22:48, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> On Mon, Sep 9, 2024 at 5:00 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

>> The attached version also has a new 0001 which bumps the minimum required
>> OpenSSL version to 1.1.1 (from 1.1.0) since this patchset requires API's only
>> present in 1.1.1 and onwards. To keep it from being hidden here I will raise a
>> separate thread about it.
>
> As implemented, my build matrix is no longer able to compile against
> LibreSSL 3.3 and below (OpenBSD 6.x). Has the lower bound on LibreSSL
> for PG18 been discussed yet?

I can't recall specific bounds for supporting LibreSSL even being discussed,
the support is also not documented as an official thing. Requiring TLS 1.3
APIs for supporting a library in 2025 (when 18 ships) doesn't seem entirely
unreasonable so maybe 3.4 is a good cutoff. The fact that LibreSSL trailed
behind OpenSSL in adding these APIs shouldn't limit our functionality.

>> +#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed TLSv1.2 ciphers
>> +#ssl_cipher_suites = '' # allowed TLSv1.3 cipher suites, blank for default
>
> After marinating on this a bit... I think the naming may result in
> some "who's on first" miscommunications in forums and on the list. "I
> set the SSL ciphers to <whatever>, but it says there are no valid
> ciphers available!" Should we put TLS 1.3 into the new GUC name
> somehow?

Yeah, I don't disagree with your concern. Thinking on it a bit I went (to some
degree inspired by what we did in curl) with ssl_tls13_ciphers which makes the
name very similar to the tls12 GUC but with the clear distinction of being
protocol specific. It also makes the GUC name more readable to place the
protocol before "ciphers" I think.

>> + {"ssl_groups", PGC_SIGHUP, CONN_AUTH_SSL,
>> + gettext_noop("Sets the curve(s) to use for ECDH."),
>
> The ECDH reference should probably be updated/removed. Maybe something
> like "Sets the group(s) to use for Diffie-Hellman key exchange." ?

Done.

>> +#if (OPENSSL_VERSION_NUMBER <= 0x30200000L)
>> + /*
>> + * OpenSSL 3.3.0 introduced proper error messages for group
>> + * parsing errors, earlier versions returns "no SSL error
>> + * reported" which is far from helpful. For older versions, we
>> + * manually set a better error message. Injecting the error
>> + * into the OpenSSL error queue need APIs from OpenSSL 3.0.
>> + */
>> + errmsg("ECDH: failed to set curve names: No valid groups in '%s'",
>> + SSLECDHCurve),
>
> nit: can we do this only when ERR_get_error() returns zero? It looks
> like LibreSSL is stuck at OPENSSL_VERSION_NUMBER == 0x20000000, so if
> they introduce a nice error message at some point it'll still get
> ignored.

We can do that, I'm not going to hold my breath on LibreSSL doing that but it
has the benefit of using the API and not hardcoded version knowledge. I ended
up adding a version of SSLerrmessage which takes a replacement string for ecode
0 (which admittedly is hardcoded version knowledge as well..). This can be
used for scenarios when it's known that OpenSSL sometimes reports and error and
sometimes not (I'm sure there are quite a few more).

>> + &SSLCipherLists,
>
> nit: a singular "SSLCipherList" would be clearer, IMO.

Done.

> Looking at the commit messages:
>
>> Support configuring multiple ECDH curves
>>
>> The ssl_ecdh_curve only GUC accepts a single value, but the TLS
>
> "GUC" and "only" are transposed here.

Fixed.

>> Support configuring TLSv1.3 cipher suites
>>
>> The ssl_ciphers GUC can only set cipher suites for TLSv1.2, and lower,
>> connections. For TLSv1.3 connections a different OpenSSL must be used.
>
> "a different OpenSSL API", maybe?

Fixed.

--
Daniel Gustafsson