From e82a5c5c544b16d550522b5bb9972f52f7d64923 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 8 Dec 2021 11:50:15 -0800 Subject: [PATCH] fix: site replication issues and add tests (#13861) - deleting policies was deleting all LDAP user mapping, this was a regression introduced in #13567 - deleting of policies is properly sent across all sites. - remove unexpected errors instead embed the real errors as part of the 500 error response. --- .github/workflows/iam-integrations.yaml | 4 + Makefile | 6 +- cmd/admin-handlers-site-replication.go | 65 ++++++----- cmd/iam-store.go | 26 +++-- cmd/site-replication.go | 23 ++-- docs/site-replication/run-multi-site.sh | 139 ++++++++++++++++++++++++ docs/site-replication/rw.json | 1 + 7 files changed, 215 insertions(+), 49 deletions(-) create mode 100755 docs/site-replication/run-multi-site.sh create mode 100644 docs/site-replication/rw.json diff --git a/.github/workflows/iam-integrations.yaml b/.github/workflows/iam-integrations.yaml index 1c1f7b74d..137480bf5 100644 --- a/.github/workflows/iam-integrations.yaml +++ b/.github/workflows/iam-integrations.yaml @@ -85,3 +85,7 @@ jobs: sudo sysctl net.ipv6.conf.all.disable_ipv6=0 sudo sysctl net.ipv6.conf.default.disable_ipv6=0 make test-iam + - name: Test LDAP for automatic site replication + if: matrix.ldap == 'localhost:389' + run: | + make test-site-replication diff --git a/Makefile b/Makefile index 079d39fe8..56cc2c691 100644 --- a/Makefile +++ b/Makefile @@ -57,9 +57,13 @@ test-iam: build ## verify IAM (external IDP, etcd backends) @CGO_ENABLED=1 go test -race -tags kqueue -v -run TestIAM* ./cmd test-replication: install ## verify multi site replication - @echo "Running tests for Replication three sites" + @echo "Running tests for replicating three sites" @(env bash $(PWD)/docs/bucket/replication/setup_3site_replication.sh) +test-site-replication: install ## verify automatic site replication + @echo "Running tests for automatic site replication of IAM" + @(env bash $(PWD)/docs/site-replication/run-multi-site.sh) + verify: ## verify minio various setups @echo "Verifying build with race" @GO111MODULE=on CGO_ENABLED=1 go build -race -tags kqueue -trimpath --ldflags "$(LDFLAGS)" -o $(PWD)/minio 1>/dev/null diff --git a/cmd/admin-handlers-site-replication.go b/cmd/admin-handlers-site-replication.go index 11676fe61..0c49d13f0 100644 --- a/cmd/admin-handlers-site-replication.go +++ b/cmd/admin-handlers-site-replication.go @@ -113,6 +113,8 @@ func (a adminAPIHandlers) SRInternalBucketOps(w http.ResponseWriter, r *http.Req var err error switch operation { + default: + err = errInvalidArgument case madmin.MakeWithVersioningBktOp: _, isLockEnabled := r.Form["lockEnabled"] _, isVersioningEnabled := r.Form["versioningEnabled"] @@ -128,13 +130,10 @@ func (a adminAPIHandlers) SRInternalBucketOps(w http.ResponseWriter, r *http.Req err = globalSiteReplicationSys.PeerBucketDeleteHandler(ctx, bucket, false) case madmin.ForceDeleteBucketBktOp: err = globalSiteReplicationSys.PeerBucketDeleteHandler(ctx, bucket, true) - default: - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL) - return } if err != nil { logger.LogIf(ctx, err) - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL) + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } @@ -160,37 +159,40 @@ func (a adminAPIHandlers) SRInternalReplicateIAMItem(w http.ResponseWriter, r *h var err error switch item.Type { + default: + err = errInvalidArgument case madmin.SRIAMItemPolicy: - var policy *iampolicy.Policy - if len(item.Policy) > 0 { - policy, err = iampolicy.ParseConfig(bytes.NewReader(item.Policy)) - if err != nil { - writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) + if item.Policy == nil { + err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, nil) + } else { + policy, perr := iampolicy.ParseConfig(bytes.NewReader(item.Policy)) + if perr != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, perr), r.URL) return } + if policy.IsEmpty() { + err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, nil) + } else { + err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, policy) + } } - err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, policy) case madmin.SRIAMItemSvcAcc: - err = globalSiteReplicationSys.PeerSvcAccChangeHandler(ctx, *item.SvcAccChange) + err = globalSiteReplicationSys.PeerSvcAccChangeHandler(ctx, item.SvcAccChange) case madmin.SRIAMItemPolicyMapping: - err = globalSiteReplicationSys.PeerPolicyMappingHandler(ctx, *item.PolicyMapping) + err = globalSiteReplicationSys.PeerPolicyMappingHandler(ctx, item.PolicyMapping) case madmin.SRIAMItemSTSAcc: - err = globalSiteReplicationSys.PeerSTSAccHandler(ctx, *item.STSCredential) - - default: - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL) - return + err = globalSiteReplicationSys.PeerSTSAccHandler(ctx, item.STSCredential) } if err != nil { logger.LogIf(ctx, err) - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL) + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } } // SRInternalReplicateBucketItem - PUT /minio/admin/v3/site-replication/bucket-meta func (a adminAPIHandlers) SRInternalReplicateBucketItem(w http.ResponseWriter, r *http.Request) { - ctx := newContext(r, w, "SRInternalReplicateIAMItem") + ctx := newContext(r, w, "SRInternalReplicateBucketItem") defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r)) @@ -208,30 +210,33 @@ func (a adminAPIHandlers) SRInternalReplicateBucketItem(w http.ResponseWriter, r var err error switch item.Type { + default: + err = errInvalidArgument case madmin.SRBucketMetaTypePolicy: - var bktPolicy *policy.Policy - if len(item.Policy) > 0 { - bktPolicy, err = policy.ParseConfig(bytes.NewReader(item.Policy), item.Bucket) - if err != nil { - writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) + if item.Policy == nil { + err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, nil) + } else { + bktPolicy, berr := policy.ParseConfig(bytes.NewReader(item.Policy), item.Bucket) + if berr != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, berr), r.URL) return } + if bktPolicy.IsEmpty() { + err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, nil) + } else { + err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, bktPolicy) + } } - err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, bktPolicy) case madmin.SRBucketMetaTypeTags: err = globalSiteReplicationSys.PeerBucketTaggingHandler(ctx, item.Bucket, item.Tags) case madmin.SRBucketMetaTypeObjectLockConfig: err = globalSiteReplicationSys.PeerBucketObjectLockConfigHandler(ctx, item.Bucket, item.ObjectLockConfig) case madmin.SRBucketMetaTypeSSEConfig: err = globalSiteReplicationSys.PeerBucketSSEConfigHandler(ctx, item.Bucket, item.SSEConfig) - - default: - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL) - return } if err != nil { logger.LogIf(ctx, err) - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL) + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } } diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 1a7f18bd6..6698b7f03 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -846,12 +846,14 @@ func (store *IAMStoreSys) PolicyNotificationHandler(ctx context.Context, policy if !pset.Contains(policy) { continue } - _, ok := cache.iamUsersMap[u] - if !ok { - // happens when account is deleted or - // expired. - delete(cache.iamUserPolicyMap, u) - continue + if store.getUsersSysType() == MinIOUsersSysType { + _, ok := cache.iamUsersMap[u] + if !ok { + // happens when account is deleted or + // expired. + delete(cache.iamUserPolicyMap, u) + continue + } } pset.Remove(policy) cache.iamUserPolicyMap[u] = newMappedPolicy(strings.Join(pset.ToSlice(), ",")) @@ -886,11 +888,13 @@ func (store *IAMStoreSys) DeletePolicy(ctx context.Context, policy string) error groups := []string{} for u, mp := range cache.iamUserPolicyMap { pset := mp.policySet() - if _, ok := cache.iamUsersMap[u]; !ok { - // This case can happen when a temporary account is - // deleted or expired - remove it from userPolicyMap. - delete(cache.iamUserPolicyMap, u) - continue + if store.getUsersSysType() == MinIOUsersSysType { + if _, ok := cache.iamUsersMap[u]; !ok { + // This case can happen when a temporary account is + // deleted or expired - remove it from userPolicyMap. + delete(cache.iamUserPolicyMap, u) + continue + } } if pset.Contains(policy) { users = append(users, u) diff --git a/cmd/site-replication.go b/cmd/site-replication.go index dc9a7b17e..e46f30e04 100644 --- a/cmd/site-replication.go +++ b/cmd/site-replication.go @@ -48,8 +48,7 @@ import ( const ( srStatePrefix = minioConfigPrefix + "/site-replication" - - srStateFile = "state.json" + srStateFile = "state.json" ) const ( @@ -1007,7 +1006,10 @@ func (c *SiteReplicationSys) PeerAddPolicyHandler(ctx context.Context, policyNam } // PeerSvcAccChangeHandler - copies service-account change to local. -func (c *SiteReplicationSys) PeerSvcAccChangeHandler(ctx context.Context, change madmin.SRSvcAccChange) error { +func (c *SiteReplicationSys) PeerSvcAccChangeHandler(ctx context.Context, change *madmin.SRSvcAccChange) error { + if change == nil { + return errInvalidArgument + } switch { case change.Create != nil: var sp *iampolicy.Policy @@ -1069,7 +1071,10 @@ func (c *SiteReplicationSys) PeerSvcAccChangeHandler(ctx context.Context, change } // PeerPolicyMappingHandler - copies policy mapping to local. -func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mapping madmin.SRPolicyMapping) error { +func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mapping *madmin.SRPolicyMapping) error { + if mapping == nil { + return errInvalidArgument + } err := globalIAMSys.PolicyDBSet(ctx, mapping.UserOrGroup, mapping.Policy, mapping.IsGroup) if err != nil { return wrapSRErr(err) @@ -1078,7 +1083,11 @@ func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mappi } // PeerSTSAccHandler - replicates STS credential locally. -func (c *SiteReplicationSys) PeerSTSAccHandler(ctx context.Context, stsCred madmin.SRSTSCredential) error { +func (c *SiteReplicationSys) PeerSTSAccHandler(ctx context.Context, stsCred *madmin.SRSTSCredential) error { + if stsCred == nil { + return errInvalidArgument + } + // Verify the session token of the stsCred claims, err := auth.ExtractClaims(stsCred.SessionToken, globalActiveCred.SecretKey) if err != nil { @@ -1089,13 +1098,13 @@ func (c *SiteReplicationSys) PeerSTSAccHandler(ctx context.Context, stsCred madm mapClaims := claims.Map() expiry, err := auth.ExpToInt64(mapClaims["exp"]) if err != nil { - return fmt.Errorf("Expiry claim was not found") + return fmt.Errorf("Expiry claim was not found: %v", mapClaims) } // Extract the username and lookup DN and groups in LDAP. ldapUser, ok := claims.Lookup(ldapUserN) if !ok { - return fmt.Errorf("Could not find LDAP username in claims") + return fmt.Errorf("Could not find LDAP username in claims: %v", mapClaims) } // Need to lookup the groups from LDAP. diff --git a/docs/site-replication/run-multi-site.sh b/docs/site-replication/run-multi-site.sh new file mode 100755 index 000000000..9c141c48b --- /dev/null +++ b/docs/site-replication/run-multi-site.sh @@ -0,0 +1,139 @@ +#!/usr/bin/env bash + +# shellcheck disable=SC2120 +exit_1() { + cleanup + exit 1 +} + +cleanup() { + echo "Cleaning up instances of MinIO" + pkill minio + pkill -9 minio + rm -rf /tmp/minio{1,2,3} +} + +cleanup + +unset MINIO_KMS_KES_CERT_FILE +unset MINIO_KMS_KES_KEY_FILE +unset MINIO_KMS_KES_ENDPOINT +unset MINIO_KMS_KES_KEY_NAME + +export MINIO_BROWSER=off +export MINIO_ROOT_USER="minio" +export MINIO_ROOT_PASSWORD="minio123" +export MINIO_KMS_AUTO_ENCRYPTION=off +export MINIO_PROMETHEUS_AUTH_TYPE=public +export MINIO_KMS_SECRET_KEY=my-minio-key:OSMM+vkKUTCvQs9YL/CVMIMt43HFhkUpqJxTmGl6rYw= +export MINIO_IDENTITY_LDAP_SERVER_ADDR="localhost:389" +export MINIO_IDENTITY_LDAP_SERVER_INSECURE="on" +export MINIO_IDENTITY_LDAP_LOOKUP_BIND_DN="cn=admin,dc=min,dc=io" +export MINIO_IDENTITY_LDAP_LOOKUP_BIND_PASSWORD="admin" +export MINIO_IDENTITY_LDAP_USER_DN_SEARCH_BASE_DN="dc=min,dc=io" +export MINIO_IDENTITY_LDAP_USER_DN_SEARCH_FILTER="(uid=%s)" +export MINIO_IDENTITY_LDAP_GROUP_SEARCH_BASE_DN="ou=swengg,dc=min,dc=io" +export MINIO_IDENTITY_LDAP_GROUP_SEARCH_FILTER="(&(objectclass=groupOfNames)(member=%d))" + +if [ ! -f ./mc ]; then + wget -O mc https://dl.minio.io/client/mc/release/linux-amd64/mc \ + && chmod +x mc +fi + +minio server --address ":9001" /tmp/minio1/{1...4} >/tmp/minio1_1.log 2>&1 & +minio server --address ":9002" /tmp/minio2/{1...4} >/tmp/minio2_1.log 2>&1 & +minio server --address ":9003" /tmp/minio3/{1...4} >/tmp/minio3_1.log 2>&1 & + +sleep 10 + +export MC_HOST_minio1=http://minio:minio123@localhost:9001 +export MC_HOST_minio2=http://minio:minio123@localhost:9002 +export MC_HOST_minio3=http://minio:minio123@localhost:9003 + +./mc admin replicate add minio1 minio2 minio3 + +./mc admin policy set minio1 consoleAdmin user="uid=dillon,ou=people,ou=swengg,dc=min,dc=io" +sleep 5 + +./mc admin user info minio2 "uid=dillon,ou=people,ou=swengg,dc=min,dc=io" +./mc admin user info minio3 "uid=dillon,ou=people,ou=swengg,dc=min,dc=io" +./mc admin policy add minio1 rw ./docs/site-replication/rw.json + +sleep 5 +./mc admin policy info minio2 rw >/dev/null 2>&1 +./mc admin policy info minio3 rw >/dev/null 2>&1 + +./mc admin policy remove minio3 rw + +sleep 10 +./mc admin policy info minio1 rw +if [ $? -eq 0 ]; then + echo "expecting the command to fail, exiting.." + exit_1; +fi + +./mc admin policy info minio2 rw +if [ $? -eq 0 ]; then + echo "expecting the command to fail, exiting.." + exit_1; +fi + +./mc admin user info minio1 "uid=dillon,ou=people,ou=swengg,dc=min,dc=io" +if [ $? -eq 1 ]; then + echo "policy mapping missing, exiting.." + exit_1; +fi + +./mc admin user info minio2 "uid=dillon,ou=people,ou=swengg,dc=min,dc=io" +if [ $? -eq 1 ]; then + echo "policy mapping missing, exiting.." + exit_1; +fi + +./mc admin user info minio3 "uid=dillon,ou=people,ou=swengg,dc=min,dc=io" +if [ $? -eq 1 ]; then + echo "policy mapping missing, exiting.." + exit_1; +fi + +# LDAP simple user +./mc admin user svcacct add minio2 dillon --access-key testsvc --secret-key testsvc123 +if [ $? -eq 1 ]; then + echo "adding svc account failed, exiting.." + exit_1; +fi + +sleep 10 + +./mc admin user svcacct info minio1 testsvc +if [ $? -eq 1 ]; then + echo "svc account not mirrored, exiting.." + exit_1; +fi + +./mc admin user svcacct info minio2 testsvc +if [ $? -eq 1 ]; then + echo "svc account not mirrored, exiting.." + exit_1; +fi + +./mc admin user svcacct rm minio1 testsvc +if [ $? -eq 1 ]; then + echo "removing svc account failed, exiting.." + exit_1; +fi + +sleep 10 +./mc admin user svcacct info minio2 testsvc +if [ $? -eq 0 ]; then + echo "svc account found after delete, exiting.." + exit_1; +fi + +./mc admin user svcacct info minio3 testsvc +if [ $? -eq 0 ]; then + echo "svc account found after delete, exiting.." + exit_1; +fi + +cleanup diff --git a/docs/site-replication/rw.json b/docs/site-replication/rw.json new file mode 100644 index 000000000..b5f5adc5a --- /dev/null +++ b/docs/site-replication/rw.json @@ -0,0 +1 @@ +{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["admin:*"]},{"Effect":"Allow","Action":["s3:*"],"Resource":["arn:aws:s3:::*"]}]}