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

On 07.06.24 08:10, Erica Zhang wrote:
> I’m a Postgres user and I’m looking into restricting the set of allowed 
> ciphers on Postgres and configure a concrete set of curves on our 
> postgres instances.

Out of curiosity, why is this needed in practice?

> Could you please help to review to see if you are interested in having 
> this change in upcoming Postgres major release(It's should be PG17)?

It would be targetting PG18 now.

On Fri, Jun 07, 2024 at 06:02:37PM +0800, Erica Zhang wrote:
> I see the https://commitfest.postgresql.org/48/ is still open, could
> it be possible to target for PG17? As I know PG17 is going to be
> release this year so that we can upgrade our instances to this new
> version accodingly.

Echoing with Peter, https://commitfest.postgresql.org/48/ is planned
to be the first commit fest of the development cycle for Postgres 18.
v17 is in feature freeze state and beta, where only bug fixes are
accepted, and not new features.
--
Michael

On Fri, Jun 7, 2024 at 3:02 AM Erica Zhang <ericazhangy2021@qq.com> wrote:
>
> For some security consideration, we prefer to use TLS1.3 cipher suites in our product with some customization values
insteadof default value "HIGH:MEDIUM:+3DES:!aNULL". Moreover we prefer to set a group of ecdh keys instead of a single
value.

+1 for the curve list feature, at least. No opinions on the 1.3
ciphersuites half, yet.

I've added this patch to my planned review for the v18 cycle. Some
initial notes:

- Could you separate the two features into two patches? That would
make it easier for reviewers. (They can still share the same thread
and CF entry.)
- The "curve" APIs have been renamed "group" in newer OpenSSLs for a
while now, and we should probably use those if possible.
- I think parsing apart the groups list to check NIDs manually could
lead to false negatives. From a docs skim, 3.0 allows providers to add
their own group names, and 3.3 now supports question marks in the
string to allow graceful fallbacks.
- I originally thought it'd be better to just stop calling
SSL_set_tmp_ecdh() entirely by default, so we could use OpenSSL's
builtin list of groups. But that may have denial-of-service concerns
[1]?
- We should maybe look into SSL_CTX_config(), if we haven't discussed
that already on the list, but that's probably a bigger tangent and
doesn't need to be part of this patch.

Thanks,
--Jacob

[1] https://www.openssl.org/blog/blog/2022/10/21/tls-groups-configuration/index.html

> On 7 Jun 2024, at 19:14, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> - Could you separate the two features into two patches? That would
> make it easier for reviewers. (They can still share the same thread
> and CF entry.)

+1, please do.

> - The "curve" APIs have been renamed "group" in newer OpenSSLs for a
> while now, and we should probably use those if possible.

Agreed.  While not deprecated per se the curve API is considered obsolete and
is just aliased to the group API (OpenSSL using both the term obsolete and
deprecated to mean the same thing but with very different mechanics is quite
confusing).

> - I think parsing apart the groups list to check NIDs manually could
> lead to false negatives. From a docs skim, 3.0 allows providers to add
> their own group names, and 3.3 now supports question marks in the
> string to allow graceful fallbacks.

Parsing the list will likely risk false negatives as you say, but from skimming
the code there doesn't seem to be a good errormessage from SSL_set1_groups_list
to indicate if listitems were invalid (unless all of them were).  Maybe calling
the associated _get function to check the number of set groups can be used to
verify what happenend?

Regarding the ciphersuites portion of the patch.  I'm not particularly thrilled
about having a GUC for TLSv1.2 ciphers and one for TLSv1.3 ciphersuites, users
not all that familiar with TLS will likely find it confusing to figure out what
to do.

In which way is this feature needed since this can be achieved with the config
directive "Ciphersuites" in openssl.conf IIUC?

If we add this I think we should keep it blank and if so skip setting it at all
falling back on OpenSSL defaults.  The below default for the GUC does not match
the OpenSSL default and I think we are better off trusting them on this.

+    "TLS_AES_256_GCM_SHA384:TLS_AES_128_GCM_SHA256",

--
Daniel Gustafsson

On Mon, 10 Jun 2024 at 12:31, Daniel Gustafsson <daniel@yesql.se> wrote:
> Regarding the ciphersuites portion of the patch.  I'm not particularly thrilled
> about having a GUC for TLSv1.2 ciphers and one for TLSv1.3 ciphersuites, users
> not all that familiar with TLS will likely find it confusing to figure out what
> to do.

I don't think it's easy to create a single GUC because OpenSSL has
different APIs for both. So we'd have to add some custom parsing for
the combined string, which is likely to cause some problems imho. I
think separating them is the best option from the options we have and
I don't think it matters much practice for users. Users not familiar
with TLS might indeed be confused, but those users shouldn't touch
these settings anyway, and just use the defaults. The users that care
about this probably already get two cipher strings from their
compliance teams, because many other applications also have two
separate options for specifying both.

On Wed, 12 Jun 2024 at 04:32, Erica Zhang <ericazhangy2021@qq.com> wrote:
> There are certain government, financial and other enterprise organizations that have very strict requirements about
theencrypted communication and more specifically about fine grained params like the TLS ciphers and curves that they
use.The default ones for those customers are not acceptable. Any products that integrate Postgres and requires
encryptedcommunication with the Postgres would have to fulfil those requirements. 

Yeah, I ran into such requirements before too. So I do think it makes
sense to have such a feature in Postgres.

> So if we can have this patch in the upcoming new major version, that means Postgres users who have similar
requirementscan upgrade to PG17. 

As Daniel mentioned you can already achieve the same using the
"Ciphersuites" directive in openssl.conf. Also you could of course
always disable TLSv1.3 support.

On 12.06.24 10:51, Jelte Fennema-Nio wrote:
> On Mon, 10 Jun 2024 at 12:31, Daniel Gustafsson <daniel@yesql.se> wrote:
>> Regarding the ciphersuites portion of the patch.  I'm not particularly thrilled
>> about having a GUC for TLSv1.2 ciphers and one for TLSv1.3 ciphersuites, users
>> not all that familiar with TLS will likely find it confusing to figure out what
>> to do.
> 
> I don't think it's easy to create a single GUC because OpenSSL has
> different APIs for both. So we'd have to add some custom parsing for
> the combined string, which is likely to cause some problems imho. I
> think separating them is the best option from the options we have and
> I don't think it matters much practice for users. Users not familiar
> with TLS might indeed be confused, but those users shouldn't touch
> these settings anyway, and just use the defaults. The users that care
> about this probably already get two cipher strings from their
> compliance teams, because many other applications also have two
> separate options for specifying both.

Maybe some comparisons with other SSL-enabled server products would be 
useful.

Here is the Apache httpd setting:

https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslciphersuite

They use a complex syntax to be able to set both via one setting.

Here is the nginx setting:

https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_ciphers

This doesn't appear to support TLS 1.3?

> On 13 Jun 2024, at 09:07, Erica Zhang <ericazhangy2021@qq.com> wrote:

> How can I achieve the value for TLS1.3? Do you mean I can set the Ciphersuites in openssl.conf, then Postgres will
pickup and use this value accordingly? 

Yes, you should be able to restrict the ciphersuites for TLSv1.3 with
openssl.conf on your system.

--
Daniel Gustafsson

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

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@eisentraut.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@eisentraut.org> wrote:

> It would be worth checking the discussion at
<https://www.postgresql.org/message-id/flat/79692bf9-17d3-41e6-b9c9-fc8c3944222a@eisentraut.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
availableon 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
avery 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@yesql.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@yesql.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 Mon, Sep 9, 2024 at 5:00 AM Daniel Gustafsson <daniel@yesql.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 Wed, Sep 25, 2024 at 6:39 AM Daniel Gustafsson <daniel@yesql.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.champion@enterprisedb.com> wrote:
>
> On Wed, Sep 25, 2024 at 6:39 AM Daniel Gustafsson <daniel@yesql.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@yesql.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 14 Oct 2024, at 15:08, Peter Eisentraut <peter@eisentraut.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
commitc3333dbc0c0 ("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@yesql.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.champion@enterprisedb.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@gmail.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.champion@enterprisedb.com> writes:
> On Wed, Dec 11, 2024 at 9:11 AM Nathan Bossart <nathandbossart@gmail.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@sss.pgh.pa.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.champion@enterprisedb.com> writes:
>> On Wed, Dec 11, 2024 at 9:11 AM Nathan Bossart <nathandbossart@gmail.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@sss.pgh.pa.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