trust_anchor: clarify and rename extract_trust_anchor

[cpu]

This function has caused some confusion w.r.t self-signed end-entity certificates and their support within rustls/webpki (see #200, briansmith/webpki#294). This branch takes two actions to try and help this situation:

1. It renames extract_trust_anchor to anchor_from_trusted_cert to emphasize that the input must be prevalidated out-of-band as suitable to use as a trust anchor.
2. It adds more detail to the rustdoc comment for the function, in particular emphasizing why no additional validations are performed. We also emphasize that this function is intended as a helper for users constructing trust anchor stores out of existing trust anchor stores that use an X.509 certificate representation, and that it is not appropriate to use with an end-entity certificate in an attempt to support self-signed certificate use-cases.

[codecov]

Codecov Report

Merging #201 (8692c5f) into main (68e8d8e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  Coverage   96.77%   96.77%           
=======================================
  Files          20       20           
  Lines        4772     4774    +2     
=======================================
+ Hits         4618     4620    +2     
  Misses        154      154           

Files	Coverage Δ
src/lib.rs	100.00% <ø> (ø)
src/trust_anchor.rs	98.43% <100.00%> (+0.05%)	⬆️
src/verify_cert.rs	98.07% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[djc]

I agree with the goals here, but not sure about this particular implementation.

On the naming side, "prevalidated" feels a bit wordy, maybe just used "valid" instead?

On the documentation side, I think you'll find that none of the callers within other rustls packages do much that could be construed as "verification" of subject name, public key and name constraints, so I feel this documentation leans too hard on that idea. In my understanding the way this is generally is used is that the data source is trusted (for example, for webpki-roots the source is the CCADB and for rustls-native-certs is the OS trust store) -- but I don't feel documenting that the caller should verify all the things does a good job of conveying that idea?

[cpu]

I'm happy to iterate further on this 👍

On the naming side, "prevalidated" feels a bit wordy, maybe just used "valid" instead?

As in "extract_prevalid_trust_anchor" or "extract_valid_trust_anchor"? I'm not sure that either read better to me. "extract_prevalid" feels awkward and "extract_valid" makes it sound like the function does validation and only accepts valid inputs, which is the opposite of what I'm trying to convey here.

On the documentation side, I think you'll find that none of the callers within other rustls packages do much that could be construed as "verification" of subject name, public key and name constraints, so I feel this documentation leans too hard on that idea. In my understanding the way this is generally is used is that the data source is trusted (for example, for webpki-roots the source is the CCADB and for rustls-native-certs is the OS trust store). Edit: And indeed, Brian's suggestion is closer to this idea: try_from_cert_der_from_trustworthy_source.

I think the key aspect of what you're pointing out is that the callers aren't doing anything that could be construed as verification because they're using a trusted data source that has done that verification. Maybe we can emphasize that instead? I'm not sure I can think of a short name, but something with the spirit of "extract trust anchor from trust store cert"?

I don't feel documenting that the caller should verify all the things does a good job of conveying that idea?

As in you think we can't solve this problem with documentation and should consider a different approach? Or am I misunderstanding?

[ctz]

I think "trust anchor" implies some significant pre-existing validity -- not sure about repeating that in the name of this function.

[cpu]

I think "trust anchor" implies some significant pre-existing validity -- not sure about repeating that in the name of this function.

What did you think about the discussion in briansmith/webpki#294 ?

[djc]

I think the key aspect of what you're pointing out is that the callers aren't doing anything that could be construed as verification because they're using a trusted data source that has done that verification. Maybe we can emphasize that instead? I'm not sure I can think of a short name, but something with the spirit of "extract trust anchor from trust store cert"?

Maybe anchor_from_trusted_cert() or anchor_info_from_trusted_cert()?

I don't feel documenting that the caller should verify all the things does a good job of conveying that idea?

As in you think we can't solve this problem with documentation and should consider a different approach? Or am I misunderstanding?

Basically what you wrote above.

[cpu]

Maybe anchor_from_trusted_cert()

SGTM. Renamed.

[djc]

I think the key aspect of what you're pointing out is that the callers aren't doing anything that could be construed as verification because they're using a trusted data source that has done that verification. Maybe we can emphasize that instead?

Did you also want to adapt the docstring to this thinking?

[cpu]

Did you also want to adapt the docstring to this thinking?

It feels to me like the docstring already covers this line of thinking with the following bits of text:

"This function extracts the components of a trust anchor (see [RFC 5280 6.1.1]) from an X.509 certificate that has already been validated as trustworthy out-of-band."
...
"This function is intended for users constructing TrustAnchor's from existing trust stores that express trust anchors as X.509 certificates."

I'm open to suggestions but don't know what other tweaks to make to the current state.

[djc]

It still has this:

All callers must verify the subject name, public key and name constraints are trustworthy and match expected values before using this function.

Which seems to point in the opposite direction.

Even the "has already been validated as trustworthy" feels different from maybe more something like "from a source trusted to appropriately validate the certificate" (which feels a little awkward).

[cpu]

Even the "has already been validated as trustworthy" feels different from maybe more something like "from a source trusted to appropriately validate the certificate" (which feels a little awkward).

I see what you mean 👍 I've iterated on the wording again. Closer to the mark this time?

[djc]

Nice!

[cpu]

@ctz this is super low priority but I'd like to clear this out of my backlog. Do you think the updated text is worthwhile or should I close this PR? I don't feel strongly at this point and mostly just want to come to a decision vs letting it sit longer. WDYT?

[ctz]

I think my opinion on this is that the commentary improvements are all positive. The benefit of the function name change seems quite marginal IMO, but let's go with it.