feat(prober): add CRL certificate revocation checking - #1583
feat(prober): add CRL certificate revocation checking#1583sebastiangaiser wants to merge 1 commit into
Conversation
7c6af3a to
707e308
Compare
709b3cc to
507f478
Compare
30f58b5 to
84d19c5
Compare
There was a problem hiding this comment.
Pull request overview
Adds opt-in CRL certificate revocation checks and metrics for TLS-enabled probes.
Changes:
- Fetches and validates CRLs across certificate chains.
- Exposes CRL availability, revocation, staleness, timing, and publication metrics.
- Adds configuration and tests for CRL behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
config/config.go |
Adds CRL configuration types and probe fields. |
config/testdata/blackbox-good.yml |
Adds valid HTTP CRL configuration. |
prober/crl.go |
Implements CRL fetching, validation, and metrics. |
prober/crl_test.go |
Tests CRL processing and metrics. |
prober/dns.go |
Adapts DNS TLS configuration wrapper. |
prober/grpc.go |
Integrates CRL checks into gRPC probes. |
prober/grpc_test.go |
Updates gRPC TLS test configuration. |
prober/handler_test.go |
Updates handler TLS test configuration. |
prober/http.go |
Integrates CRL checks into HTTP probes. |
prober/http_test.go |
Tests unavailable CRL reporting. |
prober/query_response.go |
Integrates CRL checks for TCP and Unix TLS. |
prober/tcp.go |
Adapts TCP TLS configuration. |
prober/tcp_test.go |
Updates TCP TLS test configuration. |
prober/unix.go |
Adapts Unix TLS configuration. |
Suppressed comments (2)
prober/crl.go:174
- The timer stops as soon as response headers arrive. A slow or large CRL body can consume most of the probe timeout while
probe_ssl_crl_fetch_time_secondsreports only time-to-first-byte; measure throughReadAllso the metric represents the fetch.
start := time.Now()
resp, err := client.Do(req)
fetchTime := time.Since(start).Seconds()
if err != nil {
config/config.go:315
- Without a JSON tag, non-HTTP TLS configurations serialize this property as
CRLCheck, while the YAML and all neighboring JSON config fields use snake_case. Give both configuration formats the same public key.
CRLCheck CRLCheckConfig `yaml:"crl_check,omitempty"`
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6859675 to
0e0b293
Compare
742ecbe to
4533283
Compare
|
@electron0zero thanks for the review. I've reworked the PR along your suggestions:
I also addressed the Copilot findings: |
| tls_config: | ||
| [ <tls_config> ] | ||
|
|
||
| # Check each certificate in the TLS chain against its CRL and emit probe_ssl_crl_* metrics. |
There was a problem hiding this comment.
thinking about this, one thing come to my mind. When someone sets check_revoked to true, do they just expect the metrics or they also want the probe to fail when the Cert is revoked?
What is check_revoked meant to promise? A cert confirmed on its issuer's CRL still gives probe_success=1, but the config name reads like enforcement.
we have flags like fail_if_ssl / fail_if_not_ssl / fail_if_body_matches_regexp for enforcement and failing the probe.
if your use-case and intent is to fail the probe, then we should add support for a fail_if_cert_revoked flag as well, and when that's set, we should fail the probe like we do for the fail_if_ssl and many other flags.
if you just want the metrics, then current setup is fine as is, and we can keep publishing the metrics and probe_success is left as is.
There was a problem hiding this comment.
Our use case is alerting on the metrics, with the revocation signal deliberately kept separate from probe_success:
probe_ssl_crl_revoked == 1-> critical after 5mprobe_ssl_crl_available == 0(restricted tocrl_url!="") -> warning after 15mprobe_ssl_crl_stale == 1-> warning after 1h
probe_success then keeps its own meaning: the target is reachable and answered as expected. That separation is the useful part. A CRL distribution point being down is not the same outage as the service being down, and the two want different severities and different runbooks. If check_revoked also failed the probe, every dead or slow CRL responder would look like a total outage and we'd lose the ability to grade "revoked" (act now) differently from "revocation status unknown" (look into it).
For the same reason we alert on the per-phase probe_http_duration_seconds rather than the total probe_duration_seconds for latency, since the total includes the CRL fetch and a slow responder would otherwise inflate it.
You're right that the name alone doesn't convey that, so I've spelled the semantics out in CONFIGURATION.md for all four probes: the flag only reports revocation state and never fails the probe, a revoked certificate or an unreachable CRL leaves probe_success untouched, and alerting is expected to happen on the metrics. If you'd rather have that stated somewhere more prominent than the per-probe options, I'm happy to add a short section on the CRL metrics and how to alert on them.
And if hard enforcement turns out to be wanted later, a separate fail_if_cert_revoked can be added on top without changing this behaviour.
There was a problem hiding this comment.
makes sense, I am good with current setup and we can always add fail_if_cert_revoked flag in the future.
4a4fad8 to
ae9cf0b
Compare
|
Thanks for the detailed second pass @electron0zero. I've pushed changes for all of it.
Three things where I deviated from the review, all for reasons worth flagging: Proxy config only exists on the HTTP probe: Only TCP and Unix accept The new certificate helper doesn't reuse |
Check the full certificate chain against CRLs across the HTTP, TCP, gRPC, and Unix probes. For each cert with CRLDistributionPoints, fetch the CRL, verify its signature against the issuer, reject CRLs that are stale or not yet valid, and look up the cert serial in the revocation list. Enabled per probe via the opt-in `check_revoked` flag (default false); the CRL fetch is bounded by the probe timeout, no separate knob. DNS over TLS is not covered yet. Signed-off-by: Sebastian Gaiser <sebastiangaiser@users.noreply.github.com>
ae9cf0b to
3cb5cec
Compare
electron0zero
left a comment
There was a problem hiding this comment.
lgtm, thanks for the PR ![]()
Check the full certificate chain against CRLs across the HTTP, TCP, gRPC, and Unix probes. For each cert with CRLDistributionPoints, fetch the CRL, verify its signature against the issuer, reject CRLs that are stale or not yet valid, and look up the cert serial in the revocation list. Enabled per probe via the opt-in
check_revokedflag (default false); the CRL fetch is bounded by the probe timeout, no separate knob. DNS over TLS is not covered yet.What this PR does / Which issue(s) does the PR fix:
Does this PR introduce a user-facing change?
Checklist
release-notessection of PR Desc.