observer: ccadb: check reported CRL shards for completeness#8890
observer: ccadb: check reported CRL shards for completeness#8890inahga wants to merge 2 commits into
Conversation
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Looks good to me. It's a bit funny that httpGetExpectingStatusCode will retry until timeout when it gets a 200, but given that's not the expected case, seems fine.
| "github.com/letsencrypt/boulder/observer/probers" | ||
| "github.com/letsencrypt/boulder/strictyaml" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
||
| "github.com/letsencrypt/boulder/crl/checker" | ||
| "github.com/letsencrypt/boulder/crl/idp" |
There was a problem hiding this comment.
Order third-party packages in a separate cluster, above other boulder packages.
| "github.com/letsencrypt/boulder/observer/probers" | |
| "github.com/letsencrypt/boulder/strictyaml" | |
| "github.com/prometheus/client_golang/prometheus" | |
| "github.com/letsencrypt/boulder/crl/checker" | |
| "github.com/letsencrypt/boulder/crl/idp" | |
| "github.com/prometheus/client_golang/prometheus" | |
| "github.com/letsencrypt/boulder/crl/checker" | |
| "github.com/letsencrypt/boulder/crl/idp" | |
| "github.com/letsencrypt/boulder/observer/probers" | |
| "github.com/letsencrypt/boulder/strictyaml" |
There was a problem hiding this comment.
Will do, but is this automatable? Is there a configuration of goimports or golangci-lint I may be missing?
There was a problem hiding this comment.
In vscode, it's
"gopls": {
"formatting.local": "github.com/letsencrypt/",
},I'm not sure what that translates to in other individual tools.
| // Because this prober fetches URLs controlled by external input (CCADB), we | ||
| // check this regexp to avoid arbitrary content fetching (SSRF). |
There was a problem hiding this comment.
Personally, I'd keep this comment rather than deleting it entirely. Or I'd move it down to line 83 and say something like "While the primary purpose of the regexp is to parse out the CRL Number, we specify our own c.lencr.org hostname to ensure that CCADB doesn't cause us to fetch arbitrary external data (SSRF)."
|
|
||
| if !c.crlRegexp.MatchString(url) { | ||
| matches := c.crlRegexp.FindAllStringSubmatch(url, 2) | ||
| if matches == nil { |
There was a problem hiding this comment.
If we (or anyone else running this code) currently configure a non-default CRLRegexp, then this code is not backwards-compatible. Whatever regexp is configured almost certainly doesn't have any capture groups in it, which means that the prober will bail out at this error check.
There was a problem hiding this comment.
Indeed it is not backwards compatible. This is not a problem in LE's configuration because we use the default.
3 participants
Closes #8876