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

> On 22 Jul 2024, at 19:14, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> On Fri, Jul 12, 2024 at 1:03 PM Daniel Gustafsson <daniel(at)yesql(dot)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.

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. Pushing an error on
the queue would've been nice but we can't replicate the OpenSSL level of detail
in the error until we require OpenSSL 3.0 as the base since that's when _data
error reporting was added.

>> 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".

Fair point, I've renamed to ssl_groups and added a mapping from the old name as
well as a note in the docs that the parameter has changed name (and ability to
handle more than one).

> 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.

Not really, I have changed such that a blank GUC does *no* OpenSSL call at all
which will retain the default from the local OpenSSL installation.

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.

--
Daniel Gustafsson