From 65e04eb138fed62c1d943cc5bcb52e0aaa43c5fa Mon Sep 17 00:00:00 2001 From: Krzysztof Malczuk Date: Fri, 4 Sep 2026 16:33:09 +0100 Subject: [PATCH] fix(policy): reject unknown endpoint security modes Closes #3046 Validate TLS, enforcement, and access values across policy and provider profile ingress, and prevent runtime parsing from falling back to audit for unknown enforcement values. Signed-off-by: Krzysztof Malczuk --- architecture/security-policy.md | 4 ++ crates/openshell-policy/src/l7_validate.rs | 45 +++++++++++++++++++ crates/openshell-policy/src/lib.rs | 37 ++++++++++++++- crates/openshell-providers/src/profiles.rs | 42 ++++++++++++++++- .../openshell-server/src/grpc/validation.rs | 29 ++++++++++++ .../src/l7/mod.rs | 38 ++++++++++++---- docs/providers/profiles.mdx | 2 + docs/reference/policy-schema.mdx | 6 +-- 8 files changed, 189 insertions(+), 14 deletions(-) diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 9203ba3178..db6aa81156 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -180,6 +180,10 @@ connection metadata agrees. When request paths overlap, a path endpoint with a higher specificity rank deterministically overrides broader request-processing metadata. Equally specific overlapping endpoints must agree. +Endpoint `tls`, `enforcement`, `access`, and `protocol` strings are validated +before persistence or activation. The supervisor also refuses unknown endpoint +modes defensively; an unrecognized enforcement value never falls back to audit. + Gateway mutation paths validate the complete effective candidate before persistence when the affected sandbox scope is known. Direct replacements, incremental merges and approvals, provider attachment, and profile fanout reject diff --git a/crates/openshell-policy/src/l7_validate.rs b/crates/openshell-policy/src/l7_validate.rs index d60491c4bb..a0a933bfe0 100644 --- a/crates/openshell-policy/src/l7_validate.rs +++ b/crates/openshell-policy/src/l7_validate.rs @@ -69,6 +69,30 @@ pub fn validate_explicit_tcp_additional_fields( )] } +/// Validate the security-sensitive endpoint fields whose public representation +/// is currently a string. Empty values preserve the documented defaults. +pub fn validate_endpoint_modes(tls: &str, enforcement: &str, access: &str) -> Vec { + let mut errors = Vec::new(); + + if !matches!(tls, "" | "skip" | "terminate" | "passthrough") { + errors.push(format!( + "unknown tls value '{tls}' (expected skip, terminate, or passthrough)" + )); + } + if !matches!(enforcement, "" | "enforce" | "audit") { + errors.push(format!( + "unknown enforcement value '{enforcement}' (expected enforce or audit)" + )); + } + if !matches!(access, "" | "read-only" | "read-write" | "full") { + errors.push(format!( + "unknown access value '{access}' (expected read-only, read-write, or full)" + )); + } + + errors +} + /// Fields extracted from an endpoint definition needed for L7 semantic /// validation. Both profile lint and the runtime validator construct this /// from their own data representation. @@ -212,6 +236,27 @@ mod tests { assert!(errors.is_empty(), "expected no errors, got: {errors:?}"); } + #[test] + fn endpoint_modes_reject_unknown_values() { + let errors = validate_endpoint_modes("skp", "enforc", "read-wirte"); + + assert_eq!(errors.len(), 3); + assert!(errors[0].contains("unknown tls value 'skp'")); + assert!(errors[1].contains("unknown enforcement value 'enforc'")); + assert!(errors[2].contains("unknown access value 'read-wirte'")); + } + + #[test] + fn endpoint_modes_accept_documented_values_and_defaults() { + for tls in ["", "skip", "terminate", "passthrough"] { + for enforcement in ["", "enforce", "audit"] { + for access in ["", "read-only", "read-write", "full"] { + assert!(validate_endpoint_modes(tls, enforcement, access).is_empty()); + } + } + } + } + #[test] fn rejects_unknown_protocol() { let ep = L7EndpointFields { diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 5be8173e3e..68338cd7d6 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -37,7 +37,7 @@ pub use compose::{ is_provider_rule_name, provider_rule_name, strip_provider_rule_names, }; pub use l7_validate::{ - L7EndpointFields, L7Protocol, validate_explicit_tcp_additional_fields, + L7EndpointFields, L7Protocol, validate_endpoint_modes, validate_explicit_tcp_additional_fields, validate_l7_endpoint_semantics, }; pub use merge::{ @@ -1598,6 +1598,11 @@ pub fn validate_sandbox_policy( .unwrap_or(false), }; let mut l7_errors = validate_l7_endpoint_semantics(&fields); + l7_errors.extend(validate_endpoint_modes( + &ep.tls, + &ep.enforcement, + &ep.access, + )); let mut explicit_tcp_fields = Vec::new(); if !ep.enforcement.is_empty() { explicit_tcp_fields.push("enforcement"); @@ -2013,6 +2018,36 @@ network_policies: assert!(policy.filesystem.is_none()); } + #[test] + fn validation_rejects_unknown_security_sensitive_endpoint_values() { + let policy = parse_sandbox_policy( + r" +version: 1 +network_policies: + github_api: + endpoints: + - host: api.github.com + port: 443 + protocol: rest + tls: skp + enforcement: enforc + access: read-wirte +", + ) + .expect("the string-backed protobuf shape accepts syntactically valid YAML"); + + let violations = validate_sandbox_policy(&policy).expect_err("values must be rejected"); + let message = violations + .iter() + .map(ToString::to_string) + .collect::>() + .join("\n"); + + assert!(message.contains("unknown tls value 'skp'")); + assert!(message.contains("unknown enforcement value 'enforc'")); + assert!(message.contains("unknown access value 'read-wirte'")); + } + #[test] fn process_identity_omission_survives_yaml_round_trip() { let policy = parse_sandbox_policy("version: 1\nprocess:\n run_as_user: \"1234\"\n") diff --git a/crates/openshell-providers/src/profiles.rs b/crates/openshell-providers/src/profiles.rs index a1f9f130a1..34cf14c279 100644 --- a/crates/openshell-providers/src/profiles.rs +++ b/crates/openshell-providers/src/profiles.rs @@ -15,7 +15,8 @@ use openshell_core::proto::{ }; use openshell_core::secrets::uses_reserved_revision_namespace; use openshell_policy::{ - L7EndpointFields, validate_explicit_tcp_additional_fields, validate_l7_endpoint_semantics, + L7EndpointFields, validate_endpoint_modes, validate_explicit_tcp_additional_fields, + validate_l7_endpoint_semantics, }; use serde::ser::SerializeStruct; use serde::{Deserialize, Deserializer, Serialize, Serializer, de}; @@ -2095,6 +2096,16 @@ pub fn validate_profile_set( msg, )); } + for msg in + validate_endpoint_modes(&endpoint.tls, &endpoint.enforcement, &endpoint.access) + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}]"), + msg, + )); + } for msg in validate_explicit_tcp_additional_fields( &endpoint.protocol, &additional_l7_profile_fields(endpoint), @@ -4878,6 +4889,35 @@ credentials: // -- L7 endpoint semantic validation (shared with runtime) ---------------- + #[test] + fn validate_rejects_unknown_security_sensitive_endpoint_values() { + let profile = parse_profile_yaml( + r" +id: invalid-modes +display_name: Invalid modes +endpoints: + - host: api.example.com + port: 443 + protocol: rest + tls: skp + enforcement: enforc + access: read-wirte +", + ) + .expect("string values should parse before semantic validation"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + let message = diagnostics + .iter() + .map(|diagnostic| diagnostic.message.as_str()) + .collect::>() + .join("\n"); + + assert!(message.contains("unknown tls value 'skp'")); + assert!(message.contains("unknown enforcement value 'enforc'")); + assert!(message.contains("unknown access value 'read-wirte'")); + } + #[test] fn validate_rejects_protocol_without_rules_or_access() { let profile = parse_profile_yaml( diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index c20dad7780..cc60796cf3 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -1993,6 +1993,35 @@ mod tests { assert!(err.message().contains("TLD wildcard")); } + #[test] + fn validate_policy_safety_reports_unknown_enforcement() { + use openshell_core::proto::{NetworkEndpoint, NetworkPolicyRule}; + + let mut policy = openshell_policy::restrictive_default_policy(); + policy.network_policies.insert( + "github_api".into(), + NetworkPolicyRule { + name: "github-api-readonly".into(), + endpoints: vec![NetworkEndpoint { + host: "api.github.com".into(), + port: 443, + protocol: "rest".into(), + enforcement: "enforc".into(), + access: "read-only".into(), + ..Default::default() + }], + ..Default::default() + }, + ); + + let err = validate_policy_safety(&policy).unwrap_err(); + + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("endpoint 0")); + assert!(err.message().contains("unknown enforcement value 'enforc'")); + assert!(err.message().contains("expected enforce or audit")); + } + #[test] fn validate_policy_safety_rejects_invalid_middleware_before_acceptance() { use openshell_core::proto::{MiddlewareEndpointSelector, NetworkMiddlewareConfig}; diff --git a/crates/openshell-supervisor-network/src/l7/mod.rs b/crates/openshell-supervisor-network/src/l7/mod.rs index 70a980ba2d..012d0d5e55 100644 --- a/crates/openshell-supervisor-network/src/l7/mod.rs +++ b/crates/openshell-supervisor-network/src/l7/mod.rs @@ -23,7 +23,8 @@ pub(crate) mod websocket; pub use openshell_policy::L7Protocol; use openshell_policy::{ - L7EndpointFields, validate_explicit_tcp_additional_fields, validate_l7_endpoint_semantics, + L7EndpointFields, validate_endpoint_modes, validate_explicit_tcp_additional_fields, + validate_l7_endpoint_semantics, }; pub(crate) fn build_credential_endpoint_mismatch_finding( @@ -177,9 +178,16 @@ pub fn parse_l7_config(val: ®orus::Value) -> Option { let protocol_val = get_object_str(val, "protocol")?; let protocol = L7Protocol::parse(&protocol_val)?; - let tls = match get_object_str(val, "tls").as_deref() { - Some("skip") => TlsMode::Skip, - Some("terminate") => { + let tls_value = get_object_str(val, "tls").unwrap_or_default(); + let enforcement_value = get_object_str(val, "enforcement").unwrap_or_default(); + let access_value = get_object_str(val, "access").unwrap_or_default(); + if !validate_endpoint_modes(&tls_value, &enforcement_value, &access_value).is_empty() { + return None; + } + + let tls = match tls_value.as_str() { + "skip" => TlsMode::Skip, + "terminate" => { let event = openshell_ocsf::NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) .activity(openshell_ocsf::ActivityId::Other) .severity(openshell_ocsf::SeverityId::Medium) @@ -191,7 +199,7 @@ pub fn parse_l7_config(val: ®orus::Value) -> Option { openshell_ocsf::ocsf_emit!(event); TlsMode::Auto } - Some("passthrough") => { + "passthrough" => { let event = openshell_ocsf::NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) .activity(openshell_ocsf::ActivityId::Other) .severity(openshell_ocsf::SeverityId::Medium) @@ -203,12 +211,14 @@ pub fn parse_l7_config(val: ®orus::Value) -> Option { openshell_ocsf::ocsf_emit!(event); TlsMode::Auto } - _ => TlsMode::Auto, + "" => TlsMode::Auto, + _ => unreachable!("endpoint modes were validated above"), }; - let enforcement = match get_object_str(val, "enforcement").as_deref() { - Some("enforce") => EnforcementMode::Enforce, - _ => EnforcementMode::Audit, + let enforcement = match enforcement_value.as_str() { + "enforce" => EnforcementMode::Enforce, + "" | "audit" => EnforcementMode::Audit, + _ => unreachable!("endpoint modes were validated above"), }; let allow_encoded_slash = get_object_bool(val, "allow_encoded_slash").unwrap_or(false); @@ -1762,6 +1772,16 @@ mod tests { assert_eq!(config.enforcement, EnforcementMode::Audit); } + #[test] + fn parse_l7_config_rejects_unknown_enforcement() { + let val = regorus::Value::from_json_str( + r#"{"protocol": "rest", "enforcement": "enforc", "access": "read-only", "host": "api.example.com", "port": 443}"#, + ) + .unwrap(); + + assert!(parse_l7_config(&val).is_none()); + } + #[test] fn parse_credential_signing_sigv4() { let val = regorus::Value::from_json_str( diff --git a/docs/providers/profiles.mdx b/docs/providers/profiles.mdx index c310c02b15..cc7a8dd7ba 100644 --- a/docs/providers/profiles.mdx +++ b/docs/providers/profiles.mdx @@ -437,6 +437,8 @@ environment value under the actual environment variable key. `endpoints` contains the same endpoint object shape as sandbox network policy. A profile can use access presets, protocol-specific allow rules, deny rules, WebSocket credential rewriting, request body credential rewriting, GraphQL fields, and SSRF IP allowlists. Because profile credentials are not mapped to individual endpoints, OpenShell conservatively treats every endpoint in a profile that declares credentials as credentialed. Such endpoints require L7 inspection and cannot use `tls: skip` unless the profile explicitly sets `allow_uninspected_credentials: true`. +Profile validation rejects unknown `tls`, `enforcement`, and `access` values before the profile can contribute policy to a sandbox. + `binaries` contains the executable paths allowed to reach the profile endpoints when the profile contributes policy to a sandbox. `inference_capable` marks profiles that are intended to participate in inference workflows. It does not currently mount or configure `inference.local`. diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index a0c5d29737..ea49f76512 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -164,9 +164,9 @@ Each endpoint defines a reachable destination and optional inspection rules. | `port` | integer | Yes | TCP port number. | | `path` | string | No | Optional HTTP path glob used to select between L7 endpoints that share the same host and port. Empty means all paths. Use this when REST and GraphQL live under the same host, such as `/repos/**` and `/graphql`. | | `protocol` | string | No | Set to `tcp` with a valid DNS hostname to allow native TCP clients through policy DNS and transparent capture without payload inspection. Omit the field for L4 passthrough through an explicit proxy, including legacy hostless `allowed_ips` endpoints. Set to `rest` for HTTP method/path inspection, `websocket` for RFC 6455 upgrade and client text-message inspection, `graphql` for GraphQL-over-HTTP operation inspection, `mcp` for MCP Streamable HTTP request inspection, or `json-rpc` for generic JSON-RPC-over-HTTP method inspection. WebSocket endpoints can also use GraphQL operation rules for GraphQL-over-WebSocket traffic. Provider-credentialed endpoints require an inspected protocol unless `allow_uninspected_credentials` is explicitly set. | -| `tls` | string | No | TLS handling mode. The proxy auto-detects TLS by peeking the first bytes of each connection and terminates it for inspected HTTPS traffic, so this field is optional in most cases. Set to `skip` to disable auto-detection for edge cases such as client-certificate mTLS or non-standard protocols. Provider-credentialed endpoints reject `tls: skip` unless `allow_uninspected_credentials` is explicitly set. The values `terminate` and `passthrough` are deprecated and log a warning; they are still accepted for backward compatibility but have no effect on behavior. | -| `enforcement` | string | No | `enforce` actively blocks disallowed requests. `audit` logs violations but allows traffic through. | -| `access` | string | No | Access preset. One of `read-only`, `read-write`, or `full`. Mutually exclusive with `rules`. Not valid on `protocol: mcp` or `protocol: json-rpc`; MCP uses explicit rules unless `mcp.allow_all_known_mcp_methods: true` enables the endpoint method profile, and JSON-RPC always uses explicit rules. | +| `tls` | string | No | TLS handling mode. The proxy auto-detects TLS by peeking the first bytes of each connection and terminates it for inspected HTTPS traffic, so this field is optional in most cases. Set to `skip` to disable auto-detection for edge cases such as client-certificate mTLS or non-standard protocols. Provider-credentialed endpoints reject `tls: skip` unless `allow_uninspected_credentials` is explicitly set. The values `terminate` and `passthrough` are deprecated and log a warning; they are still accepted for backward compatibility but have no effect on behavior. Other values are rejected before activation. | +| `enforcement` | string | No | `enforce` actively blocks disallowed requests. `audit` logs violations but allows traffic through. Other values are rejected before activation rather than interpreted as audit mode. | +| `access` | string | No | Access preset. One of `read-only`, `read-write`, or `full`; other values are rejected before activation. Mutually exclusive with `rules`. Not valid on `protocol: mcp` or `protocol: json-rpc`; MCP uses explicit rules unless `mcp.allow_all_known_mcp_methods: true` enables the endpoint method profile, and JSON-RPC always uses explicit rules. | | `rules` | list of allow rule objects | No | Fine-grained protocol-specific allow rules. Mutually exclusive with `access`. | | `deny_rules` | list of deny rule objects | No | L7 deny rules that block specific requests even when allowed by `access` or `rules`. Deny rules take precedence over allow rules. | | `allowed_ips` | list of string | No | CIDR or IP allowlist for SSRF override. Exact user-declared hostname endpoints may resolve to RFC 1918 private addresses without this field, but wildcard, hostless, and policy-advisor-proposed endpoints still require `allowed_ips` for private resolved IPs. A hostless allowlist is valid only for the legacy proxy path and cannot be combined with `protocol: tcp`. Entries overlapping loopback (`127.0.0.0/8`), link-local (`169.254.0.0/16`), or unspecified (`0.0.0.0`) are rejected at load time. |