Perform more validation in extract_trust_anchor()

[cpu]

The TrustAnchor::try_from_cert_der fn does not perform any validation of the provided certificate - it simply extracts the minimum necessary metadata (subject, subject public key information, and optionally, name constraints) to represent a trust anchor internally in webpki. The associated rustdoc comment explains:

In particular, there is no check that the certificate is self-signed or even that the certificate has the cA basic constraint.

The original commit that introduced the helper in 2015 did not offer any explicit rationale for why this validation isn't performed.

In particular, not enforcing the presence of a true cA basic constraint when building a TrustAnchor this way means that while we advertise an intentional limitation for webpki to not support self-signed certificates this isn't strictly true.

If a self-signed end-entity certificate includes a basic constraints extension that asserts a true cA bool then we will reject the certificate in check_issuer_independent_properties when path building, returning Error::CaUsedAsEndEntity. This happens even if the same self-signed certificate is configured as a trust anchor. The advertised limitation holds.

However, if a self-signed end-entity certificate omits the basic constraints extension (or includes one w/ a false cA bool) then we will not reject it as a CA used as an end-entity in check_issuer_independent_properties. Since TrustAnchor::try_from_cert_der doesn't enforce the presence of a true cA bool in its input it will also not error if given the self-signed certificate DER. The end result is that it is possible for path building to succeed if such a self-signed end entity certificate is also configured as a TrustAnchor created with TrustAnchor::try_from_cert_der. This is in contrast to the advertised limitation of not supporting self-signed certificates.

On the topic of basic constraints extensions RFC 5280 4.2.1.9 says:

The cA boolean indicates whether the certified public key may be used to verify certificate signatures. If the cA boolean is not asserted, then the keyCertSign bit in the key usage extension MUST NOT be asserted. If the basic constraints extension is not present in a version 3 certificate, or the extension is present but the cA boolean is not asserted, then the certified public key MUST NOT be used to verify certificate signatures.

My reading here is that we're acting in contradiction to a normative MUST NOT in 5280: We're using a trust anchor certificate without an asserted cA boolean's public key to verify the end-entity certificate signature.

If folks agree with my interpretation, I think we should update TrustAnchor::try_from_cert_der to perform more validation. Trust anchors participating in the web pki should all have the correct basic constraint extension present.

If folks disagree with my interpretation, I think we should update the limitation description to clarify this situation since there's more nuance than advertised in the README.

[djc]

(Changed the title to refer to the functionality as named on the main branch.)

I think there is a general tendency in the webpki code base to defer validation until the time it used -- see also the extensive use of untrusted::Input<'_> fields in Cert<'_> and other types. As such, the current behavior feels in line with this style. I feel like the deferred style somewhat unidiomatic, in that I think most (high-quality) Rust libraries strive for more of a "parse, don't validate" philosophy of making it impossible to represent invalid states.

Your proposal in this issue would move us closer to "parse, don't validate" style (which I generally like/prefer), but I do see some potential downsides:

• For one thing, I'm a little hesitant about deviating from the prevailing style even though I agree that the current behavior is surprising and suboptimal.
• For another, TrustAnchor today lives in the pki-types crate and it has public fields, so there isn't really a way to enforce validity over time. If we're going to check this at creation time we should probably change it so that this is always checked at initialization time and then preserved as an invariant (so for example TrustAnchor's fields should become private and exposed as getters).

@briansmith can you comment on why you choose the deferred validation style?

My guess would be that it's (a) allowing the current caller to validate the current state at the current point in time without relying on validation from an earlier time and (b) maybe legacy from when you implemented this in C++ and the culture of "parse don't validate" wasn't as obvious, or maybe hard to enforce. Personally I feel like if we are conscientious about making invalid states unrepresentable, "parse don't validate" style doesn't have many downsides (in that "earlier validation" would be ensured by the type system).

Should we worry about side channel attack issues from short-circuiting certificate validation? My guess is no, since certificates themselves are public anyway.

[cpu]

I think there is a general tendency in the webpki code base to defer validation until the time it used -- see also the extensive use of untrusted::Input<'> fields in Cert<'> and other types. As such, the current behavior feels in line with this style.

I don't know that's a fair representation of the current behaviour since it implies we do eventually validate the trust anchor has correct basic constraints, and that's not the case. Capturing more trust anchor state to defer validation would be possible, but also runs counter to webpki's trust anchor representation aiming to carry as little state as possible (e.g. vs keeping a full X.509 cert for each TA).

I feel like the deferred style somewhat unidiomatic, in that I think most (high-quality) Rust libraries strive for more of a "parse, don't validate" philosophy of making it impossible to represent invalid states.

Agreed 👍

For one thing, I'm a little hesitant about deviating from the prevailing style even though I agree that the current behavior is surprising and suboptimal.

Indeed, I'm also open to the possibility I'm overlooking strong reasons the current approach is used. If this issue only serves to uncover those reasons and document them clearly I still think it was a worthwhile endeavor :-)

