fix(policy): harden landlock.compatibility validation (#2541)

* fix(policy): reject invalid landlock.compatibility values at parse time

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

* fix(policy): abort sandbox startup when hard_requirement has no filesystem paths

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

* fix(policy): validate landlock.compatibility at gateway and fix zero-path logging

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

* fix(policy): reject invalid landlock.compatibility on serialization

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

* fix: fixed linting error

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

---------

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

Author: letv1nnn

crates/openshell-core/src/policy.rs

@@ -92,6 +92,19 @@ pub enum LandlockCompatibility {
     HardRequirement,
 }
 
+/// Accepted `landlock.compatibility` values in their proto string form.
+///
+/// Single source of truth shared by YAML parsing, proto→runtime conversion,
+/// and gateway policy validation so the accepted set cannot drift.
+pub const LANDLOCK_COMPATIBILITY_VALUES: [&str; 2] = ["best_effort", "hard_requirement"];
+
+/// Returns `true` if `value` is an accepted `landlock.compatibility` string.
+///
+/// The empty string is accepted and defaults to `best_effort`.
+pub fn is_valid_landlock_compatibility(value: &str) -> bool {
+    value.is_empty() || LANDLOCK_COMPATIBILITY_VALUES.contains(&value)
+}
+
 // ============================================================================
 // Proto to Rust type conversions
 // ============================================================================
@@ -114,7 +127,11 @@ impl TryFrom<ProtoSandboxPolicy> for SandboxPolicy {
                 .map(FilesystemPolicy::from)
                 .unwrap_or_default(),
             network,
-            landlock: proto.landlock.map(LandlockPolicy::from).unwrap_or_default(),
+            landlock: proto
+                .landlock
+                .map(LandlockPolicy::try_from)
+                .transpose()?
+                .unwrap_or_default(),
             process: proto.process.map(ProcessPolicy::from).unwrap_or_default(),
         })
     }
@@ -138,14 +155,20 @@ impl From<ProtoFilesystemPolicy> for FilesystemPolicy {
     }
 }
 
