From b8cdf060c896a11b1ac1c1dbba6e0937bf57ba79 Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Tue, 23 Aug 2022 19:11:45 +0100 Subject: [PATCH] Properly replicate policy mapping for virtual users (#15558) Currently, replicating policy mapping for STS users does not work. Fix it is by passing user type to PolicyDBSet. --- cmd/admin-handlers-users.go | 33 +++++++++++-- cmd/iam-store.go | 18 ++----- cmd/iam.go | 36 ++++++-------- cmd/notification.go | 4 +- cmd/peer-rest-client.go | 3 +- cmd/peer-rest-common.go | 3 +- cmd/peer-rest-server.go | 9 +++- cmd/site-replication.go | 94 +++++++++++++++---------------------- go.mod | 2 +- go.sum | 4 +- 10 files changed, 102 insertions(+), 104 deletions(-) diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 65e280aa7..07cd566e3 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -1529,7 +1529,29 @@ func (a adminAPIHandlers) SetPolicyForUserOrGroup(w http.ResponseWriter, r *http } } - updatedAt, err := globalIAMSys.PolicyDBSet(ctx, entityName, policyName, isGroup) + // Validate that user or group exists. + if !isGroup { + if globalIAMSys.GetUsersSysType() == MinIOUsersSysType { + _, ok := globalIAMSys.GetUser(ctx, entityName) + if !ok { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errNoSuchUser), r.URL) + return + } + } + } else { + _, err := globalIAMSys.GetGroupDescription(entityName) + if err != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) + return + } + } + + userType := regUser + if globalIAMSys.GetUsersSysType() == LDAPUsersSysType { + userType = stsUser + } + + updatedAt, err := globalIAMSys.PolicyDBSet(ctx, entityName, policyName, userType, isGroup) if err != nil { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return @@ -1539,6 +1561,7 @@ func (a adminAPIHandlers) SetPolicyForUserOrGroup(w http.ResponseWriter, r *http Type: madmin.SRIAMItemPolicyMapping, PolicyMapping: &madmin.SRPolicyMapping{ UserOrGroup: entityName, + UserType: int(userType), IsGroup: isGroup, Policy: policyName, }, @@ -2114,7 +2137,7 @@ func (a adminAPIHandlers) ImportIAM(w http.ResponseWriter, r *http.Request) { writeErrorResponseJSON(ctx, w, importError(ctx, errIAMActionNotAllowed, userPolicyMappingsFile, u), r.URL) return } - if _, err := globalIAMSys.PolicyDBSet(ctx, u, pm.Policies, false); err != nil { + if _, err := globalIAMSys.PolicyDBSet(ctx, u, pm.Policies, regUser, false); err != nil { writeErrorResponseJSON(ctx, w, importError(ctx, err, userPolicyMappingsFile, u), r.URL) return } @@ -2143,7 +2166,7 @@ func (a adminAPIHandlers) ImportIAM(w http.ResponseWriter, r *http.Request) { return } for g, pm := range grpPolicyMap { - if _, err := globalIAMSys.PolicyDBSet(ctx, g, pm.Policies, true); err != nil { + if _, err := globalIAMSys.PolicyDBSet(ctx, g, pm.Policies, unknownIAMUserType, true); err != nil { writeErrorResponseJSON(ctx, w, importError(ctx, err, groupPolicyMappingsFile, g), r.URL) return } @@ -2182,7 +2205,7 @@ func (a adminAPIHandlers) ImportIAM(w http.ResponseWriter, r *http.Request) { writeErrorResponseJSON(ctx, w, importError(ctx, errIAMActionNotAllowed, stsUserPolicyMappingsFile, u), r.URL) return } - if _, err := globalIAMSys.PolicyDBSet(ctx, u, pm.Policies, false); err != nil { + if _, err := globalIAMSys.PolicyDBSet(ctx, u, pm.Policies, stsUser, false); err != nil { writeErrorResponseJSON(ctx, w, importError(ctx, err, stsUserPolicyMappingsFile, u), r.URL) return } @@ -2211,7 +2234,7 @@ func (a adminAPIHandlers) ImportIAM(w http.ResponseWriter, r *http.Request) { return } for g, pm := range grpPolicyMap { - if _, err := globalIAMSys.PolicyDBSet(ctx, g, pm.Policies, true); err != nil { + if _, err := globalIAMSys.PolicyDBSet(ctx, g, pm.Policies, unknownIAMUserType, true); err != nil { writeErrorResponseJSON(ctx, w, importError(ctx, err, stsGroupPolicyMappingsFile, g), r.URL) return } diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 2ec36a8e7..e9b55ad84 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -862,7 +862,10 @@ func (store *IAMStoreSys) ListGroups(ctx context.Context) (res []string, err err } // PolicyDBSet - update the policy mapping for the given user or group in -// storage and in cache. +// storage and in cache. We do not check for the existence of the user here +// since users can be virtual, such as for: +// - LDAP users +// - CommonName for STS accounts generated by AssumeRoleWithCertificate func (store *IAMStoreSys) PolicyDBSet(ctx context.Context, name, policy string, userType IAMUserType, isGroup bool) (updatedAt time.Time, err error) { if name == "" { return updatedAt, errInvalidArgument @@ -871,19 +874,6 @@ func (store *IAMStoreSys) PolicyDBSet(ctx context.Context, name, policy string, cache := store.lock() defer store.unlock() - // Validate that user and group exist. - if store.getUsersSysType() == MinIOUsersSysType { - if !isGroup { - if _, ok := cache.iamUsersMap[name]; !ok { - return updatedAt, errNoSuchUser - } - } else { - if _, ok := cache.iamGroupsMap[name]; !ok { - return updatedAt, errNoSuchGroup - } - } - } - // Handle policy mapping removal. if policy == "" { if store.getUsersSysType() == LDAPUsersSysType { diff --git a/cmd/iam.go b/cmd/iam.go index beb5a2b6b..c0439935c 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -105,7 +105,8 @@ type IAMSys struct { type IAMUserType int const ( - regUser IAMUserType = iota + unknownIAMUserType IAMUserType = iota - 1 + regUser stsUser svcUser ) @@ -134,17 +135,11 @@ func (sys *IAMSys) LoadPolicy(ctx context.Context, objAPI ObjectLayer, policyNam // LoadPolicyMapping - loads the mapped policy for a user or group // from storage into server memory. -func (sys *IAMSys) LoadPolicyMapping(ctx context.Context, objAPI ObjectLayer, userOrGroup string, isGroup bool) error { +func (sys *IAMSys) LoadPolicyMapping(ctx context.Context, objAPI ObjectLayer, userOrGroup string, userType IAMUserType, isGroup bool) error { if !sys.Initialized() { return errServerNotInitialized } - // In case of LDAP, policy mappings are only applicable to sts users. - userType := regUser - if sys.usersSysType == LDAPUsersSysType { - userType = stsUser - } - return sys.store.PolicyMappingNotificationHandler(ctx, userOrGroup, isGroup, userType) } @@ -169,7 +164,7 @@ func (sys *IAMSys) LoadServiceAccount(ctx context.Context, accessKey string) err // initStore initializes IAM stores func (sys *IAMSys) initStore(objAPI ObjectLayer, etcdClient *etcd.Client) { if sys.ldapConfig.Enabled { - sys.EnableLDAPSys() + sys.SetUsersSysType(LDAPUsersSysType) } if etcdClient == nil { @@ -1437,18 +1432,12 @@ func (sys *IAMSys) ListGroups(ctx context.Context) (r []string, err error) { } } -// PolicyDBSet - sets a policy for a user or group in the PolicyDB. -func (sys *IAMSys) PolicyDBSet(ctx context.Context, name, policy string, isGroup bool) (updatedAt time.Time, err error) { +// PolicyDBSet - sets a policy for a user or group in the PolicyDB - the user doesn't have to exist since sometimes they are virtuals +func (sys *IAMSys) PolicyDBSet(ctx context.Context, name, policy string, userType IAMUserType, isGroup bool) (updatedAt time.Time, err error) { if !sys.Initialized() { return updatedAt, errServerNotInitialized } - // Determine user-type based on IDP mode. - userType := regUser - if sys.usersSysType == LDAPUsersSysType { - userType = stsUser - } - updatedAt, err = sys.store.PolicyDBSet(ctx, name, policy, userType, isGroup) if err != nil { return @@ -1456,7 +1445,7 @@ func (sys *IAMSys) PolicyDBSet(ctx context.Context, name, policy string, isGroup // Notify all other MinIO peers to reload policy if !sys.HasWatcher() { - for _, nerr := range globalNotificationSys.LoadPolicyMapping(name, isGroup) { + for _, nerr := range globalNotificationSys.LoadPolicyMapping(name, userType, isGroup) { if nerr.Err != nil { logger.GetReqInfo(ctx).SetTags("peerAddress", nerr.Host.String()) logger.LogIf(ctx, nerr.Err) @@ -1789,9 +1778,14 @@ func (sys *IAMSys) IsAllowed(args iampolicy.Args) bool { return sys.GetCombinedPolicy(policies...).IsAllowed(args) } -// EnableLDAPSys - enable ldap system users type. -func (sys *IAMSys) EnableLDAPSys() { - sys.usersSysType = LDAPUsersSysType +// SetUsersSysType - sets the users system type, regular or LDAP. +func (sys *IAMSys) SetUsersSysType(t UsersSysType) { + sys.usersSysType = t +} + +// GetUsersSysType - returns the users system type for this IAM +func (sys *IAMSys) GetUsersSysType() UsersSysType { + return sys.usersSysType } // NewIAMSys - creates new config system object. diff --git a/cmd/notification.go b/cmd/notification.go index d2da447ba..6f412178c 100644 --- a/cmd/notification.go +++ b/cmd/notification.go @@ -171,7 +171,7 @@ func (sys *NotificationSys) LoadPolicy(policyName string) []NotificationPeerErr } // LoadPolicyMapping - reloads a policy mapping across all peers -func (sys *NotificationSys) LoadPolicyMapping(userOrGroup string, isGroup bool) []NotificationPeerErr { +func (sys *NotificationSys) LoadPolicyMapping(userOrGroup string, userType IAMUserType, isGroup bool) []NotificationPeerErr { ng := WithNPeers(len(sys.peerClients)) for idx, client := range sys.peerClients { if client == nil { @@ -179,7 +179,7 @@ func (sys *NotificationSys) LoadPolicyMapping(userOrGroup string, isGroup bool) } client := client ng.Go(GlobalContext, func() error { - return client.LoadPolicyMapping(userOrGroup, isGroup) + return client.LoadPolicyMapping(userOrGroup, userType, isGroup) }, idx, *client.host) } return ng.Wait() diff --git a/cmd/peer-rest-client.go b/cmd/peer-rest-client.go index fc69161e2..a668734db 100644 --- a/cmd/peer-rest-client.go +++ b/cmd/peer-rest-client.go @@ -341,9 +341,10 @@ func (client *peerRESTClient) LoadPolicy(policyName string) (err error) { } // LoadPolicyMapping - reload a specific policy mapping -func (client *peerRESTClient) LoadPolicyMapping(userOrGroup string, isGroup bool) error { +func (client *peerRESTClient) LoadPolicyMapping(userOrGroup string, userType IAMUserType, isGroup bool) error { values := make(url.Values) values.Set(peerRESTUserOrGroup, userOrGroup) + values.Set(peerRESTUserType, strconv.Itoa(int(userType))) if isGroup { values.Set(peerRESTIsGroup, "") } diff --git a/cmd/peer-rest-common.go b/cmd/peer-rest-common.go index 3ea5257b2..bcfec96e1 100644 --- a/cmd/peer-rest-common.go +++ b/cmd/peer-rest-common.go @@ -18,7 +18,7 @@ package cmd const ( - peerRESTVersion = "v25" // Update /metrics + peerRESTVersion = "v26" // Add user-type to LoadPolicyMapping peerRESTVersionPrefix = SlashSeparator + peerRESTVersion peerRESTPrefix = minioReservedBucketPath + "/peer" peerRESTPath = peerRESTPrefix + peerRESTVersionPrefix @@ -82,6 +82,7 @@ const ( peerRESTUserTemp = "user-temp" peerRESTPolicy = "policy" peerRESTUserOrGroup = "user-or-group" + peerRESTUserType = "user-type" peerRESTIsGroup = "is-group" peerRESTSignal = "signal" peerRESTSubSys = "sub-sys" diff --git a/cmd/peer-rest-server.go b/cmd/peer-rest-server.go index ea9f781eb..d6fdd0336 100644 --- a/cmd/peer-rest-server.go +++ b/cmd/peer-rest-server.go @@ -125,8 +125,15 @@ func (s *peerRESTServer) LoadPolicyMappingHandler(w http.ResponseWriter, r *http return } + userType := -1 + userTypeStr, err := strconv.Atoi(vars[peerRESTUserType]) + if err != nil { + s.writeErrorResponse(w, fmt.Errorf("user-type `%d` is invalid: %w", userTypeStr, err)) + return + } + _, isGroup := r.Form[peerRESTIsGroup] - if err := globalIAMSys.LoadPolicyMapping(r.Context(), objAPI, userOrGroup, isGroup); err != nil { + if err := globalIAMSys.LoadPolicyMapping(r.Context(), objAPI, userOrGroup, IAMUserType(userType), isGroup); err != nil { s.writeErrorResponse(w, err) return } diff --git a/cmd/site-replication.go b/cmd/site-replication.go index 30304947e..ab82d65c2 100644 --- a/cmd/site-replication.go +++ b/cmd/site-replication.go @@ -1201,7 +1201,7 @@ func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mappi } } - _, err := globalIAMSys.PolicyDBSet(ctx, mapping.UserOrGroup, mapping.Policy, mapping.IsGroup) + _, err := globalIAMSys.PolicyDBSet(ctx, mapping.UserOrGroup, mapping.Policy, IAMUserType(mapping.UserType), mapping.IsGroup) if err != nil { return wrapSRErr(err) } @@ -1751,6 +1751,34 @@ func (c *SiteReplicationSys) syncToAllPeers(ctx context.Context) error { } } + // Followed by group policy mapping + { + // Replicate policy mappings on local to all peers. + groupPolicyMap := make(map[string]MappedPolicy) + globalIAMSys.store.rlock() + errG := globalIAMSys.store.loadMappedPolicies(ctx, unknownIAMUserType, true, groupPolicyMap) + globalIAMSys.store.runlock() + if errG != nil { + return errSRBackendIssue(errG) + } + + for group, mp := range groupPolicyMap { + err := c.IAMChangeHook(ctx, madmin.SRIAMItem{ + Type: madmin.SRIAMItemPolicyMapping, + PolicyMapping: &madmin.SRPolicyMapping{ + UserOrGroup: group, + UserType: -1, + IsGroup: true, + Policy: mp.Policies, + }, + UpdatedAt: mp.UpdatedAt, + }) + if err != nil { + return errSRIAMError(err) + } + } + } + // Service accounts are the static accounts that should be synced with // valid claims. { @@ -1812,10 +1840,8 @@ func (c *SiteReplicationSys) syncToAllPeers(ctx context.Context) error { { // Replicate policy mappings on local to all peers. userPolicyMap := make(map[string]MappedPolicy) - groupPolicyMap := make(map[string]MappedPolicy) globalIAMSys.store.rlock() errU := globalIAMSys.store.loadMappedPolicies(ctx, regUser, false, userPolicyMap) - errG := globalIAMSys.store.loadMappedPolicies(ctx, regUser, true, groupPolicyMap) globalIAMSys.store.runlock() if errU != nil { return errSRBackendIssue(errU) @@ -1826,6 +1852,7 @@ func (c *SiteReplicationSys) syncToAllPeers(ctx context.Context) error { Type: madmin.SRIAMItemPolicyMapping, PolicyMapping: &madmin.SRPolicyMapping{ UserOrGroup: user, + UserType: int(regUser), IsGroup: false, Policy: mp.Policies, }, @@ -1835,45 +1862,25 @@ func (c *SiteReplicationSys) syncToAllPeers(ctx context.Context) error { return errSRIAMError(err) } } - - if errG != nil { - return errSRBackendIssue(errG) - } - - for group, mp := range groupPolicyMap { - err := c.IAMChangeHook(ctx, madmin.SRIAMItem{ - Type: madmin.SRIAMItemPolicyMapping, - PolicyMapping: &madmin.SRPolicyMapping{ - UserOrGroup: group, - IsGroup: true, - Policy: mp.Policies, - }, - UpdatedAt: mp.UpdatedAt, - }) - if err != nil { - return errSRIAMError(err) - } - } } // and finally followed by policy mappings for for STS users. { // Replicate policy mappings on local to all peers. - userPolicyMap := make(map[string]MappedPolicy) - groupPolicyMap := make(map[string]MappedPolicy) + stsPolicyMap := make(map[string]MappedPolicy) globalIAMSys.store.rlock() - errU := globalIAMSys.store.loadMappedPolicies(ctx, stsUser, false, userPolicyMap) - errG := globalIAMSys.store.loadMappedPolicies(ctx, stsUser, true, groupPolicyMap) + errU := globalIAMSys.store.loadMappedPolicies(ctx, stsUser, false, stsPolicyMap) globalIAMSys.store.runlock() if errU != nil { return errSRBackendIssue(errU) } - for user, mp := range userPolicyMap { + for user, mp := range stsPolicyMap { err := c.IAMChangeHook(ctx, madmin.SRIAMItem{ Type: madmin.SRIAMItemPolicyMapping, PolicyMapping: &madmin.SRPolicyMapping{ UserOrGroup: user, + UserType: int(stsUser), IsGroup: false, Policy: mp.Policies, }, @@ -1883,25 +1890,6 @@ func (c *SiteReplicationSys) syncToAllPeers(ctx context.Context) error { return errSRIAMError(err) } } - - if errG != nil { - return errSRBackendIssue(errG) - } - - for group, mp := range groupPolicyMap { - err := c.IAMChangeHook(ctx, madmin.SRIAMItem{ - Type: madmin.SRIAMItemPolicyMapping, - PolicyMapping: &madmin.SRPolicyMapping{ - UserOrGroup: group, - IsGroup: true, - Policy: mp.Policies, - }, - UpdatedAt: mp.UpdatedAt, - }) - if err != nil { - return errSRIAMError(err) - } - } } return nil @@ -4323,16 +4311,10 @@ func (c *SiteReplicationSys) healIAMSystem(ctx context.Context, objAPI ObjectLay c.healGroups(ctx, objAPI, group, info) } for user := range info.UserStats { - c.healUserPolicies(ctx, objAPI, user, info, false) + c.healUserPolicies(ctx, objAPI, user, info) } for group := range info.GroupStats { - c.healGroupPolicies(ctx, objAPI, group, info, false) - } - for user := range info.UserStats { - c.healUserPolicies(ctx, objAPI, user, info, true) - } - for group := range info.GroupStats { - c.healGroupPolicies(ctx, objAPI, group, info, true) + c.healGroupPolicies(ctx, objAPI, group, info) } return nil @@ -4394,7 +4376,7 @@ func (c *SiteReplicationSys) healPolicies(ctx context.Context, objAPI ObjectLaye } // heal user policy mappings present on this site to peers, provided current cluster has the most recent update. -func (c *SiteReplicationSys) healUserPolicies(ctx context.Context, objAPI ObjectLayer, user string, info srStatusInfo, svcAcct bool) error { +func (c *SiteReplicationSys) healUserPolicies(ctx context.Context, objAPI ObjectLayer, user string, info srStatusInfo) error { // create user policy mapping on peer cluster if missing us := info.UserStats[user] @@ -4454,7 +4436,7 @@ func (c *SiteReplicationSys) healUserPolicies(ctx context.Context, objAPI Object } // heal group policy mappings present on this site to peers, provided current cluster has the most recent update. -func (c *SiteReplicationSys) healGroupPolicies(ctx context.Context, objAPI ObjectLayer, group string, info srStatusInfo, svcAcct bool) error { +func (c *SiteReplicationSys) healGroupPolicies(ctx context.Context, objAPI ObjectLayer, group string, info srStatusInfo) error { // create group policy mapping on peer cluster if missing gs := info.GroupStats[group] diff --git a/go.mod b/go.mod index e05c42593..8b501520e 100644 --- a/go.mod +++ b/go.mod @@ -48,7 +48,7 @@ require ( github.com/minio/dperf v0.4.2 github.com/minio/highwayhash v1.0.2 github.com/minio/kes v0.20.0 - github.com/minio/madmin-go v1.4.23 + github.com/minio/madmin-go v1.4.24 github.com/minio/minio-go/v7 v7.0.34 github.com/minio/pkg v1.3.0 github.com/minio/selfupdate v0.5.0 diff --git a/go.sum b/go.sum index bee6454f3..ad3cf087a 100644 --- a/go.sum +++ b/go.sum @@ -623,8 +623,8 @@ github.com/minio/highwayhash v1.0.2/go.mod h1:BQskDq+xkJ12lmlUUi7U0M5Swg3EWR+dLT github.com/minio/kes v0.20.0 h1:1tyC51Rr8zTregTESuT/QN/iebNMX7B9t7d3xLNMEpE= github.com/minio/kes v0.20.0/go.mod h1:3FW1BQkMGQW78yhy+69tUq5bdcf5rnXJizyeKB9a/tc= github.com/minio/madmin-go v1.3.5/go.mod h1:vGKGboQgGIWx4DuDUaXixjlIEZOCIp6ivJkQoiVaACc= -github.com/minio/madmin-go v1.4.23 h1:t36fg3htwB9RRa4a8I+47OZn2NgfA9T83zOjR9LuFsc= -github.com/minio/madmin-go v1.4.23/go.mod h1:ez87VmMtsxP7DRxjKJKD4RDNW+nhO2QF9KSzwxBDQ98= +github.com/minio/madmin-go v1.4.24 h1:AFn8/N/E64RUXV4ztGwQcmsKb2uTlwSwiGyk6B37PWY= +github.com/minio/madmin-go v1.4.24/go.mod h1:ez87VmMtsxP7DRxjKJKD4RDNW+nhO2QF9KSzwxBDQ98= github.com/minio/mc v0.0.0-20220818165341-8c239d16aa37 h1:UE9kHgdza2HB4DDx0nIn0jHRl6YASUomVoKjciMfqjM= github.com/minio/mc v0.0.0-20220818165341-8c239d16aa37/go.mod h1:O3pvs5/n57bZ0mpcG/skBuD4pHX6m+wVbW7lw8LO5go= github.com/minio/md5-simd v1.1.0/go.mod h1:XpBqgZULrMYD3R+M28PcmP0CkI7PEMzB3U77ZrKZ0Gw=