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

I had a look at this patchset today and I think I've come around to the idea of
having a separate GUC for cipher suites. I don't have strong opinions on
renaming ssl_ecdh_curve to reflect that it can take a list of multiple values,
there is merit to having descriptive names but it would also be an invasive
change for adding suffix 's'.

After fiddling a bit with the code and documentation I came up with the
attached version which also makes the testsuite use the list syntax in order to
test it. It's essentially just polish and adding comments with the functional
changes that a) it parses the entire list of curves so all errors can be
reported instead of giving up at the first error; b) leaving the cipher suite
GUC blank will set the suites to the OpenSSL default vale.

This patch requires OpenSSL 1.1.1 as the minimum version, which in my view is
fine. Removing support for older OpenSSL versions is being discussed already
and this makes a good case for requiring 1.1.1. It does however mean that this
patch cannot be commmitted until that has been done though. I have yet to test
this with LibreSSL.

As was suggested in a related thread I think we should change the default value
of the ECDH curves parameter, but that's for another patch.

--
Daniel Gustafsson

On 03.07.24 17:20, Daniel Gustafsson wrote:
> After fiddling a bit with the code and documentation I came up with the
> attached version which also makes the testsuite use the list syntax in order to
> test it. It's essentially just polish and adding comments with the functional
> changes that a) it parses the entire list of curves so all errors can be
> reported instead of giving up at the first error; b) leaving the cipher suite
> GUC blank will set the suites to the OpenSSL default vale.

It would be worth checking the discussion at
<https://www.postgresql.org/message-id/flat/79692bf9-17d3-41e6-b9c9-fc8c3944222a(at)eisentraut(dot)org>
about strtok()/strtok_r() issues. First, for list parsing, it sometimes
gives the wrong semantics, which I think might apply here. Maybe it's
worth comparing this with the semantics that OpenSSL provides natively.
And second, strtok_r() is not available on Windows without the
workaround provided in that thread.

I'm doubtful that it's worth replicating all this list parsing logic
instead of just letting OpenSSL do it. This is a very marginal feature
after all.

> On 11 Jul 2024, at 23:16, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> It would be worth checking the discussion at <https://www.postgresql.org/message-id/flat/79692bf9-17d3-41e6-b9c9-fc8c3944222a(at)eisentraut(dot)org> about strtok()/strtok_r() issues. First, for list parsing, it sometimes gives the wrong semantics, which I think might apply here. Maybe it's worth comparing this with the semantics that OpenSSL provides natively. And second, strtok_r() is not available on Windows without the workaround provided in that thread.
>
> I'm doubtful that it's worth replicating all this list parsing logic instead of just letting OpenSSL do it. This is a very marginal feature after all.

The original author added the string parsing in order to provide a good error
message in case of an error in the list, and since that seemed like a nice idea
I kept in my review revision. With what you said above I agree it's not worth
the extra complexity it brings so the attached revision removes it.

--
Daniel Gustafsson

On Fri, Jul 12, 2024 at 1:03 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> The original author added the string parsing in order to provide a good error
> message in case of an error in the list, and since that seemed like a nice idea
> I kept in my review revision. With what you said above I agree it's not worth
> the extra complexity it brings so the attached revision removes it.

Misspelling a group now leads to the following error message for OpenSSL 3.0:

FATAL: ECDH: failed to set curve names: no SSL error reported

Maybe a HINT would be nice here?:

HINT: Check that each group name is both spelled correctly and
supported by the installed version of OpenSSL.

or something.

> I don't have strong opinions on
> renaming ssl_ecdh_curve to reflect that it can take a list of multiple values,
> there is merit to having descriptive names but it would also be an invasive
> change for adding suffix 's'.

Can we just add an entry to map_old_guc_names to handle it? Something
like (untested)

static const char *const map_old_guc_names[] = {
"sort_mem", "work_mem",
"vacuum_mem", "maintenance_work_mem",
+ "ssl_ecdh_curve", "ssl_groups",
NULL
};

Re: Andres' concern about the ECDH part of the name, we could probably
keep the "dh" part, but I'd be wary of that changing underneath us
too. IANA changed the registry name to "TLS Supported Groups".

