Fix regression in STS permissions via group in internal IDP (#13955)

- When using MinIO's internal IDP, STS credential permissions did not check the
groups of a user.

- Also fix bug in policy checking in AccountInfo call

cmd/admin-handlers-users.go

@@ -1107,19 +1107,11 @@ func (a adminAPIHandlers) AccountInfoHandler(w http.ResponseWriter, r *http.Requ
 	}
 
 	accountName := cred.AccessKey
-	var policies []string
-	switch globalIAMSys.usersSysType {
-	case MinIOUsersSysType:
-		policies, err = globalIAMSys.PolicyDBGet(accountName, false)
-	case LDAPUsersSysType:
-		parentUser := accountName
-		if cred.ParentUser != "" {
-			parentUser = cred.ParentUser
-		}
-		policies, err = globalIAMSys.PolicyDBGet(parentUser, false, cred.Groups...)
-	default:
-		err = errors.New("should never happen")
+	if cred.IsTemp() || cred.IsServiceAccount() {
+		// For derived credentials, check the parent user's permissions.
+		accountName = cred.ParentUser
 	}
+	policies, err := globalIAMSys.PolicyDBGet(accountName, false, cred.Groups...)
 	if err != nil {
 		logger.LogIf(ctx, err)
 		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)

cmd/iam.go

@@ -1425,12 +1425,15 @@ func (sys *IAMSys) IsAllowedSTS(args iampolicy.Args, parentUser string) bool {
 			return false
 		}
 		policies = newMappedPolicy(sys.rolesMap[arn]).toSlice()
-	} else if parentUser == globalActiveCred.AccessKey {
-		policies = []string{"consoleAdmin"}
 	} else {
 		// Lookup the parent user's mapping if there's no role-ARN.
-		mp, ok := sys.store.GetMappedPolicy(parentUser, false)
-		if !ok {
+		var err error
+		policies, err = sys.store.PolicyDBGet(parentUser, false, args.Groups...)
+		if err != nil {
+			logger.LogIf(GlobalContext, fmt.Errorf("error fetching policies on %s: %v", parentUser, err))
+			return false
+		}
+		if len(policies) == 0 {
 			// TODO (deprecated in Dec 2021): Only need to handle
 			// behavior for STS credentials created in older
 			// releases. Otherwise, reject such cases, once older
@@ -1444,10 +1447,7 @@ func (sys *IAMSys) IsAllowedSTS(args iampolicy.Args, parentUser string) bool {
 				return false
 			}
 			policies = policySet.ToSlice()
-		} else {
-			policies = mp.toSlice()
 		}
-
 	}
 
 	combinedPolicy, err := sys.store.GetPolicy(strings.Join(policies, ","))

cmd/sts-handlers_test.go

@@ -40,6 +40,7 @@ func runAllIAMSTSTests(suite *TestSuiteIAM, c *check) {
 	// The STS for root test needs to be the first one after setup.
 	suite.TestSTSForRoot(c)
 	suite.TestSTS(c)
+	suite.TestSTSWithGroupPolicy(c)
 	suite.TearDownSuite(c)
 }
 
@@ -157,6 +158,103 @@ func (s *TestSuiteIAM) TestSTS(c *check) {
 	}
 }
 
+func (s *TestSuiteIAM) TestSTSWithGroupPolicy(c *check) {
+	ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
+	defer cancel()
+
+	bucket := getRandomBucketName()
+	err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{})
+	if err != nil {
+		c.Fatalf("bucket creat error: %v", err)
+	}
+
+	// Create policy, user and associate policy
+	policy := "mypolicy"
+	policyBytes := []byte(fmt.Sprintf(`{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Allow",
+   "Action": [
+    "s3:PutObject",
+    "s3:GetObject",
+    "s3:ListBucket"
+   ],
+   "Resource": [
+    "arn:aws:s3:::%s/*"
+   ]
+  }
+ ]
+}`, bucket))
+	err = s.adm.AddCannedPolicy(ctx, policy, policyBytes)
+	if err != nil {
+		c.Fatalf("policy add error: %v", err)
+	}
+
+	accessKey, secretKey := mustGenerateCredentials(c)
+	err = s.adm.SetUser(ctx, accessKey, secretKey, madmin.AccountEnabled)
+	if err != nil {
+		c.Fatalf("Unable to set user: %v", err)
+	}
+
+	// confirm that the user is unable to access the bucket - we have not
+	// yet set any policy
+	uClient := s.getUserClient(c, accessKey, secretKey, "")
+	c.mustNotListObjects(ctx, uClient, bucket)
+
+	err = s.adm.UpdateGroupMembers(ctx, madmin.GroupAddRemove{
+		Group:   "test-group",
+		Members: []string{accessKey},
+	})
+	if err != nil {
+		c.Fatalf("unable to add user to group: %v", err)
+	}
+
+	err = s.adm.SetPolicy(ctx, policy, "test-group", true)
+	if err != nil {
+		c.Fatalf("Unable to set policy: %v", err)
+	}
+
+	// confirm that the user is able to access the bucket - permission comes
+	// from group.
+	c.mustListObjects(ctx, uClient, bucket)
+
+	// Create STS user.
+	assumeRole := cr.STSAssumeRole{
+		Client:      s.TestSuiteCommon.client,
+		STSEndpoint: s.endPoint,
+		Options: cr.STSAssumeRoleOptions{
+			AccessKey: accessKey,
+			SecretKey: secretKey,
+			Location:  "",
+		},
+	}
+	value, err := assumeRole.Retrieve()
+	if err != nil {
+		c.Fatalf("err calling assumeRole: %v", err)
+	}
+
+	// Check that STS user client has access coming from parent user's
+	// group.
+	minioClient, err := minio.New(s.endpoint, &minio.Options{
+		Creds:     cr.NewStaticV4(value.AccessKeyID, value.SecretAccessKey, value.SessionToken),
+		Secure:    s.secure,
+		Transport: s.TestSuiteCommon.client.Transport,
+	})
+	if err != nil {
+		c.Fatalf("Error initializing client: %v", err)
+	}
+
+	// Validate that the client from sts creds can access the bucket.
+	c.mustListObjects(ctx, minioClient, bucket)
+
+	// Validate that the client cannot remove any objects
+	err = minioClient.RemoveObject(ctx, bucket, "someobject", minio.RemoveObjectOptions{})
+	if err.Error() != "Access Denied." {
+		c.Fatalf("unexpected non-access-denied err: %v", err)
+	}
+}
+
 // TestSTSForRoot - needs to be the first test after server setup due to the
 // buckets list check.
 func (s *TestSuiteIAM) TestSTSForRoot(c *check) {
@@ -232,10 +330,6 @@ func (s *TestSuiteIAM) TestSTSForRoot(c *check) {
 	}
 }
 
-func (s *TestSuiteIAM) GetLDAPServer(c *check) string {
-	return os.Getenv(EnvTestLDAPServer)
-}
-
 // SetUpLDAP - expects to setup an LDAP test server using the test LDAP
 // container and canned data from https://github.com/minio/minio-ldap-testing
 func (s *TestSuiteIAM) SetUpLDAP(c *check, serverAddr string) {