From 7cedc5369d7a4bbe91ccb92a9d0b706ce0252138 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 27 May 2020 12:38:44 -0700 Subject: [PATCH] fix: send valid claims in AuditLogs for browser requests (#9713) Additionally also fix STS logs to filter out LDAP password to be sent out in audit logs. Bonus fix handle the reload of users properly by making sure to preserve the newer users during the reload to be not invalidated. Fixes #9707 Fixes #9644 Fixes #9651 --- cmd/iam-etcd-store.go | 45 +++++++++++++++++++++++++++++++++-------- cmd/iam-object-store.go | 45 +++++++++++++++++++++++++++++++++-------- cmd/jwt/parser.go | 6 ++++++ cmd/sts-handlers.go | 15 +++++++++++--- cmd/web-handlers.go | 24 +++++++++++++--------- 5 files changed, 106 insertions(+), 29 deletions(-) diff --git a/cmd/iam-etcd-store.go b/cmd/iam-etcd-store.go index 64f165ffe..8916ae894 100644 --- a/cmd/iam-etcd-store.go +++ b/cmd/iam-etcd-store.go @@ -494,17 +494,46 @@ func (ies *IAMEtcdStore) loadAll(ctx context.Context, sys *IAMSys) error { return err } - // Sets default canned policies, if none are set. - setDefaultCannedPolicies(iamPolicyDocsMap) - ies.lock() defer ies.Unlock() - sys.iamUsersMap = iamUsersMap - sys.iamGroupsMap = iamGroupsMap - sys.iamUserPolicyMap = iamUserPolicyMap - sys.iamPolicyDocsMap = iamPolicyDocsMap - sys.iamGroupPolicyMap = iamGroupPolicyMap + // Merge the new reloaded entries into global map. + // See issue https://github.com/minio/minio/issues/9651 + // where the present list of entries on disk are not yet + // latest, there is a small window where this can make + // valid users invalid. + for k, v := range iamUsersMap { + sys.iamUsersMap[k] = v + } + + for k, v := range iamPolicyDocsMap { + sys.iamPolicyDocsMap[k] = v + } + + // Sets default canned policies, if none are set. + setDefaultCannedPolicies(sys.iamPolicyDocsMap) + + for k, v := range iamUserPolicyMap { + sys.iamUserPolicyMap[k] = v + } + + // purge any expired entries which became expired now. + for k, v := range sys.iamUsersMap { + if v.IsExpired() { + delete(sys.iamUsersMap, k) + delete(sys.iamUserPolicyMap, k) + // Deleting on the etcd is taken care of in the next cycle + } + } + + for k, v := range iamGroupPolicyMap { + sys.iamGroupPolicyMap[k] = v + } + + for k, v := range iamGroupsMap { + sys.iamGroupsMap[k] = v + } + sys.buildUserGroupMemberships() return nil diff --git a/cmd/iam-object-store.go b/cmd/iam-object-store.go index 8f944e1a1..85a4b619d 100644 --- a/cmd/iam-object-store.go +++ b/cmd/iam-object-store.go @@ -457,17 +457,46 @@ func (iamOS *IAMObjectStore) loadAll(ctx context.Context, sys *IAMSys) error { return err } - // Sets default canned policies, if none are set. - setDefaultCannedPolicies(iamPolicyDocsMap) - iamOS.lock() defer iamOS.unlock() - sys.iamUsersMap = iamUsersMap - sys.iamPolicyDocsMap = iamPolicyDocsMap - sys.iamUserPolicyMap = iamUserPolicyMap - sys.iamGroupPolicyMap = iamGroupPolicyMap - sys.iamGroupsMap = iamGroupsMap + // Merge the new reloaded entries into global map. + // See issue https://github.com/minio/minio/issues/9651 + // where the present list of entries on disk are not yet + // latest, there is a small window where this can make + // valid users invalid. + for k, v := range iamUsersMap { + sys.iamUsersMap[k] = v + } + + for k, v := range iamPolicyDocsMap { + sys.iamPolicyDocsMap[k] = v + } + + // Sets default canned policies, if none are set. + setDefaultCannedPolicies(sys.iamPolicyDocsMap) + + for k, v := range iamUserPolicyMap { + sys.iamUserPolicyMap[k] = v + } + + // purge any expired entries which became expired now. + for k, v := range sys.iamUsersMap { + if v.IsExpired() { + delete(sys.iamUsersMap, k) + delete(sys.iamUserPolicyMap, k) + // Deleting on the disk is taken care of in the next cycle + } + } + + for k, v := range iamGroupPolicyMap { + sys.iamGroupPolicyMap[k] = v + } + + for k, v := range iamGroupsMap { + sys.iamGroupsMap[k] = v + } + sys.buildUserGroupMemberships() return nil diff --git a/cmd/jwt/parser.go b/cmd/jwt/parser.go index 85497deef..adbf1a68c 100644 --- a/cmd/jwt/parser.go +++ b/cmd/jwt/parser.go @@ -130,6 +130,9 @@ func NewMapClaims() *MapClaims { // Lookup returns the value and if the key is found. func (c *MapClaims) Lookup(key string) (value string, ok bool) { + if c == nil { + return "", false + } var vinterface interface{} vinterface, ok = c.MapClaims[key] if ok { @@ -167,6 +170,9 @@ func (c *MapClaims) Valid() error { // Map returns underlying low-level map claims. func (c *MapClaims) Map() map[string]interface{} { + if c == nil { + return nil + } return c.MapClaims } diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go index 16b193193..1dcc48b9e 100644 --- a/cmd/sts-handlers.go +++ b/cmd/sts-handlers.go @@ -173,7 +173,7 @@ func (sts *stsAPIHandlers) AssumeRole(w http.ResponseWriter, r *http.Request) { } ctx = newContext(r, w, action) - defer logger.AuditLog(w, r, action, nil) + defer stsAuditLog(w, r, action) sessionPolicyStr := r.Form.Get(stsPolicy) // https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html @@ -258,6 +258,15 @@ func (sts *stsAPIHandlers) AssumeRole(w http.ResponseWriter, r *http.Request) { writeSuccessResponseXML(w, encodeResponse(assumeRoleResponse)) } +func stsAuditLog(w http.ResponseWriter, r *http.Request, action string) { + for _, k := range []string{ + stsLDAPPassword, // cleanup any passwords before sending to audit logs. + } { + r.URL.Query().Del(k) + } + logger.AuditLog(w, r, action, nil) +} + func (sts *stsAPIHandlers) AssumeRoleWithJWT(w http.ResponseWriter, r *http.Request) { ctx := newContext(r, w, "AssumeRoleJWTCommon") @@ -281,7 +290,7 @@ func (sts *stsAPIHandlers) AssumeRoleWithJWT(w http.ResponseWriter, r *http.Requ } ctx = newContext(r, w, action) - defer logger.AuditLog(w, r, action, nil) + defer stsAuditLog(w, r, action) if globalOpenIDValidators == nil { writeSTSErrorResponse(ctx, w, true, ErrSTSNotInitialized, errServerNotInitialized) @@ -448,7 +457,7 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r * } ctx = newContext(r, w, action) - defer logger.AuditLog(w, r, action, nil) + defer stsAuditLog(w, r, action) ldapUsername := r.Form.Get(stsLDAPUsername) ldapPassword := r.Form.Get(stsLDAPPassword) diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 993a964b0..87192c915 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -944,13 +944,17 @@ func (web *webAPIHandlers) CreateURLToken(r *http.Request, args *WebGenericArgs, func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { ctx := newContext(r, w, "WebUpload") - defer logger.AuditLog(w, r, "WebUpload", mustGetClaimsFromToken(r)) + // obtain the claims here if possible, for audit logging. + claims, owner, authErr := webRequestAuthenticate(r) + + defer logger.AuditLog(w, r, "WebUpload", claims.Map()) objectAPI := web.ObjectAPI() if objectAPI == nil { writeWebErrorResponse(w, errServerNotInitialized) return } + vars := mux.Vars(r) bucket := vars["bucket"] object, err := url.PathUnescape(vars["object"]) @@ -961,8 +965,6 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { retPerms := ErrAccessDenied holdPerms := ErrAccessDenied - - claims, owner, authErr := webRequestAuthenticate(r) if authErr != nil { if authErr == errNoAuthToken { // Check if anonymous (non-owner) has access to upload objects. @@ -1167,7 +1169,10 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { ctx := newContext(r, w, "WebDownload") - defer logger.AuditLog(w, r, "WebDownload", mustGetClaimsFromToken(r)) + vars := mux.Vars(r) + + claims, owner, authErr := webTokenAuthenticate(r.URL.Query().Get("token")) + defer logger.AuditLog(w, r, "WebDownload", claims.Map()) objectAPI := web.ObjectAPI() if objectAPI == nil { @@ -1175,19 +1180,16 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { return } - vars := mux.Vars(r) bucket := vars["bucket"] object, err := url.PathUnescape(vars["object"]) if err != nil { writeWebErrorResponse(w, err) return } - token := r.URL.Query().Get("token") getRetPerms := ErrAccessDenied legalHoldPerms := ErrAccessDenied - claims, owner, authErr := webTokenAuthenticate(token) if authErr != nil { if authErr == errNoAuthToken { // Check if anonymous (non-owner) has access to download objects. @@ -1359,8 +1361,10 @@ type DownloadZipArgs struct { func (web *webAPIHandlers) DownloadZip(w http.ResponseWriter, r *http.Request) { host := handlers.GetSourceIP(r) + claims, owner, authErr := webTokenAuthenticate(r.URL.Query().Get("token")) + ctx := newContext(r, w, "WebDownloadZip") - defer logger.AuditLog(w, r, "WebDownloadZip", mustGetClaimsFromToken(r)) + defer logger.AuditLog(w, r, "WebDownloadZip", claims.Map()) objectAPI := web.ObjectAPI() if objectAPI == nil { @@ -1377,8 +1381,7 @@ func (web *webAPIHandlers) DownloadZip(w http.ResponseWriter, r *http.Request) { writeWebErrorResponse(w, decodeErr) return } - token := r.URL.Query().Get("token") - claims, owner, authErr := webTokenAuthenticate(token) + var getRetPerms []APIErrorCode var legalHoldPerms []APIErrorCode @@ -1592,6 +1595,7 @@ type GetBucketPolicyRep struct { // GetBucketPolicy - get bucket policy for the requested prefix. func (web *webAPIHandlers) GetBucketPolicy(r *http.Request, args *GetBucketPolicyArgs, reply *GetBucketPolicyRep) error { ctx := newWebContext(r, args, "WebGetBucketPolicy") + objectAPI := web.ObjectAPI() if objectAPI == nil { return toJSONError(ctx, errServerNotInitialized)