Thanks,
--Jacob

On Wed, Jul 3, 2024 at 9:20 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> It's essentially just polish and adding comments with the functional
> changes that a) it parses the entire list of curves so all errors can be
> reported instead of giving up at the first error; b) leaving the cipher suite
> GUC blank will set the suites to the OpenSSL default vale.

Is there an advantage to setting it to a compile-time default, as
opposed to just leaving it alone and not setting it at all? With the
current patch, if you dropped in a more advanced OpenSSL 3.x that
changed up the defaults, you wouldn't see any benefit.

Thanks,
--Jacob

> On 22 Jul 2024, at 19:14, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> On Fri, Jul 12, 2024 at 1:03 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> The original author added the string parsing in order to provide a good error
>> message in case of an error in the list, and since that seemed like a nice idea
>> I kept in my review revision. With what you said above I agree it's not worth
>> the extra complexity it brings so the attached revision removes it.
>
> Misspelling a group now leads to the following error message for OpenSSL 3.0:
>
> FATAL: ECDH: failed to set curve names: no SSL error reported
>
> Maybe a HINT would be nice here?:
>
> HINT: Check that each group name is both spelled correctly and
> supported by the installed version of OpenSSL.

Good catch. OpenSSL 3.2 changed the error message to be a lot more helpful,
before that there is no error added to the queue at all for this processing
(hence the "no SSL error reported"). The attached adds a hint as well as a
proper error message for OpenSSL versions prior to 3.2. Pushing an error on
the queue would've been nice but we can't replicate the OpenSSL level of detail
in the error until we require OpenSSL 3.0 as the base since that's when _data
error reporting was added.

>> I don't have strong opinions on
>> renaming ssl_ecdh_curve to reflect that it can take a list of multiple values,
>> there is merit to having descriptive names but it would also be an invasive
>> change for adding suffix 's'.
>
> Can we just add an entry to map_old_guc_names to handle it? Something
> like (untested)
>
> static const char *const map_old_guc_names[] = {
> "sort_mem", "work_mem",
> "vacuum_mem", "maintenance_work_mem",
> + "ssl_ecdh_curve", "ssl_groups",
> NULL
> };
>
> Re: Andres' concern about the ECDH part of the name, we could probably
> keep the "dh" part, but I'd be wary of that changing underneath us
> too. IANA changed the registry name to "TLS Supported Groups".

Fair point, I've renamed to ssl_groups and added a mapping from the old name as
well as a note in the docs that the parameter has changed name (and ability to
handle more than one).

> Is there an advantage to setting it to a compile-time default, as
> opposed to just leaving it alone and not setting it at all? With the
> current patch, if you dropped in a more advanced OpenSSL 3.x that
> changed up the defaults, you wouldn't see any benefit.

Not really, I have changed such that a blank GUC does *no* OpenSSL call at all
which will retain the default from the local OpenSSL installation.

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.

--
Daniel Gustafsson

On Mon, Sep 9, 2024 at 5:00 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> Good catch. OpenSSL 3.2 changed the error message to be a lot more helpful,
> before that there is no error added to the queue at all for this processing
> (hence the "no SSL error reported"). The attached adds a hint as well as a
> proper error message for OpenSSL versions prior to 3.2.

Thanks!

> 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?

> +#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?

> + {"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." ?

> +#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.

> + &SSLCipherLists,

nit: a singular "SSLCipherList" would be clearer, IMO.

--

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.

> 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?

Thanks,
--Jacob

On 18.09.24 22:48, Jacob Champion wrote:
>> +#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 think just

ssl_ciphers =
ssl_ciphers_tlsv13 =

would be clear enough. Just using "ciphers" vs. "cipher suites" would
not be.

> 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

Attached is a v7 which address a test failure in the CI. It turns out that the
test_misc module gather GUC names using the :alpha: character class which only
allows alphabetic whereas GUC names can have digits in them. The 0001 patch
fixes this by instead using the :alnum: character class which allows all
alphanumeric characters. This is not directly related to this patch, it just
happened to be exposed by it.

--
Daniel Gustafsson

On Wed, Sep 25, 2024 at 6:39 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> 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.

Okay. At minimum I think we'll lose conchuela, plover, and morepork
from the master builds until they are updated. schnauzer is new enough
to keep going.

> 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.

Looks fine to me.

> 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).

