From 261229b65435a19dfbe7cf85add3292bd7056b99 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 7 Oct 2026 10:07:20 +1100 Subject: [PATCH] fix: reject security group rules without a protocol or tcp/udp ports, and classify rule IDs as AWS does (#1176) --- .../ec2/vpc/DescribeSecurityGroupRules.go | 8 +- .../vpc/DescribeSecurityGroupRules_test.go | 19 +- .../UpdateSecurityGroupRuleDescriptions.go | 6 +- ...pdateSecurityGroupRuleDescriptions_test.go | 2 +- spinifex/handlers/ec2/vpc/security_group.go | 75 +++--- .../security_group_rule_validation_test.go | 229 ++++++++++++++++++ .../handlers/ec2/vpc/security_group_test.go | 6 +- 7 files changed, 293 insertions(+), 52 deletions(-) create mode 100644 spinifex/handlers/ec2/vpc/security_group_rule_validation_test.go diff --git a/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules.go b/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules.go index 5acf03da5..2466e1a84 100644 --- a/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules.go +++ b/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules.go @@ -10,14 +10,14 @@ import ( "github.com/nats-io/nats.go" ) -// Nil input is valid and treated as "all rules in the account"; supplied -// SecurityGroupRuleIds are validated against the sgr-<17 hex> shape before the -// request crosses NATS so a malformed ID never hits the handler. +// Nil input is valid and treated as "all rules in the account". A supplied +// SecurityGroupRuleId AWS calls malformed is rejected before the request +// crosses NATS; whether a well-formed ID exists is the handler's to answer. func DescribeSecurityGroupRules(ctx context.Context, input *ec2.DescribeSecurityGroupRulesInput, natsConn *nats.Conn, accountID string) (ec2.DescribeSecurityGroupRulesOutput, error) { var output ec2.DescribeSecurityGroupRulesOutput if input != nil { for _, id := range input.SecurityGroupRuleIds { - if id == nil || !handlers_ec2_vpc.SGRuleIDRegex.MatchString(*id) { + if id == nil || handlers_ec2_vpc.SGRuleIDIsMalformed(*id) { return output, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) } } diff --git a/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules_test.go b/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules_test.go index 093183e90..846e48de0 100644 --- a/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules_test.go +++ b/spinifex/gateway/ec2/vpc/DescribeSecurityGroupRules_test.go @@ -23,10 +23,9 @@ func TestDescribeSecurityGroupRules_NilInput_PassesValidation(t *testing.T) { func TestDescribeSecurityGroupRules_MalformedRuleID(t *testing.T) { cases := []string{ "sgr-toolong0123456789abcdef", - "sgr-XYZ", + "SGR-0123456789abcdef0", "sg-0123456789abcdef0", "", - "sgr-0123456789abcdefX", } for _, bad := range cases { input := &ec2.DescribeSecurityGroupRulesInput{ @@ -46,12 +45,14 @@ func TestDescribeSecurityGroupRules_NilRuleIDEntry(t *testing.T) { } func TestDescribeSecurityGroupRules_ValidRuleIDPassesValidation(t *testing.T) { - // A well-formed sgr- ID passes gateway validation; the call then dives - // into the NATS service (nil conn) and fails there. - input := &ec2.DescribeSecurityGroupRulesInput{ - SecurityGroupRuleIds: []*string{aws.String("sgr-0123456789abcdef0")}, + // An ID AWS answers NotFound for, not Malformed, passes gateway validation; + // the call then dives into the NATS service (nil conn) and fails there. + for _, id := range []string{"sgr-0123456789abcdef0", "sgr-XYZ", "sgr-0123456789abcdefX"} { + input := &ec2.DescribeSecurityGroupRulesInput{ + SecurityGroupRuleIds: []*string{aws.String(id)}, + } + _, err := DescribeSecurityGroupRules(context.Background(), input, nil, testAccountID) + require.Error(t, err) + assert.NotEqual(t, awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, err.Error(), "id %q", id) } - _, err := DescribeSecurityGroupRules(context.Background(), input, nil, testAccountID) - require.Error(t, err) - assert.NotEqual(t, awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, err.Error()) } diff --git a/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions.go b/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions.go index d598c1e4d..eddecf37e 100644 --- a/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions.go +++ b/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions.go @@ -10,11 +10,11 @@ import ( "github.com/nats-io/nats.go" ) -// validateSGRuleDescriptions checks the sgr-<17 hex> shape before the request -// crosses NATS so a malformed ID never reaches the handler. +// validateSGRuleDescriptions rejects a rule ID AWS calls malformed before the +// request crosses NATS; whether a well-formed ID exists is the handler's to answer. func validateSGRuleDescriptions(descriptions []*ec2.SecurityGroupRuleDescription) error { for _, d := range descriptions { - if d == nil || d.SecurityGroupRuleId == nil || !handlers_ec2_vpc.SGRuleIDRegex.MatchString(*d.SecurityGroupRuleId) { + if d == nil || d.SecurityGroupRuleId == nil || handlers_ec2_vpc.SGRuleIDIsMalformed(*d.SecurityGroupRuleId) { return errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) } } diff --git a/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions_test.go b/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions_test.go index c17067578..6c0c3cca1 100644 --- a/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions_test.go +++ b/spinifex/gateway/ec2/vpc/UpdateSecurityGroupRuleDescriptions_test.go @@ -35,7 +35,7 @@ func TestUpdateSecurityGroupRuleDescriptions_MissingGroupId(t *testing.T) { } func TestUpdateSecurityGroupRuleDescriptions_MalformedRuleID(t *testing.T) { - for _, bad := range []string{"sgr-toolong0123456789abcdef", "sgr-XYZ", "sg-0123456789abcdef0", ""} { + for _, bad := range []string{"sgr-toolong0123456789abcdef", "SGR-0123456789abcdef0", "sg-0123456789abcdef0", ""} { _, err := gateway_ec2_vpc.UpdateSecurityGroupRuleDescriptionsIngress(context.Background(), &ec2.UpdateSecurityGroupRuleDescriptionsIngressInput{ GroupId: aws.String("sg-0123456789abcdef0"), SecurityGroupRuleDescriptions: []*ec2.SecurityGroupRuleDescription{ diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 6692f7e9c..860ff64cf 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -26,11 +26,6 @@ import ( // sgIDRegex must stay in lockstep with utils.GenerateResourceID("sg"). var sgIDRegex = regexp.MustCompile(`^sg-[0-9a-f]{17}$`) -// SGRuleIDRegex must stay in lockstep with utils.GenerateResourceID("sgr"). -// Exported so the EC2 gateway can validate SecurityGroupRuleIds without -// re-implementing the format check. -var SGRuleIDRegex = regexp.MustCompile(`^sgr-[0-9a-f]{17}$`) - // allProtocols is the IpProtocol value naming every protocol, which is also // the port value AWS reports for such a rule. const allProtocols = "-1" @@ -45,11 +40,13 @@ var canonicalIPProtocols = []string{"tcp", "udp", "icmp", "-1"} 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. +// returning "tcp" for both "tcp" and "6". An empty value is rejected as AWS +// rejects it: reading it as all protocols would open every port. func normalizeIPProtocol(proto string) (string, error) { proto = strings.ToLower(strings.TrimSpace(proto)) if proto == "" { - return "-1", nil + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Invalid value 'null' for protocol. VPC security group rules must specify protocols explicitly.") } if name, ok := numericIPProtocols[proto]; ok { return name, nil @@ -57,7 +54,18 @@ func normalizeIPProtocol(proto string) (string, error) { 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) + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "invalid IpProtocol %q: supported values are tcp, udp, icmp, -1 (or 6, 17, 1)", proto) +} + +// requireSGRulePorts rejects a tcp or udp rule missing either port, which would +// otherwise be stored as port 0. +func requireSGRulePorts(proto string, fromPort, toPort *int64) error { + if (proto == "tcp" || proto == "udp") && (fromPort == nil || toPort == nil) { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Invalid value for portRange. Must specify both from and to ports with TCP/UDP.") + } + return nil } // validateSGRulePorts rejects a tcp or udp rule whose FromPort exceeds its ToPort. @@ -887,7 +895,7 @@ func (s *VPCServiceImpl) DescribeSecurityGroupRules(ctx context.Context, input * for _, id := range input.SecurityGroupRuleIds { if !emitted[*id] { - return nil, sgRuleNotFoundError(*id) + return nil, sgRuleIDLookupError(*id) } } @@ -1242,6 +1250,9 @@ func (s *VPCServiceImpl) RevokeSecurityGroupIngress(ctx context.Context, input * revokeRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseRevoke) if err != nil { slog.WarnContext(ctx, "RevokeSecurityGroupIngress: invalid rule", "groupId", groupId, "err", err) + if _, ok := awserrors.ResolveErrorCode(err); ok { + return nil, err + } return nil, errors.New(awserrors.ErrorInvalidParameterValue) } idRules, err := resolveRuleIDsToRemove(record.IngressRules, input.SecurityGroupRuleIds) @@ -1309,6 +1320,9 @@ func (s *VPCServiceImpl) RevokeSecurityGroupEgress(ctx context.Context, input *e revokeRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseRevoke) if err != nil { slog.WarnContext(ctx, "RevokeSecurityGroupEgress: invalid rule", "groupId", groupId, "err", err) + if _, ok := awserrors.ResolveErrorCode(err); ok { + return nil, err + } return nil, errors.New(awserrors.ErrorInvalidParameterValue) } idRules, err := resolveRuleIDsToRemove(record.EgressRules, input.SecurityGroupRuleIds) @@ -1488,14 +1502,11 @@ func applySGRuleDescriptions(rules []SGRule, descriptions []*ec2.SecurityGroupRu return nil, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) } id := *d.SecurityGroupRuleId - if !SGRuleIDRegex.MatchString(id) { - return nil, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) - } // A rule on the other side, in another group or in another account is // absent from this record and so is not-found, never a silent no-op. i, ok := byID[id] if !ok { - return nil, sgRuleNotFoundError(id) + return nil, sgRuleIDLookupError(id) } if d.Description != nil { out[i].Description = *d.Description @@ -1769,21 +1780,16 @@ func sgRuleRequestToSGRule(req *ec2.SecurityGroupRuleRequest) (SGRule, error) { return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "No value specified for securityGroupRule.") } - if strings.TrimSpace(aws.StringValue(req.IpProtocol)) == "" { - return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "Invalid value 'null' for protocol. VPC security group rules must specify protocols explicitly.") - } - proto, err := normalizeIPProtocol(*req.IpProtocol) + proto, err := normalizeIPProtocol(aws.StringValue(req.IpProtocol)) if err != nil { - return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) + return SGRule{}, err } r := SGRule{IpProtocol: proto, Description: aws.StringValue(req.Description)} switch proto { case "tcp", "udp": - if req.FromPort == nil || req.ToPort == nil { - return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, - "Invalid value for portRange. Must specify both from and to ports with TCP/UDP.") + if err := requireSGRulePorts(proto, req.FromPort, req.ToPort); err != nil { + return SGRule{}, err } r.FromPort, r.ToPort = *req.FromPort, *req.ToPort if err := validateSGRulePorts(proto, r.FromPort, r.ToPort); err != nil { @@ -1850,8 +1856,8 @@ func sgRuleSourceField(r SGRule) string { const sgRuleIDMaxSuffix = 17 // SGRuleIDIsMalformed reports whether AWS rejects id as malformed: it lacks the -// lowercase "sgr-" prefix or has too long a suffix. Looser than SGRuleIDRegex, -// because AWS answers NotFound for a short or non-hex suffix such as sgr-xyz. +// lowercase "sgr-" prefix or has too long a suffix. AWS answers NotFound, not +// Malformed, for a short or non-hex suffix such as sgr-xyz. func SGRuleIDIsMalformed(id string) bool { suffix, ok := strings.CutPrefix(id, "sgr-") return !ok || len(suffix) > sgRuleIDMaxSuffix @@ -1863,10 +1869,6 @@ func sgRuleIDLookupError(id string) error { if SGRuleIDIsMalformed(id) { return awserrors.Errorf(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, "Invalid id: %q", id) } - return sgRuleNotFoundError(id) -} - -func sgRuleNotFoundError(id string) error { return awserrors.IDNotFound(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, "security group rule", id) } @@ -1923,10 +1925,18 @@ func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRu toPort = *perm.ToPort } if mode == sgParseAuthorize { + if err := requireSGRulePorts(proto, perm.FromPort, perm.ToPort); err != nil { + return nil, err + } if err := validateSGRulePorts(proto, fromPort, toPort); err != nil { return nil, err } } + // AWS ignores the ports of an all-protocol rule. Storing 0/0, as the + // default egress rule is, keeps new rules uniform with it. + if proto == allProtocols { + fromPort, toPort = 0, 0 + } appended := false for _, ipRange := range perm.IpRanges { @@ -2003,6 +2013,10 @@ func sgRulesToIpPermissions(rules []SGRule, accountID string) []*ec2.IpPermissio grouped := make(map[permKey]*ec2.IpPermission) for _, rule := range rules { key := permKey{IpProtocol: rule.IpProtocol, FromPort: rule.FromPort, ToPort: rule.ToPort} + // An all-protocol rule stored with -1/-1 is the same permission as 0/0. + if rule.IpProtocol == allProtocols { + key.FromPort, key.ToPort = 0, 0 + } perm, exists := grouped[key] if !exists { perm = &ec2.IpPermission{IpProtocol: aws.String(rule.IpProtocol)} @@ -2127,12 +2141,9 @@ func resolveRuleIDsToRemove(existing []SGRule, ruleIDs []*string) ([]SGRule, err if idp == nil || *idp == "" { continue } - if !SGRuleIDRegex.MatchString(*idp) { - return nil, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) - } r, ok := byID[*idp] if !ok { - return nil, sgRuleNotFoundError(*idp) + return nil, sgRuleIDLookupError(*idp) } out = append(out, r) } diff --git a/spinifex/handlers/ec2/vpc/security_group_rule_validation_test.go b/spinifex/handlers/ec2/vpc/security_group_rule_validation_test.go new file mode 100644 index 000000000..6f49e8683 --- /dev/null +++ b/spinifex/handlers/ec2/vpc/security_group_rule_validation_test.go @@ -0,0 +1,229 @@ +//test:in-package — builds on the package's unexported SG fixtures +//(setupTestVPCService, createTestSG, storedSGRecord) and plants rules in the +//stored record that no exported API can create. + +package handlers_ec2_vpc + +import ( + "context" + "encoding/json" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func authorizeIngress(svc *VPCServiceImpl, sgID string, perm *ec2.IpPermission) error { + _, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{perm}, + }, testAccountID) + return err +} + +// plantIngressRules appends rules straight to the stored record, standing in +// for rules written before the current validation existed. +func plantIngressRules(t *testing.T, svc *VPCServiceImpl, sgID string, rules ...SGRule) { + t.Helper() + var rec SecurityGroupRecord + require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec)) + rec.IngressRules = append(rec.IngressRules, rules...) + data, err := json.Marshal(rec) + require.NoError(t, err) + _, err = svc.sgKV.Put(t.Context(), utils.AccountKey(testAccountID, sgID), data) + require.NoError(t, err) +} + +// An omitted protocol must not be read as all protocols, which would open +// every port to the CIDR. +func TestAuthorizeSecurityGroup_RequiresProtocol(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "no-proto-sg") + before := storedSGRecord(t, svc, sgID) + const msg = "Invalid value 'null' for protocol. VPC security group rules must specify protocols explicitly." + + for _, proto := range []*string{nil, aws.String(""), aws.String(" ")} { + perm := &ec2.IpPermission{ + IpProtocol: proto, FromPort: aws.Int64(80), ToPort: aws.Int64(80), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + } + requireAWSError(t, authorizeIngress(svc, sgID, perm), awserrors.ErrorInvalidParameterValue, msg) + + _, err := svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), IpPermissions: []*ec2.IpPermission{perm}, + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, msg) + } + assert.Equal(t, before, storedSGRecord(t, svc, sgID), "a rejected authorize must store nothing") +} + +// A revoke without a protocol must not match the all-traffic rule. +func TestRevokeSecurityGroupEgress_RequiresProtocol(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "revoke-no-proto-sg") + before := storedSGRecord(t, svc, sgID) + + _, err := svc.RevokeSecurityGroupEgress(context.Background(), &ec2.RevokeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("0.0.0.0/0")}}, + }}, + }, testAccountID) + requireAWSCode(t, err, awserrors.ErrorInvalidParameterValue) + assert.Equal(t, before, storedSGRecord(t, svc, sgID), "the default egress rule must survive") +} + +func TestAuthorizeSecurityGroupIngress_TCPUDPRequireBothPorts(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "no-ports-sg") + before := storedSGRecord(t, svc, sgID) + + for _, proto := range []string{"tcp", "6", "udp", "17"} { + for _, ports := range [][2]*int64{{nil, nil}, {aws.Int64(80), nil}, {nil, aws.Int64(80)}} { + err := authorizeIngress(svc, sgID, &ec2.IpPermission{ + IpProtocol: aws.String(proto), FromPort: ports[0], ToPort: ports[1], + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }) + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, + "Invalid value for portRange. Must specify both from and to ports with TCP/UDP.") + } + } + assert.Equal(t, before, storedSGRecord(t, svc, sgID), "a rejected authorize must store nothing") +} + +// An all-protocol rule is one rule whatever ports it was sent with, so a +// second authorize differing only in ports is a duplicate. +func TestAuthorizeSecurityGroupIngress_AllProtocolPortsShareIdentity(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "all-proto-identity-sg") + + out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("-1"), FromPort: aws.Int64(-1), ToPort: aws.Int64(-1), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.SecurityGroupRules, 1) + stored := storedSGRule(t, svc, sgID, aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId)) + assert.Equal(t, [2]int64{0, 0}, [2]int64{stored.FromPort, stored.ToPort}, "stored in the default egress rule's form") + + err = authorizeIngress(svc, sgID, &ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }) + requireAWSCode(t, err, awserrors.ErrorInvalidPermissionDuplicate) +} + +// A rule stored with -1/-1 before authorize canonicalised the ports is the +// same rule as its 0/0 form for duplicate checks, revoke and reporting. +func TestAllProtocolRuleStoredWithMinusOnePorts(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "legacy-all-proto-sg") + plantIngressRules(t, svc, sgID, + SGRule{RuleId: "sgr-0000000000000000a", IpProtocol: "-1", FromPort: -1, ToPort: -1, CidrIp: "10.0.0.0/24"}, + SGRule{RuleId: "sgr-0000000000000000b", IpProtocol: "-1", FromPort: 0, ToPort: 0, CidrIp: "10.0.1.0/24"}, + ) + + err := authorizeIngress(svc, sgID, &ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }) + requireAWSCode(t, err, awserrors.ErrorInvalidPermissionDuplicate) + + desc, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{ + GroupIds: []*string{aws.String(sgID)}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.SecurityGroups, 1) + require.Len(t, desc.SecurityGroups[0].IpPermissions, 1, "both rules are one all-protocol permission") + assert.Len(t, desc.SecurityGroups[0].IpPermissions[0].IpRanges, 2) + + _, err = svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }}, + }, testAccountID) + require.NoError(t, err) + var rec SecurityGroupRecord + require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec)) + require.Len(t, rec.IngressRules, 1) + assert.Equal(t, "sgr-0000000000000000b", rec.IngressRules[0].RuleId) +} + +// AWS calls a rule ID malformed only without the lowercase sgr- prefix or with +// more than 17 characters after it; anything else that matches no rule is +// not found. Every operation taking a rule ID must agree. +func TestSecurityGroupRuleIDClassification(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "rule-id-class-sg") + + ops := map[string]func(id string) error{ + "DescribeSecurityGroupRules": func(id string) error { + _, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + SecurityGroupRuleIds: []*string{aws.String(id)}, + }, testAccountID) + return err + }, + "RevokeSecurityGroupIngress": func(id string) error { + _, err := svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), SecurityGroupRuleIds: []*string{aws.String(id)}, + }, testAccountID) + return err + }, + "RevokeSecurityGroupEgress": func(id string) error { + _, err := svc.RevokeSecurityGroupEgress(context.Background(), &ec2.RevokeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), SecurityGroupRuleIds: []*string{aws.String(id)}, + }, testAccountID) + return err + }, + "UpdateSecurityGroupRuleDescriptionsIngress": func(id string) error { + return updateIngressDescription(svc, sgID, id, "x") + }, + "UpdateSecurityGroupRuleDescriptionsEgress": func(id string) error { + _, err := svc.UpdateSecurityGroupRuleDescriptionsEgress(context.Background(), &ec2.UpdateSecurityGroupRuleDescriptionsEgressInput{ + GroupId: aws.String(sgID), + SecurityGroupRuleDescriptions: []*ec2.SecurityGroupRuleDescription{ + {SecurityGroupRuleId: aws.String(id), Description: aws.String("x")}, + }, + }, testAccountID) + return err + }, + } + ids := map[string]string{ + "sgr-xyz": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "sgr-0123-4567": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "sgr-": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "sgr-00000000000000000": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "SGR-00000000000000000": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + "sgr-000000000000000000": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + "not-a-rule-id": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + } + for name, op := range ops { + for id, code := range ids { + t.Run(name+"/"+id, func(t *testing.T) { + t.Parallel() + requireAWSCode(t, op(id), code) + }) + } + } +} diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index cb6da1f5e..5e2da0ca2 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -1098,7 +1098,6 @@ func TestValidateSGRule_Protocol(t *testing.T) { func TestNormalizeIPProtocol(t *testing.T) { t.Parallel() cases := map[string]string{ - "": "-1", "-1": "-1", "tcp": "tcp", "TCP": "tcp", @@ -1114,8 +1113,9 @@ func TestNormalizeIPProtocol(t *testing.T) { 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"} { + // 58/icmpv6 is rejected with the rest: the ACL builder is IPv4-only. An + // empty protocol is rejected rather than read as all protocols. + for _, in := range []string{"", "58", "icmpv6", "47", "50", "0", "256", "banana", "tcp; drop"} { _, err := normalizeIPProtocol(in) assert.Error(t, err, "protocol %q", in) }