On Mon, Sep 9, 2024 at 5:00 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> 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.
Thanks!
> 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.
As implemented, my build matrix is no longer able to compile against
LibreSSL 3.3 and below (OpenBSD 6.x). Has the lower bound on LibreSSL
for PG18 been discussed yet?
> +#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed TLSv1.2 ciphers
> +#ssl_cipher_suites = '' # allowed TLSv1.3 cipher suites, blank for default
After marinating on this a bit... I think the naming may result in
some "who's on first" miscommunications in forums and on the list. "I
set the SSL ciphers to <whatever>, but it says there are no valid
ciphers available!" Should we put TLS 1.3 into the new GUC name
somehow?
> + {"ssl_groups", PGC_SIGHUP, CONN_AUTH_SSL,
> + gettext_noop("Sets the curve(s) to use for ECDH."),
The ECDH reference should probably be updated/removed. Maybe something
like "Sets the group(s) to use for Diffie-Hellman key exchange." ?
> +#if (OPENSSL_VERSION_NUMBER <= 0x30200000L)
> + /*
> + * OpenSSL 3.3.0 introduced proper error messages for group
> + * parsing errors, earlier versions returns "no SSL error
> + * reported" which is far from helpful. For older versions, we
> + * manually set a better error message. Injecting the error
> + * into the OpenSSL error queue need APIs from OpenSSL 3.0.
> + */
> + errmsg("ECDH: failed to set curve names: No valid groups in '%s'",
> + SSLECDHCurve),
nit: can we do this only when ERR_get_error() returns zero? It looks
like LibreSSL is stuck at OPENSSL_VERSION_NUMBER == 0x20000000, so if
they introduce a nice error message at some point it'll still get
ignored.
> + &SSLCipherLists,
nit: a singular "SSLCipherList" would be clearer, IMO.
--
Looking at the commit messages:
> Support configuring multiple ECDH curves
>
> The ssl_ecdh_curve only GUC accepts a single value, but the TLS
"GUC" and "only" are transposed here.
> Support configuring TLSv1.3 cipher suites
>
> The ssl_ciphers GUC can only set cipher suites for TLSv1.2, and lower,
> connections. For TLSv1.3 connections a different OpenSSL must be used.
"a different OpenSSL API", maybe?
Thanks,
--Jacob
