On Thu, Sep 17, 2026 at 3:10 AM Florin Irion <[email protected]> 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
