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