From 526e10a2e032ee0fc4718c79ed0a8294fa5e103a Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Mon, 20 Dec 2021 14:07:16 -0800 Subject: [PATCH] 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 | 16 ++---- cmd/iam.go | 14 ++--- cmd/sts-handlers_test.go | 102 ++++++++++++++++++++++++++++++++++-- 3 files changed, 109 insertions(+), 23 deletions(-) diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index a5200311a..e478bbba0 100644 --- a/cmd/admin-handlers-users.go +++ b/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) diff --git a/cmd/iam.go b/cmd/iam.go index 52de3fe2d..f1a2fc552 100644 --- a/cmd/iam.go +++ b/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, ",")) diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index c907ff7a0..ff884bb93 100644 --- a/cmd/sts-handlers_test.go +++ b/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) {