fix(scheduledauth): use hardened HTTP client for JWKS warmup and fetch by cristim · Pull Request #1231 · LeanerCloud/CUDly

cristim · 2026-06-11T08:02:51Z

Problem

SEC-04 (docs/reviews/codebase-review-2026-06-10.md): the scheduled-task OIDC validator's Warmup probed the JWKS endpoint via http.DefaultClient, and oidc.NewRemoteKeySet fetched keys over go-oidc's default transport. The JWKS URL is operator-supplied (SCHEDULED_TASK_OIDC_JWKS_URL, validated only as an absolute URL), so a misconfigured or compromised value could reach internal/metadata endpoints, bypassing the IMDS-blocking hardening already mandatory for Azure provider clients.

Fix

New shared pkg/httpclient package hosting the hardened client (IMDS blocking for IPv4 169.254.169.254 and IPv6 fd00:ec2::254, dial/TLS/overall timeouts as named constants). The previous home, providers/azure/internal/httpclient, is unimportable from the root module across the module boundary; it now delegates to the shared package so there is a single implementation and the existing httpclient.New() convention in providers/azure/ is unchanged.
internal/server/scheduledauth: configureOIDC constructs the hardened client once, Warmup uses it for the probe, and it is supplied to go-oidc via oidc.ClientContext so key fetches and rotation refreshes ride the same hardened transport. No caller-supplied client exists on this path, so nothing is clobbered.

Test evidence

New regression tests (confirmed failing pre-fix by stashing the validator change):
- TestWarmup_BypassesDefaultClient and TestValidate_OIDC_KeyFetchBypassesDefaultClient install a guarded http.DefaultClient that fails the test if any JWKS traffic flows through it; pre-fix both fail (Warmup made 1 request(s) through http.DefaultClient, key fetch rejected), post-fix both pass with the JWKS test server actually reached.
- TestNew_OIDC_UsesHardenedHTTPClient asserts the constructed client is not http.DefaultClient, has a timeout, and does not ride http.DefaultTransport.
- pkg/httpclient tests: not-default-client guard, IMDS blocking (IPv4 + IPv6), and a non-IMDS endpoint still allowed.
go build ./... green in root, pkg/, and providers/azure/ modules; go vet clean on touched packages.
go test ./internal/server/scheduledauth/... (54 passed), pkg: go test ./... (448 passed), providers/azure: go test ./... (696 passed).

Closes #1145

coderabbitai · 2026-06-11T08:03:06Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 590d3baa-3be9-4810-8cd6-6725fec6bb43

📥 Commits

Reviewing files that changed from the base of the PR and between 27fdb06 and 772a66c.

📒 Files selected for processing (5)

internal/server/scheduledauth/validator.go
internal/server/scheduledauth/validator_test.go
pkg/httpclient/httpclient.go
pkg/httpclient/httpclient_test.go
providers/azure/internal/httpclient/httpclient.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/sec-04-fix

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-06-11T08:03:08Z

@coderabbitai review

coderabbitai · 2026-06-11T08:03:15Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-19T22:08:31Z

@coderabbitai review

coderabbitai · 2026-06-19T22:08:37Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-26T16:49:15Z

