Skip to content

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

Open
Cali0707 wants to merge 2 commits into
NVIDIA:mainfrom
Cali0707:provider-v2-injection
Open

feat(providersv2): inject static auth headers from v2 provider profiles#1891
Cali0707 wants to merge 2 commits 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

@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
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head 043990788a22e2a6f088cca1c074784650842570 after @Cali0707 pushed an update on 2026-06-17. The independent code review focused on provider credential injection, placeholder rewrite ordering, route-selected L7 handling, and the provider-v2 credential/header constraint.

Disposition: partially resolved.

Resolved from the previous gator review:

  • Static injectable credential bindings are now gated on active provider env keys for normal non-expired credentials, and the Vertex single-active-token-source case has regression coverage.
  • Same-header placeholder/profile-injection collisions now fail closed through the shared injector for both static and token-grant profile injection.
  • Route-selected L7 and GraphQL forwarding now call the shared injection helper before forwarding, so profile injection and collision checks are no longer skipped on that path.
  • Static custom-header values no longer require OAuth token68 syntax.
  • Fern docs document the new static profile injection and same-header placeholder collision behavior; no new docs/index.yml navigation entry appears necessary.

Remaining items:

  • Blocking: static injection metadata still treats any stored provider credential key as active, but the provider environment resolver filters expired, invalid, and non-injectable keys before exposing values. An expired static credential can therefore still produce static injection metadata or ambiguity bindings, causing false ambiguity rejections or request-time 502s when the proxy selects metadata whose resolver has no value. Please make the static binding/dynamic metadata eligibility check reuse the same active-key semantics as provider environment resolution, including expiry and key validity, while keeping token grants unconditional. Please add regression coverage with one expired Vertex token source and one active source.
  • Warning: docs/sandboxes/providers-v2.mdx still has a profile example comment saying static runtime injection uses env placeholder resolution. Please update that example so it does not contradict the new proxy-side static header injection behavior.
  • Warning: the forward proxy now uses the shared injection helper, but there is still no forward-proxy regression test for same-header placeholder/profile collisions. The shared helper unit tests are useful, but the prior gate explicitly covered forward proxy ordering.
  • Warning: several operator-facing errors/logs still say "Token grant failed" for static credential injection failures and placeholder collisions. Please use "credential injection failed" or equivalent wording at the shared call sites so provider-v2 static credential failures are debuggable.

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 0439907 to 3c91a31 Compare June 18, 2026 17:51
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head 3c91a31e3324c0a4cac02018a2562ddca6225330 after @Cali0707 pushed an update on 2026-06-18. The independent code review focused on provider credential injection, placeholder rewrite ordering, active static credential eligibility, forward proxy coverage, and the provider-v2 credential/header constraint.

Disposition: resolved.

Resolved from the previous gator review:

  • Static injection metadata now reuses the same active-key semantics as provider environment resolution for stored provider credentials, including expiry, invalid key names, and non-injectable special cases, while token grants remain unconditional.
  • Regression coverage now includes the Vertex case with one expired token source and one active token source.
  • The forward proxy has same-header placeholder/profile collision regression coverage.
  • Static credential injection and placeholder collision failures now report as credential injection failures at the shared relay/proxy call sites.
  • The Providers v2 docs no longer describe static proxy-side profile injection as placeholder-only behavior.

Remaining items:

  • No blocking items remain.
  • Non-blocking cleanup noted by the independent reviewer: some ambiguous credential-binding diagnostics and internal helper names still use token-grant-specific wording even though the path now covers static credentials too.

Docs: docs/sandboxes/providers-v2.mdx is updated for this provider-v2 behavior change; no docs/index.yml navigation update appears necessary.

E2E: applying test:e2e because this changes provider credential flow and proxy behavior.

Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status test:e2e Requires end-to-end coverage and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jun 18, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 3c91a31

@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 3c91a31. Open Branch E2E Checks, find the run for commit 3c91a31, and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After CI Failure

I re-checked head 3c91a31e3324c0a4cac02018a2562ddca6225330 after the Branch Checks failure.

Disposition: author action required.

Findings:

  • OpenShell / Branch Checks failed in both Rust jobs during mise run rust:format:check.
  • The failed log shows cargo fmt --all -- --check wants to collapse the miette! call at crates/openshell-supervisor-network/src/l7/token_grant_injection.rs:449 to return Err(miette!("credential auth_style header requires header_name"));.
  • OpenShell / E2E has started and is still running, but the required Branch Checks gate is already red.

