Implement directoryName constraint validation#489
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
==========================================
+ Coverage 97.01% 97.05% +0.04%
==========================================
Files 20 21 +1
Lines 3950 4379 +429
==========================================
+ Hits 3832 4250 +418
- Misses 118 129 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
What's your use case for this? |
|
I am unfortunately trying to deal with a (privately issued) cert issued by an intermediate with DN constraints. |
ctz
left a comment
ctz
left a comment
There was a problem hiding this comment.
Generally looks good to me. A few comments.
I have tested that TunTrust websites now work with this change in place.
| use crate::der::{self, Tag}; | ||
| use crate::error::Error; | ||
|
|
||
| // Real DNs hold one AVA per RDN, occasionally a few. 16 is well above any sane |
There was a problem hiding this comment.
Not sure I really recognise the AVA terminology here, It's a AttributeTypeAndValue right?
| let mut reader = untrusted::Reader::new(rdn); | ||
| let mut count = 0; | ||
| while !reader.at_end() { | ||
| if count >= MAX_AVAS { |
There was a problem hiding this comment.
I would use match out.get_mut(count) to ensure the bounds check and usage stay aligned in the future.
| let mut count = 0; | ||
| while !reader.at_end() { | ||
| if count >= MAX_AVAS { | ||
| return Err(Error::BadDer); |
There was a problem hiding this comment.
Probably deserves a unique error, as BadDer is overused in this library to mean a number of different cases; this is an internal limitation rather than a problem with the input.
| // RDNSequence. Strip the wrapper to expose the RDNSequence content. | ||
| pub(super) fn strip_explicit_sequence( | ||
| value: untrusted::Input<'_>, | ||
| ) -> Result<untrusted::Input<'_>, Error> { |
There was a problem hiding this comment.
I would be tempted to introduce a bunch of trivial private newtypes to explain what these untrusted::Input values (here and below) mean.
| // Both inputs are RDNSequence content (no outer SEQUENCE tag). Callers | ||
| // holding `[4]` directoryName GeneralName bytes should run them through | ||
| // `strip_explicit_sequence` first; `Cert::subject` is already RDNSequence | ||
| // content. |
There was a problem hiding this comment.
This type of comment better reified into the type system?
| excluded_subtrees: Option<untrusted::Input<'_>>, | ||
| budget: &mut Budget, | ||
| ) -> Option<Result<(), Error>> { | ||
| walk_constraints( |
There was a problem hiding this comment.
I'm wondering if this is the venue where the "when the certificate includes a non-empty subject field" in 4.2.1.10 can be addressed?
Ideally with a test, if there isn't already one. Apologies if this is already dealt with elsewhere!
| struct Normalizer<'a> { | ||
| inner: CodePoints<'a>, | ||
| state: NormState, | ||
| buffered: Option<char>, | ||
| } | ||
|
|
||
| #[derive(PartialEq)] | ||
| enum NormState { | ||
| Leading, | ||
| Content, | ||
| PendingWs, | ||
| Done, | ||
| } |
There was a problem hiding this comment.
nit: kind of wondering whether NormState could be merged into the Normalizer type? As some of these fields aren't meaningful for some states.
|
Does this really need to be so complicated? Is it not possible to restrict this to supporting only reasonable string encodings and canonical DER encoding? Like, during name constraint processing, e.g. if you see a BMPString, just fail, following "TeletexString, BMPString, and UniversalString are included for backward compatibility, and SHOULD NOT be used for certificates for new subjects." Hopefully every producer of certificates that is relevant to us is following that advice. Because X.509 certificates are DER, there is really only one correct ordering of the parts of a DN, so all the complexity with supporting the constraint and the presented name having different orderings is unnecessary if we require that the CA hierarchy be conformant with X.509. Actually the requirement would be less than that--just be consistent in how names are encoded in presented names and constraints; i.e. require that the constraint and the presented name be in the same order. If they aren't in the same order then they aren't valid. I think this would notably simplify the implementation and make it more likely that it is actually safe. |
|
More generally, the original design philosophy of mozilla::pkix and the webpki crate is to reject as many weird things as we possible can, to set the stage for having a much more limited X.509 profile for the WebPKI, to make it easier to make safe implementations. This PR is really far from that design philosophy. I highly recommend that you look at the very simple matching that mozilla::pkix does for these constraints, and aim for similar simplicity in the initial implementation. If we need more complexity then that complexity should be added in small, minimal, doses. Is Firefox able to handle the constraints that your CA hierarchy uses? |
|
Also, |
4 participants
A few notes:
GeneralName::from_der.rdn_eqis an O(n^2) nested loop around the two sets of AVAs. This mirrors the overall approach taken in BoringSSL, though the one here is implemented with fixed size buffers and a bitmask rather than vectors to keep everything no-std.Depends on rustls/rcgen#429, so I have a Cargo patch for rcgen until a new release gets cut.
Closes #19