fix: permission checks for editing access keys (#18928)

With this change, only a user with `UpdateServiceAccountAdminAction`
permission is able to edit access keys.

We would like to let a user edit their own access keys, however the
feature needs to be re-designed for better security and integration with
external systems like AD/LDAP and OpenID.

This change prevents privilege escalation via service accounts.

cmd/admin-handlers-users.go

@@ -774,6 +774,16 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re
 		return
 	}
 
+	// Permission checks:
+	//
+	// 1. Any type of account (i.e. access keys (previously/still called service
+	// accounts), STS accounts, internal IDP accounts, etc) with the
+	// policy.UpdateServiceAccountAdminAction permission can update any service
+	// account.
+	//
+	// 2. We would like to let a user update their own access keys, however it
+	// is currently blocked pending a re-design. Users are still able to delete
+	// and re-create them.
 	if !globalIAMSys.IsAllowed(policy.Args{
 		AccountName:     cred.AccessKey,
 		Groups:          cred.Groups,
@@ -782,16 +792,9 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re
 		IsOwner:         owner,
 		Claims:          cred.Claims,
 	}) {
-		requestUser := cred.AccessKey
-		if cred.ParentUser != "" {
-			requestUser = cred.ParentUser
-		}
-
-		if requestUser != svcAccount.ParentUser {
 		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL)
 		return
 	}
-	}
 
 	password := cred.SecretKey
 	reqBytes, err := madmin.DecryptData(password, io.LimitReader(r.Body, r.ContentLength))

cmd/admin-handlers-users_test.go

@@ -206,6 +206,7 @@ func TestIAMInternalIDPServerSuite(t *testing.T) {
 				suite.TestCannedPolicies(c)
 				suite.TestGroupAddRemove(c)
 				suite.TestServiceAccountOpsByAdmin(c)
+				suite.TestServiceAccountPrivilegeEscalationBug(c)
 				suite.TestServiceAccountOpsByUser(c)
 				suite.TestAddServiceAccountPerms(c)
 				suite.TearDownSuite(c)
@@ -896,14 +897,6 @@ func (s *TestSuiteIAM) TestServiceAccountOpsByUser(c *check) {
 	// 3. Check S3 access
 	c.assertSvcAccS3Access(ctx, s, cr, bucket)
 
-	// 4. Check that svc account can restrict the policy, and that the
-	// session policy can be updated.
-	c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, accessKey, bucket)
-
-	// 4. Check that service account's secret key and account status can be
-	// updated.
-	c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, accessKey, bucket)
-
 	// 5. Check that service account can be deleted.
 	c.assertSvcAccDeletion(ctx, s, userAdmClient, accessKey, bucket)
 
@@ -979,6 +972,95 @@ func (s *TestSuiteIAM) TestServiceAccountOpsByAdmin(c *check) {
 	c.assertSvcAccDeletion(ctx, s, s.adm, accessKey, bucket)
 }
 
+func (s *TestSuiteIAM) TestServiceAccountPrivilegeEscalationBug(c *check) {
+	ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
+	defer cancel()
+
+	err := s.client.MakeBucket(ctx, "public", minio.MakeBucketOptions{})
+	if err != nil {
+		c.Fatalf("bucket creat error: %v", err)
+	}
+
+	err = s.client.MakeBucket(ctx, "private", minio.MakeBucketOptions{})
+	if err != nil {
+		c.Fatalf("bucket creat error: %v", err)
+	}
+
+	pubPolicyBytes := []byte(`{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Allow",
+   "Action": [
+    "s3:*"
+   ],
+   "Resource": [
+    "arn:aws:s3:::public",
+    "arn:aws:s3:::public/*"
+   ]
+  }
+ ]
+}`)
+
+	fullS3PolicyBytes := []byte(`{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": [
+        "s3:*"
+      ],
+      "Resource": [
+        "arn:aws:s3:::*"
+      ]
+    }
+  ]
+}
+`)
+
+	// Create a service account for the root user.
+	cr, err := s.adm.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
+		TargetUser: globalActiveCred.AccessKey,
+		Policy:     pubPolicyBytes,
+	})
+	if err != nil {
+		c.Fatalf("admin should be able to create service account for themselves %s", err)
+	}
+
+	svcClient := s.getUserClient(c, cr.AccessKey, cr.SecretKey, "")
+
+	// Check that the service account can access the public bucket.
+	buckets, err := svcClient.ListBuckets(ctx)
+	if err != nil {
+		c.Fatalf("err fetching buckets %s", err)
+	}
+	if len(buckets) != 1 || buckets[0].Name != "public" {
+		c.Fatalf("service account should only have access to public bucket")
+	}
+
+	// Create an madmin client with the service account creds.
+	svcAdmClient, err := madmin.NewWithOptions(s.endpoint, &madmin.Options{
+		Creds:  credentials.NewStaticV4(cr.AccessKey, cr.SecretKey, ""),
+		Secure: s.secure,
+	})
+	if err != nil {
+		c.Fatalf("Err creating svcacct admin client: %v", err)
+	}
+	svcAdmClient.SetCustomTransport(s.TestSuiteCommon.client.Transport)
+
+	// Attempt to update the policy on the service account.
+	err = svcAdmClient.UpdateServiceAccount(ctx, cr.AccessKey,
+		madmin.UpdateServiceAccountReq{
+			NewPolicy: fullS3PolicyBytes,
+		})
+
+	if err == nil {
+		c.Fatalf("service account should not be able to update policy on itself")
+	} else if !strings.Contains(err.Error(), "Access Denied") {
+		c.Fatalf("unexpected error: %v", err)
+	}
+}
+
 func (s *TestSuiteIAM) SetUpAccMgmtPlugin(c *check) {
 	ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
 	defer cancel()

cmd/auth-handler.go

@@ -162,7 +162,8 @@ func validateAdminSignature(ctx context.Context, r *http.Request, region string)
 	s3Err := ErrAccessDenied
 	if _, ok := r.Header[xhttp.AmzContentSha256]; ok &&
 		getRequestAuthType(r) == authTypeSigned {
-		// We only support admin credentials to access admin APIs.
+
+		// Get credential information from the request.
 		cred, owner, s3Err = getReqAccessKeyV4(r, region, serviceS3)
 		if s3Err != ErrNone {
 			return cred, owner, s3Err

cmd/iam.go

@@ -974,7 +974,7 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, gro
 		m[iamPolicyClaimNameSA()] = inheritedPolicyType
 	}
 
-	// Add all the necessary claims for the service accounts.
+	// Add all the necessary claims for the service account.
 	for k, v := range opts.claims {
 		_, ok := m[k]
 		if !ok {
@@ -1848,37 +1848,14 @@ func (sys *IAMSys) IsAllowedServiceAccount(args policy.Args, parentUser string)
 		return isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)
 	}
 
-	// Now check if we have a sessionPolicy.
+	// 3. If an inline session-policy is present, evaluate it.
-	spolicy, ok := args.Claims[sessionPolicyNameExtracted]
+	hasSessionPolicy, isAllowedSP := isAllowedBySessionPolicy(args)
-	if !ok {
+	if hasSessionPolicy {
-		return false
+		return isAllowedSP && (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs))
 	}
 
-	spolicyStr, ok := spolicy.(string)
+	// Sub policy not set. Evaluate only the parent policies.
-	if !ok {
+	return (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs))
-		// Sub policy if set, should be a string reject
-		// malformed/malicious requests.
-		return false
-	}
-
-	// Check if policy is parseable.
-	subPolicy, err := policy.ParseConfig(bytes.NewReader([]byte(spolicyStr)))
-	if err != nil {
-		// Log any error in input session policy config.
-		logger.LogIf(GlobalContext, err)
-		return false
-	}
-
-	// This can only happen if policy was set but with an empty JSON.
-	if subPolicy.Version == "" && len(subPolicy.Statements) == 0 {
-		return isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)
-	}
-
-	if subPolicy.Version == "" {
-		return false
-	}
-
-	return subPolicy.IsAllowed(parentArgs) && (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs))
 }
 
 // IsAllowedSTS is meant for STS based temporary credentials,
