From 0319ae756a2b25012cbaa02c4627c8b813a8d9ec Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 7 Feb 2023 03:43:08 -0800 Subject: [PATCH] fix: pass proper username (simple) string as expected (#16555) --- cmd/admin-handlers-users_test.go | 19 +++++++ cmd/bucket-policy.go | 11 +--- cmd/iam.go | 7 --- cmd/sts-handlers_test.go | 90 ++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 16 deletions(-) diff --git a/cmd/admin-handlers-users_test.go b/cmd/admin-handlers-users_test.go index cecdc974c..49eaa1d45 100644 --- a/cmd/admin-handlers-users_test.go +++ b/cmd/admin-handlers-users_test.go @@ -1261,6 +1261,25 @@ func (c *check) mustListBuckets(ctx context.Context, client *minio.Client) { } } +func (c *check) mustDownload(ctx context.Context, client *minio.Client, bucket string) { + c.Helper() + rd, err := client.GetObject(ctx, bucket, "some-object", minio.GetObjectOptions{}) + if err != nil { + c.Fatalf("download did not succeed got %#v", err) + } + if _, err = io.Copy(io.Discard, rd); err != nil { + c.Fatalf("download did not succeed got %#v", err) + } +} + +func (c *check) mustUpload(ctx context.Context, client *minio.Client, bucket string) { + c.Helper() + _, err := client.PutObject(ctx, bucket, "some-object", bytes.NewBuffer([]byte("stuff")), 5, minio.PutObjectOptions{}) + if err != nil { + c.Fatalf("upload did not succeed got %#v", err) + } +} + func (c *check) mustNotUpload(ctx context.Context, client *minio.Client, bucket string) { c.Helper() _, err := client.PutObject(ctx, bucket, "some-object", bytes.NewBuffer([]byte("stuff")), 5, minio.PutObjectOptions{}) diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index 0028dca25..e01d09c43 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -199,15 +199,8 @@ func getConditionValues(r *http.Request, lc string, cred auth.Credentials) map[s for k, v := range claims { vStr, ok := v.(string) if ok { - // Special case for AD/LDAP STS users - switch k { - case ldapUser: - args["user"] = []string{vStr} - case ldapUserN: - args["username"] = []string{vStr} - default: - args[k] = []string{vStr} - } + // Trim any LDAP specific prefix + args[strings.ToLower(strings.TrimPrefix(k, "ldap"))] = []string{vStr} } } diff --git a/cmd/iam.go b/cmd/iam.go index 0f7ce5699..2f6f845c1 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1693,9 +1693,6 @@ func (sys *IAMSys) IsAllowedServiceAccount(args iampolicy.Args, parentUser strin parentArgs := args parentArgs.AccountName = parentUser - // These are dynamic values set them appropriately. - parentArgs.ConditionValues["username"] = []string{parentUser} - parentArgs.ConditionValues["userid"] = []string{parentUser} saPolicyClaim, ok := args.Claims[iamPolicyClaimNameSA()] if !ok { @@ -1822,10 +1819,6 @@ func (sys *IAMSys) IsAllowedSTS(args iampolicy.Args, parentUser string) bool { // 3. If an inline session-policy is present, evaluate it. - // These are dynamic values set them appropriately. - args.ConditionValues["username"] = []string{parentUser} - args.ConditionValues["userid"] = []string{parentUser} - // Now check if we have a sessionPolicy. hasSessionPolicy, isAllowedSP := isAllowedBySessionPolicy(args) if hasSessionPolicy { diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index 2a7a9a16c..0bfce0907 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -485,6 +485,7 @@ func TestIAMWithLDAPServerSuite(t *testing.T) { suite.SetUpLDAP(c, ldapServer) suite.TestLDAPSTS(c) suite.TestLDAPSTSServiceAccounts(c) + suite.TestLDAPSTSServiceAccountsWithUsername(c) suite.TestLDAPSTSServiceAccountsWithGroups(c) suite.TearDownSuite(c) }, @@ -726,6 +727,95 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccounts(c *check) { c.mustNotCreateSvcAccount(ctx, globalActiveCred.AccessKey, userAdmClient) } +func (s *TestSuiteIAM) TestLDAPSTSServiceAccountsWithUsername(c *check) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + bucket := "dillon" + err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{}) + if err != nil { + c.Fatalf("bucket create error: %v", err) + } + + // Create policy + policy := "mypolicy-username" + policyBytes := []byte(`{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "s3:PutObject", + "s3:GetObject", + "s3:ListBucket" + ], + "Resource": [ + "arn:aws:s3:::${ldap:username}/*" + ] + } + ] +}`) + err = s.adm.AddCannedPolicy(ctx, policy, policyBytes) + if err != nil { + c.Fatalf("policy add error: %v", err) + } + + userDN := "uid=dillon,ou=people,ou=swengg,dc=min,dc=io" + err = s.adm.SetPolicy(ctx, policy, userDN, false) + if err != nil { + c.Fatalf("Unable to set policy: %v", err) + } + + ldapID := cr.LDAPIdentity{ + Client: s.TestSuiteCommon.client, + STSEndpoint: s.endPoint, + LDAPUsername: "dillon", + LDAPPassword: "dillon", + } + + value, err := ldapID.Retrieve() + if err != nil { + c.Fatalf("Expected to generate STS creds, got err: %#v", err) + } + + // Check that the LDAP sts cred is actually working. + 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) + + // Create an madmin client with user creds + userAdmClient, err := madmin.NewWithOptions(s.endpoint, &madmin.Options{ + Creds: cr.NewStaticV4(value.AccessKeyID, value.SecretAccessKey, value.SessionToken), + Secure: s.secure, + }) + if err != nil { + c.Fatalf("Err creating user admin client: %v", err) + } + userAdmClient.SetCustomTransport(s.TestSuiteCommon.client.Transport) + + // Create svc acc + cr := c.mustCreateSvcAccount(ctx, value.AccessKeyID, userAdmClient) + + svcClient := s.getUserClient(c, cr.AccessKey, cr.SecretKey, "") + + // 1. Check S3 access for service account ListObjects() + c.mustListObjects(ctx, svcClient, bucket) + + // 2. Check S3 access for upload + c.mustUpload(ctx, svcClient, bucket) + + // 3. Check S3 access for download + c.mustDownload(ctx, svcClient, bucket) +} + // In this test, the parent users gets their permissions from a group, rather // than having a policy set directly on them. func (s *TestSuiteIAM) TestLDAPSTSServiceAccountsWithGroups(c *check) {