Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\))
Subject: Re: Add support to TLS 1.3 cipher suites and curves lists
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <CAOYmi+k2qaJio+JkfKC2J+Shx8Fym-ifERhFLCR6mz7UXKxLVw@mail.gmail.com>
Date: Mon, 10 Jun 2024 12:30:36 +0200
Cc: Jacob Champion <jacob.champion@enterprisedb.com>,
 Peter Eisentraut <peter@eisentraut.org>,
 pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8AD6972C-7362-4A8D-96AA-6FAC44CE154B@yesql.se>
References: <tencent_F551A28C9CFA3CE8E278FD23E22F261EE607@qq.com>
 <CAOYmi+k2qaJio+JkfKC2J+Shx8Fym-ifERhFLCR6mz7UXKxLVw@mail.gmail.com>
To: Erica Zhang <ericazhangy2021@qq.com>
Archived-At: <https://www.postgresql.org/message-id/8AD6972C-7362-4A8D-96AA-6FAC44CE154B%40yesql.se>
Precedence: bulk

> 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. =46rom 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