| From: | Florin Irion <irionr(at)gmail(dot)com> |
|---|---|
| To: | Agustín Martínez Fayó <amartinezfayo(at)gmail(dot)com> |
| Cc: | Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>, olivier cano <kindermoumoute(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, david(at)pgbackrest(dot)org, gabriele(dot)bartolini(at)enterprisedb(dot)com |
| Subject: | Re: Proposal: Supporting URI SAN in Certificate Authentication |
| Date: | 2026-10-06 06:12:11 |
| Message-ID: | 62cd727a-1898-4c61-a2aa-9a82e8ea3561@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Thanks for the detailed review and the SPIFFE perspective. v3 addresses all of it
On 26/09/2026 01:19, Agustín Martínez Fayó wrote:
>> +$node->connect_fails(
>> + "$uri_connstr user=ssltestuser sslcert=ssl/client-uri-multi.crt "
>> + . sslkey('client-uri-multi.key'),
>> + "certificate authorization fails when client certificate has multiple URI SANs",
>> + expected_stderr =>
>> + qr/certificate authentication failed for user "ssltestuser"/);
> I think this does not exercise the count check. client-uri-multi.config
> lists the non-matching URI first and the code keeps the first URI, so
> without the check the connection is authenticated as
> spiffe://.../not-the-user and then fails on the usermap, which produces
> the same client error. I deleted the check and the suite still passes
> 286/286. I wonder if putting the matching URI first in the fixture, or
> asserting the server log reason with log_like, would make the test fail
> for the right cause.
Ok changed, now it lists the matching URI first and asserts the
server-side "exactly one URI SAN" message, so the test fails for the
right reason if the count check is removed.
>> + /* fail closed if the URI could not be converted */
>> + if (port->peer_uri_count > 0 && port->peer_uri == NULL)
>> + port->peer_uri_count = 0;
> This seems to log a COMMERROR while the handshake still completes. I
> signed a certificate with the suite's client CA whose single URI SAN
> contains a null byte. On a clientname=CN line the log shows "URI subject
> alternative name contains embedded null" and the connection is then
> authenticated, with SELECT system_user returning cert:CN=ssltestuser. On
> a clientname=URI line the same log line is followed by the single-URI
> rejection message, which points at the wrong cause. Returning false
> here, as the CN path does for an embedded null, would also reject such
> certificates on CN and DN lines, which never looked at URI SANs before
> this patch. I think that is a reasonable price for consistency, but the
> alternative of failing only on URI lines with an accurate error message
> would work too.
The extraction now happens in CheckCertAuth(), only for
clientname=URI lines, so CN/DN lines never look at URI SANs. A URI
that cannot be represented as a string logs the precise cause and
fails with its own error message, instead of the misleading
"exactly one URI SAN" message.
>> + if (X509_NAME_print_ex(bio, x509name, 0, XN_FLAG_RFC2253) == -1 ||
>> + BIO_get_mem_ptr(bio, &bio_buf) < 0)
> I wonder whether relaxing this from <= 0 was needed. With OpenSSL 3.x
> and 1.1.1, X509_NAME_print_ex returns 0 for an empty name and
> BIO_get_mem_ptr still returns 1. If BIO_get_mem_ptr ever returned 0,
> bio_buf would stay NULL for the dereference below, so the original check
> seems safer to me.
Done, BIO_get_mem_ptr() is back to <= 0.
>> + contain exactly one URI subject alternative name. The comparison is
>> + case-sensitive and uses the exact URI as it appears in the
>> + certificate.
> Minor. Since the URI is matched as an opaque string, I wonder if a
> sentence advising that regular-expression maps anchor the full scheme
> and authority would help avoid patterns that match a URI from an
> unexpected authority.
>> + Note that PostgreSQL does not verify any relationship between a
>> + certificate's URI and the certificate authority that signed the
>> + certificate. If <literal>ssl_ca_file</literal> contains certificates
>> + for more than one trust domain, any of them can issue a certificate
>> + for a URI in another trust domain. For SPIFFE deployments, configure
> Minor. This uses "trust domain" before SPIFFE is introduced. Maybe it
> could say that any CA in the file can issue a certificate carrying any
> URI, and keep the trust domain wording for the SPIFFE sentence that
> follows.
Changed to advise anchoring regex maps to the scheme and authority,
introduce "trust domain" only in the SPIFFE sentence, reference
clientname from the CN-only descriptions, and include a worked
pg_hba.conf/pg_ident.conf example.
Cheers,
Florin
--
Florin Irion
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-libpq-Support-URI-SANs-in-certificate-authenticat.patch | text/plain | 42.4 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Langote | 2026-10-06 06:13:50 | Two more RI fast-path issues |
| Previous Message | Shubhra Jain | 2026-10-06 06:07:41 | Re: Warn when creating or enabling a subscription with max_logical_replication_workers = 0 |