fix(eks): validate add-on serviceAccountRoleArn syntax

CreateAddon and UpdateAddon stored serviceAccountRoleArn without
checking it was an ARN, and the guest renders the value into the
add-on's manifests. A non-empty value must now be syntactically an IAM
role ARN (12-digit account, no region, role resource, IAM name and path
rules); anything else returns InvalidParameterException before any
record is written or manifest staged.

The syntax check is foundation/aws/arn.ValidateRoleARN. iam:PassRole at
the gateway remains the permission and existence check. Stored values
are not rewritten, and an UpdateAddon that omits the field still
succeeds on a record holding a legacy value.

spinifex/foundation/aws/arn/role_validate.go

@@ -0,0 +1,51 @@
+package arn
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+)
+
+// iamAccountIDPattern matches AWS's 12-digit account ID.
+var iamAccountIDPattern = regexp.MustCompile(`^[0-9]{12}$`)
+
+// iamRoleNamePattern matches an IAM role name: up to 64 characters from
+// [\w+=,.@-], so no whitespace, quotes, newlines or colons.
+var iamRoleNamePattern = regexp.MustCompile(`^[\w+=,.@-]{1,64}$`)
+
+// iamPathPattern matches an IAM path: "/" alone, or printable ASCII between
+// a leading and trailing "/".
+var iamPathPattern = regexp.MustCompile(`^(/|/[\x21-\x7E]+/)$`)
+
+// ValidateRoleARN reports whether s is syntactically an IAM role ARN,
+// arn:aws:iam::<12-digit account>:role<path><name>, under IAM's naming rules.
+// It checks syntax only, not that the role exists or who owns it.
+func ValidateRoleARN(s string) error {
+	service, region, account, resource, ok := Split(s)
+	if !ok {
+		return fmt.Errorf("not an arn:aws ARN")
+	}
+	if service != "iam" {
+		return fmt.Errorf("not an IAM ARN")
+	}
+	if region != "" {
+		return fmt.Errorf("an IAM ARN does not carry a region")
+	}
+	if !iamAccountIDPattern.MatchString(account) {
+		return fmt.Errorf("account ID is not a 12-digit account ID")
+	}
+	rest, ok := strings.CutPrefix(resource, string(IAMRole))
+	if !ok || !strings.HasPrefix(rest, "/") {
+		return fmt.Errorf("ARN resource is not a role")
+	}
+	lastSlash := strings.LastIndex(rest, "/")
+	name := rest[lastSlash+1:]
+	path := rest[:lastSlash+1]
+	if !iamRoleNamePattern.MatchString(name) {
+		return fmt.Errorf("role name does not satisfy IAM's naming rules")
+	}
+	if !iamPathPattern.MatchString(path) {
+		return fmt.Errorf("role path does not satisfy IAM's path rules")
+	}
+	return nil
+}

spinifex/foundation/aws/arn/role_validate_test.go

@@ -0,0 +1,61 @@
+package arn_test
+
+import (
+	"testing"
+
+	"github.com/mulgadc/spinifex/spinifex/foundation/aws/arn"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateRoleARN_AcceptsValidRoleARNs(t *testing.T) {
+	tests := []struct {
+		name string
+		arn  string
+	}{
+		{"no path", "arn:aws:iam::123456789012:role/CoreDNSRole"},
+		{"nested path", "arn:aws:iam::123456789012:role/service-role/CoreDNSRole"},
+		{"64-character name", "arn:aws:iam::123456789012:role/" + stringOfLen(64)},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.NoError(t, arn.ValidateRoleARN(tt.arn))
+		})
+	}
+}
+
+func TestValidateRoleARN_RejectsMalformedAndForeignARNs(t *testing.T) {
+	tests := []struct {
+		name string
+		arn  string
+	}{
+		{"empty string", ""},
+		{"not an ARN", "not-an-arn"},
+		{"wrong service s3", "arn:aws:s3:::bucket"},
+		{"wrong resource type user", "arn:aws:iam::123456789012:user/x"},
+		{"sts assumed-role", "arn:aws:sts::123456789012:assumed-role/role-name/session-name"},
+		{"missing account", "arn:aws:iam:::role/x"},
+		{"non-12-digit account", "arn:aws:iam::12345:role/x"},
+		{"account too long", "arn:aws:iam::1234567890123:role/x"},
+		{"region present", "arn:aws:iam:us-east-1:123456789012:role/x"},
+		{"empty name", "arn:aws:iam::123456789012:role/"},
+		{"trailing slash empty name", "arn:aws:iam::123456789012:role/path/"},
+		{"newline injection", "arn:aws:iam::123456789012:role/x\nkind: Secret"},
+		{"quote injection", "arn:aws:iam::123456789012:role/x\""},
+		{"space injection", "arn:aws:iam::123456789012:role/x y"},
+		{"colon injection", "arn:aws:iam::123456789012:role/x:y"},
+		{"65-character name", "arn:aws:iam::123456789012:role/" + stringOfLen(65)},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Error(t, arn.ValidateRoleARN(tt.arn))
+		})
+	}
+}
+
+func stringOfLen(n int) string {
+	b := make([]byte, n)
+	for i := range b {
+		b[i] = 'a'
+	}
+	return string(b)
+}

