Skip to content

fix(secrets): use shared metadata guard for Key Vault - #482

Merged
cristim merged 1 commit into
mainfrom
codex/platform-go25-keyvault-shared-transport
Oct 8, 2026
Merged

cristim merged 1 commit into
mainfrom
codex/platform-go25-keyvault-shared-transport

Conversation

@cristim

@cristim cristim commented Oct 4, 2026

Copy link
Copy Markdown
Member

Summary

Use the existing shared resolved-address metadata guard for Azure Key Vault instead of the resolver's literal-host map. This closes the IPv4-mapped metadata-address bypass without changing dependencies or banning private endpoints.

Keep the production transport in the local TLS fixture, and cover constructor-to-GetSecret rejection plus the real SDK request path, authorization header, and returned secret.

Refs LeanerCloud/cloud-commitments-go#25

Exact-head verification

Head: b97a8d0068878c4ab581d6125e1326588a1c095e.

  • Independent adversarial gpt-6-astra review: no actionable findings; reviewer independently executed all native checks below.
  • Actual-parent production overlay: literal metadata control passes; mapped-address regression fails at the intended assertion.
  • Bare-transport mutation: both metadata cases fail at the intended assertions. Kernel network denial prevents the negative controls from reaching metadata services.
  • Fixed constructor: both metadata cases pass. Real SDK GetSecret through the local TLS fixture passes.
  • Native macOS Go 1.26.6: secrets-package race suite has 245 passing events and four existing credential-dependent GCP skips; repository build passes; focused configured lint reports zero issues.
  • Normal commit hooks passed before rebase; normal-config hooks for both changed files passed again on this exact rebased commit, including tidy, vet, gosec and Trivy. No checks bypassed.

Limits and merge hold

Positive SDK evidence uses synthetic credentials and a local TLS endpoint, not live Azure authentication or a deployed Private Link endpoint. Existing cloud-URL error-path tests run under kernel network denial. The local git-secrets configuration has no patterns, so its Passed status does not establish AWS credential-pattern coverage.

Exact-head CI and project-required real-scenario acceptance remain merge gates. No merge approval is claimed. This does not resolve the broader public-only policy work in LeanerCloud/cloud-commitments-go#20 or #152; #25 remains open for its remaining scope. Labels mirror #25's existing historical classification.

Replace the literal-host guard with the existing resolved-address
transport so mapped metadata addresses cannot bypass protection.
Exercise constructor wiring and retain the guard in local TLS tests.

Refs LeanerCloud/cloud-commitments-go#25
@cristim cristim added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/s Hours labels Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 13 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 81 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 92ab4ffb-ed80-4ac8-b88b-83823e1bfd43
📥 Commits

Reviewing files that changed from the base of the PR and between b4c982a and b97a8d0.

📒 Files selected for processing (2)
  • internal/secrets/azure_resolver.go
  • internal/secrets/azure_resolver_httptest_test.go
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Exact-head CI completed successfully at b97a8d0: Build & Test run 37240109834 and pre-commit run 37240109843. The independent Astra verdict and native evidence in the PR body cover this same head. CodeRabbit supplied only a review-limit notice, not a substantive review; no billing or review bypass was used. This PR remains open under the recorded real-scenario acceptance hold; green CI is not live Azure/Private Link proof.

@cristim

cristim commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Fresh verification covers unchanged head b97a8d0068878c4ab581d6125e1326588a1c095e.

  • Independent Codex adversarial review completed two clean source passes across the full two-file diff, including the pinned shared HTTP client and Azure SDK contracts. No actionable findings. The reviewer separately audited the parent/fixed regression logs, fresh build/vet output, and full secrets-suite/TLS-fixture evidence; execution was performed by this worker.
  • Regression proof uses the actual parent resolver blob 540a3901345f63ed1b7d32988474c11a03762f17 in a Go overlay with the committed new tests. The parent literal control passes. Its mapped-address case fails specifically because the SDK dial reaches kernel EPERM rather than the metadata guard. Both identical cases pass at the fixed head. The initial compiler/cache failure was rejected and is not regression evidence.
  • Fresh native macOS Go 1.26.6 repository build, focused secrets vet, and pinned formatting check pass. Full secrets suite: go test -race -short -count=1 -p=1 -v ./internal/secrets, 245 passing events and four existing credential-dependent GCP skips, exit 0. The TLS SDK GetSecret scenario passes through the retained production transport and asserts the request path, authorization header, and returned secret.
  • Every execution group first passed its exact kernel-network preflight. Metadata controls ran with all networking denied; the local suite allowed the macOS localhost matcher. The latter also includes host LAN addresses and is not claimed to be strictly loopback-only. Credentials were synthetic or explicitly unavailable; no cloud purchase, exchange, deployment, or live account acceptance was performed.

