From 8f539d431f6a72b26c75294b58e93f9ebc60d102 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Mon, 14 Sep 2026 00:17:05 -0300 Subject: [PATCH] fix(ec2): enforce security-group egress and ingress as independent gates SG enforcement rendered every instance's rules into one nftables `forward` chain: an instance's egress allows and default-deny, then the next instance's ingress allows and default-deny. nftables evaluates a chain first-match, so for a packet A -> B, A's egress `ip saddr A accept` (the default security group allows all egress) ended evaluation before B's ingress `ip daddr B drop` was reached whenever A was emitted before B. Instances are keyed by random instance id, so traffic that B's security group denies got through about half the time. That is the cause of the `EC2 SG enforcement (privileged)` job failing intermittently on main and on unrelated PRs: the ruleset captured at failure shows `ip saddr 172.18.0.2 accept` ahead of `ip daddr 172.18.0.3 drop comment "default-deny ingress"`. On AWS both gates must permit a packet. Render them as two base chains on the forward hook, `egress` then `ingress`: an `accept` ends only its own chain and the packet still traverses the other, where a drop is final. NACL denies go in the chain for their direction. The bridge-family L2 table gets the same structure through a shared renderer. Test: a ruleset evaluator with nftables base-chain semantics checks that A -> B is dropped under the default security group in both instance orders, and allowed once B permits ingress from A, for both tables. It fails against the single-chain renderer. --- .../tests/ec2_sg_enforcement_real.rs | 14 +- crates/fakecloud-ec2/src/runtime/firewall.rs | 297 ++++++++++++------ website/content/docs/services/ec2.md | 2 +- 3 files changed, 202 insertions(+), 111 deletions(-) diff --git a/crates/fakecloud-e2e/tests/ec2_sg_enforcement_real.rs b/crates/fakecloud-e2e/tests/ec2_sg_enforcement_real.rs index 465581e2c..72067474b 100644 --- a/crates/fakecloud-e2e/tests/ec2_sg_enforcement_real.rs +++ b/crates/fakecloud-e2e/tests/ec2_sg_enforcement_real.rs @@ -229,15 +229,17 @@ async fn security_group_actually_drops_and_allows_packets() { did not engage (check that `nft` is on PATH and the process has CAP_NET_ADMIN)" ); - // 1) Enforced deny: with no ingress allow, A cannot reach B. If the packet - // still flows despite the deny rule being installed (asserted above), the most - // common cause is bridge netfilter being off — same-subnet traffic is then - // L2-switched straight past the nft `forward` chain. Surface that state in the - // failure so it reads as the real cause, not a vague "not dropped". + // 1) Enforced deny: with no ingress allow, A cannot reach B -- even though + // A's default security group allows all egress, B's ingress is an + // independent gate. If the packet still flows despite the deny rule being + // installed (asserted above), the most common cause is bridge netfilter + // being off — same-subnet traffic is then L2-switched straight past the nft + // `forward` hook. Surface that state in the failure so it reads as the real + // cause, not a vague "not dropped". assert!( !wait_ping(&ca, &b_ip, false), "SG with no ingress allow must DROP the packet (real enforcement); \ - deny rule for {b_ip} IS installed, so the packet bypassed the forward chain. \ + deny rule for {b_ip} IS installed, so the packet bypassed the forward hook. \ bridge-nf-call-iptables={}", bridge_nf_call_iptables() ); diff --git a/crates/fakecloud-ec2/src/runtime/firewall.rs b/crates/fakecloud-ec2/src/runtime/firewall.rs index 4390464a5..e73bf45be 100644 --- a/crates/fakecloud-ec2/src/runtime/firewall.rs +++ b/crates/fakecloud-ec2/src/runtime/firewall.rs @@ -100,84 +100,25 @@ const TABLE: &str = "inet fakecloud_ec2"; /// (subnets and rules emitted in the order given; the caller sorts for /// stability) so the output can be diffed and unit-tested. /// -/// Model: a single `forward` chain, default-accept, that for every instance -/// emits its allow rules followed by a default-deny to that instance's IP. -/// Established/related traffic is accepted up front so security groups behave -/// statefully, like AWS. NACL deny rules are emitted per subnet before the -/// per-instance rules (stateless, subnet-wide). +/// Model: two base chains on the `forward` hook, both default-accept -- an +/// `egress` chain holding every instance's egress allows followed by a +/// default-deny from that instance, and an `ingress` chain holding every +/// instance's ingress allows followed by a default-deny to it. A security +/// group's egress and ingress rules are independent gates that must *both* +/// permit a packet, as on AWS. Separate base chains give exactly that: an +/// `accept` ends evaluation of its own chain only, and the packet still +/// traverses the other, where a `drop` is final. In a single chain an +/// instance's egress `accept` (the default security group allows all egress) +/// would end evaluation before a later instance's ingress default-deny was +/// reached, letting traffic through or not depending on which instance was +/// emitted first. +/// +/// Established/related traffic is accepted up front in each chain so security +/// groups behave statefully, like AWS. NACL deny rules are emitted per subnet +/// before the per-instance rules in the chain for their direction (stateless, +/// subnet-wide). pub fn render_ruleset(subnets: &[SubnetFirewall]) -> String { - let mut out = String::new(); - // `add table` first so the following `flush` doesn't error on the *first* - // apply (when the table doesn't exist yet) — which would fail the entire - // `nft -f -` load and leave enforcement silently off. `add` is idempotent; - // `add`+`flush`+re-add is the canonical atomic-replace idiom. - out.push_str(&format!("add table {TABLE}\n")); - out.push_str(&format!("flush table {TABLE}\n")); - out.push_str(&format!("table {TABLE} {{\n")); - out.push_str(" chain forward {\n"); - out.push_str(" type filter hook forward priority -5; policy accept;\n"); - // Stateful: let replies through so SG rules only need to describe the - // opening direction, matching AWS security-group semantics. - out.push_str(" ct state established,related accept\n"); - - for subnet in subnets { - out.push_str(&format!(" # subnet {}\n", subnet.network_name)); - - // 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")); - } - } - - for inst in &subnet.instances { - // Ingress: allow matching, then default-deny to this instance. - for rule in &inst.ingress { - out.push_str(&format!( - " {}\n", - render_rule(rule, Direction::Ingress, &inst.private_ip) - )); - } - out.push_str(&format!( - " ip daddr {} drop comment \"default-deny ingress\"\n", - inst.private_ip - )); - - // Egress: allow matching, then default-deny from this instance. - for rule in &inst.egress { - out.push_str(&format!( - " {}\n", - render_rule(rule, Direction::Egress, &inst.private_ip) - )); - } - out.push_str(&format!( - " ip saddr {} drop comment \"default-deny egress\"\n", - inst.private_ip - )); - } - } - - out.push_str(" }\n"); - out.push_str("}\n"); - out + render_table(TABLE, -5, "", subnets) } /// The bridge-family table fakecloud owns for **same-subnet L2 enforcement**. @@ -197,25 +138,73 @@ const BRIDGE_TABLE: &str = "bridge fakecloud_ec2_l2"; /// hold regardless of the bridge-netfilter sysctl. IPv4 matches are guarded /// with `ether type ip` (required in the bridge family before an `ip` match); /// `ct state established,related` keeps replies flowing statefully via -/// `nf_conntrack_bridge`. +/// `nf_conntrack_bridge`. Its chains sit at a lower (earlier) priority than the +/// default bridge filter so the decision lands before anything else in the +/// bridge path. pub fn render_bridge_ruleset(subnets: &[SubnetFirewall]) -> String { + render_table(BRIDGE_TABLE, -300, "ether type ip ", subnets) +} + +/// Render `table` as an atomic replace with independent `egress` and `ingress` +/// base chains on the `forward` hook (see [`render_ruleset`]). The ingress +/// chain hooks at `priority` and the egress chain one step earlier; `guard` is +/// prefixed to every rule that matches on an IPv4 address. +fn render_table(table: &str, priority: i32, guard: &str, subnets: &[SubnetFirewall]) -> String { let mut out = String::new(); - out.push_str(&format!("add table {BRIDGE_TABLE}\n")); - out.push_str(&format!("flush table {BRIDGE_TABLE}\n")); - out.push_str(&format!("table {BRIDGE_TABLE} {{\n")); - out.push_str(" chain forward {\n"); - // Lower (earlier) priority than the default bridge filter so our decision - // lands before anything else in the bridge path. - out.push_str(" type filter hook forward priority -300; policy accept;\n"); + // `add table` first so the following `flush` doesn't error on the *first* + // apply (when the table doesn't exist yet) — which would fail the entire + // `nft -f -` load and leave enforcement silently off. `add` is idempotent; + // `add`+`flush`+re-add is the canonical atomic-replace idiom. + out.push_str(&format!("add table {table}\n")); + out.push_str(&format!("flush table {table}\n")); + out.push_str(&format!("table {table} {{\n")); + for (dir, chain_priority) in [ + (Direction::Egress, priority - 1), + (Direction::Ingress, priority), + ] { + render_chain(&mut out, dir, chain_priority, guard, subnets); + } + out.push_str("}\n"); + out +} + +/// One direction's base chain: stateful accept, then per subnet its NACL +/// denies for that direction and each instance's allows followed by its +/// default-deny. +fn render_chain( + out: &mut String, + dir: Direction, + priority: i32, + guard: &str, + subnets: &[SubnetFirewall], +) { + let (name, egress) = match dir { + Direction::Egress => ("egress", true), + Direction::Ingress => ("ingress", false), + }; + out.push_str(&format!(" chain {name} {{\n")); + out.push_str(&format!( + " type filter hook forward priority {priority}; policy accept;\n" + )); + // Stateful: let replies through so SG rules only need to describe the + // opening direction, matching AWS security-group semantics. out.push_str(" ct state established,related accept\n"); for subnet in subnets { out.push_str(&format!(" # subnet {}\n", subnet.network_name)); + // 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 { + if rule.allow || rule.egress != egress { continue; } let shadowed = ordered[..i] @@ -225,38 +214,29 @@ pub fn render_bridge_ruleset(subnets: &[SubnetFirewall]) -> String { continue; } if let Some(line) = render_nacl_drop(rule) { - out.push_str(&format!(" ether type ip {line}\n")); + out.push_str(&format!(" {guard}{line}\n")); } } for inst in &subnet.instances { - for rule in &inst.ingress { - out.push_str(&format!( - " ether type ip {}\n", - render_rule(rule, Direction::Ingress, &inst.private_ip) - )); - } - out.push_str(&format!( - " ether type ip ip daddr {} drop comment \"default-deny ingress\"\n", - inst.private_ip - )); - - for rule in &inst.egress { + let (rules, default_deny) = match dir { + Direction::Egress => (&inst.egress, "saddr"), + Direction::Ingress => (&inst.ingress, "daddr"), + }; + for rule in rules { out.push_str(&format!( - " ether type ip {}\n", - render_rule(rule, Direction::Egress, &inst.private_ip) + " {guard}{}\n", + render_rule(rule, dir, &inst.private_ip) )); } out.push_str(&format!( - " ether type ip ip saddr {} drop comment \"default-deny egress\"\n", + " {guard}ip {default_deny} {} drop comment \"default-deny {name}\"\n", inst.private_ip )); } } out.push_str(" }\n"); - out.push_str("}\n"); - out } #[derive(Clone, Copy)] @@ -546,6 +526,115 @@ mod tests { } } + /// Evaluate a new (not established) all-protocol packet `src -> dst` + /// against a rendered ruleset the way nftables does: within each base + /// chain the first rule whose address matches decides, a `drop` anywhere + /// is final, and an `accept` only ends its own chain. Understands the + /// address matches these renderers emit (`ip saddr/daddr `, + /// optionally behind `ether type ip`); protocol and port clauses are not + /// modeled, so use all-protocol rules. + fn packet_passes(ruleset: &str, src: &str, dst: &str) -> bool { + use std::net::Ipv4Addr; + fn in_cidr(ip: &str, cidr: &str) -> bool { + let ip: Ipv4Addr = ip.parse().unwrap(); + let (net, len) = cidr.split_once('/').unwrap_or((cidr, "32")); + let (net, len): (Ipv4Addr, u32) = (net.parse().unwrap(), len.parse().unwrap()); + let mask = if len == 0 { 0 } else { u32::MAX << (32 - len) }; + u32::from(ip) & mask == u32::from(net) & mask + } + let mut in_chain = false; + let mut chain_decided = false; + for line in ruleset.lines().map(str::trim) { + if line.starts_with("chain ") { + in_chain = true; + chain_decided = false; + continue; + } + if line == "}" { + in_chain = false; + continue; + } + if !in_chain + || chain_decided + || line.starts_with("type ") + || line.starts_with("ct ") + || line.starts_with('#') + { + continue; + } + let line = line.strip_prefix("ether type ip ").unwrap_or(line); + let tokens: Vec<&str> = line.split_whitespace().collect(); + let mut matched = true; + let mut i = 0; + while i + 2 < tokens.len() && tokens[i] == "ip" { + let addr = if tokens[i + 1] == "saddr" { src } else { dst }; + matched &= in_cidr(addr, tokens[i + 2]); + i += 3; + } + if !matched { + continue; + } + match tokens.get(i) { + Some(&"drop") => return false, + Some(&"accept") => chain_decided = true, + other => panic!("unmodeled rule {line:?} ({other:?})"), + } + } + true + } + + #[test] + fn egress_and_ingress_are_independent_gates_whatever_the_instance_order() { + // The default security group: all egress allowed, no ingress. A packet + // from A to B passes A's egress but must still be dropped by B's + // ingress default-deny -- regardless of which instance is emitted + // first. In a single chain, A's egress accept ended evaluation before + // B's deny whenever A came first. + let anywhere = || FirewallRule { + protocol: "-1".into(), + from_port: -1, + to_port: -1, + cidr: None, + }; + let a = "172.30.0.2"; + let b = "172.30.0.3"; + let instance = |ip: &str, ingress: Vec| InstanceFirewall { + private_ip: ip.into(), + ingress, + egress: vec![anywhere()], + }; + let model = |instances| { + vec![SubnetFirewall { + network_name: "fakecloud-subnet-a".into(), + instances, + nacl: vec![], + }] + }; + for render in [render_ruleset, render_bridge_ruleset] { + for instances in [ + vec![instance(a, vec![]), instance(b, vec![])], + vec![instance(b, vec![]), instance(a, vec![])], + ] { + let rs = render(&model(instances)); + assert!(!packet_passes(&rs, a, b), "A -> B must be dropped:\n{rs}"); + assert!(!packet_passes(&rs, b, a), "B -> A must be dropped:\n{rs}"); + } + // B allows ingress from A: now A -> B passes, B -> A still doesn't. + let from_a = FirewallRule { + cidr: Some(format!("{a}/32")), + ..anywhere() + }; + for instances in [ + vec![instance(a, vec![]), instance(b, vec![from_a.clone()])], + vec![instance(b, vec![from_a.clone()]), instance(a, vec![])], + ] { + let rs = render(&model(instances)); + assert!(packet_passes(&rs, a, b), "A -> B must be allowed:\n{rs}"); + assert!(!packet_passes(&rs, b, a), "B -> A must be dropped:\n{rs}"); + } + } + } + #[test] fn all_protocols_and_anywhere_omit_match_clauses() { let rule = FirewallRule { diff --git a/website/content/docs/services/ec2.md b/website/content/docs/services/ec2.md index 0784c3681..d2bd629f1 100644 --- a/website/content/docs/services/ec2.md +++ b/website/content/docs/services/ec2.md @@ -44,7 +44,7 @@ VPC/subnet/security-group/NACL metadata isn't just stored — fakecloud gives in - **Default VPC** — every account+region ships a default VPC (`172.31.0.0/16`) with an internet gateway, a main route table, one default subnet per AZ, a `default` security group, and a default NACL, exactly like AWS. `RunInstances` with no `SubnetId` lands in the default subnet and attaches the `default` security group. - **L3 isolation (Docker/Podman)** — each subnet gets its own daemon network (`fakecloud-subnet-`); instances in the same subnet share a bridge and can talk, while instances in different VPCs/subnets land on different bridges and **cannot route to each other**. Private subnets (no `0.0.0.0/0 → igw` route) back onto `--internal` networks with no NAT to the host. -- **Security-group + NACL enforcement (Docker/Podman)** — when enabled, security-group and NACL rules are translated into an **nftables** ruleset applied on the host, so SG rules actually block/allow traffic. This needs `CAP_NET_ADMIN` + `nft`, so it is **opt-in** via `FAKECLOUD_EC2_SG_ENFORCEMENT=1` and **degrades gracefully**: without the capability (CI, Docker Desktop, rootless podman) the rules are tracked but not enforced, with a one-time startup warning — L3 isolation still holds. The published image ships `nft` (plus `kmod`/`procps`, which it needs to load and enable bridge netfilter so same-subnet traffic is actually filtered); you only need to grant the capability and set the env var. With `docker run`: +- **Security-group + NACL enforcement (Docker/Podman)** — when enabled, security-group and NACL rules are translated into an **nftables** ruleset applied on the host, so SG rules actually block/allow traffic. As on AWS, a packet between two instances must be allowed by the sender's egress rules **and** the receiver's ingress rules: the two are evaluated as independent gates (separate nftables base chains), so the default security group's allow-all egress never lets traffic past the receiver's ingress default-deny. This needs `CAP_NET_ADMIN` + `nft`, so it is **opt-in** via `FAKECLOUD_EC2_SG_ENFORCEMENT=1` and **degrades gracefully**: without the capability (CI, Docker Desktop, rootless podman) the rules are tracked but not enforced, with a one-time startup warning — L3 isolation still holds. The published image ships `nft` (plus `kmod`/`procps`, which it needs to load and enable bridge netfilter so same-subnet traffic is actually filtered); you only need to grant the capability and set the env var. With `docker run`: ```sh docker run -e FAKECLOUD_EC2_SG_ENFORCEMENT=1 --cap-add=NET_ADMIN \