Added OCSP worker to retrieve certificate status#286
Conversation
| res <- worker.Result{ | ||
| Success: false, | ||
| WorkerName: workerName, | ||
| Result: out, |
There was a problem hiding this comment.
Errors should be in the Errors field of the worker.Result struct. In case of error Result is nil.
| defer conn.Close() | ||
|
|
||
| issuerCertificate := conn.ConnectionState().PeerCertificates[1] | ||
| certificate := conn.ConnectionState().PeerCertificates[0] |
There was a problem hiding this comment.
worker.Input contains all the certificates so the established connection above and all certificate retrieval code is not needed.
There was a problem hiding this comment.
So the worker.Input.CertificateChain.Certs is a string array. How do you covert the strings in the array to a x509 certificate? I need to pass in the website/issuer cert as a x509 when creating the ocsp request.
There was a problem hiding this comment.
Each entry in the array is a base64 encoding of the raw certificate so all you have to do is :
raw := base64.DecodeString(c) // c being a Certs element
cert := x509.ParseCertificate(raw)
| if err != nil {w.error(res, "Could not create OCSP request: %s", err)} | ||
|
|
||
| httpResponse, err := http.Post(certificate.OCSPServer[0], "application/ocsp-request", bytes.NewReader(req)) | ||
| if err != nil {log.Fatal(err)} |
There was a problem hiding this comment.
Lose the log.Fatal, report this as an error.
|
|
||
| opts := &ocsp.RequestOptions{Hash: crypto.SHA256} | ||
| req, err := ocsp.CreateRequest(certificate, issuerCertificate, opts) | ||
| if err != nil {w.error(res, "Could not create OCSP request: %s", err)} |
There was a problem hiding this comment.
Expand the error handler to multiple lines.
You should also return after the error reporting, so you don't continue to the rest of the code in case of error.
| } | ||
| } | ||
|
|
||
| func (w ocspStatusWorker) error(res chan worker.Result, messageFormat string, args ...interface{}) { |
There was a problem hiding this comment.
I don't think a separate error function is needed.
You could just initialize a new worker.Result in the beginning, in case of error just append to the Errors slice and in case of success just marshal the result in.
|
How's this? |
jvehent
left a comment
jvehent
left a comment
There was a problem hiding this comment.
This is looking good. The only comments I have are around coding best practices. The logic is sound.
| @@ -0,0 +1,81 @@ | |||
| package ocspStatusWorker | |||
There was a problem hiding this comment.
nit: remove "Worker" from the name, it's already under /worker/
| res := worker.Result{WorkerName: workerName, Success:false} | ||
|
|
||
| rawCert, _ := base64.StdEncoding.DecodeString(in.CertificateChain.Certs[0]) | ||
| certificate, _ := x509.ParseCertificate(rawCert) |
There was a problem hiding this comment.
the end entity is already parsed in in.Certificate
| rawCert, _ := base64.StdEncoding.DecodeString(in.CertificateChain.Certs[0]) | ||
| certificate, _ := x509.ParseCertificate(rawCert) | ||
| rawIssuerCert, _ := base64.StdEncoding.DecodeString(in.CertificateChain.Certs[1]) | ||
| issuerCertificate, _ := x509.ParseCertificate(rawIssuerCert) |
There was a problem hiding this comment.
You should check for errors here.
It might also be worthwhile to make sure the issuer is the right one (that it sign the end-entity) because ordering isn't always guaranteed. You can use CheckSignatureFrom from the x509 package for this.
| opts := &ocsp.RequestOptions{Hash: crypto.SHA256} | ||
| req, err := ocsp.CreateRequest(certificate, issuerCertificate, opts) | ||
| if err != nil { | ||
| res.Errors = append(res.Errors, err.Error()) |
There was a problem hiding this comment.
errors need to be inserted into the response channel, otherwise they will be dropped. Something like this:
func (w runner) error(res chan worker.Result, messageFormat string, args ...interface{}) {
out, _ := json.Marshal(fmt.Sprintf(messageFormat, args...))
res <- worker.Result{
Success: false,
WorkerName: workerName,
Result: out,
}
}and use it as follows:
if err != nil {
w.error(res, "there was an error: %v", err)
return
}| res.Errors = append(res.Errors, err.Error()) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
add a defer httpResponse.Body.Close()
|
|
||
| status := OCSPStatus{ Status:OCSPResponse.Status, RevokedAt:OCSPResponse.RevokedAt } | ||
|
|
||
| out, _ := json.Marshal(status) |
There was a problem hiding this comment.
don't ignore errors, you never know what the OCSP response will contain
|
@jvehent It's read to be looked over again. |
|
@fr0zenbits : you say it's ready to be reviewed again but I'm not seeing any changes following my review. Are there missing commits? |
|
@fr0zenbits hey! I took your work in this PR and finished out the OCSP status worker. Hope that's ok. See #336 |
ghost
closed this
3 participants
This is one of my first go projects, so any constructive feedback is appreciated. I know the OCSP code works, but I am not sure if I implemented the worker-releated code correctly.