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

Hi,
Thanks a lot for the review.
Indeed the original ssl_ecdh_curve is used to set a single value of curve name. If we want to change it to indicate a list of curve names, is there any rule for naming in Postgres? like ssl_curve_groups?

Original Email

From:"Andres Freund"< andres(at)anarazel(dot)de &gt;;

Sent Time:2024/6/18 2:48

To:"Erica Zhang"< ericazhangy2021(at)qq(dot)com &gt;;

Cc recipient:"Jelte Fennema-Nio"< postgres(at)jeltef(dot)nl &gt;;"Daniel Gustafsson"< daniel(at)yesql(dot)se &gt;;"Jacob Champion"< jacob(dot)champion(at)enterprisedb(dot)com &gt;;"Peter Eisentraut"< peter(at)eisentraut(dot)org &gt;;"pgsql-hackers"< pgsql-hackers(at)lists(dot)postgresql(dot)org &gt;;

Subject: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:

&gt; diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
&gt; index 39b1a66236..d097e81407 100644
&gt; --- a/src/backend/libpq/be-secure-openssl.c
&gt; +++ b/src/backend/libpq/be-secure-openssl.c
&gt; @@ -1402,30 +1402,30 @@ static bool
&gt; initialize_ecdh(SSL_CTX *context, bool isServerStart)
&gt; {
&gt; #ifndef OPENSSL_NO_ECDH
&gt; - EC_KEY *ecdh;
&gt; - int nid;
&gt; + 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)?

&gt; + char *saveptr;
&gt; + char *token = strtok_r(curve_list, ":", &amp;saveptr);
&gt; + int nid;
&gt;
&gt; - nid = OBJ_sn2nid(SSLECDHCurve);
&gt; - if (!nid)
&gt; + 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().

&gt; {
&gt; - ereport(isServerStart ? FATAL : LOG,
&gt; + nid = OBJ_sn2nid(token);
&gt; + if (!nid)
&gt; + {ereport(isServerStart ? FATAL : LOG,
&gt; (errcode(ERRCODE_CONFIG_FILE_ERROR),
&gt; - errmsg("ECDH: unrecognized curve name: %s", SSLECDHCurve)));
&gt; + errmsg("ECDH: unrecognized curve name: %s", token)));
&gt; return false;
&gt; + }
&gt; + token = strtok_r(NULL, ":", &amp;saveptr);
&gt; }
&gt;
&gt; - ecdh = EC_KEY_new_by_curve_name(nid);
&gt; - if (!ecdh)
&gt; + if(SSL_CTX_set1_groups_list(context, SSLECDHCurve) !=1)
&gt; {
&gt; ereport(isServerStart ? FATAL : LOG,
&gt; (errcode(ERRCODE_CONFIG_FILE_ERROR),
&gt; - errmsg("ECDH: could not create key")));
&gt; + 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