From e91d159bb339763102afc42169de058ed4b4f3c1 Mon Sep 17 00:00:00 2001 From: Daniel Kennedy <19178580+DanielCKennedy@users.noreply.github.com> Date: Fri, 8 Dec 2023 19:10:38 -0800 Subject: [PATCH] Fix role ARN comparison for user ID strict check --- pkg/arn/arn.go | 27 ++++++++++ pkg/arn/arn_test.go | 25 ++++++++++ pkg/mapper/dynamicfile/dynamicfile_test.go | 58 +++++++++++++++++++++- pkg/mapper/dynamicfile/mapper.go | 4 +- 4 files changed, 111 insertions(+), 3 deletions(-) diff --git a/pkg/arn/arn.go b/pkg/arn/arn.go index 609b789ba..e9b73b587 100644 --- a/pkg/arn/arn.go +++ b/pkg/arn/arn.go @@ -73,6 +73,33 @@ func Canonicalize(arn string) (PrincipalType, string, error) { return NONE, "", fmt.Errorf("service %s in arn %s is not a valid service for identities", parsed.Service, arn) } +// TODO: add strip path functionality Canonicalize after testing it in all mappers - this can be used to support role paths in the configmap +func StripPath(arn string) (string, error) { + parsed, err := awsarn.Parse(arn) + if err != nil { + return "", fmt.Errorf("arn '%s' is invalid: '%v'", arn, err) + } + + if err := checkPartition(parsed.Partition); err != nil { + return "", fmt.Errorf("arn '%s' does not have a recognized partition", arn) + } + + parts := strings.Split(parsed.Resource, "/") + resource := parts[0] + + if resource != "role" { + return arn, nil + } + + if len(parts) > 2 { + // Stripping off the path means we just need to keep the first and last part of the arn resource + // role/path/for/this-role/matt -> role/matt + role := parts[len(parts)-1] + return fmt.Sprintf("arn:%s:iam::%s:role/%s", parsed.Partition, parsed.AccountID, role), nil + } + return arn, nil +} + func checkPartition(partition string) error { for _, p := range endpoints.DefaultPartitions() { if partition == p.ID() { diff --git a/pkg/arn/arn_test.go b/pkg/arn/arn_test.go index 708b3e943..fcec2814b 100644 --- a/pkg/arn/arn_test.go +++ b/pkg/arn/arn_test.go @@ -33,3 +33,28 @@ func TestUserARN(t *testing.T) { } } } + +var arnStripTests = []struct { + arn string // input arn + expected string // canonacalized arn + err error // expected error value +}{ + {"NOT AN ARN", "", fmt.Errorf("Not an arn")}, + {"arn:aws:iam::123456789012:role/Org/Team/Admin", "arn:aws:iam::123456789012:role/Admin", nil}, + {"arn:aws:iam::123456789012:role/Admin", "arn:aws:iam::123456789012:role/Admin", nil}, + {"arn:aws:iam::123456789012:user/Alice", "arn:aws:iam::123456789012:user/Alice", nil}, + {"arn:aws:sts::123456789012:federated-user/Bob", "arn:aws:sts::123456789012:federated-user/Bob", nil}, +} + +func TestStripPath(t *testing.T) { + for _, tc := range arnStripTests { + actual, err := StripPath(tc.arn) + if err != nil && tc.err == nil || err == nil && tc.err != nil { + t.Errorf("stripPath(%s) expected err: %v, actual err: %v", tc.arn, tc.err, err) + continue + } + if actual != tc.expected { + t.Errorf("stripPath(%s) expected: %s, actual: %s", tc.arn, tc.expected, actual) + } + } +} diff --git a/pkg/mapper/dynamicfile/dynamicfile_test.go b/pkg/mapper/dynamicfile/dynamicfile_test.go index 2ed993476..328b5bfac 100644 --- a/pkg/mapper/dynamicfile/dynamicfile_test.go +++ b/pkg/mapper/dynamicfile/dynamicfile_test.go @@ -472,11 +472,12 @@ func TestMap(t *testing.T) { description string identity *token.Identity users map[string]config.UserMapping + roles map[string]config.RoleMapping expectedIDMapping *config.IdentityMapping expectedError error }{ { - description: "UserID strict: ARNs match.", + description: "UserID strict: ARNs match for user.", identity: &token.Identity{ ARN: "arn:aws:iam::012345678912:user/matt", CanonicalARN: "arn:aws:iam::012345678912:user/matt", @@ -490,6 +491,7 @@ func TestMap(t *testing.T) { Groups: []string{"asdf"}, }, }, + roles: map[string]config.RoleMapping{}, expectedIDMapping: &config.IdentityMapping{ IdentityARN: "arn:aws:iam::012345678912:user/matt", Username: "asdf", @@ -497,6 +499,52 @@ func TestMap(t *testing.T) { }, expectedError: nil, }, + { + description: "UserID strict: ARNs match for role.", + identity: &token.Identity{ + ARN: "arn:aws:sts::012345678912:assumed-role/matt/extra", + CanonicalARN: "arn:aws:iam::012345678912:role/matt", + UserID: "1234", + }, + users: map[string]config.UserMapping{}, + roles: map[string]config.RoleMapping{ + "1234": { + RoleARN: "arn:aws:iam::012345678912:role/matt", + UserId: "1234", + Username: "asdf", + Groups: []string{"asdf"}, + }, + }, + expectedIDMapping: &config.IdentityMapping{ + IdentityARN: "arn:aws:iam::012345678912:role/matt", + Username: "asdf", + Groups: []string{"asdf"}, + }, + expectedError: nil, + }, + { + description: "UserID strict: ARNs match for role with path.", + identity: &token.Identity{ + ARN: "arn:aws:sts::012345678912:assumed-role/matt/extra", + CanonicalARN: "arn:aws:iam::012345678912:role/matt", + UserID: "1234", + }, + users: map[string]config.UserMapping{}, + roles: map[string]config.RoleMapping{ + "1234": { + RoleARN: "arn:aws:iam::012345678912:role/path/for/this-role/matt", + UserId: "1234", + Username: "asdf", + Groups: []string{"asdf"}, + }, + }, + expectedIDMapping: &config.IdentityMapping{ + IdentityARN: "arn:aws:iam::012345678912:role/matt", + Username: "asdf", + Groups: []string{"asdf"}, + }, + expectedError: nil, + }, { description: "UserID strict: ARNs do not match but UserIDs do.", identity: &token.Identity{ @@ -510,6 +558,7 @@ func TestMap(t *testing.T) { UserId: "1234", }, }, + roles: map[string]config.RoleMapping{}, expectedIDMapping: nil, expectedError: errutil.ErrIDAndARNMismatch, }, @@ -527,6 +576,7 @@ func TestMap(t *testing.T) { Groups: []string{"asdf"}, }, }, + roles: map[string]config.RoleMapping{}, expectedIDMapping: &config.IdentityMapping{ IdentityARN: "arn:aws:iam::012345678912:user/matt", Username: "asdf", @@ -539,7 +589,7 @@ func TestMap(t *testing.T) { for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - mapper := makeMapper(tc.users, map[string]config.RoleMapping{}, "test.txt", true) + mapper := makeMapper(tc.users, tc.roles, "test.txt", true) identityMapping, err := mapper.Map(tc.identity) if tc.expectedError != nil { @@ -550,6 +600,10 @@ func TestMap(t *testing.T) { } } + if tc.expectedError == nil && err != nil { + t.Errorf("unexpected error: %v", err) + } + if diff := cmp.Diff(tc.expectedIDMapping, identityMapping); diff != "" { t.Errorf("Result mismatch (-want +got):\n%s", diff) } diff --git a/pkg/mapper/dynamicfile/mapper.go b/pkg/mapper/dynamicfile/mapper.go index 2ccb6d5ff..d11cda4fc 100644 --- a/pkg/mapper/dynamicfile/mapper.go +++ b/pkg/mapper/dynamicfile/mapper.go @@ -3,6 +3,7 @@ package dynamicfile import ( "strings" + "sigs.k8s.io/aws-iam-authenticator/pkg/arn" "sigs.k8s.io/aws-iam-authenticator/pkg/config" "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" "sigs.k8s.io/aws-iam-authenticator/pkg/fileutil" @@ -66,7 +67,8 @@ func (m *DynamicFileMapper) match(token *token.Identity, mappedARN, mappedUserID // If ARN is provided, ARN must be validated along with UserID. This avoids having to // support IAM user name/ARN changes. Without preventing this the mapping would look // invalid but still work and auditing would be difficult/impossible. - if mappedARN != "" && token.ARN != mappedARN { + strippedArn, _ := arn.StripPath(mappedARN) + if strippedArn != "" && token.CanonicalARN != strings.ToLower(strippedArn) { return errutil.ErrIDAndARNMismatch } }