From c19f823d1e325e2ffc4ada0ed0f13f5649762911 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 13 May 2026 13:40:50 +1000 Subject: [PATCH] fix: unblock DHCP under narrow-egress SGs --- spinifex/services/vpcd/reconcile_sgs_test.go | 2 +- spinifex/services/vpcd/security.go | 63 ++++++++++++-- spinifex/services/vpcd/security_test.go | 88 ++++++++++++++++---- 3 files changed, 130 insertions(+), 23 deletions(-) diff --git a/spinifex/services/vpcd/reconcile_sgs_test.go b/spinifex/services/vpcd/reconcile_sgs_test.go index ba9fad0be..b80fd5340 100644 --- a/spinifex/services/vpcd/reconcile_sgs_test.go +++ b/spinifex/services/vpcd/reconcile_sgs_test.go @@ -71,7 +71,7 @@ func TestReconcileSGsOnce_RecreatesMissingPortGroup(t *testing.T) { assert.Equal(t, 1, result.PortGroupsRecreated) pg, err := ovn.GetPortGroup(context.Background(), portGroupName("sg-missing0000000")) require.NoError(t, err) - assert.Len(t, pg.ACLs, 4, "deny-ingress + deny-egress + 1 allow ingress + 1 allow egress") + assert.Len(t, pg.ACLs, 6, "deny-ingress + deny-egress + dhcp-egress + dhcp-ingress + 1 allow ingress + 1 allow egress") } // TestReconcileSGsOnce_RecreatesPortGroupIdempotent: a second pass on a fresh diff --git a/spinifex/services/vpcd/security.go b/spinifex/services/vpcd/security.go index e646728a0..6424f1381 100644 --- a/spinifex/services/vpcd/security.go +++ b/spinifex/services/vpcd/security.go @@ -99,10 +99,11 @@ func (h *TopologyHandler) provisionSG(ctx context.Context, groupId string, ingre } }() - // Default deny ACLs (priority 900, logged for CMMC SC.L1-3.13.1) and - // allow rules (priority 1000) go in one OVSDB transaction so a 60-rule - // SG creation is one round-trip, not 62. - specs := append([]ACLSpec{denyIngressACL(pgName), denyEgressACL(pgName)}, ruleACLSpecs(pgName, ingress, egress)...) + // Default deny ACLs (priority 900, logged for CMMC SC.L1-3.13.1), + // infrastructure DHCP allows (priority 1050), and tenant allow rules + // (priority 1000) go in one OVSDB transaction so a 60-rule SG creation + // is one round-trip, not 64. + specs := append(infrastructureACLs(pgName), ruleACLSpecs(pgName, ingress, egress)...) if err := h.ovn.AddACLs(ctx, pgName, specs); err != nil { return fmt.Errorf("add ACLs on %s: %w", pgName, err) } @@ -173,8 +174,8 @@ func (h *TopologyHandler) handleUpdateSG(msg *nats.Msg) { return } - // Re-add default deny ACLs + current rules in one transaction. - specs := append([]ACLSpec{denyIngressACL(pgName), denyEgressACL(pgName)}, ruleACLSpecs(pgName, evt.IngressRules, evt.EgressRules)...) + // Re-add default deny + DHCP-infra allow ACLs + current rules in one transaction. + specs := append(infrastructureACLs(pgName), ruleACLSpecs(pgName, evt.IngressRules, evt.EgressRules)...) if err := h.ovn.AddACLs(ctx, pgName, specs); err != nil { slog.Error("vpcd: failed to add ACLs for update", "pg", pgName, "err", err) respond(msg, err) @@ -231,3 +232,53 @@ func denyEgressACL(pgName string) ACLSpec { Severity: denyACLSeverity, } } + +// infrastructureACLs returns the platform-level ACLs that every port group +// carries regardless of tenant rules: the priority-900 default-deny pair and +// the priority-1050 DHCPv4 client/server allow pair. OVN's built-in DHCP +// responder runs inside the LS pipeline *after* the port-group ACL stage, so +// without these allows a narrow-egress tenant SG (e.g. egress restricted to +// the VPC CIDR) silently drops the guest's DHCPDISCOVER (dst=255.255.255.255) +// against the default-deny and the VM never gets an IPv4 lease. AWS users +// can't write a rule for the limited broadcast address, so the platform must +// allow DHCP itself — matching AWS's implicit behaviour. +// +// Priority 1050 sits above the default-deny (900) and above tenant rules +// (1000) so DHCP works regardless of tenant configuration; tenants in our +// model don't write ACLs at >1000, so there is no collision. +func infrastructureACLs(pgName string) []ACLSpec { + return []ACLSpec{ + denyIngressACL(pgName), + denyEgressACL(pgName), + dhcpEgressACL(pgName), + dhcpIngressACL(pgName), + } +} + +// dhcpEgressACL allows the guest's DHCP client traffic (DHCPDISCOVER, +// DHCPREQUEST: udp.src=68, udp.dst=67) out of the port. Action is plain +// "allow", not "allow-related" — DHCP is single-shot request/reply and +// allow-related on broadcast UDP interacts oddly with OVN's CT zones. +func dhcpEgressACL(pgName string) ACLSpec { + return ACLSpec{ + Direction: "from-lport", + Priority: 1050, + Match: fmt.Sprintf("inport == @%s && udp && udp.src == 68 && udp.dst == 67", pgName), + Action: "allow", + Name: pgName + "-allow-dhcp-egress", + } +} + +// dhcpIngressACL allows the OVN-generated DHCP server reply (DHCPOFFER, +// DHCPACK: udp.src=67, udp.dst=68) back to the guest. OVN's DHCP responder +// emits the reply from inside the LS pipeline, so without this matching +// to-lport allow the reply hits the priority-900 ingress default-deny. +func dhcpIngressACL(pgName string) ACLSpec { + return ACLSpec{ + Direction: "to-lport", + Priority: 1050, + Match: fmt.Sprintf("outport == @%s && udp && udp.src == 67 && udp.dst == 68", pgName), + Action: "allow", + Name: pgName + "-allow-dhcp-ingress", + } +} diff --git a/spinifex/services/vpcd/security_test.go b/spinifex/services/vpcd/security_test.go index 1e5b2f554..274303363 100644 --- a/spinifex/services/vpcd/security_test.go +++ b/spinifex/services/vpcd/security_test.go @@ -39,13 +39,17 @@ func TestSecurity_CreatePortGroup(t *testing.T) { assert.True(t, exists, "port group sg_abc123 should exist") assert.NotNil(t, pg) - // Verify default deny ACLs were created (2 deny ACLs at priority 900) - // and that each has logging enabled per CMMC SC.L1-3.13.1. + // Verify the infrastructure ACL set was emitted: 2 default-deny at + // priority 900 (logged for CMMC SC.L1-3.13.1) plus 2 DHCP-infra allows + // at priority 1050 (unlogged — high-volume, low-signal). type aclSnapshot struct { - action string - log bool - severity string - name string + action string + log bool + severity string + name string + priority int + direction string + match string } var snapshots []aclSnapshot mock.mu.Lock() @@ -55,7 +59,13 @@ func TestSecurity_CreatePortGroup(t *testing.T) { if a == nil { continue } - snap := aclSnapshot{action: a.Action, log: a.Log} + snap := aclSnapshot{ + action: a.Action, + log: a.Log, + priority: a.Priority, + direction: a.Direction, + match: a.Match, + } if a.Severity != nil { snap.severity = *a.Severity } @@ -66,18 +76,64 @@ func TestSecurity_CreatePortGroup(t *testing.T) { } mock.mu.Unlock() - assert.Equal(t, 2, aclCount, "should have 2 default deny ACLs (ingress + egress)") - denyCount := 0 + assert.Equal(t, 4, aclCount, "should have 4 infra ACLs (2 deny + 2 DHCP allow)") + denyCount, dhcpAllowCount := 0, 0 for _, s := range snapshots { - if s.action != "drop" { - continue + switch s.action { + case "drop": + denyCount++ + assert.True(t, s.log, "default deny ACL must have log=true for boundary monitoring") + assert.Equal(t, "info", s.severity, "default deny ACL must use info severity") + assert.Contains(t, s.name, "sg_abc123-deny-", "default deny ACL must have a name for syslog correlation") + assert.Equal(t, 900, s.priority, "default deny ACL must be priority 900") + case "allow": + dhcpAllowCount++ + assert.False(t, s.log, "DHCP infra allow must not be logged (high-volume, low-signal)") + assert.Equal(t, 1050, s.priority, "DHCP infra allow must be priority 1050 (above tenant rules and default-deny)") + assert.Contains(t, s.name, "sg_abc123-allow-dhcp-", "DHCP allow must have a name for operator correlation") } - denyCount++ - assert.True(t, s.log, "default deny ACL must have log=true for boundary monitoring") - assert.Equal(t, "info", s.severity, "default deny ACL must use info severity") - assert.Contains(t, s.name, "sg_abc123-deny-", "default deny ACL must have a name for syslog correlation") } assert.Equal(t, 2, denyCount, "should have 2 drop ACLs") + assert.Equal(t, 2, dhcpAllowCount, "should have 2 DHCP allow ACLs (egress client→server, ingress server→client)") +} + +// TestSecurity_DHCPInfraACLs locks the exact shape of the DHCPv4 infra-allow +// ACLs that vpcd emits on every port group. This is the fix for the +// "narrow-egress SG silently breaks DHCP" bug: OVN's DHCP responder runs +// inside the LS pipeline *after* the port-group ACL stage, so without these +// the guest's DHCPDISCOVER (dst=255.255.255.255) hits the priority-900 +// default-deny under any non-0.0.0.0/0 egress rule and the VM never gets a +// lease. +func TestSecurity_DHCPInfraACLs(t *testing.T) { + const pg = "sg_dhcp_shape" + + egress := dhcpEgressACL(pg) + assert.Equal(t, "from-lport", egress.Direction) + assert.Equal(t, 1050, egress.Priority, "must outrank tenant rules (1000) and default-deny (900)") + assert.Equal(t, "allow", egress.Action, "plain allow — DHCP is single-shot, no conntrack") + assert.Equal(t, "inport == @sg_dhcp_shape && udp && udp.src == 68 && udp.dst == 67", egress.Match) + assert.Equal(t, "sg_dhcp_shape-allow-dhcp-egress", egress.Name) + assert.False(t, egress.Log) + + ingress := dhcpIngressACL(pg) + assert.Equal(t, "to-lport", ingress.Direction) + assert.Equal(t, 1050, ingress.Priority) + assert.Equal(t, "allow", ingress.Action) + assert.Equal(t, "outport == @sg_dhcp_shape && udp && udp.src == 67 && udp.dst == 68", ingress.Match) + assert.Equal(t, "sg_dhcp_shape-allow-dhcp-ingress", ingress.Name) + assert.False(t, ingress.Log) +} + +// TestSecurity_InfrastructureACLs_Order locks the deny-before-allow ordering +// that provisionSG/handleUpdateSG rely on when concatenating infrastructure +// ACLs with tenant rule ACLs. +func TestSecurity_InfrastructureACLs_Order(t *testing.T) { + specs := infrastructureACLs("sg_order") + require.Len(t, specs, 4) + assert.Equal(t, "sg_order-deny-ingress", specs[0].Name) + assert.Equal(t, "sg_order-deny-egress", specs[1].Name) + assert.Equal(t, "sg_order-allow-dhcp-egress", specs[2].Name) + assert.Equal(t, "sg_order-allow-dhcp-ingress", specs[3].Name) } func TestSecurity_DeletePortGroup(t *testing.T) { @@ -175,7 +231,7 @@ func TestSecurity_UpdateSGAddRules(t *testing.T) { } mock.mu.Unlock() - assert.Equal(t, 4, aclCount, "should have 4 ACLs (2 deny + 2 ingress allow)") + assert.Equal(t, 6, aclCount, "should have 6 ACLs (2 deny + 2 DHCP infra allow + 2 ingress allow)") // Check that at least one match contains tcp.dst == 22. Also verify // logging policy: denies logged, allows not logged (CMMC SC.L1-3.13.1).