diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 070ec9f33..ba9c43067 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/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 diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index ccb3d1658..7b951c5ed 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/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) { diff --git a/spinifex/network/policy/acl.go b/spinifex/network/policy/acl.go index f9366fa8e..4213607c2 100644 --- a/spinifex/network/policy/acl.go +++ b/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 { diff --git a/spinifex/network/policy/acl_test.go b/spinifex/network/policy/acl_test.go index bb7f773ab..59731f1a8 100644 --- a/spinifex/network/policy/acl_test.go +++ b/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) diff --git a/spinifex/network/policy/sg.go b/spinifex/network/policy/sg.go index f8d460af7..0b92177e6 100644 --- a/spinifex/network/policy/sg.go +++ b/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) }