-impl From<ProtoLandlockPolicy> for LandlockPolicy {
-    fn from(proto: ProtoLandlockPolicy) -> Self {
-        let compatibility = if proto.compatibility == "hard_requirement" {
-            LandlockCompatibility::HardRequirement
-        } else {
-            LandlockCompatibility::BestEffort
+impl TryFrom<ProtoLandlockPolicy> for LandlockPolicy {
+    type Error = miette::Error;
+
+    fn try_from(proto: ProtoLandlockPolicy) -> Result<Self, Self::Error> {
+        let compatibility = match proto.compatibility.as_str() {
+            "best_effort" | "" => LandlockCompatibility::BestEffort,
+            "hard_requirement" => LandlockCompatibility::HardRequirement,
+            otherwise => miette::bail!(
+                "invalid landlock.compatibility {:?}; accepted: {}",
+                otherwise,
+                LANDLOCK_COMPATIBILITY_VALUES.join(", ")
+            ),
         };
-        Self { compatibility }
+        Ok(Self { compatibility })
     }
 }
 
@@ -165,3 +188,49 @@ impl From<ProtoProcessPolicy> for ProcessPolicy {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn try_from_maps_known_compatibility_values() {
+        for (input, expected) in [
+            ("", LandlockCompatibility::BestEffort),
+            ("best_effort", LandlockCompatibility::BestEffort),
+            ("hard_requirement", LandlockCompatibility::HardRequirement),
+        ] {
+            let proto = ProtoLandlockPolicy {
+                compatibility: input.into(),
+            };
+            let policy = LandlockPolicy::try_from(proto).expect("should convert");
+            assert_eq!(
+                std::mem::discriminant(&policy.compatibility),
+                std::mem::discriminant(&expected),
+                "input {input:?} mapped to unexpected variant",
+            );
+        }
+    }
+
+    #[test]
+    fn try_from_rejects_invalid_compatibility() {
+        let proto = ProtoLandlockPolicy {
+            compatibility: "hard-requirement".into(),
+        };
+        let err = LandlockPolicy::try_from(proto).expect_err("should reject");
+        let msg = format!("{err:?}");
+        assert!(
+            msg.contains("best_effort") && msg.contains("hard_requirement"),
+            "error should list accepted values, got: {msg}",
+        );
+    }
+
+    #[test]
+    fn is_valid_landlock_compatibility_accepts_empty_and_known() {
+        assert!(is_valid_landlock_compatibility(""));
+        assert!(is_valid_landlock_compatibility("best_effort"));
+        assert!(is_valid_landlock_compatibility("hard_requirement"));
+        assert!(!is_valid_landlock_compatibility("nope"));
+        assert!(!is_valid_landlock_compatibility("BestEffort"));
+    }
+}

crates/openshell-policy/src/lib.rs

@@ -80,11 +80,19 @@ struct FilesystemDef {
     read_write: Vec<String>,
 }
 
+#[derive(Debug, Default, Serialize, Deserialize)]
+#[serde(rename_all = "snake_case")]
+enum LandlockCompatibilityDef {
+    #[default]
+    BestEffort,
+    HardRequirement,
+}
+
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
 struct LandlockDef {
-    #[serde(default, skip_serializing_if = "String::is_empty")]
-    compatibility: String,
+    #[serde(default)]
+    compatibility: LandlockCompatibilityDef,
 }
 
 #[derive(Debug, Serialize, Deserialize)]
@@ -918,7 +926,10 @@ fn to_proto(raw: PolicyFile) -> Result<SandboxPolicy> {
             read_write: fs.read_write,
         }),
         landlock: raw.landlock.map(|ll| LandlockPolicy {
-            compatibility: ll.compatibility,
+            compatibility: match ll.compatibility {
+                LandlockCompatibilityDef::BestEffort => "best_effort".to_string(),
+                LandlockCompatibilityDef::HardRequirement => "hard_requirement".to_string(),
+            },
         }),
         process: raw.process.map(|p| ProcessPolicy {
             run_as_user: p.run_as_user,
@@ -933,16 +944,28 @@ fn to_proto(raw: PolicyFile) -> Result<SandboxPolicy> {
 // Proto → YAML conversion
 // ---------------------------------------------------------------------------
 
-fn from_proto(policy: &SandboxPolicy) -> PolicyFile {
+fn from_proto(policy: &SandboxPolicy) -> Result<PolicyFile> {
     let filesystem_policy = policy.filesystem.as_ref().map(|fs| FilesystemDef {
         include_workdir: fs.include_workdir,
         read_only: fs.read_only.clone(),
         read_write: fs.read_write.clone(),
     });
 
-    let landlock = policy.landlock.as_ref().map(|ll| LandlockDef {
-        compatibility: ll.compatibility.clone(),
-    });
+    let landlock = match policy.landlock.as_ref() {
+        Some(ll) => {
+            let compatibility = match ll.compatibility.as_str() {
+                "hard_requirement" => LandlockCompatibilityDef::HardRequirement,
+                "best_effort" | "" => LandlockCompatibilityDef::BestEffort,
+                otherwise => miette::bail!(
+                    "invalid landlock.compatibility {:?}; accepted: {}",
+                    otherwise,
+                    openshell_core::policy::LANDLOCK_COMPATIBILITY_VALUES.join(", ")
+                ),
+            };
+            Some(LandlockDef { compatibility })
+        }
+        _ => None,
+    };
 
     let process = policy.process.as_ref().and_then(|p| {
         if p.run_as_user.is_empty() && p.run_as_group.is_empty() {
@@ -1066,14 +1089,14 @@ fn from_proto(policy: &SandboxPolicy) -> PolicyFile {
 
     let network_middlewares = middleware::from_proto(&policy.network_middlewares);
 
-    PolicyFile {
+    Ok(PolicyFile {
         version: policy.version,
         filesystem_policy,
         landlock,
         process,
         network_policies,
         network_middlewares,
-    }
+    })
 }
 
 // ---------------------------------------------------------------------------
@@ -1176,7 +1199,7 @@ pub fn parse_sandbox_policy(yaml: &str) -> Result<SandboxPolicy> {
 pub fn serialize_sandbox_policy(policy: &SandboxPolicy) -> Result<String> {
     let canonical = validate_and_canonicalize_mcp_policy_schema(policy.clone())
         .map_err(|error| miette::miette!("cannot serialize invalid sandbox policy: {error}"))?;
-    let yaml_repr = from_proto(&canonical);
+    let yaml_repr = from_proto(&canonical)?;
     serde_yml::to_string(&yaml_repr)
         .into_diagnostic()
         .wrap_err("failed to serialize policy to YAML")
@@ -1189,7 +1212,7 @@ pub fn serialize_sandbox_policy(policy: &SandboxPolicy) -> Result<String> {
 pub fn sandbox_policy_to_json_value(policy: &SandboxPolicy) -> Result<serde_json::Value> {
     let canonical = validate_and_canonicalize_mcp_policy_schema(policy.clone())
         .map_err(|error| miette::miette!("cannot serialize invalid sandbox policy: {error}"))?;
-    let json_repr = from_proto(&canonical);
+    let json_repr = from_proto(&canonical)?;
     serde_json::to_value(&json_repr)
         .into_diagnostic()
         .wrap_err("failed to serialize policy to JSON")
@@ -1372,6 +1395,8 @@ pub enum PolicyViolation {
         policy_name: String,
         host: String,
     },
+    /// `landlock.compatibility` has an unrecognized value.
+    InvalidLandlockCompatibility { value: String },
     /// An effective MCP endpoint has not materialized a protocol revision.
     MissingMcpVersions { policy_name: String, host: String },
     /// A non-MCP endpoint carries MCP-only configuration.
@@ -1554,6 +1579,13 @@ impl fmt::Display for PolicyViolation {
                      '{policy_name}' tls: skip endpoint '{host}'"
                 )
             }
+            Self::InvalidLandlockCompatibility { value } => {
+                write!(
+                    f,
+                    "invalid landlock.compatibility '{value}'; accepted: {}",
+                    openshell_core::policy::LANDLOCK_COMPATIBILITY_VALUES.join(", ")
+                )
+            }
             Self::MissingMcpVersions { policy_name, host } => {
                 write!(
                     f,
@@ -1650,6 +1682,17 @@ fn validate_sandbox_policy_with_mcp_presence(
         }
     }
 
+    // Check landlock compatibility mode is a recognized value. Direct gRPC/SDK
+    // clients bypass YAML serde validation, so reject invalid values here at the
+    // gateway create path rather than deferring rejection to sandbox startup.
+    if let Some(ref landlock) = policy.landlock
+        && !openshell_core::policy::is_valid_landlock_compatibility(&landlock.compatibility)
+    {
+        violations.push(PolicyViolation::InvalidLandlockCompatibility {
+            value: landlock.compatibility.clone(),
+        });
+    }
+
     // Check filesystem paths
     if let Some(ref fs) = policy.filesystem {
         let total_paths = fs.read_only.len() + fs.read_write.len();
@@ -3152,6 +3195,80 @@ network_policies:
         assert_eq!(violations.len(), 2);
     }
 
+    #[test]
+    fn parse_rejects_invalid_landlock_compatibility() {
+        let err = parse_sandbox_policy("version: 1\nlandlock:\n  compatibility: bogus\n")
+            .expect_err("should reject invalid YAML enum value");
+        let msg = format!("{err:?}");
+        assert!(
+            msg.contains("best_effort") && msg.contains("hard_requirement"),
+            "error should list accepted values, got: {msg}",
+        );
+    }
+
+    #[test]
+    fn parse_accepts_known_landlock_compatibility() {
+        for value in ["best_effort", "hard_requirement"] {
+            let yaml = format!("version: 1\nlandlock:\n  compatibility: {value}\n");
+            let policy = parse_sandbox_policy(&yaml).expect("should parse");
+            assert_eq!(
+                policy.landlock.as_ref().expect("landlock").compatibility,
+                value,
+            );
+        }
+    }
+
+    #[test]
+    fn validate_rejects_invalid_landlock_compatibility_proto() {
+        let mut policy = restrictive_default_policy();
+        policy.landlock = Some(LandlockPolicy {
+            compatibility: "nope".into(),
+        });
+        let violations = validate_sandbox_policy(&policy).unwrap_err();
+        assert!(
+            violations
+                .iter()
+                .any(|v| matches!(v, PolicyViolation::InvalidLandlockCompatibility { .. })),
+            "expected InvalidLandlockCompatibility, got: {violations:?}",
+        );
+    }
+
+    #[test]
+    fn validate_accepts_empty_landlock_compatibility() {
+        // Empty string is the proto default and maps to best_effort.
+        let mut policy = restrictive_default_policy();
+        policy.landlock = Some(LandlockPolicy {
+            compatibility: String::new(),
+        });
+        assert!(validate_sandbox_policy(&policy).is_ok());
+    }
+
+    #[test]
+    fn serialize_rejects_invalid_landlock_compatibility() {
+        // Old policies persisted before gateway validation can hold invalid
+        // values; serialization must error rather than normalize to best_effort.
+        let mut policy = restrictive_default_policy();
+        policy.landlock = Some(LandlockPolicy {
+            compatibility: "hard-requirement".to_string(),
+        });
+        let err = serialize_sandbox_policy(&policy).expect_err("should reject");
+        let msg = format!("{err:?}");
+        assert!(
+            msg.contains("best_effort") && msg.contains("hard_requirement"),
+            "error should list accepted values, got: {msg}",
+        );
+    }
+
+    #[test]
+    fn serialize_accepts_empty_landlock_compatibility() {
+        // Empty string is the proto default; serialize must not error on it.
+        let mut policy = restrictive_default_policy();
+        policy.landlock = Some(LandlockPolicy {
+            compatibility: String::new(),
+        });
+        assert!(serialize_sandbox_policy(&policy).is_ok());
+    }
+
     #[test]
     fn validate_rejects_invalid_middleware_control_fields() {
         let cases = [