From 372991c304df17889db589e04f99198feaf29337 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 24 Sep 2026 15:20:57 +1000 Subject: [PATCH] feat: implement EC2 ModifySecurityGroupRules (#1103) --- docs/coverage/README.md | 6 +- docs/coverage/coverage.json | 3 +- docs/coverage/ec2/README.md | 3 +- docs/coverage/internal-report.md | 3 +- spinifex/daemon/daemon.go | 1 + spinifex/gateway/ec2.go | 3 + spinifex/gateway/ec2/authz.go | 1 + spinifex/gateway/ec2/authz_test.go | 2 + .../ec2/vpc/ModifySecurityGroupRules.go | 35 + .../ec2/vpc/ModifySecurityGroupRules_test.go | 67 ++ spinifex/gateway/ec2_authz_test.go | 16 + spinifex/handlers/ec2/vpc/security_group.go | 268 ++++++++ .../ec2/vpc/security_group_modify_test.go | 633 ++++++++++++++++++ spinifex/handlers/ec2/vpc/service.go | 1 + spinifex/handlers/ec2/vpc/service_nats.go | 4 + .../subscribers/security_description_test.go | 40 ++ tests/e2e/single/sgreachability_test.go | 82 +++ 17 files changed, 1161 insertions(+), 7 deletions(-) create mode 100644 spinifex/gateway/ec2/vpc/ModifySecurityGroupRules.go create mode 100644 spinifex/gateway/ec2/vpc/ModifySecurityGroupRules_test.go create mode 100644 spinifex/handlers/ec2/vpc/security_group_modify_test.go diff --git a/docs/coverage/README.md b/docs/coverage/README.md index 024218c8e..04828d905 100644 --- a/docs/coverage/README.md +++ b/docs/coverage/README.md @@ -17,12 +17,12 @@ tags: ## Overview -The platform serves **390 operations** across the AWS APIs below. Every page names the operations its service implements from the pinned model, generated from the dispatch tables on each build rather than written by hand. +The platform serves **391 operations** across the AWS APIs below. Every page names the operations its service implements from the pinned model, generated from the dispatch tables on each build rather than written by hand. | Service | Operations | |---|---:| | [ACM](/coverage/acm) | 9 | -| [EC2](/coverage/ec2) | 125 | +| [EC2](/coverage/ec2) | 126 | | [ECR](/coverage/ecr) | 22 | | [ECS](/coverage/ecs) | 31 | | [EKS](/coverage/eks) | 36 | @@ -31,4 +31,4 @@ The platform serves **390 operations** across the AWS APIs below. Every page nam | [RDS](/coverage/rds) | 26 | | [S3](/coverage/s3) | 26 | | [STS](/coverage/sts) | 5 | -| **Total** | **390** | +| **Total** | **391** | diff --git a/docs/coverage/coverage.json b/docs/coverage/coverage.json index cc184516a..adc187971 100644 --- a/docs/coverage/coverage.json +++ b/docs/coverage/coverage.json @@ -800,6 +800,7 @@ "ModifyInstanceMetadataOptions", "ModifyLaunchTemplate", "ModifyNetworkInterfaceAttribute", + "ModifySecurityGroupRules", "ModifySubnetAttribute", "ModifyVolume", "ModifyVpcAttribute", @@ -927,6 +928,7 @@ "ModifyInstanceMetadataOptions", "ModifyLaunchTemplate", "ModifyNetworkInterfaceAttribute", + "ModifySecurityGroupRules", "ModifySubnetAttribute", "ModifyVolume", "ModifyVpcAttribute", @@ -1371,7 +1373,6 @@ "ModifyManagedPrefixList", "ModifyPrivateDnsNameOptions", "ModifyReservedInstances", - "ModifySecurityGroupRules", "ModifySnapshotAttribute", "ModifySnapshotTier", "ModifySpotFleetRequest", diff --git a/docs/coverage/ec2/README.md b/docs/coverage/ec2/README.md index 6c85968ba..8052afb82 100644 --- a/docs/coverage/ec2/README.md +++ b/docs/coverage/ec2/README.md @@ -18,7 +18,7 @@ tags: ## Overview -Spinifex implements **125 operations** in the EC2 `2016-11-15` API model. +Spinifex implements **126 operations** in the EC2 `2016-11-15` API model. ### Spot Instances Are a Mock @@ -132,6 +132,7 @@ Spot Instance Requests are a mock over the on-demand `RunInstances` path. A requ | `ModifyInstanceMetadataOptions` | | `ModifyLaunchTemplate` | | `ModifyNetworkInterfaceAttribute` | +| `ModifySecurityGroupRules` | | `ModifySubnetAttribute` | | `ModifyVolume` | | `ModifyVpcAttribute` | diff --git a/docs/coverage/internal-report.md b/docs/coverage/internal-report.md index a884a6111..43b208e2a 100644 --- a/docs/coverage/internal-report.md +++ b/docs/coverage/internal-report.md @@ -21,7 +21,7 @@ Pinned model source: `aws-sdk-go v1.55.8`. ## ec2 -`2016-11-15` — 125 of 625 modelled operations implemented (20.0%), 0 stubbed, 39 not applicable, 0 registered outside the pinned model. +`2016-11-15` — 126 of 625 modelled operations implemented (20.2%), 0 stubbed, 39 not applicable, 0 registered outside the pinned model. ### Not implemented @@ -409,7 +409,6 @@ Pinned model source: `aws-sdk-go v1.55.8`. - `ModifyManagedPrefixList` - `ModifyPrivateDnsNameOptions` - `ModifyReservedInstances` -- `ModifySecurityGroupRules` - `ModifySnapshotAttribute` - `ModifySnapshotTier` - `ModifySpotFleetRequest` diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 1c523324c..5258ccfd0 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -979,6 +979,7 @@ func (d *Daemon) subscribeAll() error { {"ec2.RevokeSecurityGroupEgress", handleNATSRequest(d.node, d.vpcService.RevokeSecurityGroupEgress), "spinifex-workers"}, {"ec2.UpdateSecurityGroupRuleDescriptionsIngress", handleNATSRequest(d.node, d.vpcService.UpdateSecurityGroupRuleDescriptionsIngress), "spinifex-workers"}, {"ec2.UpdateSecurityGroupRuleDescriptionsEgress", handleNATSRequest(d.node, d.vpcService.UpdateSecurityGroupRuleDescriptionsEgress), "spinifex-workers"}, + {"ec2.ModifySecurityGroupRules", handleNATSRequest(d.node, d.vpcService.ModifySecurityGroupRules), "spinifex-workers"}, {"ec2.ModifyInstanceAttribute", handleNATSRequest(d.node, d.instanceService.ModifyInstanceAttribute), "spinifex-workers"}, {"ec2.ModifyInstanceMetadataOptions", handleNATSRequest(d.node, d.instanceService.ModifyInstanceMetadataOptions), "spinifex-workers"}, {"ec2.MonitorInstances", handleNATSRequest(d.node, d.monitorInstances), "spinifex-workers"}, diff --git a/spinifex/gateway/ec2.go b/spinifex/gateway/ec2.go index 651f62789..dab658f3d 100644 --- a/spinifex/gateway/ec2.go +++ b/spinifex/gateway/ec2.go @@ -575,6 +575,9 @@ var ec2Actions = map[string]ec2Action{ "UpdateSecurityGroupRuleDescriptionsEgress": ec2Handler(func(ctx context.Context, input *ec2.UpdateSecurityGroupRuleDescriptionsEgressInput, gw *GatewayConfig, accountID string) (any, error) { return gateway_ec2_vpc.UpdateSecurityGroupRuleDescriptionsEgress(ctx, input, gw.NATSConn, accountID) }), + "ModifySecurityGroupRules": ec2Handler(func(ctx context.Context, input *ec2.ModifySecurityGroupRulesInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_vpc.ModifySecurityGroupRules(ctx, input, gw.NATSConn, accountID) + }), "AllocateAddress": ec2Handler(func(ctx context.Context, input *ec2.AllocateAddressInput, gw *GatewayConfig, accountID string) (any, error) { if err := gw.Quota.EnforceEIPs(ctx, gw.NATSConn, accountID, 1); err != nil { return nil, err diff --git a/spinifex/gateway/ec2/authz.go b/spinifex/gateway/ec2/authz.go index 4f02e5654..060c27aa0 100644 --- a/spinifex/gateway/ec2/authz.go +++ b/spinifex/gateway/ec2/authz.go @@ -216,6 +216,7 @@ var ec2Scopes = map[string][]*resourceScope{ "UpdateSecurityGroupRuleDescriptionsIngress": {securityGroupScope}, "UpdateSecurityGroupRuleDescriptionsEgress": {securityGroupScope}, + "ModifySecurityGroupRules": {securityGroupScope}, // Unlike the account-wide describes, this one names the VPC it reports on, // so a policy scoped to one VPC has to apply. diff --git a/spinifex/gateway/ec2/authz_test.go b/spinifex/gateway/ec2/authz_test.go index 7f4dea7b5..bbadb0c79 100644 --- a/spinifex/gateway/ec2/authz_test.go +++ b/spinifex/gateway/ec2/authz_test.go @@ -182,6 +182,7 @@ func TestResourceARNs(t *testing.T) { []string{ec2arn("security-group", "sg-1")}}, {"UpdateSecurityGroupRuleDescriptionsEgress", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, + {"ModifySecurityGroupRules", map[string]string{"GroupId": "sg-1"}, []string{ec2arn("security-group", "sg-1")}}, // Addresses. {"AllocateAddress", map[string]string{"Domain": "vpc"}, []string{ec2arn("elastic-ip", "*")}}, @@ -392,6 +393,7 @@ var ec2Inputs = map[string]any{ "ReplaceIamInstanceProfileAssociation": &ec2.ReplaceIamInstanceProfileAssociationInput{}, "ReplaceRoute": &ec2.ReplaceRouteInput{}, "ReplaceRouteTableAssociation": &ec2.ReplaceRouteTableAssociationInput{}, + "ModifySecurityGroupRules": &ec2.ModifySecurityGroupRulesInput{}, "RequestSpotInstances": &ec2.RequestSpotInstancesInput{}, "ResetImageAttribute": &ec2.ResetImageAttributeInput{}, "RevokeSecurityGroupEgress": &ec2.RevokeSecurityGroupEgressInput{}, diff --git a/spinifex/gateway/ec2/vpc/ModifySecurityGroupRules.go b/spinifex/gateway/ec2/vpc/ModifySecurityGroupRules.go new file mode 100644 index 000000000..391a974c5 --- /dev/null +++ b/spinifex/gateway/ec2/vpc/ModifySecurityGroupRules.go @@ -0,0 +1,35 @@ +package gateway_ec2_vpc + +import ( + "context" + "errors" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + "github.com/nats-io/nats.go" +) + +func ModifySecurityGroupRules(ctx context.Context, input *ec2.ModifySecurityGroupRulesInput, natsConn *nats.Conn, accountID string) (ec2.ModifySecurityGroupRulesOutput, error) { + var output ec2.ModifySecurityGroupRulesOutput + if input == nil { + return output, errors.New(awserrors.ErrorInvalidParameterValue) + } + if aws.StringValue(input.GroupId) == "" || len(input.SecurityGroupRules) == 0 { + return output, errors.New(awserrors.ErrorMissingParameter) + } + // Only the malformed half is decidable here; whether a well-formed ID + // exists is the handler's to answer. + for _, u := range input.SecurityGroupRules { + if u != nil && u.SecurityGroupRuleId != nil && handlers_ec2_vpc.SGRuleIDIsMalformed(*u.SecurityGroupRuleId) { + return output, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) + } + } + svc := handlers_ec2_vpc.NewNATSVPCService(natsConn) + result, err := svc.ModifySecurityGroupRules(ctx, input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/vpc/ModifySecurityGroupRules_test.go b/spinifex/gateway/ec2/vpc/ModifySecurityGroupRules_test.go new file mode 100644 index 000000000..162a232d5 --- /dev/null +++ b/spinifex/gateway/ec2/vpc/ModifySecurityGroupRules_test.go @@ -0,0 +1,67 @@ +package gateway_ec2_vpc_test + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_ec2_vpc "github.com/mulgadc/spinifex/spinifex/gateway/ec2/vpc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func modifyUpdate(ruleID string) *ec2.SecurityGroupRuleUpdate { + return &ec2.SecurityGroupRuleUpdate{ + SecurityGroupRuleId: aws.String(ruleID), + SecurityGroupRule: &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(22), + ToPort: aws.Int64(22), + CidrIpv4: aws.String("10.0.0.0/24"), + }, + } +} + +func TestModifySecurityGroupRules_NilInput(t *testing.T) { + _, err := gateway_ec2_vpc.ModifySecurityGroupRules(context.Background(), nil, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue) +} + +func TestModifySecurityGroupRules_MissingGroupId(t *testing.T) { + _, err := gateway_ec2_vpc.ModifySecurityGroupRules(context.Background(), &ec2.ModifySecurityGroupRulesInput{ + SecurityGroupRules: []*ec2.SecurityGroupRuleUpdate{modifyUpdate("sgr-0123456789abcdef0")}, + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestModifySecurityGroupRules_EmptyUpdateList(t *testing.T) { + _, err := gateway_ec2_vpc.ModifySecurityGroupRules(context.Background(), &ec2.ModifySecurityGroupRulesInput{ + GroupId: aws.String("sg-0123456789abcdef0"), + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestModifySecurityGroupRules_MalformedRuleID(t *testing.T) { + for _, bad := range []string{"foo", "SGR-0123456789abcdef0", "sgr-0123456789abcdef01", "sg-0123456789abcdef0", ""} { + _, err := gateway_ec2_vpc.ModifySecurityGroupRules(context.Background(), &ec2.ModifySecurityGroupRulesInput{ + GroupId: aws.String("sg-0123456789abcdef0"), + SecurityGroupRules: []*ec2.SecurityGroupRuleUpdate{modifyUpdate(bad)}, + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, "expected malformed for %q", bad) + } +} + +// A short or non-hex suffix is NotFound on AWS, so it must reach the handler +// rather than be rejected here as malformed. +func TestModifySecurityGroupRules_WellFormedRuleIDPassesValidation(t *testing.T) { + for _, id := range []string{"sgr-0123456789abcdef0", "sgr-xyz", "sgr-"} { + _, err := gateway_ec2_vpc.ModifySecurityGroupRules(context.Background(), &ec2.ModifySecurityGroupRulesInput{ + GroupId: aws.String("sg-0123456789abcdef0"), + SecurityGroupRules: []*ec2.SecurityGroupRuleUpdate{modifyUpdate(id)}, + }, nil, testAccountID) + require.Error(t, err) + assert.NotEqual(t, awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, err.Error(), "id %q", id) + } +} diff --git a/spinifex/gateway/ec2_authz_test.go b/spinifex/gateway/ec2_authz_test.go index 0b437df05..904329b74 100644 --- a/spinifex/gateway/ec2_authz_test.go +++ b/spinifex/gateway/ec2_authz_test.go @@ -205,6 +205,22 @@ func TestEC2Request_UpdateSecurityGroupRuleDescriptionsIsGroupScoped(t *testing. } } +// TestEC2Request_ModifySecurityGroupRulesIsGroupScoped pins that the modify +// names the group it mutates, so a policy fencing one security group applies. +func TestEC2Request_ModifySecurityGroupRulesIsGroupScoped(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "ec2:*", "*"), + statement("Deny", "ec2:ModifySecurityGroupRules", "arn:aws:ec2:*:*:security-group/sg-prod"), + ) + + const update = "&SecurityGroupRule.1.SecurityGroupRuleId=sgr-0123456789abcdef0" + + "&SecurityGroupRule.1.SecurityGroupRule.IpProtocol=tcp" + + "&SecurityGroupRule.1.SecurityGroupRule.FromPort=22&SecurityGroupRule.1.SecurityGroupRule.ToPort=22" + + "&SecurityGroupRule.1.SecurityGroupRule.CidrIpv4=10.0.0.0/24" + assertDenied(t, dispatchEC2(t, gw, "Action=ModifySecurityGroupRules&GroupId=sg-prod"+update)) + assertPermitted(t, dispatchEC2(t, gw, "Action=ModifySecurityGroupRules&GroupId=sg-dev"+update)) +} + func TestEC2Request_RejectsOversizedResourceList(t *testing.T) { var body strings.Builder body.WriteString("Action=TerminateInstances") diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 7ab11d8a4..4f0f60c83 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -1517,6 +1517,274 @@ func ipPermissionsToDescriptionTargets(perms []*ec2.IpPermission) ([]sgDescripti return targets, nil } +// ModifySecurityGroupRules replaces the body of stored rules in place, keeping +// each rule's SecurityGroupRuleId and tags. Every update is validated before +// anything is written, so one bad update leaves the whole group untouched. +func (s *VPCServiceImpl) ModifySecurityGroupRules(ctx context.Context, input *ec2.ModifySecurityGroupRulesInput, accountID string) (*ec2.ModifySecurityGroupRulesOutput, error) { + if input == nil || aws.StringValue(input.GroupId) == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + if len(input.SecurityGroupRules) == 0 { + return nil, awserrors.Errorf(awserrors.ErrorMissingParameter, "The request must contain the parameter securityGroupRules") + } + + groupId := *input.GroupId + key := utils.AccountKey(accountID, groupId) + + entry, err := s.sgKV.Get(ctx, key) + if err != nil { + return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + } + + var record SecurityGroupRecord + if err := json.Unmarshal(entry.Value(), &record); err != nil { + return nil, errors.New(awserrors.ErrorServerInternal) + } + + if err := s.requireVPCExists(ctx, accountID, record.VpcId); err != nil { + return nil, err + } + + modified, err := applySGRuleModifications(&record, input.SecurityGroupRules) + if err != nil { + slog.WarnContext(ctx, "ModifySecurityGroupRules: invalid update", "groupId", groupId, "err", err) + return nil, err + } + if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, modified); err != nil { + return nil, err + } + + data, err := json.Marshal(record) + if err != nil { + return nil, fmt.Errorf("failed to marshal security group record: %w", err) + } + if _, err := s.sgKV.Update(ctx, key, data, entry.Revision()); err != nil { + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.InfoContext(ctx, "ModifySecurityGroupRules completed", "groupId", groupId, "modifiedRules", len(modified), "accountID", accountID) + + if err := s.requestSGEvent("vpc.update-sg", SGEvent{ + GroupId: groupId, + VpcId: record.VpcId, + IngressRules: record.IngressRules, + EgressRules: record.EgressRules, + }); err != nil { + slog.ErrorContext(ctx, "ModifySecurityGroupRules: vpcd request failed", "groupId", groupId, "err", err) + return nil, err + } + + return &ec2.ModifySecurityGroupRulesOutput{Return: aws.Bool(true)}, nil +} + +// sgRuleModification is one resolved update: the side and index of the stored +// rule it replaces, and the rule that replaces it. +type sgRuleModification struct { + egress bool + index int + rule SGRule +} + +// applySGRuleModifications resolves and validates every update, then writes +// the new bodies into record. It returns the modified rules, and on error +// leaves record untouched. +func applySGRuleModifications(record *SecurityGroupRecord, updates []*ec2.SecurityGroupRuleUpdate) ([]SGRule, error) { + seen := make(map[string]bool, len(updates)) + mods := make([]sgRuleModification, 0, len(updates)) + for _, u := range updates { + if u == nil || u.SecurityGroupRuleId == nil { + return nil, awserrors.Errorf(awserrors.ErrorMissingParameter, "The request must contain the parameter securityGroupRuleId") + } + id := *u.SecurityGroupRuleId + if SGRuleIDIsMalformed(id) { + return nil, sgRuleIDLookupError(id) + } + if seen[id] { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Duplicated security group rule ID '%s'. The same security group rule ID must not appear multiple times.", id) + } + seen[id] = true + + mod := sgRuleModification{index: slices.IndexFunc(record.IngressRules, func(r SGRule) bool { return r.RuleId == id })} + stored := record.IngressRules + if mod.index < 0 { + mod.egress = true + mod.index = slices.IndexFunc(record.EgressRules, func(r SGRule) bool { return r.RuleId == id }) + stored = record.EgressRules + } + if mod.index < 0 { + return nil, sgRuleIDLookupError(id) + } + current := stored[mod.index] + + rule, err := sgRuleRequestToSGRule(u.SecurityGroupRule) + if err != nil { + return nil, err + } + if want, have := sgRuleSourceField(rule), sgRuleSourceField(current); want != have { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Invalid rule type for security group rule '%s'. You may not specify %s for an existing %s rule.", id, want, have) + } + rule.RuleId = current.RuleId + rule.Tags = current.Tags + mod.rule = rule + mods = append(mods, mod) + } + + for _, egress := range []bool{false, true} { + if err := checkSGRuleModificationDuplicates(record, mods, egress); err != nil { + return nil, err + } + } + + ingress, egressRules := slices.Clone(record.IngressRules), slices.Clone(record.EgressRules) + modified := make([]SGRule, 0, len(mods)) + for _, m := range mods { + if m.egress { + egressRules[m.index] = m.rule + } else { + ingress[m.index] = m.rule + } + modified = append(modified, m.rule) + } + record.IngressRules, record.EgressRules = ingress, egressRules + return modified, nil +} + +// checkSGRuleModificationDuplicates rejects new contents that collide with each +// other or with a rule on the same side the request leaves alone. Excluding +// the rules being modified lets a rule keep its own content and two rules swap. +func checkSGRuleModificationDuplicates(record *SecurityGroupRecord, mods []sgRuleModification, egress bool) error { + rules := record.IngressRules + if egress { + rules = record.EgressRules + } + + replaced := make(map[int]bool) + newKeys := make(map[string]bool) + for _, m := range mods { + if m.egress != egress { + continue + } + replaced[m.index] = true + k := sgRuleKey(m.rule) + if newKeys[k] { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "The same permission must not appear multiple times") + } + newKeys[k] = true + } + + for i, r := range rules { + if !replaced[i] && newKeys[sgRuleKey(r)] { + return errors.New(awserrors.ErrorInvalidPermissionDuplicate) + } + } + return nil +} + +// sgRuleRequestToSGRule parses a SecurityGroupRuleRequest into a full rule. +// It is stricter than the authorize path: the protocol is required and tcp/udp +// need both ports, as AWS requires for a modify. +func sgRuleRequestToSGRule(req *ec2.SecurityGroupRuleRequest) (SGRule, error) { + if req == nil { + 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) + if err != nil { + return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", 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.") + } + r.FromPort, r.ToPort = *req.FromPort, *req.ToPort + 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. + noPorts := req.FromPort == nil && req.ToPort == nil + allPorts := req.FromPort != nil && req.ToPort != nil && *req.FromPort == -1 && *req.ToPort == -1 + if !noPorts && !allPorts { + return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "You may not specify all protocols and specific ports. Please specify each protocol and port range combination individually, or all protocols and no port range.") + } + default: + r.FromPort, r.ToPort = aws.Int64Value(req.FromPort), aws.Int64Value(req.ToPort) + } + + sources := 0 + for _, src := range []*string{req.CidrIpv4, req.CidrIpv6, req.ReferencedGroupId, req.PrefixListId} { + if aws.StringValue(src) != "" { + sources++ + } + } + switch { + case sources == 0: + return SGRule{}, awserrors.Errorf(awserrors.ErrorMissingParameter, + "The request must contain exactly one of: cidrIp, cidrIpv6, prefixListId, or referencedGroupId") + case sources > 1: + return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "Only one of cidrIp, cidrIpv6, prefixListId, or referencedGroupId can be specified") + case aws.StringValue(req.PrefixListId) != "": + return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidPrefixListIDNotFound, + "The prefix list ID '%s' does not exist", *req.PrefixListId) + } + + r.CidrIp = aws.StringValue(req.CidrIpv4) + r.CidrIpv6 = aws.StringValue(req.CidrIpv6) + r.SourceSG = aws.StringValue(req.ReferencedGroupId) + if r.SourceSG != "" && !sgIDRegex.MatchString(r.SourceSG) { + return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidGroupIdMalformed, "Invalid id: %q", r.SourceSG) + } + if err := validateSGRule(r); err != nil { + return SGRule{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) + } + return r, nil +} + +// sgRuleSourceField names a rule's source type by its API field, since a +// modify may not move a rule from one source type to another. +func sgRuleSourceField(r SGRule) string { + switch { + case r.CidrIpv6 != "": + return "CidrIpv6" + case r.SourceSG != "": + return "ReferencedGroupId" + default: + return "CidrIpv4" + } +} + +// sgRuleIDMaxSuffix is the longest suffix AWS accepts after "sgr-" before it +// calls a rule ID malformed rather than unknown. +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. +func SGRuleIDIsMalformed(id string) bool { + suffix, ok := strings.CutPrefix(id, "sgr-") + return !ok || len(suffix) > sgRuleIDMaxSuffix +} + +// sgRuleIDLookupError is the error AWS returns for a rule ID that resolves to +// no stored rule: Malformed or NotFound by SGRuleIDIsMalformed. +func sgRuleIDLookupError(id string) error { + if SGRuleIDIsMalformed(id) { + return awserrors.Errorf(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, "Invalid id: %q", id) + } + return awserrors.Errorf(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "The security group rule ID '%s' does not exist", id) +} + // sgRecordToEC2 converts a SecurityGroupRecord to an EC2 SecurityGroup. func (s *VPCServiceImpl) sgRecordToEC2(record *SecurityGroupRecord, accountID string) *ec2.SecurityGroup { sg := &ec2.SecurityGroup{ diff --git a/spinifex/handlers/ec2/vpc/security_group_modify_test.go b/spinifex/handlers/ec2/vpc/security_group_modify_test.go new file mode 100644 index 000000000..8d6d454b6 --- /dev/null +++ b/spinifex/handlers/ec2/vpc/security_group_modify_test.go @@ -0,0 +1,633 @@ +//test:in-package — builds on the package's unexported SG fixtures +//(setupTestVPCService, createTestSG, authorizeIngressTCP, failingUpdateResponder) +//and asserts on the stored record, which no exported API returns verbatim. + +package handlers_ec2_vpc + +import ( + "context" + "encoding/json" + "sync" + "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/filterutil" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func requireAWSCode(t *testing.T, err error, code string) { + t.Helper() + require.Error(t, err) + got, ok := awserrors.ResolveErrorCode(err) + require.True(t, ok, "error %q carries no AWS code", err) + assert.Equal(t, code, got, "error: %v", err) +} + +func modifyRules(svc *VPCServiceImpl, sgID string, updates ...*ec2.SecurityGroupRuleUpdate) error { + _, err := svc.ModifySecurityGroupRules(context.Background(), &ec2.ModifySecurityGroupRulesInput{ + GroupId: aws.String(sgID), + SecurityGroupRules: updates, + }, testAccountID) + return err +} + +func ruleUpdate(ruleID string, req *ec2.SecurityGroupRuleRequest) *ec2.SecurityGroupRuleUpdate { + return &ec2.SecurityGroupRuleUpdate{SecurityGroupRuleId: aws.String(ruleID), SecurityGroupRule: req} +} + +func tcpCIDRRequest(port int64, cidr string) *ec2.SecurityGroupRuleRequest { + return &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(port), + ToPort: aws.Int64(port), + CidrIpv4: aws.String(cidr), + } +} + +func tcpPeerRequest(port int64, peer string) *ec2.SecurityGroupRuleRequest { + return &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(port), + ToPort: aws.Int64(port), + ReferencedGroupId: aws.String(peer), + } +} + +// storedSGRecord returns the raw stored record, for asserting a rejected +// modify wrote nothing. +func storedSGRecord(t *testing.T, svc *VPCServiceImpl, sgID string) []byte { + t.Helper() + entry, err := svc.sgKV.Get(t.Context(), utils.AccountKey(testAccountID, sgID)) + require.NoError(t, err) + return entry.Value() +} + +func storedSGRule(t *testing.T, svc *VPCServiceImpl, sgID, ruleID string) SGRule { + t.Helper() + var rec SecurityGroupRecord + require.NoError(t, json.Unmarshal(storedSGRecord(t, svc, sgID), &rec)) + for _, r := range append(rec.IngressRules, rec.EgressRules...) { + if r.RuleId == ruleID { + return r + } + } + t.Fatalf("rule %s not stored on %s", ruleID, sgID) + return SGRule{} +} + +func authorizeIngressPeer(t *testing.T, svc *VPCServiceImpl, sgID string, port int64, peer string) string { + t.Helper() + out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(port), + ToPort: aws.Int64(port), + UserIdGroupPairs: []*ec2.UserIdGroupPair{{GroupId: aws.String(peer)}}, + }}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.SecurityGroupRules, 1) + return aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId) +} + +func authorizeIngressIPv6(t *testing.T, svc *VPCServiceImpl, sgID string, port int64, cidr string) string { + t.Helper() + out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(port), + ToPort: aws.Int64(port), + Ipv6Ranges: []*ec2.Ipv6Range{{CidrIpv6: aws.String(cidr)}}, + }}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.SecurityGroupRules, 1) + return aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId) +} + +func TestModifySecurityGroupRules_ChangesContentKeepsID(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-content") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(8080, "10.0.0.0/24")))) + got := sgRuleByID(t, svc, testAccountID, ruleID) + assert.Equal(t, int64(8080), aws.Int64Value(got.FromPort)) + assert.Equal(t, int64(8080), aws.Int64Value(got.ToPort)) + + udp := tcpCIDRRequest(8080, "10.0.0.0/24") + udp.IpProtocol = aws.String("udp") + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, udp))) + assert.Equal(t, "udp", aws.StringValue(sgRuleByID(t, svc, testAccountID, ruleID).IpProtocol)) + + udp.CidrIpv4 = aws.String("192.168.0.0/16") + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, udp))) + got = sgRuleByID(t, svc, testAccountID, ruleID) + assert.Equal(t, "192.168.0.0/16", aws.StringValue(got.CidrIpv4)) + assert.Equal(t, ruleID, aws.StringValue(got.SecurityGroupRuleId)) + + peerA := createTestSG(t, svc, vpcID, "mod-peer-a") + peerB := createTestSG(t, svc, vpcID, "mod-peer-b") + peerRule := authorizeIngressPeer(t, svc, sgID, 5432, peerA) + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(peerRule, tcpPeerRequest(5432, peerB)))) + got = sgRuleByID(t, svc, testAccountID, peerRule) + require.NotNil(t, got.ReferencedGroupInfo) + assert.Equal(t, peerB, aws.StringValue(got.ReferencedGroupInfo.GroupId)) + assert.Equal(t, peerRule, aws.StringValue(got.SecurityGroupRuleId)) +} + +func TestModifySecurityGroupRules_EgressByID(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-egress") + ruleID := defaultEgressRuleID(t, svc, sgID) + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(443, "0.0.0.0/0")))) + + got := sgRuleByID(t, svc, testAccountID, ruleID) + assert.True(t, aws.BoolValue(got.IsEgress)) + assert.Equal(t, "tcp", aws.StringValue(got.IpProtocol)) + assert.Equal(t, int64(443), aws.Int64Value(got.FromPort)) +} + +// TestModifySecurityGroupRules_RevocableByNewContent pins that sgRuleKey is +// recomputed: the rule answers to its new content and no longer to its old. +func TestModifySecurityGroupRules_RevocableByNewContent(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-revoke") + ruleID := authorizeIngressTCP(t, svc, sgID, 8080, "192.0.2.0/24") + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(8081, "198.51.100.0/24")))) + + revoke := func(port int64, cidr string) error { + _, err := svc.RevokeSecurityGroupIngress(context.Background(), &ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(port), + ToPort: aws.Int64(port), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String(cidr)}}, + }}, + }, testAccountID) + return err + } + requireAWSCode(t, revoke(8080, "192.0.2.0/24"), awserrors.ErrorInvalidPermissionNotFound) + require.NoError(t, revoke(8081, "198.51.100.0/24")) +} + +func TestModifySecurityGroupRules_PreservesTags(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + writer := &fakeCentralTagStore{} + svc.SetCentralTagStore(writer) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-tags") + + out, err := svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(80), ToPort: aws.Int64(80), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }}, + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String(ec2.ResourceTypeSecurityGroupRule), + Tags: []*ec2.Tag{{Key: aws.String("Name"), Value: aws.String("web")}}, + }}, + }, testAccountID) + require.NoError(t, err) + ruleID := aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId) + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(81, "10.0.0.0/24")))) + + want := map[string]string{"Name": "web"} + assert.Equal(t, want, filterutil.EC2TagsToMap(sgRuleByID(t, svc, testAccountID, ruleID).Tags)) + assert.Equal(t, want, writer.calls[ruleID], "the central tag store must still hold the rule's tags") + assert.NotContains(t, writer.deleted, ruleID, "a modify must not clear the rule's central tags") +} + +// TestModifySecurityGroupRules_DescriptionIsReplaced pins AWS's whole-rule +// replace: an omitted description clears the stored one, unlike the +// description operations, where omission leaves it untouched. +func TestModifySecurityGroupRules_DescriptionIsReplaced(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-desc") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + require.NoError(t, updateIngressDescription(svc, sgID, ruleID, "old")) + + req := tcpCIDRRequest(80, "10.0.0.0/24") + req.Description = aws.String("new") + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, req))) + assert.Equal(t, "new", aws.StringValue(sgRuleByID(t, svc, testAccountID, ruleID).Description)) + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(80, "10.0.0.0/24")))) + assert.Nil(t, sgRuleByID(t, svc, testAccountID, ruleID).Description, "an omitted description must clear the stored one") +} + +func TestModifySecurityGroupRules_RejectsSourceTypeChange(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-source-type") + peer := createTestSG(t, svc, vpcID, "mod-source-peer") + v4Rule := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + v6Rule := authorizeIngressIPv6(t, svc, sgID, 80, "2001:db8::/32") + peerRule := authorizeIngressPeer(t, svc, sgID, 80, peer) + v6Request := &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(81), ToPort: aws.Int64(81), + CidrIpv6: aws.String("2001:db8::/48"), + } + + for name, u := range map[string]*ec2.SecurityGroupRuleUpdate{ + "ipv4 to ipv6": ruleUpdate(v4Rule, v6Request), + "ipv4 to peer": ruleUpdate(v4Rule, tcpPeerRequest(81, peer)), + "ipv6 to ipv4": ruleUpdate(v6Rule, tcpCIDRRequest(81, "10.1.0.0/24")), + "ipv6 to peer": ruleUpdate(v6Rule, tcpPeerRequest(81, peer)), + "peer to ipv4": ruleUpdate(peerRule, tcpCIDRRequest(81, "10.1.0.0/24")), + "peer to ipv6": ruleUpdate(peerRule, v6Request), + } { + before := storedSGRecord(t, svc, sgID) + err := modifyRules(svc, sgID, u) + requireAWSCode(t, err, awserrors.ErrorInvalidParameterValue) + assert.ErrorContains(t, err, "Invalid rule type", name) + assert.Equal(t, before, storedSGRecord(t, svc, sgID), name) + } +} + +func TestModifySecurityGroupRules_SameSideCollisionIsDuplicate(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-dup") + ruleA := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + authorizeIngressTCP(t, svc, sgID, 443, "10.0.0.0/24") + before := storedSGRecord(t, svc, sgID) + + requireAWSCode(t, modifyRules(svc, sgID, ruleUpdate(ruleA, tcpCIDRRequest(443, "10.0.0.0/24"))), + awserrors.ErrorInvalidPermissionDuplicate) + assert.Equal(t, before, storedSGRecord(t, svc, sgID), "a rejected modify must leave the record byte-identical") +} + +// An all-protocol rule sent with -1/-1 is the same rule as one with no ports, +// so it collides with the default egress rule. +func TestModifySecurityGroupRules_AllProtocolMinusOnePortsCollides(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-dup-all") + _, err := svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(443), ToPort: aws.Int64(443), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/8")}}, + }}, + }, testAccountID) + require.NoError(t, err) + var ruleID string + for _, r := range describeGroupRules(t, svc, sgID) { + if aws.BoolValue(r.IsEgress) && aws.StringValue(r.IpProtocol) == "tcp" { + ruleID = aws.StringValue(r.SecurityGroupRuleId) + } + } + require.NotEmpty(t, ruleID) + + requireAWSCode(t, modifyRules(svc, sgID, ruleUpdate(ruleID, &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("-1"), FromPort: aws.Int64(-1), ToPort: aws.Int64(-1), + CidrIpv4: aws.String("0.0.0.0/0"), + })), awserrors.ErrorInvalidPermissionDuplicate) +} + +func describeGroupRules(t *testing.T, svc *VPCServiceImpl, sgID string) []*ec2.SecurityGroupRule { + t.Helper() + out, err := svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + Filters: []*ec2.Filter{{Name: aws.String("group-id"), Values: []*string{aws.String(sgID)}}}, + }, testAccountID) + require.NoError(t, err) + return out.SecurityGroupRules +} + +func TestModifySecurityGroupRules_OtherSideCollisionSucceeds(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-other-side") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + // Same content as the default egress rule, which lives on the other side. + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("-1"), + CidrIpv4: aws.String("0.0.0.0/0"), + }))) + assert.Equal(t, "-1", aws.StringValue(sgRuleByID(t, svc, testAccountID, ruleID).IpProtocol)) +} + +func TestModifySecurityGroupRules_OwnContentSucceeds(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-own") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(80, "10.0.0.0/24")))) +} + +func TestModifySecurityGroupRules_SwapSucceeds(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-swap") + ruleA := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + ruleB := authorizeIngressTCP(t, svc, sgID, 443, "10.0.0.0/24") + + require.NoError(t, modifyRules(svc, sgID, + ruleUpdate(ruleA, tcpCIDRRequest(443, "10.0.0.0/24")), + ruleUpdate(ruleB, tcpCIDRRequest(80, "10.0.0.0/24")), + )) + assert.Equal(t, int64(443), aws.Int64Value(sgRuleByID(t, svc, testAccountID, ruleA).FromPort)) + assert.Equal(t, int64(80), aws.Int64Value(sgRuleByID(t, svc, testAccountID, ruleB).FromPort)) +} + +func TestModifySecurityGroupRules_TwoUpdatesSameContent(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-same-content") + ruleA := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + ruleB := authorizeIngressTCP(t, svc, sgID, 443, "10.0.0.0/24") + + err := modifyRules(svc, sgID, + ruleUpdate(ruleA, tcpCIDRRequest(8080, "10.0.0.0/24")), + ruleUpdate(ruleB, tcpCIDRRequest(8080, "10.0.0.0/24")), + ) + requireAWSCode(t, err, awserrors.ErrorInvalidParameterValue) + assert.ErrorContains(t, err, "The same permission must not appear multiple times") +} + +func TestModifySecurityGroupRules_OneBadUpdateAppliesNothing(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-atomic") + ruleA := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + ruleB := authorizeIngressTCP(t, svc, sgID, 443, "10.0.0.0/24") + before := storedSGRecord(t, svc, sgID) + + requireAWSCode(t, modifyRules(svc, sgID, + ruleUpdate(ruleA, tcpCIDRRequest(8080, "10.0.0.0/24")), + ruleUpdate(ruleB, tcpCIDRRequest(8443, "10.0.0.1/24")), + ), awserrors.ErrorInvalidParameterValue) + assert.Equal(t, before, storedSGRecord(t, svc, sgID), "no update in a failed request may be applied") +} + +func TestModifySecurityGroupRules_PeerValidation(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + otherVPC := createTestVPC(t, svc, "10.1.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-peer") + peer := createTestSG(t, svc, vpcID, "mod-peer-same") + foreign := createTestSG(t, svc, otherVPC, "mod-peer-foreign") + ruleID := authorizeIngressPeer(t, svc, sgID, 22, peer) + + for peerID, code := range map[string]string{ + foreign: awserrors.ErrorInvalidGroupNotFound, + "sg-00000000000000000": awserrors.ErrorInvalidGroupNotFound, + "sg-xyz": awserrors.ErrorInvalidGroupIdMalformed, + testAccountID + "/" + peer: awserrors.ErrorInvalidGroupIdMalformed, + testAccountID + "/sg-xyz": awserrors.ErrorInvalidGroupIdMalformed, + "sg-0123456789abcdef0123456789abcde": awserrors.ErrorInvalidGroupIdMalformed, + } { + err := modifyRules(svc, sgID, ruleUpdate(ruleID, tcpPeerRequest(22, peerID))) + got, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, code, got, "peer %q: %v", peerID, err) + } + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, tcpPeerRequest(22, sgID))), "a self-reference is allowed") + assert.Equal(t, sgID, aws.StringValue(sgRuleByID(t, svc, testAccountID, ruleID).ReferencedGroupInfo.GroupId)) +} + +func TestSGRuleIDLookupError(t *testing.T) { + t.Parallel() + for id, code := range map[string]string{ + "sgr-xyz": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "sgr-0123-4567": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "sgr-": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "sgr-0123456789abcdef0": awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, + "foo": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + "": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + "SGR-0123456789abcdef0": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + "sgr-0123456789abcdef01": awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, + } { + requireAWSCode(t, sgRuleIDLookupError(id), code) + } +} + +func TestModifySecurityGroupRules_RuleIDResolution(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-ids") + otherSG := createTestSG(t, svc, vpcID, "mod-ids-other") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + otherRule := authorizeIngressTCP(t, svc, otherSG, 80, "10.0.0.0/24") + + requireAWSCode(t, modifyRules(svc, sgID, ruleUpdate("sgr-00000000000000000", tcpCIDRRequest(81, "10.0.0.0/24"))), + awserrors.ErrorInvalidSecurityGroupRuleIdNotFound) + requireAWSCode(t, modifyRules(svc, sgID, ruleUpdate("sgr-xyz", tcpCIDRRequest(81, "10.0.0.0/24"))), + awserrors.ErrorInvalidSecurityGroupRuleIdNotFound) + requireAWSCode(t, modifyRules(svc, sgID, ruleUpdate(otherRule, tcpCIDRRequest(81, "10.0.0.0/24"))), + awserrors.ErrorInvalidSecurityGroupRuleIdNotFound) + requireAWSCode(t, modifyRules(svc, sgID, ruleUpdate("SGR-0123456789abcdef0", tcpCIDRRequest(81, "10.0.0.0/24"))), + awserrors.ErrorInvalidSecurityGroupRuleIdMalformed) + + err := modifyRules(svc, sgID, + ruleUpdate(ruleID, tcpCIDRRequest(81, "10.0.0.0/24")), + ruleUpdate(ruleID, tcpCIDRRequest(82, "10.0.0.0/24")), + ) + requireAWSCode(t, err, awserrors.ErrorInvalidParameterValue) + assert.ErrorContains(t, err, "Duplicated security group rule ID") +} + +func TestModifySecurityGroupRules_RejectsEmptyRequest(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-empty") + + requireAWSCode(t, modifyRules(svc, sgID), awserrors.ErrorMissingParameter) + requireAWSCode(t, modifyRules(svc, ""), awserrors.ErrorMissingParameter) + _, err := svc.ModifySecurityGroupRules(context.Background(), nil, testAccountID) + requireAWSCode(t, err, awserrors.ErrorMissingParameter) +} + +func TestModifySecurityGroupRules_GroupNotFound(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + + requireAWSCode(t, modifyRules(svc, "sg-00000000000000000", ruleUpdate("sgr-00000000000000000", tcpCIDRRequest(80, "10.0.0.0/24"))), + awserrors.ErrorInvalidGroupNotFound) +} + +func TestSGRuleRequestToSGRule_Rejections(t *testing.T) { + t.Parallel() + for name, tc := range map[string]struct { + req *ec2.SecurityGroupRuleRequest + code string + }{ + "no body": {nil, awserrors.ErrorInvalidParameterValue}, + "omitted protocol": {&ec2.SecurityGroupRuleRequest{CidrIpv4: aws.String("10.0.0.0/24")}, awserrors.ErrorInvalidParameterValue}, + "unknown protocol": {&ec2.SecurityGroupRuleRequest{IpProtocol: aws.String("gre"), CidrIpv4: aws.String("10.0.0.0/24")}, awserrors.ErrorInvalidParameterValue}, + "tcp without ports": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), CidrIpv4: aws.String("10.0.0.0/24"), + }, awserrors.ErrorInvalidParameterValue}, + "udp with one port": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("udp"), FromPort: aws.Int64(53), CidrIpv4: aws.String("10.0.0.0/24"), + }, awserrors.ErrorInvalidParameterValue}, + "all protocols with 0/0": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("-1"), FromPort: aws.Int64(0), ToPort: aws.Int64(0), CidrIpv4: aws.String("10.0.0.0/24"), + }, awserrors.ErrorInvalidParameterValue}, + "all protocols with 22/22": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("-1"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), CidrIpv4: aws.String("10.0.0.0/24"), + }, awserrors.ErrorInvalidParameterValue}, + "no source": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), + }, awserrors.ErrorMissingParameter}, + "two sources": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), + CidrIpv4: aws.String("10.0.0.0/24"), ReferencedGroupId: aws.String("sg-0123456789abcdef0"), + }, awserrors.ErrorInvalidParameterCombination}, + "prefix list": {&ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), + PrefixListId: aws.String("pl-0123456789abcdef0"), + }, awserrors.ErrorInvalidPrefixListIDNotFound}, + "non-canonical cidr": {tcpCIDRRequest(22, "10.0.0.1/24"), awserrors.ErrorInvalidParameterValue}, + "unparseable cidr": {tcpCIDRRequest(22, "10.0.0.0/33"), awserrors.ErrorInvalidParameterValue}, + "ipv6 in CidrIpv4": {tcpCIDRRequest(22, "2001:db8::/32"), awserrors.ErrorInvalidParameterValue}, + } { + _, err := sgRuleRequestToSGRule(tc.req) + require.Error(t, err, name) + got, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, tc.code, got, "%s: %v", name, err) + } +} + +func TestSGRuleRequestToSGRule_Normalisation(t *testing.T) { + t.Parallel() + for name, tc := range map[string]struct { + req *ec2.SecurityGroupRuleRequest + want SGRule + }{ + "all protocols without ports": { + &ec2.SecurityGroupRuleRequest{IpProtocol: aws.String("-1"), CidrIpv4: aws.String("10.0.0.0/24")}, + SGRule{IpProtocol: "-1", CidrIp: "10.0.0.0/24"}, + }, + "all protocols with -1/-1": { + &ec2.SecurityGroupRuleRequest{IpProtocol: aws.String("-1"), FromPort: aws.Int64(-1), ToPort: aws.Int64(-1), CidrIpv4: aws.String("10.0.0.0/24")}, + SGRule{IpProtocol: "-1", CidrIp: "10.0.0.0/24"}, + }, + "numeric tcp": { + &ec2.SecurityGroupRuleRequest{IpProtocol: aws.String("6"), FromPort: aws.Int64(22), ToPort: aws.Int64(22), CidrIpv4: aws.String("10.0.0.0/24")}, + SGRule{IpProtocol: "tcp", FromPort: 22, ToPort: 22, CidrIp: "10.0.0.0/24"}, + }, + "description carried": { + &ec2.SecurityGroupRuleRequest{IpProtocol: aws.String("icmp"), FromPort: aws.Int64(8), ToPort: aws.Int64(0), CidrIpv6: aws.String("2001:db8::/32"), Description: aws.String("ping")}, + SGRule{IpProtocol: "icmp", FromPort: 8, CidrIpv6: "2001:db8::/32", Description: "ping"}, + }, + } { + got, err := sgRuleRequestToSGRule(tc.req) + require.NoError(t, err, name) + assert.Equal(t, tc.want, got, name) + } +} + +// A modified -1 rule is stored as 0/0 and reported as -1/-1, like authorize's +// no-ports form. +func TestModifySecurityGroupRules_AllProtocolStoresZeroPorts(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-all-proto") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + require.NoError(t, modifyRules(svc, sgID, ruleUpdate(ruleID, &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("-1"), FromPort: aws.Int64(-1), ToPort: aws.Int64(-1), + CidrIpv4: aws.String("10.0.0.0/24"), + }))) + + stored := storedSGRule(t, svc, sgID, ruleID) + assert.Equal(t, int64(0), stored.FromPort) + assert.Equal(t, int64(0), stored.ToPort) + got := sgRuleByID(t, svc, testAccountID, ruleID) + assert.Equal(t, int64(-1), aws.Int64Value(got.FromPort)) + assert.Equal(t, int64(-1), aws.Int64Value(got.ToPort)) +} + +func TestModifySecurityGroupRules_VpcdFailureFailsCall(t *testing.T) { + t.Parallel() + svc, nc := setupTestVPCServiceWithNC(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-vpcd-fail") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + failingUpdateResponder(t, nc, "ovn unavailable") + + err := modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(81, "10.0.0.0/24"))) + require.Error(t, err, "a vpc.update-sg failure must fail the call") + assert.ErrorContains(t, err, "ovn unavailable") +} + +// TestModifySecurityGroupRules_ConcurrentAuthorize races a modify against an +// authorize on the same group. The compare-and-swap makes one of them fail +// loudly; neither may silently drop the other's write. +func TestModifySecurityGroupRules_ConcurrentAuthorize(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + sgID := createTestSG(t, svc, vpcID, "mod-concurrent") + ruleID := authorizeIngressTCP(t, svc, sgID, 80, "10.0.0.0/24") + + var wg sync.WaitGroup + var modErr, authErr error + wg.Add(2) + go func() { + defer wg.Done() + modErr = modifyRules(svc, sgID, ruleUpdate(ruleID, tcpCIDRRequest(8080, "10.0.0.0/24"))) + }() + go func() { + defer wg.Done() + _, authErr = svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(8443), + ToPort: aws.Int64(8443), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}}, + }}, + }, testAccountID) + }() + wg.Wait() + + var modified, authorized bool + for _, r := range describeGroupRules(t, svc, sgID) { + if aws.StringValue(r.SecurityGroupRuleId) == ruleID && aws.Int64Value(r.FromPort) == 8080 { + modified = true + } + if aws.Int64Value(r.FromPort) == 8443 { + authorized = true + } + } + 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") +} diff --git a/spinifex/handlers/ec2/vpc/service.go b/spinifex/handlers/ec2/vpc/service.go index bf19e09d3..3f5b9a23d 100644 --- a/spinifex/handlers/ec2/vpc/service.go +++ b/spinifex/handlers/ec2/vpc/service.go @@ -32,6 +32,7 @@ type VPCService interface { RevokeSecurityGroupEgress(ctx context.Context, input *ec2.RevokeSecurityGroupEgressInput, accountID string) (*ec2.RevokeSecurityGroupEgressOutput, error) UpdateSecurityGroupRuleDescriptionsIngress(ctx context.Context, input *ec2.UpdateSecurityGroupRuleDescriptionsIngressInput, accountID string) (*ec2.UpdateSecurityGroupRuleDescriptionsIngressOutput, error) UpdateSecurityGroupRuleDescriptionsEgress(ctx context.Context, input *ec2.UpdateSecurityGroupRuleDescriptionsEgressInput, accountID string) (*ec2.UpdateSecurityGroupRuleDescriptionsEgressOutput, error) + ModifySecurityGroupRules(ctx context.Context, input *ec2.ModifySecurityGroupRulesInput, accountID string) (*ec2.ModifySecurityGroupRulesOutput, error) } // CentralTagStore keeps the central tag index in step with this package's diff --git a/spinifex/handlers/ec2/vpc/service_nats.go b/spinifex/handlers/ec2/vpc/service_nats.go index 020533dc2..126d37a34 100644 --- a/spinifex/handlers/ec2/vpc/service_nats.go +++ b/spinifex/handlers/ec2/vpc/service_nats.go @@ -116,3 +116,7 @@ func (s *NATSVPCService) UpdateSecurityGroupRuleDescriptionsIngress(ctx context. func (s *NATSVPCService) UpdateSecurityGroupRuleDescriptionsEgress(ctx context.Context, input *ec2.UpdateSecurityGroupRuleDescriptionsEgressInput, accountID string) (*ec2.UpdateSecurityGroupRuleDescriptionsEgressOutput, error) { return utils.NATSRequest[ec2.UpdateSecurityGroupRuleDescriptionsEgressOutput](ctx, s.natsConn, "ec2.UpdateSecurityGroupRuleDescriptionsEgress", input, 30*time.Second, accountID) } + +func (s *NATSVPCService) ModifySecurityGroupRules(ctx context.Context, input *ec2.ModifySecurityGroupRulesInput, accountID string) (*ec2.ModifySecurityGroupRulesOutput, error) { + return utils.NATSRequest[ec2.ModifySecurityGroupRulesOutput](ctx, s.natsConn, "ec2.ModifySecurityGroupRules", input, 30*time.Second, accountID) +} diff --git a/spinifex/network/subscribers/security_description_test.go b/spinifex/network/subscribers/security_description_test.go index 52f96948f..2a9553f1a 100644 --- a/spinifex/network/subscribers/security_description_test.go +++ b/spinifex/network/subscribers/security_description_test.go @@ -73,3 +73,43 @@ func TestUpdateSG_DescriptionDoesNotChangeACLSet(t *testing.T) { assert.Equal(t, before, aclFingerprints(t, withDescriptions), "a description change must rebuild an identical ACL set") } + +// TestUpdateSG_ModifiedRuleMatchesAuthorizedRule pins that the dataplane cannot +// tell a modify from an authorize: a rule edited in place under its old ID +// commits the same ACL set as a fresh rule with the same content. +func TestUpdateSG_ModifiedRuleMatchesAuthorizedRule(t *testing.T) { + const beforeModify = `{ + "group_id": "sg-mod", "vpc_id": "vpc-1", + "ingress_rules": [ + {"rule_id": "sgr-aaaaaaaaaaaaaaaaa", "ip_protocol": "tcp", "from_port": 8080, "to_port": 8080, "cidr_ip": "192.0.2.0/24"} + ], + "egress_rules": [ + {"rule_id": "sgr-ccccccccccccccccc", "ip_protocol": "-1", "from_port": 0, "to_port": 0, "cidr_ip": "0.0.0.0/0"} + ] + }` + const afterModify = `{ + "group_id": "sg-mod", "vpc_id": "vpc-1", + "ingress_rules": [ + {"rule_id": "sgr-aaaaaaaaaaaaaaaaa", "ip_protocol": "tcp", "from_port": 8081, "to_port": 8081, "cidr_ip": "198.51.100.0/24"} + ], + "egress_rules": [ + {"rule_id": "sgr-ccccccccccccccccc", "ip_protocol": "-1", "from_port": 0, "to_port": 0, "cidr_ip": "0.0.0.0/0"} + ] + }` + const freshlyAuthorized = `{ + "group_id": "sg-mod", "vpc_id": "vpc-1", + "ingress_rules": [ + {"rule_id": "sgr-ddddddddddddddddd", "ip_protocol": "tcp", "from_port": 8081, "to_port": 8081, "cidr_ip": "198.51.100.0/24"} + ], + "egress_rules": [ + {"rule_id": "sgr-ccccccccccccccccc", "ip_protocol": "-1", "from_port": 0, "to_port": 0, "cidr_ip": "0.0.0.0/0"} + ] + }` + + modified := aclFingerprints(t, afterModify) + require.NotEmpty(t, modified) + assert.Equal(t, aclFingerprints(t, freshlyAuthorized), modified, + "a modified rule must commit the same ACL set as an authorized one with its content") + assert.NotEqual(t, aclFingerprints(t, beforeModify), modified, + "a modify must change the committed ACL set") +} diff --git a/tests/e2e/single/sgreachability_test.go b/tests/e2e/single/sgreachability_test.go index e855bf8ab..b87e38e69 100644 --- a/tests/e2e/single/sgreachability_test.go +++ b/tests/e2e/single/sgreachability_test.go @@ -36,6 +36,8 @@ func sgReachabilityEgressRounds() int { // everything below it; failure here is fatal in the ordinary Go-test sense. // - Deny probes the pre-authorize state. Authorize doesn't need Deny to have // passed to be meaningful on its own, so there is no gate between them. +// - ModifyRule is API-only and edits a throwaway rule on the dedicated SG, +// never tcp/22, so no later stage depends on it. // - Authorize is the hard dependency for every later stage: Resolver, // WANEgress, and the egress round-trip all need a working SSH session, so // Authorize's failure aborts the rest of the scenario rather than let four @@ -109,6 +111,10 @@ func runSGReachabilityPolicy(t *testing.T, fix *Fixture) { t.Fatalf("Authorize stage failed; skipping every later stage that depends on tcp/22 ingress being open") } + t.Run("ModifyRule", func(t *testing.T) { + runSGModifyRule(t, fix, sgID) + }) + tgt := harness.SSHTarget{User: "ubuntu", Host: pubIP, Port: 22, KeyPath: keyPath} // Resolver exercises the guest resolver datapath: DHCP pointed the guest @@ -245,3 +251,79 @@ func runSGReachabilityPolicy(t *testing.T, fix *Fixture) { }) } } + +// runSGModifyRule edits a throwaway ingress rule in place and asserts the +// modify kept its SecurityGroupRuleId while replacing its content. +func runSGModifyRule(t *testing.T, fix *Fixture, sgID string) { + const oldPort, newPort = int64(8080), int64(8081) + const oldCIDR, newCIDR = "192.0.2.0/24", "198.51.100.0/24" + + harness.Step(t, "authorizing throwaway rule tcp/%d from %s", oldPort, oldCIDR) + out, err := fix.AWS.EC2.AuthorizeSecurityGroupIngress(&ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(oldPort), + ToPort: aws.Int64(oldPort), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String(oldCIDR)}}, + }}, + }) + require.NoError(t, err, "authorize throwaway rule") + require.Len(t, out.SecurityGroupRules, 1) + ruleID := aws.StringValue(out.SecurityGroupRules[0].SecurityGroupRuleId) + revoked := false + t.Cleanup(func() { + if revoked { + return + } + if _, err := fix.AWS.EC2.RevokeSecurityGroupIngress(&ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + SecurityGroupRuleIds: []*string{aws.String(ruleID)}, + }); err != nil { + t.Logf("WARNING: cleanup failed to revoke %s on %s: %v", ruleID, sgID, err) + } + }) + + harness.Step(t, "modifying %s to tcp/%d from %s", ruleID, newPort, newCIDR) + _, err = fix.AWS.EC2.ModifySecurityGroupRules(&ec2.ModifySecurityGroupRulesInput{ + GroupId: aws.String(sgID), + SecurityGroupRules: []*ec2.SecurityGroupRuleUpdate{{ + SecurityGroupRuleId: aws.String(ruleID), + SecurityGroupRule: &ec2.SecurityGroupRuleRequest{ + IpProtocol: aws.String("tcp"), + FromPort: aws.Int64(newPort), + ToPort: aws.Int64(newPort), + CidrIpv4: aws.String(newCIDR), + }, + }}, + }) + require.NoError(t, err, "ModifySecurityGroupRules") + + desc, err := fix.AWS.EC2.DescribeSecurityGroupRules(&ec2.DescribeSecurityGroupRulesInput{ + Filters: []*ec2.Filter{{Name: aws.String("group-id"), Values: []*string{aws.String(sgID)}}}, + }) + require.NoError(t, err, "DescribeSecurityGroupRules") + var found bool + for _, r := range desc.SecurityGroupRules { + if aws.BoolValue(r.IsEgress) { + continue + } + assert.Falsef(t, aws.Int64Value(r.FromPort) == oldPort || aws.StringValue(r.CidrIpv4) == oldCIDR, + "rule %s still carries the pre-modify content", aws.StringValue(r.SecurityGroupRuleId)) + if aws.StringValue(r.SecurityGroupRuleId) == ruleID { + found = true + assert.Equal(t, newPort, aws.Int64Value(r.FromPort)) + assert.Equal(t, newPort, aws.Int64Value(r.ToPort)) + assert.Equal(t, newCIDR, aws.StringValue(r.CidrIpv4)) + } + } + require.Truef(t, found, "rule %s is gone after the modify; its ID must survive", ruleID) + + harness.Step(t, "revoking %s by ID", ruleID) + _, err = fix.AWS.EC2.RevokeSecurityGroupIngress(&ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + SecurityGroupRuleIds: []*string{aws.String(ruleID)}, + }) + require.NoError(t, err, "revoke modified rule by ID") + revoked = true +}