Skip to content

Commit c0d4322

Browse files
committed
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 <kmalczuk@redhat.com>
1 parent 320d4ef commit c0d4322

8 files changed

Lines changed: 188 additions & 14 deletions

File tree

architecture/security-policy.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,10 @@ connection metadata agrees. When request paths overlap, a path endpoint with a
187187
higher specificity rank deterministically overrides broader request-processing
188188
metadata. Equally specific overlapping endpoints must agree.
189189

190+
Endpoint `tls`, `enforcement`, `access`, and `protocol` strings are validated
191+
before persistence or activation. The supervisor also refuses unknown endpoint
192+
modes defensively; an unrecognized enforcement value never falls back to audit.
193+
190194
Gateway mutation paths validate the complete effective candidate before
191195
persistence when the affected sandbox scope is known. Direct replacements,
192196
incremental merges and approvals, provider attachment, and profile fanout reject

crates/openshell-policy/src/l7_validate.rs

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,30 @@ mod agent_transport_tests {
107107
}
108108
}
109109

110+
/// Validate the security-sensitive endpoint fields whose public representation
111+
/// is currently a string. Empty values preserve the documented defaults.
112+
pub fn validate_endpoint_modes(tls: &str, enforcement: &str, access: &str) -> Vec<String> {
113+
let mut errors = Vec::new();
114+
115+
if !matches!(tls, "" | "skip" | "terminate" | "passthrough") {
116+
errors.push(format!(
117+
"unknown tls value '{tls}' (expected skip, terminate, or passthrough)"
118+
));
119+
}
120+
if !matches!(enforcement, "" | "enforce" | "audit") {
121+
errors.push(format!(
122+
"unknown enforcement value '{enforcement}' (expected enforce or audit)"
123+
));
124+
}
125+
if !matches!(access, "" | "read-only" | "read-write" | "full") {
126+
errors.push(format!(
127+
"unknown access value '{access}' (expected read-only, read-write, or full)"
128+
));
129+
}
130+
131+
errors
132+
}
133+
110134
/// Fields extracted from an endpoint definition needed for L7 semantic
111135
/// validation. Both profile lint and the runtime validator construct this
112136
/// from their own data representation.
@@ -250,6 +274,27 @@ mod tests {
250274
assert!(errors.is_empty(), "expected no errors, got: {errors:?}");
251275
}
252276

