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


Reply via email to