spinifex/handlers/eks/addons.go

@@ -9,6 +9,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/eks"
 	eksv1 "github.com/mulgadc/spinifex/contracts/eks/v1"
 	"github.com/mulgadc/spinifex/spinifex/domains/eks/addon"
+	"github.com/mulgadc/spinifex/spinifex/foundation/aws/arn"
 	"github.com/mulgadc/spinifex/spinifex/foundation/aws/errors"
 	"github.com/nats-io/nats.go/jetstream"
 )
@@ -56,6 +57,20 @@ func (s *EKSServiceImpl) DescribeAddonVersions(ctx context.Context, input *eks.D
 	return &eks.DescribeAddonVersionsOutput{Addons: out}, nil
 }
 
+// validateAddonServiceAccountRoleArn rejects a non-empty value that is not
+// syntactically an IAM role ARN. iam:PassRole at the gateway remains the
+// permission and existence check.
+func validateAddonServiceAccountRoleArn(roleArn string) error {
+	if roleArn == "" {
+		return nil
+	}
+	if err := arn.ValidateRoleARN(roleArn); err != nil {
+		return awserrors.Errorf(awserrors.ErrorEKSInvalidParameter,
+			"1 validation error detected: Value at 'serviceAccountRoleArn' failed to satisfy constraint: Member must be a valid IAM role ARN")
+	}
+	return nil
+}
+
 // CreateAddon validates, persists a CREATING record, and stages it for delivery.
 // Transitions to ACTIVE once the cluster state report confirms delivery.
 func (s *EKSServiceImpl) CreateAddon(ctx context.Context, input *eks.CreateAddonInput, accountID string) (*eks.CreateAddonOutput, error) {
@@ -67,6 +82,9 @@ func (s *EKSServiceImpl) CreateAddon(ctx context.Context, input *eks.CreateAddon
 	if addonName == "" {
 		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
 	}
+	if err := validateAddonServiceAccountRoleArn(aws.StringValue(input.ServiceAccountRoleArn)); err != nil {
+		return nil, err
+	}
 	spec, ok := addon.Lookup(addonName)
 	if !ok {
 		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
@@ -164,6 +182,11 @@ func (s *EKSServiceImpl) UpdateAddon(ctx context.Context, input *eks.UpdateAddon
 	if input == nil {
 		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
 	}
+	if input.ServiceAccountRoleArn != nil {
+		if err := validateAddonServiceAccountRoleArn(*input.ServiceAccountRoleArn); err != nil {
+			return nil, err
+		}
+	}
 	cluster := aws.StringValue(input.ClusterName)
 	addonName := aws.StringValue(input.AddonName)
 	acctKV, err := s.acctKVForCluster(ctx, accountID, cluster)

spinifex/handlers/eks/addons_test.go

@@ -309,5 +309,150 @@ func TestListStagedAddonManifests(t *testing.T) {
 	require.EqualError(t, err, awserrors.ErrorInvalidParameterValue)
 }
 
+func TestCreateAddon_RejectsMalformedServiceAccountRoleArn(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name string
+		arn  string
+	}{
+		{"trailing slash empty name", "arn:aws:iam::123456789012:role/"},
+		{"newline and colon injection", "arn:aws:iam::123456789012:role/x\nkind: Secret"},
+		{"quote and space injection", "arn:aws:iam::123456789012:role/x\" y"},
+		{"missing account", "arn:aws:iam:::role/x"},
+		{"non-12-digit account", "arn:aws:iam::12345:role/x"},
+		{"wrong service s3", "arn:aws:s3:::bucket"},
+		{"wrong resource type iam user", "arn:aws:iam::123456789012:user/x"},
+		{"sts assumed-role", "arn:aws:sts::123456789012:assumed-role/role-name/session-name"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			svc, fake := setupAddonService(t)
+			_, err := svc.CreateAddon(context.Background(), &eks.CreateAddonInput{
+				ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+				ServiceAccountRoleArn: aws.String(tt.arn),
+			}, testAccountID)
+			require.Error(t, err)
+			assert.Equal(t, awserrors.ErrorEKSInvalidParameter, awserrors.ValidErrorCodeFromError(err))
+			assert.Empty(t, fake.installs, "a rejected create must not stage anything")
+
+			_, descErr := svc.DescribeAddon(context.Background(), &eks.DescribeAddonInput{
+				ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+			}, testAccountID)
+			require.EqualError(t, descErr, awserrors.ErrorEKSResourceNotFound, "a rejected create must not persist a record")
+		})
+	}
+}
+
+func TestCreateAddon_AcceptsValidServiceAccountRoleArnAndAbsentValue(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name string
+		arn  *string
+	}{
+		{"role without a path", aws.String("arn:aws:iam::123456789012:role/alb")},
+		{"role with a path", aws.String("arn:aws:iam::123456789012:role/service-role/alb")},
+		{"absent value", nil},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			svc, _ := setupAddonService(t)
+			out, err := svc.CreateAddon(context.Background(), &eks.CreateAddonInput{
+				ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+				ServiceAccountRoleArn: tt.arn,
+			}, testAccountID)
+			require.NoError(t, err)
+			require.NotNil(t, out.Addon)
+		})
+	}
+}
+
+func TestUpdateAddon_RejectsMalformedServiceAccountRoleArn(t *testing.T) {
+	t.Parallel()
+	svc, fake := setupAddonService(t)
+	_, err := svc.CreateAddon(context.Background(), &eks.CreateAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+		ServiceAccountRoleArn: aws.String("arn:aws:iam::123456789012:role/alb"),
+	}, testAccountID)
+	require.NoError(t, err)
+	installsBefore := len(fake.installs)
+
+	_, err = svc.UpdateAddon(context.Background(), &eks.UpdateAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+		ServiceAccountRoleArn: aws.String("arn:aws:iam::123456789012:role/x\nkind: Secret"),
+	}, testAccountID)
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorEKSInvalidParameter, awserrors.ValidErrorCodeFromError(err))
+	assert.Len(t, fake.installs, installsBefore, "a rejected update must not re-stage")
+
+	desc, err := svc.DescribeAddon(context.Background(), &eks.DescribeAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Equal(t, "arn:aws:iam::123456789012:role/alb", aws.StringValue(desc.Addon.ServiceAccountRoleArn),
+		"a rejected update must not change the stored value")
+}
+
+func TestUpdateAddon_AcceptsValidServiceAccountRoleArn(t *testing.T) {
+	t.Parallel()
+	svc, _ := setupAddonService(t)
+	_, err := svc.CreateAddon(context.Background(), &eks.CreateAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+	}, testAccountID)
+	require.NoError(t, err)
+
+	_, err = svc.UpdateAddon(context.Background(), &eks.UpdateAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+		ServiceAccountRoleArn: aws.String("arn:aws:iam::123456789012:role/service-role/alb"),
+	}, testAccountID)
+	require.NoError(t, err)
+
+	desc, err := svc.DescribeAddon(context.Background(), &eks.DescribeAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Equal(t, "arn:aws:iam::123456789012:role/service-role/alb", aws.StringValue(desc.Addon.ServiceAccountRoleArn))
+}
+
+// A record written before this check existed may hold a value that is not a
+// well-formed ARN. UpdateAddon only validates the value supplied in the
+// request, so an update that omits serviceAccountRoleArn must still succeed
+// and must not touch the legacy stored value.
+func TestUpdateAddon_LeavesLegacyNonARNValueUntouchedWhenOmitted(t *testing.T) {
+	t.Parallel()
+	svc, fake := setupAddonService(t)
+	acctKV, err := svc.acctKVForCluster(context.Background(), testAccountID, "c1")
+	require.NoError(t, err)
+	_, err = svc.addons().Create(context.Background(), acctKV, testAccountID, addon.Desired{
+		Cluster:               "c1",
+		Name:                  albController,
+		Version:               "1.0",
+		ServiceAccountRoleArn: "legacy-not-an-arn",
+	})
+	require.NoError(t, err)
+
+	desc, err := svc.DescribeAddon(context.Background(), &eks.DescribeAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+	}, testAccountID)
+	require.NoError(t, err)
+	require.Equal(t, "legacy-not-an-arn", aws.StringValue(desc.Addon.ServiceAccountRoleArn))
+
+	_, err = svc.UpdateAddon(context.Background(), &eks.UpdateAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+		ConfigurationValues: aws.String(`{"replicaCount":2}`),
+	}, testAccountID)
+	require.NoError(t, err, "an update omitting serviceAccountRoleArn must succeed on a legacy non-ARN record")
+	require.Len(t, fake.installs, 2, "create plus the successful update re-stage")
+
+	desc, err = svc.DescribeAddon(context.Background(), &eks.DescribeAddonInput{
+		ClusterName: aws.String("c1"), AddonName: aws.String(albController),
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Equal(t, "legacy-not-an-arn", aws.StringValue(desc.Addon.ServiceAccountRoleArn),
+		"the legacy value must survive an update that does not supply the field")
+	assert.Equal(t, `{"replicaCount":2}`, aws.StringValue(desc.Addon.ConfigurationValues))
+}
+
 // Not parallel: it swaps the package-wide catalogue check every service the
 // parallel tests construct runs.