Fix role ARN comparison for user ID strict check (#669) (#671)

kubernetes-sigs · Dec 9, 2023 · e3aaa0e · e3aaa0e
1 parent b978afa
commit e3aaa0e
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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,13 +491,60 @@ 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",
 				Groups:      []string{"asdf"},
 			},
 			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
@@ -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
 		}
 	}