add awsCertlint worker #315
Conversation
worker/awsCertlint/awsCertlint.go
Outdated
| workerName = "awsCertlint" | ||
| workerDesc = "Runs awslabs/certlint over a given Certificate, categoriezes output for display on the certificate" | ||
|
|
||
| certlintDirectory = "/go/certlint" // path from tools/Dockerfile-scanner |
There was a problem hiding this comment.
This path isn't ideal, but is it worthwhile to change? Probably, since I assume we're wanting to add more binaries to check certificates with.
There was a problem hiding this comment.
This should be configurable, similar to what we did with evChecker https://github.com/mozilla/tls-observatory/blob/master/worker/evCheckerWorker/evCheckerWorker.go#L24
|
Oh. I called this |
tools/Dockerfile-scanner
Outdated
|
|
||
| ADD . /go/src/$PROJECT | ||
| ADD cipherscan/ /opt/cipherscan/ | ||
| ADD conf/ /etc/tls-observatory/ | ||
| RUN chown tlsobs -R /go /opt -R | ||
|
|
||
| # switch to tlsobs user | ||
| RUN mkdir -p /home/tlsobs && \ |
There was a problem hiding this comment.
maybe use -m in the useradd command instead of mkdir /home/tlsobs ?
worker/awsCertlint/awsCertlint.go
Outdated
|
|
||
| var ( | ||
| workerName = "awsCertlint" | ||
| workerDesc = "Runs awslabs/certlint over a given Certificate, categoriezes output for display on the certificate" |
There was a problem hiding this comment.
s/categoriezes/categorize/, but even though that sentence doesn't make a lot of sense. can you rephrase?
worker/awsCertlint/awsCertlint.go
Outdated
| Notices []string `json:"Notices"` | ||
| Warnings []string `json:"Warnings"` | ||
| Errors []string `json:"Errors"` | ||
| FatalErrors []string `json:"FatalErrors"` |
There was a problem hiding this comment.
nit:gofmt
nit: json keys should be lowercase
worker/awsCertlint/awsCertlint.go
Outdated
| func (e eval) runCertlint(cert certificate.Certificate) (*Result, error) { | ||
| tmp, err := ioutil.TempFile("", "awslabs-certlint") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating temp dir", cert.ID) |
There was a problem hiding this comment.
also you're missing a %s in the format string
worker/awsCertlint/awsCertlint.go
Outdated
| defer os.Remove(tmp.Name()) | ||
| x509Cert, err := cert.ToX509() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting to x509.Certificate", cert.ID) |
worker/awsCertlint/awsCertlint.go
Outdated
| } | ||
|
|
||
| // Run certlint over certificate | ||
| cmd := exec.Command("ruby", "-I", "lib:ext", "bin/certlint", tmp.Name()) |
There was a problem hiding this comment.
I like to run those with a timeout to prevent them from hanging. Take a look at https://github.com/mozilla/tls-observatory/blob/master/connection/retriever.go#L47-L56
worker/awsCertlint/awsCertlint.go
Outdated
| } | ||
|
|
||
|
|
||
| // From: https://github.com/awslabs/certlint#output |
There was a problem hiding this comment.
I wonder if certlint could return JSON output to make it easier to process it on our side. What do you think of that, @pzb?
There was a problem hiding this comment.
Fyi, I filed an issue about this too. amazon-archives/certlint#63
worker/awsCertlint/awsCertlint.go
Outdated
| } | ||
|
|
||
| // Build results for webview | ||
| results = append(results, "* awslabs/certlint Results") |
There was a problem hiding this comment.
nit: remove "Results"
It would be nice to have a summary on the first line, such as * awslabs/certlint: no issues found or 1 warning found
|
This is looking pretty good! |
|
@jvehent this should be good to go. |
There was a problem hiding this comment.
Almost there! Since this makes use of a separate binary, you should disable the worker at init if the binary isn't found. Otherwise, all scans will fail with this error:
time="2018-04-09T09:11:15-04:00" level=error msg="Worker returned with errors" errors="[error starting awslabs/certlint on certificate %!s(int64=407), err=chdir /go/certlint: no such file or directory, out=\"\" for certificate %!s(int64=407)]" worker_name=awsCertlint
Fixes: #257