Repository navigation
feat(vertex): accept credential JSON with secure marker lifecycle (carry #6843) - #7139
Merged
Merged
Conversation
…6821) Let the provider's API-key field take a Service Account JSON directly: - gcp-adc: a `gcp-sa:<account>` marker resolves the credential from the OS keychain (same service + entry factory as `keychain:` references) and runs the existing service_account JWT / authorized_user flow. The marker sits above GOOGLE_APPLICATION_CREDENTIALS in the source priority; everything downstream (token cache, refresh skew, in-flight dedup) is untouched. - api-keys: addProviderApiKey detects a pasted credential JSON (single, pretty-printed, or comma-separated for future pool rotation), verifies the "type" field, stores the secret in the OS keychain with a read-back check, and persists only the marker to config. File paths are rejected with paste guidance; unrecognized types are rejected before anything is stored. - router: usableResolvedApiKey registers the active `gcp-sa:` marker with the ADC resolver, so a marker in any pool entry becomes the active source. - adapters/google: vertex's API-key fast path skips `gcp-sa:` markers — they are not API-key material and must reach the ADC branch instead. Markers keep resolveProviderApiKey synchronous (no signature changes) and work naturally with the existing 429 key-pool failover: each pool entry can independently hold a raw key, an ${ENV} ref, a keychain: ref, or a gcp-sa: marker, and per-key cooldown rotation applies unchanged. Closes #6821 (partially — live model discovery for vertex mode is a separate discussion). Co-Authored-By: Claude <noreply@anthropic.com>
…6842) Address CodeRabbit findings: - gcp-adc: remove the process-global activeMarkerAccount/registration (a module-level marker would race across concurrent requests for different providers and could resolve one call with another provider's credential). The marker account now travels per request via getVertexAccessToken(options.markerAccount) and flows through currentAdcSourceKey/loadAdcCredentials as an argument. The reconcile sweep passes no marker (no request context) — marker entries are pruned by expiry instead. - adapters/google: pass gcpCredentialMarkerAccount(provider.apiKey) per request. router: drop the registration; the routed provider config carries the marker verbatim. - api-keys: delete a just-written keychain entry ONLY when the write created it — on a failed commit, an upsert must keep the credential the previous reference still names. - tests: narrow the adapter assertion to the ADC-shaped request (URL + Bearer header + no x-goog-api-key, with a stubbed fetch), and isolate the marker-source tests from host ADC state (empty CLOUDSDK_CONFIG, no GOOGLE_APPLICATION_CREDENTIALS, 404 fetch stub). Co-Authored-By: Claude <noreply@anthropic.com>
…ng (#6842) Address CodeRabbit round-2 findings: - router: usableResolvedApiKey returns gcp-sa: marker values UNCHANGED — replacing a marker with undefined (it resolves to no key material) would silently drop it from the routed config and send the vertex adapter to host ADC instead of the configured service account. - api-keys: storeGcpCredentialJson takes existedBefore and deletes the keychain entry after a read-back mismatch only when the entry did not exist before the write — a re-paste targets an account an existing marker already names, and deleting it would destroy the live credential. - api-keys: splitCredentialPaste honors the parseGcpCredentialJson contract (a single-line brace-leading literal is never eaten — it falls back to a plain key row), tracks JSON string/escape state in the brace scanner (a } inside a string field no longer splits early), and rejects non-separator content before/between objects instead of discarding it. - tests: router-level pass-through test (routedProviderConfig preserves the marker), end-to-end test (routed config feeds buildRequest, ADC branch wins), literal-key fallback + gap-rejection tests. Markers keep resolveProviderApiKey synchronous. Co-Authored-By: Claude <noreply@anthropic.com>
…rkers; strict paste validation (#6842) Address Prana review findings: - adapters/google: a `gcp-sa:` marker means the operator named ONE credential explicitly — the GOOGLE_CLOUD_API_KEY env fallback no longer fires. Without this guard, a marker + env key would silently route the provider's inference through the ambient env key instead of the configured service account. - providers/api-keys (capture): a parseable JSON object with an unrecognized "type" field (e.g. external_account) is REJECTED with guidance, not saved as a plaintext literal key — saving it would send it to the wire verbatim. Non-JSON brace values ({abc:def}) keep their key-compat behavior. - providers/api-keys (tail): content after the LAST object's close up to EOF is validated like the mid-gaps — trailing garbage is rejected, not silently kept. (Covers captureTrailing otherwise.) Registrations: per-finding regression tests added (explicit-credential-wins for markers, JSON-with-unknown-type rejection, tail-gap rejection). Co-Authored-By: Claude <noreply@anthropic.com>
…eat/vertex-sa-json-paste-dev
…t/vertex-sa-json-paste-dev
…paste-dev Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…re persistence Co-Authored-By: Claude <noreply@anthropic.com>
Reject wrapped or malformed credential material before literal persistence or restore, and prevent keychain aliases exposing GCP JSON as wire keys. Revalidate marker readability before cached-token use. Capture deletion candidates and persist CLI/management removal under one mutation lock, preserving post-commit shared-reference cleanup. Cover mixed SA/key failover and DTO redaction with synthetic credentials. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
Decode structured JSON before credential classification so escaped field and type names cannot enter literal key resolution or keychain restore. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Reject object, array and quoted-string carriers directly so nested or escaped GCP credential material cannot bypass alias, ingestion or restore validation. Preserve non-JSON literal token handling and dedicated supported GCP-object ingestion. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
7 tasks done
Contributor
|
✅ Deterministic PR hygiene checks passed. |
# Conflicts: # scripts/test-layout/layout.json # tests/fixtures/test-layout-expected.json
Move credential-input rejection responses and locked provider deletion into a cohesive sibling module. Preserve validation responses, mutation ordering, save callback and post-commit cleanup behavior while keeping provider-routes.ts below the fixed 2000-line limit after dev integration. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
# Conflicts: # scripts/test-layout/layout.json
# Conflicts: # structure/config.md # structure/gui-and-management-api.md # structure/providers-and-adapters.md
# Conflicts: # structure/config.md # structure/gui-and-management-api.md
# Conflicts: # scripts/test-layout/layout.json # structure/gui-and-management-api.md # tests/fixtures/test-layout-expected.json
Owner
Author
|
Maintainer integration into
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e6e7478886
ℹ️ 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".
| const stored: Array<{ id: string; marker: string }> = []; | ||
| const created: string[] = []; | ||
| try { | ||
| const committed = commitProviderApiKeySelection(config, name, fresh => { |
There was a problem hiding this comment.
When /api/providers/keys is called for any key-auth provider other than Vertex—for example, an OpenAI-compatible relay—this path still converts the JSON into a gcp-sa: marker and commits it as active. The router preserves that marker, but only the Google Vertex adapter resolves it; other adapters transmit the marker as their API key, so the endpoint returns success while replacing working authentication with an unusable value. Validate that the target is an effective Vertex provider before accepting credential JSON, matching the newly documented Vertex-only workflow.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
Comment on lines
+245
to
+248
| const isJsonPaste = first !== undefined && typeof first !== "string"; | ||
| if (!isJsonPaste) { | ||
| // Not a credential paste — fall through to the plain API-key path below. splitCredentialPaste | ||
| // has already thrown for file paths; a plain key lands here as a single literal row. |
There was a problem hiding this comment.
A gcp-sa: value takes this plain-key branch, so posting a typo or copied marker to /api/providers/keys returns 201 and activates it without verifying that its OS-store entry exists. Reposting an existing marker also hashes the marker text rather than the original credential JSON, appending the same marker under a second pool ID; rotation then attempts the same credential twice and deleting one row does not remove it. Reject marker strings here, or validate and upsert them by their existing pool entry instead of treating them as new secrets.
Useful? React with 👍 / 👎.
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #6842.
Carry GSL-R's #6843 (head
59c4e2c018becc3527e4cabbd36afb203174f809) onto dev7640a942b3, preserving its complete commit history with merge10d06e0718a8. The feature starts at36b9c1ef80; all subsequent contributor and maintainer fixes remain in the ancestry. No changes from the separate #6937 round-robin proposal are included.Vertex's dedicated API-key pool accepts GCP service-account or authorized-user JSON. Verified OS credential-store writes commit only
gcp-sa:markers to config/backups, with transactional batch admission and shared-reference-aware cleanup. Provider create/edit and CLI add reject credential JSON and direct users to the pool endpoint.Follow-up corrections reject structured JSON carriers, malformed and JSON-escaped credential material in literal ingestion/restore and keychain alias resolution; marker errors omit account identity; cached bearer use verifies the selected stored payload remains readable. CLI and management DELETE capture live/durable marker candidates under the same mutation lock as persistence, then clean up only after success. Added generated service-account tests exercise existing 401/429 failover between markers and ordinary API keys, plus management DTO redaction. No GUI, provider preset, dependency, or workflow changes.
Security: implementation-side credential audit and regression verification completed; independent bounded read-only code/security review PASS on
65a032c7065d0b9a53a32d429a681b2ce4a540ad, with no remaining P0–P2 findings. The reviewer inspected source only; it did not independently rerun tests or qualify OS stores. Explicit maintainer security review is required before merge under MAINTAINERS.md. Fake OS-store tests do not qualify real macOS Keychain, Linux Secret Service or Windows Credential Manager behavior. No real user configuration, credentials, keychains or accounts were accessed. The original PR was not edited, commented on, closed or merged.Co-authored-by: GSL-R 174104911+GSL-R@users.noreply.github.com
Verification
Local runtime: isolated Bun 1.4.3 in ignored scratch space; temporary OPENCODEX_HOME/CODEX_HOME/CLOUDSDK_CONFIG and fake credential stores. Root
bun installandcd docs-site && bun install --frozen-lockfilesucceeded. No new dependency or lockfile changes.Red-to-green checks: wrapped/malformed input, restore and token removal initially 0 pass / 9 fail; overlapping deletion initially 13 pass / 2 fail; encoded credential classification initially 13 pass / 3 fail. A further combined escaped-and-wrapped regression initially passed 15 / failed 7; the final structured-carrier fix at 65a032c passes all 22 boundary cases. Earlier broad targeted attempt: 385 pass / 2 fail; one pinned-runner mismatch fixed by using Bun 1.4.3 and one error-guidance mismatch fixed in source, then the 391-case run above passed.
Full local
bun run test,bun run prepush, and whole-directory suites were NOT RUN, per maintainer shared-host resource constraints. Broad repository, packaging, real OS credential-store, and cross-platform validation remain with CI. PR runs the fast Ubuntu subset; after it passes, dispatchgh workflow run ci.yml --ref carry/6843-vertex-sa-json -f lane=allfor full cross-platform evidence. Exact-head PR run: https://github.com/lidge-jun/opencodex/actions/runs/38699863108 (pending/queued as of 2026-10-15 07:37 UTC). No broad-suite or platform pass is claimed. This PR remains draft pending hosted evidence and maintainer security review.Checklist