From dbcce7f8fbf386bbf105451db23807a1459328b8 Mon Sep 17 00:00:00 2001 From: Nicholas Turner <1205393+nckturner@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:32:28 -0800 Subject: [PATCH] Check ARN for user ID strict check (#660) --- pkg/config/mapper.go | 25 ++++ pkg/errutil/errors.go | 9 ++ pkg/mapper/configmap/mapper.go | 6 +- pkg/mapper/crd/mapper.go | 6 +- pkg/mapper/dynamicfile/dynamicfile.go | 20 ++-- pkg/mapper/dynamicfile/dynamicfile_test.go | 133 +++++++++++++++++++-- pkg/mapper/dynamicfile/mapper.go | 41 ++++--- pkg/mapper/file/mapper.go | 4 +- pkg/mapper/mapper.go | 4 +- pkg/server/server.go | 5 +- 10 files changed, 201 insertions(+), 52 deletions(-) create mode 100644 pkg/config/mapper.go create mode 100644 pkg/errutil/errors.go diff --git a/pkg/config/mapper.go b/pkg/config/mapper.go new file mode 100644 index 000000000..9b9c0940c --- /dev/null +++ b/pkg/config/mapper.go @@ -0,0 +1,25 @@ +package config + +import ( + "strings" + + "sigs.k8s.io/aws-iam-authenticator/pkg/token" +) + +// IdentityMapping converts the RoleMapping into a generic IdentityMapping object +func (m *RoleMapping) IdentityMapping(identity *token.Identity) *IdentityMapping { + return &IdentityMapping{ + IdentityARN: strings.ToLower(identity.CanonicalARN), + Username: m.Username, + Groups: m.Groups, + } +} + +// IdentityMapping converts the UserMapping into a generic IdentityMapping object +func (m *UserMapping) IdentityMapping(identity *token.Identity) *IdentityMapping { + return &IdentityMapping{ + IdentityARN: strings.ToLower(identity.CanonicalARN), + Username: m.Username, + Groups: m.Groups, + } +} diff --git a/pkg/errutil/errors.go b/pkg/errutil/errors.go new file mode 100644 index 000000000..f777566ec --- /dev/null +++ b/pkg/errutil/errors.go @@ -0,0 +1,9 @@ +package errutil + +import ( + "errors" +) + +var ErrNotMapped = errors.New("Identity is not mapped") + +var ErrIDAndARNMismatch = errors.New("ARN does not match User ID") diff --git a/pkg/mapper/configmap/mapper.go b/pkg/mapper/configmap/mapper.go index a1f84e894..b27607c92 100644 --- a/pkg/mapper/configmap/mapper.go +++ b/pkg/mapper/configmap/mapper.go @@ -1,9 +1,11 @@ package configmap import ( - "sigs.k8s.io/aws-iam-authenticator/pkg/token" "strings" + "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" + "sigs.k8s.io/aws-iam-authenticator/pkg/token" + "sigs.k8s.io/aws-iam-authenticator/pkg/config" "sigs.k8s.io/aws-iam-authenticator/pkg/mapper" ) @@ -53,7 +55,7 @@ func (m *ConfigMapMapper) Map(identity *token.Identity) (*config.IdentityMapping }, nil } - return nil, mapper.ErrNotMapped + return nil, errutil.ErrNotMapped } func (m *ConfigMapMapper) IsAccountAllowed(accountID string) bool { diff --git a/pkg/mapper/crd/mapper.go b/pkg/mapper/crd/mapper.go index bdf184781..6eaef399e 100644 --- a/pkg/mapper/crd/mapper.go +++ b/pkg/mapper/crd/mapper.go @@ -2,10 +2,12 @@ package crd import ( "fmt" - "sigs.k8s.io/aws-iam-authenticator/pkg/token" "strings" "time" + "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" + "sigs.k8s.io/aws-iam-authenticator/pkg/token" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" @@ -114,7 +116,7 @@ func (m *CRDMapper) Map(identity *token.Identity) (*config.IdentityMapping, erro } } - return nil, mapper.ErrNotMapped + return nil, errutil.ErrNotMapped } func (m *CRDMapper) IsAccountAllowed(accountID string) bool { diff --git a/pkg/mapper/dynamicfile/dynamicfile.go b/pkg/mapper/dynamicfile/dynamicfile.go index c63dc9261..4a4f3c6c9 100644 --- a/pkg/mapper/dynamicfile/dynamicfile.go +++ b/pkg/mapper/dynamicfile/dynamicfile.go @@ -2,7 +2,6 @@ package dynamicfile import ( "encoding/json" - "errors" "fmt" "strings" "sync" @@ -10,6 +9,7 @@ import ( "github.com/sirupsen/logrus" "sigs.k8s.io/aws-iam-authenticator/pkg/arn" "sigs.k8s.io/aws-iam-authenticator/pkg/config" + "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" ) type DynamicFileMapStore struct { @@ -81,27 +81,21 @@ func (ms *DynamicFileMapStore) saveMap( } } -// UserNotFound is the error returned when the user is not found in the config map. -var UserNotFound = errors.New("User not found in dynamic file") - -// RoleNotFound is the error returned when the role is not found in the config map. -var RoleNotFound = errors.New("Role not found in dynamic file") - -func (ms *DynamicFileMapStore) UserMapping(arn string) (config.UserMapping, error) { +func (ms *DynamicFileMapStore) UserMapping(key string) (config.UserMapping, error) { ms.mutex.RLock() defer ms.mutex.RUnlock() - if user, ok := ms.users[arn]; !ok { - return config.UserMapping{}, UserNotFound + if user, ok := ms.users[key]; !ok { + return config.UserMapping{}, errutil.ErrNotMapped } else { return user, nil } } -func (ms *DynamicFileMapStore) RoleMapping(arn string) (config.RoleMapping, error) { +func (ms *DynamicFileMapStore) RoleMapping(key string) (config.RoleMapping, error) { ms.mutex.RLock() defer ms.mutex.RUnlock() - if role, ok := ms.roles[arn]; !ok { - return config.RoleMapping{}, RoleNotFound + if role, ok := ms.roles[key]; !ok { + return config.RoleMapping{}, errutil.ErrNotMapped } else { return role, nil } diff --git a/pkg/mapper/dynamicfile/dynamicfile_test.go b/pkg/mapper/dynamicfile/dynamicfile_test.go index 9c1df591d..2ed993476 100644 --- a/pkg/mapper/dynamicfile/dynamicfile_test.go +++ b/pkg/mapper/dynamicfile/dynamicfile_test.go @@ -6,8 +6,11 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "sigs.k8s.io/aws-iam-authenticator/pkg/config" + "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" "sigs.k8s.io/aws-iam-authenticator/pkg/fileutil" + "sigs.k8s.io/aws-iam-authenticator/pkg/token" ) var ( @@ -15,21 +18,36 @@ var ( testRole = config.RoleMapping{RoleARN: "arn:aws:iam::012345678912:role/computer", Username: "computer", Groups: []string{"system:nodes"}} ) -func makeStore() DynamicFileMapStore { +func makeStore(users map[string]config.UserMapping, roles map[string]config.RoleMapping, filename string, userIDStrict bool) DynamicFileMapStore { ms := DynamicFileMapStore{ - users: make(map[string]config.UserMapping), - roles: make(map[string]config.RoleMapping), - awsAccounts: make(map[string]interface{}), - filename: "test.txt", + users: users, + roles: roles, + awsAccounts: make(map[string]interface{}), + filename: filename, + userIDStrict: userIDStrict, } - ms.users["arn:aws:iam::012345678912:user/matt"] = testUser - ms.roles["UserId001"] = testRole + ms.awsAccounts["123"] = nil return ms } +func makeDefaultStore() DynamicFileMapStore { + users := make(map[string]config.UserMapping) + roles := make(map[string]config.RoleMapping) + users["arn:aws:iam::012345678912:user/matt"] = testUser + roles["UserId001"] = testRole + return makeStore(users, roles, "test.txt", false) +} + +func makeMapper(users map[string]config.UserMapping, roles map[string]config.RoleMapping, filename string, userIDStrict bool) *DynamicFileMapper { + store := makeStore(users, roles, filename, userIDStrict) + return &DynamicFileMapper{ + DynamicFileMapStore: &store, + } +} + func TestUserMapping(t *testing.T) { - ms := makeStore() + ms := makeDefaultStore() user, err := ms.UserMapping("arn:aws:iam::012345678912:user/matt") if err != nil { t.Errorf("Could not find user 'matt' in map") @@ -39,7 +57,7 @@ func TestUserMapping(t *testing.T) { } user, err = ms.UserMapping("nic") - if err != UserNotFound { + if err != errutil.ErrNotMapped { t.Errorf("UserNotFound error was not returned for user 'nic'") } if !reflect.DeepEqual(user, config.UserMapping{}) { @@ -48,7 +66,7 @@ func TestUserMapping(t *testing.T) { } func TestRoleMapping(t *testing.T) { - ms := makeStore() + ms := makeDefaultStore() role, err := ms.RoleMapping("UserId001") if err != nil { t.Errorf("Could not find user 'instance in map") @@ -58,7 +76,7 @@ func TestRoleMapping(t *testing.T) { } role, err = ms.RoleMapping("borg") - if err != RoleNotFound { + if err != errutil.ErrNotMapped { t.Errorf("RoleNotFound error was not returend for role 'borg'") } if !reflect.DeepEqual(role, config.RoleMapping{}) { @@ -67,7 +85,7 @@ func TestRoleMapping(t *testing.T) { } func TestAWSAccount(t *testing.T) { - ms := makeStore() + ms := makeDefaultStore() if !ms.AWSAccount("123") { t.Errorf("Expected aws account '123' to be in accounts list: %v", ms.awsAccounts) } @@ -447,3 +465,94 @@ func TestCallBackForFileLoad(t *testing.T) { } } } + +func TestMap(t *testing.T) { + + tests := []struct { + description string + identity *token.Identity + users map[string]config.UserMapping + expectedIDMapping *config.IdentityMapping + expectedError error + }{ + { + description: "UserID strict: ARNs match.", + identity: &token.Identity{ + ARN: "arn:aws:iam::012345678912:user/matt", + CanonicalARN: "arn:aws:iam::012345678912:user/matt", + UserID: "1234", + }, + users: map[string]config.UserMapping{ + "1234": { + UserARN: "arn:aws:iam::012345678912:user/matt", + UserId: "1234", + Username: "asdf", + Groups: []string{"asdf"}, + }, + }, + expectedIDMapping: &config.IdentityMapping{ + IdentityARN: "arn:aws:iam::012345678912:user/matt", + Username: "asdf", + Groups: []string{"asdf"}, + }, + expectedError: nil, + }, + { + description: "UserID strict: ARNs do not match but UserIDs do.", + identity: &token.Identity{ + ARN: "arn:aws:iam::012345678912:user/alice", + CanonicalARN: "arn:aws:iam::012345678912:user/alice", + UserID: "1234", + }, + users: map[string]config.UserMapping{ + "1234": { + UserARN: "arn:aws:iam::012345678912:user/bob", + UserId: "1234", + }, + }, + expectedIDMapping: nil, + expectedError: errutil.ErrIDAndARNMismatch, + }, + { + description: "UserID strict: No ARN provided.", + identity: &token.Identity{ + ARN: "arn:aws:iam::012345678912:user/matt", + CanonicalARN: "arn:aws:iam::012345678912:user/matt", + UserID: "1234", + }, + users: map[string]config.UserMapping{ + "1234": { + UserId: "1234", + Username: "asdf", + Groups: []string{"asdf"}, + }, + }, + expectedIDMapping: &config.IdentityMapping{ + IdentityARN: "arn:aws:iam::012345678912:user/matt", + Username: "asdf", + Groups: []string{"asdf"}, + }, + expectedError: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.description, func(t *testing.T) { + + mapper := makeMapper(tc.users, map[string]config.RoleMapping{}, "test.txt", true) + identityMapping, err := mapper.Map(tc.identity) + + if tc.expectedError != nil { + if err == nil { + t.Errorf("expected error %v but didn't get one", tc.expectedError) + } else if err != tc.expectedError { + t.Errorf("expected error %v but got %v", tc.expectedError, 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 dd2090d07..2ccb6d5ff 100644 --- a/pkg/mapper/dynamicfile/mapper.go +++ b/pkg/mapper/dynamicfile/mapper.go @@ -4,6 +4,7 @@ import ( "strings" "sigs.k8s.io/aws-iam-authenticator/pkg/config" + "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" "sigs.k8s.io/aws-iam-authenticator/pkg/fileutil" "sigs.k8s.io/aws-iam-authenticator/pkg/mapper" "sigs.k8s.io/aws-iam-authenticator/pkg/token" @@ -37,31 +38,39 @@ func (m *DynamicFileMapper) Start(stopCh <-chan struct{}) error { func (m *DynamicFileMapper) Map(identity *token.Identity) (*config.IdentityMapping, error) { canonicalARN := strings.ToLower(identity.CanonicalARN) + key := canonicalARN if m.userIDStrict { key = identity.UserID } - rm, err := m.RoleMapping(key) - // TODO: Check for non Role/UserNotFound errors - if err == nil { - return &config.IdentityMapping{ - IdentityARN: canonicalARN, - Username: rm.Username, - Groups: rm.Groups, - }, nil + if roleMapping, err := m.RoleMapping(key); err == nil { + if err := m.match(identity, roleMapping.RoleARN, roleMapping.UserId); err != nil { + return nil, err + } + return roleMapping.IdentityMapping(identity), nil } - um, err := m.UserMapping(key) - if err == nil { - return &config.IdentityMapping{ - IdentityARN: canonicalARN, - Username: um.Username, - Groups: um.Groups, - }, nil + if userMapping, err := m.UserMapping(key); err == nil { + if err := m.match(identity, userMapping.UserARN, userMapping.UserId); err != nil { + return nil, err + } + return userMapping.IdentityMapping(identity), nil } - return nil, mapper.ErrNotMapped + return nil, errutil.ErrNotMapped +} + +func (m *DynamicFileMapper) match(token *token.Identity, mappedARN, mappedUserID string) error { + if m.userIDStrict { + // 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 { + return errutil.ErrIDAndARNMismatch + } + } + return nil } func (m *DynamicFileMapper) IsAccountAllowed(accountID string) bool { diff --git a/pkg/mapper/file/mapper.go b/pkg/mapper/file/mapper.go index c4fc0e61e..ffe64ce12 100644 --- a/pkg/mapper/file/mapper.go +++ b/pkg/mapper/file/mapper.go @@ -6,6 +6,7 @@ import ( "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/mapper" "sigs.k8s.io/aws-iam-authenticator/pkg/token" ) @@ -86,8 +87,7 @@ func (m *FileMapper) Map(identity *token.Identity) (*config.IdentityMapping, err Groups: userMapping.Groups, }, nil } - - return nil, mapper.ErrNotMapped + return nil, errutil.ErrNotMapped } func (m *FileMapper) IsAccountAllowed(accountID string) bool { diff --git a/pkg/mapper/mapper.go b/pkg/mapper/mapper.go index 0afa0ab35..3b10462e4 100644 --- a/pkg/mapper/mapper.go +++ b/pkg/mapper/mapper.go @@ -1,8 +1,8 @@ package mapper import ( - "errors" "fmt" + "sigs.k8s.io/aws-iam-authenticator/pkg/token" "github.com/sirupsen/logrus" @@ -34,8 +34,6 @@ var ( BackendModeChoices = []string{ModeMountedFile, ModeEKSConfigMap, ModeCRD, ModeDynamicFile} ) -var ErrNotMapped = errors.New("ARN is not mapped") - type Mapper interface { Name() string // Start must be non-blocking diff --git a/pkg/server/server.go b/pkg/server/server.go index 9939cc4df..d15890268 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -32,6 +32,7 @@ import ( "github.com/aws/aws-sdk-go/aws/session" "sigs.k8s.io/aws-iam-authenticator/pkg/config" "sigs.k8s.io/aws-iam-authenticator/pkg/ec2provider" + "sigs.k8s.io/aws-iam-authenticator/pkg/errutil" "sigs.k8s.io/aws-iam-authenticator/pkg/fileutil" "sigs.k8s.io/aws-iam-authenticator/pkg/mapper" "sigs.k8s.io/aws-iam-authenticator/pkg/mapper/configmap" @@ -417,7 +418,7 @@ func (h *handler) doMapping(identity *token.Identity) (string, []string, error) } return username, groups, nil } else { - if err != mapper.ErrNotMapped { + if err != errutil.ErrNotMapped { errs = append(errs, fmt.Errorf("mapper %s Map error: %v", m.Name(), err)) } @@ -430,7 +431,7 @@ func (h *handler) doMapping(identity *token.Identity) (string, []string, error) if len(errs) > 0 { return "", nil, utilerrors.NewAggregate(errs) } - return "", nil, mapper.ErrNotMapped + return "", nil, errutil.ErrNotMapped } func (h *handler) renderTemplates(mapping config.IdentityMapping, identity *token.Identity) (string, []string, error) {