I like this new API! And yeah, I think it'll get more use elsewhere.

My only nitpick for this particular error message is that there's no
longer any breadcrumb back to the setting that's broken:

FATAL: ECDH: failed to set curve names: No valid groups found
HINT: Ensure that each group name is spelled correctly and
supported by the installed version of OpenSSL

If I migrate a server to a different machine that doesn't support my
groups, I don't know that this would give me enough information to fix
the configuration.

--

One nice side effect of the new ssl_groups implementation is that we
now support common group aliases. For example, "P-256", "prime256v1",
and "secp256r1" can all be specified now, whereas before ony
"prime256v1" worked because of how we looked up curves. Is that worth
a note in the docs? Even if not, it might be good to keep in mind for
support threads.

Thanks,
--Jacob

> On 2 Oct 2024, at 19:16, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> On Wed, Sep 25, 2024 at 6:39 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> 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.
>
> Okay. At minimum I think we'll lose conchuela, plover, and morepork
> from the master builds until they are updated. schnauzer is new enough
> to keep going.

I will raise it on the thread where bumping to 1.1.1 as the lowest supported
version to make sure it doesn't land as a surprise.

> My only nitpick for this particular error message is that there's no
> longer any breadcrumb back to the setting that's broken:
>
> FATAL: ECDH: failed to set curve names: No valid groups found
> HINT: Ensure that each group name is spelled correctly and
> supported by the installed version of OpenSSL
>
> If I migrate a server to a different machine that doesn't support my
> groups, I don't know that this would give me enough information to fix
> the configuration.

Fair point, how about something along the lines of:

+ errmsg("ECDH: failed to set curve names specified in ssl_groups: %s",
+ SSLerrmessageExt(ERR_get_error(),
+ _("No valid groups found"))),

> One nice side effect of the new ssl_groups implementation is that we
> now support common group aliases. For example, "P-256", "prime256v1",
> and "secp256r1" can all be specified now, whereas before ony
> "prime256v1" worked because of how we looked up curves. Is that worth
> a note in the docs?

Maybe. We have this currently in the manual:

"The full list of available curves can be shown with the command
<command>openssl ecparam -list_curves</command>. Not all of them are
usable with <acronym>TLS</acronym> though."

Perhaps we can extend that with a short not on aliases? Got any suggested
wordings for that if so?

--
Daniel Gustafsson

On Wed, Oct 2, 2024 at 11:33 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> > If I migrate a server to a different machine that doesn't support my
> > groups, I don't know that this would give me enough information to fix
> > the configuration.
>
> Fair point, how about something along the lines of:
>
> + errmsg("ECDH: failed to set curve names specified in ssl_groups: %s",
> + SSLerrmessageExt(ERR_get_error(),
> + _("No valid groups found"))),

Yeah, I think that's enough of a pointer. And then maybe "Failed to
set group names specified in ssl_groups: %s" to get rid of the
lingering ECC references?

> > One nice side effect of the new ssl_groups implementation is that we
> > now support common group aliases. For example, "P-256", "prime256v1",
> > and "secp256r1" can all be specified now, whereas before ony
> > "prime256v1" worked because of how we looked up curves. Is that worth
> > a note in the docs?
>
> Maybe. We have this currently in the manual:
>
> "The full list of available curves can be shown with the command
> <command>openssl ecparam -list_curves</command>. Not all of them are
> usable with <acronym>TLS</acronym> though."
>
> Perhaps we can extend that with a short not on aliases? Got any suggested
> wordings for that if so?

Hm, well, I went down a rabbit hole this afternoon -- OpenSSL has an
open feature request [1] that might eventually document this the right
way. In the meantime, maybe something like...

