Repository navigation
feat(providers): report bounded token-grant failures - #4427
johntmyers merged 14 commits into
Conversation
|
Label |
e7d1a41 to
b36ab6c
Compare
shiju-nv
changed the title
Signed-off-by: Shiju <shiju@nvidia.com>
Signed-off-by: Shiju <shiju@nvidia.com>
Signed-off-by: Shiju <shiju@nvidia.com>
Signed-off-by: Shiju <shiju@nvidia.com>
Remove duplication and dead paths from the token-grant failure reporting without changing classification, the workload-visible 502 response, or the wire format. - Build the gRPC failure envelope with tonic-types, store the typed failure in EndpointFailure, and share one request-ID rule and one endpoint-status authority check. - Merge the gateway exchange client wrapper, drop forwarding validators, and name the request field when a reported cause is rejected. - Inject fallback credentials only from live provider state and delete the unread per-connection credential copy. Keep one 502 writer per path and restore the base token-cache handling. - Check the token endpoint after the cache lookup, still before any credential acquisition. - Document 429 and 5xx classification as implemented, and drop tests that repeat retained assertions through the same code path. Signed-off-by: Shiju <shiju@nvidia.com>
Cut the token-grant failure tests to one test per distinct behavior across core, gateway, supervisor, CLI and Go, and replace the 794-line closure E2E with status assertions in the existing route-recovery E2E: the refused grant appears in sandbox status with the response's request ID, code, stage and upstream status, and the matching grant's later success clears it. - Pin the live provider snapshot with one helper used by inspected and fallback forwarding, and inject selector-matched grants through inject_for_selectors, which borrows the pinned context. Move the test-only wrapper into test_support. - Drop the 35-second wrappers around the gateway grant RPC. The shared channel bounds the connection attempt and the gateway bounds its token-service calls. - Log the token service's OAuth error and error_description, without control characters and limited to 256 characters each, with the request ID in the gateway or supervisor log. Status and the workload response still carry only the bounded cause. - Keep one check that an untrusted token-service certificate cannot assert a cause. Signed-off-by: Shiju <shiju@nvidia.com>
The injection tests named EndpointObservationSender through the parent module, which no longer imports it. The features suite lockfile kept the tonic TLS dependency that the removed E2E fixture added. Signed-off-by: Shiju <shiju@nvidia.com>
Pass the response-size width as a named argument; an implicitly captured constant panics at runtime. Name the mutation type in the validation table for Clippy. Signed-off-by: Shiju <shiju@nvidia.com>
MAX_TOKEN_RESPONSE_BYTES exceeds u16::MAX, the largest format width, so the padded fixture panicked. Pad with repeat_n as before. Signed-off-by: Shiju <shiju@nvidia.com>
b019445 to
488ead9
Compare
Main added the ListProvidersRequest label selector. The merged public schema hash comes from the Branch Checks storage_proto test output. Signed-off-by: Shiju <shiju@nvidia.com>
BlockedGitHub reports merge conflicts between this PR and its base branch. Code review is paused until those conflicts are resolved. DCO, Branch Checks, and E2E passed on the current head. Action required: @shiju-nv, please resolve the merge conflicts and update the PR. Gator will recheck the updated head and continue the review. Gator metadata
|
johntmyers
added
the
gator:blocked
Compatibility review (advisory)Unaddressed breaking changes New fields on Rust raw protobuf structs break exhaustive struct literals in the stable 1. rust-sdk — NOT ACKED
Review details
Advisory review; publication gates are unchanged. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The merge conflicts are resolved, and the structured failure-reporting work fits validated issue #3319. One blocking credential-disclosure issue remains in the new OAuth diagnostic logging.
Action required: @shiju-nv, remove remote OAuth error text from the new log emission, add the canary regression test, and align the documentation with the redacted behavior.
Blocking findings:
GATOR-b312d3bd-01: Token-service error text can expose credentials through supervisor logs.
Carried findings: None.
Non-blocking suggestions: None.
Gator metadata
- Validation: Implements validated issue #3319; author repository write permission verified.
- Docs: Fern updates present; the raw OAuth logging paragraph needs correction with the finding.
- Checks: DCO, Branch Checks, Helm Lint and Trivy Changes passed; current-head E2E running at review time.
- E2E:
test:e2eapplied; run 38671292915 confirmed in progress for this head. No dispatch or rerun needed. - Review: Independent code-only review; no local tests run.
- Head SHA:
b312d3bdc6f812f1274dadb316df3dc95afa6d8b - Base SHA:
909f336b624a1b7efc5a0c607e35e20dcb96923e - Merge base SHA:
909f336b624a1b7efc5a0c607e35e20dcb96923e - Patch ID:
d7941f7d57b9f7329ed8e80a6b1bd7cff1d1ab8d - Gator payload:
12 - Review mode:
initial - Previous reviewed SHA:
none - Review budget exhausted:
no - Maintainer decision required:
no - Next state:
gator:in-review
johntmyers
added
gator:in-review
A token service can echo a submitted credential in its OAuth error or error_description. Log only the classified failure code, HTTP status and request ID, and cover the canary echo in a dedicated test binary. Signed-off-by: Shiju <shiju@nvidia.com>
Compatibility review (advisory)Unaddressed breaking changes The candidate adds members to generated Rust protobuf structs exposed by 1. rust-sdk — NOT ACKED
Review details
Advisory review; publication gates are unchanged. |
PR Review Status@shiju-nv, your update fixes the credential-logging finding. I checked the shared grant helper, the canary regression test for both remote error fields, the structured log event fields, and the corrected Fern documentation. The independent follow-up review found no new blocking issues in the author-only delta. Blocking findings: None. Carried findings:
Branch Checks and E2E are still pending on the current head. Gator will inspect their results next cycle. Gator metadata
|
johntmyers
added
gator:watch-pipeline
acknowledged: adds fields to SandboxStatus, EndpointObservation, ReportEndpointStatusRequest and EndpointStatus; Rust struct literals need ..Default::default() |
johntmyers
added
gator:approval-needed
Compatibility review (advisory)Unaddressed breaking changes Generated Rust protobuf structs exposed through 1. rust-sdk — ACKED
Review details
Advisory review; publication gates are unchanged. |
323dee1 to
b4864ba
Compare
Signed-off-by: Shiju <shiju@nvidia.com>
b4864ba to
b5e5457
Compare
PR Review Status@shiju-nv, I reviewed your documentation update against the existing token-grant behavior; the independent follow-up review found no new blocking issues. I also checked your acknowledgement that exhaustive Rust protobuf struct literals need Blocking findings: None. Carried findings:
Current-head Branch Checks and E2E are running. Gator will monitor their results before handing this back for maintainer approval. Gator metadata
|
johntmyers
added
gator:watch-pipeline
Compatibility review (advisory)Unaddressed breaking changes Generated Rust protobuf structs exposed through 1. rust-sdk — ACKED
Review details
Advisory review; publication gates are unchanged. |
johntmyers
added
gator:approval-needed
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Compatibility review (advisory)Unaddressed breaking changes Generated Rust protobuf structs exposed through 1. rust-sdk — ACKED
Review details
Advisory review; publication gates are unchanged. |
PR Review Status@shiju-nv, I checked your condensed token-grant failure guidance against the previously reviewed behavior. The independent Critical-only review found no new blocking issues in this documentation-only delta; the credential-logging finding remains fixed and its thread remains resolved. Blocking findings: None. Carried findings:
Branch Checks passed on the current head, and E2E is running. Gator is returning this PR to pipeline monitoring until the required checks are green; maintainer approval is still outstanding. Gator metadata
|
johntmyers
added
gator:watch-pipeline
johntmyers
added
gator:merge-ready
and removed
gator:watch-pipeline
Monitoring CompleteMonitoring is complete because this PR has merged. The last Gator review found no blocking issues, and the previously reported credential-logging finding remains fixed. I removed the active Gator metadata
|
2 participants
Summary
A refused dynamic token grant now gives authorized callers a bounded cause and recovery action through sandbox reads, while the workload receives a generic HTTP 502 and an opaque request ID. Reporting covers inspected HTTP protocols and selector-based forwarding, including failures before credential selection.
Related Issue
Closes #3319
Changes
Classify each refused grant into one closed set of codes, each with a fixed recovery action from the existing provider recovery vocabulary. No cause retains remote descriptions, tokens or error chains.
provider_authorization_requiredFAILED_PRECONDITIONworkload_identity_rejectedPERMISSION_DENIEDconfiguration_errorFAILED_PRECONDITIONtimeoutDEADLINE_EXCEEDEDunavailableUNAVAILABLEunclassifiedUNKNOWNRecognize a token service's
provider_authorization_requiredonly with the exactopenshell.token-grant.v1contract over verified HTTPS; a bare 401 staysunclassified. The gateway can also establish that cause from missing or expired stored subject credentials. HTTP 429 and 5xx without a recognized extension code areunavailable, as are connect failures, including TLS certificate or hostname verification failures. A failure after the connection is established isunclassified, and any timeout, including a slow response body, istimeout.The gateway returns a refused grant to the supervisor as a gRPC status with a single
ErrorInfodetail (reasonTOKEN_GRANT_FAILURE). The supervisor decodes it strictly. Any other status, such as details over 1024 bytes, more than one detail, or unknown metadata keys, codes or stages, becomesunclassifiedat thegateway_rpcstage.Carry one supervisor-minted request ID through both token-service hops, the gateway, refusal logs, status and workload response (
x-openshell-request-id).Retain one
SandboxStatus.last_token_grant_failurewith a bounded destination and failure cause. Reuse authenticated status reports and sandbox reads, with no new public RPC or request history. Matching grant success and configuration/session resets clear it; unrelated success retains it. MCP endpoint status also retains its configured-endpoint cause.Proto additions, all on existing messages or new messages they reference:
EndpointFailureandEndpointFailureDomain(domainTOKEN_GRANT), carrying code, recovery action, provider, credential key, stage, optional upstream HTTP status and request ID.TokenGrantStatusandTokenGrantFailureObservation(destination host and port plus anEndpointFailure).ReportEndpointStatusRequest.token_grant_status = 8. Omitting it keeps the previous behavior; a present message with nolast_failureclears the stored cause.EndpointObservation.failure = 3,EndpointStatus.last_failure = 7andSandboxStatus.last_token_grant_failure = 17.In Rust, the supervisor builds the proto
EndpointFailurefrom a typedTokenGrantFailurethroughopenshell_core::endpoint_failure::EndpointFailure. The gateway validates reported causes against the same closed sets and names the rejected field and reason (EndpointFailureValidationError) without echoing rejected values.Select the live credential snapshot on fallback forwarding so an idle socket cannot report an old grant under a new provider revision, and drop the per-connection credential copy that no request reads anymore. Expose the result through CLI JSON/YAML/text, the Go SDK and documentation.
Add a real REST workload test with native CA verification at both grant hops, authenticated persisted reads, exact correlation, redaction, denied anonymous and supervisor-credential readers, and recovery.
Testing
Focused checks pass on the final head: core 586, server 747 (provider, policy, sandbox, session and schema modules), network 1120 (token grant, L7 and proxy modules) and CLI rendering tests; Go converter tests; strict Clippy on all targets of the five affected production packages; E2E target compilation; formatting and Markdown lint. The repository pre-commit hook (workspace formatting and Clippy on all targets) passed.
Final-head Branch Checks passed all 24 jobs on the first attempt. E2E passed with 59 successful jobs and six optional skipped jobs. The verified-HTTPS REST case passed in bundled Podman, and its target completed all three tests. Optional Windows, GPU and Helm contexts are not runtime proof.
The feature-enabled E2E target retains 13 preexisting Clippy diagnostics. Its library and a diagnostic run allowing only those known categories pass; no lint configuration was changed. The hosted default-feature strict Clippy gate passed.
Checklist