-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lint to check that SubCA certificates do not have illegal values in their EKU extension #840
Conversation
Added //nolint:all to comment block to avoid golangci-lint to complain about duplicate words in comment
Fixed import block
Fine to me. Co-authored-by: Christopher Henderson <chris@chenderson.org>
As per Chris Henderson's suggestion, to "improve readability".
As per Chris Henderson's suggestion.
Added CABFEV_Sec9_2_8_Date
if eku == x509.ExtKeyUsageEmailProtection || | ||
eku == x509.ExtKeyUsageCodeSigning || | ||
eku == x509.ExtKeyUsageTimeStamping || | ||
eku == x509.ExtKeyUsageOcspSigning { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that we are referring to sub-section g?
For Subordinate CA Certificates that will be used to issue TLS certificates, the value
id-kp-serverAuth [RFC5280] MUST be present. The value id-kp-clientAuth
[RFC5280] MAY be present. The values id-kp-emailProtection [RFC5280], id-kpcodeSigning [RFC5280], id-kp-timeStamping [RFC5280], and anyExtendedKeyUsage
[RFC5280] MUST NOT be present. Other values SHOULD NOT be present.For Subordinate CA Certificates that are not used to issue TLS certificates, then the
value id-kp-serverAuth [RFC5280] MUST NOT be present. Other values MAY be
present, but SHOULD NOT combine multiple independent usages (e.g. including idkp-timeStamping [RFC5280] with id-kp-codeSigning [RFC5280]).
So unless my eyes are missing it, I think that since OCSP Signing is not explicitly enumerated that it, so would fall under the Other values SHOULD NOT be present
clause, thus making it a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So unless my eyes are missing it, I think that since OCSP Signing is not explicitly enumerated that it, so would fall under the
Other values SHOULD NOT be present
clause, thus making it a warning?
This list matches 7.1.2.10.6 CA Certificate Extended Key Usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathewhodson you are correct, it is enumerated in latest. This is where it gets mildly tricky since this lint is marked as being effective as of August 2020 via CABF/BR 1.7.1 where I don't think I see it marked as a MUST NOT
.
@defacto64 did you have a particular reason for choosing 1.7.1?
This lint verifies that Subordinate CA certificates do not have illegal values in their EKU extension according to the CABF BRs, and generates an error in case they do. This kind of problem has happened a few times in the past, as can be seen on https://bugzilla.mozilla.org, but apparently not in recent times; at any rate, it's better to prevent it.
This lint does not discriminate between normal SubCA certificates and cross-certificates, this being impossible in the absence of an extra input (which could be passed through a specific lint-configuration, but I find it preferable to do without it). However, this is not necessary in order to do a few essential checks that were missing until now.
The lint's logic is as follows: in the CABF BR context, if the anyExtendedKeyUsage is present in a SubCA cert, then it can be inferred that it's actually an unrestricted cross certificate; if instead anyExtendedKeyUsage is not present, then it must be either a restricted cross certificate or a normal SubCA (non cross), and in both these cases the same rules apply to the EKU extension.