diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index d7fb4b10b..45b2ab7c7 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -59,6 +59,16 @@ func normalizeIPProtocol(proto string) (string, error) { return "", fmt.Errorf("invalid IpProtocol %q: supported values are tcp, udp, icmp, -1 (or 6, 17, 1)", proto) } +// validateSGRulePorts rejects a tcp or udp rule whose FromPort exceeds its ToPort. +// Only create paths call it, so a rule stored before the check can still be +// revoked or re-described by its ports. +func validateSGRulePorts(proto string, fromPort, toPort int64) error { + if (proto == "tcp" || proto == "udp") && fromPort > toPort { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "Invalid TCP/UDP port range(%d:%d)", fromPort, toPort) + } + return nil +} + // 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 @@ -995,6 +1005,9 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu newRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseAuthorize) if err != nil { slog.WarnContext(ctx, "AuthorizeSecurityGroupIngress: invalid rule", "groupId", groupId, "err", err) + if _, ok := awserrors.ResolveErrorCode(err); ok { + return nil, err + } return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) } newRules = applySGRuleTags(newRules, input.TagSpecifications) @@ -1075,6 +1088,9 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input newRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseAuthorize) if err != nil { slog.WarnContext(ctx, "AuthorizeSecurityGroupEgress: invalid rule", "groupId", groupId, "err", err) + if _, ok := awserrors.ResolveErrorCode(err); ok { + return nil, err + } return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) } newRules = applySGRuleTags(newRules, input.TagSpecifications) @@ -1695,6 +1711,9 @@ func sgRuleRequestToSGRule(req *ec2.SecurityGroupRuleRequest) (SGRule, error) { "Invalid value for portRange. Must specify both from and to ports with TCP/UDP.") } r.FromPort, r.ToPort = *req.FromPort, *req.ToPort + if err := validateSGRulePorts(proto, r.FromPort, r.ToPort); err != nil { + return SGRule{}, err + } case allProtocols: // Stored as 0/0, the form authorize stores for the no-ports case, so the // rule's sgRuleKey matches Terraform's and the default egress rule's. @@ -1825,6 +1844,11 @@ func ipPermissionsToSGRules(perms []*ec2.IpPermission, mode sgParseMode) ([]SGRu if perm.ToPort != nil { toPort = *perm.ToPort } + if mode == sgParseAuthorize { + if err := validateSGRulePorts(proto, fromPort, toPort); err != nil { + return nil, err + } + } appended := false for _, ipRange := range perm.IpRanges { diff --git a/spinifex/handlers/ec2/vpc/security_group_modify_test.go b/spinifex/handlers/ec2/vpc/security_group_modify_test.go index b04bcb223..47648f9da 100644 --- a/spinifex/handlers/ec2/vpc/security_group_modify_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_modify_test.go @@ -7,6 +7,7 @@ package handlers_ec2_vpc import ( "context" "encoding/json" + "fmt" "sync" "testing" @@ -631,3 +632,85 @@ func TestModifySecurityGroupRules_ConcurrentAuthorize(t *testing.T) { assert.Equal(t, modErr == nil, modified, "the modify succeeded iff its effect is stored") assert.Equal(t, authErr == nil, authorized, "the authorize succeeded iff its effect is stored") } + +func requireInvertedPortRange(t *testing.T, err error, from, to int64) { + t.Helper() + require.Error(t, err) + code, message, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok, "error %q carries no AWS code", err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Equal(t, fmt.Sprintf("Invalid TCP/UDP port range(%d:%d)", from, to), message) +} + +func TestAuthorizeSecurityGroupRules_RejectsInvertedPortRange(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "inverted-ports") + before := storedSGRecord(t, svc, sgID) + + perm := func(proto string) []*ec2.IpPermission { + return []*ec2.IpPermission{{ + IpProtocol: aws.String(proto), FromPort: aws.Int64(100), ToPort: aws.Int64(50), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }} + } + _, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), IpPermissions: perm("tcp"), + }, testAccountID) + requireInvertedPortRange(t, err, 100, 50) + + _, err = svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), IpPermissions: perm("udp"), + }, testAccountID) + requireInvertedPortRange(t, err, 100, 50) + + assert.Equal(t, before, storedSGRecord(t, svc, sgID), "a rejected authorize may store nothing") +} + +func TestModifySecurityGroupRules_RejectsInvertedPortRange(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-inverted-ports") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + req := tcpCIDRRequest(100, "10.0.0.0/24") + req.ToPort = aws.Int64(50) + requireInvertedPortRange(t, modifyRules(svc, sgID, ruleUpdate(ruleID, req)), 100, 50) + assert.Equal(t, int64(80), storedSGRule(t, svc, sgID, ruleID).FromPort) +} + +// TestRevokeSecurityGroupIngress_InvertedPortRangeStillRevocable covers a rule +// stored before the range check existed: revoking it by its ports must work. +func TestRevokeSecurityGroupIngress_InvertedPortRangeStillRevocable(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "stale-inverted-ports") + + key := utils.AccountKey(testAccountID, sgID) + var rec SecurityGroupRecord + require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec)) + rec.IngressRules = append(rec.IngressRules, SGRule{ + RuleId: "sgr-0123456789abcdef0", IpProtocol: "tcp", FromPort: 100, ToPort: 50, CidrIp: "10.0.0.0/24", + }) + data, err := json.Marshal(rec) + require.NoError(t, err) + _, err = svc.sgKV.Put(t.Context(), key, data) + require.NoError(t, err) + + _, err = svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(100), ToPort: aws.Int64(50), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }}, + }, testAccountID) + require.NoError(t, err) + + require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec)) + for _, r := range rec.IngressRules { + assert.NotEqual(t, "sgr-0123456789abcdef0", r.RuleId) + } +}