Skip to content

feat(providersv2): inject static auth headers from v2 provider profiles#1891

Open
Cali0707 wants to merge 1 commit into
NVIDIA:mainfrom
Cali0707:provider-v2-injection
Open

feat(providersv2): inject static auth headers from v2 provider profiles#1891
Cali0707 wants to merge 1 commit into
NVIDIA:mainfrom
Cali0707:provider-v2-injection

Conversation

@Cali0707

Copy link
Copy Markdown
Contributor

Summary

This PR enables injection of static provider credentials that are auth headers when providers_v2_enabled is set. It extends the existing token grant injection path to resolve and inject bearer/header credentials from provider profiles, without requiring child-env placeholder resolution.

Related Issue

Part of #896

Changes

  • Gate static credential inclusion in dynamic_credentials behind providers_v2_enabled in the server
  • Extend sandbox side inject_if_needed to handle static credentials as well as the existing token grant path

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@Cali0707 Cali0707 requested review from a team, derekwaynecarr and mrunalp as code owners June 12, 2026 21:37
@copy-pr-bot

copy-pr-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements profile-driven credential injection work called out in roadmap issue #896, and the operator confirmed profile injection plus placeholder rewrite may be handled in the same request when they do not operate on the same credential.
Head SHA: 00f1bfdabf11d8cbf3ea8a3d79b45480164c29e9

Review findings:

  • Blocking: crates/openshell-server/src/grpc/provider.rs currently includes every non-token-grant credential in static proxy injection when providers_v2_enabled is set, while crates/openshell-sandbox/src/l7/token_grant_injection.rs treats empty auth_style as bearer. Built-in profiles such as Codex have multiple credentials without explicit header placement, so the proxy can inject the wrong secret as Authorization: Bearer .... Please restrict static proxy injection to credentials that explicitly declare supported header placement, for example auth_style: bearer or auth_style: header.
  • Blocking: static endpoint-bound credentials do not appear to have the ambiguity protection that token grants have. Equal host/port/path static bindings can silently select one credential by lexicographic key order. Please reject equal-specificity overlapping static proxy-injectable bindings, or otherwise make selection deterministic by explicit profile semantics that cannot mix credentials.
  • Blocking: provider_env_revision does not appear to include providers_v2_enabled, but running sandboxes refresh provider credentials only when that revision changes. Toggling the setting can leave running sandboxes with stale static injection metadata, including after disabling provider-v2 policy layers. Please include the setting in the provider environment revision or force a provider environment refresh when it changes.
  • Warning: static bearer/header profile validation should reject invalid/framing header names before runtime, reusing the existing token-grant header validation rules with static-credential wording.
  • Warning: Fern docs still describe static credentials as placeholder-only and not endpoint-scoped. Because this changes direct provider-v2 behavior, please update docs/sandboxes/providers-v2.mdx; no new docs/index.yml navigation entry appears necessary.

Suggested tests:

  • Built-in multi-credential profile behavior, for example Codex.
  • Equal-specificity static credential ambiguity.
  • providers_v2_enabled toggling and provider environment refresh behavior.
  • Static invalid/framing header profile validation.

Docs: missing for a direct provider-v2 behavior change.

Next state: gator:in-review

@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jun 13, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Blocked

Gator is blocked because this PR currently has merge conflicts with main (mergeable_state: dirty) at head SHA 00f1bfdabf11d8cbf3ea8a3d79b45480164c29e9.

Next action: @Cali0707, please rebase or merge the latest main and resolve the conflicts. After the branch is mergeable again, gator will re-check the outstanding provider-v2 review findings and docs requirement from the previous review comment.

@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jun 15, 2026
@Cali0707 Cali0707 force-pushed the provider-v2-injection branch from 00f1bfd to b96f329 Compare June 15, 2026 20:48
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head b96f329f2cef7849946f897aaacd9601e4315dac after @Cali0707 pushed an update on 2026-06-15. The previous merge-conflict blocker is resolved: GitHub now reports mergeable: true and mergeable_state: blocked, with required checks waiting for the copy-pr /ok to test mirror.

Disposition: partially resolved.

Resolved from the previous gator review:

  • Static proxy injection is now limited to credentials with explicit supported header placement, aside from the case-sensitivity gap below.
  • Equal-specificity static credential ambiguity protection is now present.
  • provider_env_revision now includes providers_v2_enabled.
  • Static bearer/header header-name validation now rejects invalid/framing header names.
  • docs/sandboxes/providers-v2.mdx now documents the provider-v2 behavior change.

Remaining items:

  • Blocking: profile injection currently removes an existing same-name header before placeholder rewrite runs. In the L7 path, injection happens before the normal placeholder rewrite; the forward proxy path has the same order. A request such as Authorization: Bearer openshell:resolve:env:OTHER_TOKEN to an endpoint whose profile also injects Authorization would have the explicit placeholder header dropped and replaced by the profile credential. That violates the gate constraint that profile injection and placeholder rewrite may operate on the same request only when they are not trying to operate on the same credential/header. Please fail closed on same-header placeholder/profile-injection collisions or otherwise reject this combination, and add regression coverage for the selected L7 and forward proxy paths.
  • Warning: static auth_style: header reuses token-grant access-token validation, which enforces token68 syntax. Static custom-header credentials can be valid HTTP field values without being token68 values, so this can reject valid API keys. Please split static header value validation from OAuth token-grant access-token validation.
  • Warning: can_inject_credential checks auth_style case-sensitively while profile validation treats it case-insensitively. A valid auth_style: Bearer or Header profile would pass validation but not inject.

I am moving this back to gator:in-review. I am not posting /ok to test yet because blocking review feedback remains.

