Check ARN for user ID strict check (kubernetes-sigs#660)

nckturner · Dec 7, 2023 · dbcce7f · dbcce7f
1 parent a8ee0d9
commit dbcce7f
Show file tree

Hide file tree

Showing 10 changed files with 201 additions and 52 deletions.
diff --git a/pkg/config/mapper.go 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
@@ -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
@@ -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
@@ -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
@@ -2,14 +2,14 @@ package dynamicfile
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"strings"
 	"sync"
 
 	"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
@@ -6,30 +6,48 @@ 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 (
 	testUser = config.UserMapping{UserARN: "arn:aws:iam::012345678912:user/matt", Username: "matlan", Groups: []string{"system:master", "dev"}}
 	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
@@ -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 {