diff --git a/spinifex/foundation/aws/arn/role_validate.go b/spinifex/foundation/aws/arn/role_validate.go new file mode 100644 index 000000000..095b4c4db --- /dev/null +++ b/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, 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 +} diff --git a/spinifex/foundation/aws/arn/role_validate_test.go b/spinifex/foundation/aws/arn/role_validate_test.go new file mode 100644 index 000000000..aff4b4002 --- /dev/null +++ b/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) +} diff --git a/spinifex/handlers/eks/addons.go b/spinifex/handlers/eks/addons.go index 19d8ab295..55cc7bc08 100644 --- a/spinifex/handlers/eks/addons.go +++ 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) diff --git a/spinifex/handlers/eks/addons_test.go b/spinifex/handlers/eks/addons_test.go index 2691f7f80..50d82bcb3 100644 --- a/spinifex/handlers/eks/addons_test.go +++ b/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.