diff --git a/Cargo.lock b/Cargo.lock index 619570d9..640d7d9d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -805,6 +805,17 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "console" +version = "0.16.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d64e8af5551369d19cf50138de61f1c42074ab970f74e99be916646777f8fc87" +dependencies = [ + "encode_unicode", + "libc", + "windows-sys 0.61.2", +] + [[package]] name = "const-oid" version = "0.9.6" @@ -1260,7 +1271,7 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "658bce805d770f407bc62102fca7c2c64ceef2fbcb2b8bd19d2765ce093980de" dependencies = [ - "console", + "console 0.15.11", "shell-words", "tempfile", "thiserror 1.0.69", @@ -1760,6 +1771,7 @@ dependencies = [ "http-body-util", "hyper", "hyper-util", + "insta", "loom", "lru 0.17.0", "nix 0.31.3", @@ -1768,6 +1780,7 @@ dependencies = [ "pingora-core", "pingora-http", "pingora-proxy", + "pretty_assertions", "proptest", "prost-types", "rand 0.9.4", @@ -2537,6 +2550,18 @@ dependencies = [ "libc", ] +[[package]] +name = "insta" +version = "1.48.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "86f0f8fee8c926415c58d6ae43a08523a26faccb2323f5e6b644fe7dd4ef6b82" +dependencies = [ + "console 0.16.3", + "once_cell", + "similar", + "tempfile", +] + [[package]] name = "instability" version = "0.3.12" @@ -5131,6 +5156,12 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e3a9fe34e3e7a50316060351f37187a3f546bce95496156754b601a5fa71b76e" +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "siphasher" version = "1.0.3" diff --git a/crates/firma-sidecar/Cargo.toml b/crates/firma-sidecar/Cargo.toml index ea74e003..9f6f2bce 100644 --- a/crates/firma-sidecar/Cargo.toml +++ b/crates/firma-sidecar/Cargo.toml @@ -58,8 +58,10 @@ xxhash-rust = { workspace = true } [dev-dependencies] criterion = { workspace = true } +insta = "1" loom = "0.7" pasetors = { workspace = true } +pretty_assertions = { workspace = true, features = ["unstable"] } proptest = { workspace = true } rcgen = { workspace = true } rustls-pemfile = { workspace = true } diff --git a/crates/firma-sidecar/src/normalizer/mapping.rs b/crates/firma-sidecar/src/normalizer/mapping.rs index 06fb1c43..9d5951ba 100644 --- a/crates/firma-sidecar/src/normalizer/mapping.rs +++ b/crates/firma-sidecar/src/normalizer/mapping.rs @@ -10,9 +10,46 @@ //! be deterministically mapped to a registry entry fail closed with //! `DENY: UNCLASSIFIED_INTENT` (FEP \[I-N1\]). -use crate::config::MappingRulesFile; +use crate::config::{MappingRuleConfig, MappingRulesFile}; use crate::enforcement::registry::ActionClassRegistry; +/// Errors that can occur while building a [`MappingTable`] from configuration. +#[derive(Debug, thiserror::Error)] +pub enum MappingTableError { + /// The mapping rules file itself failed validation (empty file, + /// malformed rule, invalid HTTP method, etc). + #[error("invalid mapping rules file: {0}")] + InvalidRulesFile(String), + + /// Two rules share the same `(method, host, path)` tuple, which + /// makes classification ambiguous. + #[error( + "rule {index}: duplicate mapping tuple method={:?} host={:?} path={:?}", + rule.method.as_deref().unwrap_or_default(), + rule.host, + rule.path.as_deref().unwrap_or_default() + )] + DuplicateRule { + index: usize, + rule: MappingRuleConfig, + }, + + /// A rule references an action class outside the built-in FEP + /// registry. + #[error( + "rule {index}: action class '{}' is not in the built-in FEP action class registry.\n\ + Note: schema_path in [authority] only affects Cedar policy validation in the \ + Authority — it does not extend this registry, which is fixed when the Sidecar \ + loads.\nTo use this action class in a mapping rule, add it to the registry first.\n\ + See: https://firma-ai.github.io/openfirma/guides/extend-mapping/", + rule.action_class + )] + NonExistingActionClass { + index: usize, + rule: MappingRuleConfig, + }, +} + /// A validated mapping rule ready for matching. #[derive(Debug, Clone)] pub struct MappingRule { @@ -81,13 +118,15 @@ impl MappingTable { /// Load and validate mapping rules from a parsed config. /// /// # Errors - /// Returns an error if any rule references an unknown action class. + /// Returns an error if the rules are structurally invalid or + /// ambiguous. pub fn from_config( file: &MappingRulesFile, registry: &ActionClassRegistry, default_protected: bool, - ) -> Result { - file.validate()?; + ) -> Result { + file.validate() + .map_err(MappingTableError::InvalidRulesFile)?; // Duplicate (method, host, path) tuple detection. Two rules // with the same triple across merged mapping files produces @@ -100,11 +139,11 @@ impl MappingTable { rule_cfg.host.clone(), rule_cfg.path.clone().unwrap_or_default(), ); - if !seen.insert(key.clone()) { - return Err(format!( - "rule {i}: duplicate mapping tuple method={:?} host={:?} path={:?}", - key.0, key.1, key.2 - )); + if !seen.insert(key) { + return Err(MappingTableError::DuplicateRule { + index: i, + rule: rule_cfg.clone(), + }); } } @@ -112,10 +151,10 @@ impl MappingTable { for (i, rule_cfg) in file.rules.iter().enumerate() { if !registry.contains(&rule_cfg.action_class) { - return Err(format!( - "rule {i}: action class '{}' not in registry", - rule_cfg.action_class - )); + return Err(MappingTableError::NonExistingActionClass { + index: i, + rule: rule_cfg.clone(), + }); } let specificity = MappingRule::compute_specificity( @@ -230,47 +269,14 @@ fn glob_match(pattern: &str, value: &str) -> bool { #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { + use insta::assert_snapshot; + use pretty_assertions::assert_matches; + use super::*; use crate::config::MappingRuleConfig; - fn test_registry() -> ActionClassRegistry { - ActionClassRegistry::v0_1() - } - - fn test_rules_file() -> MappingRulesFile { - MappingRulesFile { - rules: vec![ - MappingRuleConfig { - method: Some("POST".to_string()), - host: "api.openai.com".to_string(), - path: Some("/v1/chat/completions".to_string()), - action_class: "communication.external.send".to_string(), - }, - MappingRuleConfig { - method: Some("POST".to_string()), - host: "api.anthropic.com".to_string(), - path: Some("/v1/messages".to_string()), - action_class: "communication.external.send".to_string(), - }, - MappingRuleConfig { - method: Some("GET".to_string()), - host: "*".to_string(), - path: None, - action_class: "filesystem.read".to_string(), - }, - MappingRuleConfig { - method: Some("POST".to_string()), - host: "*".to_string(), - path: None, - action_class: "communication.internal.send".to_string(), - }, - ], - } - } - #[test] - fn test_from_config_validates_action_classes() { - let registry = test_registry(); + fn mapping_rules_rejects_nonexisting_action_class() { let bad_file = MappingRulesFile { rules: vec![MappingRuleConfig { method: None, @@ -279,13 +285,23 @@ mod tests { action_class: "nonexistent.action".to_string(), }], }; - let result = MappingTable::from_config(&bad_file, ®istry, true); - assert!(result.is_err()); + let err = + MappingTable::from_config(&bad_file, &ActionClassRegistry::v0_1(), true).unwrap_err(); + assert_matches!( + err, + MappingTableError::NonExistingActionClass { index: 0, ref rule } + if rule.action_class == "nonexistent.action" + ); + assert_snapshot!(err.to_string(), @" + rule 0: action class 'nonexistent.action' is not in the built-in FEP action class registry. + Note: schema_path in [authority] only affects Cedar policy validation in the Authority — it does not extend this registry, which is fixed when the Sidecar loads. + To use this action class in a mapping rule, add it to the registry first. + See: https://firma-ai.github.io/openfirma/guides/extend-mapping/ + "); } #[test] - fn duplicate_tuple_across_merged_rules_is_startup_error() { - let registry = test_registry(); + fn duplicated_mapping_rule_is_rejected() { let file = MappingRulesFile { rules: vec![ MappingRuleConfig { @@ -302,17 +318,34 @@ mod tests { }, ], }; - let result = MappingTable::from_config(&file, ®istry, true); - assert!(result.is_err()); - let msg = result.err().unwrap_or_default(); - assert!(msg.contains("duplicate"), "expected duplicate: {msg}"); + let err = MappingTable::from_config(&file, &ActionClassRegistry::v0_1(), true).unwrap_err(); + assert_matches!( + err, + MappingTableError::DuplicateRule { index: 1, ref rule } + if rule.action_class == "code.review.read" + ); + assert_snapshot!(err.to_string(), @r#"rule 1: duplicate mapping tuple method="GET" host="api.github.com" path="/repos/*/*""#); } #[test] - fn test_specific_rule_matches_first() { - let registry = test_registry(); - let table = MappingTable::from_config(&test_rules_file(), ®istry, true) - .unwrap_or_else(|e| panic!("{e}")); + fn specific_rule_matches_first() { + let file = MappingRulesFile { + rules: vec![ + MappingRuleConfig { + method: Some("POST".to_string()), + host: "*".to_string(), + path: None, + action_class: "communication.internal.send".to_string(), + }, + MappingRuleConfig { + method: Some("POST".to_string()), + host: "api.openai.com".to_string(), + path: Some("/v1/chat/completions".to_string()), + action_class: "communication.external.send".to_string(), + }, + ], + }; + let table = MappingTable::from_config(&file, &ActionClassRegistry::v0_1(), true).unwrap(); match table.find_match("POST", "api.openai.com", "/v1/chat/completions") { MatchResult::Matched(rule) => { @@ -323,10 +356,24 @@ mod tests { } #[test] - fn test_wildcard_rule_matches_unknown_host() { - let registry = test_registry(); - let table = MappingTable::from_config(&test_rules_file(), ®istry, true) - .unwrap_or_else(|e| panic!("{e}")); + fn wildcard_rule_matches_unknown_host() { + let file = MappingRulesFile { + rules: vec![ + MappingRuleConfig { + method: Some("GET".to_string()), + host: "api.other.com".to_string(), + path: None, + action_class: "communication.internal.send".to_string(), + }, + MappingRuleConfig { + method: Some("GET".to_string()), + host: "*".to_string(), + path: None, + action_class: "filesystem.read".to_string(), + }, + ], + }; + let table = MappingTable::from_config(&file, &ActionClassRegistry::v0_1(), true).unwrap(); match table.find_match("GET", "api.weather.com", "/forecast") { MatchResult::Matched(rule) => assert_eq!(rule.action_class, "filesystem.read"), @@ -335,8 +382,7 @@ mod tests { } #[test] - fn test_no_match_protected_returns_unclassified() { - let registry = test_registry(); + fn no_match_protected_returns_unclassified() { // Table with only specific rules, no wildcard let file = MappingRulesFile { rules: vec![MappingRuleConfig { @@ -346,8 +392,7 @@ mod tests { action_class: "communication.external.send".to_string(), }], }; - let table = - MappingTable::from_config(&file, ®istry, true).unwrap_or_else(|e| panic!("{e}")); + let table = MappingTable::from_config(&file, &ActionClassRegistry::v0_1(), true).unwrap(); assert!(matches!( table.find_match("GET", "unknown.host", "/"), @@ -356,24 +401,24 @@ mod tests { } #[test] - fn test_glob_match_exact() { + fn glob_match_exact() { assert!(glob_match("api.openai.com", "api.openai.com")); assert!(!glob_match("api.openai.com", "api.anthropic.com")); } #[test] - fn test_glob_match_wildcard_prefix() { + fn glob_match_wildcard_prefix() { assert!(glob_match("*.openai.com", "api.openai.com")); assert!(!glob_match("*.openai.com", "api.anthropic.com")); } #[test] - fn test_glob_match_star_matches_all() { + fn glob_match_star_matches_all() { assert!(glob_match("*", "anything.at.all")); } #[test] - fn test_glob_match_path_wildcard() { + fn glob_match_path_wildcard() { assert!(glob_match("/v1/*/completions", "/v1/chat/completions")); assert!(!glob_match("/v1/*/completions", "/v2/chat/completions")); }