Re: Proposal: Supporting URI SAN in Certificate Authentication

From: Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>
To: Florin Irion <irionr(at)gmail(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-18 15:58:39
Message-ID: CAOYmi+=FTZS-USmzkyYJ5TwonhCCooHw7vhoPokzq5rmtCQrPA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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.

X.509-SVID isn't relevant in this particular part of the code, IMO.
Empty Subjects have been permissible in general for a very long time,
and criticality should generally be handled by the OpenSSL layer
rather than us.

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.

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

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

> For SPIFFE deployments, configure
> + <literal>ssl_ca_file</literal> with a single trust domain's bundle;
> + </para>

Looks like this was cut off?

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

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

Thanks,
--Jacob

[1] https://spiffe.io/docs/latest/spiffe-specs/x509-svid/#5-validation

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexandre Felipe 2026-09-18 16:01:37 [patch] Cache invalidation for I/O Workers
Previous Message Nathan Bossart 2026-09-18 15:52:12 Re: add list of major features to the v19 release notes