Re: Proposal: Supporting URI SAN in Certificate Authentication

From: Florin Irion <irionr(at)gmail(dot)com>
To: Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>
Cc: 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-09-22 09:46:10
Message-ID: d8a3e053-0e19-42cc-b525-2ce115d9e4fe@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 18/09/2026 17:58, Jacob Champion wrote:
> On Thu, Sep 17, 2026 at 3:10 AM Florin Irion <irionr(at)gmail(dot)com> wrote:
>> We decided to
>> strictly allow only a single URI SAN per certificate. This follows the
>> X509-SVID spec, avoiding weird edge cases with user mapping, and making
>> connection logging very clear.
> I'd like opinions from the crowd on the use of X509-SVID as a
> governing spec here. I don't mind the 1:1 requirement for now; it
> simplifies things. But if someone comes forward proposing multiple-URI
> support, are we going to reject it since the original use case was
> SPIFFE? Also, this patch doesn't seem to perform the other required
> checks for X509-SVID validation [1], so it seems a little weird to
> cite SPIFFE in the comments and tests and documentation.

The single-URI requirement is a design simplification, not a spec
requirement.
SPIFFE motivated the feature but doesn't necessarily govern it. If someone
later proposes matching against multiple URI SANs, it can be evaluated
on its
own merits.
v2 drops all X509-SVID citations from comments, tests, and docs, since
we don't
implement the other X509-SVID validation checks.

> Partial review follows (I have not read in detail):
>
>> + * An empty subject means there is no DN to record, so leave
>> + * peer_dn NULL. The X.509-SVID specification allows this when a
>> + * (critical) URI subjectAltName is present.
> From a skim, it looks like other parts of the code assume either
> peer_dn or peer_cn is set; they'll need to be changed too.

With V2 I don't see any other places where we assume peer_dn or peer_cn
is set.

> The growing indentation in be_tls_open_server is, I think, a smell.
> We've reached rule of three (or at least 2.5 depending on your view of
> peer_dn and peer_cn) and I think this code is going to need to be
> restructured a bit in order to absorb the new use case. I don't have
> anything in particular in mind yet.

I restructured the code adding be_tls_extract_peer_cert_names()

>> The X.509-SVID specification
>> +# requires the URI SAN to be critical when the subject is empty.
> We can cite RFC 5280 for the criticality requirement (X509-SVID defers
> to it anyway).

done.

>> +# Client certificate with an empty subject and a single, critical URI
>> +# subjectAltName. Unlike the standard client certificates, this needs
>> +# an explicit "-subj /" when generating the CSR, since the config file
>> +# alone cannot express an empty subject.
> That's fun. Let's just pull the `-subj /` into a default-empty
> variable that the new CSR can tweak. That should get rid of most of
> the new copy-paste boilerplate, I think.

done.

>> For SPIFFE deployments, configure
>> + <literal>ssl_ca_file</literal> with a single trust domain's bundle;
>> + </para>
> Looks like this was cut off?

Yup, fixed.

>> + /*
>> + * The SPIFFE X.509-SVID specification requires an SVID to carry
>> + * exactly one URI SAN, so refuse both none and more than one: the
>> + * authenticated identity must be unambiguous.
>> + */
>> + if (port->peer_uri_count != 1)
> I think it's really important that we keep unedited LLMisms out of the
> security code. (And elsewhere too.)
>
> Like, definitely push back if I've misattributed this, but I imagine
> that you wouldn't have willingly typed three lines of comments in
> order to say "we require exactly one URI", and I don't really want to
> read three lines of comments that say that. We need the
> signal-to-noise ratio to be very high here.

True.
I tend to be very succinct and sometimes I fear I don't give enough context,
but I agree here the LLM added way too much.
Fixed.

>> - if (peer_username == NULL ||
>> - strlen(peer_username) <= 0)
>> + if (peer_username == NULL || strlen(peer_username) <= 0)
> Drive-by cleanup is usually very welcome (thank you), but for security
> patches I think it's good to keep unrelated code motion separate from
> new implementation.

Makes sense. Removed.

Thank you for checking this!
V2 attached.

Florin

--
_/**/__/**/__/**/__/**/__/* Florin Irion */__/**/__/**/__/**/__/**/__/**/_
/https://www.enterprisedb.com/

Attachment Content-Type Size
v2-0001-libpq-Support-URI-SANs-in-certificate-authenticat.patch text/plain 36.9 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Matthias van de Meent 2026-09-22 09:55:14 Re: [PATCH] set_byte() with a count argument
Previous Message vignesh C 2026-09-22 09:33:39 Re: sequencesync worker race with REFRESH SEQUENCES