-
-
Notifications
You must be signed in to change notification settings - Fork 607
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
ca/linter: Port safety checks from ceremony tool #7498
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.
Logic LGTM, just some suggestions for cleanup
ca/ca.go
Outdated
return err | ||
} | ||
|
||
if lintRawTBSCert == nil || leafRawTBSCert == 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.
a) don't directly check for nil here, instead check len(foo) == 0
b) no need for this since the anonymous function does the same check on line 673; pick one or the other
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.
What is the functional difference between checking for nil
or len 0
on a []byte
? The zero value of a []byte
is going to be 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.
You're right, the true zero value of []byte
is nil
. But generally the purpose of checks like this is not simply to check for the zero value, but rather to check for obviously invalid inputs, so that more helpful error messages can be displayed and so that we can avoid doing expensive checks if we already know they're going to fail. So a len(foo) == 0
check also catches the case where the byte slice exists but has an obviously-invalid (zero) length.
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 can get rid of this because tbs.Empty()
on line 670 is handling the len(foo) == 0
case anyways.
In #7005 several safety checks were added to the
ceremony
tool:This change extracts the
RawSubject
toRawIssuer
DER byte comparison into the//linter
package proper so that it can serve both//ca
and//cmd/ceremony
.Adds a helper function
verifyTBSCertificateDeterminism
to//ca
similar to an existing check in//cmd/ceremony
. This code is not shared because we want//cmd/ceremony
to largely stand alone from boulder proper. The helper performs a byte comparison on theRawTBSCertificate
DER bytes for a given linting certificate and leaf certificate. The goal is to verify thatx509.CreateCertificate
was deterministic and produced identical DER bytes after each signing operation.Fixes #6965