> On 7 Jun 2024, at 19:14, Jacob Champion <jacob.champ...@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
