diff --git a/crates/fakecloud-ec2/src/runtime/firewall.rs b/crates/fakecloud-ec2/src/runtime/firewall.rs index 102645b7..27c44886 100644 --- a/crates/fakecloud-ec2/src/runtime/firewall.rs +++ b/crates/fakecloud-ec2/src/runtime/firewall.rs @@ -63,10 +63,14 @@ pub struct InstanceRules { pub egress: Vec, } -/// A subnet-level NACL entry (allow/deny, ordered by rule number by the -/// caller). NACLs are stateless and apply to the whole subnet. +/// A subnet-level NACL entry. NACLs are stateless and apply to the whole +/// subnet; AWS evaluates them in ascending `rule_number` order, first match +/// wins (so a lower-numbered `allow` shadows a higher-numbered `deny` for the +/// same traffic). #[derive(Debug, Clone, PartialEq, Eq)] pub struct NaclRule { + /// AWS rule number; lower numbers evaluate first. + pub rule_number: i64, pub egress: bool, /// True = allow, false = deny. pub allow: bool, @@ -114,8 +118,26 @@ pub fn render_ruleset(subnets: &[SubnetFirewall]) -> String { for subnet in subnets { out.push_str(&format!(" # subnet {}\n", subnet.network_name)); - // Subnet-wide NACL denies first (stateless, highest precedence). - for rule in subnet.nacl.iter().filter(|r| !r.allow) { + // Subnet-wide NACL denies, evaluated in ascending rule-number order so + // a lower-numbered `allow` shadows a higher-numbered `deny` for the + // same traffic (AWS first-match semantics). A deny is emitted as a drop + // only when no earlier-numbered allow covers the identical + // direction/protocol/ports/CIDR — otherwise the allow wins and the deny + // never fires (bug-hunt 2026-06-18 finding 1.4). NACL allows ride the + // default-accept policy (the SG layer below still applies; NACL and SG + // are independent gates, both must permit). + let mut ordered = subnet.nacl.clone(); + ordered.sort_by_key(|r| r.rule_number); + for (i, rule) in ordered.iter().enumerate() { + if rule.allow { + continue; + } + let shadowed = ordered[..i] + .iter() + .any(|earlier| earlier.allow && nacl_same_traffic(earlier, rule)); + if shadowed { + continue; + } if let Some(line) = render_nacl_drop(rule) { out.push_str(&format!(" {line}\n")); } @@ -182,6 +204,19 @@ fn render_rule(rule: &FirewallRule, dir: Direction, instance_ip: &str) -> String parts.join(" ") } +/// Whether two NACL entries match the *same* traffic (same direction, +/// protocol, port range, CIDR) — used to decide when a lower-numbered allow +/// shadows a higher-numbered deny. Conservative: only exact matches shadow, so +/// partially-overlapping rules still emit their drop (safer to over-deny than +/// to silently allow). +fn nacl_same_traffic(a: &NaclRule, b: &NaclRule) -> bool { + a.egress == b.egress + && a.protocol == b.protocol + && a.from_port == b.from_port + && a.to_port == b.to_port + && a.cidr == b.cidr +} + /// Render a NACL deny as a drop line scoped to its direction + match. Returns /// `None` for an allow rule (allows are the default-accept policy; only denies /// need an explicit line). @@ -407,6 +442,7 @@ mod tests { egress: vec![], }], nacl: vec![NaclRule { + rule_number: 100, egress: false, allow: false, protocol: "tcp".into(), @@ -428,6 +464,44 @@ mod tests { assert!(!rs.contains("nacl-allow")); } + #[test] + fn nacl_lower_numbered_allow_shadows_higher_numbered_deny() { + // AWS first-match-by-rule-number: `100 allow tcp/22 10/8` must win over + // `200 deny tcp/22 10/8`, so the deny is NOT emitted (finding 1.4). + let nacl_entry = |rule_number, allow| NaclRule { + rule_number, + egress: false, + allow, + protocol: "tcp".into(), + from_port: 22, + to_port: 22, + cidr: Some("10.0.0.0/8".into()), + }; + let model = vec![SubnetFirewall { + network_name: "fakecloud-subnet-a".into(), + instances: vec![InstanceFirewall { + private_ip: "172.30.0.2".into(), + ingress: vec![], + egress: vec![], + }], + // Intentionally out of order to exercise the sort. + nacl: vec![nacl_entry(200, false), nacl_entry(100, true)], + }]; + let rs = render_ruleset(&model); + assert!( + !rs.contains("ip saddr 10.0.0.0/8 tcp dport 22 drop"), + "a lower-numbered allow must shadow the deny:\n{rs}" + ); + + // Reverse the precedence: deny 100 before allow 200 -> deny fires. + let model2 = vec![SubnetFirewall { + network_name: "fakecloud-subnet-a".into(), + instances: vec![], + nacl: vec![nacl_entry(100, false), nacl_entry(200, true)], + }]; + assert!(render_ruleset(&model2).contains("ip saddr 10.0.0.0/8 tcp dport 22 drop")); + } + #[test] fn enforcement_mode_is_opt_in_and_capability_gated() { // not opted in -> disabled regardless of nft availability @@ -479,6 +553,7 @@ mod tests { nacls.insert( "net-a".to_string(), vec![NaclRule { + rule_number: 100, egress: false, allow: false, protocol: "-1".into(), diff --git a/crates/fakecloud-ec2/src/runtime/netpolicy.rs b/crates/fakecloud-ec2/src/runtime/netpolicy.rs index e0eec912..b191e898 100644 --- a/crates/fakecloud-ec2/src/runtime/netpolicy.rs +++ b/crates/fakecloud-ec2/src/runtime/netpolicy.rs @@ -133,7 +133,12 @@ fn ports(r: &FirewallRule) -> Option> { // port -> leave ports unset (all ports of all protocols). _ => return None, }; - if r.from_port < 0 || r.to_port < 0 { + // Ports are parsed permissively as i64; a value outside the valid + // 0..=65535 TCP/UDP port range can't be a real port. Reject the whole rule + // rather than silently truncating via `as i32` into a wrong/negative port + // (bug-hunt 2026-06-18 finding 2.1). + let valid = |p: i64| (0..=65535).contains(&p); + if !valid(r.from_port) || !valid(r.to_port) { return None; } let mut port = NetworkPolicyPort { @@ -285,6 +290,24 @@ mod tests { assert_eq!(port.end_port, Some(8100)); } + #[test] + fn out_of_range_port_is_rejected_not_truncated() { + // A port > 65535 (parsed permissively as i64) must not silently + // truncate via `as i32` into a wrong/negative port (finding 2.1). + let huge = FirewallRule { + protocol: "tcp".into(), + from_port: 70000, + to_port: 70000, + cidr: None, + }; + let r = rules("i-1", vec![huge], vec![]); + let p = &build_policies(&[r], "fakecloud", "x")[0]; + // No ports clause -> the rule isn't narrowed to a bogus port. + assert!(p.spec.as_ref().unwrap().ingress.as_ref().unwrap()[0] + .ports + .is_none()); + } + #[test] fn referenced_member_ip_becomes_ipblock() { let r = rules("i-1", vec![tcp(80, Some("172.30.0.3/32"))], vec![]); diff --git a/crates/fakecloud-ec2/src/service/firewall_model.rs b/crates/fakecloud-ec2/src/service/firewall_model.rs index 8cb796a1..e6fa064c 100644 --- a/crates/fakecloud-ec2/src/service/firewall_model.rs +++ b/crates/fakecloud-ec2/src/service/firewall_model.rs @@ -54,8 +54,9 @@ fn flatten_rule( } /// NACL deny/allow entries that apply to a subnet, as the renderer's model. -/// (Only denies become explicit nft lines; allows ride the default-accept -/// policy. We still pass both so future ordering work has the full picture.) +/// Both allows and denies are carried with their `rule_number` so the renderer +/// can apply AWS first-match-by-rule-number precedence (allow shadows a +/// higher-numbered deny). fn nacl_rules(acl: &NetworkAcl) -> Vec { acl.entries .iter() @@ -63,6 +64,7 @@ fn nacl_rules(acl: &NetworkAcl) -> Vec { // user rule — skip it so it doesn't blackhole everything. .filter(|e| e.rule_number != 32767) .map(|e| NaclRule { + rule_number: e.rule_number, egress: e.egress, allow: e.rule_action == "allow", protocol: e.protocol.clone(),