Add lint to cover TLS BR v2 EKU checks #833
Conversation
These lints are covered by the new `e_sub_cert_eku_check` lint that will lint according to the TLS BR v2 language.
…erAuth The `util.IsServerAuthCert` did not consider certificates that attest the CA/Browser Forum policy OIDs but do not include the `serverAuth` EKU. This has been addressed and caused some mintor changes in the test corpus.
vanbroup
force-pushed
the
e_sub_cert_eku_check
branch
from
c38b81d to
15db930
Compare
| } | ||
| } | ||
|
|
||
| if (len(c.ExtKeyUsage) > 2 && !hasClientAuthEKU) || len(c.UnknownExtKeyUsage) > 0 { |
There was a problem hiding this comment.
Maybe you could set a variable in the default case in the range c.ExtKeyUsage loop and check that.
There was a problem hiding this comment.
I may advocate for keeping a list of of OIDs found during the execution of the lint and cross referencing them at them end against the list of known OIDs.
var known = map[string]struct{}{
"1.3.6.1.5.5.7.3.1": {},
"1.3.6.1.5.5.7.3.2": {},
"1.3.6.1.5.5.7.3.3": {},
"1.3.6.1.5.5.7.3.4": {},
"1.3.6.1.5.5.7.3.8": {},
"1.3.6.1.5.5.7.3.9": {},
"2.5.29.37.0": {},
"1.3.6.1.4.1.11129.2.4.4": {},
}
func lint() {
found := []string
for _, eku := range c.ExtKeyUsage {
found = append(found, eku.String()
....
....
....
}
for _, eku := range.UnknownExtKeyUsage {
found = append(found, eku.String()
....
....
....
}
for _, oid := range found {
_, ok := known[oid]
if !ok {
return &lint.LintResult{Status: lint.Warn, Details: "any other value than id-kp-serverAuth and id-kp-clientAuth is NOT RECOMMENDED"}
}
}
}There was a problem hiding this comment.
The ExtKeyUsage and UnknownExtKeyUsage are not of the same type:
ExtKeyUsage []ExtKeyUsage // Sequence of extended key usages.
UnknownExtKeyUsage []asn1.ObjectIdentifier // Encountered extended key usages unknown to this package.The lint currently checks for MUST, MUST NOT, MAY, and NOT RECOMMENDED, and returns the corresponding lint status and error.
We can create a map with each EKU and the requirement (i.e., MUST) and lookup the value while ranging over the two EKU lists, but I guess this would be less efficient
There was a problem hiding this comment.
I reckon that this would be an efficiency modifier in the range of nanoseconds (possibly into ms) for each run of the lint. In general, legibility trumps such optimizations since correctness of the lint is paramount.
| return &lint.LintResult{Status: lint.Error, Details: "id-kp-serverAuth MUST be present"} | ||
| } | ||
|
|
||
| if len(c.UnknownExtKeyUsage) > 0 { |
There was a problem hiding this comment.
This check is not needed
| } | ||
| } | ||
|
|
||
| if (len(c.ExtKeyUsage) > 2 && !hasClientAuthEKU) || len(c.UnknownExtKeyUsage) > 0 { |
There was a problem hiding this comment.
The ExtKeyUsage and UnknownExtKeyUsage are not of the same type:
ExtKeyUsage []ExtKeyUsage // Sequence of extended key usages.
UnknownExtKeyUsage []asn1.ObjectIdentifier // Encountered extended key usages unknown to this package.The lint currently checks for MUST, MUST NOT, MAY, and NOT RECOMMENDED, and returns the corresponding lint status and error.
We can create a map with each EKU and the requirement (i.e., MUST) and lookup the value while ranging over the two EKU lists, but I guess this would be less efficient
This introduces a new lint called
e_sub_cert_eku_checkand sets the ineffective date fore_sub_cert_eku_server_auth_client_auth_missingandw_sub_cert_eku_extra_values.This update only covers subscriber certificates, the lints for CA certificates will also need to be reviewed.
The
util.IsServerAuthCertdid not consider certificates that attest the CA/Browser Forum Reserved Certificate Policy Identifiers as specified in section 7.1.6.1 of the BRs, but who did not include theserverAuthEKU. This has been addressed to cover the expectations of attesting a policy and to cover all test scenario's of this lint.