fix(network): harden SSRF fetch validation and apply default connect timeout#108
fix(network): harden SSRF fetch validation and apply default connect timeout#108danielewood merged 8 commits intodevelopfrom
Conversation
![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: 4398eeb9bc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if network.Contains(ip) { | ||
| return fmt.Errorf("blocked private address %s", host) | ||
|
|
||
| resolveCtx, cancel := context.WithTimeout(context.Background(), aiaURLResolveTimeout) |
There was a problem hiding this comment.
ValidateAIAURLWithOptions builds its resolver context from context.Background(), so AIA/OCSP/CRL URL validation can continue for up to the fixed 2s timeout even after the caller’s context has been canceled or its deadline has expired. In practice this breaks timeout guarantees for flows that rely on short contexts (for example ConnectTLS with ConnectTimeout or revocation checks under tight deadlines), because each hostname validation can overrun the caller’s budget instead of failing promptly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in fe2a1e9. ValidateAIAURLWithOptions now accepts ctx context.Context and derives its DNS resolver timeout from that caller context (context.WithTimeout(ctx, aiaURLResolveTimeout)), and all AIA/OCSP/CRL call sites now pass request/caller contexts through (including redirect checks via req.Context()). I also added TestValidateAIAURLWithOptions_ContextDeadline to verify deadline propagation.
| // httpAIAFetcher fetches raw certificate bytes from a URL via HTTP. | ||
| func httpAIAFetcher(ctx context.Context, rawURL string) ([]byte, error) { | ||
| if err := certkit.ValidateAIAURL(rawURL); err != nil { | ||
| if err := certkit.ValidateAIAURLWithOptions(certkit.ValidateAIAURLInput{URL: rawURL, AllowPrivateNetworks: scanAllowPrivateNetwork}); err != nil { |
There was a problem hiding this comment.
This fetcher now enforces private-network blocking via scanAllowPrivateNetwork, but it is also reused by inspect (cmd/certkit/inspect.go) where no --allow-private-network flag is exposed and ResolveInspectAIA does not pass AllowPrivateNetworks. That means inspect’s AIA resolution now always rejects internal/private endpoints, so internal PKI chains that previously resolved intermediates will silently degrade to unresolved/untrusted output with no opt-in path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in fe2a1e9. inspect now has an explicit --allow-private-network flag, and inspect AIA resolution now passes that opt-in through its own fetch path instead of inheriting scan globals. This restores an opt-in path for internal/private PKI AIA endpoints during inspect flows.
There was a problem hiding this comment.
Pull request overview
This PR strengthens certkit’s network-fetch safety by tightening SSRF validation for AIA/OCSP/CRL URLs (including redirect targets) and ensuring ConnectTLS won’t hang indefinitely when called with a no-deadline context. It also adds explicit CLI/library opt-ins for private/internal PKI endpoints and updates tests and documentation accordingly.
Changes:
- Add DNS-resolution-based SSRF validation with an explicit
AllowPrivateNetworksopt-in threaded through AIA/OCSP/CRL flows. - Apply a default connect/handshake timeout in
ConnectTLSwhen the provided context has no deadline (configurable viaConnectTimeout). - Update tests plus README/CHANGELOG to reflect new behavior and flags.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
bundle.go |
Introduces ValidateAIAURLWithOptions with DNS resolution + private-network blocking; threads allow-private option into AIA fetching. |
bundle_test.go |
Updates AIA fetch tests to opt into private network behavior for localhost-based integration cases. |
certkit_test.go |
Expands SSRF validation tests and adds resolver-injection coverage for hostname resolution logic. |
connect.go |
Adds ConnectTimeout/default timeout behavior and threads AllowPrivateNetworks into AIA/OCSP/CRL checks during connection probing. |
connect_test.go |
Adds coverage for timeout behavior when callers pass a no-deadline context; updates existing tests to pass allow-private where needed. |
crl.go |
Switches CRL URL checks to the new SSRF validation options (including redirects) and documents updated behavior. |
crl_test.go |
Adds loopback/localhost SSRF cases and threads allow-private into tests that intentionally use local servers. |
ocsp.go |
Adds AllowPrivateNetworks to OCSP checking and validates redirect targets using the options-based validator. |
ocsp_test.go |
Updates OCSP tests to opt into private networks for localhost responders; adds a default-blocking test. |
internal/certstore/aia.go |
Threads allow-private behavior into AIA resolution URL validation. |
internal/certstore/aia_test.go |
Updates AIA resolution tests to opt into private networks for local fetchers. |
internal/verify.go |
Threads allow-private behavior through verify → bundle/OCSP/CRL paths. |
internal/verify_test.go |
Updates verification tests to opt into private networks for localhost-based revocation fixtures. |
cmd/certkit/connect.go |
Adds --allow-private-network flag and threads it into ConnectTLS. |
cmd/certkit/verify.go |
Adds --allow-private-network flag and threads it into verification input. |
cmd/certkit/ocsp.go |
Adds --allow-private-network flag and threads it into OCSP checks. |
cmd/certkit/scan.go |
Adds --allow-private-network flag and threads it into AIA fetching + redirect validation. |
README.md |
Updates CLI flag tables to document the new --allow-private-network options. |
CHANGELOG.md |
Adds Unreleased entries for SSRF hardening and default connect timeout behavior. |
Comments suppressed due to low confidence (6)
connect.go:175
- ConnectTimeout was added in the middle of the exported ConnectTLSInput struct (between Port and ServerName). This is a source-compatible but not binary-/compile-compatible change for any callers using unkeyed composite literals (positional struct initialization). To reduce the blast radius of this API change, consider appending new exported fields to the end of the struct (or document the breaking change explicitly if that’s intended).
type ConnectTLSInput struct {
// Host is the hostname or IP to connect to.
Host string
// Port is the TCP port (default: "443").
Port string
// ConnectTimeout is used when ctx has no deadline (default: 10s).
ConnectTimeout time.Duration
// ServerName overrides the SNI hostname (defaults to Host).
ServerName string
connect.go:370
- ConnectTLS now passes connectCtx (the derived timeout context) into result.populate. This makes the connect timeout also cap AIA/OCSP/CRL work performed after the handshake, which can cause unexpected cancellations when callers set a short ConnectTimeout just to bound dial/handshake time. Consider using connectCtx only for Dial/Handshake and then calling populate with the original ctx (or introducing a separate overall timeout field if the intent is to bound the full operation).
result.populate(connectCtx, input)
result.Diagnostics = append(result.Diagnostics, ChainDiagnostic{
Check: "legacy-only",
connect.go:401
- ConnectTLS now calls result.populate with connectCtx, so the derived ConnectTimeout (when ctx has no deadline) also limits post-handshake checks (AIA/OCSP/CRL/CT). If the goal is only to prevent indefinite hangs during TCP/TLS connect + handshake, it’s safer to use connectCtx for dial/handshake only and run populate under the original ctx (which already has per-feature timeouts).
result.populate(connectCtx, input)
return result, nil
connect_test.go:628
- This test sets ConnectTimeout explicitly (50ms), so it doesn’t actually validate the new default (10s) timeout path implied by the test name/comment. Consider renaming the test (or adjusting it) so it clearly covers the intended behavior (e.g., “uses ConnectTimeout when ctx has no deadline”, and a separate test for the 10s default if feasible).
func TestConnectTLS_DefaultTimeoutWhenContextHasNoDeadline(t *testing.T) {
// WHY: ConnectTLS must apply a safe timeout when callers pass context.Background()
// so stalled handshakes do not block indefinitely.
t.Parallel()
listener, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() { _ = listener.Close() })
go func() {
for {
conn, acceptErr := listener.Accept()
if acceptErr != nil {
return
}
// Hold the socket open without speaking TLS until the client times out.
time.Sleep(250 * time.Millisecond)
_ = conn.Close()
}
}()
_, portStr, err := net.SplitHostPort(listener.Addr().String())
if err != nil {
t.Fatal(err)
}
start := time.Now()
_, err = ConnectTLS(context.Background(), ConnectTLSInput{
Host: "127.0.0.1",
Port: portStr,
ConnectTimeout: 50 * time.Millisecond,
})
CHANGELOG.md:90
- The changelog entry says this “block[s] DNS-rebind SSRF”. The current implementation validates hostnames via a separate preflight DNS lookup, but net/http will resolve again when dialing, so DNS rebinding between validation and connect is still possible. Consider softening the wording (e.g., “harden” / “mitigate”) or implementing dial-time enforcement to match the claim.
- Block DNS-rebind SSRF in AIA/OCSP/CRL fetches by validating DNS-resolved hostnames against private/internal address ranges by default, and add explicit `--allow-private-network` opt-in flags for internal PKI endpoints in `connect`, `verify`, `ocsp`, and `scan` ([#98])
certkit_test.go:1813
- This table test switches behavior based on
tt.name == "allow private network option", which makes it easy to break the test accidentally during renames. Consider adding an explicit per-case field (e.g., allowPrivate bool / useOptions bool) instead of branching on the name string.
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
var err error
if tt.name == "allow private network option" {
err = ValidateAIAURLWithOptions(ValidateAIAURLInput{URL: tt.url, AllowPrivateNetworks: true})
} else {
err = ValidateAIAURL(tt.url)
}
| resolveCtx, cancel := context.WithTimeout(context.Background(), aiaURLResolveTimeout) | ||
| defer cancel() | ||
|
|
||
| ips, err := lookup(resolveCtx, host) | ||
| if err != nil { | ||
| return fmt.Errorf("resolving host %q: %w", host, err) | ||
| } | ||
| if len(ips) == 0 { | ||
| return fmt.Errorf("resolving host %q: no IP addresses returned", host) | ||
| } | ||
| for _, resolvedIP := range ips { | ||
| if blockedErr := ipBlockedForAIA(resolvedIP); blockedErr != nil { | ||
| return fmt.Errorf("host %q resolved to %s: %w", host, resolvedIP.String(), blockedErr) | ||
| } |
There was a problem hiding this comment.
ValidateAIAURLWithOptions does a standalone DNS lookup to validate hostnames, but the subsequent net/http request will perform its own DNS resolution when dialing. Because these are two separate lookups, a DNS-rebind TOCTOU window still exists (validation can see a public IP while the actual connection resolves to a private IP). For stronger SSRF/DNS-rebind protection, consider enforcing the IP-range check at dial time (e.g., a custom http.Transport/DialContext that resolves + validates + dials the chosen IP(s) directly).
There was a problem hiding this comment.
Agreed on the TOCTOU limitation. I did not add dial-time IP pinning in this PR (that would require transport-level changes), but I updated wording to avoid overstating guarantees: changelog text now says we harden SSRF checks rather than block DNS-rebind SSRF.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Addressed reviewer feedback in fe2a1e9:
All pre-commit checks pass locally ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe2a1e91ae
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Follow-up: all review feedback has been addressed and fixes pushed. Please take another look when you can. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0073502000
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| seen[aiaURL] = true | ||
|
|
||
| if err := certkit.ValidateAIAURL(aiaURL); err != nil { | ||
| if err := certkit.ValidateAIAURLWithOptions(ctx, certkit.ValidateAIAURLInput{URL: aiaURL, AllowPrivateNetworks: input.AllowPrivateNetworks}); err != nil { |
There was a problem hiding this comment.
ResolveAIA now enforces ValidateAIAURLWithOptions with AllowPrivateNetworks defaulting to false, but the WASM inspect flow still invokes ResolveInspectAIA without any way to set this option (checked cmd/wasm/inspect.go and the certkitInspect(...) call in web/public/app.js). As a result, internal PKI chains whose AIA hostnames resolve to private/internal IPs now fail to auto-resolve in the web inspector with no opt-in path, which is a functional regression for those environments.
Useful? React with 👍 / 👎.
|
Completed another review-feedback sweep and addressed the new actionable items from the latest round (post-0073502):
Validation:
Pushed commit: |
|
Follow-up (clean formatting): completed another review-feedback sweep and addressed the new actionable items since commit 0073502.
Validation:
Pushed commit: 7b2f4c7 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b2f4c731c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type lookupIPAddressesFunc func(ctx context.Context, host string) ([]net.IP, error) | ||
|
|
||
| func defaultLookupIPAddresses(ctx context.Context, host string) ([]net.IP, error) { | ||
| return net.DefaultResolver.LookupIP(ctx, "ip", host) |
There was a problem hiding this comment.
This DNS lookup path breaks AIA validation in the web inspector: cmd/wasm/inspect.go calls ResolveInspectAIA with AllowPrivateNetworks defaulting to false, and internal/certstore.ResolveAIA now routes every hostname through ValidateAIAURLWithOptions, which reaches net.DefaultResolver.LookupIP here. In GOOS=js/wasm, the Go resolver uses the fake net stack and cannot reliably resolve external hosts, so public AIA/OCSP/CRL hostnames get rejected as resolving host ... and intermediate auto-resolution silently regresses unless users disable protections via allowPrivateNetwork.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in df297ea. I moved DNS lookup helpers behind build tags and skip hostname DNS pre-resolution in GOOS=js builds, so WASM no longer fails AIA/OCSP/CRL URL validation on hostnames due resolver limitations. Literal IP SSRF checks still run, and browser scan flow now has an explicit private-network opt-in wired through certkitAddFiles(..., allowPrivateNetwork?).
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Completed another feedback sweep and addressed new actionable items from the latest commit window.
Validation: Pushed: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f59ac82617
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !aiaDNSResolutionAvailable() { | ||
| return nil |
There was a problem hiding this comment.
In ValidateAIAURLWithOptions, the !aiaDNSResolutionAvailable() branch returns success for any hostname, and the js build sets DNS resolution as unavailable. In GOOS=js (WASM), this means default private-network blocking is bypassed for hostname URLs like http://localhost/... (only literal IPs are checked), so AIA/OCSP/CRL fetches can still reach private/internal targets even when AllowPrivateNetworks is false and the UI/CLI opt-in is not enabled.
Useful? React with 👍 / 👎.
|
CS-5 violation on |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
* fix(wasm): harden ingestion and require explicit unverified export (#105) * fix(wasm): harden ingest and make unverified export explicit * fix: address PR 105 reliability review feedback * fix(web): enforce upstream timeout through body reads * fix: address latest PR 105 review feedback * fix(crl): clarify read errors and harden size-limit coverage * fix: harden core parsing and identity selection (#107) * fix: harden parsing and issuer/key selection correctness * docs(changelog): reference PR for unreleased parsing fixes * fix: prevent parser fallback and JKS identity regressions * fix(inspect): preserve valid keys when PEM bundle has malformed blocks * fix: address remaining PR 107 review feedback * fix(jks): surface skipped-entry reasons in debug logs * fix(scan): harden scan boundaries and export text summary (#106) * fix(scan): keep traversal bounded and restore export summaries * fix(scan): fail fast on walker processing errors * fix(scan): use typed max-size errors in read paths * fix(scan): reject invalid export formats consistently * fix(scan): keep export destination off stdout * fix(network): harden SSRF fetch validation and apply default connect timeout (#108) * fix(network): harden revocation fetch SSRF checks and connect timeout defaults * fix(network): propagate SSRF validation deadlines and unblock inspect AIA opt-in * fix(bundle): restore private-network opt-in for AIA chain fetches * fix(network): address remaining PR feedback for inspect AIA handling * fix(wasm): keep AIA resolution working without DNS lookups * fix(cli): normalize validation exits, JSON schema, and export defaults (#109) * fix(cli): align validation exits and secure export defaults * fix(cli): address PR #109 review feedback * fix(tests): align PR #109 follow-up feedback * fix(security): remediate current code scanning alerts * build(hooks): consume shared develop branch-name exemption * fix: address PR 110 review feedback
Summary
--allow-private-networkopt-in for internal PKI fetches inconnect,verify,ocsp, andscan, and thread allow controls through library input structsConnectTLSwhen callers provide a context without a deadline, and add coverage for timeout fallback plus new SSRF validation behaviorValidation
Closes #98
Closes #99