-
Notifications
You must be signed in to change notification settings - Fork 0
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
Chris Henderson's Code Suggestions for CRL Linting Infrastructure #2
Conversation
onOrAfterEffective := l.EffectiveDate.IsZero() || util.OnOrAfter(c.NotBefore, l.EffectiveDate) | ||
strictlyBeforeIneffective := l.IneffectiveDate.IsZero() || c.NotBefore.Before(l.IneffectiveDate) | ||
return onOrAfterEffective && strictlyBeforeIneffective | ||
return checkEffective(l.EffectiveDate, l.IneffectiveDate, 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.
This datetime range checking function has had a history of causing trouble due to off-by-one errors caused by general confusion on the semantics of the function.
So, in order to reduce the surface area caused by having the logic copied in two spots, I moved it's body out to a private utility function located at the end of this file.
Status: Fatal, | ||
Details: details} | ||
} | ||
lint := l.Lint() |
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.
Similar to the above centralization of CheckApplies
, lifted the interface cast-and-check into Configuration
type itself so as to centralize the check (plus the error string construction).
lookup.Lock() | ||
defer lookup.Unlock() | ||
if existing := lookup.lintsByName[name]; existing != nil { |
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 went ahead and placed a write lock on the full scope of the body in order to avoid the time-of-check-time-of-use bug.
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.
@aaomidi note that I did not touch the use of generics in this change as I did not have quite enough time in my afternoon to fit the semantics of a non-generic LinterLookup
into my head. I am hoping that shifting LinterLookup
down into a non-generic is fairly straightforward to you.
Thank you for making this PR! It definitely is a great way to collaborate on larger changes such as this. All of these changes look ideal. And understood on LinterLookup - I'll handle that in a future commit. We also probably need to discuss and figure out how we want to handle the situation with zcrypto. There's a couple of solutions we can take there, but I think none of them are ideal from my PoV. |
No description provided.