Обсуждение: 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