-
Notifications
You must be signed in to change notification settings - Fork 110
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
Permit underscores in DNSNames if-and-only-if those certificates are valid for less than 30 days and during BR 1.6.2's permissibility period #660
Conversation
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.
A minor nit here, and then I suspect that the dupe certs will get resolved from the other PR. If you decide to not go with that, then I think this is pretty good.
v3/util/time.go
Outdated
@@ -60,8 +60,10 @@ var ( | |||
MozillaPolicy24Date = time.Date(2017, time.February, 28, 0, 0, 0, 0, time.UTC) | |||
MozillaPolicy241Date = time.Date(2017, time.March, 31, 0, 0, 0, 0, time.UTC) | |||
MozillaPolicy27Date = time.Date(2020, time.January, 1, 0, 0, 0, 0, time.UTC) | |||
BALLOT_SC_12_Ineffective = time.Date(2019, time.April, 1, 0, 0, 0, 0, time.UTC) |
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.
BALLOT_SC_12_Ineffective = time.Date(2019, time.April, 1, 0, 0, 0, 0, time.UTC) | |
BallotSC12Ineffective = time.Date(2019, time.April, 1, 0, 0, 0, 0, time.UTC) |
WDYT? (e.g. lines 47-62)
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.
Thanks, I hop back-and-forth between Go/Rust/Python/Java often enough that the naming conventions bleed over each other sometimes.
And yeah, with regard to the certs, they may be duplicates DNS names but I believe that I did generate entirely different ones just for the sake of tests not sharing literally same test certs, which you can easily imagine exploding in someone's face in the future (and also because merge management woulda been a little annoying).
v3/lints/cabf_br/lint_underscore_present_with_too_long_validity.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
func (l *UnderscorePresentWithTooLongValidity) Execute(c *x509.Certificate) *lint.LintResult { | ||
validity := c.NotAfter.Sub(c.NotBefore) |
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.
Something something "inclusive", but this predates the 5280 language coming over :)
type UnderscorePresentWithTooLongValidity struct{} | ||
|
||
func (l *UnderscorePresentWithTooLongValidity) CheckApplies(c *x509.Certificate) bool { | ||
return util.IsSubscriberCert(c) && util.DNSNamesExist(c) |
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.
Can/should the check from 47 - 49 be moved here?
return util.IsSubscriberCert(c) && util.DNSNamesExist(c) | |
return util.IsSubscriberCert(c) && | |
util.DNSNamesExist(c) && | |
c.NotBefore.AddDate(0, 0, 30).Before(c.NotAfter) |
Might be a way to simplify further. But is a bit of a suggestion
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 don't think so? If we do this check here then what we would be saying is that this lint does not apply to this certificate at all (which is usually reserved for scenarios such as being an irrelevant cert type or not having a particular field).
I do prefer that method of adding 30 days, though. Far more correct than the naive time.Hour*24*30
.
NIST CURVE: P-256 | ||
X509v3 extensions: | ||
X509v3 Subject Alternative Name: | ||
DNS:no.underscore.com |
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.
ditto re: .test
fixes
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 (thank you for your patience on this, I've been traveling for business and I almost missed a flight trying to juggle these similiar-but-different certs so I just punted for the week).
…y.go Co-authored-by: Ryan Sleevi <ryan.sleevi@gmail.com>
@sleevi gentle re-engagement on this |
func init() { | ||
lint.RegisterLint(&lint.Lint{ | ||
Name: "e_underscore_present_with_too_long_validity", | ||
Description: "From 20128-12-10 to 2019-04-01, DNSNames may contain underscores if-and-only-if the certificate is valid for less than thirty days.", |
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.
nit: there is a typo on this date (20128 vs 2018)
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.
Thi looks good other than the one nit.
This lint encodes a brief period from December 10th 2018 to April 1st 2019 wherein CABF BR permitted underscores within DNS names if-and-only if those certificates were valid for fewer than 30 days.