From 79a58e275cb6eabf6110488c8cd0a048651ae361 Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Mon, 1 Nov 2021 15:03:07 -0700 Subject: [PATCH] fix: race in delete user functionality (#13547) - The race happens with a goroutine that refreshes IAM cache data from storage. - It could lead to deleted users re-appearing as valid live credentials. - This change also causes CI to run tests without a race flag (in addition to running it with). --- .github/workflows/go-lint.yml | 1 + cmd/admin-handlers-users-race_test.go | 126 ++++++++++++++++++++++++++ cmd/admin-handlers-users_test.go | 2 +- cmd/gateway-main.go | 2 +- cmd/iam.go | 19 ++-- cmd/server-main.go | 2 +- cmd/test-utils_test.go | 2 +- 7 files changed, 141 insertions(+), 13 deletions(-) create mode 100644 cmd/admin-handlers-users-race_test.go diff --git a/.github/workflows/go-lint.yml b/.github/workflows/go-lint.yml index 11e477dcf..6a8d2f4a6 100644 --- a/.github/workflows/go-lint.yml +++ b/.github/workflows/go-lint.yml @@ -45,4 +45,5 @@ jobs: curl -L -o nancy https://github.com/sonatype-nexus-community/nancy/releases/download/${nancy_version}/nancy-${nancy_version}-linux-amd64 && chmod +x nancy go list -deps -json ./... | jq -s 'unique_by(.Module.Path)|.[]|select(has("Module"))|.Module' | ./nancy sleuth make + make test make test-race diff --git a/cmd/admin-handlers-users-race_test.go b/cmd/admin-handlers-users-race_test.go new file mode 100644 index 000000000..f3ccd9c11 --- /dev/null +++ b/cmd/admin-handlers-users-race_test.go @@ -0,0 +1,126 @@ +// Copyright (c) 2015-2021 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +// +build !race + +// Tests in this file are not run under the `-race` flag as they are too slow +// and cause context deadline errors. + +package cmd + +import ( + "context" + "fmt" + "sync" + "testing" + "time" + + "github.com/minio/madmin-go" + minio "github.com/minio/minio-go/v7" +) + +func runAllIAMConcurrencyTests(suite *TestSuiteIAM, c *check) { + suite.SetUpSuite(c) + suite.TestDeleteUserRace(c) + suite.TearDownSuite(c) +} + +func TestIAMInternalIDPConcurrencyServerSuite(t *testing.T) { + testCases := []*TestSuiteIAM{ + // Init and run test on FS backend with signature v4. + newTestSuiteIAM(TestSuiteCommon{serverType: "FS", signer: signerV4}), + // Init and run test on FS backend, with tls enabled. + newTestSuiteIAM(TestSuiteCommon{serverType: "FS", signer: signerV4, secure: true}), + // Init and run test on Erasure backend. + newTestSuiteIAM(TestSuiteCommon{serverType: "Erasure", signer: signerV4}), + // Init and run test on ErasureSet backend. + newTestSuiteIAM(TestSuiteCommon{serverType: "ErasureSet", signer: signerV4}), + } + for i, testCase := range testCases { + t.Run(fmt.Sprintf("Test: %d, ServerType: %s", i+1, testCase.serverType), func(t *testing.T) { + runAllIAMConcurrencyTests(testCase, &check{t, testCase.serverType}) + }) + } +} + +func (s *TestSuiteIAM) TestDeleteUserRace(c *check) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + bucket := getRandomBucketName() + err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{}) + if err != nil { + c.Fatalf("bucket creat error: %v", err) + } + + // Create a policy 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) + } + + userCount := 50 + accessKeys := make([]string, userCount) + secretKeys := make([]string, userCount) + for i := 0; i < userCount; i++ { + accessKey, secretKey := mustGenerateCredentials(c) + err = s.adm.SetUser(ctx, accessKey, secretKey, madmin.AccountEnabled) + if err != nil { + c.Fatalf("Unable to set user: %v", err) + } + + err = s.adm.SetPolicy(ctx, policy, accessKey, false) + if err != nil { + c.Fatalf("Unable to set policy: %v", err) + } + + accessKeys[i] = accessKey + secretKeys[i] = secretKey + } + + wg := sync.WaitGroup{} + for i := 0; i < userCount; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + uClient := s.getUserClient(c, accessKeys[i], secretKeys[i], "") + err := s.adm.RemoveUser(ctx, accessKeys[i]) + if err != nil { + c.Fatalf("unable to remove user: %v", err) + } + c.mustNotListObjects(ctx, uClient, bucket) + }(i) + } + wg.Wait() +} diff --git a/cmd/admin-handlers-users_test.go b/cmd/admin-handlers-users_test.go index b9d72496c..634acfe34 100644 --- a/cmd/admin-handlers-users_test.go +++ b/cmd/admin-handlers-users_test.go @@ -32,7 +32,7 @@ import ( ) const ( - testDefaultTimeout = 10 * time.Second + testDefaultTimeout = 30 * time.Second ) // API suite container for IAM diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index f0bc035e2..dd56751c8 100644 --- a/cmd/gateway-main.go +++ b/cmd/gateway-main.go @@ -306,7 +306,7 @@ func StartGateway(ctx *cli.Context, gw Gateway) { logger.FatalIf(globalNotificationSys.Init(GlobalContext, buckets, newObject), "Unable to initialize notification system") } - go globalIAMSys.Init(GlobalContext, newObject, globalEtcdClient) + go globalIAMSys.Init(GlobalContext, newObject, globalEtcdClient, globalRefreshIAMInterval) if globalCacheConfig.Enabled { // initialize the new disk cache objects. diff --git a/cmd/iam.go b/cmd/iam.go index 3db497fb0..e8f2470cf 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -206,6 +206,8 @@ func newMappedPolicy(policy string) MappedPolicy { type IAMSys struct { sync.Mutex + iamRefreshInterval time.Duration + usersSysType UsersSysType // map of policy names to policy definitions @@ -473,9 +475,9 @@ func (sys *IAMSys) Load(ctx context.Context, store IAMStorageAPI) error { iamGroupPolicyMap := make(map[string]MappedPolicy) iamPolicyDocsMap := make(map[string]iampolicy.Policy) - store.rlock() + store.lock() + defer store.unlock() isMinIOUsersSys := sys.usersSysType == MinIOUsersSysType - store.runlock() if err := store.loadPolicyDocs(ctx, iamPolicyDocsMap); err != nil { return err @@ -518,9 +520,6 @@ func (sys *IAMSys) Load(ctx context.Context, store IAMStorageAPI) error { return err } - store.lock() - defer store.unlock() - for k, v := range iamPolicyDocsMap { sys.iamPolicyDocsMap[k] = v } @@ -565,7 +564,9 @@ func (sys *IAMSys) Load(ctx context.Context, store IAMStorageAPI) error { } // Init - initializes config system by reading entries from config/iam -func (sys *IAMSys) Init(ctx context.Context, objAPI ObjectLayer, etcdClient *etcd.Client) { +func (sys *IAMSys) Init(ctx context.Context, objAPI ObjectLayer, etcdClient *etcd.Client, iamRefreshInterval time.Duration) { + sys.iamRefreshInterval = iamRefreshInterval + // Initialize IAM store sys.InitStore(objAPI, etcdClient) @@ -649,14 +650,14 @@ func (sys *IAMSys) Init(ctx context.Context, objAPI ObjectLayer, etcdClient *etc case globalOpenIDConfig.ProviderEnabled(): go func() { for { - time.Sleep(globalRefreshIAMInterval) + time.Sleep(sys.iamRefreshInterval) sys.purgeExpiredCredentialsForExternalSSO(ctx) } }() case globalLDAPConfig.EnabledWithLookupBind(): go func() { for { - time.Sleep(globalRefreshIAMInterval) + time.Sleep(sys.iamRefreshInterval) sys.purgeExpiredCredentialsForLDAP(ctx) sys.updateGroupMembershipsForLDAP(ctx) } @@ -686,7 +687,7 @@ func (sys *IAMSys) watch(ctx context.Context) { } else { // Fall back to loading all items for { - time.Sleep(globalRefreshIAMInterval) + time.Sleep(sys.iamRefreshInterval) if err := sys.Load(ctx, sys.store); err != nil { logger.LogIf(ctx, err) } diff --git a/cmd/server-main.go b/cmd/server-main.go index 2a8dcf9f9..8997e9d04 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -570,7 +570,7 @@ func serverMain(ctx *cli.Context) { } // Initialize users credentials and policies in background right after config has initialized. - go globalIAMSys.Init(GlobalContext, newObject, globalEtcdClient) + go globalIAMSys.Init(GlobalContext, newObject, globalEtcdClient, globalRefreshIAMInterval) initDataScanner(GlobalContext, newObject) diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go index 250d5f987..a347a75a0 100644 --- a/cmd/test-utils_test.go +++ b/cmd/test-utils_test.go @@ -350,7 +350,7 @@ func UnstartedTestServer(t TestErrHandler, instanceType string) TestServer { initAllSubsystems(ctx, objLayer) - globalIAMSys.Init(ctx, objLayer, globalEtcdClient) + globalIAMSys.Init(ctx, objLayer, globalEtcdClient, 2*time.Second) return testServer }