-
Notifications
You must be signed in to change notification settings - Fork 108
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
Ensure AIA URLs point to public paths #760
Ensure AIA URLs point to public paths #760
Conversation
… test results and update integration config
@christopher-henderson this is ready for review. I can't add you as a reviewer myself. |
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.
Thank you for the lints! Looks like a pretty straight forward change.
The current lint tries to parse the url and make sure the DNS name has a valid TLD. Should I instead try to validate the scheme of the field before parsing in that lint only?
I think we can let the URL parser do its thing as it will parse out the scheme for us. If there is no scheme, then that is a a problem in-and-of-itself that the parser will report to us (that is, other schemes are allowed however I doubt that not any scheme attached is okay).
v3/lints/cabf_smime_br/lint_legacy_aia_contains_internal_names.go
Outdated
Show resolved
Hide resolved
v3/lints/cabf_smime_br/lint_legacy_aia_contains_internal_names.go
Outdated
Show resolved
Hide resolved
return util.IsLegacySMIMECertificate(c) | ||
} | ||
|
||
func (l *smimeLegacyAIAContainsInternalNames) Execute(c *x509.Certificate) *lint.LintResult { |
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.
...at least one accessMethod SHALL have the URI scheme HTTP
I reckon that we can also enforce this clause a bit more closely by checking up on the parse scheme
atLeastOneHttp := false
...
...
if purl.Scheme == "http" {
atLeastOneHttp = true
}
...
...
if !atLeastOneHttp {
return &lint.LintResult{Status: lint.Error}
}
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 implemented my take on this one, but I became unsure about it so if you can take a look that would be helpful. Gist is, in the requirement is mentions that at least one accessMethod MUST be http, however I believe it applies independently to the OCSPServers and IssuingCAs. If you think I'm being overly aggressive here, let me know.
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.
hrmmm I think that my only thought is that this would also require that there be a minimum of one at least OCSP and at least one CRL (that is, if the loop never enters then atLeastOnHttp
will be false
).
Regardless, if there is such a requirement (at least one OCSP and at least one CRL) then it should be a separate lint (trying to encode all surrounding facts into each lint is how they commonly end up in deadlock).
So I guess we could just check the empty condition before failing out.
if !atLeastOneHttp && len(c.OCSPServer) != 0 {
return &lint.LintResult{Status: lint.Error, Details: "at least one accessMethod MUST have the URI scheme HTTP"}
}
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.
Okay, I agree and made that change. Do you assume that the same change is not needed for the IssuingCertificateURL list?
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.
If you extend this small change to CRLs as well then I think we'd be gtg
on these!
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.
done
v3/lints/cabf_smime_br/lint_strict_aia_contains_internal_names.go
Outdated
Show resolved
Hide resolved
v3/lints/cabf_smime_br/lint_strict_aia_contains_internal_names.go
Outdated
Show resolved
Hide resolved
Thanks for the review @christopher-henderson I had one question above but otherwise should be ready to re-review. 👍 e: I confirmed internally that I shouldn't have allowed https. |
fd93f7f
to
8a819d7
Compare
As part of the SMIME work, I discovered at least one example of a cert that was issued using an outdated OCSP URL that would have been valid prior to the SMIME BRs. The rules around the AIA paths are not super strict but they are similar for both TLS and SMIME requirements, which read:
For publicly trusted certificates that are present in CT logs, these URLs are required to point to a resolvable location and shouldn't contain internal names. However, that requirement is not strictly codified in the requirements but is implied by what the resource is intended to point to. This lint intends to check and warn for this condition that may not be an actual problem, but probably is.
Note: I don't believe the Legacy lint is quite right here. The rule for Legacy reads
The current lint tries to parse the url and make sure the DNS name has a valid TLD. Should I instead try to validate the scheme of the field before parsing in that lint only?
I also combined the Strict and Multipurpose lints into one as they have the same requirements.
Feedback welcome and desired!