Re: [PATCH v2] Add ssl_alt_cert_file/ssl_alt_key_file for dual RSA+ECDSA certificate support - Mailing list pgsql-hackers

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