From d9c1d79e307b85e919583638cab1103fe41e47f1 Mon Sep 17 00:00:00 2001 From: Klaus Post Date: Thu, 28 Oct 2021 07:35:28 -0700 Subject: [PATCH] Protect logger targets (#13529) Logger targets were not race protected against concurrent updates from for example `HTTPConsoleLoggerSys`. Restrict direct access to targets and make slices immutable so a returned slice can be processed safely without locks. --- cmd/admin-handlers.go | 4 +-- cmd/erasure-sets.go | 2 +- cmd/post-policy_test.go | 12 +++++++- internal/logger/audit.go | 5 +-- internal/logger/logger.go | 2 +- internal/logger/targets.go | 63 ++++++++++++++++++++++++++++++++++---- 6 files changed, 75 insertions(+), 13 deletions(-) diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index e64fd937c..67e868074 100644 --- a/cmd/admin-handlers.go +++ b/cmd/admin-handlers.go @@ -2096,7 +2096,7 @@ func fetchKMSStatus() madmin.KMS { func fetchLoggerInfo() ([]madmin.Logger, []madmin.Audit) { var loggerInfo []madmin.Logger var auditloggerInfo []madmin.Audit - for _, target := range logger.Targets { + for _, target := range logger.Targets() { if target.Endpoint() != "" { tgt := target.String() err := checkConnection(target.Endpoint(), 15*time.Second) @@ -2112,7 +2112,7 @@ func fetchLoggerInfo() ([]madmin.Logger, []madmin.Audit) { } } - for _, target := range logger.AuditTargets { + for _, target := range logger.AuditTargets() { if target.Endpoint() != "" { tgt := target.String() err := checkConnection(target.Endpoint(), 15*time.Second) diff --git a/cmd/erasure-sets.go b/cmd/erasure-sets.go index d902d6a90..c1bcb327c 100644 --- a/cmd/erasure-sets.go +++ b/cmd/erasure-sets.go @@ -516,7 +516,7 @@ func (a *auditObjectErasureMap) MarshalJSON() ([]byte, error) { } func auditObjectErasureSet(ctx context.Context, object string, set *erasureObjects) { - if len(logger.AuditTargets) == 0 { + if len(logger.AuditTargets()) == 0 { return } diff --git a/cmd/post-policy_test.go b/cmd/post-policy_test.go index da5ee254e..321000f5a 100644 --- a/cmd/post-policy_test.go +++ b/cmd/post-policy_test.go @@ -30,7 +30,7 @@ import ( "testing" "time" - humanize "github.com/dustin/go-humanize" + "github.com/dustin/go-humanize" ) const ( @@ -267,6 +267,16 @@ func testPostPolicyBucketHandler(obj ObjectLayer, instanceType string, t TestErr dates: []interface{}{curTimePlus5Min.Format(iso8601TimeFormat), curTime.Format(iso8601DateFormat), curTime.Format(yyyymmdd)}, policy: `{"expiration": "%s","conditions":[["eq", "$bucket", "` + bucketName + `"], ["starts-with", "$key", "test/"], ["eq", "$x-amz-algorithm", "AWS4-HMAC-SHA256"], ["eq", "$x-amz-date", "%s"], ["eq", "$x-amz-credential", "` + credentials.AccessKey + `/%s/us-east-1/s3/aws4_request"],["eq", "$x-amz-meta-uuid", "1234"]]}`, }, + // Success case, big body. + { + objectName: "test", + data: bytes.Repeat([]byte("a"), 10<<20), + expectedRespStatus: http.StatusNoContent, + accessKey: credentials.AccessKey, + secretKey: credentials.SecretKey, + dates: []interface{}{curTimePlus5Min.Format(iso8601TimeFormat), curTime.Format(iso8601DateFormat), curTime.Format(yyyymmdd)}, + policy: `{"expiration": "%s","conditions":[["eq", "$bucket", "` + bucketName + `"], ["starts-with", "$key", "test/"], ["eq", "$x-amz-algorithm", "AWS4-HMAC-SHA256"], ["eq", "$x-amz-date", "%s"], ["eq", "$x-amz-credential", "` + credentials.AccessKey + `/%s/us-east-1/s3/aws4_request"],["eq", "$x-amz-meta-uuid", "1234"]]}`, + }, // Corrupted Base 64 result { objectName: "test", diff --git a/internal/logger/audit.go b/internal/logger/audit.go index 3f39c4ce4..e001819b3 100644 --- a/internal/logger/audit.go +++ b/internal/logger/audit.go @@ -24,6 +24,7 @@ import ( "io" "net/http" "strconv" + "sync/atomic" "time" "github.com/klauspost/compress/gzhttp" @@ -157,7 +158,7 @@ func GetAuditEntry(ctx context.Context) *audit.Entry { // AuditLog - logs audit logs to all audit targets. func AuditLog(ctx context.Context, w http.ResponseWriter, r *http.Request, reqClaims map[string]interface{}, filterKeys ...string) { // Fast exit if there is not audit target configured - if len(AuditTargets) == 0 { + if atomic.LoadInt32(&nAuditTargets) == 0 { return } @@ -225,7 +226,7 @@ func AuditLog(ctx context.Context, w http.ResponseWriter, r *http.Request, reqCl } // Send audit logs only to http targets. - for _, t := range AuditTargets { + for _, t := range AuditTargets() { _ = t.Send(entry, string(All)) } } diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 10ee389d1..74f4cbb2f 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -379,7 +379,7 @@ func logIf(ctx context.Context, err error, errKind ...interface{}) { } // Iterate over all logger targets to send the log entry - for _, t := range Targets { + for _, t := range Targets() { t.Send(entry, entry.LogKind) } } diff --git a/internal/logger/targets.go b/internal/logger/targets.go index 5aff7bff2..60c10b599 100644 --- a/internal/logger/targets.go +++ b/internal/logger/targets.go @@ -17,6 +17,11 @@ package logger +import ( + "sync" + "sync/atomic" +) + // Target is the entity that we will receive // a single log entry and Send it to the log target // e.g. Send the log to a http server @@ -27,11 +32,46 @@ type Target interface { Send(entry interface{}, errKind string) error } -// Targets is the set of enabled loggers -var Targets = []Target{} +// swapMu must be held while reading slice info or swapping targets or auditTargets. +var swapMu sync.Mutex -// AuditTargets is the list of enabled audit loggers -var AuditTargets = []Target{} +// targets is the set of enabled loggers. +// Must be immutable at all times. +// Can be swapped to another while holding swapMu +var targets = []Target{} +var nTargets int32 // atomic count of len(targets) + +// Targets returns active targets. +// Returned slice may not be modified in any way. +func Targets() []Target { + if atomic.LoadInt32(&nTargets) == 0 { + // Lock free if none... + return nil + } + swapMu.Lock() + res := targets + swapMu.Unlock() + return res +} + +// AuditTargets returns active audit targets. +// Returned slice may not be modified in any way. +func AuditTargets() []Target { + if atomic.LoadInt32(&nAuditTargets) == 0 { + // Lock free if none... + return nil + } + swapMu.Lock() + res := auditTargets + swapMu.Unlock() + return res +} + +// auditTargets is the list of enabled audit loggers +// Must be immutable at all times. +// Can be swapped to another while holding swapMu +var auditTargets = []Target{} +var nAuditTargets int32 // atomic count of len(auditTargets) // AddAuditTarget adds a new audit logger target to the // list of enabled loggers @@ -40,7 +80,12 @@ func AddAuditTarget(t Target) error { return err } - AuditTargets = append(AuditTargets, t) + swapMu.Lock() + updated := append(make([]Target, 0, len(auditTargets)+1), auditTargets...) + updated = append(updated, t) + auditTargets = updated + atomic.StoreInt32(&nAuditTargets, int32(len(updated))) + swapMu.Unlock() return nil } @@ -50,6 +95,12 @@ func AddTarget(t Target) error { if err := t.Init(); err != nil { return err } - Targets = append(Targets, t) + swapMu.Lock() + updated := append(make([]Target, 0, len(targets)+1), targets...) + updated = append(updated, t) + targets = updated + atomic.StoreInt32(&nTargets, int32(len(updated))) + swapMu.Unlock() + return nil }