diff --git a/acl-filter/src/tests.rs b/acl-filter/src/tests.rs index 88210d505b..136dc22c7c 100644 --- a/acl-filter/src/tests.rs +++ b/acl-filter/src/tests.rs @@ -158,6 +158,18 @@ fn build_filter_v4(acl: Option) -> AclFilter { build_filter(V1_IPS, V2_IPS, acl) } +fn build_filter_v4_masquerade(acl: Option) -> AclFilter { + acl_filter(&overlay( + &[("vpc1", VNI1), ("vpc2", VNI2)], + vec![peering( + "vpc1-to-vpc2", + ("vpc1", vec![expose(V1_IPS)]), + ("vpc2", vec![expose_masquerade(V2_IPS, V2_IPS)]), + acl, + )], + )) +} + // ------------------------------------------------------------------------------------------------- // ACL rule builders @@ -682,7 +694,7 @@ fn flow_scope_allows_reply_for_allowed_request() { pattern(&[V1_IPS], &[V2_IPS], AclProtoMatch::Tcp), )], ); - let mut filter = build_filter_v4(Some(acl)); + let mut filter = build_filter_v4_masquerade(Some(acl)); // The request is allowed by the rule directly let request = packet( @@ -763,7 +775,7 @@ fn explicit_deny_rule_drops_reply_despite_matching_flow() { ), ], ); - let mut filter = build_filter_v4(Some(acl)); + let mut filter = build_filter_v4_masquerade(Some(acl)); // The request is allowed by the request-direction rule let request = packet( diff --git a/config/src/converters/k8s/config/acl.rs b/config/src/converters/k8s/config/acl.rs index 05d658c488..c1ddc0fc11 100644 --- a/config/src/converters/k8s/config/acl.rs +++ b/config/src/converters/k8s/config/acl.rs @@ -333,6 +333,7 @@ mod test { to: &str, action: GatewayAgentPeeringsAclRulesAction, r#match: Option, + scope: Option, ) -> GatewayAgentPeeringsAclRules { GatewayAgentPeeringsAclRules { action, @@ -340,7 +341,7 @@ mod test { log: None, r#match, name: (!name.is_empty()).then(|| name.to_string()), - scope: None, + scope, to: (!to.is_empty()).then(|| to.to_string()), } } @@ -433,6 +434,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, None, + None, ), ) .unwrap(); @@ -450,6 +452,7 @@ mod test { "", GatewayAgentPeeringsAclRulesAction::Allow, None, + None, ), ) .is_ok() @@ -461,7 +464,14 @@ mod test { &vpc_subnets, "VPC-1", "VPC-2", - &rule("r", "", "", GatewayAgentPeeringsAclRulesAction::Allow, None), + &rule( + "r", + "", + "", + GatewayAgentPeeringsAclRulesAction::Allow, + None, + None, + ), ) .is_err() ); @@ -478,6 +488,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, None, + None, ), ) .is_err() @@ -509,6 +520,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, Some(m.clone()), + None, ), ) .unwrap(); @@ -526,6 +538,7 @@ mod test { "VPC-1", GatewayAgentPeeringsAclRulesAction::Allow, Some(m), + None, ), ) .is_err() @@ -557,6 +570,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, Some(m), + None, ), ); assert!( @@ -589,6 +603,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, Some(m), + None, ), ) .unwrap(); @@ -622,6 +637,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, Some(ports_only), + None, ), ) .unwrap(); @@ -646,6 +662,7 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, Some(empty_ports), + None, ), ); assert!( @@ -680,6 +697,10 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Allow, Some(m), + // "scope: flow" (default scope) not currently supported with the peering + // configuration, because there's no masquerade/port forwarding. + // See https://github.com/githedgehog/dataplane/issues/1625 + Some(GatewayAgentPeeringsAclRulesScope::Packet), ), ) .unwrap(); @@ -733,6 +754,7 @@ mod test { proto: Some("tcp".to_string()), src: None, }), + None, ), rule( "return", @@ -740,6 +762,7 @@ mod test { "VPC-1", GatewayAgentPeeringsAclRulesAction::Allow, None, + None, ), ]), }; @@ -750,8 +773,8 @@ mod test { "VPC-2", GatewayAgentPeeringsAclRulesAction::Deny, None, + Some(GatewayAgentPeeringsAclRulesScope::Packet), ); - rule_with_scope.scope = Some(GatewayAgentPeeringsAclRulesScope::Packet); rule_with_scope.log = Some(true); let mut acl_with_scope = acl.clone(); acl_with_scope.rules.as_mut().unwrap().push(rule_with_scope); diff --git a/config/src/external/overlay/acl.rs b/config/src/external/overlay/acl.rs index 253bceeefb..add75d6fb6 100644 --- a/config/src/external/overlay/acl.rs +++ b/config/src/external/overlay/acl.rs @@ -4,8 +4,8 @@ //! Access Control Lists (ACLs) use super::vpcpeering::ValidatedManifest; -use crate::ConfigError; use crate::utils::normalize; +use crate::{ConfigError, ConfigResult}; use lpm::prefix::{PortRange, Prefix, PrefixPortsSet, PrefixWithOptionalPorts}; use match_action::MaskSpec; use net::ip::NextHeader; @@ -182,7 +182,7 @@ impl AclRule { &self, manifest_left: &ValidatedManifest, manifest_right: &ValidatedManifest, - ) -> Result<(), ConfigError> { + ) -> ConfigResult { match (self.from.as_str(), self.to.as_str()) { (from, to) if from == manifest_left.name() && to == manifest_right.name() => Ok(()), (from, to) if from == manifest_right.name() && to == manifest_left.name() => Ok(()), @@ -221,6 +221,7 @@ impl AclRule { &src_any_ports, &dst_any_ports, )?; + validated_rule.validate_scope(manifest_left, manifest_right)?; Ok(validated_rule) } } @@ -333,7 +334,7 @@ impl ValidatedAclRule { name: &str, prefixes_acl: &mut PrefixPortsSet, prefixes_manifest: &PrefixPortsSet, - ) -> Result<(), ConfigError> { + ) -> ConfigResult { // If the list of prefixes is empty, it means we match all prefixes from the manifest. if prefixes_acl.is_empty() { *prefixes_acl = prefixes_manifest.clone(); @@ -365,7 +366,7 @@ impl ValidatedAclRule { manifest_right: &ValidatedManifest, src_any_ports: &[PortRange], dst_any_ports: &[PortRange], - ) -> Result<(), ConfigError> { + ) -> ConfigResult { // A default-only manifest's is_v4()/is_v6() are always false, so fall back to the other side let is_v4 = if manifest_left.is_default_only() { manifest_right.is_v4() @@ -387,6 +388,41 @@ impl ValidatedAclRule { Ok(()) } + + // TODO: Remove once we support flow tracking for non-NAT and static NAT flows + // See https://github.com/githedgehog/dataplane/issues/1625 + fn validate_scope( + &self, + manifest_left: &ValidatedManifest, + manifest_right: &ValidatedManifest, + ) -> ConfigResult { + if self.scope() != AclScope::Flow { + return Ok(()); + } + // If one side uses masquerade or port forwarding for all exposes, then we're good + if manifest_left.valexp().iter().all(|expose| { + expose + .nat() + .is_some_and(|nat| nat.is_masquerade() || nat.is_port_forwarding()) + }) { + return Ok(()); + } + // If the other side uses masquerade or port forwarding for all exposes, then we're good + if manifest_right.valexp().iter().all(|expose| { + expose + .nat() + .is_some_and(|nat| nat.is_masquerade() || nat.is_port_forwarding()) + }) { + return Ok(()); + } + // If both sides have at least one prefix without masquerade or port forwarding, the peering + // will expose flows that won't get entries in the flow table, and ACLs with "scope: flow" + // are not supported + Err(ConfigError::InvalidAcl(format!( + "ACL rule '{}': At the moment, 'scope: flow' is only supported when all connections in the peering use masquerade or port forwarding", + self.name, + ))) + } } #[derive(Debug, Clone, PartialEq, Eq, Default)] @@ -411,7 +447,7 @@ impl Acl { &self.rules } - fn validate_rules_names(&self) -> Result<(), ConfigError> { + fn validate_rules_names(&self) -> ConfigResult { let mut seen_names = std::collections::HashSet::new(); for rule in &self.rules { if rule.name.is_empty() { @@ -500,6 +536,20 @@ mod validation_tests { .unwrap() } + // Helper: build a validated manifest, exposing the given prefixes with masquerade + fn manifest_masquerade(name: &str, ips: &[&str], as_ranges: &[&str]) -> ValidatedManifest { + let mut expose = VpcExpose::empty().make_masquerade(None).unwrap(); + for ip in ips { + expose = expose.ip((*ip).into()); + } + for as_range in as_ranges { + expose = expose.as_range((*as_range).into()).unwrap(); + } + VpcManifest::with_exposes(name, vec![expose]) + .validate() + .unwrap() + } + // Helper: build a validated manifest with only a default expose (no concrete prefixes) fn default_manifest(name: &str) -> ValidatedManifest { VpcManifest::with_exposes(name, vec![VpcExpose::empty().set_default()]) @@ -545,6 +595,25 @@ mod validation_tests { // Helper: assemble an AclRule fn rule(name: &str, from: &str, to: &str, action: AclAction, pattern: AclPattern) -> AclRule { + AclRule { + name: name.to_owned(), + from: from.to_owned(), + to: to.to_owned(), + action, + pattern, + scope: AclScope::Packet, + log: false, + } + } + + // Helper: assemble an AclRule + fn rule_scope_flow( + name: &str, + from: &str, + to: &str, + action: AclAction, + pattern: AclPattern, + ) -> AclRule { AclRule { name: name.to_owned(), from: from.to_owned(), @@ -999,6 +1068,44 @@ mod validation_tests { ); } + // ============================================================================================= + // User ACLs limitation: restriction on "scope: flow" + // ============================================================================================= + + #[test] + fn test_flow_scope_rejected_when_no_flow_tracking() { + let left = manifest("VPC-1", &["10.0.0.0/16"]); + let right = manifest("VPC-2", &["10.1.0.0/16"]); + + let p = pattern( + PrefixPortsSet::new(), + PrefixPortsSet::new(), + AclProtoMatch::Any, + ); + let rule = rule_scope_flow("r", "VPC-1", "VPC-2", AclAction::Allow, p); + + let result = rule.validate(&left, &right); + assert!( + matches!(result, Err(ConfigError::InvalidAcl(_))), + "{result:?}" + ); + } + + #[test] + fn test_flow_scope_valid_with_flow_tracking() { + let left = manifest("VPC-1", &["10.0.0.0/16"]); + let right = manifest_masquerade("VPC-2", &["1.0.0.0/16"], &["10.1.0.0/16"]); + + let p = pattern( + PrefixPortsSet::new(), + PrefixPortsSet::new(), + AclProtoMatch::Any, + ); + let rule = rule_scope_flow("r", "VPC-1", "VPC-2", AclAction::Allow, p); + + rule.validate(&left, &right).expect("should validate"); + } + // ============================================================================================= // Default ACL values // =============================================================================================