feat: implement EC2 ModifySecurityGroupRules (#1103)

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** |

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",

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` |

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`

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"},

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

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.

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{},

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
+}

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)
+	}
+}

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")

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{

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")
+}

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

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)
+}

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")
+}

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
+}