An incomplete list of available groups can be shown with the
command openssl ecparam -list_curves. Not all of them are usable with
TLS though, and many supported group names and aliases are omitted.

In PostgreSQL versions before 18.0 this setting was named
ssl_ecdh_curve. It only accepted a single value and did not recognize
group aliases at all.

Thanks,
--Jacob

[1] https://github.com/openssl/openssl/issues/17953

On 26.09.24 11:01, Daniel Gustafsson wrote:
> Attached is a v7 which address a test failure in the CI. It turns out that the
> test_misc module gather GUC names using the :alpha: character class which only
> allows alphabetic whereas GUC names can have digits in them. The 0001 patch
> fixes this by instead using the :alnum: character class which allows all
> alphanumeric characters. This is not directly related to this patch, it just
> happened to be exposed by it.

If we are raising the minimum version to OpenSSL 1.1.1, couldn't we then
remove the version check introduced by commit c3333dbc0c0 ("Only perform
pg_strong_random init when required")?

FWIW, these patches generally look okay to me. I haven't done much
in-depth checking, but overall everything looks sensible. I think Jacob
already provided more in-depth reviews, but let me know if you need
anything else on this.

> On 3 Oct 2024, at 01:20, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> On Wed, Oct 2, 2024 at 11:33 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>> If I migrate a server to a different machine that doesn't support my
>>> groups, I don't know that this would give me enough information to fix
>>> the configuration.
>>
>> Fair point, how about something along the lines of:
>>
>> + errmsg("ECDH: failed to set curve names specified in ssl_groups: %s",
>> + SSLerrmessageExt(ERR_get_error(),
>> + _("No valid groups found"))),
>
> Yeah, I think that's enough of a pointer. And then maybe "Failed to
> set group names specified in ssl_groups: %s" to get rid of the
> lingering ECC references?
>
>>> One nice side effect of the new ssl_groups implementation is that we
>>> now support common group aliases. For example, "P-256", "prime256v1",
>>> and "secp256r1" can all be specified now, whereas before ony
>>> "prime256v1" worked because of how we looked up curves. Is that worth
>>> a note in the docs?
>>
>> Maybe. We have this currently in the manual:
>>
>> "The full list of available curves can be shown with the command
>> <command>openssl ecparam -list_curves</command>. Not all of them are
>> usable with <acronym>TLS</acronym> though."
>>
>> Perhaps we can extend that with a short not on aliases? Got any suggested
>> wordings for that if so?
>
> Hm, well, I went down a rabbit hole this afternoon -- OpenSSL has an
> open feature request [1] that might eventually document this the right
> way. In the meantime, maybe something like...
>
> An incomplete list of available groups can be shown with the
> command openssl ecparam -list_curves. Not all of them are usable with
> TLS though, and many supported group names and aliases are omitted.
>
> In PostgreSQL versions before 18.0 this setting was named
> ssl_ecdh_curve. It only accepted a single value and did not recognize
> group aliases at all.

Attached is a v8 which address the above two raised points, as well as adds a
small note about LibreSSL in the docs as discussed in the retire-1.1.0-thread.

--
Daniel Gustafsson

> On 14 Oct 2024, at 15:08, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 26.09.24 11:01, Daniel Gustafsson wrote:
>> Attached is a v7 which address a test failure in the CI. It turns out that the
>> test_misc module gather GUC names using the :alpha: character class which only
>> allows alphabetic whereas GUC names can have digits in them. The 0001 patch
>> fixes this by instead using the :alnum: character class which allows all
>> alphanumeric characters. This is not directly related to this patch, it just
>> happened to be exposed by it.
>
> If we are raising the minimum version to OpenSSL 1.1.1, couldn't we then remove the version check introduced by commit c3333dbc0c0 ("Only perform pg_strong_random init when required")?

That's a very good point, I've done this in the v8 attached just upthread.

> FWIW, these patches generally look okay to me. I haven't done much in-depth checking, but overall everything looks sensible. I think Jacob already provided more in-depth reviews, but let me know if you need anything else on this.

Thanks! I think the v8 posted todays is about ready to go in and unless there
are objections I'll go ahead with it shortly.

--
Daniel Gustafsson

On Tue, Oct 15, 2024 at 3:42 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> Thanks! I think the v8 posted todays is about ready to go in and unless there
> are objections I'll go ahead with it shortly.

This new paragraph is missing a close-paren:

> + <para>
> + Additionally, <productname>LibreSSL</productname> is supported using the
> + <productname>OpenSSL</productname> compatibility layer. The minimum
> + required version is 3.4 (from <systemitem class="osname">OpenBSD</systemitem>
> + version 7.0.
> </para>

Other than that, LGTM!

Thanks,
--Jacob

> On 16 Oct 2024, at 17:30, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:

> Other than that, LGTM!

Thanks for all the review work, I went ahead and pushed this patchseries today
after a little bit more polishing of comments and docs. So far plover has
failed which was expected due to the raised OpenSSL/LibreSSL requirement, I
will reach out to BF animal owners for upgrades.

--
Daniel Gustafsson

First of all, thank you all for working on this feature.

On Wed, Sep 25, 2024 at 10:51:05AM +0200, Peter Eisentraut wrote:
> On 18.09.24 22:48, Jacob Champion wrote:
>> > +#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 think just
>
> ssl_ciphers =
> ssl_ciphers_tlsv13 =
>
> would be clear enough. Just using "ciphers" vs. "cipher suites" would not
> be.

Sorry for chiming in so late here, but I was a little surprised to see the
TLS version in the GUC name. ISTM this would require us to create a new
GUC for every new TLS version, or explain that ssl_tls13_ciphers isn't just
for 1.3. Perhaps neither of those things are too terrible, but I felt it
was worth bringing up.

--
nathan

On Wed, Dec 11, 2024 at 9:11 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> Sorry for chiming in so late here, but I was a little surprised to see the
> TLS version in the GUC name. ISTM this would require us to create a new
> GUC for every new TLS version, or explain that ssl_tls13_ciphers isn't just
> for 1.3.

I agree it's not ideal. But part of the problem IMO is that we might
actually _have_ to introduce a new GUC for a future TLS 1.4, because
we have no idea if the ciphersuites will change incompatibly again. (I
hope not, but they did it once and they could do it again.)

If 1.4, or 2.0, or... 4? [1] comes out later, and it turns out to be
compatible, we could probably add a more appropriate alias then. (For
now, just as some additional data points, both Apache and Curl use
"1.3" or "13" in the configuration as a differentiator.) Do you have a
different naming scheme in mind?

--Jacob

[1] https://mailarchive.ietf.org/arch/msg/tls/KmLJ2pk0c-s3MN7ojCrXy31SjmI/

Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> writes:
> On Wed, Dec 11, 2024 at 9:11 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>> Sorry for chiming in so late here, but I was a little surprised to see the
>> TLS version in the GUC name. ISTM this would require us to create a new
>> GUC for every new TLS version, or explain that ssl_tls13_ciphers isn't just
>> for 1.3.

> I agree it's not ideal. But part of the problem IMO is that we might
> actually _have_ to introduce a new GUC for a future TLS 1.4, because
> we have no idea if the ciphersuites will change incompatibly again. (I
> hope not, but they did it once and they could do it again.)
> If 1.4, or 2.0, or... 4? [1] comes out later, and it turns out to be
> compatible, we could probably add a more appropriate alias then. (For
> now, just as some additional data points, both Apache and Curl use
> "1.3" or "13" in the configuration as a differentiator.) Do you have a
> different naming scheme in mind?

Oh yay, another naming problem :-(. I think that neither "ciphers"
vs. "cipher suites" nor "ssl_ciphers" vs. "ssl_ciphers_tlsv13" is
going to convey a lot to the average person who's not steeped in
TLS minutiae. However, following the precedent of Apache and Curl
seems like a good answer --- that will ensure that at least some
part of the internet-using world has seen this before. So I guess
I'm +0.5 for the ssl_ciphers_tlsv13 answer, at least out of the
choices suggested so far.

regards, tom lane

> On 11 Dec 2024, at 18:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Oh yay, another naming problem :-(. I think that neither "ciphers"
> vs. "cipher suites" nor "ssl_ciphers" vs. "ssl_ciphers_tlsv13" is
> going to convey a lot to the average person who's not steeped in
> TLS minutiae. However, following the precedent of Apache and Curl
> seems like a good answer --- that will ensure that at least some
> part of the internet-using world has seen this before. So I guess
> I'm +0.5 for the ssl_ciphers_tlsv13 answer, at least out of the
> choices suggested so far.

The subset of users who are likely to be interested in this setting would
probably be more confused if we didn't follow the precedent from other
well-known projects.

--
Daniel Gustafsson

On Wed, Dec 11, 2024 at 12:47:01PM -0500, Tom Lane wrote:
> Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> writes:
>> On Wed, Dec 11, 2024 at 9:11 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>>> Sorry for chiming in so late here, but I was a little surprised to see the
>>> TLS version in the GUC name. ISTM this would require us to create a new
>>> GUC for every new TLS version, or explain that ssl_tls13_ciphers isn't just
>>> for 1.3.
>
>> I agree it's not ideal. But part of the problem IMO is that we might
>> actually _have_ to introduce a new GUC for a future TLS 1.4, because
>> we have no idea if the ciphersuites will change incompatibly again. (I
>> hope not, but they did it once and they could do it again.)
>> If 1.4, or 2.0, or... 4? [1] comes out later, and it turns out to be
>> compatible, we could probably add a more appropriate alias then. (For
>> now, just as some additional data points, both Apache and Curl use
>> "1.3" or "13" in the configuration as a differentiator.) Do you have a
>> different naming scheme in mind?

In a vacuum, I would've probably voted for ssl_cipher_suites since it is
reasonably descriptive and version-independent. It's true that we'd need
lots of documentation to explain which parameter is used for which TLS
version, but I think we need that regardless of the parameter name.

> Oh yay, another naming problem :-(. I think that neither "ciphers"
> vs. "cipher suites" nor "ssl_ciphers" vs. "ssl_ciphers_tlsv13" is
> going to convey a lot to the average person who's not steeped in
> TLS minutiae. However, following the precedent of Apache and Curl
> seems like a good answer --- that will ensure that at least some
> part of the internet-using world has seen this before. So I guess
> I'm +0.5 for the ssl_ciphers_tlsv13 answer, at least out of the
> choices suggested so far.

I wasn't aware that other projects were including the version, too. IMHO
that's a fair argument for the current name, if for no other reason than
we'll be in good company if/when things change. All things considered, I'd
probably still vote for something like ssl_cipher_suites, but I'm happy to
consider the matter resolved if we've given it some thought and decided to
stick with ssl_tls13_ciphers.

--
nathan

On 12/11/24 10:14 AM, Daniel Gustafsson wrote:
>> On 11 Dec 2024, at 18:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> Oh yay, another naming problem :-(. I think that neither "ciphers"
>> vs. "cipher suites" nor "ssl_ciphers" vs. "ssl_ciphers_tlsv13" is
>> going to convey a lot to the average person who's not steeped in
>> TLS minutiae. However, following the precedent of Apache and Curl
>> seems like a good answer --- that will ensure that at least some
>> part of the internet-using world has seen this before. So I guess
>> I'm +0.5 for the ssl_ciphers_tlsv13 answer, at least out of the
>> choices suggested so far.
>
> The subset of users who are likely to be interested in this setting would
> probably be more confused if we didn't follow the precedent from other
> well-known projects.

+1 to this point. The people I talk to who are interested in the
`cipher_suites` setting, are also the folks who are actually paying
attention to when and how ciphers/ciphersuites are used, and have strong
opinions on such. It also seems that OpenSSL is pushing in the direction
of making everything a "ciphersuite", albeit the -ciphersuites flag is
just for TLS v1.3+[1].

I think the `ssl_cipher_suites` proposal is fine; OK with bikeshedding
to `ssl_ciphersuites`.

Thanks,

Jonathan

[1] https://docs.openssl.org/3.3/man1/openssl-ciphers/#options