x509 Verification: ExtensionPolicy refactor. #11801
Conversation
…rbitrary extensions support.
| ( | ||
| // 5280 4.2.2.1: Authority Information Access | ||
| AUTHORITY_INFORMATION_ACCESS_OID, | ||
| ExtensionValidator::maybe_present( |
There was a problem hiding this comment.
I would be glad to hear any ideas on improving the compactness of the initialisation code here.
I considered implementing a function that accepts an argument for each of these "default" validators, but that would be extremely bug-prone since it would be order-dependent.
There was a problem hiding this comment.
(The github preview doesn't really make it clear what I'm talking about, so check out the full file)
| pub(crate) fn new( | ||
| validator_by_oid: HashMap<asn1::ObjectIdentifier, ExtensionValidator<B>>, | ||
| ) -> Self { | ||
| Self { validator_by_oid } |
There was a problem hiding this comment.
So this was the original design here, and we intentionally moved away from it for performance. Unless it's absolutely necessary, I'd prefer not to revert this change.
Can you explain a bit more what the motivation for this change is?
There was a problem hiding this comment.
The motivation is to be able to support custom extension types. I was initially planning to limit the scope of user-customizable extension verification to user provided callbacks, but @woodruffw said he would prefer to allow custom extension OIDs since they have a usecase for that (see #11165 (comment) and previous comments)
If performance is really such an issue with this implementation and we still want to keep the ability to define validators for arbitrary extensions, we could potentially just leave the default extension validation as is and add a map for custom extension validators.
There was a problem hiding this comment.
Yes, I think named validators for the standard ones + a map for extending it sounds good.
There was a problem hiding this comment.
Alright. What I'm not sure about though is if I should once again wait until the meat of the feature is implemented so the coverage for this is free, or if I should add some tests specific to this to the rust crate and make the PR now.
There was a problem hiding this comment.
I think either approach could work, so whichever you prefer is fine. But if it were me, I'd implement the whole feature, and then if the overall patch is too large we can look at ways to split pieces out.
|
Closing this, will be implemented together with the other changes and split out if necessary. |
Another quick preliminary PR that just refactors some existing code to make implementation of user-provided extension policies more straightforward. (A good change even on it's own in my opinion.)