277+
#[test]
278+
fn endpoint_modes_reject_unknown_values() {
279+
let errors = validate_endpoint_modes("skp", "enforc", "read-wirte");
280+
281+
assert_eq!(errors.len(), 3);
282+
assert!(errors[0].contains("unknown tls value 'skp'"));
283+
assert!(errors[1].contains("unknown enforcement value 'enforc'"));
284+
assert!(errors[2].contains("unknown access value 'read-wirte'"));
285+
}
286+
287+
#[test]
288+
fn endpoint_modes_accept_documented_values_and_defaults() {
289+
for tls in ["", "skip", "terminate", "passthrough"] {
290+
for enforcement in ["", "enforce", "audit"] {
291+
for access in ["", "read-only", "read-write", "full"] {
292+
assert!(validate_endpoint_modes(tls, enforcement, access).is_empty());
293+
}
294+
}
295+
}
296+
}
297+
253298
#[test]
254299
fn rejects_unknown_protocol() {
255300
let ep = L7EndpointFields {

crates/openshell-policy/src/lib.rs

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ pub use compose::{
3838
is_provider_rule_name, provider_rule_name, strip_provider_rule_names,
3939
};
4040
pub use l7_validate::{
41-
L7EndpointFields, L7Protocol, agent_authored_transport_rejection,
41+
L7EndpointFields, L7Protocol, agent_authored_transport_rejection, validate_endpoint_modes,
4242
validate_explicit_tcp_additional_fields, validate_l7_endpoint_semantics,
4343
};
4444
pub use merge::{
@@ -1814,6 +1814,11 @@ fn validate_sandbox_policy_with_mcp_presence(
18141814
.unwrap_or(false),
18151815
};
18161816
let mut l7_errors = validate_l7_endpoint_semantics(&fields);
1817+
l7_errors.extend(validate_endpoint_modes(
1818+
&ep.tls,
1819+
&ep.enforcement,
1820+
&ep.access,
1821+
));
18171822
let mut explicit_tcp_fields = Vec::new();
18181823
if !ep.enforcement.is_empty() {
18191824
explicit_tcp_fields.push("enforcement");
@@ -2407,6 +2412,36 @@ network_policies:
24072412
assert!(policy.filesystem.is_none());
24082413
}
24092414

2415+
#[test]
2416+
fn validation_rejects_unknown_security_sensitive_endpoint_values() {
2417+
let policy = parse_sandbox_policy(
2418+
r"
2419+
version: 1
2420+
network_policies:
2421+
github_api:
2422+
endpoints:
2423+
- host: api.github.com
2424+
port: 443
2425+
protocol: rest
2426+
tls: skp
2427+
enforcement: enforc
2428+
access: read-wirte
2429+
",
2430+
)
2431+
.expect("the string-backed protobuf shape accepts syntactically valid YAML");
2432+
2433+
let violations = validate_sandbox_policy(&policy).expect_err("values must be rejected");
2434+
let message = violations
2435+
.iter()
2436+
.map(ToString::to_string)
2437+
.collect::<Vec<_>>()
2438+
.join("\n");
2439+
2440+
assert!(message.contains("unknown tls value 'skp'"));
2441+
assert!(message.contains("unknown enforcement value 'enforc'"));
2442+
assert!(message.contains("unknown access value 'read-wirte'"));
2443+
}
2444+
24102445
#[test]
24112446
fn process_identity_omission_survives_yaml_round_trip() {
24122447
let policy = parse_sandbox_policy("version: 1\nprocess:\n run_as_user: \"1234\"\n")

crates/openshell-providers/src/profiles.rs

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use openshell_core::proto::{
1616
};
1717
use openshell_core::secrets::uses_reserved_revision_namespace;
1818
use openshell_policy::{
19-
L7EndpointFields, L7Protocol, validate_explicit_tcp_additional_fields,
19+
L7EndpointFields, L7Protocol, validate_endpoint_modes, validate_explicit_tcp_additional_fields,
2020
validate_l7_endpoint_semantics,
2121
};
2222
use serde::ser::SerializeStruct;
@@ -2343,6 +2343,16 @@ pub fn validate_profile_set(
23432343
msg,
23442344
));
23452345
}
2346+
for msg in
2347+
validate_endpoint_modes(&endpoint.tls, &endpoint.enforcement, &endpoint.access)
2348+
{
2349+
diagnostics.push(ProfileValidationDiagnostic::error(
2350+
source,
2351+
profile_id,
2352+
format!("endpoints[{index}]"),
2353+
msg,
2354+
));
2355+
}
23462356
for msg in validate_explicit_tcp_additional_fields(
23472357
&endpoint.protocol,
23482358
&additional_l7_profile_fields(endpoint),
@@ -5837,6 +5847,35 @@ credentials:
58375847

58385848
// -- L7 endpoint semantic validation (shared with runtime) ----------------
58395849

5850+
#[test]
5851+
fn validate_rejects_unknown_security_sensitive_endpoint_values() {
5852+
let profile = parse_profile_yaml(
5853+
r"
5854+
id: invalid-modes
5855+
display_name: Invalid modes
5856+
endpoints:
5857+
- host: api.example.com
5858+
port: 443
5859+
protocol: rest
5860+
tls: skp
5861+
enforcement: enforc
5862+
access: read-wirte
5863+
",
5864+
)
5865+
.expect("string values should parse before semantic validation");
5866+
5867+
let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]);
5868+
let message = diagnostics
5869+
.iter()
5870+
.map(|diagnostic| diagnostic.message.as_str())
5871+
.collect::<Vec<_>>()
5872+
.join("\n");
5873+
5874+
assert!(message.contains("unknown tls value 'skp'"));
5875+
assert!(message.contains("unknown enforcement value 'enforc'"));
5876+
assert!(message.contains("unknown access value 'read-wirte'"));
5877+
}
5878+
58405879
#[test]
58415880
fn validate_rejects_protocol_without_rules_or_access() {
58425881
let profile = parse_profile_yaml(

crates/openshell-server/src/grpc/validation.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2006,6 +2006,35 @@ mod tests {
20062006
assert!(err.message().contains("TLD wildcard"));
20072007
}
20082008

2009+
#[test]
2010+
fn validate_policy_safety_reports_unknown_enforcement() {
2011+
use openshell_core::proto::{NetworkEndpoint, NetworkPolicyRule};
2012+
2013+
let mut policy = openshell_policy::restrictive_default_policy();
2014+
policy.network_policies.insert(
2015+
"github_api".into(),
2016+
NetworkPolicyRule {
2017+
name: "github-api-readonly".into(),
2018+
endpoints: vec![NetworkEndpoint {
2019+
host: "api.github.com".into(),
2020+
port: 443,
2021+
protocol: "rest".into(),
2022+
enforcement: "enforc".into(),
2023+
access: "read-only".into(),
2024+
..Default::default()
2025+
}],
2026+
..Default::default()
2027+
},
2028+
);
2029+
2030+
let err = validate_policy_safety(&policy).unwrap_err();
2031+
2032+
assert_eq!(err.code(), Code::InvalidArgument);
2033+
assert!(err.message().contains("endpoint 0"));
2034+
assert!(err.message().contains("unknown enforcement value 'enforc'"));
2035+
assert!(err.message().contains("expected enforce or audit"));
2036+
}
2037+
20092038
#[test]
20102039
fn validate_policy_safety_rejects_invalid_middleware_before_acceptance() {
20112040
use openshell_core::proto::{MiddlewareEndpointSelector, NetworkMiddlewareConfig};

crates/openshell-supervisor-network/src/l7/mod.rs

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ pub(crate) mod websocket;
2323

2424
pub use openshell_policy::L7Protocol;
2525
use openshell_policy::{
26-
L7EndpointFields, validate_explicit_tcp_additional_fields, validate_l7_endpoint_semantics,
26+
L7EndpointFields, validate_endpoint_modes, validate_explicit_tcp_additional_fields,
27+
validate_l7_endpoint_semantics,
2728
};
2829

2930
pub(crate) fn build_credential_endpoint_mismatch_finding(
@@ -177,9 +178,16 @@ pub fn parse_l7_config(val: &regorus::Value) -> Option<L7EndpointConfig> {
177178
let protocol_val = get_object_str(val, "protocol")?;
178179
let protocol = L7Protocol::parse(&protocol_val)?;
179180

180-
let tls = match get_object_str(val, "tls").as_deref() {
181-
Some("skip") => TlsMode::Skip,
182-
Some("terminate") => {
181+
let tls_value = get_object_str(val, "tls").unwrap_or_default();
182+
let enforcement_value = get_object_str(val, "enforcement").unwrap_or_default();
183+
let access_value = get_object_str(val, "access").unwrap_or_default();
184+
if !validate_endpoint_modes(&tls_value, &enforcement_value, &access_value).is_empty() {
185+
return None;
186+
}
187+
188+
let tls = match tls_value.as_str() {
189+
"skip" => TlsMode::Skip,
190+
"terminate" => {
183191
let event = openshell_ocsf::NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx())
184192
.activity(openshell_ocsf::ActivityId::Other)
185193
.severity(openshell_ocsf::SeverityId::Medium)
@@ -191,7 +199,7 @@ pub fn parse_l7_config(val: &regorus::Value) -> Option<L7EndpointConfig> {
191199
openshell_ocsf::ocsf_emit!(event);
192200
TlsMode::Auto
193201
}
194-
Some("passthrough") => {
202+
"passthrough" => {
195203
let event = openshell_ocsf::NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx())
196204
.activity(openshell_ocsf::ActivityId::Other)
197205
.severity(openshell_ocsf::SeverityId::Medium)
@@ -203,12 +211,14 @@ pub fn parse_l7_config(val: &regorus::Value) -> Option<L7EndpointConfig> {
203211
openshell_ocsf::ocsf_emit!(event);
204212
TlsMode::Auto
205213
}
206-
_ => TlsMode::Auto,
214+
"" => TlsMode::Auto,
215+
_ => unreachable!("endpoint modes were validated above"),
207216
};
208217

209-
let enforcement = match get_object_str(val, "enforcement").as_deref() {
210-
Some("enforce") => EnforcementMode::Enforce,
211-
_ => EnforcementMode::Audit,
218+
let enforcement = match enforcement_value.as_str() {
219+
"enforce" => EnforcementMode::Enforce,
220+
"" | "audit" => EnforcementMode::Audit,
221+
_ => unreachable!("endpoint modes were validated above"),
212222
};
213223

214224
let allow_encoded_slash = get_object_bool(val, "allow_encoded_slash").unwrap_or(false);
@@ -1762,6 +1772,16 @@ mod tests {
17621772
assert_eq!(config.enforcement, EnforcementMode::Audit);
17631773
}
17641774

1775+
#[test]
1776+
fn parse_l7_config_rejects_unknown_enforcement() {
1777+
let val = regorus::Value::from_json_str(
1778+
r#"{"protocol": "rest", "enforcement": "enforc", "access": "read-only", "host": "api.example.com", "port": 443}"#,
1779+
)
1780+
.unwrap();
1781+
1782+
assert!(parse_l7_config(&val).is_none());
1783+
}
1784+
17651785
#[test]
17661786
fn parse_credential_signing_sigv4() {
17671787
let val = regorus::Value::from_json_str(

docs/providers/profiles.mdx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,8 @@ environment value under the actual environment variable key.
437437

438438
`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`.
439439

440+
Profile validation rejects unknown `tls`, `enforcement`, and `access` values before the profile can contribute policy to a sandbox.
441+
440442
`binaries` contains the executable paths allowed to reach the profile endpoints when the profile contributes policy to a sandbox.
441443

442444
`inference_capable` marks profiles that are intended to participate in inference workflows. It does not currently mount or configure `inference.local`.

docs/reference/policy-schema.mdx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,9 +164,9 @@ Each endpoint defines a reachable destination and optional inspection rules.
164164
| `port` | integer | Yes | TCP port number. |
165165
| `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`. |
166166
| `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. |
167-
| `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. |
168-
| `enforcement` | string | No | `enforce` actively blocks disallowed requests. `audit` logs violations but allows traffic through. |
169-
| `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. |
167+
| `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. |
168+
| `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. |
169+
| `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. |
170170
| `rules` | list of allow rule objects | No | Fine-grained protocol-specific allow rules. Mutually exclusive with `access`. |
171171
| `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. |
172172
| `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. |

0 commit comments

Comments
 (0)