Re: [PATCH v2] Add ssl_alt_cert_file/ssl_alt_key_file for dual RSA+ECDSA certificate support

Hi Zsolt,

Thanks for the thorough review, I will take a look at all this.

Best regards,

Renaud Métrich
Red Hat

Le 15/06/2026 à 11:13 PM, Zsolt Parragi a écrit :
> + primary_type = EVP_PKEY_get_base_id(X509_get0_pubkey(primary_cert));
> + alt_type = EVP_PKEY_get_base_id(X509_get0_pubkey(alt_cert));
> +
>
> Isn't EVP_PKEY_get_base_id also a function introduced by OpenSSL 3.0?
>
> -# Test 5: Verify ssl_server_cert_type() returns correct type per connection
> +# Test 5: Verify TLS 1.3 connection works (exercises HelloRetryRequest path)
> +note "testing TLS 1.3 connection with dual certs";
> +$node->connect_ok(
> + "$common_connstr sslcert=invalid",
> + "connect via TLS 1.3 with dual certs (default negotiation)",
> + sql => "SELECT 1");
> +
> +# Test 6: Verify ssl_server_cert_type() returns correct type per connection
>
> I don't think this properly tests HelloRetryRequest, as there's no
> key-share group mismatch in the testcase.
>
> Also, the testcase file should be included in
> src/test/ssl/meson.build, currently it is only executed by make.
>
> +
> + /*
> + * Verify that the alternate certificate uses a different key type
> + * than the primary. If both are the same type (e.g. both RSA),
> + * the alternate silently replaces the primary, which is not useful.
> + */
> + {
> + X509 *primary_cert;
> + X509 *alt_cert;
> + int primary_type;
> + int alt_type;
> +
> + SSL_CTX_set_current_cert(ctx, SSL_CERT_SET_FIRST);
> + primary_cert = SSL_CTX_get0_certificate(ctx);
> +
> + SSL_CTX_set_current_cert(ctx, SSL_CERT_SET_NEXT);
> + alt_cert = SSL_CTX_get0_certificate(ctx);
> +
> + if (primary_cert && alt_cert)
> + {
> + primary_type = EVP_PKEY_get_base_id(X509_get0_pubkey(primary_cert));
> + alt_type = EVP_PKEY_get_base_id(X509_get0_pubkey(alt_cert));
> +
> + if (primary_type == alt_type)
> + {
> + ereport(isServerStart ? FATAL : LOG,
> + (errcode(ERRCODE_CONFIG_FILE_ERROR),
> + errmsg("alternate certificate has the same key type (%s) as
> the primary certificate",
> + evp_pkey_type_name(alt_type))));
> + goto error;
> + }
> + }
> + }
>
> Are you sure about this code? The comment says "the alternate silently
> replaces the primary, which is not useful." - which was also my
> observation, but replacing means that there's no alt certificate
> really. I think this check works by accident because if there's no
> NEXT certificate, OpenSSL returns the first certificate again, but
> this behavior is undocumented.
>
>> 3) Per-host SNI support: documented that ssl_alt_cert_file applies only
>> to the default SSL configuration from postgresql.conf.
> I don't think documenting the limitation is a good approach for this
> feature, it should be supported uniformly everywhere. The question of
> how it could fit into pg_hosts is a different question, so I am not
> suggesting that you should simply add a few more columns there, but I
> would at least keep it as a TODO item. The format/extensibility of
> hosts and other configuration files is a bigger question I want to
> start a discussion about soon, and this could fit there.
>
>> For PostgreSQL, since GUCs are strings, this could take the form of
>> comma-separated paths:
>>
>> ssl_cert_file = 'server-rsa.crt, server-ecdsa.crt'
>> ssl_key_file = 'server-rsa.key, server-ecdsa.key'
> List style GUCs already exist (for example unix_socket_directories),
> so there's a good precedent for this. This could also fit into
> pg_hosts, where the host part already accepts a list of hosts.
>
> Also I'm not sure if changing the single string GUC to a list could
> cause any issues with dump/restore. I didn't check this in detail, but
> there's a comment about this in
> src/backend/utils/misc/guc_parameters.dat and dumputils.c.
>
> Another option would be to add new GUCs ending with _files, and make
> them mutually exclusive?
>
>> and let OpenSSL sort out the type matching — no same-type detection
>> needed, no "alt" naming, and natural support for all three key types.
> I'm quite sure we would still have to do some validation, but that's a
> minor detail.
>
>