fix(secrets): migrate to GA Azure Key Vault azsecrets module

[cristim]

Problem

Closes #1146 (review finding SUP-01, P2).

internal/secrets/azure_resolver.go imported the retired beta module github.com/Azure/azure-sdk-for-go/sdk/keyvault/azsecrets v0.12.0 (plus the transitively retired sdk/keyvault/internal v0.7.1). Microsoft retired the entire sdk/keyvault/* beta tree in 2023 in favor of sdk/security/keyvault/*, so the old module receives no bug or security fixes despite sitting on the live secrets-resolution path that fetches admin passwords, API keys, and encryption keys from Azure Key Vault. The repo already used the GA tree elsewhere (sdk/security/keyvault/azkeys v1.4.0), so the retired and GA trees coexisted.

Fix

• Migrate internal/secrets/azure_resolver.go to the GA module sdk/security/keyvault/azsecrets v1.4.0. The only API change on our surface is the rename NewListSecretsPager -> NewListSecretPropertiesPager; GetSecret, SetSecret, response shapes, and ID.Name() semantics are unchanged.
• Pin v1.4.0 (matching the in-repo azkeys version) so no other dependency bumps are needed; go mod tidy drops the retired sdk/keyvault/azsecrets and sdk/keyvault/internal from go.mod and go.sum entirely.
• Update the existing unit and httptest suites to the GA type names (SecretProperties, Secret, ListSecretPropertiesResponse/Options, SecretPropertiesListResult). The httptest harness now uses httptest.NewTLSServer with the test server's client as transport, because the GA Key Vault challenge policy only attaches credentials over TLS-protected connections.
• Add a regression guard test (internal/secrets/azure_sdk_module_guard_test.go) that fails if any retired sdk/keyvault/* module path reappears in the root go.mod.

Test evidence

• Guard test verified against the pre-fix code: with the migration stashed, go test ./internal/secrets/ -run TestNoRetiredAzureKeyVaultModule FAILS on sdk/keyvault/azsecrets v0.12.0; after the fix it passes.
• go build ./... succeeds.
• go test ./internal/secrets/ -count=1: 243 passed, 0 failed.
• go vet ./internal/secrets/: no issues.
• grep "sdk/keyvault/" go.mod go.sum: no matches remaining.

[coderabbitai]

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: 52 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: d4c735a8-7a66-4d3e-aac7-4e1642cc3bc4

📥 Commits

Reviewing files that changed from the base of the PR and between f3134a7 and 2425412.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (6)

• go.mod
• internal/secrets/azure_resolver.go
• internal/secrets/azure_resolver_coverage_test.go
• internal/secrets/azure_resolver_httptest_test.go
• internal/secrets/azure_resolver_test.go
• internal/secrets/azure_sdk_module_guard_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sup-01-migrate-azsecrets-ga

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

Adversarial review (PR #1224)

Credential-surface PR, so I exercised every risk-axis from the brief. The migration is sound and the SSRF hardening is a strict improvement; two duplication / DNS-bypass items go out as follow-ups rather than blocking the GA migration.

Verified

• API rename map complete: NewListSecretsPager -> NewListSecretPropertiesPager, SecretItem -> SecretProperties, SecretBundle -> Secret, ListSecretsResponse -> ListSecretPropertiesResponse, SecretListResult -> SecretPropertiesListResult, ListSecretsOptions -> ListSecretPropertiesOptions. Cross-checked against the runtime file (azure_resolver.go:117 SetSecretParameters, :149 NewListSecretPropertiesPager, :157-165 page.Value / secret.ID.Name()) and against the mock + httptest harness (azure_resolver_test.go:35,48,50,53,75,77,277-291,..., azure_resolver_httptest_test.go). No callers of the old names remain. grep -rn "sdk/keyvault/" --include='*.go' returns zero hits; grep -rn "sdk/security/keyvault/azsecrets" --include='*.go' returns the four expected migrated files. go build ./... clean, go vet ./internal/secrets/ clean, go test ./internal/secrets/ -count=1 = 244/0. ✓
• Secret-value format is byte-identical pre/post migration: both API versions return resp.Value as *string containing the raw stored bytes; GetSecret ends in return *resp.Value, nil and GetSecretJSON runs the same json.Unmarshal on it. Existing secrets in Azure Key Vault remain readable. ✓
• Fail-closed on resolution failure: every runtime consumer of Resolver.GetSecret (internal/database/connection.go:77, internal/server/app.go:326,649,725 for the scheduled-task secret, admin-password sync, encryption key) returns the error up; internal/secrets/resolver.go:80-83 already errors if AZURE_KEY_VAULT_URL is empty. The app cannot boot with empty credentials when the resolver fails. ✓ (Memory feedback_no_silent_fallbacks.md satisfied.)
• IMDS-blocking transport correctly scoped: NewAzureResolver builds azidentity.NewDefaultAzureCredential(nil) first - that credential keeps the SDK's default HTTP client, so managed-identity token acquisition via IMDS still works. Only the azsecrets pipeline gets the IMDS-blocked transport via policy.ClientOptions.Transport, which sits at the bottom of the azcore pipeline (retry / auth / logging still wrapped around it). Key Vault REST calls are blocked from talking to IMDS; token fetch is not. ✓
• TLS-only test path matches GA challenge policy: newTestAzureResolver switched from httptest.NewServer to httptest.NewTLSServer and replaced InsecureAllowCredentialWithHTTP: true with Transport: server.Client(). This is the documented requirement (GA challenge policy refuses to attach Bearer tokens over plaintext) and the only way credentials would round-trip in the mock. Confirmed by the 11 TestAzureResolverReal_* cases all going green. ✓
• PII / log discipline: error messages include the secret name (failed to get secret %s, failed to set secret %s), never the value; ListSecrets returns names only. Same shape as the AWS and GCP resolvers, no regression. (Memory feedback_pii_in_logs.md satisfied.) ✓
• Module guard test is real: stashed the resolver migration, reverted go.mod manually, ran go test ./internal/secrets/ -run TestNoRetiredAzureKeyVaultModule against an in-place sdk/keyvault/azsecrets v0.12.0 line - test fails with the documented message. After the fix it passes. The grep is anchored on the full prefix github.com/Azure/azure-sdk-for-go/sdk/keyvault/, so it cannot misfire on sdk/security/keyvault/. ✓
• Version pin: azsecrets v1.4.0 matches the in-repo azkeys v1.4.0. No sdk/keyvault/internal v0.7.1 indirect leftover - go.mod and go.sum both drop it cleanly. azsecrets v1.4.0 (current GA) has no open CVEs on the Go vuln db. ✓
• Token-cache interference: nothing in the codebase still imports the retired sdk/keyvault/azsecrets. The only other GA Key Vault import is sdk/security/keyvault/azkeys in internal/oidc/azure_signer.go, which uses the same azcore credential cache - no parallel cache stacks. ✓

Follow-ups filed (out of scope)

• chore(secrets): extract IMDS-blocking transport into shared pkg/httpclient #1323 - chore(secrets): extract IMDS-blocking transport into shared pkg/httpclient. imdsBlockingTransport/blockIMDSDialer/imdsAddresses at internal/secrets/azure_resolver.go:17-60 are bit-for-bit copies of providers/azure/internal/httpclient/httpclient.go:14-53. The copy is currently forced (root module cannot import providers/azure/internal/*), but pkg/ is already a separate module both can depend on; consolidating restores the single-source-of-truth the feedback_azure_use_httpclient_new.md grep guard assumes. Triage: type/chore, priority/p3, severity/low, urgency/eventually, impact/internal, effort/s, triaged.
• sec(httpclient): resolve hostname before IMDS-block check (closes DNS-bypass gap) #1324 - sec(httpclient): resolve hostname before IMDS-block check (closes DNS-bypass gap). Both implementations check the literal addr host against an IP-only map; a request to a hostname that resolves to 169.254.169.254 (e.g. metadata.google.internal) is NOT blocked because DNS resolution happens inside inner.DialContext. Not exploitable today (vault URL is env-controlled and azcore does not auto-follow redirects), but the memory feedback_azure_use_httpclient_new.md frames this as defense-in-depth. Best landed atop chore(secrets): extract IMDS-blocking transport into shared pkg/httpclient #1323. Triage: type/security, priority/p3, severity/low, urgency/eventually, impact/internal, effort/s, triaged.

UNSTABLE state

All failing checks are pre-existing on main (cross-checked against the PR #1245 review of the same triad):

• Security Scanning: npm-side pnpm audit / Snyk hits in jest dep chain (@tootallnate/once, fast-uri, flatted, transitive js-yaml); no Go-side govulncheck finding. Cannot be caused by a go.mod-only secrets-package change.
• Integration Tests: RDS DescribeDBMajorEngineVersions fails with no EC2 IMDS role found - CI runner lacks AWS creds, identical to the failure mode on every recent main run.
• Lint Code: pre-existing errorlint findings spread across cmd/multi_service_csv.go, internal/api/handler_*.go, internal/commitmentopts/, internal/config/store_postgres*.go, internal/database/postgres/migrations/migrate.go - all files this PR does not touch.
• CI Success: aggregator of the above.

None are caused by this diff. Not blocking.

Verdict

LGTM. CR re-ping posted (@coderabbitai review).

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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]

@cristim Thanks for the thorough adversarial pass — this is very helpful.

Your validation makes sense, and I agree with the conclusion: the GA Azure Key Vault migration in #1224 looks sound, the IMDS scoping is an improvement, and the duplication / DNS-resolution items are reasonable follow-ups rather than blockers for this PR.

I’m re-running the review now.

🐇✅

---

✏️ Learnings added

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

Timestamp: 2026-06-26T17:25:01.261Z
Learning: In this repository's secrets flow, resolution failures are expected to fail closed rather than silently falling back: `internal/database/connection.go` and `internal/server/app.go` propagate `Resolver.GetSecret` errors, and `internal/secrets/resolver.go` already errors when `AZURE_KEY_VAULT_URL` is unset.

---

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

Timestamp: 2026-06-26T17:25:01.261Z
Learning: For Azure Key Vault tests in this repository, the GA `sdk/security/keyvault/azsecrets` challenge policy requires TLS-protected requests. The `internal/secrets/azure_resolver_httptest_test.go` harness should use `httptest.NewTLSServer` and the test server's client transport instead of plaintext HTTP with `InsecureAllowCredentialWithHTTP`.

---

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

Timestamp: 2026-06-26T17:25:01.261Z
Learning: The IMDS-blocking transport logic in `internal/secrets/azure_resolver.go` duplicates `providers/azure/internal/httpclient/httpclient.go`, but the root module cannot import `providers/azure/internal/*`. Any future consolidation should move the shared logic into a non-internal module such as `pkg/httpclient`.

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.

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

Merged to main after pre-merge adversarial review (verdict MERGE): beta sdk/keyvault/azsecrets fully replaced by the GA module (only rename NewListSecretsPager->NewListSecretPropertiesPager; GetSecret empty-version semantics unchanged), fail-loud error wrapping preserved, IMDS-blocking transport correctly scoped to the azsecrets pipeline, guard test blocks beta re-introduction, union with main green (244 secrets tests). Zero unresolved CR threads. Consolidation/DNS hardening tracked in #1323/#1324.