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
checkocsp: verify validity period of signing certificate #4852
Conversation
// certificate signed it, and that that certificate is currently valid. | ||
func checkSignature(resp *ocsp.Response, issuer *x509.Certificate) error { | ||
var ocspSigner = issuer | ||
if delegatedSigner := resp.Certificate; delegatedSigner != 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.
x/crypto/ocsp already does this signature checking for us, I think all we need to do is check that the certificate is still valid.
test/ocsp/helper/helper.go
Outdated
@@ -222,5 +266,10 @@ func parseAndPrint(respBytes []byte, cert, issuer *x509.Certificate, expectStatu | |||
fmt.Printf(" RevocationReason %d\n", resp.RevocationReason) | |||
fmt.Printf(" SignatureAlgorithm %s\n", resp.SignatureAlgorithm) | |||
fmt.Printf(" Extensions %#v\n", resp.Extensions) | |||
fmt.Printf(" Certificate:\n") |
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 will panic if we don't get a bundled certificate.
test/ocsp/helper/helper.go
Outdated
if resp.Certificate == nil { | ||
fmt.Printf(" Certificate: nil\n") | ||
} else { | ||
fmt.Printf(" Certificate:\n") |
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.
extremely nit: Printf with no formatting directive.
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.
somewhat surprised we don;t get a vet error for this.
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.
go vet actually does spot this, but we don't run go vet in Travis because it's too noisy, and it's explicitly documented as something not recommended for CI.
But thanks for the suggestion on running it- I actually found a bug on another line.
No description provided.