crt-sh: scope to owned zones, score on DNS liveness - #142
Conversation
crt.sh returns every certificate ever logged for a query, which is a
decade of history presented as if it were live infrastructure. The plugin
emitted all of it unscored, so the graph filled with hostnames that no
longer exist and the source became untrustworthy.
Measured against archived output for three tenants, the dominant noise
sources were not the multi-tenant certificate case the old TODO described
(that is real but ~1.6% of names). They were, in order: names whose DNS is
long gone, the queried domain itself being re-emitted as a discovery
(5,477 times on one tenant), and high-entropy ACME validation hostnames.
Changes:
- Never re-emit the queried domain.
- Recover wildcard parents ("*.foo.example.com" -> "foo.example.com")
instead of discarding them; the parent is a zone that had a
certificate issued for it.
- Score each finding with three independent signals rather than one
flat value: observed on a logged certificate (0.30), sits in a zone
the caller owns (0.30), has live DNS presence (0.20).
- Add scope.go: OwnedZones/InOwnedZone/DropWildcard, keyed on the public
suffix list so "a.example.co.uk" and "example.co.uk" compare equal
while "example.co.uk" and "other.co.uk" do not. The other subdomain
plugins have the same scoping gap and can adopt it directly.
- Log crt.sh transport and decode failures. crt.sh 502s frequently and
the silent (nil, nil) return made an outage indistinguishable from
"this domain has no subdomains".
Two findings drove the design and are documented at their call sites:
Scoping to the *queried* domain's zone would have been wrong. 92% of the
names it would have dropped for one tenant sit on other brands the same
tenant owns; a certificate for one brand also covering another is the most
valuable thing this source produces. Scope is therefore taken from the
caller's owned-domain set via Input.Meta["owned_domains"], falling back to
the queried zone when the caller has no inventory.
A dangling CNAME has no A/AAAA record. Treating "no address" as dead put
subdomain-takeover candidates into the needs-review band, where an
asset-only consumer never sees them. Liveness now means address *or*
published CNAME, via an optional CNAMEResolver interface that crt-sh
type-asserts; Resolver itself is unchanged, since dns-brute and the
reverse-ip caller depend on its shape.
The entropy heuristic no longer drops on its own: it cannot distinguish a
random ACME token from a long English compound, so it applies only to
names that also failed DNS.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe CRT.sh plugin now accepts an injected DNS resolver and supports optional CNAME detection. It normalizes and deduplicates certificate names, removes seeds, invalid wildcards, out-of-band names, and dead high-entropy names. It applies owned-zone checks and bounded concurrent DNS resolution. Findings include certificate expiry metadata and separate confidence contributions for certificate observation, ownership, and live DNS evidence. Tests cover these behaviors and failure paths. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/plugins/domains/crt_sh_run_test.go`:
- Around line 422-425: Replace t.Fatalf with t.Errorf in the fakeResolver
callback used by the resolveAll cancellation test, keeping the existing failure
message and return behavior so the resolver goroutine reports the unexpected
call without terminating itself.
In `@pkg/plugins/domains/crt_sh.go`:
- Around line 143-162: Update the Run flow around resolveAll to check ctx.Err()
immediately after DNS resolution and before dropDeadJunk or scoring; when
cancellation is detected, return the context error explicitly, otherwise
preserve the existing filtering and scoring behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b253cdfa-dfeb-45a1-a6e4-029d12ecc6e2
📒 Files selected for processing (6)
pkg/plugins/domains/crt_sh.gopkg/plugins/domains/crt_sh_run_test.gopkg/plugins/domains/dns_brute.gopkg/plugins/domains/dns_helpers.gopkg/plugins/domains/scope.gopkg/plugins/domains/scope_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4634f462b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| name, d.CNAME)) | ||
| } | ||
|
|
||
| findings = append(findings, finding) |
There was a problem hiding this comment.
When a crt.sh name is outside the owned zones and has no live DNS, the only evidence added is confCRTShObservation (0.30), which is below plugins.ConfidenceLow (0.35). This unconditional append still returns those findings; checked pkg/lib/invoke.go and pkg/runner/run.go, and neither applies the low-confidence discard, so historical unrelated crt.sh names are printed/emitted and can flow into phase-3 enrichment instead of being dropped. Please skip findings whose TotalConfidence is below ConfidenceLow before appending them.
Useful? React with 👍 / 👎.
Return an error on cancelled resolution instead of degrading silently. resolveAll yields a zero-value dnsPresence for every name it did not reach, which downstream is indistinguishable from "no DNS record": the entropy pass would delete unreached names as dead, and every survivor would silently lose a confidence band. A fetch failure yields no data and returning nothing is honest; a cancelled resolution yields data that is wrong in a way the caller cannot detect, so Run now returns ctx.Err(). Discard findings below plugins.ConfidenceLow. Only one band reaches it — outside every owned zone and with no DNS presence at all, scoring the bare 0.30 observation — and nothing downstream enforces the floor, so those names were reaching output and could flow into later enrichment. This follows the in-plugin precedent in github_org.go. The "de-rank, never drop" pattern governs the review band, not the noise floor. Do not call t.Fatalf from a resolver goroutine: it calls runtime.Goexit on the worker rather than failing the test, so the report is not guaranteed. Both changes were previously unguarded; adds a cancellation test asserting no partial findings survive, and a floor test isolated from the four-band test so a regression in the comparison direction is caught on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Problem
crt.sh returns every certificate ever logged for a query — a decade of history presented as if it were live infrastructure. The plugin emitted all of it unscored, so the graph filled with hostnames that no longer exist and the source became untrustworthy.
Measured against archived shadow output for three tenants, the dominant noise was not the multi-tenant-certificate case the old TODO described (real, but ~1.6% of names). In order of impact:
Changes
*.foo.example.com→foo.example.com) instead of discarding them; the parent is a zone that had a certificate issued for it.scope.go—OwnedZones/InOwnedZone/DropWildcard, keyed on the public suffix list soa.example.co.ukandexample.co.ukcompare equal whileexample.co.ukandother.co.ukdo not.urlscan,passivednsandwaybackhave the same scoping gap and can adopt it directly.(nil, nil)return made an outage indistinguishable from "this domain has no subdomains".Two findings that drove the design
Scoping to the queried domain's zone would have been wrong. 92% of the names it would have dropped for one tenant sit on other brands the same tenant owns. A certificate for one brand also covering another is the most valuable thing this source produces. Scope is therefore taken from the caller's owned-domain set via
Input.Meta["owned_domains"], falling back to the queried zone when the caller has no inventory.A dangling CNAME has no A/AAAA record. Treating "no address" as dead pushes subdomain-takeover candidates into the needs-review band, where an asset-only consumer never sees them. Liveness now means address or published CNAME, via an optional
CNAMEResolverinterface that crt-sh type-asserts.Resolveritself is unchanged, sincedns-bruteand the reverse-ip caller depend on its shape.Result
Operator-facing asset counts:
Nothing real is lost — in-scope-but-dead names land in the needs-review band as leads rather than being discarded.
Testing
go build,go vetand the full suite pass.pkg/plugins/domainsruns 442 tests in ~3s with no network access; every test injects a resolver, so none reach real DNS.Follow-up
Guard-side work is tracked in ENG-5929 and is blocked on this merging and tagging: the new constructor is two-arg and exists in no released version. That PR also has to fix
convert.go:47, where an upstream rename (plugins.Confidence→TotalConfidence) breaks Guard on any pius bump — pre-existing and unrelated to this change.🤖 Generated with Claude Code