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

Hi,

This thread was referenced by https://www.postgresql.org/message-id/48F0A1F8-E0B4-41F8-990F-41E6BA2A6185%40yesql.se

On 2024-06-13 14:34:27 +0800, Erica Zhang wrote:

> diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
> index 39b1a66236..d097e81407 100644
> --- a/src/backend/libpq/be-secure-openssl.c
> +++ b/src/backend/libpq/be-secure-openssl.c
> @@ -1402,30 +1402,30 @@ static bool
> initialize_ecdh(SSL_CTX *context, bool isServerStart)
> {
> #ifndef OPENSSL_NO_ECDH
> - EC_KEY *ecdh;
> - int nid;
> + char *curve_list = strdup(SSLECDHCurve);

ISTM we'd want to eventually rename the GUC variable to indicate it's a list?
I think the "ecdh" portion is actually not accurate anymore either, it's used
outside of ecdh if I understand correctly (probably I am not)?

> + char *saveptr;
> + char *token = strtok_r(curve_list, ":", &saveptr);
> + int nid;
>
> - nid = OBJ_sn2nid(SSLECDHCurve);
> - if (!nid)
> + while (token != NULL)

It'd be good to have a comment explaining why we're parsing the list ourselves
instead of using just the builtin SSL_CTX_set1_groups_list().

> {
> - ereport(isServerStart ? FATAL : LOG,
> + nid = OBJ_sn2nid(token);
> + if (!nid)
> + {ereport(isServerStart ? FATAL : LOG,
> (errcode(ERRCODE_CONFIG_FILE_ERROR),
> - errmsg("ECDH: unrecognized curve name: %s", SSLECDHCurve)));
> + errmsg("ECDH: unrecognized curve name: %s", token)));
> return false;
> + }
> + token = strtok_r(NULL, ":", &saveptr);
> }
>
> - ecdh = EC_KEY_new_by_curve_name(nid);
> - if (!ecdh)
> + if(SSL_CTX_set1_groups_list(context, SSLECDHCurve) !=1)
> {
> ereport(isServerStart ? FATAL : LOG,
> (errcode(ERRCODE_CONFIG_FILE_ERROR),
> - errmsg("ECDH: could not create key")));
> + errmsg("ECDH: failed to set curve names")));

Probably worth including the value of the GUC here?

This also needs to change the documentation for the GUC.

Once we have this parameter we probably should add at least x25519 to the
allowed list, as that's the client side default these days.

But that can be done in a separate patch.

Greetings,

Andres Freund