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

@@ -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

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 <http://www.gnu.org/licenses/>.
+
+// +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()
+}

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

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.

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)
 			}

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)
 

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
 }