Next action: @Cali0707, please run Rust formatting and push the formatted update. Gator will re-check review status and CI on the new head.

Next state: gator:in-review

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jun 18, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After CI Failure

I re-checked head 3c91a31e3324c0a4cac02018a2562ddca6225330 after the E2E run completed.

Disposition: author action required.

Findings:

  • OpenShell / Branch Checks is still failing in both Rust jobs during mise run rust:format:check; cargo fmt --all -- --check wants the miette! call at crates/openshell-supervisor-network/src/l7/token_grant_injection.rs:449 formatted onto one line.
  • OpenShell / E2E also failed in e2e / E2E (rust-docker). The failing test is forward_proxy_allows_l7_permitted_request in tests/forward_proxy_l7_bypass.rs; it expected HTTP 200 for an L7-allowed forward-proxy GET and got HTTP 403.
  • The other visible E2E variants passed: rust-podman-rootless, rust-podman, python, and Kubernetes Rust smoke.

Next action: @Cali0707, please push a formatting fix and investigate the Docker forward-proxy/L7 allow-path failure. Gator will re-check review status and CI on the new head.

Next state: gator:in-review

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707

Copy link
Copy Markdown
Contributor Author

@johntmyers I fixed the formatting issue, for the e2e test I think that may have been a flake? When I ran the e2e test locally it passed for me...:

running 1 test
test forward_proxy_allows_l7_permitted_request ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 12.05s

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Update

I re-evaluated latest head 33c3f7ade7eb4f914a6fc946202c9ee17f9c18b1 after @Cali0707 pushed the 2026-06-18 formatting update.

Disposition: resolved.

Remaining items:

  • No blocking review items remain.
  • Non-blocking cleanup noted by the independent reviewer: the shared forward-proxy injector helper name still uses token-grant-specific wording even though it now handles static credentials too.
  • Required checks must be refreshed for this head. OpenShell / Branch Checks, OpenShell / Helm Lint, and OpenShell / E2E are currently pending on the copy-pr mirror.

Docs: docs/sandboxes/providers-v2.mdx is updated for this provider-v2 behavior change; no docs/index.yml navigation update appears necessary.

E2E: test:e2e is already applied because this changes provider credential flow and proxy behavior.

I am moving this back to gator:watch-pipeline and posting /ok to test for the current head so CI can run.

Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jun 18, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 33c3f7a

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Author Comment

I also evaluated @Cali0707's 2026-06-18 19:59 UTC comment that the formatting issue is fixed and the previously failing forward_proxy_allows_l7_permitted_request E2E passed locally.

Disposition: resolved for pipeline re-check.

Remaining items:

  • No blocking review items remain at head 33c3f7ade7eb4f914a6fc946202c9ee17f9c18b1.
  • The prior Docker E2E failure still needs confirmation from the refreshed required CI run, which is now queued after /ok to test.

Next state: gator:watch-pipeline

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After CI Failure

I re-checked head 33c3f7ade7eb4f914a6fc946202c9ee17f9c18b1 after the refreshed E2E run completed.

Disposition: author action required.

Findings:

  • OpenShell / Branch Checks and OpenShell / Helm Lint passed.
  • OpenShell / E2E failed in all three Rust runtime variants: rust-docker, rust-podman, and rust-podman-rootless; the Python E2E and Kubernetes Rust smoke variants passed.
  • The failing Rust E2E test is graphql_l7_enforces_allow_and_deny_rules_on_forward_and_connect_paths in tests/forward_proxy_graphql_l7.rs.
  • The failures are on allowed forward-proxy GraphQL requests returning 403 instead of 200: Docker and rootless reported forward_query_allowed=403; Podman reported forward_chunked_query_allowed=403 with {"detail":"POST graphql-l7.openshell.test:8000/graphql not permitted by policy","error":"policy_denied"}.

Next action: @Cali0707, please investigate the forward-proxy GraphQL L7 allow path and push an update or explain why the CI result should be retried as infrastructure-related. Gator will re-check review status and CI on the new head or maintainer guidance.

Next state: gator:in-review

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jun 18, 2026
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 test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants