fix: reject unsupported SG protocols instead of emitting an allow-all ACL (#946)

spinifex/handlers/ec2/vpc/security_group.go

@@ -28,13 +28,42 @@ var sgIDRegex = regexp.MustCompile(`^sg-[0-9a-f]{17}$`)
 // re-implementing the format check.
 var SGRuleIDRegex = regexp.MustCompile(`^sgr-[0-9a-f]{17}$`)
 
+// canonicalIPProtocols are the only values the OVN ACL builder can express.
+// Anything else must be rejected: the builder emits no L4 predicate for an
+// unknown protocol, which widens the rule to every IP protocol.
+var canonicalIPProtocols = []string{"tcp", "udp", "icmp", "-1"}
+
+// numericIPProtocols maps the IANA numbers AWS accepts onto the canonical
+// names. 58 (ICMPv6) is absent deliberately: the ACL builder is IPv4-only.
+var numericIPProtocols = map[string]string{"1": "icmp", "6": "tcp", "17": "udp"}
+
+// normalizeIPProtocol canonicalises an AWS IpProtocol value the way AWS does,
+// returning "tcp" for both "tcp" and "6". An empty value means all protocols.
+func normalizeIPProtocol(proto string) (string, error) {
+	proto = strings.ToLower(strings.TrimSpace(proto))
+	if proto == "" {
+		return "-1", nil
+	}
+	if name, ok := numericIPProtocols[proto]; ok {
+		return name, nil
+	}
+	if slices.Contains(canonicalIPProtocols, proto) {
+		return proto, nil
+	}
+	return "", fmt.Errorf("invalid IpProtocol %q: supported values are tcp, udp, icmp, -1 (or 6, 17, 1)", proto)
+}
+
 // validateSGRule rejects CidrIp values that are non-canonical or IPv6 (OVN ACL
-// builder is IPv4-only), and SourceSG values not matching the sg-ID format.
-// At least one source must be specified.
+// builder is IPv4-only), IpProtocol values the ACL builder cannot express, and
+// SourceSG values not matching the sg-ID format. At least one source must be
+// specified.
 func validateSGRule(r SGRule) error {
 	if r.CidrIp == "" && r.SourceSG == "" {
 		return errors.New("rule must specify CidrIp or SourceSG")
 	}
+	if !slices.Contains(canonicalIPProtocols, r.IpProtocol) {
+		return fmt.Errorf("invalid IpProtocol %q: must be one of %v", r.IpProtocol, canonicalIPProtocols)
+	}
 	if r.CidrIp != "" {
 		_, ipnet, err := net.ParseCIDR(r.CidrIp)
 		if err != nil {
@@ -938,8 +967,9 @@ const (
 )
 
 // ipPermissionsToSGRules converts AWS IpPermission slice to SGRule slice,
-// validating every tenant-supplied CidrIp/SourceSG. IPv6-only permissions
-// error in Authorize mode and are silently skipped in Revoke mode.
+// normalising IpProtocol to its canonical name and validating every
+// tenant-supplied CidrIp/SourceSG. IPv6-only permissions error in Authorize
+// mode and are silently skipped in Revoke mode.
 func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRule, error) {
 	var rules []SGRule
 	for _, perm := range perms {
@@ -947,9 +977,13 @@ func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRu
 			continue
 		}
 
-		proto := "-1"
+		raw := ""
 		if perm.IpProtocol != nil {
-			proto = *perm.IpProtocol
+			raw = *perm.IpProtocol
+		}
+		proto, err := normalizeIPProtocol(raw)
+		if err != nil {
+			return nil, err
 		}
 
 		var fromPort, toPort int64

spinifex/handlers/ec2/vpc/security_group_test.go

@@ -825,6 +825,11 @@ func TestValidateSGRule(t *testing.T) {
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
+			// Fixtures exercise CidrIp/SourceSG; supply a valid protocol so the
+			// protocol check is never what fails.
+			if c.rule.IpProtocol == "" {
+				c.rule.IpProtocol = "-1"
+			}
 			err := validateSGRule(c.rule)
 			if c.wantErr {
 				assert.Error(t, err)
@@ -835,6 +840,42 @@ func TestValidateSGRule(t *testing.T) {
 	}
 }
 
+// validateSGRule is the last gate before the OVN ACL builder, which emits no L4
+// predicate for a protocol it does not recognise. Only canonical names pass.
+func TestValidateSGRule_Protocol(t *testing.T) {
+	for _, proto := range []string{"tcp", "udp", "icmp", "-1"} {
+		assert.NoError(t, validateSGRule(SGRule{IpProtocol: proto, CidrIp: "10.0.0.0/8"}), "protocol %q", proto)
+	}
+	for _, proto := range []string{"", "6", "17", "1", "58", "47", "icmpv6", "banana"} {
+		assert.Error(t, validateSGRule(SGRule{IpProtocol: proto, CidrIp: "10.0.0.0/8"}), "protocol %q", proto)
+	}
+}
+
+func TestNormalizeIPProtocol(t *testing.T) {
+	cases := map[string]string{
+		"":     "-1",
+		"-1":   "-1",
+		"tcp":  "tcp",
+		"TCP":  "tcp",
+		"6":    "tcp",
+		"udp":  "udp",
+		"17":   "udp",
+		"icmp": "icmp",
+		"1":    "icmp",
+	}
+	for in, want := range cases {
+		got, err := normalizeIPProtocol(in)
+		assert.NoError(t, err, "protocol %q", in)
+		assert.Equal(t, want, got, "protocol %q", in)
+	}
+
+	// 58/icmpv6 is rejected with the rest: the ACL builder is IPv4-only.
+	for _, in := range []string{"58", "icmpv6", "47", "50", "0", "256", "banana", "tcp; drop"} {
+		_, err := normalizeIPProtocol(in)
+		assert.Error(t, err, "protocol %q", in)
+	}
+}
+
 // Each handler smoke-test confirms the wire-up to ipPermissionsToSGRules; the
 // full payload table lives in TestValidateSGRule above.
 
@@ -910,6 +951,70 @@ func TestAuthorizeSecurityGroupIngress_AcceptsValidSourceSG(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// --protocol 6 must be stored as tcp, so it builds the same ACL as
+// --protocol tcp instead of an L4-less match that allows every IP protocol.
+func TestAuthorizeSecurityGroupIngress_NormalizesNumericProtocol(t *testing.T) {
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "numeric-proto-sg")
+
+	_, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("6"),
+			FromPort:   aws.Int64(22),
+			ToPort:     aws.Int64(22),
+			IpRanges:   []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	out, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{
+		GroupIds: []*string{aws.String(sgID)},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, out.SecurityGroups, 1)
+	require.Len(t, out.SecurityGroups[0].IpPermissions, 1)
+	assert.Equal(t, "tcp", aws.StringValue(out.SecurityGroups[0].IpPermissions[0].IpProtocol))
+
+	// The rule was authorized as "6"; revoking it as "tcp" must match.
+	_, err = svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{
+		GroupId: aws.String(sgID),
+		IpPermissions: []*ec2.IpPermission{{
+			IpProtocol: aws.String("tcp"),
+			FromPort:   aws.Int64(22),
+			ToPort:     aws.Int64(22),
+			IpRanges:   []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}},
+		}},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	out, err = svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{
+		GroupIds: []*string{aws.String(sgID)},
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, out.SecurityGroups, 1)
+	assert.Empty(t, out.SecurityGroups[0].IpPermissions)
+}
+
+func TestAuthorizeSecurityGroupIngress_RejectsUnsupportedProtocol(t *testing.T) {
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	sgID := createTestSG(t, svc, vpcID, "bad-proto-sg")
+
+	for _, proto := range []string{"58", "icmpv6", "47", "banana"} {
+		_, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+			GroupId: aws.String(sgID),
+			IpPermissions: []*ec2.IpPermission{{
+				IpProtocol: aws.String(proto),
+				IpRanges:   []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}},
+			}},
+		}, testAccountID)
+		require.Error(t, err, "protocol %q", proto)
+		assert.Contains(t, err.Error(), "InvalidParameterValue")
+	}
+}
+
 // --- DescribeSecurityGroups filter tests ---
 
 func TestDescribeSecurityGroups_FilterByGroupId(t *testing.T) {

spinifex/network/policy/acl.go

@@ -54,51 +54,65 @@ func InfrastructureACLs(portGroupName string) []ovn.ACLSpec {
 }
 
 // RuleACLSpecs builds priority-1000 allow ACLs with "allow-related" action.
-func RuleACLSpecs(portGroupName string, ingress, egress []Rule) []ovn.ACLSpec {
+func RuleACLSpecs(portGroupName string, ingress, egress []Rule) ([]ovn.ACLSpec, error) {
 	specs := make([]ovn.ACLSpec, 0, len(ingress)+len(egress))
 	for _, rule := range ingress {
+		match, err := BuildIngressACLMatch(portGroupName, rule)
+		if err != nil {
+			return nil, err
+		}
 		specs = append(specs, ovn.ACLSpec{
 			Direction: "to-lport",
 			Priority:  ACLPriorityTenantAllow,
-			Match:     BuildIngressACLMatch(portGroupName, rule),
+			Match:     match,
 			Action:    "allow-related",
 		})
 	}
 	for _, rule := range egress {
+		match, err := BuildEgressACLMatch(portGroupName, rule)
+		if err != nil {
+			return nil, err
+		}
 		specs = append(specs, ovn.ACLSpec{
 			Direction: "from-lport",
 			Priority:  ACLPriorityTenantAllow,
-			Match:     BuildEgressACLMatch(portGroupName, rule),
+			Match:     match,
 			Action:    "allow-related",
 		})
 	}
-	return specs
+	return specs, nil
 }
 
 // BuildIngressACLMatch builds an OVN to-lport match expression.
-func BuildIngressACLMatch(portGroupName string, rule Rule) string {
+func BuildIngressACLMatch(portGroupName string, rule Rule) (string, error) {
 	parts := []string{fmt.Sprintf("outport == @%s", portGroupName), "ip4"}
-	parts = appendProtocolMatch(parts, rule)
+	parts, err := appendProtocolMatch(parts, rule)
+	if err != nil {
+		return "", err
+	}
 	if rule.CIDR != "" && rule.CIDR != "0.0.0.0/0" {
 		parts = append(parts, fmt.Sprintf("ip4.src == %s", rule.CIDR))
 	}
 	if rule.SourceSG != "" {
 		parts = append(parts, fmt.Sprintf("ip4.src == $%s", addressSetName(topology.SecurityGroupPortGroup(rule.SourceSG))))
 	}
-	return strings.Join(parts, " && ")
+	return strings.Join(parts, " && "), nil
 }
 
 // BuildEgressACLMatch builds an OVN from-lport match.
-func BuildEgressACLMatch(portGroupName string, rule Rule) string {
+func BuildEgressACLMatch(portGroupName string, rule Rule) (string, error) {
 	parts := []string{fmt.Sprintf("inport == @%s", portGroupName), "ip4"}
-	parts = appendProtocolMatch(parts, rule)
+	parts, err := appendProtocolMatch(parts, rule)
+	if err != nil {
+		return "", err
+	}
 	if rule.CIDR != "" && rule.CIDR != "0.0.0.0/0" {
 		parts = append(parts, fmt.Sprintf("ip4.dst == %s", rule.CIDR))
 	}
 	if rule.SourceSG != "" {
 		parts = append(parts, fmt.Sprintf("ip4.dst == $%s", addressSetName(topology.SecurityGroupPortGroup(rule.SourceSG))))
 	}
-	return strings.Join(parts, " && ")
+	return strings.Join(parts, " && "), nil
 }
 
 // addressSetName returns the ovn-northd-derived SB Address_Set name for a PG's
@@ -107,18 +121,22 @@ func addressSetName(portGroupName string) string {
 	return portGroupName + "_ip4"
 }
 
-func appendProtocolMatch(parts []string, rule Rule) []string {
+// appendProtocolMatch errors on an unrecognised protocol rather than emitting
+// no L4 predicate: a silent no-op here widens the rule to every IP protocol.
+// Callers must normalise numeric IANA values (6, 17, 1) to names first.
+func appendProtocolMatch(parts []string, rule Rule) ([]string, error) {
 	switch rule.IPProtocol {
 	case "tcp":
-		parts = appendPortMatch(parts, "tcp", rule.FromPort, rule.ToPort)
+		return appendPortMatch(parts, "tcp", rule.FromPort, rule.ToPort), nil
 	case "udp":
-		parts = appendPortMatch(parts, "udp", rule.FromPort, rule.ToPort)
+		return appendPortMatch(parts, "udp", rule.FromPort, rule.ToPort), nil
 	case "icmp":
-		parts = append(parts, "icmp4")
+		return append(parts, "icmp4"), nil
 	case "-1", "":
+		return parts, nil
 	default:
+		return nil, fmt.Errorf("unsupported IP protocol %q", rule.IPProtocol)
 	}
-	return parts
 }
 
 func appendPortMatch(parts []string, proto string, fromPort, toPort int64) []string {

spinifex/network/policy/acl_test.go

@@ -7,12 +7,13 @@ import (
 )
 
 func TestACL_TCPPortFromCIDR(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "tcp",
 		FromPort:   22,
 		ToPort:     22,
 		CIDR:       "10.0.0.0/8",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "tcp.dst == 22")
 	assert.Contains(t, match, "ip4.src == 10.0.0.0/8")
 	assert.Contains(t, match, "outport == @sg_test")
@@ -20,40 +21,44 @@ func TestACL_TCPPortFromCIDR(t *testing.T) {
 }
 
 func TestACL_AllTrafficFromSG(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "-1",
 		SourceSG:   "sg-abc123",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "ip4.src == $sg_abc123_ip4")
 	assert.Contains(t, match, "outport == @sg_test")
 	assert.Contains(t, match, "ip4")
 }
 
 func TestACL_PortRange(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "udp",
 		FromPort:   1024,
 		ToPort:     65535,
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "udp.dst >= 1024")
 	assert.Contains(t, match, "udp.dst <= 65535")
 }
 
 func TestACL_ICMP(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "icmp",
 		CIDR:       "0.0.0.0/0",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "icmp4")
 	assert.NotContains(t, match, "tcp.dst")
 	assert.NotContains(t, match, "udp.dst")
 }
 
 func TestACL_AllProtocols(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "-1",
 		CIDR:       "10.0.0.0/16",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "ip4")
 	assert.Contains(t, match, "ip4.src == 10.0.0.0/16")
 	assert.NotContains(t, match, "tcp")
@@ -62,50 +67,74 @@ func TestACL_AllProtocols(t *testing.T) {
 }
 
 func TestACL_EgressAll(t *testing.T) {
-	match := BuildEgressACLMatch("sg_test", Rule{
+	match, err := BuildEgressACLMatch("sg_test", Rule{
 		IPProtocol: "-1",
 		CIDR:       "0.0.0.0/0",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "inport == @sg_test")
 	assert.NotContains(t, match, "outport")
 	assert.Contains(t, match, "ip4")
 }
 
 func TestACL_TCPSinglePort(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "tcp",
 		FromPort:   443,
 		ToPort:     443,
 		CIDR:       "10.0.0.0/8",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "tcp.dst == 443")
 	assert.NotContains(t, match, "tcp.dst >=")
 	assert.NotContains(t, match, "tcp.dst <=")
 }
 
 func TestACL_NoSource(t *testing.T) {
-	match := BuildIngressACLMatch("sg_test", Rule{
+	match, err := BuildIngressACLMatch("sg_test", Rule{
 		IPProtocol: "tcp",
 		FromPort:   80,
 		ToPort:     80,
 		CIDR:       "0.0.0.0/0",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "tcp.dst == 80")
 	assert.NotContains(t, match, "ip4.src")
 }
 
 func TestACL_EgressFromSGToSG(t *testing.T) {
-	match := BuildEgressACLMatch("sg_test", Rule{
+	match, err := BuildEgressACLMatch("sg_test", Rule{
 		IPProtocol: "tcp",
 		FromPort:   3306,
 		ToPort:     3306,
 		SourceSG:   "sg-db-tier",
 	})
+	assert.NoError(t, err)
 	assert.Contains(t, match, "inport == @sg_test")
 	assert.Contains(t, match, "tcp.dst == 3306")
 	assert.Contains(t, match, "ip4.dst == $sg_db_tier_ip4")
 }
 
+// An unrecognised protocol must error, not fall through to a match with no L4
+// predicate — that would allow every IP protocol from the source.
+func TestACL_UnsupportedProtocolErrors(t *testing.T) {
+	for _, proto := range []string{"6", "58", "47", "banana"} {
+		_, err := BuildIngressACLMatch("sg_test", Rule{IPProtocol: proto, CIDR: "10.0.0.0/8"})
+		assert.Error(t, err, "ingress protocol %q", proto)
+
+		_, err = BuildEgressACLMatch("sg_test", Rule{IPProtocol: proto, CIDR: "10.0.0.0/8"})
+		assert.Error(t, err, "egress protocol %q", proto)
+	}
+}
+
+func TestRuleACLSpecs_UnsupportedProtocolErrors(t *testing.T) {
+	_, err := RuleACLSpecs("sg_test", []Rule{{IPProtocol: "6", FromPort: 22, ToPort: 22, CIDR: "10.0.0.0/8"}}, nil)
+	assert.Error(t, err)
+
+	_, err = RuleACLSpecs("sg_test", nil, []Rule{{IPProtocol: "6", CIDR: "10.0.0.0/8"}})
+	assert.Error(t, err)
+}
+
 func TestInfrastructureACLs_Shape(t *testing.T) {
 	specs := InfrastructureACLs("sg_test")
 	if assert.Len(t, specs, 4) {
@@ -122,10 +151,11 @@ func TestInfrastructureACLs_Shape(t *testing.T) {
 }
 
 func TestRuleACLSpecs_PriorityAndAction(t *testing.T) {
-	specs := RuleACLSpecs("sg_test",
+	specs, err := RuleACLSpecs("sg_test",
 		[]Rule{{IPProtocol: "tcp", FromPort: 80, ToPort: 80, CIDR: "0.0.0.0/0"}},
 		[]Rule{{IPProtocol: "-1", CIDR: "0.0.0.0/0"}},
 	)
+	assert.NoError(t, err)
 	if assert.Len(t, specs, 2) {
 		assert.Equal(t, ACLPriorityTenantAllow, specs[0].Priority)
 		assert.Equal(t, "to-lport", specs[0].Direction)

spinifex/network/policy/sg.go

@@ -63,7 +63,11 @@ func (m *sgManager) DeleteSG(ctx context.Context, groupID string) error {
 // ClearACLs+AddACLs split left zero-ACL gaps that defaulted traffic to drop.
 func (m *sgManager) applyACLs(ctx context.Context, sg SGSpec) error {
 	pg := topology.SecurityGroupPortGroup(sg.GroupID)
-	specs := append(InfrastructureACLs(pg), RuleACLSpecs(pg, sg.IngressRules, sg.EgressRules)...)
+	ruleSpecs, err := RuleACLSpecs(pg, sg.IngressRules, sg.EgressRules)
+	if err != nil {
+		return fmt.Errorf("build ACLs for %s: %w", pg, err)
+	}
+	specs := append(InfrastructureACLs(pg), ruleSpecs...)
 	if err := m.ovn.ReplaceACLs(ctx, pg, specs); err != nil {
 		return fmt.Errorf("replace ACLs on %s: %w", pg, err)
 	}