Add lints for S/MIME BR 7.1.2.3.b #779
Conversation
| } | ||
| } | ||
| } | ||
| return &lint.LintResult{Status: lint.Pass} |
There was a problem hiding this comment.
This looks like it passes if a CRL distribution point is not present.
b. cRLDistributionPoints (SHALL be present
This extension SHOULD NOT be marked critical. It SHALL contain at least one distributionPoint...
I realize this requirement doesn't perfectly match the lint name, but it is part of 7.1.2.3.b. Is the intention to follow this up with another PR that finishes 7.1.2.3.b?
There was a problem hiding this comment.
This is covered separately in #742.
There was a problem hiding this comment.
The rest of 7.1.2.3.b is handled by #742.
| } | ||
|
|
||
| func (l *subscriberCrlDistributionPointsHTTP) CheckApplies(c *x509.Certificate) bool { | ||
| return util.IsSubscriberCert(c) && (util.IsMultipurposeSMIMECertificate(c) || util.IsStrictSMIMECertificate(c)) |
There was a problem hiding this comment.
I think we need to handle Legacy certificates in this PR or acknowledge that the work has not yet been completed.
Edit: Requirement - "Legacy - At least one uniformResourceIdentifier SHALL have the URI scheme HTTP. Other schemes (LDAP, FTP, ...) MAY be present."
https://github.com/cabforum/smime/blob/main/SBR.md#7123-subscriber-certificates
There was a problem hiding this comment.
I see that the original bug (#712) did not mention the legacy requirement. I think that is a gap on the requirements, but presumably this could be left as a followup.
There was a problem hiding this comment.
IMO the challenge with that is that it leaves a gap in the requirement the lint is fulfilling. From purely a tracking perspective, we'd have to add another task to the checklist for handling the legacy version of the same lint.
There was a problem hiding this comment.
I updated this PR to handle legacy certificates.
| Name: "error - cert without a non-HTTP CRL distribution point", | ||
| InputFilename: "smime/subscriber_with_non_http_crl_distribution_point.pem", | ||
| ExpectedResult: lint.Error, | ||
| }, |
There was a problem hiding this comment.
Could you add an expected details check to ensure the lint is failing for the right reasons in your test cases?
There was a problem hiding this comment.
Done. Thanks for the review.
| InputFilename: "smime/legacy_subscriber_with_non_http_crl_distribution_point.pem", | ||
| ExpectedResult: lint.Error, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Can you also add a test case for a legacy certificate with one http CRL distribution point and one non-HTTP distribution point? It wouldn't hurt to add a failing case on non-legacy with the same setup although it's not strictly required it would show that your lint is effectively covering it's multipurpose(s)
There was a problem hiding this comment.
I commented "Done" on your other comment twice when I meant to comment it on both comments (including this one). I'm still kinda new to GitHub.
| func init() { | ||
| lint.RegisterLint(&lint.Lint{ | ||
| Name: "e_subscribers_crl_distribution_points_are_http", | ||
| Description: "cRLDistributionPoints SHALL have URL scheme HTTP.", |
There was a problem hiding this comment.
Minor, but the BR uses 'URI' rather than 'URL' in this section.
| Description: "cRLDistributionPoints SHALL have URL scheme HTTP.", | |
| Description: "cRLDistributionPoints SHALL have URI scheme HTTP.", |
There was a problem hiding this comment.
I suggest a similar change to the details of the error if you opt to make this change. Otherwise the PR looks good to me.
There was a problem hiding this comment.
Done. Thanks for the review.
v3/testdata/smime/subscriber_with_http_crl_distribution_point.pem
Outdated
Show resolved
Hide resolved
v3/testdata/smime/subscriber_with_mixed_crl_distribution_points.pem
Outdated
Show resolved
Hide resolved
This addresses the first open task of #712.