From 1b122526aa1de67b22b6e97d6da75b8594f0b5ef Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 28 Apr 2020 12:49:56 -0700 Subject: [PATCH] fix: add service account support for AssumeRole/LDAPIdentity creds (#9451) allow generating service accounts for temporary credentials which have a designated parent, currently OpenID is not yet supported. added checks to ensure that service account cannot generate further service accounts for itself, service accounts can never be a parent to any credential. --- cmd/admin-handlers-users.go | 9 ++++- cmd/iam.go | 77 ++++++++++++++++++------------------- cmd/sts-handlers.go | 26 +++++++++---- pkg/auth/credentials.go | 2 +- 4 files changed, 65 insertions(+), 49 deletions(-) diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 256251a7d..236eb068e 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -397,7 +397,12 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque return } - newCred, err := globalIAMSys.NewServiceAccount(ctx, cred.AccessKey, createReq.Policy) + parentUser := cred.AccessKey + if cred.ParentUser != "" { + parentUser = cred.ParentUser + } + + newCred, err := globalIAMSys.NewServiceAccount(ctx, parentUser, createReq.Policy) if err != nil { writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return @@ -516,7 +521,7 @@ func (a adminAPIHandlers) DeleteServiceAccount(w http.ResponseWriter, r *http.Re return } - if cred.AccessKey != user { + if cred.AccessKey != user || cred.ParentUser != user { // The service account belongs to another user but return not found error to mitigate brute force attacks. writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrServiceAccountNotFound), r.URL) return diff --git a/cmd/iam.go b/cmd/iam.go index c5e2088c0..e70c99b4f 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -145,8 +145,8 @@ type UserIdentity struct { Credentials auth.Credentials `json:"credentials"` } -func newUserIdentity(creds auth.Credentials) UserIdentity { - return UserIdentity{Version: 1, Credentials: creds} +func newUserIdentity(cred auth.Credentials) UserIdentity { + return UserIdentity{Version: 1, Credentials: cred} } // GroupInfo contains info about a group @@ -440,7 +440,7 @@ func (sys *IAMSys) DeletePolicy(policyName string) error { // This case cannot happen return errNoSuchUser } - // User is from STS if the creds are temporary + // User is from STS if the cred are temporary if cr.IsTemp() { usersType = append(usersType, stsUser) } else { @@ -553,6 +553,16 @@ func (sys *IAMSys) DeleteUser(accessKey string) error { return errIAMActionNotAllowed } + // Delete any service accounts if any first. + for _, u := range sys.iamUsersMap { + if u.IsServiceAccount() { + if u.ParentUser == accessKey { + _ = sys.store.deleteUserIdentity(u.AccessKey, srvAccUser) + delete(sys.iamUsersMap, u.AccessKey) + } + } + } + // It is ok to ignore deletion error on the mapped policy sys.store.deleteMappedPolicy(accessKey, regularUser, false) err := sys.store.deleteUserIdentity(accessKey, regularUser) @@ -564,15 +574,6 @@ func (sys *IAMSys) DeleteUser(accessKey string) error { delete(sys.iamUsersMap, accessKey) delete(sys.iamUserPolicyMap, accessKey) - for _, u := range sys.iamUsersMap { - if u.IsServiceAccount() { - if u.ParentUser == accessKey { - _ = sys.store.deleteUserIdentity(u.AccessKey, srvAccUser) - delete(sys.iamUsersMap, u.AccessKey) - } - } - } - return err } @@ -659,12 +660,12 @@ func (sys *IAMSys) IsTempUser(name string) (bool, error) { sys.store.rlock() defer sys.store.runlock() - creds, found := sys.iamUsersMap[name] + cred, found := sys.iamUsersMap[name] if !found { return false, errNoSuchUser } - return creds.IsTemp(), nil + return cred.IsTemp(), nil } // IsServiceAccount - returns if given key is a service account @@ -677,13 +678,13 @@ func (sys *IAMSys) IsServiceAccount(name string) (bool, string, error) { sys.store.rlock() defer sys.store.runlock() - creds, found := sys.iamUsersMap[name] + cred, found := sys.iamUsersMap[name] if !found { return false, "", errNoSuchUser } - if creds.IsServiceAccount() { - return true, creds.ParentUser, nil + if cred.IsServiceAccount() { + return true, cred.ParentUser, nil } return false, "", nil @@ -713,19 +714,19 @@ func (sys *IAMSys) GetUserInfo(name string) (u madmin.UserInfo, err error) { }, nil } - creds, found := sys.iamUsersMap[name] + cred, found := sys.iamUsersMap[name] if !found { return u, errNoSuchUser } - if creds.IsTemp() { + if cred.IsTemp() || cred.IsServiceAccount() { return u, errIAMActionNotAllowed } u = madmin.UserInfo{ PolicyName: sys.iamUserPolicyMap[name].Policy, Status: func() madmin.AccountStatus { - if creds.IsValid() { + if cred.IsValid() { return madmin.AccountEnabled } return madmin.AccountDisabled @@ -758,7 +759,7 @@ func (sys *IAMSys) SetUserStatus(accessKey string, status madmin.AccountStatus) return errNoSuchUser } - if cred.IsTemp() { + if cred.IsTemp() || cred.IsServiceAccount() { return errIAMActionNotAllowed } @@ -806,10 +807,6 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, ses sys.store.lock() defer sys.store.unlock() - if sys.usersSysType != MinIOUsersSysType { - return auth.Credentials{}, errIAMActionNotAllowed - } - if parentUser == globalActiveCred.AccessKey { return auth.Credentials{}, errIAMActionNotAllowed } @@ -819,7 +816,16 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, ses return auth.Credentials{}, errNoSuchUser } - if cr.IsTemp() { + // Disallow service accounts to further create more service accounts. + if cr.IsServiceAccount() { + return auth.Credentials{}, errIAMActionNotAllowed + } + + // FIXME: Disallow temporary users with no parent, this is most + // probably with OpenID which we don't support to provide + // any parent user, LDAPUsersType and MinIOUsersType are + // currently supported. + if cr.ParentUser == "" && cr.IsTemp() { return auth.Credentials{}, errIAMActionNotAllowed } @@ -838,8 +844,8 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, ses if err != nil { return auth.Credentials{}, err } - cred.ParentUser = parentUser + u := newUserIdentity(cred) if err := sys.store.saveUserIdentity(u.Credentials.AccessKey, srvAccUser, u); err != nil { @@ -861,10 +867,6 @@ func (sys *IAMSys) ListServiceAccounts(ctx context.Context, accessKey string) ([ sys.store.rlock() defer sys.store.runlock() - if sys.usersSysType != MinIOUsersSysType { - return nil, errIAMActionNotAllowed - } - var serviceAccounts []string for k, v := range sys.iamUsersMap { @@ -886,10 +888,6 @@ func (sys *IAMSys) GetServiceAccountParent(ctx context.Context, accessKey string sys.store.rlock() defer sys.store.runlock() - if sys.usersSysType != MinIOUsersSysType { - return "", errIAMActionNotAllowed - } - sa, ok := sys.iamUsersMap[accessKey] if !ok || !sa.IsServiceAccount() { return "", errNoSuchServiceAccount @@ -908,10 +906,6 @@ func (sys *IAMSys) DeleteServiceAccount(ctx context.Context, accessKey string) e sys.store.lock() defer sys.store.unlock() - if sys.usersSysType != MinIOUsersSysType { - return errIAMActionNotAllowed - } - sa, ok := sys.iamUsersMap[accessKey] if !ok || !sa.IsServiceAccount() { return errNoSuchServiceAccount @@ -1009,6 +1003,11 @@ func (sys *IAMSys) GetUser(accessKey string) (cred auth.Credentials, ok bool) { defer sys.store.runlock() cred, ok = sys.iamUsersMap[accessKey] + if ok && cred.IsValid() { + if cred.ParentUser != "" { + _, ok = sys.iamUsersMap[cred.ParentUser] + } + } return cred, ok && cred.IsValid() } diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go index d3f761d60..6645807b3 100644 --- a/cmd/sts-handlers.go +++ b/cmd/sts-handlers.go @@ -22,6 +22,7 @@ import ( "encoding/base64" "fmt" "net/http" + "strings" "github.com/gorilla/mux" "github.com/minio/minio/cmd/config/identity/openid" @@ -135,6 +136,11 @@ func checkAssumeRoleAuth(ctx context.Context, r *http.Request) (user auth.Creden return user, ErrSTSAccessDenied } + // Temporary credentials or Service accounts cannot generate further temporary credentials. + if user.IsTemp() || user.IsServiceAccount() { + return user, ErrSTSAccessDenied + } + return user, ErrSTSNone } @@ -207,10 +213,7 @@ func (sts *stsAPIHandlers) AssumeRole(w http.ResponseWriter, r *http.Request) { return } - policyName := "" - if len(policies) > 0 { - policyName = policies[0] - } + policyName := strings.Join(policies, ",") // This policy is the policy associated with the user // requesting for temporary credentials. The temporary @@ -228,6 +231,10 @@ func (sts *stsAPIHandlers) AssumeRole(w http.ResponseWriter, r *http.Request) { return } + // Set the parent of the temporary access key, this is useful + // in obtaining service accounts by this cred. + cred.ParentUser = user.AccessKey + // Set the newly generated credentials. if err = globalIAMSys.SetTempUser(cred.AccessKey, cred, policyName); err != nil { writeSTSErrorResponse(ctx, w, ErrSTSInternalError, err) @@ -500,9 +507,14 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r * return } - policyName := "" - // Set the newly generated credentials. - if err = globalIAMSys.SetTempUser(cred.AccessKey, cred, policyName); err != nil { + // Set the parent of the temporary access key, this is useful + // in obtaining service accounts by this cred. + cred.ParentUser = ldapUsername + + // Set the newly generated credentials, policyName is empty on purpose + // LDAP policies are applied automatically using their ldapUser, ldapGroups + // mapping. + if err = globalIAMSys.SetTempUser(cred.AccessKey, cred, ""); err != nil { writeSTSErrorResponse(ctx, w, ErrSTSInternalError, err) return } diff --git a/pkg/auth/credentials.go b/pkg/auth/credentials.go index 190a176cc..63bc5de4d 100644 --- a/pkg/auth/credentials.go +++ b/pkg/auth/credentials.go @@ -120,7 +120,7 @@ func (cred Credentials) IsExpired() bool { // IsTemp - returns whether credential is temporary or not. func (cred Credentials) IsTemp() bool { - return cred.SessionToken != "" && cred.ParentUser == "" && !cred.Expiration.IsZero() && !cred.Expiration.Equal(timeSentinel) + return cred.SessionToken != "" && !cred.Expiration.IsZero() && !cred.Expiration.Equal(timeSentinel) } // IsServiceAccount - returns whether credential is a service account or not