Next state: gator:in-review

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Jun 15, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This PR remains project-valid because it implements profile-driven credential injection work from roadmap issue #896 under the operator constraint that profile injection and placeholder rewrite may operate on the same request only when they are not targeting the same credential/header.
Head SHA: b96f329f2cef7849946f897aaacd9601e4315dac

Review findings:

  • Blocking: same-header placeholder/profile-injection collisions remain unresolved. The shared injection helper still removes an existing same-name header before the normal placeholder rewrite path sees it, so a request such as Authorization: Bearer openshell:resolve:env:OTHER_TOKEN can be replaced by the profile-injected credential instead of failing closed. Please reject same-header placeholder/profile-injection collisions in the shared injector and add regression coverage for both the L7 and forward proxy paths.
  • Warning: static auth_style: header still reuses OAuth token68 access-token validation. Static custom-header values should reject header injection characters such as CR/LF/NUL, but should not need to be token68 values.
  • Warning: can_inject_credential still checks auth_style case-sensitively while profile validation treats it case-insensitively. Valid Bearer or Header profiles can pass validation without producing injection metadata.
  • Warning: static bearer handling is internally inconsistent: the runtime defaults a blank bearer header_name to Authorization, but profile validation still requires header_name for auth_style: bearer. Please either require explicit Authorization consistently or allow blank bearer header_name as the runtime default implies.

Docs: docs/sandboxes/providers-v2.mdx has been updated for this provider-v2 behavior change; no new docs/index.yml navigation entry appears necessary.

Next state: gator:in-review

@Cali0707 Cali0707 force-pushed the provider-v2-injection branch from b96f329 to 033803b Compare June 16, 2026 16:29
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head 033803bef0ca7db6c27ad8e777d6dd16cbbcd145 after @Cali0707 pushed an update on 2026-06-16. The independent code review focused on the provider credential injection and placeholder rewrite paths under the operator constraint that both mechanisms may operate on the same request only when they are not targeting the same credential/header.

Disposition: partially resolved.

Resolved from the previous gator review:

  • can_inject_credential now treats auth_style case-insensitively.
  • Static bearer blank header_name handling now defaults to Authorization, matching runtime behavior.
  • provider_env_revision still includes providers_v2_enabled.
  • Fern docs remain updated for the provider-v2 behavior change; no new docs/index.yml navigation entry appears necessary.

Remaining items:

  • Blocking: same-header placeholder/profile-injection collisions still do not fail closed. The current L7 path skips profile injection and preserves the placeholder-bearing header for the legacy rewrite path, so a request can still choose placeholder resolution for the same header that profile injection would target. Please reject this collision instead of returning the original request, and ensure the same fail-closed behavior covers every request path that can rewrite credentials, including the forward proxy path if it does not share the exact helper.
  • Warning: static auth_style: header still appears to reuse token-grant/Bearer-oriented value handling. Static custom-header values should reject unsafe HTTP header bytes such as CR, LF, and NUL at the injection sink, but should not require OAuth token68 syntax or rely only on SecretResolver side effects.
  • Warning: profile-side injection currently selects only one matching credential at equal specificity. If single-credential-per-endpoint injection is intentional for this PR, please document that limitation in the Providers v2 docs.

I am keeping this in gator:in-review and am not posting /ok to test while blocking review feedback remains.

Next state: gator:in-review

@Cali0707 Cali0707 force-pushed the provider-v2-injection branch from 033803b to d18c6f0 Compare June 16, 2026 17:21
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head d18c6f0d5b911f8d2fc32f1930c6b67a9e846d2f after @Cali0707 pushed an update on 2026-06-16. The independent code review focused on provider credential injection, placeholder rewrite ordering, and the provider-v2 credential/header constraint.

Disposition: partially resolved.

Resolved from the previous gator review:

  • Static auth_style: header values now reject unsafe HTTP header bytes without requiring OAuth token68 syntax.
  • Static same-header placeholder/profile-injection collisions now fail closed in the shared static injection helper.
  • The Providers v2 docs document static proxy injection and same-header placeholder/profile collision behavior.

Remaining items:

  • Blocking: static injectable credential bindings are generated from profile metadata even when the attached provider does not currently have any active env var for that credential. This can reject valid providers as ambiguous, including the built-in google-vertex-ai profile where service_account_token and gcloud_adc_token both target authorization on the same endpoints. Please emit static injection metadata/bindings only for static credentials that have at least one active provider env key, while keeping token grants unconditional, and add a regression test for a Vertex provider with only one active token source.
  • Blocking: the same-header placeholder collision check only covers static credential injection. Token-grant profile injection still calls the shared header replacement path directly, so an existing Authorization: Bearer openshell:resolve:env:... header can be removed before placeholder rewrite sees it. Please factor the collision check into a shared helper and apply it before both token-grant and static profile injection in the L7 and forward proxy paths.
  • Blocking: route-selected L7 traffic does not call the profile injection helper before forwarding. Built-in profiles such as GitHub have multiple L7 configs on the same host/port (api.github.com REST plus /graphql), which uses relay_with_route_selection; that path currently forwards with placeholder rewrite only and skips profile injection and its collision checks. Please route allowed requests through the same injection helper used by the single-config L7 and passthrough paths, with regression coverage.

I am keeping this in gator:in-review and am not posting /ok to test while blocking review feedback remains.

Next state: gator:in-review

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707 Cali0707 force-pushed the provider-v2-injection branch from d18c6f0 to 0439907 Compare June 17, 2026 18:16
@Cali0707 Cali0707 requested a review from maxamillion as a code owner June 17, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants