> 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
