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

@@ -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)

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
 	}
 

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",

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))
 	}
 }

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)
 	}
 }

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
+// swapMu must be held while reading slice info or swapping targets or auditTargets.
-var Targets = []Target{}
+var swapMu sync.Mutex
 
-// AuditTargets is the list of enabled audit loggers
+// targets is the set of enabled loggers.
-var AuditTargets = []Target{}
+// 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
 }