diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index ccee476f3..a37b6b9bb 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -54,13 +54,14 @@ func normalizeIPProtocol(proto string) (string, error) { 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), IpProtocol values the ACL builder cannot express, and -// SourceSG values not matching the sg-ID format. At least one source must be -// specified. +// validateSGRule rejects CidrIp values that are non-canonical or IPv6, CidrIpv6 +// values that are non-canonical or IPv4, IpProtocol values the ACL builder +// cannot express, and SourceSG values not matching the sg-ID format. At least +// one source must be specified. The two CIDR fields stay strictly separated so +// that address family remains part of a rule's identity. func validateSGRule(r SGRule) error { - if r.CidrIp == "" && r.SourceSG == "" { - return errors.New("rule must specify CidrIp or SourceSG") + if r.CidrIp == "" && r.CidrIpv6 == "" && r.SourceSG == "" { + return errors.New("rule must specify CidrIp, CidrIpv6 or SourceSG") } if !slices.Contains(canonicalIPProtocols, r.IpProtocol) { return fmt.Errorf("invalid IpProtocol %q: must be one of %v", r.IpProtocol, canonicalIPProtocols) @@ -71,12 +72,24 @@ func validateSGRule(r SGRule) error { return fmt.Errorf("invalid CidrIp %q: %w", r.CidrIp, err) } if ipnet.IP.To4() == nil { - return fmt.Errorf("invalid CidrIp %q: IPv6 not supported", r.CidrIp) + return fmt.Errorf("invalid CidrIp %q: IPv6 belongs in CidrIpv6", r.CidrIp) } if ipnet.String() != r.CidrIp { return fmt.Errorf("invalid CidrIp %q: not canonical (expected %q)", r.CidrIp, ipnet.String()) } } + if r.CidrIpv6 != "" { + _, ipnet, err := net.ParseCIDR(r.CidrIpv6) + if err != nil { + return fmt.Errorf("invalid CidrIpv6 %q: %w", r.CidrIpv6, err) + } + if ipnet.IP.To4() != nil { + return fmt.Errorf("invalid CidrIpv6 %q: IPv4 belongs in CidrIp", r.CidrIpv6) + } + if ipnet.String() != r.CidrIpv6 { + return fmt.Errorf("invalid CidrIpv6 %q: not canonical (expected %q)", r.CidrIpv6, ipnet.String()) + } + } if r.SourceSG != "" && !sgIDRegex.MatchString(r.SourceSG) { return fmt.Errorf("invalid SourceSG %q: must match sg-<17 hex chars>", r.SourceSG) } @@ -107,7 +120,11 @@ type SGRule struct { FromPort int64 `json:"from_port"` ToPort int64 `json:"to_port"` CidrIp string `json:"cidr_ip,omitempty"` - SourceSG string `json:"source_sg,omitempty"` // Another SG ID for intra-SG rules + // CidrIpv6 is stored and projected but never reaches the ACL builder: no + // interface here has an IPv6 address, so the rule matches nothing, which is + // what an IPv6 rule does on AWS in a VPC with no IPv6 CIDR. + CidrIpv6 string `json:"cidr_ipv6,omitempty"` + SourceSG string `json:"source_sg,omitempty"` // Another SG ID for intra-SG rules // Description is rule metadata, not identity: it is excluded from sgRuleKey // so revoke/duplicate matching ignores it. The AWS Load Balancer Controller // tags the node-SG rules it manages here and revokes them by this value. @@ -840,6 +857,9 @@ func sgRuleToSecurityGroupRule(record *SecurityGroupRecord, rule SGRule, isEgres if rule.CidrIp != "" { out.CidrIpv4 = aws.String(rule.CidrIp) } + if rule.CidrIpv6 != "" { + out.CidrIpv6 = aws.String(rule.CidrIpv6) + } if rule.SourceSG != "" { out.ReferencedGroupInfo = &ec2.ReferencedSecurityGroup{ GroupId: aws.String(rule.SourceSG), @@ -1348,11 +1368,19 @@ func ipPermissionsToDescriptionTargets(perms []*ec2.IpPermission) ([]sgDescripti targets = append(targets, sgDescriptionTarget{key: sgRuleKey(r), description: pair.Description}) } - if len(targets) == before { - if len(perm.Ipv6Ranges) > 0 { - return nil, errors.New("IPv6 rules are not supported") + for _, r6 := range perm.Ipv6Ranges { + if r6 == nil || r6.CidrIpv6 == nil { + continue } - return nil, errors.New("IpPermission must specify at least one IpRange or UserIdGroupPair") + r := SGRule{IpProtocol: proto, FromPort: fromPort, ToPort: toPort, CidrIpv6: *r6.CidrIpv6} + if err := validateSGRule(r); err != nil { + return nil, err + } + targets = append(targets, sgDescriptionTarget{key: sgRuleKey(r), description: r6.Description}) + } + + if len(targets) == before { + return nil, errors.New("IpPermission must specify at least one IpRange, Ipv6Range or UserIdGroupPair") } } return targets, nil @@ -1375,9 +1403,8 @@ func (s *VPCServiceImpl) sgRecordToEC2(record *SecurityGroupRecord, accountID st return sg } -// sgParseMode selects how ipPermissionsToSGRules handles IPv6 sources. -// Authorize rejects IPv6; Revoke silently skips it (Terraform auto-revokes -// ::/0 IPv6 egress and must not abort on no-op). +// sgParseMode selects whether ipPermissionsToSGRules mints a RuleId for each +// parsed rule. Authorize does; Revoke matches on rule identity instead. type sgParseMode int const ( @@ -1387,8 +1414,7 @@ const ( // ipPermissionsToSGRules converts AWS IpPermission slice to SGRule slice, // 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. +// tenant-supplied CidrIp/CidrIpv6/SourceSG. func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRule, error) { var rules []SGRule for _, perm := range perms { @@ -1450,24 +1476,26 @@ func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRu appended = true } - hasIPv6 := false for _, r6 := range perm.Ipv6Ranges { - if r6 != nil && r6.CidrIpv6 != nil { - hasIPv6 = true - break - } - } - if hasIPv6 { - if mode == sgParseAuthorize { - return nil, errors.New("IPv6 rules are not supported") - } - if !appended { + if r6 == nil || r6.CidrIpv6 == nil { continue } + r := SGRule{IpProtocol: proto, FromPort: fromPort, ToPort: toPort, CidrIpv6: *r6.CidrIpv6} + if r6.Description != nil { + r.Description = *r6.Description + } + if err := validateSGRule(r); err != nil { + return nil, err + } + if mode == sgParseAuthorize { + r.RuleId = utils.GenerateResourceID("sgr") + } + rules = append(rules, r) + appended = true } if !appended { - return nil, errors.New("IpPermission must specify at least one IpRange or UserIdGroupPair") + return nil, errors.New("IpPermission must specify at least one IpRange, Ipv6Range or UserIdGroupPair") } } return rules, nil @@ -1502,6 +1530,13 @@ func sgRulesToIpPermissions(rules []SGRule) []*ec2.IpPermission { } perm.IpRanges = append(perm.IpRanges, ipRange) } + if rule.CidrIpv6 != "" { + ipRange := &ec2.Ipv6Range{CidrIpv6: aws.String(rule.CidrIpv6)} + if rule.Description != "" { + ipRange.Description = aws.String(rule.Description) + } + perm.Ipv6Ranges = append(perm.Ipv6Ranges, ipRange) + } if rule.SourceSG != "" { pair := &ec2.UserIdGroupPair{GroupId: aws.String(rule.SourceSG)} if rule.Description != "" { @@ -1555,7 +1590,7 @@ func removeSGRules(existing, toRemove []SGRule) []SGRule { // sgRuleKey returns a string key for deduplication/matching of SG rules. func sgRuleKey(r SGRule) string { - return fmt.Sprintf("%s:%d:%d:%s:%s", r.IpProtocol, r.FromPort, r.ToPort, r.CidrIp, r.SourceSG) + return fmt.Sprintf("%s:%d:%d:%s:%s:%s", r.IpProtocol, r.FromPort, r.ToPort, r.CidrIp, r.CidrIpv6, r.SourceSG) } // resolveRuleIDsToRemove maps SecurityGroupRuleIds to the matching rules in the diff --git a/spinifex/handlers/ec2/vpc/security_group_descriptions_test.go b/spinifex/handlers/ec2/vpc/security_group_descriptions_test.go index 4b2bb29c3..15bbfcbe6 100644 --- a/spinifex/handlers/ec2/vpc/security_group_descriptions_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_descriptions_test.go @@ -288,7 +288,7 @@ func TestUpdateSecurityGroupRuleDescriptionsIngress_UnmatchedPermission(t *testi // TestUpdateSecurityGroupRuleDescriptions_UnresolvablePermission pins that a // permission naming no rule the store can hold is an error. Authorize rejects -// all three of these, so no matching rule can exist to describe. +// both of these, so no matching rule can exist to describe. func TestUpdateSecurityGroupRuleDescriptions_UnresolvablePermission(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) @@ -297,12 +297,6 @@ func TestUpdateSecurityGroupRuleDescriptions_UnresolvablePermission(t *testing.T ruleID := authorizeIngressTCP(t, svc, sgID, 443, "10.0.0.0/24") cases := map[string]*ec2.IpPermission{ - "ipv6 only": { - IpProtocol: aws.String("tcp"), - FromPort: aws.Int64(443), - ToPort: aws.Int64(443), - Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0"), Description: aws.String("v6")}}, - }, "prefix list only": { IpProtocol: aws.String("tcp"), FromPort: aws.Int64(443), @@ -330,6 +324,45 @@ func TestUpdateSecurityGroupRuleDescriptions_UnresolvablePermission(t *testing.T } } +// TestUpdateSecurityGroupRuleDescriptionsIngress_IPv6 pins that the description +// path reaches an IPv6 rule. It used to be unreachable because authorize +// refused to store one, so an Ipv6Ranges permission could only ever be an +// error; now it resolves like any other source. +func TestUpdateSecurityGroupRuleDescriptionsIngress_IPv6(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "desc-ipv6") + + perm := &ec2.IpPermission{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(443), + ToPort: aws.Int64(443), + Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0")}}, + } + _, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{perm}, + }, testAccountID) + require.NoError(t, err) + + perm.Ipv6Ranges[0].Description = aws.String("from anywhere v6") + _, err = svc.UpdateSecurityGroupRuleDescriptionsIngress(context.Background(), &ec2.UpdateSecurityGroupRuleDescriptionsIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{perm}, + }, 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[0].IpPermissions, 1) + ranges := out.SecurityGroups[0].IpPermissions[0].Ipv6Ranges + require.Len(t, ranges, 1) + assert.Equal(t, "from anywhere v6", aws.StringValue(ranges[0].Description)) +} + // TestUpdateSecurityGroupRuleDescriptions_InvalidPermissionKeepsItsReason pins // that the underlying validation failure survives into the returned error, // rather than collapsing to a bare code the logs cannot explain. diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index c89092896..ee1c484d1 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -413,56 +413,59 @@ func TestRevokeSecurityGroupEgress_NotFound(t *testing.T) { assert.Error(t, err) } -// TestRevokeSecurityGroupEgress_IPv6OnlyIsNoOp asserts that revoking a -// IPv6-only permission succeeds as a no-op; Terraform auto-revokes ::/0 IPv6 -// egress and must not abort when spinifex stores no IPv6 rules. -func TestRevokeSecurityGroupEgress_IPv6OnlyIsNoOp(t *testing.T) { +// TestRevokeSecurityGroupEgress_IPv6NotFound: an IPv6 rule that was never +// authorized is missing in the ordinary way. The old silent no-op existed only +// because authorize rejected IPv6, so a caller's state could never match what +// was stored; with authorize working there is nothing left to paper over, and a +// v6 revoke behaves exactly like the v4 revoke beside it. +func TestRevokeSecurityGroupEgress_IPv6NotFound(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") - sgID := createTestSG(t, svc, vpcID, "ipv6-revoke-noop") + sgID := createTestSG(t, svc, vpcID, "ipv6-revoke-notfound") allProto := "-1" - out, err := svc.RevokeSecurityGroupEgress(context.Background(), &ec2.RevokeSecurityGroupEgressInput{ + _, err := svc.RevokeSecurityGroupEgress(context.Background(), &ec2.RevokeSecurityGroupEgressInput{ GroupId: aws.String(sgID), IpPermissions: []*ec2.IpPermission{{ IpProtocol: &allProto, Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0")}}, }}, }, testAccountID) - require.NoError(t, err) - require.NotNil(t, out) - require.True(t, aws.BoolValue(out.Return)) + require.Error(t, err) + assert.Contains(t, err.Error(), "InvalidPermission.NotFound") } -// TestRevokeSecurityGroupIngress_IPv6OnlyIsNoOp mirrors the egress no-op test -// — the ingress path shares the same parsing code and must behave the same. -func TestRevokeSecurityGroupIngress_IPv6OnlyIsNoOp(t *testing.T) { +// TestRevokeSecurityGroupIngress_IPv6NotFound mirrors the egress case — the +// ingress path shares the same parsing code and must behave the same. +func TestRevokeSecurityGroupIngress_IPv6NotFound(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") - sgID := createTestSG(t, svc, vpcID, "ipv6-revoke-ingress-noop") + sgID := createTestSG(t, svc, vpcID, "ipv6-revoke-ingress-notfound") allProto := "-1" - out, err := svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{ + _, err := svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{ GroupId: aws.String(sgID), IpPermissions: []*ec2.IpPermission{{ IpProtocol: &allProto, Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0")}}, }}, }, testAccountID) - require.NoError(t, err) - require.NotNil(t, out) - require.True(t, aws.BoolValue(out.Return)) + require.Error(t, err) + assert.Contains(t, err.Error(), "InvalidPermission.NotFound") } -// TestAuthorizeSecurityGroupEgress_IPv6Rejected asserts that authorizing -// IPv6 sources is rejected; spinifex cannot program IPv6 OVN ACLs. -func TestAuthorizeSecurityGroupEgress_IPv6Rejected(t *testing.T) { +// TestAuthorizeSecurityGroupEgress_IPv6Accepted pins the whole of TFC-0001: the +// module's default egress rule carries ::/0, the provider has already revoked +// the group's own default egress by the time it is authorized, and rejecting it +// left the group with no egress at all. The v4 half here is the group's default +// rule, so only the v6 half is authorized. +func TestAuthorizeSecurityGroupEgress_IPv6Accepted(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") - sgID := createTestSG(t, svc, vpcID, "ipv6-authorize-rejected") + sgID := createTestSG(t, svc, vpcID, "ipv6-authorize-accepted") allProto := "-1" _, err := svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ @@ -472,8 +475,57 @@ func TestAuthorizeSecurityGroupEgress_IPv6Rejected(t *testing.T) { Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0")}}, }}, }, testAccountID) - require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + 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].IpPermissionsEgress, 1) + perm := out.SecurityGroups[0].IpPermissionsEgress[0] + require.Len(t, perm.IpRanges, 1) + require.Len(t, perm.Ipv6Ranges, 1) + assert.Equal(t, "0.0.0.0/0", aws.StringValue(perm.IpRanges[0].CidrIp)) + assert.Equal(t, "::/0", aws.StringValue(perm.Ipv6Ranges[0].CidrIpv6)) +} + +// TestRevokeSecurityGroupEgress_IPv6RevokesOnlyIPv6 pins that the two families +// are separately revocable. They share every other field, so a key that ignored +// the family would take both out at once. +func TestRevokeSecurityGroupEgress_IPv6RevokesOnlyIPv6(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "ipv6-revoke-scoped") + + allProto := "-1" + _, err := svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: &allProto, + Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0")}}, + }}, + }, testAccountID) + require.NoError(t, err) + + _, err = svc.RevokeSecurityGroupEgress(context.Background(), &ec2.RevokeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: &allProto, + Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String("::/0")}}, + }}, + }, 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[0].IpPermissionsEgress, 1) + perm := out.SecurityGroups[0].IpPermissionsEgress[0] + assert.Len(t, perm.IpRanges, 1) + assert.Empty(t, perm.Ipv6Ranges) } // TestRevokeSecurityGroupEgress_RuleNotFound: egress counterpart — revoking a @@ -720,8 +772,9 @@ func TestIpPermissionsToSGRules_SourceSG(t *testing.T) { assert.Equal(t, srcSG, rules[0].SourceSG) } -// TestIpPermissionsToSGRules_IPv6OnlyAuthorize rejects authorize calls whose -// only source is Ipv6Ranges (OVN ACL builder is IPv4-only). +// TestIpPermissionsToSGRules_IPv6OnlyAuthorize accepts a permission whose only +// source is Ipv6Ranges. The module's default egress rule is exactly this, and +// rejecting it left the group with no egress at all. func TestIpPermissionsToSGRules_IPv6OnlyAuthorize(t *testing.T) { t.Parallel() allProto := "-1" @@ -732,14 +785,16 @@ func TestIpPermissionsToSGRules_IPv6OnlyAuthorize(t *testing.T) { }, } - _, err := ipPermissionsToSGRules(perms, sgParseAuthorize) - require.Error(t, err) - assert.Contains(t, err.Error(), "IPv6") + rules, err := ipPermissionsToSGRules(perms, sgParseAuthorize) + require.NoError(t, err) + require.Len(t, rules, 1) + assert.Equal(t, "::/0", rules[0].CidrIpv6) + assert.Empty(t, rules[0].CidrIp) + assert.NotEmpty(t, rules[0].RuleId) } -// TestIpPermissionsToSGRules_IPv6OnlyRevoke tolerates IPv6-only permissions on -// revoke (returns zero rules, no error), matching Terraform's auto-revoke of -// the default ::/0 IPv6 egress rule. +// TestIpPermissionsToSGRules_IPv6OnlyRevoke parses an IPv6-only permission on +// revoke into the same rule authorize stored, so the revoke can match it. func TestIpPermissionsToSGRules_IPv6OnlyRevoke(t *testing.T) { t.Parallel() allProto := "-1" @@ -752,12 +807,14 @@ func TestIpPermissionsToSGRules_IPv6OnlyRevoke(t *testing.T) { rules, err := ipPermissionsToSGRules(perms, sgParseRevoke) require.NoError(t, err) - assert.Empty(t, rules) + require.Len(t, rules, 1) + assert.Equal(t, "::/0", rules[0].CidrIpv6) + assert.Empty(t, rules[0].RuleId) } -// TestIpPermissionsToSGRules_MixedV4V6Revoke keeps the v4 rule and drops the -// v6 part on a mixed-source permission on revoke. -func TestIpPermissionsToSGRules_MixedV4V6Revoke(t *testing.T) { +// TestIpPermissionsToSGRules_MixedV4V6 splits a permission carrying both +// families into one rule each, in both modes. +func TestIpPermissionsToSGRules_MixedV4V6(t *testing.T) { t.Parallel() proto := "tcp" perms := []*ec2.IpPermission{ @@ -770,10 +827,62 @@ func TestIpPermissionsToSGRules_MixedV4V6Revoke(t *testing.T) { }, } - rules, err := ipPermissionsToSGRules(perms, sgParseRevoke) - require.NoError(t, err) - require.Len(t, rules, 1) - assert.Equal(t, "10.0.0.0/8", rules[0].CidrIp) + for _, mode := range []sgParseMode{sgParseAuthorize, sgParseRevoke} { + rules, err := ipPermissionsToSGRules(perms, mode) + require.NoError(t, err) + require.Len(t, rules, 2) + assert.Equal(t, "10.0.0.0/8", rules[0].CidrIp) + assert.Empty(t, rules[0].CidrIpv6) + assert.Equal(t, "::/0", rules[1].CidrIpv6) + assert.Empty(t, rules[1].CidrIp) + } +} + +// TestSGRuleKey_AddressFamilyIsIdentity pins that ::/0 and 0.0.0.0/0 on the +// same protocol and ports are distinct rules. They share every other field, so +// a key omitting CidrIpv6 would collide them and a revoke of one would match +// the other. +func TestSGRuleKey_AddressFamilyIsIdentity(t *testing.T) { + t.Parallel() + v4 := SGRule{IpProtocol: "-1", CidrIp: "0.0.0.0/0"} + v6 := SGRule{IpProtocol: "-1", CidrIpv6: "::/0"} + assert.NotEqual(t, sgRuleKey(v4), sgRuleKey(v6)) +} + +// TestValidateSGRule_AddressFamiliesDoNotCross keeps each CIDR field to its own +// family, so the family stays part of a rule's identity rather than something +// the caller can smuggle past by choosing a field. +func TestValidateSGRule_AddressFamiliesDoNotCross(t *testing.T) { + t.Parallel() + err := validateSGRule(SGRule{IpProtocol: "-1", CidrIp: "::/0"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "CidrIpv6") + + err = validateSGRule(SGRule{IpProtocol: "-1", CidrIpv6: "0.0.0.0/0"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "CidrIp") + + err = validateSGRule(SGRule{IpProtocol: "-1", CidrIpv6: "2001:db8::1/64"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "canonical") + + require.NoError(t, validateSGRule(SGRule{IpProtocol: "-1", CidrIpv6: "::/0"})) +} + +// TestSGRulesToIpPermissions_IPv6RoundTrip pins that a v6 rule comes back in +// Ipv6Ranges rather than IpRanges, sharing the protocol and port grouping with +// the v4 rule beside it. +func TestSGRulesToIpPermissions_IPv6RoundTrip(t *testing.T) { + t.Parallel() + perms := sgRulesToIpPermissions([]SGRule{ + {IpProtocol: "-1", CidrIp: "0.0.0.0/0"}, + {IpProtocol: "-1", CidrIpv6: "::/0"}, + }) + require.Len(t, perms, 1) + require.Len(t, perms[0].IpRanges, 1) + require.Len(t, perms[0].Ipv6Ranges, 1) + assert.Equal(t, "0.0.0.0/0", *perms[0].IpRanges[0].CidrIp) + assert.Equal(t, "::/0", *perms[0].Ipv6Ranges[0].CidrIpv6) } func TestSGRulesToIpPermissions_Conversion(t *testing.T) { diff --git a/spinifex/network/reconcile/intent.go b/spinifex/network/reconcile/intent.go index ee0a1a21e..e9b701339 100644 --- a/spinifex/network/reconcile/intent.go +++ b/spinifex/network/reconcile/intent.go @@ -699,20 +699,26 @@ func loadNATGWs( return nil } -// sgRulesToPolicyRules adapts handler-side SGRule to policy.Rule. +// sgRulesToPolicyRules adapts handler-side SGRule to policy.Rule, dropping any +// rule with no IPv4 source. The ACL builder is IPv4-only, and an IPv6 rule +// reaching it carries an empty CIDR and an empty SourceSG, which is not a +// narrower ACL but an unspecified one. func sgRulesToPolicyRules(in []handlers_ec2_vpc.SGRule) []policy.Rule { if len(in) == 0 { return nil } - out := make([]policy.Rule, len(in)) - for i, r := range in { - out[i] = policy.Rule{ + out := make([]policy.Rule, 0, len(in)) + for _, r := range in { + if r.CidrIp == "" && r.SourceSG == "" { + continue + } + out = append(out, policy.Rule{ IPProtocol: r.IpProtocol, FromPort: r.FromPort, ToPort: r.ToPort, CIDR: r.CidrIp, SourceSG: r.SourceSG, - } + }) } return out } diff --git a/spinifex/network/reconcile/intent_test.go b/spinifex/network/reconcile/intent_test.go index c1ac7bf27..1f89e02e5 100644 --- a/spinifex/network/reconcile/intent_test.go +++ b/spinifex/network/reconcile/intent_test.go @@ -628,3 +628,22 @@ func mustJSON(t *testing.T, v any) []byte { } return b } + +// TestSGRulesToPolicyRules_DropsIPv6 pins that an IPv6 rule never reaches the +// ACL builder. It carries no CIDR and no SourceSG, so an ACL built from it +// would be unspecified rather than narrow, and no interface has an IPv6 +// address for it to match. +func TestSGRulesToPolicyRules_DropsIPv6(t *testing.T) { + t.Parallel() + out := sgRulesToPolicyRules([]handlers_ec2_vpc.SGRule{ + {IpProtocol: "-1", CidrIp: "0.0.0.0/0"}, + {IpProtocol: "-1", CidrIpv6: "::/0"}, + {IpProtocol: "tcp", FromPort: 443, ToPort: 443, SourceSG: "sg-abc"}, + }) + if len(out) != 2 { + t.Fatalf("want 2 policy rules, got %d: %+v", len(out), out) + } + if out[0].CIDR != "0.0.0.0/0" || out[1].SourceSG != "sg-abc" { + t.Fatalf("wrong rules survived: %+v", out) + } +} diff --git a/spinifex/network/subscribers/security.go b/spinifex/network/subscribers/security.go index e0ba217fb..2c751b268 100644 --- a/spinifex/network/subscribers/security.go +++ b/spinifex/network/subscribers/security.go @@ -24,16 +24,22 @@ func (e SGEvent) toSpec() policy.SGSpec { } } +// toPolicyRules drops any rule with no IPv4 source. The ACL builder is +// IPv4-only, and an IPv6 rule reaching it carries an empty CIDR and an empty +// SourceSG, which is not a narrower ACL but an unspecified one. func toPolicyRules(in []SGRule) []policy.Rule { - out := make([]policy.Rule, len(in)) - for i, r := range in { - out[i] = policy.Rule{ + out := make([]policy.Rule, 0, len(in)) + for _, r := range in { + if r.CidrIp == "" && r.SourceSG == "" { + continue + } + out = append(out, policy.Rule{ IPProtocol: r.IpProtocol, FromPort: r.FromPort, ToPort: r.ToPort, CIDR: r.CidrIp, SourceSG: r.SourceSG, - } + }) } return out } diff --git a/spinifex/network/subscribers/subscribers_test.go b/spinifex/network/subscribers/subscribers_test.go index 614dfe74e..42cc3e584 100644 --- a/spinifex/network/subscribers/subscribers_test.go +++ b/spinifex/network/subscribers/subscribers_test.go @@ -686,3 +686,18 @@ func TestHandlePort_NilFlusherIsNoop(t *testing.T) { sub.handleCreatePort(&nats.Msg{Data: mustJSON(t, evt)}) sub.handleDeletePort(&nats.Msg{Data: mustJSON(t, evt)}) } + +// TestToPolicyRules_DropsIPv6 pins that an IPv6 rule never reaches the ACL +// builder. It carries no CIDR and no SourceSG, so an ACL built from it would be +// unspecified rather than narrow. +func TestToPolicyRules_DropsIPv6(t *testing.T) { + t.Parallel() + out := toPolicyRules([]SGRule{ + {IpProtocol: "-1", CidrIp: "0.0.0.0/0"}, + {IpProtocol: "-1"}, + {IpProtocol: "tcp", FromPort: 443, ToPort: 443, SourceSG: "sg-abc"}, + }) + require.Len(t, out, 2) + assert.Equal(t, "0.0.0.0/0", out[0].CIDR) + assert.Equal(t, "sg-abc", out[1].SourceSG) +}