diff --git a/crates/openshell-core/src/secrets.rs b/crates/openshell-core/src/secrets.rs index 2e51c9de28..d22e3a07fb 100644 --- a/crates/openshell-core/src/secrets.rs +++ b/crates/openshell-core/src/secrets.rs @@ -111,9 +111,13 @@ pub struct UnresolvedPlaceholderError { } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum UnresolvedPlaceholderReason { +pub enum UnresolvedPlaceholderReason { + /// No usable credential value exists for the placeholder. Unavailable, + /// The credential exists but is bound to a different request endpoint. EndpointMismatch, + /// Workload-controlled fields of a signed request cannot contain placeholders. + ForbiddenInSignedRequest, } impl UnresolvedPlaceholderError { @@ -126,6 +130,16 @@ impl UnresolvedPlaceholderError { pub fn is_endpoint_mismatch(&self) -> bool { self.reason == UnresolvedPlaceholderReason::EndpointMismatch } + + #[must_use] + pub fn is_forbidden_in_signed_request(&self) -> bool { + self.reason == UnresolvedPlaceholderReason::ForbiddenInSignedRequest + } + + #[must_use] + pub fn reason(&self) -> UnresolvedPlaceholderReason { + self.reason + } } impl fmt::Display for UnresolvedPlaceholderError { @@ -138,6 +152,8 @@ impl fmt::Display for UnresolvedPlaceholderError { "credential placeholder could not be resolved", UnresolvedPlaceholderReason::EndpointMismatch => "credential is not authorized for the request endpoint", + UnresolvedPlaceholderReason::ForbiddenInSignedRequest => + "credential placeholders are not allowed in signed request fields", }, self.location, ) @@ -184,6 +200,7 @@ pub struct RewriteTargetResult { pub struct SecretResolver { by_placeholder: HashMap, denied_env_keys: HashSet, + forbid_placeholders_in_signed_request: bool, identity_bound_env_keys: HashSet, revision_fallback_allowed_revisions: HashMap>>, } @@ -293,6 +310,7 @@ impl SecretResolver { (!by_placeholder.is_empty()).then_some(Self { by_placeholder, denied_env_keys: HashSet::new(), + forbid_placeholders_in_signed_request: false, identity_bound_env_keys: HashSet::new(), revision_fallback_allowed_revisions: HashMap::new(), }) @@ -348,6 +366,7 @@ impl SecretResolver { Some(Self { by_placeholder, denied_env_keys: HashSet::new(), + forbid_placeholders_in_signed_request: false, identity_bound_env_keys: HashSet::new(), revision_fallback_allowed_revisions: HashMap::new(), }), @@ -358,21 +377,27 @@ impl SecretResolver { pub fn merge<'a>(resolvers: impl IntoIterator) -> Option { let mut by_placeholder = HashMap::new(); let mut denied_env_keys = HashSet::new(); + let mut forbid_placeholders_in_signed_request = false; let mut identity_bound_env_keys = HashSet::new(); let mut revision_fallback_allowed_revisions = HashMap::new(); for resolver in resolvers { by_placeholder.extend(resolver.by_placeholder.clone()); denied_env_keys.extend(resolver.denied_env_keys.iter().cloned()); + forbid_placeholders_in_signed_request |= resolver.forbid_placeholders_in_signed_request; identity_bound_env_keys.extend(resolver.identity_bound_env_keys.iter().cloned()); revision_fallback_allowed_revisions .extend(resolver.revision_fallback_allowed_revisions.clone()); } - if by_placeholder.is_empty() { + if forbid_placeholders_in_signed_request { + by_placeholder.clear(); + } + if by_placeholder.is_empty() && !forbid_placeholders_in_signed_request { None } else { Some(Self { by_placeholder, denied_env_keys, + forbid_placeholders_in_signed_request, identity_bound_env_keys, revision_fallback_allowed_revisions, }) @@ -406,17 +431,35 @@ impl SecretResolver { Self { by_placeholder, denied_env_keys, + forbid_placeholders_in_signed_request: self.forbid_placeholders_in_signed_request, identity_bound_env_keys: bound_keys.clone(), revision_fallback_allowed_revisions, } } + /// Return a resolver view that rejects every credential placeholder. + /// + /// Trusted transforms can keep the original resolver while using this view + /// to scan untrusted request content without materializing any secret. + #[must_use] + pub fn without_resolvable_placeholders(&self) -> Self { + Self { + by_placeholder: HashMap::new(), + denied_env_keys: self.denied_env_keys.clone(), + forbid_placeholders_in_signed_request: true, + identity_bound_env_keys: self.identity_bound_env_keys.clone(), + revision_fallback_allowed_revisions: self.revision_fallback_allowed_revisions.clone(), + } + } + fn unresolved_for( &self, location: &'static str, placeholder: &str, ) -> UnresolvedPlaceholderError { - let reason = if placeholder_env_key(placeholder) + let reason = if self.forbid_placeholders_in_signed_request { + UnresolvedPlaceholderReason::ForbiddenInSignedRequest + } else if placeholder_env_key(placeholder) .is_some_and(|key| self.denied_env_keys.contains(key)) { UnresolvedPlaceholderReason::EndpointMismatch @@ -481,6 +524,12 @@ impl SecretResolver { key: &str, location: &'static str, ) -> Result, UnresolvedPlaceholderError> { + if self.forbid_placeholders_in_signed_request { + return Err(UnresolvedPlaceholderError { + location, + reason: UnresolvedPlaceholderReason::ForbiddenInSignedRequest, + }); + } if self.denied_env_keys.contains(key) { return Err(UnresolvedPlaceholderError { location, @@ -504,11 +553,11 @@ impl SecretResolver { value: &str, location: &'static str, ) -> Result, UnresolvedPlaceholderError> { - if placeholder_env_key(value).is_some_and(|key| self.denied_env_keys.contains(key)) { - return Err(UnresolvedPlaceholderError { - location, - reason: UnresolvedPlaceholderReason::EndpointMismatch, - }); + if (self.forbid_placeholders_in_signed_request + && contains_reserved_credential_marker(value)) + || placeholder_env_key(value).is_some_and(|key| self.denied_env_keys.contains(key)) + { + return Err(self.unresolved_for(location, value)); } Ok(self.resolve_placeholder(value)) } @@ -586,7 +635,7 @@ impl SecretResolver { if text[abs_start..].starts_with(PLACEHOLDER_PREFIX) { let Some((token_end, token)) = self.credential_token_at(text, abs_start) else { - return Err(unresolved(location)); + return Err(self.unresolved_for(location, &text[abs_start..])); }; let Some(secret) = self.resolve_placeholder(token) else { return Err(self.unresolved_for(location, token)); @@ -607,11 +656,11 @@ impl SecretResolver { continue; } - return Err(unresolved(location)); + return Err(self.unresolved_for(location, &text[abs_start..])); } if contains_raw_reserved_marker(&rewritten) { - return Err(unresolved(location)); + return Err(self.unresolved_for(location, &rewritten)); } *text = rewritten; @@ -1154,7 +1203,7 @@ fn rewrite_path_segment( let Some((token_end, full_placeholder)) = canonical_token_at(segment, abs_start) .or_else(|| alias_token_at(segment, abs_start)) else { - return Err(unresolved("path")); + return Err(resolver.unresolved_for("path", &segment[abs_start..])); }; if let Some(secret) = resolver.resolve_placeholder(full_placeholder) { validate_credential_for_path(secret).map_err(|reason| { @@ -1205,7 +1254,7 @@ fn rewrite_uri_query_params( let replacements = resolver.rewrite_text_placeholders(&mut rewritten, "query_param")?; if replacements == 0 || contains_raw_reserved_marker(&rewritten) { - return Err(unresolved("query_param")); + return Err(resolver.unresolved_for("query_param", &decoded_value)); } resolved_params.push(format!("{key}={}", percent_encode_query(&rewritten))); redacted_params.push(format!("{key}=[CREDENTIAL]")); @@ -1304,7 +1353,7 @@ pub fn rewrite_http_header_block( // provider-shaped aliases in both raw and percent-decoded header bytes. let output_header = String::from_utf8_lossy(&output[..output.len().min(header_end + 256)]); if contains_reserved_credential_marker(&output_header) { - return Err(unresolved("header")); + return Err(resolver.unresolved_for("header", &output_header)); } Ok(RewriteResult { @@ -1520,6 +1569,130 @@ mod tests { ); } + #[test] + fn non_resolving_view_separates_request_content_from_trusted_resolution() { + const ENV_KEYS: &[&str] = &[ + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "API_TOKEN", + ]; + let provider_env: HashMap = [ + ("AWS_ACCESS_KEY_ID".to_string(), "access-value".to_string()), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "secret-value".to_string(), + ), + ("AWS_SESSION_TOKEN".to_string(), "session-value".to_string()), + ("API_TOKEN".to_string(), "allowed-value".to_string()), + ] + .into_iter() + .collect(); + let stable_handles = ENV_KEYS + .iter() + .enumerate() + .map(|(index, key)| ((*key).to_string(), format!("{:064x}", index + 1))) + .collect::>(); + let (child_env, generation_resolver, current_resolver) = + SecretResolver::from_provider_env_for_current_revision( + provider_env.clone(), + HashMap::new(), + 7, + ); + let (stable_child_env, stable_generation_resolver, stable_current_resolver) = + SecretResolver::from_provider_env_for_current_revision_with_stable_handles( + provider_env, + HashMap::new(), + 7, + &stable_handles, + ); + let resolver = SecretResolver::merge( + [ + generation_resolver.as_ref(), + current_resolver.as_ref(), + stable_generation_resolver.as_ref(), + stable_current_resolver.as_ref(), + ] + .into_iter() + .flatten(), + ) + .expect("combined resolver"); + let request_resolver = resolver.without_resolvable_placeholders(); + + for key in ENV_KEYS { + assert!( + resolver + .resolve_current_env_key_checked(key, "trusted-transform") + .expect("trusted lookup") + .is_some() + ); + + for placeholder in [ + placeholder_for_env_key(key), + child_env[*key].clone(), + stable_child_env[*key].clone(), + format!("provider-{PROVIDER_ALIAS_MARKER}{key}"), + ] { + assert!( + resolver.resolve_placeholder(&placeholder).is_some(), + "test placeholder must resolve before it is denied" + ); + + let error = request_resolver + .rewrite_header_value(&placeholder) + .expect_err("credential must be unavailable to request content"); + assert!(error.is_forbidden_in_signed_request()); + assert!(!error.is_endpoint_mismatch()); + + let basic = + base64::engine::general_purpose::STANDARD.encode(format!("user:{placeholder}")); + let error = request_resolver + .rewrite_header_value(&format!("Basic {basic}")) + .expect_err("credential must be denied inside Basic authentication"); + assert!(error.is_forbidden_in_signed_request()); + + for target in [format!("/{placeholder}"), format!("/?value={placeholder}")] { + let raw = format!("GET {target} HTTP/1.1\r\nHost: example.com\r\n\r\n"); + let error = rewrite_http_header_block(raw.as_bytes(), Some(&request_resolver)) + .expect_err("credential must be rejected in request target"); + assert!(error.is_forbidden_in_signed_request()); + } + } + } + + let error = request_resolver + .rewrite_header_value("openshell:resolve:env:UNKNOWN") + .expect_err("unknown placeholders are also forbidden in signed request fields"); + assert_eq!( + error.reason(), + UnresolvedPlaceholderReason::ForbiddenInSignedRequest + ); + assert_eq!( + error.to_string(), + "credential placeholders are not allowed in signed request fields in header" + ); + + let merged = SecretResolver::merge([&resolver, &request_resolver]) + .expect("a merged non-resolving view remains available for validation"); + let error = merged + .rewrite_header_value(&child_env["AWS_SECRET_ACCESS_KEY"]) + .expect_err("merging cannot make a signed-request view resolve secrets again"); + assert!(error.is_forbidden_in_signed_request()); + + let bound_keys = ENV_KEYS + .iter() + .map(|key| (*key).to_string()) + .collect::>(); + let scoped_request_resolver = + request_resolver.scoped_to_env_keys(&bound_keys, &bound_keys, HashMap::new()); + let merged = SecretResolver::merge([&resolver, &scoped_request_resolver]) + .expect("a scoped non-resolving view remains available for validation"); + let error = merged + .rewrite_header_value(&child_env["AWS_SECRET_ACCESS_KEY"]) + .expect_err("scoping and merging cannot re-enable signed-request placeholders"); + assert!(error.is_forbidden_in_signed_request()); + } + #[test] fn provider_env_rejects_revision_namespace_keys() { let (child_env, resolver) = SecretResolver::from_provider_env( diff --git a/crates/openshell-supervisor-network/src/l7/mod.rs b/crates/openshell-supervisor-network/src/l7/mod.rs index 3f26114998..575d8901f3 100644 --- a/crates/openshell-supervisor-network/src/l7/mod.rs +++ b/crates/openshell-supervisor-network/src/l7/mod.rs @@ -292,6 +292,19 @@ impl EndpointObserver { self.observe(EndpointResult::CredentialUnavailable); } } + + /// Report a typed credential-resolution failure without conflating a + /// signed-request placeholder denial with endpoint binding. + pub(crate) fn observe_credential_resolution_failure( + &self, + error: &openshell_core::secrets::UnresolvedPlaceholderError, + ) { + if error.is_endpoint_mismatch() || error.is_forbidden_in_signed_request() { + self.observe(EndpointResult::PolicyDenied); + } else if self.uses_provider_credentials { + self.observe(EndpointResult::CredentialUnavailable); + } + } } /// Result of an L7 policy decision for a single request. @@ -2013,6 +2026,26 @@ mod tests { .. } )); + + let (_, resolver) = openshell_core::secrets::SecretResolver::from_provider_env( + std::iter::once(("AWS_SECRET_ACCESS_KEY".to_string(), "secret".to_string())).collect(), + ); + let request_resolver = resolver + .expect("resolver") + .without_resolvable_placeholders(); + let error = request_resolver + .rewrite_header_value("openshell:resolve:env:AWS_SECRET_ACCESS_KEY") + .expect_err("signed request placeholder denial"); + let observer = EndpointObserver::begin(Some(&sender), &config).expect("begin observation"); + observer.observe_credential_resolution_failure(&error); + + assert!(matches!( + receiver.recv().await.expect("signed request observation"), + EndpointStatusCommand::Observe { + result: EndpointResult::PolicyDenied, + .. + } + )); } #[tokio::test] diff --git a/crates/openshell-supervisor-network/src/l7/path.rs b/crates/openshell-supervisor-network/src/l7/path.rs index 078248f058..8d7278b854 100644 --- a/crates/openshell-supervisor-network/src/l7/path.rs +++ b/crates/openshell-supervisor-network/src/l7/path.rs @@ -208,7 +208,10 @@ pub fn canonicalize_request_target( // any byte that must be percent-encoded in the pchar set. let canonical = build_canonical_path(&resolved, decoded.last().copied() == Some(b'/'), *opts); - let rewritten = canonical != raw_path; + // L7 forwarding always uses origin-form. Converting a validated + // absolute-form target keeps policy evaluation and the upstream request + // line on the same canonical path. + let rewritten = canonical != raw_path || absolute_form_uri.is_some(); Ok(( CanonicalPath { path: canonical, @@ -498,6 +501,15 @@ mod tests { assert_eq!(canon("http://host/a/../b").unwrap(), "/b"); assert_eq!(canon("https://host").unwrap(), "/"); assert_eq!(canon("http://host:443/foo").unwrap(), "/foo"); + + let (path, query) = canonicalize_request_target( + "https://s3.us-east-1.amazonaws.com/bucket/key?versionId=1", + &CanonicalizeOptions::default(), + ) + .expect("absolute-form conversion preserves canonical path bytes"); + assert_eq!(path.path, "/bucket/key"); + assert!(path.rewritten, "the outbound request must use origin-form"); + assert_eq!(query.as_deref(), Some("versionId=1")); } #[test] diff --git a/crates/openshell-supervisor-network/src/l7/relay.rs b/crates/openshell-supervisor-network/src/l7/relay.rs index 9603f65a02..9ba20ca20d 100644 --- a/crates/openshell-supervisor-network/src/l7/relay.rs +++ b/crates/openshell-supervisor-network/src/l7/relay.rs @@ -23,7 +23,7 @@ use crate::opa::{PolicyGenerationGuard, TunnelPolicyEngine}; use miette::{IntoDiagnostic, Result, miette}; use openshell_core::activity::{ActivitySender, try_record_activity}; use openshell_core::endpoint_status::{EndpointObservationSender, EndpointResult}; -use openshell_core::secrets::{self, SecretResolver}; +use openshell_core::secrets::{self, SecretResolver, UnresolvedPlaceholderReason}; use openshell_ocsf::{ ActionId, ActivityId, DetectionFindingBuilder, DispositionId, Endpoint, FindingInfo, HttpActivityBuilder, HttpRequest, NetworkActivityBuilder, SeverityId, StatusId, Url as OcsfUrl, @@ -514,9 +514,40 @@ fn build_request_authority_mismatch_finding(ctx: &L7EvalContext) -> openshell_oc fn build_credential_resolution_event( ctx: &L7EvalContext, method: &str, - endpoint_mismatch: bool, + reason: UnresolvedPlaceholderReason, ) -> openshell_ocsf::OcsfEvent { - let response_code = if endpoint_mismatch { 403 } else { 500 }; + let (response_code, severity, engine, message, status_detail) = match reason { + UnresolvedPlaceholderReason::EndpointMismatch => ( + 403, + SeverityId::High, + "credential-binding", + format!( + "Credential use denied: credential is not authorized for {}:{}", + ctx.host, ctx.port + ), + "credential_endpoint_mismatch", + ), + UnresolvedPlaceholderReason::ForbiddenInSignedRequest => ( + 403, + SeverityId::Medium, + "credential-signing", + format!( + "Credential placeholder denied in signed request fields for {}:{}", + ctx.host, ctx.port + ), + "credential_placeholder_forbidden_in_signed_request", + ), + UnresolvedPlaceholderReason::Unavailable => ( + 500, + SeverityId::Medium, + "credential-binding", + format!( + "Credential use denied: credential is unavailable for {}:{}", + ctx.host, ctx.port + ), + "credential_unavailable", + ), + }; HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) .activity(ActivityId::for_http_method(method)) .http_request(method_only_request(method)) @@ -525,30 +556,12 @@ fn build_credential_resolution_event( }) .action(ActionId::Denied) .disposition(DispositionId::Blocked) - .severity(if endpoint_mismatch { - SeverityId::High - } else { - SeverityId::Medium - }) + .severity(severity) .status(StatusId::Failure) .dst_endpoint(Endpoint::from_domain(&ctx.host, ctx.port)) - .firewall_rule(&ctx.policy_name, "credential-binding") - .message(if endpoint_mismatch { - format!( - "Credential use denied: credential is not authorized for {}:{}", - ctx.host, ctx.port - ) - } else { - format!( - "Credential use denied: credential is unavailable for {}:{}", - ctx.host, ctx.port - ) - }) - .status_detail(if endpoint_mismatch { - "credential_endpoint_mismatch" - } else { - "credential_unavailable" - }) + .firewall_rule(&ctx.policy_name, engine) + .message(message) + .status_detail(status_detail) .build() } @@ -570,16 +583,19 @@ pub(crate) async fn reject_credential_resolution( where W: AsyncWrite + Unpin, { - let endpoint_mismatch = error.is_endpoint_mismatch(); - let status = if endpoint_mismatch { - "403 Forbidden" - } else { - "500 Internal Server Error" - }; - let body = if endpoint_mismatch { - r#"{"error":"credential_endpoint_mismatch","message":"Credential is not authorized for this request endpoint"}"# - } else { - r#"{"error":"credential_unavailable","message":"Credential placeholder could not be resolved"}"# + let (status, body) = match error.reason() { + UnresolvedPlaceholderReason::EndpointMismatch => ( + "403 Forbidden", + r#"{"error":"credential_endpoint_mismatch","message":"Credential is not authorized for this request endpoint"}"#, + ), + UnresolvedPlaceholderReason::ForbiddenInSignedRequest => ( + "403 Forbidden", + r#"{"error":"credential_placeholder_forbidden_in_signed_request","message":"Credential placeholders are not allowed in signed request fields"}"#, + ), + UnresolvedPlaceholderReason::Unavailable => ( + "500 Internal Server Error", + r#"{"error":"credential_unavailable","message":"Credential placeholder could not be resolved"}"#, + ), }; let response = format!( "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", @@ -594,10 +610,10 @@ where ocsf_emit!(build_credential_resolution_event( ctx, method, - endpoint_mismatch + error.reason() )); - if endpoint_mismatch { + if error.is_endpoint_mismatch() { ocsf_emit!(build_credential_endpoint_mismatch_finding(ctx)); } Ok(()) @@ -806,7 +822,7 @@ where } else if let Some(error) = report.downcast_ref::() { if let Some(observer) = observer { - observer.observe_credential_failure(error.is_endpoint_mismatch()); + observer.observe_credential_resolution_failure(error); } reject_credential_resolution(client, ctx, &request.action, error).await?; Ok(None) @@ -1325,7 +1341,7 @@ where Ok(target) => target, Err(error) => { if let Some(observer) = observer.as_ref() { - observer.observe_credential_failure(error.is_endpoint_mismatch()); + observer.observe_credential_resolution_failure(&error); } reject_credential_resolution(client, ctx, &req.action, &error).await?; return Ok(()); @@ -2497,7 +2513,7 @@ where Ok(target) => target, Err(error) => { if let Some(observer) = observer.as_ref() { - observer.observe_credential_failure(error.is_endpoint_mismatch()); + observer.observe_credential_resolution_failure(&error); } reject_credential_resolution(client, ctx, &req.action, &error).await?; return Ok(()); @@ -3997,15 +4013,32 @@ mod tests { 403, ), ( - build_credential_resolution_event(&ctx, "POST", true), + build_credential_resolution_event( + &ctx, + "POST", + UnresolvedPlaceholderReason::EndpointMismatch, + ), "POST", 403, ), ( - build_credential_resolution_event(&ctx, "HEAD", false), + build_credential_resolution_event( + &ctx, + "HEAD", + UnresolvedPlaceholderReason::Unavailable, + ), "HEAD", 500, ), + ( + build_credential_resolution_event( + &ctx, + "PUT", + UnresolvedPlaceholderReason::ForbiddenInSignedRequest, + ), + "PUT", + 403, + ), ] { let json = event.to_json().unwrap(); assert_eq!(json["class_uid"], 4002); @@ -4019,6 +4052,48 @@ mod tests { } } + #[tokio::test] + async fn signed_request_placeholder_denial_is_typed_without_endpoint_mismatch_alert() { + let (_, resolver) = SecretResolver::from_provider_env(TestHashMap::from([( + "AWS_SECRET_ACCESS_KEY".to_string(), + "secret".to_string(), + )])); + let request_resolver = resolver + .expect("resolver") + .without_resolvable_placeholders(); + let error = request_resolver + .rewrite_header_value("openshell:resolve:env:AWS_SECRET_ACCESS_KEY") + .expect_err("signed request fields must reject placeholders"); + assert!(error.is_forbidden_in_signed_request()); + assert!(!error.is_endpoint_mismatch()); + + let ctx = L7EvalContext { + host: "s3.example.test".into(), + port: 443, + policy_name: "s3".into(), + ..Default::default() + }; + let (mut relay, mut caller) = tokio::io::duplex(4096); + reject_credential_resolution(&mut relay, &ctx, "PUT", &error) + .await + .expect("local rejection"); + drop(relay); + let mut response = String::new(); + caller.read_to_string(&mut response).await.unwrap(); + assert!(response.starts_with("HTTP/1.1 403 Forbidden")); + assert!(response.contains("credential_placeholder_forbidden_in_signed_request")); + assert!(!response.contains("credential_endpoint_mismatch")); + + let event = build_credential_resolution_event(&ctx, "PUT", error.reason()); + assert_eq!(event.base().severity, SeverityId::Medium); + let json = event.to_json().unwrap(); + assert_eq!( + json["status_detail"], + "credential_placeholder_forbidden_in_signed_request" + ); + assert_eq!(json["http_response"]["code"], 403); + } + #[test] fn websocket_preflight_input_carries_real_sandbox_name() { let sandbox = openshell_ocsf::EventContext { @@ -4197,9 +4272,13 @@ mod tests { "credential mismatch must not write upstream" ); - let activity = build_credential_resolution_event(ctx, "GET", true) - .to_json() - .expect("serialize credential mismatch activity"); + let activity = build_credential_resolution_event( + ctx, + "GET", + UnresolvedPlaceholderReason::EndpointMismatch, + ) + .to_json() + .expect("serialize credential mismatch activity"); assert_eq!(activity["status_detail"], "credential_endpoint_mismatch"); assert_eq!(activity["action"], "Denied"); assert_eq!(activity["disposition"], "Blocked"); @@ -4259,9 +4338,13 @@ mod tests { "credential mismatch must not write upstream" ); - let activity = build_credential_resolution_event(&ctx, "GET", true) - .to_json() - .unwrap(); + let activity = build_credential_resolution_event( + &ctx, + "GET", + UnresolvedPlaceholderReason::EndpointMismatch, + ) + .to_json() + .unwrap(); assert_eq!(activity["status_detail"], "credential_endpoint_mismatch"); assert_eq!(activity["action"], "Denied"); assert_eq!(activity["disposition"], "Blocked"); @@ -8858,6 +8941,171 @@ network_policies: - { path: /usr/bin/node } "#; + const S3_CANONICALIZATION_POLICY: &str = r#" +network_policies: + route_api: + name: route_api + endpoints: + - host: gateway.example.test + port: 443 + path: /api/** + protocol: rest + enforcement: enforce + rules: + - allow: + method: GET + path: "/api/**" + - host: gateway.example.test + port: 443 + path: /bucket/** + protocol: rest + enforcement: enforce + credential_signing: sigv4 + signing_service: s3 + signing_region: us-east-1 + rules: + - allow: + method: GET + path: "/bucket/**" + binaries: + - { path: /usr/bin/node } +"#; + + async fn relay_mixed_s3_target(target: &str) -> (String, String) { + let engine = OpaEngine::from_strings(TEST_POLICY, S3_CANONICALIZATION_POLICY).unwrap(); + let input = NetworkInput { + host: "gateway.example.test".into(), + port: 443, + binary_path: PathBuf::from("/usr/bin/node"), + binary_sha256: "unused".into(), + ancestors: vec![], + cmdline_paths: vec![], + }; + let (endpoints, generation) = engine + .query_endpoint_configs_with_generation(&input) + .unwrap(); + let configs: Vec<_> = endpoints + .iter() + .map(|endpoint| crate::l7::parse_l7_config(endpoint).unwrap()) + .collect(); + let tunnel_engine = engine.clone_engine_for_tunnel(generation).unwrap(); + let (_, resolver) = SecretResolver::from_provider_env(TestHashMap::from([ + ( + "AWS_ACCESS_KEY_ID".to_string(), + "test-access-key".to_string(), + ), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "test-secret-key".to_string(), + ), + ])); + let ctx = L7EvalContext { + secret_resolver: resolver.map(Arc::new), + ..encoded_slash_scoping_ctx() + }; + let (mut app, mut relay_client) = tokio::io::duplex(8192); + let (mut relay_upstream, mut upstream) = tokio::io::duplex(8192); + let relay = tokio::spawn(async move { + relay_with_route_selection( + &configs, + tunnel_engine, + &mut relay_client, + &mut relay_upstream, + &ctx, + ) + .await + }); + let server = tokio::spawn(async move { + let mut forwarded = Vec::new(); + let mut chunk = [0u8; 512]; + while let Ok(Ok(count)) = tokio::time::timeout( + std::time::Duration::from_millis(100), + upstream.read(&mut chunk), + ) + .await + { + if count == 0 { + break; + } + forwarded.extend_from_slice(&chunk[..count]); + if forwarded.ends_with(b"\r\n\r\n") { + break; + } + } + if !forwarded.is_empty() { + upstream + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") + .await + .unwrap(); + } + String::from_utf8(forwarded).unwrap() + }); + + app.write_all( + format!( + "GET {target} HTTP/1.1\r\nHost: gateway.example.test\r\nConnection: close\r\n\r\n" + ) + .as_bytes(), + ) + .await + .unwrap(); + let response = tokio::time::timeout( + std::time::Duration::from_secs(1), + read_http_headers(&mut app), + ) + .await + .expect("route-selected relay must answer"); + drop(app); + + let forwarded = server.await.unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(1), relay) + .await + .expect("route-selected relay must finish") + .unwrap() + .unwrap(); + (String::from_utf8(response).unwrap(), forwarded) + } + + #[tokio::test] + async fn route_selected_s3_canonicalizes_before_signing() { + let (response, forwarded) = relay_mixed_s3_target("/api//v1").await; + assert!(response.starts_with("HTTP/1.1 200 OK"), "{response}"); + assert!(forwarded.starts_with("GET /api/v1 HTTP/1.1"), "{forwarded}"); + assert!(!forwarded.to_ascii_lowercase().contains("authorization:")); + + let (response, forwarded) = relay_mixed_s3_target("/api/../bucket/key").await; + assert!(response.starts_with("HTTP/1.1 200 OK"), "{response}"); + assert!( + forwarded.starts_with("GET /bucket/key HTTP/1.1"), + "{forwarded}" + ); + let signed = forwarded.to_ascii_lowercase(); + assert!(signed.contains("authorization: aws4-hmac-sha256")); + assert!(signed.contains("x-amz-")); + assert!(!signed.contains("/api/../bucket/key")); + + let (response, forwarded) = relay_mixed_s3_target("/bucket/../api/v1").await; + assert!(response.starts_with("HTTP/1.1 200 OK"), "{response}"); + assert!(forwarded.starts_with("GET /api/v1 HTTP/1.1"), "{forwarded}"); + let forwarded = forwarded.to_ascii_lowercase(); + assert!(!forwarded.contains("authorization:")); + assert!(!forwarded.contains("x-amz-")); + } + + #[tokio::test] + async fn route_selected_s3_noncanonical_target_rejects_query_placeholder_before_upstream() { + let (response, forwarded) = relay_mixed_s3_target( + "/api/../bucket/key?leak=openshell%3Aresolve%3Aenv%3AAWS_SECRET_ACCESS_KEY", + ) + .await; + + assert!(response.starts_with("HTTP/1.1 403 Forbidden"), "{response}"); + assert!( + forwarded.is_empty(), + "placeholder-bearing signed request reached upstream: {forwarded}" + ); + } + fn encoded_slash_scoping_configs() -> Vec { let rest = |path: &str, allow_encoded_slash: bool| L7EndpointConfig { protocol: L7Protocol::Rest, diff --git a/crates/openshell-supervisor-network/src/l7/rest.rs b/crates/openshell-supervisor-network/src/l7/rest.rs index 7b85942666..51bb437aac 100644 --- a/crates/openshell-supervisor-network/src/l7/rest.rs +++ b/crates/openshell-supervisor-network/src/l7/rest.rs @@ -945,8 +945,18 @@ where offered_subprotocols: request.subprotocols.clone(), }); + // The signer may resolve credentials selected by trusted supervisor code, + // but workload-controlled SigV4 request fields must not materialize secrets. + let sigv4_request_resolver = if options.credential_signing.is_sigv4() { + options + .resolver + .map(SecretResolver::without_resolvable_placeholders) + } else { + None + }; + let request_resolver = sigv4_request_resolver.as_ref().or(options.resolver); let rewrite_result = - rewrite_http_header_block(&header_bytes, options.resolver).map_err(miette::Report::new)?; + rewrite_http_header_block(&header_bytes, request_resolver).map_err(miette::Report::new)?; if let Some(validation) = options.mcp_request_validation { // Header credential resolution and hop-by-hop cleanup must finish @@ -1268,6 +1278,7 @@ where websocket_extensions: options.websocket_extensions, websocket: websocket_response, client_requested_upgrade, + suppress_signed_non_success_details: options.credential_signing.is_sigv4(), observer, }, response_middleware, @@ -5840,6 +5851,39 @@ mod tests { ); } + #[tokio::test] + async fn s3_sigv4_parser_canonicalizes_object_key_before_signing() { + for (target, canonical) in [ + ("/bucket/a//b", "/bucket/a/b"), + ("/bucket/a/../b", "/bucket/b"), + ("/bucket/a%3Db", "/bucket/a=b"), + ("/bucket/key;version=1", "/bucket/key"), + ] { + let (mut client, mut peer) = tokio::io::duplex(4096); + peer.write_all( + format!("GET {target} HTTP/1.1\r\nHost: s3.us-east-1.amazonaws.com\r\n\r\n") + .as_bytes(), + ) + .await + .unwrap(); + + let request = parse_http_request( + &mut client, + &crate::l7::path::CanonicalizeOptions::default(), + ) + .await + .expect("S3 target must parse") + .expect("request must be present"); + assert_eq!(request.target, canonical); + assert!( + request + .raw_header + .starts_with(format!("GET {canonical} HTTP/1.1\r\n").as_bytes()), + "forwarded request line must use the policy-evaluated canonical path" + ); + } + } + #[tokio::test] async fn parse_http_request_rejects_absolute_authority_mismatched_with_host() { let (mut client, mut peer) = tokio::io::duplex(1024); @@ -5863,6 +5907,30 @@ mod tests { ); } + #[tokio::test] + async fn s3_sigv4_parser_rewrites_absolute_form_to_canonical_origin_form() { + let (mut client, mut peer) = tokio::io::duplex(1024); + peer.write_all( + b"GET https://s3.us-east-1.amazonaws.com/bucket/a+b%20c?versionId=1 HTTP/1.1\r\nHost: s3.us-east-1.amazonaws.com\r\n\r\n", + ) + .await + .unwrap(); + + let request = parse_http_request( + &mut client, + &crate::l7::path::CanonicalizeOptions::default(), + ) + .await + .expect("absolute-form request must parse") + .expect("request must be present"); + assert_eq!(request.target, "/bucket/a+b%20c"); + assert!( + request + .raw_header + .starts_with(b"GET /bucket/a+b%20c?versionId=1 HTTP/1.1\r\n") + ); + } + #[test] fn origin_form_targets_with_embedded_urls_use_host_authority() { let host: http::uri::Authority = "api.example.test".parse().unwrap(); @@ -10456,6 +10524,183 @@ mod tests { assert!(!forwarded.contains("openshell:resolve:env:")); } + #[tokio::test] + async fn relay_sigv4_rejects_signing_credential_in_request_header() { + let (child_env, generation_resolver, current_resolver) = + SecretResolver::from_provider_env_for_current_revision( + [ + ("AWS_ACCESS_KEY_ID".to_string(), "AKIATESTKEY".to_string()), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "test-secret-key".to_string(), + ), + ] + .into_iter() + .collect(), + HashMap::new(), + 7, + ); + let resolver = SecretResolver::merge( + [generation_resolver.as_ref(), current_resolver.as_ref()] + .into_iter() + .flatten(), + ) + .expect("combined resolver"); + let placeholder = &child_env["AWS_SECRET_ACCESS_KEY"]; + let raw_header = format!( + "GET / HTTP/1.1\r\n\ + Host: s3.us-east-1.amazonaws.com\r\n\ + x-amz-meta-value: {placeholder}\r\n\r\n" + ) + .into_bytes(); + let req = L7Request { + action: "GET".to_string(), + target: "/".to_string(), + query_params: HashMap::new(), + raw_header, + body_length: BodyLength::None, + }; + let (mut proxy_to_upstream, mut upstream_side) = tokio::io::duplex(8192); + let (mut _app_side, mut proxy_to_client) = tokio::io::duplex(8192); + + let upstream_task = tokio::spawn(async move { + let mut forwarded = vec![0; 8192]; + let mut total = 0; + loop { + let read = upstream_side.read(&mut forwarded[total..]).await.unwrap(); + if read == 0 { + break; + } + total += read; + if forwarded[..total].windows(4).any(|w| w == b"\r\n\r\n") { + upstream_side + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n") + .await + .unwrap(); + break; + } + } + forwarded.truncate(total); + forwarded + }); + + let result = relay_http_request_with_options_guarded( + &req, + &mut proxy_to_client, + &mut proxy_to_upstream, + RelayRequestOptions { + resolver: Some(&resolver), + credential_signing: crate::l7::CredentialSigning::SigV4NoBody, + signing_service: "s3", + signing_region: "us-east-1", + host: "s3.us-east-1.amazonaws.com", + port: 443, + ..Default::default() + }, + ) + .await; + drop(proxy_to_upstream); + let forwarded = upstream_task.await.expect("upstream task"); + + let error = result.expect_err("signing credentials in request content must be rejected"); + assert!( + error + .downcast_ref::() + .is_some_and( + openshell_core::secrets::UnresolvedPlaceholderError::is_forbidden_in_signed_request, + ) + ); + assert!(forwarded.is_empty(), "rejected request reached upstream"); + } + + #[tokio::test] + async fn relay_sigv4_error_response_cannot_reflect_signed_credentials() { + let session_token = "SUPER_SECRET_CONNECT_SESSION_TOKEN"; + let reflected_hex = hex::encode(session_token); + let (_, resolver) = SecretResolver::from_provider_env( + [ + ("AWS_ACCESS_KEY_ID".to_string(), "ASIATESTKEY".to_string()), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "test-secret-key".to_string(), + ), + ("AWS_SESSION_TOKEN".to_string(), session_token.to_string()), + ] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let req = L7Request { + action: "GET".to_string(), + target: "/bucket/key".to_string(), + query_params: HashMap::new(), + raw_header: b"GET /bucket/key HTTP/1.1\r\nHost: s3.us-east-1.amazonaws.com\r\n\r\n" + .to_vec(), + body_length: BodyLength::None, + }; + let (mut proxy_to_upstream, mut upstream_side) = tokio::io::duplex(16 * 1024); + let (mut app_side, mut proxy_to_client) = tokio::io::duplex(16 * 1024); + let token_for_upstream = session_token.to_string(); + let hex_for_upstream = reflected_hex.clone(); + let upstream_task = tokio::spawn(async move { + let request = read_http_header_block(&mut upstream_side).await; + let request = String::from_utf8_lossy(&request); + assert!(request.contains(&format!("x-amz-security-token: {token_for_upstream}\r\n"))); + + let body = format!( + "{token_for_upstream}{hex_for_upstream}" + ); + let head = format!( + "HTTP/1.1 403 Forbidden\r\nX-Reflected: {token_for_upstream}\r\nTransfer-Encoding: chunked\r\n\r\n{:X}\r\n", + body.len() + ); + let split = head.len() / 2; + upstream_side + .write_all(&head.as_bytes()[..split]) + .await + .unwrap(); + tokio::task::yield_now().await; + upstream_side + .write_all(&head.as_bytes()[split..]) + .await + .unwrap(); + upstream_side.write_all(body.as_bytes()).await.unwrap(); + upstream_side.write_all(b"\r\n0\r\n\r\n").await.unwrap(); + }); + + let outcome = tokio::time::timeout( + std::time::Duration::from_secs(5), + relay_http_request_with_options_guarded( + &req, + &mut proxy_to_client, + &mut proxy_to_upstream, + RelayRequestOptions { + resolver: Some(&resolver), + credential_signing: crate::l7::CredentialSigning::SigV4NoBody, + signing_service: "s3", + signing_region: "us-east-1", + host: "s3.us-east-1.amazonaws.com", + port: 443, + ..Default::default() + }, + ), + ) + .await + .expect("relay must not deadlock") + .expect("signed rejection must be replaced"); + assert!(matches!(outcome, RelayOutcome::Consumed)); + upstream_task.await.expect("upstream task"); + + let mut delivered = Vec::new(); + app_side.read_to_end(&mut delivered).await.unwrap(); + let delivered = String::from_utf8(delivered).unwrap(); + assert!(delivered.starts_with("HTTP/1.1 403 Upstream Rejected\r\n")); + assert!(delivered.contains("signed_upstream_rejected")); + assert!(!delivered.contains(session_token)); + assert!(!delivered.contains(&reflected_hex)); + assert!(!delivered.contains("CanonicalRequest")); + } + #[tokio::test] async fn relay_sigv4_body_signing_signs_empty_body_without_content_length() { let (_, resolver) = SecretResolver::from_provider_env( diff --git a/crates/openshell-supervisor-network/src/l7/rest/http_response.rs b/crates/openshell-supervisor-network/src/l7/rest/http_response.rs index 3efc323bdb..b6f4d9692a 100644 --- a/crates/openshell-supervisor-network/src/l7/rest/http_response.rs +++ b/crates/openshell-supervisor-network/src/l7/rest/http_response.rs @@ -26,6 +26,9 @@ pub(super) struct RelayResponseOptions<'a> { pub(super) client_requested_upgrade: bool, pub(super) websocket: Option, pub(super) observer: Option<&'a EndpointObserver>, + /// Hide untrusted upstream non-success details after `OpenShell` signs a + /// request with credentials that must never be reflected to the workload. + pub(super) suppress_signed_non_success_details: bool, } impl Default for RelayResponseOptions<'_> { @@ -35,6 +38,7 @@ impl Default for RelayResponseOptions<'_> { client_requested_upgrade: true, websocket: None, observer: None, + suppress_signed_non_success_details: false, } } } @@ -54,8 +58,9 @@ where let mut buf = Vec::with_capacity(4096); let mut tmp = [0u8; 1024]; - // Forward interim responses unchanged, but retain the final response head - // until response middleware preflight completes. + // Informational responses are not required once the fully buffered request + // has been sent. Drop them for signed requests: diagnostic headers can + // reflect fields from the canonical request before the final response. let mut informational_header_bytes = 0; let header_end = loop { let header_end = buf @@ -69,6 +74,9 @@ where if let Some(observer) = options.observer.as_ref() { observer.observe(EndpointResult::TransportFailed); } + if options.suppress_signed_non_success_details { + return send_suppressed_signed_response(client, request_method, None).await; + } return Err(miette!("HTTP response headers exceed limit")); } @@ -79,11 +87,13 @@ where Some(100 | 102..=199) ) { informational_header_bytes += header_end; - client - .write_all(&buf[..header_end]) - .await - .into_diagnostic()?; - client.flush().await.into_diagnostic()?; + if !options.suppress_signed_non_success_details { + client + .write_all(&buf[..header_end]) + .await + .into_diagnostic()?; + client.flush().await.into_diagnostic()?; + } buf.drain(..header_end); continue; } @@ -95,6 +105,9 @@ where if let Some(observer) = options.observer.as_ref() { observer.observe(EndpointResult::TransportFailed); } + if options.suppress_signed_non_success_details { + return send_suppressed_signed_response(client, request_method, None).await; + } if !buf.is_empty() { client.write_all(&buf).await.into_diagnostic()?; } @@ -108,19 +121,42 @@ where let observed_status = parse_observed_http_status_code(&header_str); if let Some(observer) = options.observer.as_ref() { match observed_status { - Some(status_code) if status_code < 400 => { + Some(status_code) + if status_code < 400 + && !(options.suppress_signed_non_success_details && status_code >= 300) => + { observer.observe(EndpointResult::HttpResponseReceived); } Some(_) => observer.observe(EndpointResult::UpstreamRejected), None => observer.observe(EndpointResult::TransportFailed), } } + if options.suppress_signed_non_success_details { + match observed_status { + Some(101) | None => { + return send_suppressed_signed_response(client, request_method, None).await; + } + Some(status_code) if status_code >= 300 => { + return send_suppressed_signed_response(client, request_method, Some(status_code)) + .await; + } + Some(_) => {} + } + } + let status_code = parse_status_code(&header_str).unwrap_or(200); let server_wants_close = parse_connection_close(&header_str); let http_10_closes_by_default = response_is_http_10(&header_str) && !parse_connection_keep_alive(&header_str); let event_stream = response_is_event_stream(&header_str); - let body_length = parse_body_length(&header_str)?; + let body_length = match parse_body_length(&header_str) { + Ok(body_length) => body_length, + Err(error) if options.suppress_signed_non_success_details => { + debug!(%error, "suppressing malformed signed upstream response"); + return send_suppressed_signed_response(client, request_method, None).await; + } + Err(error) => return Err(error), + }; debug!( status_code, @@ -270,6 +306,28 @@ where finish_response(client, server_wants_close || http_10_closes_by_default).await } +async fn send_suppressed_signed_response( + client: &mut C, + request_method: &str, + upstream_status: Option, +) -> Result +where + C: AsyncWrite + Unpin, +{ + let status_code = upstream_status.filter(|status| (400..=599).contains(status)); + let status_code = status_code.unwrap_or(502); + let body = br#"{"error":"signed_upstream_rejected"}"#; + let head = format!( + "HTTP/1.1 {status_code} Upstream Rejected\r\nContent-Type: application/json\r\nContent-Length: {}\r\nCache-Control: no-store\r\nConnection: close\r\n\r\n", + body.len() + ); + client.write_all(head.as_bytes()).await.into_diagnostic()?; + if !request_method.eq_ignore_ascii_case("HEAD") { + client.write_all(body).await.into_diagnostic()?; + } + finish_response(client, true).await +} + /// Body framing determines when delivery finishes, not whether another /// request is permitted. Signal EOF (including TLS `close_notify`) before the /// caller tears down a closing CONNECT tunnel; waiting for another request @@ -2082,12 +2140,168 @@ pub(super) fn response_is_event_stream(headers: &str) -> bool { #[cfg(test)] mod tests { use super::{ - ResponseFraming, parse_connection_keep_alive, parse_response_head_for_middleware, - response_is_http_10, serialize_response_head, + RelayResponseOptions, ResponseFraming, parse_connection_keep_alive, + parse_response_head_for_middleware, relay_response, response_is_http_10, + serialize_response_head, }; - use crate::l7::provider::BodyLength; + use crate::l7::provider::{BodyLength, RelayOutcome}; use openshell_core::proto::HttpHeader; + async fn relay_with_signed_error_suppression( + request_method: &str, + response: &[u8], + ) -> (RelayOutcome, Vec) { + let mut upstream = response; + let mut delivered = Vec::new(); + let outcome = relay_response( + request_method, + &mut upstream, + &mut delivered, + RelayResponseOptions { + suppress_signed_non_success_details: true, + ..Default::default() + }, + None, + ) + .await + .expect("relay signed response"); + (outcome, delivered) + } + + #[tokio::test] + async fn signed_upstream_errors_are_replaced_for_every_body_framing() { + const REFLECTED: &[u8] = b"REFLECTED_SIGNED_CREDENTIAL"; + let responses = [ + [ + b"HTTP/1.1 403 Forbidden\r\nX-Reflected: ".as_slice(), + REFLECTED, + b"\r\nContent-Length: 27\r\n\r\n".as_slice(), + REFLECTED, + ] + .concat(), + [ + b"HTTP/1.1 403 Forbidden\r\nX-Reflected: ".as_slice(), + REFLECTED, + b"\r\nTransfer-Encoding: chunked\r\n\r\n1B\r\n".as_slice(), + REFLECTED, + b"\r\n0\r\n\r\n".as_slice(), + ] + .concat(), + [ + b"HTTP/1.1 403 Forbidden\r\nConnection: close\r\nX-Reflected: ".as_slice(), + REFLECTED, + b"\r\n\r\n".as_slice(), + REFLECTED, + ] + .concat(), + ]; + + for response in responses { + let (outcome, delivered) = relay_with_signed_error_suppression("GET", &response).await; + assert!(matches!(outcome, RelayOutcome::Consumed)); + assert!(delivered.starts_with(b"HTTP/1.1 403 Upstream Rejected\r\n")); + assert!(delivered.ends_with(b"{\"error\":\"signed_upstream_rejected\"}")); + assert!( + delivered + .windows(REFLECTED.len()) + .all(|window| window != REFLECTED) + ); + assert!( + !delivered + .windows(b"X-Reflected".len()) + .any(|window| window == b"X-Reflected") + ); + } + } + + #[tokio::test] + async fn signed_response_suppression_discards_informational_redirect_and_upgrade_bytes() { + const REFLECTED: &[u8] = b"REFLECTED_SIGNED_CREDENTIAL"; + let responses = [ + [ + b"HTTP/1.1 103 Early Hints\r\nX-Reflected: ".as_slice(), + REFLECTED, + b"\r\n\r\nHTTP/1.1 500 Internal Server Error\r\nContent-Length: 27\r\n\r\n" + .as_slice(), + REFLECTED, + ] + .concat(), + [ + b"HTTP/1.1 307 Temporary Redirect\r\nLocation: https://example.test/?token=" + .as_slice(), + REFLECTED, + b"\r\nContent-Length: 0\r\n\r\n".as_slice(), + ] + .concat(), + [ + b"HTTP/1.1 101 Switching Protocols\r\nUpgrade: reflected\r\nX-Reflected: " + .as_slice(), + REFLECTED, + b"\r\n\r\n".as_slice(), + REFLECTED, + ] + .concat(), + ]; + + for (response, expected_status) in responses.into_iter().zip([500, 502, 502]) { + let (outcome, delivered) = relay_with_signed_error_suppression("GET", &response).await; + assert!(matches!(outcome, RelayOutcome::Consumed)); + assert!(delivered.starts_with( + format!("HTTP/1.1 {expected_status} Upstream Rejected\r\n").as_bytes() + )); + assert!( + delivered + .windows(REFLECTED.len()) + .all(|window| window != REFLECTED) + ); + assert!( + !delivered + .windows(b"103 Early Hints".len()) + .any(|window| window == b"103 Early Hints") + ); + assert!( + !delivered + .windows(b"101 Switching Protocols".len()) + .any(|window| window == b"101 Switching Protocols") + ); + } + } + + #[tokio::test] + async fn signed_error_suppression_replaces_malformed_eof_after_informational_response() { + const REFLECTED: &[u8] = b"REFLECTED_SIGNED_CREDENTIAL"; + let response = [ + b"HTTP/1.1 103 Early Hints\r\nX-Reflected: ".as_slice(), + REFLECTED, + b"\r\n\r\nHTTP/1.1 5".as_slice(), + ] + .concat(); + + let (outcome, delivered) = relay_with_signed_error_suppression("GET", &response).await; + assert!(matches!(outcome, RelayOutcome::Consumed)); + assert!(delivered.starts_with(b"HTTP/1.1 502 Upstream Rejected\r\n")); + assert!( + delivered + .windows(REFLECTED.len()) + .all(|window| window != REFLECTED) + ); + assert!( + !delivered + .windows(b"103 Early Hints".len()) + .any(|window| window == b"103 Early Hints") + ); + } + + #[tokio::test] + async fn signed_success_response_is_unchanged_but_informational_headers_are_dropped() { + let response = b"HTTP/1.1 103 Early Hints\r\nLink: \r\n\r\nHTTP/1.1 200 OK\r\nContent-Length: 4\r\nX-Safe: value\r\n\r\nbody"; + let expected = b"HTTP/1.1 200 OK\r\nContent-Length: 4\r\nX-Safe: value\r\n\r\nbody"; + + let (outcome, delivered) = relay_with_signed_error_suppression("GET", response).await; + assert!(matches!(outcome, RelayOutcome::Reusable)); + assert_eq!(delivered, expected); + } + #[test] fn http_10_connection_persistence() { let default_close = "HTTP/1.0 200 OK\r\nServer: test\r\n\r\n"; diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index e3e2515d50..ac0d64aa32 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -31,7 +31,9 @@ use openshell_core::net::{ }; use openshell_core::policy::ProxyPolicy; use openshell_core::provider_credentials::ProviderCredentialState; -use openshell_core::secrets::{self, SecretResolver, rewrite_header_line_checked}; +use openshell_core::secrets::{ + self, SecretResolver, UnresolvedPlaceholderReason, rewrite_header_line_checked, +}; #[cfg(test)] use openshell_isolation_interface::contract::ExecutableIdentity as ContractExecutableIdentity; use openshell_isolation_interface::contract::{ @@ -204,6 +206,32 @@ fn emit_credential_endpoint_mismatch(method: &str, host: &str, port: u16, policy ocsf_emit!(finding); } +fn build_signed_request_placeholder_denial_event( + method: &str, + host: &str, + port: u16, + policy_name: &str, +) -> openshell_ocsf::OcsfEvent { + HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) + .activity(ActivityId::for_http_method(method)) + .http_request(HttpRequest { + http_method: method.parse().expect("HTTP method parsing is infallible"), + url: None, + }) + .http_response(HttpResponse { code: 403 }) + .action(ActionId::Denied) + .disposition(DispositionId::Blocked) + .severity(SeverityId::Medium) + .status(StatusId::Failure) + .dst_endpoint(Endpoint::from_domain(host, port)) + .firewall_rule(policy_name, "credential-signing") + .message(format!( + "Credential placeholder denied in signed request fields for {host}:{port}" + )) + .status_detail("credential_placeholder_forbidden_in_signed_request") + .build() +} + /// Hostnames injected by compute drivers as `/etc/hosts` aliases for the host /// machine. Traffic to these names is eligible for the trusted-gateway SSRF /// exemption when the resolved IP matches the driver-injected value read from @@ -4905,6 +4933,7 @@ fn endpoint_credentials_for_request( } } +#[derive(Debug)] struct PreparedForwardTarget { canonical_path: String, raw_query: Option, @@ -5009,6 +5038,27 @@ fn rewrite_forward_request( path: &str, canonical_authority: &str, secret_resolver: Option<&SecretResolver>, +) -> Result, secrets::UnresolvedPlaceholderError> { + rewrite_forward_request_inner(raw, used, path, canonical_authority, secret_resolver, false) +} + +fn rewrite_sigv4_forward_request( + raw: &[u8], + used: usize, + path: &str, + canonical_authority: &str, + secret_resolver: Option<&SecretResolver>, +) -> Result, secrets::UnresolvedPlaceholderError> { + rewrite_forward_request_inner(raw, used, path, canonical_authority, secret_resolver, true) +} + +fn rewrite_forward_request_inner( + raw: &[u8], + used: usize, + path: &str, + canonical_authority: &str, + secret_resolver: Option<&SecretResolver>, + strip_sigv4_auth_headers: bool, ) -> Result, secrets::UnresolvedPlaceholderError> { let header_end = raw[..used] .windows(4) @@ -5085,6 +5135,18 @@ fn rewrite_forward_request( continue; } + // The trusted signer replaces these client-supplied fields. Drop them + // before placeholder resolution, while preserving X-Amz-Content-Sha256 + // so the relay can still detect the client's payload-signing mode. + if strip_sigv4_auth_headers + && matches!( + field_name.as_str(), + "authorization" | "x-amz-date" | "x-amz-security-token" + ) + { + continue; + } + let rewritten_line = match secret_resolver { Some(resolver) => rewrite_header_line_checked(line, resolver)?, None => line.to_string(), @@ -6474,13 +6536,39 @@ async fn handle_forward_proxy( } // 9. Rewrite request and forward to upstream - let rewritten = match rewrite_forward_request( - &forward_request_bytes, - forward_request_bytes.len(), - &upstream_target, - &canonical_authority, - secret_resolver.as_deref(), - ) { + let credential_signing = forward_upgrade_config + .as_ref() + .map_or(crate::l7::CredentialSigning::None, |config| { + config.credential_signing + }); + let sigv4_request_resolver = if credential_signing.is_sigv4() { + secret_resolver + .as_deref() + .map(SecretResolver::without_resolvable_placeholders) + } else { + None + }; + let request_resolver = sigv4_request_resolver + .as_ref() + .or(secret_resolver.as_deref()); + let rewritten_result = if credential_signing.is_sigv4() { + rewrite_sigv4_forward_request( + &forward_request_bytes, + forward_request_bytes.len(), + &upstream_target, + &canonical_authority, + request_resolver, + ) + } else { + rewrite_forward_request( + &forward_request_bytes, + forward_request_bytes.len(), + &upstream_target, + &canonical_authority, + request_resolver, + ) + }; + let rewritten = match rewritten_result { Ok(bytes) => bytes, Err(e) => { warn!( @@ -6494,21 +6582,18 @@ async fn handle_forward_proxy( .end(openshell_core::proto::MiddlewareSessionEndReason::Cancellation) .await; } + if let Some(observer) = endpoint_observer.as_ref() { + observer.observe_credential_resolution_failure(&e); + } if e.is_endpoint_mismatch() { - if let Some(observer) = endpoint_observer.as_ref() { - observer.observe_credential_failure(true); - } emit_credential_endpoint_mismatch(method, &host_lc, port, policy_str); - respond( - client, - &build_json_error_response( - 403, - "Forbidden", - "credential_endpoint_mismatch", - "credential is not authorized for this request endpoint", - ), - ) - .await?; + } else if e.is_forbidden_in_signed_request() { + ocsf_emit!(build_signed_request_placeholder_denial_event( + method, &host_lc, port, policy_str, + )); + } + respond(client, &build_credential_resolution_error_response(&e)).await?; + if e.is_endpoint_mismatch() || e.is_forbidden_in_signed_request() { client.shutdown().await.into_diagnostic()?; let mut discard = [0_u8; 1024]; let _ = tokio::time::timeout(std::time::Duration::from_secs(1), async { @@ -6520,17 +6605,6 @@ async fn handle_forward_proxy( } }) .await; - } else { - respond( - client, - &build_json_error_response( - 500, - "Internal Server Error", - "credential_injection_failed", - "unresolved credential placeholder in request", - ), - ) - .await?; } return Ok(()); } @@ -6675,11 +6749,6 @@ async fn handle_forward_proxy( return Ok(()); } - let credential_signing = forward_upgrade_config - .as_ref() - .map_or(crate::l7::CredentialSigning::None, |config| { - config.credential_signing - }); let signing_service = forward_upgrade_config .as_ref() .map_or("", |config| config.signing_service.as_str()); @@ -6886,6 +6955,31 @@ async fn respond(client: &mut (impl TokioAsyncWrite + Unpin), bytes: &[u8]) -> R /// Returns bytes ready to write to the client socket. The body is a JSON /// object with `error` and `detail` fields, matching the format used by the /// L7 deny path in `l7/rest.rs`. +fn build_credential_resolution_error_response( + error: &secrets::UnresolvedPlaceholderError, +) -> Vec { + match error.reason() { + UnresolvedPlaceholderReason::EndpointMismatch => build_json_error_response( + 403, + "Forbidden", + "credential_endpoint_mismatch", + "credential is not authorized for this request endpoint", + ), + UnresolvedPlaceholderReason::ForbiddenInSignedRequest => build_json_error_response( + 403, + "Forbidden", + "credential_placeholder_forbidden_in_signed_request", + "credential placeholders are not allowed in signed request fields", + ), + UnresolvedPlaceholderReason::Unavailable => build_json_error_response( + 500, + "Internal Server Error", + "credential_injection_failed", + "unresolved credential placeholder in request", + ), + } +} + fn build_json_error_response(status: u16, status_text: &str, error: &str, detail: &str) -> Vec { let body = serde_json::json!({ "error": error, @@ -12960,6 +13054,39 @@ network_policies: validate_required_fields(&json, &load_class_schema("http_activity")); } + #[test] + fn signed_request_placeholder_denial_is_not_endpoint_mismatch() { + use openshell_ocsf::validation::{load_class_schema, validate_required_fields}; + + let (_, resolver) = SecretResolver::from_provider_env(TestHashMap::from([( + "AWS_SECRET_ACCESS_KEY".to_string(), + "secret".to_string(), + )])); + let request_resolver = resolver + .expect("resolver") + .without_resolvable_placeholders(); + let error = request_resolver + .rewrite_header_value("openshell:resolve:env:AWS_SECRET_ACCESS_KEY") + .expect_err("signed request fields must reject placeholders"); + + let response = String::from_utf8(build_credential_resolution_error_response(&error)) + .expect("ASCII response"); + assert!(response.starts_with("HTTP/1.1 403 Forbidden")); + assert!(response.contains("credential_placeholder_forbidden_in_signed_request")); + assert!(!response.contains("credential_endpoint_mismatch")); + + let event = + build_signed_request_placeholder_denial_event("PUT", "s3.example.com", 443, "s3"); + assert_eq!(event.base().severity, SeverityId::Medium); + let json = event.to_json().unwrap(); + assert_eq!( + json["status_detail"], + "credential_placeholder_forbidden_in_signed_request" + ); + assert_eq!(json["http_response"]["code"], 403); + validate_required_fields(&json, &load_class_schema("http_activity")); + } + #[test] fn forward_credentials_capture_endpoint_resolver_and_revision_together() { use openshell_core::proto::{StaticCredentialBinding, StaticCredentialEndpointBinding}; @@ -13048,6 +13175,33 @@ network_policies: } } + #[test] + fn forward_s3_sigv4_target_canonicalizes_before_signing() { + for (target, expected) in [ + ( + "http://s3.us-east-1.amazonaws.com/bucket/a//b", + "/bucket/a/b", + ), + ( + "http://s3.us-east-1.amazonaws.com/bucket/a/../b", + "/bucket/b", + ), + ( + "http://s3.us-east-1.amazonaws.com/bucket/a%3Db", + "/bucket/a=b", + ), + ( + "http://s3.us-east-1.amazonaws.com/bucket/a+b=c@d%20e", + "/bucket/a+b=c@d%20e", + ), + ] { + let prepared = + prepare_forward_target(target, crate::l7::path::CanonicalizeOptions::default()) + .expect("S3 target must canonicalize"); + assert_eq!(prepared.upstream_target, expected); + } + } + #[test] fn live_forward_state_is_authoritative_after_revocation() { use openshell_core::proto::{StaticCredentialBinding, StaticCredentialEndpointBinding}; @@ -13727,6 +13881,302 @@ network_policies: assert!(!result_str.contains("openshell:resolve:env:ANTHROPIC_API_KEY")); } + #[test] + fn forward_sigv4_rewrite_rejects_signing_credential_in_request_header() { + let (child_env, generation_resolver, current_resolver) = + SecretResolver::from_provider_env_for_current_revision( + [ + ("AWS_ACCESS_KEY_ID".to_string(), "access-value".to_string()), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "secret-value".to_string(), + ), + ] + .into_iter() + .collect(), + TestHashMap::new(), + 7, + ); + let resolver = SecretResolver::merge( + [generation_resolver.as_ref(), current_resolver.as_ref()] + .into_iter() + .flatten(), + ) + .expect("combined resolver"); + let request_resolver = resolver.without_resolvable_placeholders(); + let raw = format!( + "GET http://host/p HTTP/1.1\r\n\ + Host: host\r\n\ + Authorization: AWS4-HMAC-SHA256 Credential={}/scope\r\n\ + x-amz-meta-value: {}\r\n\r\n", + child_env["AWS_ACCESS_KEY_ID"], child_env["AWS_SECRET_ACCESS_KEY"] + ); + let error = rewrite_sigv4_forward_request( + raw.as_bytes(), + raw.len(), + "/p", + "host", + Some(&request_resolver), + ) + .expect_err("signing credential must be rejected before forward relay"); + + assert!(error.is_forbidden_in_signed_request()); + assert!(!error.is_endpoint_mismatch()); + } + + #[test] + fn forward_sigv4_rewrite_preserves_payload_hint_while_replacing_auth_headers() { + let (child_env, generation_resolver, current_resolver) = + SecretResolver::from_provider_env_for_current_revision( + [ + ("AWS_ACCESS_KEY_ID".to_string(), "access-value".to_string()), + ("AWS_SESSION_TOKEN".to_string(), "session-value".to_string()), + ] + .into_iter() + .collect(), + TestHashMap::new(), + 7, + ); + let resolver = SecretResolver::merge( + [generation_resolver.as_ref(), current_resolver.as_ref()] + .into_iter() + .flatten(), + ) + .expect("combined resolver"); + let request_resolver = resolver.without_resolvable_placeholders(); + let raw = format!( + "PUT http://host/bucket/key HTTP/1.1\r\n\ + Host: host\r\n\ + Authorization: AWS4-HMAC-SHA256 Credential={}/scope\r\n\ + X-Amz-Date: 20261012T000000Z\r\n\ + X-Amz-Security-Token: {}\r\n\ + X-Amz-Content-Sha256: UNSIGNED-PAYLOAD\r\n\r\n", + child_env["AWS_ACCESS_KEY_ID"], child_env["AWS_SESSION_TOKEN"] + ); + + let rewritten = rewrite_sigv4_forward_request( + raw.as_bytes(), + raw.len(), + "/bucket/key", + "host", + Some(&request_resolver), + ) + .expect("old auth headers are replaced by the trusted signer"); + let rewritten = String::from_utf8(rewritten).unwrap(); + let rewritten_lower = rewritten.to_ascii_lowercase(); + + assert!(!rewritten_lower.contains("authorization:")); + assert!(!rewritten_lower.contains("x-amz-date:")); + assert!(!rewritten_lower.contains("x-amz-security-token:")); + assert!(rewritten.contains("X-Amz-Content-Sha256: UNSIGNED-PAYLOAD")); + } + + #[tokio::test] + async fn forward_sigv4_rewrite_preserves_streaming_payload_mode_through_relay() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [ + ("AWS_ACCESS_KEY_ID".to_string(), "access-value".to_string()), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "secret-value".to_string(), + ), + ] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let request_resolver = resolver.without_resolvable_placeholders(); + let body = "3\r\nabc\r\n0\r\nx-amz-checksum-crc32: AAAA==\r\n\r\n"; + let raw = format!( + "PUT http://s3.us-east-1.amazonaws.com/bucket/key HTTP/1.1\r\n\ + Host: s3.us-east-1.amazonaws.com\r\n\ + Authorization: AWS4-HMAC-SHA256 Credential={}/scope\r\n\ + X-Amz-Date: 20261012T000000Z\r\n\ + X-Amz-Content-Sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER\r\n\ + Content-Encoding: aws-chunked\r\n\ + Transfer-Encoding: chunked\r\n\ + X-Amz-Trailer: x-amz-checksum-crc32\r\n\r\n{body}", + child_env["AWS_ACCESS_KEY_ID"] + ); + let rewritten = rewrite_sigv4_forward_request( + raw.as_bytes(), + raw.len(), + "/bucket/key", + "s3.us-east-1.amazonaws.com", + Some(&request_resolver), + ) + .expect("forward rewrite"); + let guard = forward_test_guard(); + let (mut proxy_to_upstream, mut upstream_side) = tokio::io::duplex(16 * 1024); + let (mut _app_side, mut proxy_to_client) = tokio::io::duplex(16 * 1024); + let body_bytes = body.as_bytes().to_vec(); + let upstream_task = tokio::spawn(async move { + let mut forwarded = Vec::new(); + let mut buf = [0u8; 4096]; + loop { + let read = upstream_side.read(&mut buf).await.unwrap(); + assert_ne!(read, 0, "upstream request ended before chunk trailers"); + forwarded.extend_from_slice(&buf[..read]); + if forwarded.ends_with(&body_bytes) { + break; + } + } + upstream_side + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n") + .await + .unwrap(); + forwarded + }); + + tokio::time::timeout( + std::time::Duration::from_secs(5), + relay_rewritten_forward_request( + "PUT", + "/bucket/key", + rewritten, + &mut proxy_to_client, + &mut proxy_to_upstream, + ForwardRelayOptions { + generation_guard: &guard, + credential_generation: None, + body_classifier: None, + websocket_extensions: crate::l7::rest::WebSocketExtensionMode::Preserve, + secret_resolver: Some(&resolver), + request_body_credential_rewrite: false, + deny_uninspected_credentials: false, + credential_signing: crate::l7::CredentialSigning::SigV4, + signing_service: "s3", + signing_region: "us-east-1", + host: "s3.us-east-1.amazonaws.com", + port: 80, + response_middleware: None, + endpoint_observer: None, + }, + ), + ) + .await + .expect("relay must not deadlock") + .expect("relay should preserve supported streaming mode"); + drop(proxy_to_upstream); + let forwarded = upstream_task.await.expect("upstream task"); + let forwarded_headers = String::from_utf8_lossy( + &forwarded[..forwarded + .windows(4) + .position(|window| window == b"\r\n\r\n") + .expect("forwarded header terminator") + + 4], + ); + + assert!( + forwarded_headers + .contains("x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER\r\n") + ); + assert!(forwarded_headers.contains("authorization: AWS4-HMAC-SHA256")); + assert!(forwarded.ends_with(body.as_bytes())); + } + + #[tokio::test] + async fn forward_sigv4_error_response_cannot_reflect_signed_credentials() { + let session_token = "SUPER_SECRET_FORWARD_SESSION_TOKEN"; + let reflected_hex = hex::encode(session_token); + let (_, resolver) = SecretResolver::from_provider_env( + [ + ("AWS_ACCESS_KEY_ID".to_string(), "ASIATESTKEY".to_string()), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "test-secret-key".to_string(), + ), + ("AWS_SESSION_TOKEN".to_string(), session_token.to_string()), + ] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let request_resolver = resolver.without_resolvable_placeholders(); + let raw = b"GET http://s3.us-east-1.amazonaws.com/bucket/key HTTP/1.1\r\nHost: s3.us-east-1.amazonaws.com\r\n\r\n"; + let rewritten = rewrite_sigv4_forward_request( + raw, + raw.len(), + "/bucket/key", + "s3.us-east-1.amazonaws.com", + Some(&request_resolver), + ) + .expect("forward rewrite"); + let guard = forward_test_guard(); + let (mut proxy_to_upstream, mut upstream_side) = tokio::io::duplex(16 * 1024); + let (mut app_side, mut proxy_to_client) = tokio::io::duplex(16 * 1024); + let token_for_upstream = session_token.to_string(); + let hex_for_upstream = reflected_hex.clone(); + let upstream_task = tokio::spawn(async move { + let request = read_http_headers(&mut upstream_side).await; + let request = String::from_utf8_lossy(&request); + assert!(request.contains(&format!("x-amz-security-token: {token_for_upstream}\r\n"))); + + let body = format!( + "{token_for_upstream}{hex_for_upstream}" + ); + let head = format!( + "HTTP/1.1 403 Forbidden\r\nX-Reflected: {token_for_upstream}\r\nContent-Length: {}\r\n\r\n", + body.len() + ); + let split = head.len() / 2; + upstream_side + .write_all(&head.as_bytes()[..split]) + .await + .unwrap(); + tokio::task::yield_now().await; + upstream_side + .write_all(&head.as_bytes()[split..]) + .await + .unwrap(); + upstream_side.write_all(body.as_bytes()).await.unwrap(); + }); + + let outcome = tokio::time::timeout( + std::time::Duration::from_secs(5), + relay_rewritten_forward_request( + "GET", + "/bucket/key", + rewritten, + &mut proxy_to_client, + &mut proxy_to_upstream, + ForwardRelayOptions { + generation_guard: &guard, + credential_generation: None, + body_classifier: None, + websocket_extensions: crate::l7::rest::WebSocketExtensionMode::Preserve, + secret_resolver: Some(&resolver), + request_body_credential_rewrite: false, + deny_uninspected_credentials: false, + credential_signing: crate::l7::CredentialSigning::SigV4NoBody, + signing_service: "s3", + signing_region: "us-east-1", + host: "s3.us-east-1.amazonaws.com", + port: 80, + response_middleware: None, + endpoint_observer: None, + }, + ), + ) + .await + .expect("relay must not deadlock") + .expect("signed rejection must be replaced"); + assert!(matches!( + outcome, + crate::l7::provider::RelayOutcome::Consumed + )); + upstream_task.await.expect("upstream task"); + + let mut delivered = Vec::new(); + app_side.read_to_end(&mut delivered).await.unwrap(); + let delivered = String::from_utf8(delivered).unwrap(); + assert!(delivered.starts_with("HTTP/1.1 403 Upstream Rejected\r\n")); + assert!(delivered.contains("signed_upstream_rejected")); + assert!(!delivered.contains(session_token)); + assert!(!delivered.contains(&reflected_hex)); + assert!(!delivered.contains("CanonicalRequest")); + } + #[tokio::test] async fn forward_initial_body_is_classified_separately_from_auth_headers() { use openshell_core::proto::{StaticCredentialBinding, StaticCredentialEndpointBinding}; @@ -14616,9 +15066,11 @@ network_policies: let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let proxy_port = listener.local_addr().unwrap().port(); let target = target.to_string(); + let (_, target_host, target_port, _) = parse_proxy_uri(&target).expect("valid test target"); + let host_header = canonical_forward_authority(&target_host, target_port); let client = tokio::spawn(async move { let mut socket = TcpStream::connect(("127.0.0.1", proxy_port)).await.unwrap(); - let request = format!("GET {target} HTTP/1.1\r\nHost: 127.0.0.1\r\n\r\n"); + let request = format!("GET {target} HTTP/1.1\r\nHost: {host_header}\r\n\r\n"); socket.write_all(request.as_bytes()).await.unwrap(); let mut response = Vec::new(); socket.read_to_end(&mut response).await.unwrap(); @@ -14627,6 +15079,16 @@ network_policies: let (server, _) = listener.accept().await.unwrap(); let (denial_tx, mut denial_rx) = mpsc::unbounded_channel(); + let (_, secret_resolver) = SecretResolver::from_provider_env(TestHashMap::from([ + ( + "AWS_ACCESS_KEY_ID".to_string(), + "test-access-key".to_string(), + ), + ( + "AWS_SECRET_ACCESS_KEY".to_string(), + "test-secret-key".to_string(), + ), + ])); Box::pin(handle_tcp_connection( server, engine, @@ -14639,7 +15101,7 @@ network_policies: Arc::new(None), Arc::new(None), None, - None, + secret_resolver.map(Arc::new), Some(denial_tx), None, None, @@ -14755,6 +15217,104 @@ network_policies: assert_eq!(denial_stages, ["ssrf"]); } + #[tokio::test] + async fn forward_s3_canonicalizes_routes_and_rejects_query_placeholder_before_upstream() { + if !cfg!(target_os = "linux") { + eprintln!("skipping: handler identity binding requires /proc (Linux)"); + return; + } + let Some(upstream_ip) = non_loopback_test_ipv4() else { + eprintln!("skipping: no routable non-loopback IPv4 test address"); + return; + }; + + for (raw_path, expected_upstream) in [ + ("/api//v1", Some(("/api/v1", false))), + ("/api/../bucket/key", Some(("/bucket/key", true))), + ("/bucket/../api/v1", Some(("/api/v1", false))), + ( + "/api/../bucket/key?leak=openshell%3Aresolve%3Aenv%3AAWS_SECRET_ACCESS_KEY", + None, + ), + ] { + let upstream_listener = TcpListener::bind((upstream_ip, 0)) + .await + .expect("bind test upstream"); + let upstream_port = upstream_listener.local_addr().unwrap().port(); + let mut upstream_task = tokio::spawn(async move { + let (mut socket, _) = upstream_listener.accept().await.unwrap(); + let request = read_http_headers(&mut socket).await; + socket + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") + .await + .unwrap(); + String::from_utf8(request).unwrap() + }); + let endpoint_yaml = format!( + r#" - host: "{upstream_ip}" + port: {upstream_port} + path: "/api/**" + protocol: rest + access: full + - host: "{upstream_ip}" + port: {upstream_port} + path: "/bucket/**" + protocol: rest + access: full + credential_signing: sigv4:no_body + signing_service: s3 + signing_region: us-east-1 +"#, + ); + let target = format!("http://{upstream_ip}:{upstream_port}{raw_path}"); + let (response, _) = + Box::pin(drive_forward_through_handler(&endpoint_yaml, &target)).await; + let response = String::from_utf8(response).unwrap(); + + if let Some((expected_upstream_path, expect_sigv4)) = expected_upstream { + assert!(response.starts_with("HTTP/1.1 200 OK"), "{response}"); + let forwarded = + tokio::time::timeout(std::time::Duration::from_secs(1), &mut upstream_task) + .await + .expect("canonical request must reach upstream") + .unwrap(); + assert!( + forwarded.starts_with(&format!("GET {expected_upstream_path} HTTP/1.1")), + "{forwarded}" + ); + assert!( + !forwarded.contains(raw_path), + "raw non-canonical target reached upstream: {forwarded}" + ); + let forwarded = forwarded.to_ascii_lowercase(); + if expect_sigv4 { + assert!(forwarded.contains("authorization: aws4-hmac-sha256")); + assert!(forwarded.contains("x-amz-")); + } else { + assert!(!forwarded.contains("authorization:")); + assert!(!forwarded.contains("x-amz-")); + } + } else { + assert!(response.starts_with("HTTP/1.1 403 Forbidden"), "{response}"); + assert!( + response.contains("credential_placeholder_forbidden_in_signed_request"), + "{response}" + ); + assert!( + tokio::time::timeout( + std::time::Duration::from_millis(100), + &mut upstream_task, + ) + .await + .is_err(), + "query placeholder rejection must happen before upstream connect" + ); + upstream_task.abort(); + let _ = upstream_task.await; + } + } + } + /// A real `tls: skip` policy path through the handler is exempt from the /// fail-closed gate even with no TLS termination state: the handler proceeds /// past the refusal to the raw-tunnel upstream connect (which stalls on the diff --git a/crates/openshell-supervisor-network/src/sigv4.rs b/crates/openshell-supervisor-network/src/sigv4.rs index 79e4c32e18..518d6de5fc 100644 --- a/crates/openshell-supervisor-network/src/sigv4.rs +++ b/crates/openshell-supervisor-network/src/sigv4.rs @@ -2,14 +2,80 @@ // SPDX-License-Identifier: Apache-2.0 use aws_credential_types::Credentials; +use aws_sigv4::SigningOutput; use aws_sigv4::http_request::{ - PayloadChecksumKind, SignableBody, SignableRequest, SigningSettings, sign, + PayloadChecksumKind, PercentEncodingMode, SignableBody, SignableRequest, SigningInstructions, + SigningSettings, UriPathNormalizationMode, sign, }; use aws_sigv4::sign::v4; use aws_smithy_runtime_api::client::identity::Identity; use miette::{Result, miette}; use std::time::SystemTime; +/// Sign without allowing the AWS SDK's canonical-request TRACE event to reach +/// supervisor log sinks. The canonical request can contain a raw session token. +fn sign_without_tracing<'a>( + request: SignableRequest<'a>, + params: &'a aws_sigv4::http_request::SigningParams<'a>, +) -> std::result::Result, aws_sigv4::http_request::SigningError> +{ + let no_tracing = tracing::Dispatch::new(tracing::subscriber::NoSubscriber::default()); + tracing::dispatcher::with_default(&no_tracing, || sign(request, params)) +} + +fn aws_uri_encode_path_once(path: &str) -> Result { + let mut encoded = String::with_capacity(path.len()); + let bytes = path.as_bytes(); + let mut index = 0; + + while index < bytes.len() { + let byte = bytes[index]; + if byte == b'/' || byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'.' | b'_' | b'~') + { + encoded.push(char::from(byte)); + index += 1; + } else if byte == b'%' { + if index + 2 >= bytes.len() + || !bytes[index + 1].is_ascii_hexdigit() + || !bytes[index + 2].is_ascii_hexdigit() + { + return Err(miette!( + "SigV4 signing: request path contains invalid percent encoding" + )); + } + encoded.push('%'); + encoded.push(char::from(bytes[index + 1].to_ascii_uppercase())); + encoded.push(char::from(bytes[index + 2].to_ascii_uppercase())); + index += 3; + } else { + use std::fmt::Write as _; + write!(&mut encoded, "%{byte:02X}").expect("writing to a String cannot fail"); + index += 1; + } + } + + Ok(encoded) +} + +fn signing_uri(host: &str, request_target: &str, service: &str) -> Result { + let (path, query) = request_target + .split_once('?') + .map_or((request_target, None), |(path, query)| (path, Some(query))); + let path = if service.eq_ignore_ascii_case("s3") { + aws_uri_encode_path_once(path)? + } else { + path.to_string() + }; + // `aws-sigv4` parses the already-encoded query as form data, where a raw + // `+` means space. In an HTTP URI it is a literal plus, whose AWS canonical + // representation is `%2B`; protect it before the signer parses the query. + let query = query.map(|query| query.replace('+', "%2B")); + Ok(query.map_or_else( + || format!("https://{host}{path}"), + |query| format!("https://{host}{path}?{query}"), + )) +} + /// AWS regions contain a hyphen followed by a digit (e.g., `us-east-1`). /// Service names like `s3` or `bedrock-runtime` do not. fn looks_like_region(s: &str) -> bool { @@ -208,6 +274,10 @@ fn build_signing_params<'a>( ) -> Result> { let mut settings = SigningSettings::default(); settings.payload_checksum_kind = PayloadChecksumKind::XAmzSha256; + if service.eq_ignore_ascii_case("s3") { + settings.percent_encoding_mode = PercentEncodingMode::Single; + settings.uri_path_normalization_mode = UriPathNormalizationMode::Disabled; + } Ok(v4::SigningParams::builder() .identity(identity) @@ -233,7 +303,7 @@ fn build_identity(access_key: &str, secret_key: &str, session_token: Option<&str fn rebuild_request( parts: &RequestParts<'_>, - instructions: &aws_sigv4::http_request::SigningInstructions, + instructions: &SigningInstructions, body: &[u8], ) -> Vec { let mut output = Vec::with_capacity(256 + body.len()); @@ -282,7 +352,7 @@ pub fn apply_sigv4_to_request( let header_str = std::str::from_utf8(&raw[..header_end]) .map_err(|e| miette!("SigV4 signing: request headers are not valid UTF-8: {e}"))?; let parts = parse_request_parts(header_str); - let uri = format!("https://{host}{}", parts.path); + let uri = signing_uri(host, parts.path, service)?; let identity = build_identity(access_key, secret_key, session_token); let signing_params = build_signing_params(&identity, region, service)?; @@ -297,7 +367,7 @@ pub fn apply_sigv4_to_request( ) .map_err(|e| miette!("SigV4 signable request: {e}"))?; - let (instructions, _signature) = sign(signable_request, &signing_params) + let (instructions, _signature) = sign_without_tracing(signable_request, &signing_params) .map_err(|e| miette!("SigV4 signing failed: {e}"))? .into_parts(); @@ -349,7 +419,7 @@ pub fn apply_sigv4_headers_only_with_body( let header_str = std::str::from_utf8(raw_headers) .map_err(|e| miette!("SigV4 signing: request headers are not valid UTF-8: {e}"))?; let parts = parse_request_parts(header_str); - let uri = format!("https://{host}{}", parts.path); + let uri = signing_uri(host, parts.path, service)?; let identity = build_identity(access_key, secret_key, session_token); let signing_params = build_signing_params(&identity, region, service)?; @@ -364,7 +434,7 @@ pub fn apply_sigv4_headers_only_with_body( ) .map_err(|e| miette!("SigV4 signable request: {e}"))?; - let (instructions, _signature) = sign(signable_request, &signing_params) + let (instructions, _signature) = sign_without_tracing(signable_request, &signing_params) .map_err(|e| miette!("SigV4 signing failed: {e}"))? .into_parts(); @@ -374,6 +444,148 @@ pub fn apply_sigv4_headers_only_with_body( #[cfg(test)] mod tests { use super::*; + use std::sync::{Arc, Mutex}; + use tracing::Subscriber; + use tracing::field::{Field, Visit}; + use tracing_subscriber::Layer; + use tracing_subscriber::layer::SubscriberExt as _; + + #[derive(Clone, Default)] + struct EventCapture(Arc>>); + + struct FieldCapture<'a>(&'a mut String); + + impl Visit for FieldCapture<'_> { + fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { + use std::fmt::Write as _; + write!(self.0, "{}={value:?};", field.name()) + .expect("writing tracing fields to a String cannot fail"); + } + } + + impl Layer for EventCapture { + fn on_event( + &self, + event: &tracing::Event<'_>, + _ctx: tracing_subscriber::layer::Context<'_, S>, + ) { + let mut fields = String::new(); + event.record(&mut FieldCapture(&mut fields)); + self.0 + .lock() + .expect("event capture mutex must not be poisoned") + .push(format!("{}:{fields}", event.metadata().target())); + } + } + + #[test] + fn s3_signing_uri_encodes_reserved_path_bytes_exactly_once() { + assert_eq!( + signing_uri( + "s3.us-east-1.amazonaws.com", + "/bucket/a+b=c@d%20e%2ff?prefix=a+b", + "s3", + ) + .unwrap(), + "https://s3.us-east-1.amazonaws.com/bucket/a%2Bb%3Dc%40d%20e%2Ff?prefix=a%2Bb" + ); + } + + #[test] + fn non_s3_signing_preserves_path_and_escapes_query_plus() { + assert_eq!( + signing_uri( + "execute-api.us-east-1.amazonaws.com", + "/stage/a+b?value=a+b", + "execute-api", + ) + .unwrap(), + "https://execute-api.us-east-1.amazonaws.com/stage/a+b?value=a%2Bb" + ); + } + + #[test] + fn s3_signing_uri_rejects_invalid_percent_encoding() { + assert!(signing_uri("s3.us-east-1.amazonaws.com", "/bucket/bad%2", "s3").is_err()); + } + + #[test] + fn s3_signing_keeps_the_wire_request_target_unchanged() { + let raw = b"GET /bucket/a+b=c@d%20e?prefix=a+b HTTP/1.1\r\nHost: s3.us-east-1.amazonaws.com\r\n\r\n"; + let signed = apply_sigv4_to_request( + raw, + "s3.us-east-1.amazonaws.com", + "us-east-1", + "s3", + "AKIATEST", + "secret", + None, + ) + .unwrap(); + + assert!(signed.starts_with(b"GET /bucket/a+b=c@d%20e?prefix=a+b HTTP/1.1\r\n")); + } + + #[test] + fn signing_does_not_emit_the_canonical_request_to_tracing() { + let events = EventCapture::default(); + let captured = Arc::clone(&events.0); + let subscriber = tracing_subscriber::registry().with(events); + let session_token = "SUPER_SECRET_SIGV4_SESSION_TOKEN"; + + tracing::subscriber::with_default(subscriber, || { + tracing::trace!(target: "test_capture", "capture is active"); + apply_sigv4_to_request( + b"GET /bucket/key HTTP/1.1\r\nHost: s3.us-east-1.amazonaws.com\r\n\r\n", + "s3.us-east-1.amazonaws.com", + "us-east-1", + "s3", + "ASIATEST", + "secret", + Some(session_token), + ) + .unwrap(); + }); + + let events = captured + .lock() + .expect("event capture mutex must not be poisoned") + .join("\n"); + assert!(events.contains("test_capture")); + assert!(!events.contains("aws_sigv4")); + assert!(!events.contains(session_token)); + } + + #[test] + fn s3_signing_uses_service_specific_path_settings() { + let identity = build_identity("access-key", "secret-key", None); + let params = build_signing_params(&identity, "us-east-1", "s3").unwrap(); + + assert_eq!( + params.settings().percent_encoding_mode, + PercentEncodingMode::Single + ); + assert_eq!( + params.settings().uri_path_normalization_mode, + UriPathNormalizationMode::Disabled + ); + } + + #[test] + fn non_s3_signing_preserves_default_path_settings() { + let identity = build_identity("access-key", "secret-key", None); + let params = build_signing_params(&identity, "us-east-1", "bedrock").unwrap(); + let defaults = SigningSettings::default(); + + assert_eq!( + params.settings().percent_encoding_mode, + defaults.percent_encoding_mode + ); + assert_eq!( + params.settings().uri_path_normalization_mode, + defaults.uri_path_normalization_mode + ); + } #[test] fn extract_region_from_hostname() { diff --git a/crates/openshell-supervisor-process/src/log_push.rs b/crates/openshell-supervisor-process/src/log_push.rs index d12061e82e..6c494fde22 100644 --- a/crates/openshell-supervisor-process/src/log_push.rs +++ b/crates/openshell-supervisor-process/src/log_push.rs @@ -46,6 +46,12 @@ impl Layer for LogPushLayer { fn on_event(&self, event: &Event<'_>, _ctx: Context<'_, S>) { let meta = event.metadata(); + // aws-sigv4 TRACE events include the canonical request, which can + // contain credentials added during signing. Never push them remotely. + if meta.target() == "aws_sigv4" || meta.target().starts_with("aws_sigv4::") { + return; + } + // Filter by configured max level (default: info). if *meta.level() > self.max_level { return; @@ -335,13 +341,16 @@ mod tests { } } - /// Capture lines emitted by `f` with an `INFO` level filter. - fn capture(capacity: usize, f: impl FnOnce()) -> Vec { + fn capture_at( + capacity: usize, + max_level: tracing::Level, + f: impl FnOnce(), + ) -> Vec { let (tx, mut rx) = mpsc::channel::(capacity); let layer = LogPushLayer { sandbox_id: "sb-test".to_string(), tx, - max_level: tracing::Level::INFO, + max_level, }; let subscriber = tracing_subscriber::registry().with(layer); tracing::subscriber::with_default(subscriber, f); @@ -353,6 +362,11 @@ mod tests { out } + /// Capture lines emitted by `f` with an `INFO` level filter. + fn capture(capacity: usize, f: impl FnOnce()) -> Vec { + capture_at(capacity, tracing::Level::INFO, f) + } + #[test] fn ocsf_events_push_shorthand_with_ocsf_level_and_no_fields() { let event = NetworkActivityBuilder::new(&ocsf_ctx()) @@ -423,6 +437,29 @@ mod tests { assert_eq!(messages, vec!["info line", "warn line"]); } + #[test] + fn aws_sigv4_events_are_never_pushed_at_trace_level() { + let sentinel = "SUPER_SECRET_SESSION_TOKEN"; + let lines = capture_at(16, tracing::Level::TRACE, || { + tracing::trace!( + target: "aws_sigv4", + canonical_request = sentinel, + "canonical request" + ); + tracing::trace!( + target: "aws_sigv4::http_request", + canonical_request = sentinel, + "canonical request" + ); + tracing::trace!(target: "unrelated", "safe trace"); + }); + + assert_eq!(lines.len(), 1); + assert_eq!(lines[0].target, "unrelated"); + assert_eq!(lines[0].message, "safe trace"); + assert!(!format!("{lines:#?}").contains(sentinel)); + } + #[test] fn lines_are_dropped_when_the_channel_is_full() { let lines = capture(2, || { diff --git a/docs/how-it-works/providers/aws.mdx b/docs/how-it-works/providers/aws.mdx index 087161faa9..f94cd89b98 100644 --- a/docs/how-it-works/providers/aws.mdx +++ b/docs/how-it-works/providers/aws.mdx @@ -166,6 +166,9 @@ endpoints: - OpenShell rejects a signed sandbox policy before activation unless the endpoint has a resolvable AWS credential source. An endpoint-bearing profile must declare `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` and cover the signed host, port, and path. An endpointless profile must declare those keys and be selected with `credential_binding.provider` on the signed endpoint. - The `sigv4:body` mode buffers at most 10 MiB. Requests with larger bodies are rejected. Use `sigv4:no_body` or `sigv4` (auto-detect) for large payloads. - The active provider must contain current `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` values. If either becomes unavailable after policy activation, the request fails closed. +- Signed request fields cannot resolve OpenShell credential placeholders. The proxy strips client-supplied AWS authentication fields and adds trusted SigV4 fields after request rewriting. +- OpenShell canonicalizes the request path before policy evaluation, then signs and forwards that same canonical request target. S3 signing uses single percent-encoding without additional path normalization so the signature matches the forwarded target. +- OpenShell relays only final 2xx responses from a signed request. It drops informational responses and replaces redirects, malformed responses, and 4xx or 5xx response details with a generic error. For 4xx and 5xx responses, it preserves the status code. This prevents AWS signature diagnostics from returning signed credentials to the sandbox. ## Use from a Sandbox