Fix locking in policy attach API (#17426)

For policy attach/detach API to work correctly the server should hold a
lock before reading existing policy mapping and until after writing the
updated policy mapping. This is fixed in this change.

A site replication bug, where LDAP policy attach/detach were not
correctly propagated is also fixed in this change.

Bonus: Additionally, the server responds with the actual (or net)
changes performed in the attach/detach API call. For e.g. if a user
already has policy A applied, and a call to attach policies A and B is
performed, the server will respond that B was attached successfully.
This commit is contained in:
Aditya Manthramurthy 2023-06-21 22:44:50 -07:00 committed by GitHub
parent 9af6c6ceef
commit 82ce78a17c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 163 additions and 269 deletions

View file

@ -150,7 +150,7 @@ func (a adminAPIHandlers) AttachDetachPolicyLDAP(w http.ResponseWriter, r *http.
}
// Call IAM subsystem
updatedAt, addedOrRemoved, err := globalIAMSys.PolicyDBUpdateLDAP(ctx, isAttach, par)
updatedAt, addedOrRemoved, _, err := globalIAMSys.PolicyDBUpdateLDAP(ctx, isAttach, par)
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return

View file

@ -26,7 +26,6 @@ import (
"net/http"
"os"
"sort"
"strings"
"time"
"github.com/klauspost/compress/zip"
@ -1771,24 +1770,40 @@ func (a adminAPIHandlers) ListPolicyMappingEntities(w http.ResponseWriter, r *ht
writeSuccessResponseJSON(w, econfigData)
}
// AttachPolicyBuiltin - POST /minio/admin/v3/idp/builtin/policy/attach
func (a adminAPIHandlers) AttachPolicyBuiltin(w http.ResponseWriter, r *http.Request) {
// AttachDetachPolicyBuiltin - POST /minio/admin/v3/idp/builtin/policy/{operation}
func (a adminAPIHandlers) AttachDetachPolicyBuiltin(w http.ResponseWriter, r *http.Request) {
ctx := newContext(r, w, "AttachPolicyBuiltin")
defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
objectAPI, _ := validateAdminReq(ctx, w, r, iampolicy.AttachPolicyAdminAction)
objectAPI, cred := validateAdminReq(ctx, w, r, iampolicy.UpdatePolicyAssociationAction,
iampolicy.AttachPolicyAdminAction)
if objectAPI == nil {
return
}
cred, _, s3Err := validateAdminSignature(ctx, r, "")
if s3Err != ErrNone {
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL)
if r.ContentLength > maxEConfigJSONSize || r.ContentLength == -1 {
// More than maxConfigSize bytes were available
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigTooLarge), r.URL)
return
}
password := cred.SecretKey
// Ensure body content type is opaque to ensure that request body has not
// been interpreted as form data.
contentType := r.Header.Get("Content-Type")
if contentType != "application/octet-stream" {
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL)
return
}
operation := mux.Vars(r)["operation"]
if operation != "attach" && operation != "detach" {
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL)
return
}
isAttach := operation == "attach"
password := cred.SecretKey
reqBytes, err := madmin.DecryptData(password, io.LimitReader(r.Body, r.ContentLength))
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
@ -1806,235 +1821,42 @@ func (a adminAPIHandlers) AttachPolicyBuiltin(w http.ResponseWriter, r *http.Req
return
}
userOrGroup := par.User
isGroup := false
if userOrGroup == "" {
userOrGroup = par.Group
isGroup = true
}
if isGroup {
_, err := globalIAMSys.GetGroupDescription(userOrGroup)
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
} else {
ok, _, err := globalIAMSys.IsTempUser(userOrGroup)
if err != nil && err != errNoSuchUser {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
if ok {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errIAMActionNotAllowed), r.URL)
return
}
// When the user is root credential you are not allowed to
// add policies for root user.
if userOrGroup == globalActiveCred.AccessKey {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errIAMActionNotAllowed), r.URL)
return
}
// Validate that user exists.
_, ok = globalIAMSys.GetUser(ctx, userOrGroup)
if !ok {
updatedAt, addedOrRemoved, _, err := globalIAMSys.PolicyDBUpdateBuiltin(ctx, isAttach, par)
if err != nil {
if err == errNoSuchUser || err == errNoSuchGroup {
if globalIAMSys.LDAPConfig.Enabled() {
// When LDAP is enabled, warn user that they are using the wrong
// API.
// API. FIXME: error can be no such group as well - fix errNoSuchUserLDAPWarn
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errNoSuchUserLDAPWarn), r.URL)
return
}
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errNoSuchUser), r.URL)
return
}
}
var existingPolicies []string
if isGroup {
existingPolicies, err = globalIAMSys.PolicyDBGet(userOrGroup, true)
} else {
existingPolicies, err = globalIAMSys.GetUserPolicies(userOrGroup)
}
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
policyMap := make(map[string]bool)
for _, p := range existingPolicies {
policyMap[p] = true
}
policiesToAttach := par.Policies
// Check if policy is already attached to user.
for _, p := range policiesToAttach {
if _, ok := policyMap[p]; ok {
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrPolicyAlreadyAttached), r.URL)
return
}
}
existingPolicies = append(existingPolicies, policiesToAttach...)
newPolicies := strings.Join(existingPolicies, ",")
userType := regUser
updatedAt, err := globalIAMSys.PolicyDBSet(ctx, userOrGroup, newPolicies, userType, isGroup)
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
logger.LogIf(ctx, globalSiteReplicationSys.IAMChangeHook(ctx, madmin.SRIAMItem{
Type: madmin.SRIAMItemPolicyMapping,
PolicyMapping: &madmin.SRPolicyMapping{
UserOrGroup: userOrGroup,
UserType: int(userType),
IsGroup: isGroup,
Policy: strings.Join(policiesToAttach, ","),
},
respBody := madmin.PolicyAssociationResp{
UpdatedAt: updatedAt,
}))
writeResponse(w, http.StatusCreated, nil, mimeNone)
}
// DetachPolicyBuiltin - POST /minio/admin/v3/idp/builtin/policy/detach
func (a adminAPIHandlers) DetachPolicyBuiltin(w http.ResponseWriter, r *http.Request) {
ctx := newContext(r, w, "DetachPolicyBuiltin")
defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
objectAPI, _ := validateAdminReq(ctx, w, r, iampolicy.AttachPolicyAdminAction)
if objectAPI == nil {
return
}
cred, _, s3Err := validateAdminSignature(ctx, r, "")
if s3Err != ErrNone {
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL)
return
}
password := cred.SecretKey
reqBytes, err := madmin.DecryptData(password, io.LimitReader(r.Body, r.ContentLength))
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
var par madmin.PolicyAssociationReq
if err = json.Unmarshal(reqBytes, &par); err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
if err = par.IsValid(); err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
userOrGroup := par.User
isGroup := false
if userOrGroup == "" {
userOrGroup = par.Group
isGroup = true
}
if isGroup {
_, err := globalIAMSys.GetGroupDescription(userOrGroup)
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
if isAttach {
respBody.PoliciesAttached = addedOrRemoved
} else {
ok, _, err := globalIAMSys.IsTempUser(userOrGroup)
if err != nil && err != errNoSuchUser {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
if ok {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errIAMActionNotAllowed), r.URL)
return
}
// Validate that user exists.
_, ok = globalIAMSys.GetUser(ctx, userOrGroup)
if !ok {
if globalIAMSys.LDAPConfig.Enabled() {
// When LDAP is enabled, warn user that they are using the wrong
// API.
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errNoSuchUserLDAPWarn), r.URL)
return
}
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errNoSuchUser), r.URL)
return
}
respBody.PoliciesDetached = addedOrRemoved
}
userType := regUser
if globalIAMSys.GetUsersSysType() == LDAPUsersSysType {
userType = stsUser
}
var existingPolicies []string
if isGroup {
existingPolicies, err = globalIAMSys.PolicyDBGet(userOrGroup, true)
} else {
existingPolicies, err = globalIAMSys.GetUserPolicies(userOrGroup)
}
data, err := json.Marshal(respBody)
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
policyMap := make(map[string]bool)
for _, p := range existingPolicies {
policyMap[p] = true
}
policiesToDetach := par.Policies
// Check if policy is already attached to user.
for _, p := range policiesToDetach {
if _, ok := policyMap[p]; ok {
delete(policyMap, p)
} else {
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrPolicyNotAttached), r.URL)
return
}
}
newPoliciesSl := []string{}
for p := range policyMap {
newPoliciesSl = append(newPoliciesSl, p)
}
newPolicies := strings.Join(newPoliciesSl, ",")
updatedAt, err := globalIAMSys.PolicyDBSet(ctx, userOrGroup, newPolicies, userType, isGroup)
encryptedData, err := madmin.EncryptData(password, data)
if err != nil {
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
return
}
logger.LogIf(ctx, globalSiteReplicationSys.IAMChangeHook(ctx, madmin.SRIAMItem{
Type: madmin.SRIAMItemPolicyMapping,
PolicyMapping: &madmin.SRPolicyMapping{
UserOrGroup: userOrGroup,
UserType: int(userType),
IsGroup: isGroup,
Policy: newPolicies,
},
UpdatedAt: updatedAt,
}))
// Return successful JSON response
writeSuccessNoContent(w)
writeSuccessResponseJSON(w, encryptedData)
}
const (

View file

@ -162,11 +162,8 @@ func registerAdminRouter(router *mux.Router, enableConfigOps bool) {
HandlerFunc(gz(httpTraceHdrs(adminAPI.SetPolicyForUserOrGroup))).
Queries("policyName", "{policyName:.*}", "userOrGroup", "{userOrGroup:.*}", "isGroup", "{isGroup:true|false}")
// Attach policies to user or group
adminRouter.Methods(http.MethodPost).Path(adminVersion + "/idp/builtin/policy/attach").HandlerFunc(gz(httpTraceHdrs(adminAPI.AttachPolicyBuiltin)))
// Detach policies from user or group
adminRouter.Methods(http.MethodPost).Path(adminVersion + "/idp/builtin/policy/detach").HandlerFunc(gz(httpTraceHdrs(adminAPI.DetachPolicyBuiltin)))
// Attach/Detach policies to/from user or group
adminRouter.Methods(http.MethodPost).Path(adminVersion + "/idp/builtin/policy/{operation}").HandlerFunc(gz(httpTraceHdrs(adminAPI.AttachDetachPolicyBuiltin)))
// Remove user IAM
adminRouter.Methods(http.MethodDelete).Path(adminVersion+"/remove-user").HandlerFunc(gz(httpTraceHdrs(adminAPI.RemoveUser))).Queries("accessKey", "{accessKey:.*}")

View file

@ -914,11 +914,12 @@ func (store *IAMStoreSys) listGroups(ctx context.Context) (res []string, err err
// PolicyDBUpdate - adds or removes given policies to/from the user or group's
// policy associations.
func (store *IAMStoreSys) PolicyDBUpdate(ctx context.Context, name string, isGroup bool,
userType IAMUserType, policies []string, isAttach bool) (updatedAt time.Time, addedOrRemoved []string,
err error,
userType IAMUserType, policies []string, isAttach bool) (updatedAt time.Time,
addedOrRemoved, effectivePolicies []string, err error,
) {
if name == "" {
return updatedAt, nil, errInvalidArgument
err = errInvalidArgument
return
}
cache := store.lock()
@ -932,11 +933,13 @@ func (store *IAMStoreSys) PolicyDBUpdate(ctx context.Context, name string, isGro
if store.getUsersSysType() == MinIOUsersSysType {
g, ok := cache.iamGroupsMap[name]
if !ok {
return updatedAt, nil, errNoSuchGroup
err = errNoSuchGroup
return
}
if g.Status == statusDisabled {
return updatedAt, nil, errGroupDisabled
err = errGroupDisabled
return
}
}
mp = cache.iamGroupPolicyMap[name]
@ -945,6 +948,7 @@ func (store *IAMStoreSys) PolicyDBUpdate(ctx context.Context, name string, isGro
// Compute net policy change effect and updated policy mapping
existingPolicySet := mp.policySet()
policiesToUpdate := set.CreateStringSet(policies...)
var newPolicySet set.StringSet
newPolicyMapping := mp
if isAttach {
// new policies to attach => inputPolicies - existing (set difference)
@ -952,26 +956,29 @@ func (store *IAMStoreSys) PolicyDBUpdate(ctx context.Context, name string, isGro
// validate that new policies to add are defined.
for _, p := range policiesToUpdate.ToSlice() {
if _, found := cache.iamPolicyDocsMap[p]; !found {
return updatedAt, nil, errNoSuchPolicy
err = errNoSuchPolicy
return
}
}
newPolicyMapping.Policies = strings.Join(existingPolicySet.Union(policiesToUpdate).ToSlice(), ",")
newPolicySet = existingPolicySet.Union(policiesToUpdate)
} else {
// policies to detach => inputPolicies ∩ existing (intersection)
policiesToUpdate = policiesToUpdate.Intersection(existingPolicySet)
newPolicyMapping.Policies = strings.Join(existingPolicySet.Difference(policiesToUpdate).ToSlice(), ",")
newPolicySet = existingPolicySet.Difference(policiesToUpdate)
}
newPolicyMapping.UpdatedAt = UTCNow()
// We return an error if the requested policy update will have no effect.
if policiesToUpdate.IsEmpty() {
return updatedAt, nil, errNoPolicyToAttachOrDetach
err = errNoPolicyToAttachOrDetach
return
}
newPolicies := newPolicySet.ToSlice()
newPolicyMapping.Policies = strings.Join(newPolicies, ",")
newPolicyMapping.UpdatedAt = UTCNow()
addedOrRemoved = policiesToUpdate.ToSlice()
if err := store.saveMappedPolicy(ctx, name, userType, isGroup, newPolicyMapping); err != nil {
return updatedAt, addedOrRemoved, err
if err = store.saveMappedPolicy(ctx, name, userType, isGroup, newPolicyMapping); err != nil {
return
}
if !isGroup {
cache.iamUserPolicyMap[name] = newPolicyMapping
@ -979,7 +986,7 @@ func (store *IAMStoreSys) PolicyDBUpdate(ctx context.Context, name string, isGro
cache.iamGroupPolicyMap[name] = newPolicyMapping
}
cache.updatedAt = UTCNow()
return cache.updatedAt, addedOrRemoved, nil
return cache.updatedAt, addedOrRemoved, newPolicies, nil
}
// PolicyDBSet - update the policy mapping for the given user or group in
@ -1451,23 +1458,6 @@ func (store *IAMStoreSys) GetUserInfo(name string) (u madmin.UserInfo, err error
}, nil
}
// GetUserPolicies - returns the policies attached to a user.
func (store *IAMStoreSys) GetUserPolicies(name string) ([]string, error) {
if name == "" {
return nil, errInvalidArgument
}
cache := store.rlock()
defer store.runlock()
if cache.iamUserPolicyMap[name].Policies == "" {
return []string{}, nil
}
policies := cache.iamUserPolicyMap[name].toSlice()
return policies, nil
}
// PolicyMappingNotificationHandler - handles updating a policy mapping from storage.
func (store *IAMStoreSys) PolicyMappingNotificationHandler(ctx context.Context, userOrGroup string, isGroup bool, userType IAMUserType) error {
if userOrGroup == "" {

View file

@ -813,7 +813,7 @@ func (sys *IAMSys) QueryLDAPPolicyEntities(ctx context.Context, q madmin.PolicyE
}
}
// IsTempUser - returns if given key is a temporary user.
// IsTempUser - returns if given key is a temporary user and parent user.
func (sys *IAMSys) IsTempUser(name string) (bool, string, error) {
if !sys.Initialized() {
return false, "", errServerNotInitialized
@ -889,15 +889,6 @@ func (sys *IAMSys) QueryPolicyEntities(ctx context.Context, q madmin.PolicyEntit
}
}
// GetUserPolicies - get policies attached to a user.
func (sys *IAMSys) GetUserPolicies(name string) (p []string, err error) {
if !sys.Initialized() {
return p, errServerNotInitialized
}
return sys.store.GetUserPolicies(name)
}
// SetUserStatus - sets current user status, supports disabled or enabled.
func (sys *IAMSys) SetUserStatus(ctx context.Context, accessKey string, status madmin.AccountStatus) (updatedAt time.Time, err error) {
if !sys.Initialized() {
@ -1582,13 +1573,93 @@ func (sys *IAMSys) PolicyDBSet(ctx context.Context, name, policy string, userTyp
return updatedAt, nil
}
// PolicyDBUpdateBuiltin - adds or removes policies from a user or a group
// verified to be an internal IDP user.
func (sys *IAMSys) PolicyDBUpdateBuiltin(ctx context.Context, isAttach bool,
r madmin.PolicyAssociationReq,
) (updatedAt time.Time, addedOrRemoved, effectivePolicies []string, err error) {
if !sys.Initialized() {
err = errServerNotInitialized
return
}
userOrGroup := r.User
var isGroup bool
if userOrGroup == "" {
isGroup = true
userOrGroup = r.Group
}
if isGroup {
_, err = sys.GetGroupDescription(userOrGroup)
if err != nil {
return
}
} else {
var isTemp bool
isTemp, _, err = sys.IsTempUser(userOrGroup)
if err != nil && err != errNoSuchUser {
return
}
if isTemp {
err = errIAMActionNotAllowed
return
}
// When the user is root credential you are not allowed to
// add policies for root user.
if userOrGroup == globalActiveCred.AccessKey {
err = errIAMActionNotAllowed
return
}
// Validate that user exists.
var userExists bool
_, userExists = sys.GetUser(ctx, userOrGroup)
if !userExists {
err = errNoSuchUser
return
}
}
updatedAt, addedOrRemoved, effectivePolicies, err = sys.store.PolicyDBUpdate(ctx, userOrGroup, isGroup,
regUser, r.Policies, isAttach)
if err != nil {
return
}
// Notify all other MinIO peers to reload policy
if !sys.HasWatcher() {
for _, nerr := range globalNotificationSys.LoadPolicyMapping(userOrGroup, regUser, isGroup) {
if nerr.Err != nil {
logger.GetReqInfo(ctx).SetTags("peerAddress", nerr.Host.String())
logger.LogIf(ctx, nerr.Err)
}
}
}
logger.LogIf(ctx, globalSiteReplicationSys.IAMChangeHook(ctx, madmin.SRIAMItem{
Type: madmin.SRIAMItemPolicyMapping,
PolicyMapping: &madmin.SRPolicyMapping{
UserOrGroup: userOrGroup,
UserType: int(regUser),
IsGroup: isGroup,
Policy: strings.Join(effectivePolicies, ","),
},
UpdatedAt: updatedAt,
}))
return
}
// PolicyDBUpdateLDAP - adds or removes policies from a user or a group verified
// to be in the LDAP directory.
func (sys *IAMSys) PolicyDBUpdateLDAP(ctx context.Context, isAttach bool,
r madmin.PolicyAssociationReq,
) (updatedAt time.Time, addedOrRemoved []string, err error) {
) (updatedAt time.Time, addedOrRemoved, effectivePolicies []string, err error) {
if !sys.Initialized() {
return updatedAt, nil, errServerNotInitialized
err = errServerNotInitialized
return
}
var dn string
@ -1597,28 +1668,31 @@ func (sys *IAMSys) PolicyDBUpdateLDAP(ctx context.Context, isAttach bool,
dn, err = sys.LDAPConfig.DoesUsernameExist(r.User)
if err != nil {
logger.LogIf(ctx, err)
return updatedAt, nil, err
return
}
if dn == "" {
return updatedAt, nil, errNoSuchUser
err = errNoSuchUser
return
}
isGroup = false
} else {
if exists, err := sys.LDAPConfig.DoesGroupDNExist(r.Group); err != nil {
var exists bool
if exists, err = sys.LDAPConfig.DoesGroupDNExist(r.Group); err != nil {
logger.LogIf(ctx, err)
return updatedAt, nil, err
return
} else if !exists {
return updatedAt, nil, errNoSuchGroup
err = errNoSuchGroup
return
}
dn = r.Group
isGroup = true
}
userType := stsUser
updatedAt, addedOrRemoved, err = sys.store.PolicyDBUpdate(ctx, dn, isGroup,
updatedAt, addedOrRemoved, effectivePolicies, err = sys.store.PolicyDBUpdate(ctx, dn, isGroup,
userType, r.Policies, isAttach)
if err != nil {
return updatedAt, nil, err
return
}
// Notify all other MinIO peers to reload policy
@ -1631,7 +1705,18 @@ func (sys *IAMSys) PolicyDBUpdateLDAP(ctx context.Context, isAttach bool,
}
}
return updatedAt, addedOrRemoved, nil
logger.LogIf(ctx, globalSiteReplicationSys.IAMChangeHook(ctx, madmin.SRIAMItem{
Type: madmin.SRIAMItemPolicyMapping,
PolicyMapping: &madmin.SRPolicyMapping{
UserOrGroup: dn,
UserType: int(userType),
IsGroup: isGroup,
Policy: strings.Join(effectivePolicies, ","),
},
UpdatedAt: updatedAt,
}))
return
}
// PolicyDBGet - gets policy set on a user or group. If a list of groups is