Reintroduce lint for inconsistent KU and EKU #708
Conversation
|
@christopher-henderson any comments on this PR? |
|
@vanbroup thank you for your patience. I have been under some extreme duress outside of ZLint in the past month. This, coupled with not getting a push notification on this PR, left me somewhat derelict on the issue. There are a lot of corrections to the expectations in the test corpus, which are typically a fair deal of work on me to independently verify vis-a-vis governing requirements. I will likely start there as the code itself is rarely a blocking issue. |
There was a problem hiding this comment.
Thank @vanbroup !
It seems that the most common breach is caused by attempts to double-bill client certificates. From what I've smoke checked so far that makes sense. Plus, it makes sense in general. I have some experience building infrastructure that relies on client certificates for techniques such as mTLS and it is pretty common for implementers to let the rules fly out the door when doing so.
Aside from that, my only sincere concern is the illegibility of multiPurpose. The boolean expressions involved in this requirement has a history of causing confusion, as-is. So I have a sincere interest in the surrounding logic being as clear as possible, both for the purposes of immediate verification as well as ensuring future generations are not left reverse engineering a difficult function while trying to troubleshoot an already difficult requirement.
| for _, extKeyUsage := range c.ExtKeyUsage { | ||
| var i int | ||
| if _, ok := eku[extKeyUsage]; !ok { | ||
| return &lint.LintResult{Status: lint.Pass} |
There was a problem hiding this comment.
Should this be continue rather than return? It seems slightly off to succeed fast if we hit an EKU we're not familiar with.
There was a problem hiding this comment.
If we are not familiar with one of the EKU's it will be impossible to come to a conclusion, as the KU might be authorized by this EKU.
|
Thanks for the review, I have not forgotten this but need to free some time to address the comments. |
vanbroup
force-pushed
the
e_key_usage_and_extended_key_usage_inconsistent
branch
from
5d1c794
to
af55782
Compare
|
@christopher-henderson sorry for the delay, I think that your comments have been addressed |
There was a problem hiding this comment.
Thank you @vanbroup! Even with small changes, this is much easier to reason through than I recall it being (it is, indeed, a slightly tricky algorithm).
|
@vanbroup dataEncipherment is currently permitted by the TLS BRs for RSA keys (7.1.2.7.11 Subscriber Certificate Key Usage) but rejected by this lint. Although it's highlighted that this KU is Pending Prohibition, I'm not sure if this was intentional? Perhaps the TLS BRs are overriding RFC5280 on purpose, or it's an error. Either way, the lint will block a currently permitted value by the BRs. |
|
The lint follows RFC 5280 and by checking against the major CT logs we tried to identify any inconsistent implementations, we did not explicitly check the CA/Browser Forum requirements. We checked this lint to hundreds of millions of certificates in the CT log's without finding any issues, do you know how often this happens across the ecosystem or are you running into this problem in a new certificate profile? |
|
@chrisbn hmmm...it's unfortunate that BRs and RFC5280 seem to be disagreeing here. With regards to getting infra working again, there may be several options. The not-so-great option is to utilize Alternatively, consumers of the CLI tool can choose to parse out the printed JSON and decide if a failure in this lint is ok-or-not. Consumers of the library can similarly choose whether-or-not they prefer the BRs interpretation over 5280. |
This updated lint has been created and reviewed by several members of the @pkic and its addressing the issues with
e_key_usage_and_extended_key_usage_inconsistentintroduced in #497 (and refined in #528), see also #553 and #556.We have tried to provide sufficient details in the error for debugging purposes:
{"e_key_usage_and_extended_key_usage_inconsistent":{"result":"error","details":"KeyUsage [DigitalSignature KeyEncipherment] (00000101) inconsistent with ExtKeyUsage serverAuth"}}The lint has been tested against all valid certificates in
yeti2024, most ofyeti2023, and hundreds of millions inargon2023without issues.In the test corpus we identified 29960 certificates with an invalid combination:
CC: @sandorszoke