Adversarial review (PR #1231)

Reviewed against the six risk surfaces from the brief (IMDS/SSRF blocking, JWKS URL validation, ctx-aware warmup, fail-closed semantics, refresh path uses hardened transport, PII/key material in logs). One canonical-vs-textual IMDS bypass found and fixed in this push; three follow-ups filed for the larger SSRF / scheme-enforcement / fail-closed-warmup work that was deliberately out of scope for SEC-04.

Verified

oidc.ClientContext covers both initial fetch and rotation refresh. configureOIDC passes the hardened client to oidc.NewRemoteKeySet(oidc.ClientContext(ctx, v.httpClient), cfg.JWKSURL); go-oidc stores the client off the supplied ctx and uses it for both the first fetch and refresh-on-unknown-kid. The two new tests TestWarmup_BypassesDefaultClient and TestValidate_OIDC_KeyFetchBypassesDefaultClient both pin this by installing a guarded http.DefaultClient that fails the test if used and verifying the JWKS test server actually serves the request. Both fail pre-fix (validator was using http.DefaultClient / go-oidc's default transport), pass post-fix.
TestNew_OIDC_UsesHardenedHTTPClient asserts the constructed client is not http.DefaultClient, has a non-zero timeout, and does not ride http.DefaultTransport. Locks the three independent properties separately.
No caller-supplied *http.Client on the JWKS path (feedback_preserve_caller_httpclient.md): configureOIDC builds the hardened client itself; the brief's concern about clobbering a base client's Transport does not apply here.
Warmup is ctx-aware (feedback_ctx_aware_patterns.md, feedback_ctx_cancel_terminal.md): http.NewRequestWithContext(ctx, ...) plus a 5s fallback deadline when the caller's ctx has none. No retry loop, no sleep, single-shot probe; ctx cancellation aborts the in-flight request via the transport.
No key material or tokens in logs (feedback_pii_in_logs.md): warmup logs the operator-supplied JWKS URL on failure (config, not PII); validateOIDC logs sub and aud on success (service-identity, not PII; no JWT body, no signature, no keys).
SDK/typed enums + no hardcoded magic values (feedback_no_hardcoded_magic_values.md, feedback_sdk_enum_string_literals.md): timeouts named as constants (dialTimeout, keepAliveInterval, tlsHandshakeTimeout, requestTimeout); the signing-alg allowlist uses oidc.RS256, not a string literal.
feedback_azure_use_httpclient_new.md: Azure's providers/azure/internal/httpclient.New() now delegates to pkg/httpclient.New() (single implementation, same hardening). Azure module uses a local replace to ../../pkg, so the canonical-form fix below propagates there transitively.

Fix pushed (`a8c5315`)

pkg/httpclient: block IMDS by net.IP.Equal, catch the IPv6 expanded form. The pre-PR Azure-internal imdsAddresses was a literal-string map[string]bool{"169.254.169.254": true, "fd00:ec2::254": true} and the move to pkg/httpclient carried it verbatim. The lookup imdsAddresses[host] uses the host string as it came off net.SplitHostPort, which is whatever the caller wrote in the URL. The AWS IPv6 IMDS endpoint has two equally-valid textual encodings: the canonical short form fd00:ec2::254 (in the map) and the fully-expanded form fd00:ec2:0:0:0:0:0:254 (not in the map). A JWKS URL http://[fd00:ec2:0:0:0:0:0:254]/... splits to host fd00:ec2:0:0:0:0:0:254, misses the map lookup, and reaches the metadata service.

Switched to net.ParseIP + net.IP.Equal so the comparison is on the parsed address. Added an explicit TestNew_BlocksIMDS/ipv6_AWS_IMDS_expanded_form subtest; confirmed it fails on the pre-fix shape and passes after (go test -run TestNew_BlocksIMDS: 2 passed, 2 failed pre-fix → 4 passed post-fix).

Test suite delta: internal/server/scheduledauth 54/54 still green, pkg 449/449, providers/azure 696/696 (Azure picks up the fix via local replace).

Out-of-scope, filed as follow-ups

scheduledauth: enforce HTTPS scheme on SCHEDULED_TASK_OIDC_JWKS_URL #1311 — scheduledauth: enforce HTTPS scheme on SCHEDULED_TASK_OIDC_JWKS_URL (p2/security/medium). validateAbsoluteURL accepts http:// today; a misconfigured cleartext JWKS URL is MITM-bait for every inbound JWT in oidc mode.
pkg/httpclient: extend SSRF block-list (loopback, RFC 1918, all link-local, hostname/DNS resolution) #1312 — pkg/httpclient: extend SSRF block-list (loopback, RFC 1918, all link-local, hostname/DNS resolution) (p1/security/high). The block-list currently only covers two specific IMDS IPs; loopback / private / sibling link-local / hostname-based metadata.google.internal all reach. Sketches the pre-resolve-then-dial pattern to close the DNS-rebinding window.
scheduledauth: decide JWKS warmup fail-open vs fail-closed (currently fail-open on misconfig) #1314 — scheduledauth: decide JWKS warmup fail-open vs fail-closed (currently fail-open on misconfig) (p2/security/low). The PR documents the fail-open choice explicitly (Google CDN hiccups); flagged for team decision against the global no-silent-fallbacks rule (feedback_no_silent_fallbacks.md). Either choice is defensible; we should pick one and lock the comment to it.

UNSTABLE state

gh pr checks 1231 is UNSTABLE because Lint Code, Integration Tests, Security Scanning, and CI Success are failing on main itself (gh run list --workflow="CI - Build & Test" --branch=main --limit=3: 3/3 failure). Lint failures are errorlint findings in cmd/multi_service_csv.go, internal/api/handler_purchases.go, internal/config/store_postgres.go, etc.; this PR touches none of those files. Same pre-existing failure shape as #1232, #1233, #1228, etc. Not blocking.

Verdict

LGTM with the canonical-form fix applied. Re-pinging @coderabbitai review.

cristim · 2026-06-26T16:49:22Z

@coderabbitai review

coderabbitai · 2026-06-26T16:49:27Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2026-06-26T16:49:42Z

@cristim Thanks for the thorough adversarial review and for calling out the canonical-vs-textual IPv6 IMDS bypass fix. I’ll re-review PR #1231 with that context in mind.

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1231

Timestamp: 2026-06-26T16:49:41.634Z
Learning: In `internal/server/scheduledauth`, `configureOIDC` constructs its own hardened `*http.Client` and passes it to `oidc.NewRemoteKeySet` via `oidc.ClientContext`, so both the initial JWKS fetch and refresh-on-unknown-kid use the hardened transport; there is no caller-supplied HTTP client on this path.

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1231

Timestamp: 2026-06-26T16:49:41.634Z
Learning: In the Go package `pkg/httpclient`, IMDS blocking compares parsed IP addresses with `net.ParseIP` and `net.IP.Equal` rather than raw host strings so both canonical and fully expanded textual forms of the AWS IPv6 IMDS address `fd00:ec2::254` are blocked.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

The scheduled-task OIDC validator issued its JWKS warmup probe via http.DefaultClient and let go-oidc's RemoteKeySet fetch keys over the default transport. The JWKS URL is operator-supplied (SCHEDULED_TASK_OIDC_JWKS_URL), so a misconfigured or compromised value could reach internal/metadata endpoints, bypassing the IMDS-blocking hardening the Azure provider clients already use. Move the hardened client (IMDS blocking, dial/TLS/overall timeouts) from providers/azure/internal/httpclient to the shared pkg/httpclient package, since the Azure-internal package is unimportable across the module boundary; the Azure package now delegates to it so there is a single implementation. The validator constructs this client once in configureOIDC, uses it for the warmup probe, and supplies it to go-oidc via oidc.ClientContext so key fetches and rotation refreshes ride the same hardened transport. Regression tests guard all paths: the constructed client must not be http.DefaultClient and must carry a timeout and dedicated transport; Warmup and token-verification key fetches are run with a guarded http.DefaultClient that fails the test if used (both fail pre-fix); pkg/httpclient gets direct IMDS-blocking coverage (IPv4 + IPv6). Closes #1145

…octx

cristim · 2026-07-17T08:07:26Z

@coderabbitai review

coderabbitai · 2026-07-17T08:07:36Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

gosec's taint analysis flags v.httpClient.Do(req) because v.jwksURL comes from operator-supplied configuration (SCHEDULED_TASK_OIDC_JWKS_URL). The call is safe: jwksURL is operator config, not request-tainted user input, and v.httpClient is the hardened client from pkg/httpclient that blocks IMDS/link-local endpoints and enforces dial/TLS/overall timeouts. Add #nosec G704 with a full justification naming both guards.

cristim · 2026-07-17T16:46:24Z

@coderabbitai review

coderabbitai · 2026-07-17T16:46:41Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/s Hours type/security Security finding labels Jun 11, 2026

cristim force-pushed the fix/sec-04-fix branch from 2c100fb to 7bfec12 Compare June 19, 2026 22:08

cristim added 2 commits July 17, 2026 11:03

fix(lint): use NewRequestWithContext in httpclient tests to satisfy n…

e47ba68

…octx

cristim force-pushed the fix/sec-04-fix branch from a8c5315 to e47ba68 Compare July 17, 2026 08:07

cristim merged commit c10989d into main Jul 17, 2026
19 checks passed

cristim deleted the fix/sec-04-fix branch July 27, 2026 11:10

Uh oh!

Conversation

cristim commented Jun 11, 2026

Problem

Fix

Test evidence

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented Jun 11, 2026

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 19, 2026

Uh oh!

coderabbitai Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 26, 2026

Adversarial review (PR #1231)

Verified

Fix pushed (a8c5315)

Out-of-scope, filed as follow-ups

UNSTABLE state

Verdict

Uh oh!

cristim commented Jun 26, 2026

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 19, 2026 •

edited

Loading

Fix pushed (`a8c5315`)

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading