Skip to content

Commit 29a2b8e

Browse files
shiju-nvjohntmyers
andauthored
feat(providers): report bounded token-grant failures (#4427)
* feat(providers): report bounded token-grant failures Signed-off-by: Shiju <shiju@nvidia.com> * test(providers): align policy metadata and TLS lockfile Signed-off-by: Shiju <shiju@nvidia.com> * test(providers): fix trusted grant recovery fixture Signed-off-by: Shiju <shiju@nvidia.com> * test(providers): enable denial API in recovery fixture Signed-off-by: Shiju <shiju@nvidia.com> * refactor(providers): simplify token-grant failure reporting 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> * refactor(providers): trim token-grant failure tests and supervisor API 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> * fix(providers): restore test import and features lockfile 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> * test(providers): fix body-bound fixture width and table type 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> * test(providers): pad exact-bound body without a format width 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> * test(server): update public RPC schema hash after rebase 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> * fix(providers): keep token-service error text out of grant logs 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> * docs(providers): tighten token grant failure documentation Signed-off-by: Shiju <shiju@nvidia.com> * docs(providers): condense token grant failure guidance Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --------- Signed-off-by: Shiju <shiju@nvidia.com> Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> Co-authored-by: John Myers <9696606+johntmyers@users.noreply.github.com>
1 parent 01087a4 commit 29a2b8e

48 files changed

Lines changed: 6318 additions & 1868 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎Cargo.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/openshell-cli/src/commands/provider.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2118,7 +2118,7 @@ fn refresh_status_row(status: &ProviderCredentialRefreshStatus) -> String {
21182118
)
21192119
}
21202120

2121-
fn provider_refresh_recovery_action_name(
2121+
pub fn provider_refresh_recovery_action_name(
21222122
action: ProviderCredentialRefreshRecoveryAction,
21232123
) -> &'static str {
21242124
match action {

‎crates/openshell-cli/src/run.rs‎

Lines changed: 192 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1808,6 +1808,17 @@ where
18081808
);
18091809
}
18101810

1811+
if let Some(observation) = sandbox
1812+
.status
1813+
.as_ref()
1814+
.and_then(|status| status.last_token_grant_failure.as_deref())
1815+
{
1816+
println!(" {}", "Last token grant failure:".dimmed());
1817+
for line in token_grant_failure_display_lines(observation) {
1818+
println!(" {line}");
1819+
}
1820+
}
1821+
18111822
let policy_from_global = config.policy_source == PolicySource::Global as i32;
18121823
println!(
18131824
" {} {}",
@@ -3037,10 +3048,22 @@ fn sandbox_to_json(sandbox: &Sandbox) -> serde_json::Value {
30373048
"path": endpoint.path,
30383049
"last_result": endpoint_result_name(endpoint.last_result()),
30393050
"last_reported_at": endpoint.last_reported_time.as_ref().map(ToString::to_string).unwrap_or_default(),
3051+
"last_failure": endpoint.last_failure.as_ref().map(endpoint_failure_to_json),
30403052
})
30413053
})
30423054
.collect::<Vec<_>>()
30433055
});
3056+
let last_token_grant_failure = sandbox
3057+
.status
3058+
.as_ref()
3059+
.and_then(|status| status.last_token_grant_failure.as_deref())
3060+
.map(|observation| {
3061+
serde_json::json!({
3062+
"destination_host": observation.destination_host,
3063+
"destination_port": observation.destination_port,
3064+
"failure": observation.failure.as_ref().map(endpoint_failure_to_json),
3065+
})
3066+
});
30443067
let admission = sandbox
30453068
.status
30463069
.as_ref()
@@ -3097,6 +3120,7 @@ fn sandbox_to_json(sandbox: &Sandbox) -> serde_json::Value {
30973120
"exit_code": sandbox.status.as_ref().and_then(|status| status.exit_code),
30983121
"conditions": conditions,
30993122
"endpoint_statuses": endpoint_statuses,
3123+
"last_token_grant_failure": last_token_grant_failure,
31003124
"configuration_admission": admission,
31013125
"provisioning": provisioning,
31023126
"created_from_workload_template": created_from_workload_template,
@@ -3154,6 +3178,83 @@ fn endpoint_result_name(result: EndpointResult) -> &'static str {
31543178
}
31553179
}
31563180

3181+
fn endpoint_failure_domain_name(domain: i32) -> &'static str {
3182+
match openshell_core::proto::EndpointFailureDomain::try_from(domain) {
3183+
Ok(openshell_core::proto::EndpointFailureDomain::TokenGrant) => "token_grant",
3184+
Ok(openshell_core::proto::EndpointFailureDomain::Unspecified) | Err(_) => "unknown",
3185+
}
3186+
}
3187+
3188+
fn endpoint_failure_recovery_name(
3189+
action: openshell_core::proto::ProviderCredentialRefreshRecoveryAction,
3190+
) -> &'static str {
3191+
match action {
3192+
// Structured output names every value; "-" is a table placeholder.
3193+
openshell_core::proto::ProviderCredentialRefreshRecoveryAction::Unspecified => {
3194+
"unspecified"
3195+
}
3196+
action => crate::commands::provider::provider_refresh_recovery_action_name(action),
3197+
}
3198+
}
3199+
3200+
fn endpoint_failure_to_json(failure: &openshell_core::proto::EndpointFailure) -> serde_json::Value {
3201+
serde_json::json!({
3202+
"domain": endpoint_failure_domain_name(failure.domain),
3203+
"code": failure.code,
3204+
"recovery_action": endpoint_failure_recovery_name(failure.recovery_action()),
3205+
"provider": failure.provider,
3206+
"credential_key": failure.credential_key,
3207+
"stage": failure.stage,
3208+
"upstream_http_status": failure.upstream_http_status,
3209+
"request_id": failure.request_id,
3210+
})
3211+
}
3212+
3213+
fn endpoint_failure_display_lines(failure: &openshell_core::proto::EndpointFailure) -> Vec<String> {
3214+
use openshell_core::proto::ProviderCredentialRefreshRecoveryAction as Recovery;
3215+
let upstream = failure
3216+
.upstream_http_status
3217+
.map(|status| format!(", upstream HTTP {status}"))
3218+
.unwrap_or_default();
3219+
let recovery = match failure.recovery_action() {
3220+
Recovery::Retry => "Retry later; the failure can be temporary.",
3221+
Recovery::Reauthorize => "Reauthorize the provider credential.",
3222+
Recovery::FixConfiguration => {
3223+
"An operator must fix the provider or identity configuration."
3224+
}
3225+
Recovery::Investigate => "An operator must investigate gateway and supervisor logs.",
3226+
Recovery::Unspecified => "No recovery action provided.",
3227+
};
3228+
let mut lines = vec![format!(
3229+
"Cause: {} ({}, stage {}{upstream})",
3230+
failure.code,
3231+
endpoint_failure_domain_name(failure.domain).replace('_', " "),
3232+
failure.stage,
3233+
)];
3234+
if !failure.provider.is_empty() {
3235+
lines.push(format!(
3236+
"Credential: {}/{}",
3237+
failure.provider, failure.credential_key
3238+
));
3239+
}
3240+
lines.push(format!("Recovery: {recovery}"));
3241+
lines.push(format!("Request ID: {}", failure.request_id));
3242+
lines
3243+
}
3244+
3245+
fn token_grant_failure_display_lines(
3246+
observation: &openshell_core::proto::TokenGrantFailureObservation,
3247+
) -> Vec<String> {
3248+
let mut lines = vec![format!(
3249+
"Destination: {} (port {})",
3250+
observation.destination_host, observation.destination_port
3251+
)];
3252+
if let Some(failure) = observation.failure.as_ref() {
3253+
lines.extend(endpoint_failure_display_lines(failure));
3254+
}
3255+
lines
3256+
}
3257+
31573258
fn endpoint_status_display_lines(endpoint: &EndpointStatus) -> Vec<String> {
31583259
let ports = endpoint
31593260
.ports
@@ -3179,14 +3280,18 @@ fn endpoint_status_display_lines(endpoint: &EndpointStatus) -> Vec<String> {
31793280
.last_reported_time
31803281
.as_ref()
31813282
.map_or_else(|| "no report yet".to_string(), ToString::to_string);
3182-
vec![
3283+
let mut lines = vec![
31833284
format!(
31843285
"{} (ports: {ports}; path: {})",
31853286
endpoint.host, endpoint.path
31863287
),
31873288
format!("Last result: {result}"),
3188-
format!("Reported at (gateway acceptance): {reported_at}"),
3189-
]
3289+
];
3290+
if let Some(failure) = endpoint.last_failure.as_ref() {
3291+
lines.extend(endpoint_failure_display_lines(failure));
3292+
}
3293+
lines.push(format!("Reported at (gateway acceptance): {reported_at}"));
3294+
lines
31903295
}
31913296

31923297
fn restart_policy_name(policy: i32) -> &'static str {
@@ -8678,6 +8783,7 @@ mod tests {
86788783
path: "/mcp".to_string(),
86798784
last_result: EndpointResult::TransportFailed as i32,
86808785
last_reported_time: Some("2026-09-05T10:01:00Z".parse().unwrap()),
8786+
last_failure: None,
86818787
}],
86828788
conditions: vec![SandboxCondition {
86838789
r#type: "Ready".to_string(),
@@ -8725,6 +8831,7 @@ mod tests {
87258831
"path": "/mcp",
87268832
"last_result": "TransportFailed",
87278833
"last_reported_at": "2026-09-05T10:01:00Z",
8834+
"last_failure": null,
87288835
}])
87298836
);
87308837
}
@@ -8739,6 +8846,7 @@ mod tests {
87398846
path: "/mcp".to_string(),
87408847
last_result: EndpointResult::TransportFailed as i32,
87418848
last_reported_time: Some("2026-09-05T11:01:00Z".parse().unwrap()),
8849+
last_failure: None,
87428850
};
87438851

87448852
assert_eq!(
@@ -8760,6 +8868,7 @@ mod tests {
87608868
path: "/**".to_string(),
87618869
last_result: EndpointResult::NoObservedExchange as i32,
87628870
last_reported_time: None,
8871+
last_failure: None,
87638872
};
87648873

87658874
assert_eq!(
@@ -8787,11 +8896,91 @@ mod tests {
87878896
"path": "/**",
87888897
"last_result": "NoObservedExchange",
87898898
"last_reported_at": "",
8899+
"last_failure": null,
87908900
}])
87918901
);
87928902
assert_eq!(output["conditions"], serde_json::json!([]));
87938903
}
87948904

8905+
#[test]
8906+
fn token_grant_failure_renders_in_text_and_structured_output() {
8907+
use openshell_core::proto::{
8908+
EndpointFailure, EndpointFailureDomain, ProviderCredentialRefreshRecoveryAction,
8909+
TokenGrantFailureObservation,
8910+
};
8911+
let failure = EndpointFailure {
8912+
domain: EndpointFailureDomain::TokenGrant.into(),
8913+
code: "provider_authorization_required".to_string(),
8914+
recovery_action: ProviderCredentialRefreshRecoveryAction::Reauthorize.into(),
8915+
provider: "github".to_string(),
8916+
credential_key: "access_token".to_string(),
8917+
stage: "token_endpoint".to_string(),
8918+
upstream_http_status: Some(409),
8919+
request_id: "7e6d5c4b-3a29-4180-9f6e-5d4c3b2a1908".to_string(),
8920+
};
8921+
let endpoint = EndpointStatus {
8922+
host: "tools.example.test".to_string(),
8923+
last_failure: Some(failure.clone()),
8924+
..Default::default()
8925+
};
8926+
let observation = TokenGrantFailureObservation {
8927+
destination_host: "api.example.test".to_string(),
8928+
destination_port: 8443,
8929+
failure: Some(failure),
8930+
};
8931+
let cause = [
8932+
"Cause: provider_authorization_required (token grant, stage token_endpoint, upstream HTTP 409)",
8933+
"Credential: github/access_token",
8934+
"Recovery: Reauthorize the provider credential.",
8935+
"Request ID: 7e6d5c4b-3a29-4180-9f6e-5d4c3b2a1908",
8936+
];
8937+
// MCP endpoint status places the cause between the result and report time.
8938+
let endpoint_lines = super::endpoint_status_display_lines(&endpoint);
8939+
assert_eq!(endpoint_lines[2..6], cause);
8940+
assert!(endpoint_lines[6].starts_with("Reported at"));
8941+
assert_eq!(
8942+
super::token_grant_failure_display_lines(&observation),
8943+
[&["Destination: api.example.test (port 8443)"][..], &cause].concat()
8944+
);
8945+
8946+
let output = super::sandbox_to_json(&Sandbox {
8947+
status: Some(SandboxStatus {
8948+
endpoint_statuses: vec![endpoint],
8949+
last_token_grant_failure: Some(Box::new(observation)),
8950+
..Default::default()
8951+
}),
8952+
..Default::default()
8953+
});
8954+
let failure_json = serde_json::json!({
8955+
"domain": "token_grant",
8956+
"code": "provider_authorization_required",
8957+
"recovery_action": "reauthorize",
8958+
"provider": "github",
8959+
"credential_key": "access_token",
8960+
"stage": "token_endpoint",
8961+
"upstream_http_status": 409,
8962+
"request_id": "7e6d5c4b-3a29-4180-9f6e-5d4c3b2a1908",
8963+
});
8964+
assert_eq!(output["endpoint_statuses"][0]["last_failure"], failure_json);
8965+
assert_eq!(
8966+
output["last_token_grant_failure"],
8967+
serde_json::json!({
8968+
"destination_host": "api.example.test",
8969+
"destination_port": 8443,
8970+
"failure": failure_json,
8971+
})
8972+
);
8973+
assert!(super::sandbox_to_json(&Sandbox::default())["last_token_grant_failure"].is_null());
8974+
8975+
let unknown = super::endpoint_failure_to_json(&EndpointFailure {
8976+
domain: 99,
8977+
recovery_action: 99,
8978+
..Default::default()
8979+
});
8980+
assert_eq!(unknown["domain"], "unknown");
8981+
assert_eq!(unknown["recovery_action"], "unspecified");
8982+
}
8983+
87958984
#[test]
87968985
fn endpoint_status_http_response_does_not_claim_a_successful_tool_call() {
87978986
let endpoint = EndpointStatus {

‎crates/openshell-core/Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ protoc-bin-vendored = { workspace = true }
6666

6767
[dev-dependencies]
6868
tempfile = "3"
69+
tracing-subscriber = { workspace = true }
6970
rcgen = { workspace = true }
71+
tokio-rustls = { workspace = true }
7072

7173
[lints]
7274
workspace = true

‎crates/openshell-core/build.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
5151
// the build process environment. With a caller-supplied PROTOC, leave both
5252
// to prost.
5353
let mut proto_config = tonic_prost_build::Config::new();
54+
// Keep optional grant evidence off sandbox lifecycle futures' stacks.
55+
proto_config.boxed(".openshell.v1.SandboxStatus.last_token_grant_failure");
5456
let proto_include = if use_vendored_protoc {
5557
proto_config.protoc_executable(protoc_bin_vendored::protoc_bin_path()?);
5658
Some(protoc_bin_vendored::include_path()?)

0 commit comments

Comments
 (0)