@@ -2000,8 +1977,14 @@ func isAllowedBySessionPolicy(args policy.Args) (hasSessionPolicy bool, isAllowe
 		return
 	}
 
+	// As the session policy exists, even if the parent is the root account, it
+	// must be restricted by it. So, we set `.IsOwner` to false here
+	// unconditionally.
+	sessionPolicyArgs := args
+	sessionPolicyArgs.IsOwner = false
+
 	// Sub policy is set and valid.
-	return hasSessionPolicy, subPolicy.IsAllowed(args)
+	return hasSessionPolicy, subPolicy.IsAllowed(sessionPolicyArgs)
 }
 
 // GetCombinedPolicy returns a combined policy combining all policies

cmd/signature-v4-utils.go

@@ -158,8 +158,8 @@ func checkKeyValid(r *http.Request, accessKey string) (auth.Credentials, bool, A
 		// Check if the access key is part of users credentials.
 		u, ok := globalIAMSys.GetUser(r.Context(), accessKey)
 		if !ok {
-			// Credentials will be invalid but and disabled
+			// Credentials could be valid but disabled - return a different
-			// return a different error in such a scenario.
+			// error in such a scenario.
 			if u.Credentials.Status == auth.AccountOff {
 				return cred, false, ErrAccessKeyDisabled
 			}

cmd/sts-handlers_test.go

@@ -909,14 +909,6 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccounts(c *check) {
 	// 3. Check S3 access
 	c.assertSvcAccS3Access(ctx, s, cr, bucket)
 
-	// 4. Check that svc account can restrict the policy, and that the
-	// session policy can be updated.
-	c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
-	// 4. Check that service account's secret key and account status can be
-	// updated.
-	c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
 	// 5. Check that service account can be deleted.
 	c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket)
 
@@ -1101,14 +1093,6 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccountsWithGroups(c *check) {
 	// 3. Check S3 access
 	c.assertSvcAccS3Access(ctx, s, cr, bucket)
 
-	// 4. Check that svc account can restrict the policy, and that the
-	// session policy can be updated.
-	c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
-	// 4. Check that service account's secret key and account status can be
-	// updated.
-	c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
 	// 5. Check that service account can be deleted.
 	c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket)
 
@@ -1358,14 +1342,6 @@ func (s *TestSuiteIAM) TestOpenIDServiceAcc(c *check) {
 	// 3. Check S3 access
 	c.assertSvcAccS3Access(ctx, s, cr, bucket)
 
-	// 4. Check that svc account can restrict the policy, and that the
-	// session policy can be updated.
-	c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
-	// 4. Check that service account's secret key and account status can be
-	// updated.
-	c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
 	// 5. Check that service account can be deleted.
 	c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket)
 
@@ -1658,14 +1634,6 @@ func (s *TestSuiteIAM) TestOpenIDServiceAccWithRolePolicy(c *check) {
 	// 3. Check S3 access
 	c.assertSvcAccS3Access(ctx, s, cr, bucket)
 
-	// 4. Check that svc account can restrict the policy, and that the
-	// session policy can be updated.
-	c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
-	// 4. Check that service account's secret key and account status can be
-	// updated.
-	c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
-
 	// 5. Check that service account can be deleted.
 	c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket)
 }