fix(ec2): refuse malformed image, instance and volume IDs on Describe instead of answering NotFound (#1425)

spinifex/domains/ec2/awsapi/actions.go

@@ -134,6 +134,9 @@ func marshalEC2Response(action string, output any) ([]byte, error) {
 
 var ec2Actions = map[string]ec2Action{
 	"DescribeInstances": ec2Handler(func(ctx context.Context, input *ec2.DescribeInstancesInput, c *call, accountID string) (any, error) {
+		if err := ec2instanceapi.ValidateDescribeInstancesInput(input); err != nil {
+			return nil, err
+		}
 		out, err := ec2instanceapi.DescribeInstancesChecked(ctx, input, c.NATS, 0, c.NodeIDs, accountID)
 		if err != nil {
 			return out, err
@@ -257,6 +260,9 @@ var ec2Actions = map[string]ec2Action{
 		}
 	}),
 	"DescribeImages": ec2Handler(func(ctx context.Context, input *ec2.DescribeImagesInput, c *call, accountID string) (any, error) {
+		if err := ec2imageapi.ValidateDescribeImagesInput(input); err != nil {
+			return nil, err
+		}
 		return ec2imageapi.DescribeImages(ctx, input, c.NATS, accountID)
 	}),
 	"CreateImage": ec2Handler(func(ctx context.Context, input *ec2.CreateImageInput, c *call, accountID string) (any, error) {
@@ -287,6 +293,9 @@ var ec2Actions = map[string]ec2Action{
 		return ec2zoneapi.DescribeAvailabilityZones(input, c.Region, c.AZ)
 	}),
 	"DescribeVolumes": ec2Handler(func(ctx context.Context, input *ec2.DescribeVolumesInput, c *call, accountID string) (any, error) {
+		if err := ec2volumeapi.ValidateDescribeVolumesInput(input); err != nil {
+			return nil, err
+		}
 		return ec2volumeapi.DescribeVolumes(ctx, input, c.NATS, accountID)
 	}),
 	"ModifyVolume": ec2Handler(func(ctx context.Context, input *ec2.ModifyVolumeInput, c *call, accountID string) (any, error) {

spinifex/domains/ec2/awsapi/actions_test.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/mulgadc/spinifex/spinifex/foundation/aws/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -30,3 +31,27 @@ func TestImportKeyPair_Base64PaddingWorkaround(t *testing.T) {
 	assert.NotContains(t, q["PublicKeyMaterial"], "%3D",
 		"Expected no URL-encoded padding remaining")
 }
+
+// A well-prefixed ID of the wrong shape is refused before any service is
+// asked, so it can never come back as NotFound.
+func TestDescribe_MalformedIDRejectedBeforeLookup(t *testing.T) {
+	tests := []struct {
+		action, param, id, code, msg string
+	}{
+		{"DescribeInstances", "InstanceId.1", "i-zzz", awserrors.ErrorInvalidInstanceIDMalformed, `Invalid id: "i-zzz"`},
+		{"DescribeImages", "ImageId.1", "ami-zzz", awserrors.ErrorInvalidAMIIDMalformed, `Invalid id: "ami-zzz"`},
+		{"DescribeVolumes", "VolumeId.1", "vol-zzz", awserrors.ErrorInvalidParameterValue, "Value (vol-zzz) for parameter volumes is invalid. Expected: 'vol-...'."},
+	}
+	for _, tt := range tests {
+		t.Run(tt.action, func(t *testing.T) {
+			h := ec2Actions[tt.action]
+			input, err := h.parse(map[string]string{tt.param: tt.id})
+			require.NoError(t, err)
+			_, err = h.dispatch(t.Context(), tt.action, input, &call{Handler: NewHandler(Deps{})}, "123456789012")
+			code, msg, ok := awserrors.ResolveErrorDetail(err)
+			require.True(t, ok)
+			assert.Equal(t, tt.code, code)
+			assert.Equal(t, tt.msg, msg)
+		})
+	}
+}

spinifex/domains/ec2/awsapi/image/DescribeImages.go

@@ -5,6 +5,7 @@ package image
 import (
 	"context"
 	"errors"
+	"regexp"
 	"strings"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -13,8 +14,12 @@ import (
 	"github.com/nats-io/nats.go"
 )
 
-// ValidateDescribeImagesInput accepts a nil input and rejects any ImageIds entry without the ami-
-// prefix with InvalidAMIID.Malformed.
+// imageIDSuffixPattern is the AWS AMI ID shape after ami-: 8 (legacy) or 17
+// lowercase hex characters.
+var imageIDSuffixPattern = regexp.MustCompile(`^([0-9a-f]{8}|[0-9a-f]{17})$`)
+
+// ValidateDescribeImagesInput accepts a nil input and rejects any ImageIds entry that is not
+// ami- followed by 8 or 17 lowercase hex characters with InvalidAMIID.Malformed.
 func ValidateDescribeImagesInput(input *ec2.DescribeImagesInput) (err error) {
 	if input == nil {
 		return nil
@@ -22,9 +27,16 @@ func ValidateDescribeImagesInput(input *ec2.DescribeImagesInput) (err error) {
 
 	if input.ImageIds != nil {
 		for _, imageId := range input.ImageIds {
-			if imageId != nil && !strings.HasPrefix(*imageId, "ami-") {
+			if imageId == nil {
+				continue
+			}
+			suffix, ok := strings.CutPrefix(*imageId, "ami-")
+			if !ok {
 				return errors.New(awserrors.ErrorInvalidAMIIDMalformed)
 			}
+			if !imageIDSuffixPattern.MatchString(suffix) {
+				return awserrors.Errorf(awserrors.ErrorInvalidAMIIDMalformed, "Invalid id: %q", *imageId)
+			}
 		}
 	}
 
@@ -34,11 +46,6 @@ func ValidateDescribeImagesInput(input *ec2.DescribeImagesInput) (err error) {
 // DescribeImages implements the EC2 DescribeImages action, returning the AMIs visible to
 // accountID from the NATS image service.
 func DescribeImages(ctx context.Context, input *ec2.DescribeImagesInput, natsConn *nats.Conn, accountID string) (output ec2.DescribeImagesOutput, err error) {
-	err = ValidateDescribeImagesInput(input)
-	if err != nil {
-		return output, err
-	}
-
 	imageService := ec2image.NewNATSImageService(natsConn, 0)
 	result, err := imageService.DescribeImages(ctx, input, accountID)
 	if err != nil {

spinifex/domains/ec2/awsapi/image/DescribeImages_test.go

@@ -53,8 +53,8 @@ func TestValidateDescribeImagesInput(t *testing.T) {
 			name: "MultipleValidImageIds",
 			input: &ec2.DescribeImagesInput{
 				ImageIds: []*string{
-					aws.String("ami-111"),
-					aws.String("ami-222"),
+					aws.String("ami-1a2b3c4d"),
+					aws.String("ami-0123456789abcdef1"),
 				},
 			},
 			wantErr: false,
@@ -63,7 +63,7 @@ func TestValidateDescribeImagesInput(t *testing.T) {
 			name: "MixedValidAndInvalidImageIds",
 			input: &ec2.DescribeImagesInput{
 				ImageIds: []*string{
-					aws.String("ami-valid"),
+					aws.String("ami-0123456789abcdef0"),
 					aws.String("invalid-id"),
 				},
 			},
@@ -85,11 +85,24 @@ func TestValidateDescribeImagesInput(t *testing.T) {
 			err := ValidateDescribeImagesInput(tt.input)
 
 			if tt.wantErr {
-				assert.Error(t, err)
-				assert.Equal(t, tt.errMsg, err.Error())
+				code, ok := awserrors.ResolveErrorCode(err)
+				assert.True(t, ok)
+				assert.Equal(t, tt.errMsg, code)
 			} else {
 				assert.NoError(t, err)
 			}
 		})
 	}
 }
+
+// AWS answers a well-prefixed ID whose suffix is not 8 or 17 lowercase hex
+// characters with Malformed, not NotFound.
+func TestValidateDescribeImagesInput_MalformedSuffix(t *testing.T) {
+	for _, id := range []string{"ami-zzz", "ami-0123456789ABCDEF0", "ami-0123456789abcdef", "ami-"} {
+		err := ValidateDescribeImagesInput(&ec2.DescribeImagesInput{ImageIds: []*string{aws.String(id)}})
+		code, msg, ok := awserrors.ResolveErrorDetail(err)
+		assert.True(t, ok, id)
+		assert.Equal(t, awserrors.ErrorInvalidAMIIDMalformed, code, id)
+		assert.Equal(t, `Invalid id: "`+id+`"`, msg, id)
+	}
+}

spinifex/domains/ec2/awsapi/image/handler_test.go

@@ -39,13 +39,6 @@ func TestCreateImage_NilNATS(t *testing.T) {
 	assert.Error(t, err)
 }
 
-func TestDescribeImages_ValidationErrors(t *testing.T) {
-	_, err := DescribeImages(context.Background(), &ec2.DescribeImagesInput{
-		ImageIds: []*string{aws.String("bad-id")},
-	}, nil, "")
-	assert.EqualError(t, err, awserrors.ErrorInvalidAMIIDMalformed)
-}
-
 func TestDescribeImages_NilNATS(t *testing.T) {
 	_, err := DescribeImages(context.Background(), nil, nil, "acct-123")
 	assert.Error(t, err)

spinifex/domains/ec2/awsapi/instance/DescribeInstances.go

@@ -89,6 +89,12 @@ func DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, n
 	return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil
 }
 
+// ValidateDescribeInstancesInput rejects any InstanceIds entry that is not a
+// well-formed instance ID with InvalidInstanceID.Malformed.
+func ValidateDescribeInstancesInput(input *ec2.DescribeInstancesInput) error {
+	return validateInstanceIDFormat(input.InstanceIds)
+}
+
 // DescribeInstancesChecked is the customer-facing variant served by the
 // gateway's DescribeInstances action. On top of DescribeInstances' aggregation,
 // it asserts InvalidInstanceID.NotFound for any explicitly named instance ID

spinifex/domains/ec2/awsapi/instance/instanceid.go

@@ -25,7 +25,7 @@ var instanceIDPattern = regexp.MustCompile(`^i-([0-9a-f]{8}|[0-9a-f]{17})$`)
 func validateInstanceIDFormat(ids []*string) error {
 	for _, id := range ids {
 		if id != nil && !instanceIDPattern.MatchString(*id) {
-			return errors.New(awserrors.ErrorInvalidInstanceIDMalformed)
+			return awserrors.Errorf(awserrors.ErrorInvalidInstanceIDMalformed, "Invalid id: %q", *id)
 		}
 	}
 	return nil

spinifex/domains/ec2/awsapi/instance/instanceid_test.go

@@ -57,8 +57,10 @@ func TestValidateInstanceIDFormat(t *testing.T) {
 	}
 	for _, id := range malformed {
 		err := validateInstanceIDFormat([]*string{aws.String(id)})
-		require.Error(t, err, id)
-		assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, err.Error(), id)
+		code, msg, ok := awserrors.ResolveErrorDetail(err)
+		require.True(t, ok, id)
+		assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, code, id)
+		assert.Equal(t, `Invalid id: "`+id+`"`, msg, id)
 	}
 	err := validateInstanceIDFormat([]*string{aws.String("i-0123456789abcdef0"), aws.String("i-abc")})
 	require.Error(t, err, "one malformed id fails the whole batch")
@@ -100,8 +102,9 @@ func TestLifecycleVerbs_MalformedIDRejectedBeforeNATS(t *testing.T) {
 	}
 	for name, call := range calls {
 		err := call()
-		require.Error(t, err, name)
-		assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, err.Error(), name)
+		code, ok := awserrors.ResolveErrorCode(err)
+		require.True(t, ok, name)
+		assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, code, name)
 	}
 	require.NoError(t, nc.Flush())
 	assert.Zero(t, requests.Load(), "no NATS request may be sent for a batch holding a malformed id")

spinifex/domains/ec2/awsapi/volume/DescribeVolumes.go

@@ -5,6 +5,7 @@ package volume
 import (
 	"context"
 	"errors"
+	"regexp"
 	"strings"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -13,8 +14,13 @@ import (
 	"github.com/nats-io/nats.go"
 )
 
-// ValidateDescribeVolumesInput accepts a nil input and rejects any VolumeIds entry without the
-// vol- prefix with InvalidVolumeID.Malformed.
+// volumeIDSuffixPattern is the AWS volume ID shape after vol-: 8 (legacy) or
+// 17 lowercase hex characters.
+var volumeIDSuffixPattern = regexp.MustCompile(`^([0-9a-f]{8}|[0-9a-f]{17})$`)
+
+// ValidateDescribeVolumesInput accepts a nil input, rejects any VolumeIds entry without the
+// vol- prefix with InvalidVolumeID.Malformed, and any with a malformed suffix with
+// InvalidParameterValue, as AWS does.
 func ValidateDescribeVolumesInput(input *ec2.DescribeVolumesInput) error {
 	if input == nil {
 		return nil
@@ -22,9 +28,17 @@ func ValidateDescribeVolumesInput(input *ec2.DescribeVolumesInput) error {
 
 	if input.VolumeIds != nil {
 		for _, volumeId := range input.VolumeIds {
-			if volumeId != nil && !strings.HasPrefix(*volumeId, "vol-") {
+			if volumeId == nil {
+				continue
+			}
+			suffix, ok := strings.CutPrefix(*volumeId, "vol-")
+			if !ok {
 				return errors.New(awserrors.ErrorInvalidVolumeIDMalformed)
 			}
+			if !volumeIDSuffixPattern.MatchString(suffix) {
+				return awserrors.Errorf(awserrors.ErrorInvalidParameterValue,
+					"Value (%s) for parameter volumes is invalid. Expected: 'vol-...'.", *volumeId)
+			}
 		}
 	}
 
@@ -35,11 +49,6 @@ func ValidateDescribeVolumesInput(input *ec2.DescribeVolumesInput) error {
 func DescribeVolumes(ctx context.Context, input *ec2.DescribeVolumesInput, natsConn *nats.Conn, accountID string) (ec2.DescribeVolumesOutput, error) {
 	var output ec2.DescribeVolumesOutput
 
-	err := ValidateDescribeVolumesInput(input)
-	if err != nil {
-		return output, err
-	}
-
 	volumeService := ec2volume.NewNATSVolumeService(natsConn)
 	result, err := volumeService.DescribeVolumes(ctx, input, accountID)
 	if err != nil {

spinifex/domains/ec2/awsapi/volume/DescribeVolumes_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/mulgadc/spinifex/spinifex/foundation/aws/errors"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -36,9 +37,9 @@ func TestValidateDescribeVolumesInput(t *testing.T) {
 			name: "ValidMultipleVolumeIds",
 			input: &ec2.DescribeVolumesInput{
 				VolumeIds: []*string{
-					aws.String("vol-abc123"),
-					aws.String("vol-def456"),
-					aws.String("vol-ghi789"),
+					aws.String("vol-0123456789abcdef1"),
+					aws.String("vol-1a2b3c4d"),
+					aws.String("vol-0123456789abcdef2"),
 				},
 			},
 			wantErr: false,
@@ -71,7 +72,7 @@ func TestValidateDescribeVolumesInput(t *testing.T) {
 			name: "MixedValidAndInvalid",
 			input: &ec2.DescribeVolumesInput{
 				VolumeIds: []*string{
-					aws.String("vol-valid123"),
+					aws.String("vol-0123456789abcdef3"),
 					aws.String("invalid-id"),
 				},
 			},
@@ -81,7 +82,7 @@ func TestValidateDescribeVolumesInput(t *testing.T) {
 		{
 			name: "NilVolumeIdInList",
 			input: &ec2.DescribeVolumesInput{
-				VolumeIds: []*string{nil, aws.String("vol-abc123")},
+				VolumeIds: []*string{nil, aws.String("vol-0123456789abcdef1")},
 			},
 			wantErr: false,
 		},
@@ -100,3 +101,15 @@ func TestValidateDescribeVolumesInput(t *testing.T) {
 		})
 	}
 }
+
+// AWS answers a well-prefixed ID whose suffix is not 8 or 17 lowercase hex
+// characters with InvalidParameterValue, not NotFound.
+func TestValidateDescribeVolumesInput_MalformedSuffix(t *testing.T) {
+	for _, id := range []string{"vol-zzz", "vol-0123456789ABCDEF0", "vol-0123456789abcdef", "vol-"} {
+		err := ValidateDescribeVolumesInput(&ec2.DescribeVolumesInput{VolumeIds: []*string{aws.String(id)}})
+		code, msg, ok := awserrors.ResolveErrorDetail(err)
+		assert.True(t, ok, id)
+		assert.Equal(t, awserrors.ErrorInvalidParameterValue, code, id)
+		assert.Equal(t, "Value ("+id+") for parameter volumes is invalid. Expected: 'vol-...'.", msg, id)
+	}
+}

spinifex/domains/ec2/awsapi/volume/handler_test.go

@@ -96,13 +96,6 @@ func TestModifyVolume_NilNATS(t *testing.T) {
 	assert.Error(t, err)
 }
 
-func TestDescribeVolumes_ValidationErrors(t *testing.T) {
-	_, err := DescribeVolumes(context.Background(), &ec2.DescribeVolumesInput{
-		VolumeIds: []*string{aws.String("bad-id")},
-	}, nil, "")
-	assert.EqualError(t, err, awserrors.ErrorInvalidVolumeIDMalformed)
-}
-
 func TestDescribeVolumes_NilNATS(t *testing.T) {
 	_, err := DescribeVolumes(context.Background(), nil, nil, "acct-123")
 	assert.Error(t, err)