Repository navigation
fix(providers): harden static credential placement - #4886
johntmyers wants to merge 5 commits into
Conversation
johntmyers
added
area:supervisor
|
🌿 Preview your docs: https://nvidia-preview-pr-4886.docs.buildwithfern.com/openshell |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance foundStatic-credential delivery previously required endpoint-binding support; now polling and configuration push also require supports_static_credential_placement.
2. configuration — No explicit risk acceptance foundEndpoint-bound profiles previously delivered sandbox credentials without placement metadata; now missing placement causes withholding, including Codex refresh and ID tokens.
3. policy — No explicit risk acceptance foundStatic placeholders previously resolved at supported endpoint-authorized locations; now profile placement restricts them and overrides request_body_credential_rewrite and websocket_credential_rewrite.
4. rust-sdk — No explicit risk acceptance foundGenerated GetSandboxProviderEnvironmentRequest, StaticCredentialBinding, and SupervisorHello gain public fields absent from their baseline shapes.
5. typescript-sdk — No explicit risk acceptance foundGenerated raw GetSandboxProviderEnvironmentRequest and SupervisorHello types gain the required boolean property supportsStaticCredentialPlacement.
Review details
Advisory review; publication gates are unchanged. |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. policy — No explicit risk acceptance foundStatic placeholders previously resolved in supported headers, query values, and paths at authorized endpoints; existing auth_style metadata now restricts those locations automatically.
Review details
Advisory review; publication gates are unchanged. |
|
Label |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The independent code review found no blocking issues in the static credential placement changes. The implementation follows issue #4883, with documented compatibility limits and regression coverage for placement, credential ownership, and explicit body/WebSocket rewriting.
Blocking findings: No blocking findings remain.
Carried findings: None.
Non-blocking suggestions: None.
Gator metadata
- Validation: Focused provider/supervisor hardening requested by linked issue #4883; author repository admin authority verified.
- Docs: Provider overview, profiles, and network policy guidance updated.
- Checks: Current-head Branch Checks, Helm Lint, Trivy Changes, and DCO pass.
- E2E: test:e2e required for credential flow and network enforcement; E2E Label Help is queued; follow its instructions next cycle before pipeline handoff.
- Head SHA:
8531c4b4fd8f4080d9b41499a2517e4612ef19df - Base SHA:
92434dcb2b6248ac827df05c00551fe7a1fedf17 - Merge base SHA:
92434dcb2b6248ac827df05c00551fe7a1fedf17 - Patch ID:
d07c1a03a4ccf6863379380eaf9475b4dc605fac - Gator payload:
13 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review(review complete; awaiting E2E dispatch)
johntmyers
added
gator:in-review
Blocker Follow-Up NudgeThis PR is still blocked by merge conflicts with Next action: @johntmyers, please resolve the merge conflicts and push an updated head so Gator can check the delta and current-head CI. Gator metadata
|
8531c4b to
2d80c50
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @johntmyers for resolving the merge conflicts and reporting the post-rebase checks. The independent follow-up review confirmed that all 22 files retain the previously reviewed additions and deletions, checked the affected integration context, and found no new blocking issues. The merge-conflict blocker is cleared, and current-head CI and E2E are queued or running.
Blocking findings: No blocking findings remain.
Carried findings: None.
Gator metadata
- Validation: Focused provider/supervisor hardening follows issue #4883; author repository admin authority verified.
- Docs: Provider overview, profiles, and network policy guidance cover automatic enforcement and compatibility limits.
- Checks: DCO, Helm Lint, and Trivy Changes pass; Branch Checks is running. Maintainer approval remains missing after the rebase and is required before merge.
- E2E: test:e2e is applied. Current-head Branch E2E Checks run 41140967194 is queued with successful build prerequisites and remaining build jobs queued. The older E2E Label Help rerun instruction targets the prior head; the new head already has its own required run.
- Head SHA:
2d80c50d12a6be3bf9c6b0421264de9c694afdd4 - Base SHA:
c2311ad01eca37a12abc3ea50446c6487d862ffb - Merge base SHA:
c2311ad01eca37a12abc3ea50446c6487d862ffb - Patch ID:
48db3882e2aa27cfa7e44fdb186b3b3ee62721f8 - Gator payload:
13 - Review mode:
follow_up - Previous reviewed SHA:
8531c4b4fd8f4080d9b41499a2517e4612ef19df - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
Maintainer Review NudgeThis PR has been in @NVIDIA/openshell-maintainers, can someone review and either approve, request changes, or close this out? Action required: A maintainer listed in Gator metadata
|
|
The PR enforces placement when metadata is present but missing metadata still falls back to As discussed I’d replace We should keep endpoint restrictions, reject invalid metadata and keep body/WebSocket rewriting explicitly opt-in. We should also check all bundled providers and add the correct placement metadata where needed This would still break configurations that rely on implicit placement outside Authorization so we should document the required configuration change. I think that is a reasonable tradeoff compared with preserving the vulnerable fallback |
johntmyers
added
gator:in-review
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
2d80c50 to
442a5b3
Compare
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. policy — Risk explicitly acceptedStatic placeholders previously resolved in supported endpoint-authorized headers, query values, and paths; they now require declared placement or default to Authorization when metadata is absent.
2. configuration — No explicit risk acceptance foundBearer profiles previously accepted header_name alongside query_param and path_template, but the supervisor now rejects that documented combination.
Review details
Advisory review; publication gates are unchanged. |
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @alangou for spelling out the Authorization-only fallback and its compatibility tradeoff. I checked @johntmyers's update against that request: missing metadata now defaults to Authorization, explicit malformed metadata is rejected, endpoint restrictions and opt-in body/WebSocket rewriting remain in place, and the provider and migration guidance is updated. The independent follow-up review found no new blocking code issues; the earlier two commits retain their previously reviewed patches.
Action required: @johntmyers, remove the changes to sdk/go/proto/openshellv1/openshell.pb.go from this PR. The current-head Go SDK Freeze job fails because the in-tree Go SDK is frozen. Its generated bindings are not needed here; any Go SDK changes belong in ai-openshell/sdk-go. Push an updated head so Gator can check the delta and CI again.
Blocking findings: No blocking code-review findings remain. The Go SDK Freeze CI failure still requires the author action above.
Carried findings: None.
Gator metadata
- Validation: Focused credential-placement hardening follows issue #4883; author and responding maintainer repository admin authority verified.
- Docs: Fern provider overview and profiles document the Authorization default, gateway-first upgrade order, and imported-profile migration; existing navigation remains applicable.
- Checks: Current-head DCO, Helm Lint gate, and Trivy Changes pass. Branch Checks remains active with Go SDK Freeze failed. The failed-job annotations and local comparison to the current base both identify the generated Go binding changes.
- E2E: test:e2e is applied. Current-head Branch E2E Checks run 41894090218 is queued with build prerequisites active. The mirror matches the current head; the older Label Help rerun instruction applies to an earlier head, so no command or rerun is needed now.
- Head SHA:
442a5b3d4797ba02639e33c7c6d63808c22dd216 - Base SHA:
dbc0517a3103a1361194262a70800cf30c8fc939 - Merge base SHA:
dbc0517a3103a1361194262a70800cf30c8fc939 - Patch ID:
32640781425366bb3ec4bcc7440f688a2936fb8f - Gator payload:
13 - Review mode:
follow_up - Previous reviewed SHA:
2d80c50d12a6be3bf9c6b0421264de9c694afdd4 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review - Blocked reason:
ci_author_changes_required
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @johntmyers for restoring the frozen Go bindings and protobuf source. I compared both files with the current base: they are identical, and the current-head Go SDK Freeze check passes. The independent Critical-only follow-up review found no new Critical defects in the latest delta. The previously reviewed implementation and @alangou's Authorization-default guidance remain unchanged.
Blocking findings: No blocking findings remain.
Carried findings: None.
Current-head Branch Checks and E2E are running. Gator will monitor their results before handing this to a maintainer for approval.
Gator metadata
- Validation: Focused credential-placement hardening follows issue #4883; author and responding maintainer repository admin authority verified.
- Docs: Existing Fern provider and migration updates cover the Authorization default and gateway-first upgrade order; this delta changes comments and generated formatting only.
- Checks: Go SDK Freeze, DCO, Helm Lint gate, and Trivy Changes pass. Branch Checks run 41897628906 remains in progress.
- E2E: test:e2e is applied. Current-head Branch E2E Checks run 41897629072 is in progress, and the copy-pr mirror matches the head. The old Label Help rerun instruction targets an earlier head; no additional dispatch is needed.
- Head SHA:
d58eff510ebd25ca78707dc472712e95d71f2a52 - Base SHA:
dbc0517a3103a1361194262a70800cf30c8fc939 - Merge base SHA:
dbc0517a3103a1361194262a70800cf30c8fc939 - Patch ID:
8557dcdef50c29c8244e9c172ade7d8555d25a4b - Gator payload:
13 - Review mode:
critical_only - Previous reviewed SHA:
442a5b3d4797ba02639e33c7c6d63808c22dd216 - Review budget exhausted: yes
- Maintainer decision required: no — prior CI obligation is resolved, with no qualifying scope growth or new Critical finding.
- Next state:
gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @johntmyers for the compatibility follow-up. I checked that auth_style selects exactly one placement, unused fields do not authorize other locations, and invalid selected placement still fails closed. The added regressions cover legacy profiles with multiple placement fields and preserve other attached providers. The independent Critical-only review found no new Critical defects in this delta; @alangou's Authorization-default guidance remains intact.
Blocking findings: No blocking findings remain.
Carried findings: None.
Current-head Branch Checks and E2E are queued or running. Gator will inspect their results before handing this to a maintainer for approval.
Gator metadata
- Validation: Focused credential-placement hardening follows issue #4883; author and responding maintainer repository admin authority verified.
- Docs: Fern profiles guidance and the core README explain active-style selection and ignored unused fields; prior migration guidance remains applicable.
- Checks: DCO, Go SDK Freeze, Helm Lint gate, and Trivy Changes pass. Branch Checks run 41900244742 remains pending. Frozen Go SDK and protobuf source match the base.
- E2E: test:e2e is applied. Current-head Branch E2E Checks run 41900244832 is queued with completed build jobs and remaining jobs pending; the copy-pr mirror matches the head. The old Label Help rerun instruction targets an earlier head, so no additional dispatch is needed.
- Head SHA:
f59db0685893762c1a011e05707bd5a9b76e5f11 - Base SHA:
dbc0517a3103a1361194262a70800cf30c8fc939 - Merge base SHA:
dbc0517a3103a1361194262a70800cf30c8fc939 - Patch ID:
4105b46ed726c7212d52eb37ffae357777bb5ad3 - Gator payload:
13 - Review mode:
critical_only - Previous reviewed SHA:
d58eff510ebd25ca78707dc472712e95d71f2a52 - Review budget exhausted:
yes - Maintainer decision required: no — no unresolved obligations, qualifying scope growth, or new Critical finding.
- Next state:
gator:watch-pipeline
johntmyers
added
gator:approval-needed
johntmyers
added
gator:merge-ready
and removed
gator:approval-needed
Summary
Restrict static credential substitution to the provider's declared request location. When placement metadata is absent, default to the
Authorizationheader rather than allowing arbitrary headers, query parameters, or URL paths. Preserve explicit body/WebSocket rewrite policies and trusted request signing.Related Issue
Closes #4883
Changes
Authorization(including Bearer, token, and Basic formats).auth_style, ignoring fields for other styles without granting additional locations. Reject invalid selected placement or malformed binding metadata instead of silently falling back. Preserve atomic snapshot installation and separation from token grants.Release Notes
Static credentials without placement metadata now resolve only in
Authorization, or through explicitly enabled body/WebSocket rewrite policies. Custom headers such asx-api-key, query parameters, and path slots require an explicit credential placement declaration.Upgrade the gateway before the supervisor for those clients: an older gateway does not forward placement metadata, so a new supervisor applies the Authorization default even when the stored profile declares another location. Update existing imported profiles as needed; editing reference YAML alone does not update the gateway catalog. Older supervisors retain their previous behavior. Wire formats and credential delivery identities remain unchanged.
auth_styleselects the active placement. Existing profiles may retain fields for other styles; those fields are ignored and never authorize additional request locations.Testing
Full deployed old/new gateway-supervisor combinations were not exercised.
Known limitation from the broader validation:
proxy::tests::mediated_connect_keeps_workload_bytes_read_with_the_synthesized_headertimed out in the unfiltered run and two exact reruns, then passed on a later rerun of the same test binary. Baseline main (dbc0517a3) passed its exact comparison. No baseline failure was reproduced and the cause remains unresolved. The passing Rust aggregate above explicitly excludes this credential-free raw CONNECT test.Checklist