Evidence hashes:

Artifact SHA256
Fresh build 0df3afc3f19227b4b4d2430377f0f0778f762262bdff2a04a06a59eacb32aa6b
Focused vet 6b790c54bf8151b4d930a9571e80ce4e8bf88865db2c90d7a2824afa7dd55b00
Parent negative control 8881460ed65c91afbfb6363853ef4812e2a5cd791091eaa2095ffcdecc054521
Fixed regression 228949623474bf24bf0edaee98a7bb14d6ff8c028edaa8828f64d7db2499e548
Full secrets race suite 3f0cd22df668a82bd22f4eb9d9ab7f9e0d08008f23dc26d376ed54f11a315295

Logs are retained under /private/tmp/cc-platform482-evidence-20261007 and /private/tmp/cc-platform482-evidence-retry-20261007. The isolated verification worktree and original source worktree are clean; the unrelated staged #651 patch remains preserved in its original checkout.

Fixture evidence does not establish live Azure credential acquisition, production DNS, or deployed Private Link connectivity. Existing cloud-URL error tests can receive sandbox denial; four GCP skips are not cloud evidence. The local installed linter differs from CI's pinned version and was not used to claim a new lint pass. CI lint/security/normal-hook results were freshly re-fetched as successful on this exact head.

The owner's explicit override removes the earlier exact-model and mandatory real-account merge prerequisites. Those former holds are superseded. No substantive CodeRabbit review or actionable finding exists on this head; its recorded response is a quota notice, and its green status is not a review. No waiver merge or retrospective ledger entry is claimed before a merge actually occurs.

PR remains open pending coordinator merge/main sequencing and the separate no-deployment coupling hold: active push-to-main deployment workflows must not be triggered under current owner scope. No workflow changes, branch deletion, or protection bypass were performed. Before any later normal merge, recheck the unchanged reviewed head, mergeability, CI and available findings; reconcile the integration baseline after any main advance.

@cristim

cristim commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

The owner has explicitly authorized normal existing merge-triggered CI/development deployments. The prior blanket main/deployment hold in my earlier comment is superseded; workflows and protections remain unchanged.

Head remains b97a8d0068878c4ab581d6125e1326588a1c095e, with freshly queried green checks, CLEAN mergeability and no inline findings. Existing independent review is clean. The reviewer is preparing a direct, separately executed parent/fixed/full-secrets-suite replay under the canonical shared test lock, rather than relying on the earlier log audit. Coordinator merge sequencing remains in force.

Prior counts include nested events: 245 PASS / 4 SKIP total, 167 PASS / 1 SKIP top-level. Package duration is 54.331s; verification runner wall duration is 57.012s. These are distinct measures from the same successful log. Four credential-dependent GCP skips are not live-cloud evidence.

@cristim

cristim commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

Head reviewed: b97a8d0068878c4ab581d6125e1326588a1c095e.
Integrated main: 2cd196fa05650fdf4aef1bd31cd95a9b916d7a89.

Independent Codex reviewer completed two clean adversarial source passes and personally reproduced the regression and integrated SDK fixture suite. No actionable findings remain.

  • Parent resolver with the committed regression tests: literal metadata control PASS; mapped-address case FAIL at the intended assertion (kernel EPERM instead of the metadata guard). Fixed exact head: both PASS. The initial compiler/cache failure in earlier work is not regression evidence.
  • Independent integrated full secrets race suite, current main with exactly the two reviewed PR blobs overlaid: exit 0, 245 passing events and four existing GCP credential skips. SDK TLS GetSecret success/error/nil, JSON success/invalid/error, and list success/filter/error/empty cases PASS. The positive fixture retains the production dial transport and asserts the request path, authorization header, and secret value. Main's newer shared HTTP client is byte-identical to the reviewed dependency.
  • Fresh integrated repository build PASS. Earlier exact-head build, focused vet and pinned formatting checks PASS. No source or dependency changes were made during takeover.

Commands (offline Go 1.26.6, GOWORK=off, readonly dependencies, -p=1, synthetic/absent credentials, reviewed kernel sandbox preflight):

go test -race -count=1 -v -timeout=90s -run=^TestNewAzureResolver_GetSecret_BlocksMetadata$ ./internal/secrets
# Parent uses actual parent resolver overlay; fixed uses committed source.
go build ./...
go test -race -short -count=1 -v -timeout=420s ./internal/secrets
# Integrated build/suite use only the two frozen reviewed blobs over main's manifests.
Log SHA256
Independently executed parent e2e780c4237b66020177fbd87d6944d0e8cccf853985c2d1ac4a929c8724fcc6
Independently executed fixed 3b14c24e36ded75382411b59cba5f7ecab4e13ddee6476be5cc355e80964a0e9
Integrated repository build 3a1c8af0ffc57c0283222617923d20aa35050a09539412942f322ffc270b634d
Independently executed integrated suite d047fe694144ea99b5be45ca7b85f86ce777d9f4084a0f0a6c9198d785a4d96e

Logs are preserved in /private/tmp/cc-platform482-independent-evidence-20261008 and /private/tmp/cc-platform482-main-integration-evidence-20261008. Metadata negative controls deny all networking. The local suite permits macOS's localhost matcher, which also includes host LAN addresses; strict loopback-only confinement is not claimed. Live Azure authentication, production DNS and deployed Private Link remain untested. Credential-dependent GCP skips do not establish cloud coverage.

CR waived: quota, adversarial review + local verification + green CI

The exact-head CodeRabbit status explicitly says Review rate limited; its recorded response is a quota notice, not a substantive review. No available review findings exist. The owner authorizes normal existing merge-triggered CI/development deployments and removes exact-model/live-account acceptance holds. Workflows and branch protections remain unchanged. Normal merge will freshly check unchanged head, all CI, findings, CLEAN mergeability and current main under the sole coordinator merge lock. The expected-head guard protects the PR head; the local lock cannot exclude unrelated remote main advances, so actual postmerge parent/blob identities will also be reconciled. Record retrospective CR review debt after the actual qualifying merge.

Upstream #25 remains open for its other scope, with broader policy tracked by #20 and #152. No new follow-up issue was needed.

@cristim
cristim merged commit d3c5bb8 into main Oct 8, 2026
25 checks passed
@cristim

cristim commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

PR #482 merged normally as d3c5bb8bb35588cc3a7b10875ac9310182190195 from independently reviewed head b97a8d0068878c4ab581d6125e1326588a1c095e. Fresh API reconciliation confirms first parent 2cd196fa05650fdf4aef1bd31cd95a9b916d7a89 and both changed source blobs exactly match the independently executed integration.

Independent parent FAIL/fixed PASS, actual SDK TLS success/failure/edge fixtures and integrated full secrets race suite passed; fresh repository build passed. Commands, outputs and hashes. No actionable findings. Live Azure authentication, production DNS and deployed Private Link were not exercised. Broader policy work in go #20/#152 and the remaining scope of go #25 remain open; no duplicate follow-up issue needed.

CR waived: quota, adversarial review + local verification + green CI

Actual merged-commit CI Build/Test 37710048533, pre-commit 37710048484 and frontend sentinel 37710048483 are completed SUCCESS. Existing authorized development deployment runs AWS 37710048456, GCP 37710048453, Azure 37710048437 are pending. Coordinator existing cadence owns observation/failure handling; no duplicate watcher/timer created.

Coordinator: add #482 to the existing retrospective CR set. Local ledger /private/tmp/cc-platform-w3-cr-waived-reviews-20261008.tsv and complete receipt /private/tmp/cc-platform-w3-pr482-merged-result-20261008.txt are preserved. Automatic permission review timed out before copying these final records to the existing bus, so no successful bus copy is claimed. w3 is idle and available for another disjoint coordinator-dispatched PR. Original source worktrees and the staged five-line #651 fixture patch remain preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant