gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-19 10:52:04 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: unblock DHCP under narrow-egress SGs

Browse Source
This commit is contained in:
Julian Sommer
2026-05-13 13:40:50 +10:00
parent 7c08c62e0d
commit c19f823d1e
3 changed files with 130 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/services/vpcd/reconcile_sgs_test.go
+1 -1
View File
@@ -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
spinifex/services/vpcd/security.go
+57 -6
View File
@@ -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",
}
}
spinifex/services/vpcd/security_test.go
+72 -16
View File
@@ -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).