fix: reject sg- group names, aws: tag keys and multi-attribute Modify*Attribute calls as AWS does

spinifex/daemon/daemon_handlers_test.go

@@ -608,7 +608,7 @@ func runInstancesAndCheckENISGs(t *testing.T, mutator func(input *ec2.RunInstanc
 	subnetID := *subnetOut.Subnet.SubnetId
 
 	sg1Out, err := daemon.vpcService.CreateSecurityGroup(t.Context(), &ec2.CreateSecurityGroupInput{
-		GroupName:   aws.String("sg-prop-1"),
+		GroupName:   aws.String("grp-prop-1"),
 		Description: aws.String("test"),
 		VpcId:       aws.String(vpcID),
 	}, testAccountID)
@@ -616,7 +616,7 @@ func runInstancesAndCheckENISGs(t *testing.T, mutator func(input *ec2.RunInstanc
 	sg1 = *sg1Out.GroupId
 
 	sg2Out, err := daemon.vpcService.CreateSecurityGroup(t.Context(), &ec2.CreateSecurityGroupInput{
-		GroupName:   aws.String("sg-prop-2"),
+		GroupName:   aws.String("grp-prop-2"),
 		Description: aws.String("test"),
 		VpcId:       aws.String(vpcID),
 	}, testAccountID)

spinifex/gateway/ec2/tags/CreateTags.go

@@ -3,6 +3,7 @@ package gateway_ec2_tags
 import (
 	"context"
 	"errors"
+	"strings"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
@@ -28,6 +29,11 @@ func ValidateCreateTagsInput(input *ec2.CreateTagsInput) error {
 		if tag.Key == nil || *tag.Key == "" {
 			return errors.New(awserrors.ErrorInvalidParameterValue)
 		}
+		// AWS reserves the prefix in any case.
+		if strings.HasPrefix(strings.ToLower(*tag.Key), "aws:") {
+			return awserrors.Errorf(awserrors.ErrorInvalidParameterValue,
+				"Value ( %s ) for parameter key is invalid. Tag keys starting with 'aws:' are reserved for internal use", *tag.Key)
+		}
 	}
 
 	return nil

spinifex/gateway/ec2/tags/CreateTags_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestValidateCreateTagsInput(t *testing.T) {
@@ -138,3 +139,21 @@ func TestValidateDeleteTagsInput(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateCreateTagsInput_RejectsReservedAWSPrefix(t *testing.T) {
+	for _, key := range []string{"aws:foo", "AWS:Foo"} {
+		err := ValidateCreateTagsInput(&ec2.CreateTagsInput{
+			Resources: []*string{aws.String("vpc-1234567890abcdef0")},
+			Tags:      []*ec2.Tag{{Key: aws.String(key), Value: aws.String("v")}},
+		})
+		code, msg, ok := awserrors.ResolveErrorDetail(err)
+		require.True(t, ok, "error %v carries no AWS code", err)
+		assert.Equal(t, awserrors.ErrorInvalidParameterValue, code)
+		assert.Equal(t, "Value ( "+key+" ) for parameter key is invalid. Tag keys starting with 'aws:' are reserved for internal use", msg)
+	}
+
+	assert.NoError(t, ValidateCreateTagsInput(&ec2.CreateTagsInput{
+		Resources: []*string{aws.String("vpc-1234567890abcdef0")},
+		Tags:      []*ec2.Tag{{Key: aws.String("team:aws:owner"), Value: aws.String("v")}},
+	}))
+}

spinifex/gateway/ec2/vpc/ModifyNetworkInterfaceAttribute.go

@@ -17,14 +17,7 @@ func ValidateModifyNetworkInterfaceAttributeInput(input *ec2.ModifyNetworkInterf
 	if input.NetworkInterfaceId == nil || *input.NetworkInterfaceId == "" {
 		return errors.New(awserrors.ErrorMissingParameter)
 	}
-	if len(input.Groups) == 0 && input.Description == nil && input.SourceDestCheck == nil {
-		return errors.New(awserrors.ErrorInvalidParameterValue)
-	}
-	// Disabling source/dest check is unsupported: OVN port security enforces it.
-	if input.SourceDestCheck != nil && input.SourceDestCheck.Value != nil && !*input.SourceDestCheck.Value {
-		return errors.New(awserrors.ErrorUnsupported)
-	}
-	return nil
+	return handlers_ec2_vpc.ValidateModifyNetworkInterfaceAttributeAttributes(input)
 }
 
 // ModifyNetworkInterfaceAttribute handles the EC2 ModifyNetworkInterfaceAttribute API call.

spinifex/gateway/ec2/vpc/ModifyNetworkInterfaceAttribute_test.go

@@ -57,3 +57,16 @@ func TestModifyNetworkInterfaceAttribute_NilNATS(t *testing.T) {
 	}, nil, "123456789012")
 	assert.Error(t, err)
 }
+
+// AWS rejects the combination before judging any one attribute, so disabling
+// source/dest check alongside a description is not reported as Unsupported.
+func TestModifyNetworkInterfaceAttribute_CombinationBeforeUnsupported(t *testing.T) {
+	err := ValidateModifyNetworkInterfaceAttributeInput(&ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String("eni-abc123"),
+		SourceDestCheck:    &ec2.AttributeBooleanValue{Value: aws.Bool(false)},
+		Description:        &ec2.AttributeValue{Value: aws.String("desc")},
+	})
+	code, ok := awserrors.ResolveErrorCode(err)
+	assert.True(t, ok, "error %v carries no AWS code", err)
+	assert.Equal(t, awserrors.ErrorInvalidParameterCombination, code)
+}

spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go

@@ -161,7 +161,7 @@ func TestGetSecurityGroupsForVpc_PagesEveryGroupOnce(t *testing.T) {
 	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
 
 	expected := describedGroupIDsForVpc(t, svc, vpcID)
-	for _, name := range []string{"sg-a", "sg-b", "sg-c", "sg-d", "sg-e", "sg-f", "sg-g"} {
+	for _, name := range []string{"grp-a", "grp-b", "grp-c", "grp-d", "grp-e", "grp-f", "grp-g"} {
 		expected = append(expected, createTestSG(t, svc, vpcID, name))
 	}
 

spinifex/handlers/ec2/vpc/eni.go

@@ -405,18 +405,48 @@ func (s *VPCServiceImpl) DetachAndDeleteENI(ctx context.Context, accountID, eniI
 	return false, fmt.Errorf("DetachAndDeleteENI: exhausted %d CAS attempts for %s: %w", maxAttempts, eniID, lastErr)
 }
 
-// ModifyNetworkInterfaceAttribute modifies ENI attributes (security groups,
-// description). SourceDestCheck=true is accepted as a no-op.
+// ValidateModifyNetworkInterfaceAttributeAttributes checks the attribute fields
+// of a ModifyNetworkInterfaceAttribute request, which AWS does before it looks
+// the interface up. The gateway and the handler share it so their order agrees.
+func ValidateModifyNetworkInterfaceAttributeAttributes(input *ec2.ModifyNetworkInterfaceAttributeInput) error {
+	var set []string
+	if input.SourceDestCheck != nil {
+		set = append(set, "sourceDestCheck")
+	}
+	if input.Description != nil {
+		set = append(set, "description")
+	}
+	if len(input.Groups) > 0 {
+		set = append(set, "securityGroups")
+	}
+	if len(set) == 0 {
+		return errors.New(awserrors.ErrorInvalidParameterValue)
+	}
+	if len(set) > 1 {
+		return multipleAttributesError(set)
+	}
+	// Disabling source/dest check is unsupported: OVN port security enforces it.
+	if input.SourceDestCheck != nil && input.SourceDestCheck.Value != nil && !*input.SourceDestCheck.Value {
+		return errors.New(awserrors.ErrorUnsupported)
+	}
+	return nil
+}
+
+// multipleAttributesError is AWS's answer to a Modify*Attribute request that
+// sets more than one attribute. AWS's name order is not stable; this one is.
+func multipleAttributesError(names []string) error {
+	return awserrors.Errorf(awserrors.ErrorInvalidParameterCombination,
+		"Fields for multiple attribute types specified: %s", strings.Join(names, ", "))
+}
+
+// ModifyNetworkInterfaceAttribute modifies one ENI attribute (security groups
+// or description). SourceDestCheck=true is accepted as a no-op.
 func (s *VPCServiceImpl) ModifyNetworkInterfaceAttribute(ctx context.Context, input *ec2.ModifyNetworkInterfaceAttributeInput, accountID string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) {
 	if input.NetworkInterfaceId == nil || *input.NetworkInterfaceId == "" {
 		return nil, errors.New(awserrors.ErrorMissingParameter)
 	}
-	if len(input.Groups) == 0 && input.Description == nil && input.SourceDestCheck == nil {
-		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
-	}
-	// Disabling source/dest check is unsupported: OVN port security enforces it.
-	if input.SourceDestCheck != nil && input.SourceDestCheck.Value != nil && !*input.SourceDestCheck.Value {
-		return nil, errors.New(awserrors.ErrorUnsupported)
+	if err := ValidateModifyNetworkInterfaceAttributeAttributes(input); err != nil {
+		return nil, err
 	}
 
 	eniId := *input.NetworkInterfaceId

spinifex/handlers/ec2/vpc/eni_test.go

@@ -830,8 +830,8 @@ func TestModifyNetworkInterfaceAttribute_SecurityGroups(t *testing.T) {
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
 	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
 	eniId := createTestENI(t, svc, subnetId)
-	sg1 := createTestSG(t, svc, vpcId, "sg-one")
-	sg2 := createTestSG(t, svc, vpcId, "sg-two")
+	sg1 := createTestSG(t, svc, vpcId, "grp-one")
+	sg2 := createTestSG(t, svc, vpcId, "grp-two")
 
 	_, err := svc.ModifyNetworkInterfaceAttribute(context.Background(), &ec2.ModifyNetworkInterfaceAttributeInput{
 		NetworkInterfaceId: aws.String(eniId),
@@ -855,8 +855,8 @@ func TestModifyNetworkInterfaceAttribute_PublishesUpdatePortSGs(t *testing.T) {
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
 	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
 	eniId := createTestENI(t, svc, subnetId)
-	sg1 := createTestSG(t, svc, vpcId, "sg-mod-1")
-	sg2 := createTestSG(t, svc, vpcId, "sg-mod-2")
+	sg1 := createTestSG(t, svc, vpcId, "grp-mod-1")
+	sg2 := createTestSG(t, svc, vpcId, "grp-mod-2")
 
 	eventCh := make(chan *nats.Msg, 1)
 	sub, err := nc.Subscribe("vpc.update-port-sgs", func(msg *nats.Msg) {
@@ -913,7 +913,7 @@ func TestCreateNetworkInterface_PublishesEventCarriesSGs(t *testing.T) {
 	svc, nc := setupTestVPCServiceWithNC(t)
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
 	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
-	sgA := createTestSG(t, svc, vpcId, "sg-evt-A")
+	sgA := createTestSG(t, svc, vpcId, "grp-evt-A")
 
 	eventCh := make(chan *nats.Msg, 1)
 	sub, err := nc.Subscribe("vpc.create-port", func(msg *nats.Msg) {
@@ -1062,8 +1062,8 @@ func TestCreateNetworkInterface_WithSecurityGroups(t *testing.T) {
 	svc := setupTestVPCService(t)
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
 	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
-	sgA := createTestSG(t, svc, vpcId, "sg-aaa")
-	sgB := createTestSG(t, svc, vpcId, "sg-bbb")
+	sgA := createTestSG(t, svc, vpcId, "grp-aaa")
+	sgB := createTestSG(t, svc, vpcId, "grp-bbb")
 
 	out, err := svc.CreateNetworkInterface(context.Background(), &ec2.CreateNetworkInterfaceInput{
 		SubnetId: aws.String(subnetId),
@@ -1222,8 +1222,8 @@ func TestDescribeNetworkInterfaces_FilterByGroupId(t *testing.T) {
 	svc := setupTestVPCService(t)
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
 	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
-	sgA := createTestSG(t, svc, vpcId, "sg-aaa")
-	sgB := createTestSG(t, svc, vpcId, "sg-bbb")
+	sgA := createTestSG(t, svc, vpcId, "grp-aaa")
+	sgB := createTestSG(t, svc, vpcId, "grp-bbb")
 
 	// Create ENI with security groups
 	out, err := svc.CreateNetworkInterface(context.Background(), &ec2.CreateNetworkInterfaceInput{
@@ -1465,7 +1465,7 @@ func TestValidateSGAttachment_TooMany(t *testing.T) {
 	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
 	sgs := make([]string, 6)
 	for i := range sgs {
-		sgs[i] = createTestSG(t, svc, vpcID, fmt.Sprintf("sg-%d", i))
+		sgs[i] = createTestSG(t, svc, vpcID, fmt.Sprintf("grp-%d", i))
 	}
 
 	err := svc.validateSGAttachment(t.Context(), testAccountID, sgs, vpcID)
@@ -1508,7 +1508,7 @@ func TestModifyNetworkInterfaceAttribute_VpcdError_Propagated(t *testing.T) {
 	vpcId := createTestVPC(t, svc, "10.0.0.0/16")
 	subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24")
 	eniId := createTestENI(t, svc, subnetId)
-	sg1 := createTestSG(t, svc, vpcId, "sg-mod-fail-1")
+	sg1 := createTestSG(t, svc, vpcId, "grp-mod-fail-1")
 
 	// Swap the stub's vpc.update-port-sgs reply to an error in-place so
 	// there's exactly one responder (no race with a layered subscriber).
@@ -1552,3 +1552,22 @@ func TestUpdateENIPublicIP_NotFound(t *testing.T) {
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "eni-missing")
 }
+
+// AWS checks the attribute combination before it looks the interface up, and
+// its order of names is unstable, so only the names are asserted.
+func TestModifyNetworkInterfaceAttribute_MultipleAttributes(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+
+	_, err := svc.ModifyNetworkInterfaceAttribute(context.Background(), &ec2.ModifyNetworkInterfaceAttributeInput{
+		NetworkInterfaceId: aws.String("eni-nonexistent"),
+		Groups:             []*string{aws.String("sg-111")},
+		Description:        &ec2.AttributeValue{Value: aws.String("desc")},
+	}, testAccountID)
+	code, msg, ok := awserrors.ResolveErrorDetail(err)
+	require.True(t, ok, "error %v carries no AWS code", err)
+	assert.Equal(t, awserrors.ErrorInvalidParameterCombination, code)
+	assert.Contains(t, msg, "Fields for multiple attribute types specified: ")
+	assert.Contains(t, msg, "description")
+	assert.Contains(t, msg, "securityGroups")
+}

spinifex/handlers/ec2/vpc/security_group.go

@@ -193,6 +193,10 @@ func (s *VPCServiceImpl) CreateSecurityGroup(ctx context.Context, input *ec2.Cre
 	if err := s.requireVPCExists(ctx, accountID, vpcId); err != nil {
 		return nil, err
 	}
+	if strings.HasPrefix(groupName, "sg-") {
+		return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue,
+			"Value (%s) for parameter GroupName is invalid. Group names may not be in the format sg-*.", groupName)
+	}
 
 	// Check for duplicate group name in the same VPC and enforce the per-VPC
 	// SG quota in the same bucket walk.

spinifex/handlers/ec2/vpc/security_group_test.go

@@ -131,8 +131,8 @@ func TestDescribeSecurityGroups_All(t *testing.T) {
 	t.Parallel()
 	svc := setupTestVPCService(t)
 	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
-	createTestSG(t, svc, vpcID, "sg-a")
-	createTestSG(t, svc, vpcID, "sg-b")
+	createTestSG(t, svc, vpcID, "grp-a")
+	createTestSG(t, svc, vpcID, "grp-b")
 
 	desc, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{}, testAccountID)
 	require.NoError(t, err)
@@ -1301,8 +1301,8 @@ func TestDescribeSecurityGroups_FilterByVpcId(t *testing.T) {
 	svc := setupTestVPCService(t)
 	vpc1 := createTestVPC(t, svc, "10.0.0.0/16")
 	vpc2 := createTestVPC(t, svc, "172.16.0.0/16")
-	createTestSG(t, svc, vpc1, "sg-in-vpc1")
-	createTestSG(t, svc, vpc2, "sg-in-vpc2")
+	createTestSG(t, svc, vpc1, "grp-in-vpc1")
+	createTestSG(t, svc, vpc2, "grp-in-vpc2")
 
 	out, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{
 		Filters: []*ec2.Filter{
@@ -1354,13 +1354,13 @@ func TestDescribeSecurityGroups_FilterMultipleValues_OR(t *testing.T) {
 	t.Parallel()
 	svc := setupTestVPCService(t)
 	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
-	createTestSG(t, svc, vpcID, "sg-alpha")
-	createTestSG(t, svc, vpcID, "sg-beta")
-	createTestSG(t, svc, vpcID, "sg-gamma")
+	createTestSG(t, svc, vpcID, "grp-alpha")
+	createTestSG(t, svc, vpcID, "grp-beta")
+	createTestSG(t, svc, vpcID, "grp-gamma")
 
 	out, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{
 		Filters: []*ec2.Filter{
-			{Name: aws.String("group-name"), Values: []*string{aws.String("sg-alpha"), aws.String("sg-gamma")}},
+			{Name: aws.String("group-name"), Values: []*string{aws.String("grp-alpha"), aws.String("grp-gamma")}},
 		},
 	}, testAccountID)
 	require.NoError(t, err)
@@ -1554,6 +1554,30 @@ func TestCreateSecurityGroup_RejectsReservedDefaultName(t *testing.T) {
 	assert.Contains(t, err.Error(), "InvalidGroup.Reserved")
 }
 
+func TestCreateSecurityGroup_RejectsSgPrefixedName(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+
+	_, err := svc.CreateSecurityGroup(context.Background(), &ec2.CreateSecurityGroupInput{
+		GroupName:   aws.String("sg-foo"),
+		Description: aws.String("d"),
+		VpcId:       aws.String(vpcID),
+	}, testAccountID)
+	code, msg, ok := awserrors.ResolveErrorDetail(err)
+	require.True(t, ok, "error %v carries no AWS code", err)
+	assert.Equal(t, awserrors.ErrorInvalidParameterValue, code)
+	assert.Equal(t, "Value (sg-foo) for parameter GroupName is invalid. Group names may not be in the format sg-*.", msg)
+
+	// AWS reports a missing VPC ahead of the name.
+	_, err = svc.CreateSecurityGroup(context.Background(), &ec2.CreateSecurityGroupInput{
+		GroupName:   aws.String("sg-foo"),
+		Description: aws.String("d"),
+		VpcId:       aws.String("vpc-nonexistent"),
+	}, testAccountID)
+	assert.ErrorContains(t, err, awserrors.ErrorInvalidVpcIDNotFound)
+}
+
 func TestDeleteSecurityGroup_RejectsDefault(t *testing.T) {
 	t.Parallel()
 	svc := setupTestVPCService(t)

spinifex/handlers/ec2/vpc/service_impl.go

@@ -1363,9 +1363,22 @@ func (s *VPCServiceImpl) ModifyVpcAttribute(ctx context.Context, input *ec2.Modi
 	if input.VpcId == nil || *input.VpcId == "" {
 		return nil, errors.New(awserrors.ErrorMissingParameter)
 	}
-	if input.EnableDnsHostnames == nil && input.EnableDnsSupport == nil && input.EnableNetworkAddressUsageMetrics == nil {
+	var set []string
+	if input.EnableDnsSupport != nil {
+		set = append(set, "enableDnsSupport")
+	}
+	if input.EnableDnsHostnames != nil {
+		set = append(set, "enableDnsHostnames")
+	}
+	if input.EnableNetworkAddressUsageMetrics != nil {
+		set = append(set, "enableNetworkAddressUsageMetrics")
+	}
+	if len(set) == 0 {
 		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
 	}
+	if len(set) > 1 {
+		return nil, multipleAttributesError(set)
+	}
 
 	vpcID := *input.VpcId
 	key := utils.AccountKey(accountID, vpcID)

spinifex/handlers/ec2/vpc/service_impl_test.go

@@ -2485,3 +2485,22 @@ func findTag(tags []*ec2.Tag, key string) string {
 	}
 	return ""
 }
+
+// AWS checks the attribute combination before it looks the VPC up, and its
+// order of names is unstable, so only the names are asserted.
+func TestVpc_ModifyVpcAttribute_MultipleAttributes(t *testing.T) {
+	t.Parallel()
+	svc := setupTestVPCService(t)
+
+	_, err := svc.ModifyVpcAttribute(context.Background(), &ec2.ModifyVpcAttributeInput{
+		VpcId:              aws.String("vpc-nonexistent"),
+		EnableDnsSupport:   &ec2.AttributeBooleanValue{Value: aws.Bool(true)},
+		EnableDnsHostnames: &ec2.AttributeBooleanValue{Value: aws.Bool(true)},
+	}, testAccountID)
+	code, msg, ok := awserrors.ResolveErrorDetail(err)
+	require.True(t, ok, "error %v carries no AWS code", err)
+	assert.Equal(t, awserrors.ErrorInvalidParameterCombination, code)
+	assert.Contains(t, msg, "Fields for multiple attribute types specified: ")
+	assert.Contains(t, msg, "enableDnsSupport")
+	assert.Contains(t, msg, "enableDnsHostnames")
+}