For another, TrustAnchor today lives in the pki-types crate and it has public fields, so there isn't really a way to enforce validity over time. If we're going to check this at creation time we should probably change it so that this is always checked at initialization time and then preserved as an invariant (so for example TrustAnchor's fields should become private and exposed as getters).

Making the fields private seems reasonable to me fwiw.

In the case of the pki-types TrustAnchor type there isn't a helper for converting from an X.509 cert to the TrustAnchor, you have to construct one field-by-field. While maybe not optimal it does feel a bit clearer in this context that no additional metadata is being considered or captured. With the extract_trust_anchor approach we have the relevant metadata in hand and it's only the rustdoc comment that informs you that parts that might seem relevant per-spec are not used.

Let's let this sit a bit and see if we get any more input?

[briansmith]

Since TrustAnchor::try_from_cert_der doesn't enforce the presence of a true cA bool in its input it will also not error if given the self-signed certificate DER.

Many CA root certificates are v1 (no extensions), and all of them are self-signed.

[cpu]

Many CA root certificates are v1 (no extensions)

Is there a reason to avoid enforcing the presence of the expected basic constraints extension for trust anchors that are v3?

[briansmith]

Is there a reason to avoid enforcing the presence of the expected basic constraints extension for trust anchors that are v3?

Any usage of this function where that check would matter sounds completely wrong. You've already manually verified that the (subject name, public key, name constraints) are trustworthy out-of-band. If you haven't, you're in big trouble. Though I am open to be proven wrong.

[cpu]

Clarifying the comment on extract_trust_anchor to explain why the validation isn't performed instead of just saying that it isn't, and updating the README's description of self-signed certificates not being supported seem like the right path in that case.

[est31]

You've already manually verified that the (subject name, public key, name constraints) are trustworthy out-of-band. If you haven't, you're in big trouble.

That's also my opinion on whether webpki should do such checks for end entity certificates in the list of root certs. You've already manually verified the trustworthiness of a certificate if you add it to the root store. If it's an end entity certificate, why should the library reject it. CAs only exist because it's not viable for the root store to contain all end entity certificates used in TLS, as a means to an end.

In general, I find people reject using X.509 for things due to its complexity, and the view that you have to have separate CA and end entity certificates doesn't help. It's usually taught that way when explaining TLS. But often it's a misconception because libraries/software accepts also self signed certs in many instances. Software authors should keep accepting self signed certs, at least that's my opinion.

[briansmith]

. If it's an end entity certificate, why should the library reject it. CAs only exist because it's not viable for the root store to contain all end entity certificates used in TLS, as a means to an end.

because libraries/software accepts also self signed certs in many instances. Software authors should keep accepting self signed certs, at least that's my opinion.

I find the discussion a bit alarming and I worry people are not understanding what it means to construct a TrustAnchor from a certificate at all, or even what a TrustAnchor really is.

Please read briansmith/webpki#294.

[cpu]

I worry people are not understanding what it means to construct a TrustAnchor from a certificate at all

Indeed, I think we need to do a better job of documenting the discussion that's bubbling up here and in briansmith/webpki#294. I've clearly been confused, and so have several other folks.

Here's an initial attempt to add some more guidance for downstream users: #201

Happy to iterate on that if I've still missed the mark.

[cpu]

Here's an initial attempt to add some more guidance for downstream users: #201

We landed that update, I don't think there are any other actions to take at this time.