audit: entry_formatter update to ensure no race detection issues (#24811)

* audit: entry_formatter update to ensure no race detection issues
* in progress with looking at a clone method for LogInput
* Tidy up LogInput Clone method
* less memory allocation
* fix hmac key clone

audit/entry_formatter.go

@@ -82,15 +82,19 @@ func (f *EntryFormatter) Process(ctx context.Context, e *eventlogger.Event) (*ev
 		return nil, fmt.Errorf("%s: cannot parse event payload: %w", op, event.ErrInvalidParameter)
 	}
 
-	var result []byte
-	data := new(logical.LogInput)
-	headers := make(map[string][]string)
+	if a.Data == nil {
+		return nil, fmt.Errorf("%s: cannot audit event (%s) with no data: %w", op, a.Subtype, event.ErrInvalidParameter)
+	}
 
-	if a.Data != nil {
-		*data = *a.Data
-		if a.Data.Request != nil && a.Data.Request.Headers != nil {
-			headers = a.Data.Request.Headers
-		}
+	// Take a copy of the event data before we modify anything.
+	data, err := a.Data.Clone()
+	if err != nil {
+		return nil, fmt.Errorf("%s: unable to copy audit event data: %w", op, err)
+	}
+
+	var headers map[string][]string
+	if data.Request != nil && data.Request.Headers != nil {
+		headers = data.Request.Headers
 	}
 
 	if f.headerFormatter != nil {
@@ -102,6 +106,8 @@ func (f *EntryFormatter) Process(ctx context.Context, e *eventlogger.Event) (*ev
 		data.Request.Headers = adjustedHeaders
 	}
 
+	var result []byte
+
 	switch a.Subtype {
 	case RequestType:
 		entry, err := f.FormatRequest(ctx, data)

audit/entry_formatter_test.go

@@ -180,14 +180,14 @@ func TestEntryFormatter_Process(t *testing.T) {
 	}{
 		"json-request-no-data": {
 			IsErrorExpected:      true,
-			ExpectedErrorMessage: "audit.(EntryFormatter).Process: unable to parse request from audit event: request to request-audit a nil request",
+			ExpectedErrorMessage: "audit.(EntryFormatter).Process: cannot audit event (AuditRequest) with no data: invalid parameter",
 			Subtype:              RequestType,
 			RequiredFormat:       JSONFormat,
 			Data:                 nil,
 		},
 		"json-response-no-data": {
 			IsErrorExpected:      true,
-			ExpectedErrorMessage: "audit.(EntryFormatter).Process: unable to parse response from audit event: request to response-audit a nil request",
+			ExpectedErrorMessage: "audit.(EntryFormatter).Process: cannot audit event (AuditResponse) with no data: invalid parameter",
 			Subtype:              ResponseType,
 			RequiredFormat:       JSONFormat,
 			Data:                 nil,
@@ -236,14 +236,14 @@ func TestEntryFormatter_Process(t *testing.T) {
 		},
 		"jsonx-request-no-data": {
 			IsErrorExpected:      true,
-			ExpectedErrorMessage: "audit.(EntryFormatter).Process: unable to parse request from audit event: request to request-audit a nil request",
+			ExpectedErrorMessage: "audit.(EntryFormatter).Process: cannot audit event (AuditRequest) with no data: invalid parameter",
 			Subtype:              RequestType,
 			RequiredFormat:       JSONxFormat,
 			Data:                 nil,
 		},
 		"jsonx-response-no-data": {
 			IsErrorExpected:      true,
-			ExpectedErrorMessage: "audit.(EntryFormatter).Process: unable to parse response from audit event: request to response-audit a nil request",
+			ExpectedErrorMessage: "audit.(EntryFormatter).Process: cannot audit event (AuditResponse) with no data: invalid parameter",
 			Subtype:              ResponseType,
 			RequiredFormat:       JSONxFormat,
 			Data:                 nil,

sdk/logical/audit.go

@@ -3,6 +3,13 @@
 
 package logical
 
+import (
+	"fmt"
+
+	"github.com/mitchellh/copystructure"
+)
+
+// LogInput is used as the input to the audit system on which audit entries are based.
 type LogInput struct {
 	Type                string
 	Auth                *Auth
@@ -53,3 +60,122 @@ func (l *LogInput) BexprDatum(namespace string) *LogInputBexpr {
 		Path:       path,
 	}
 }
+
+// Clone will attempt to create a deep copy of the LogInput.
+// This is preferred over attempting to use other libraries such as copystructure.
+// Audit formatting methods which parse LogInput into an audit request/response
+// entry, call receivers on the LogInput struct to get their value. These values
+// would be lost using copystructure as it cannot copy unexported fields.
+// If the LogInput type or any of the subtypes referenced by LogInput fields are
+// changed, then the Clone methods will need to be updated.
+// NOTE: Does not deep clone the LogInput.OuterError field as it represents an
+// error interface.
+func (l *LogInput) Clone() (*LogInput, error) {
+	// Clone Auth
+	auth, err := cloneAuth(l.Auth)
+	if err != nil {
+		return nil, err
+	}
+
+	// Clone Request
+	req, err := cloneRequest(l.Request)
+	if err != nil {
+		return nil, err
+	}
+
+	// Clone Response
+	resp, err := cloneResponse(l.Response)
+	if err != nil {
+		return nil, err
+	}
+
+	// Copy HMAC keys
+	reqDataKeys := make([]string, len(l.NonHMACReqDataKeys))
+	copy(reqDataKeys, l.NonHMACReqDataKeys)
+	respDataKeys := make([]string, len(l.NonHMACRespDataKeys))
+	copy(respDataKeys, l.NonHMACRespDataKeys)
+
+	// OuterErr is just linked in a non-deep way as it's an interface, and we
+	// don't know for sure which type this might actually be.
+	// At the time of writing this code, OuterErr isn't modified by anything,
+	// so we shouldn't get any race issues.
+	cloned := &LogInput{
+		Type:                l.Type,
+		Auth:                auth,
+		Request:             req,
+		Response:            resp,
+		OuterErr:            l.OuterErr,
+		NonHMACReqDataKeys:  reqDataKeys,
+		NonHMACRespDataKeys: respDataKeys,
+	}
+
+	return cloned, nil
+}
+
+// clone will deep-copy the supplied struct.
+// However, it cannot copy unexported fields or evaluate methods.
+func clone[V any](s V) (V, error) {
+	var result V
+
+	data, err := copystructure.Copy(s)
+	if err != nil {
+		return result, err
+	}
+
+	result = data.(V)
+
+	return result, err
+}
+
+// cloneAuth deep copies an Auth struct.
+func cloneAuth(auth *Auth) (*Auth, error) {
+	// If auth is nil, there's nothing to clone.
+	if auth == nil {
+		return nil, nil
+	}
+
+	auth, err := clone[*Auth](auth)
+	if err != nil {
+		return nil, fmt.Errorf("unable to clone auth: %w", err)
+	}
+
+	return auth, nil
+}
+
+// cloneRequest deep copies a Request struct.
+// It will set unexported fields which were only previously accessible outside
+// the package via receiver methods.
+func cloneRequest(request *Request) (*Request, error) {
+	// If request is nil, there's nothing to clone.
+	if request == nil {
+		return nil, nil
+	}
+
+	req, err := clone[*Request](request)
+	if err != nil {
+		return nil, fmt.Errorf("unable to clone request: %w", err)
+	}
+
+	// Add the unexported values that were only retrievable via receivers.
+	req.mountClass = request.MountClass()
+	req.mountRunningVersion = request.MountRunningVersion()
+	req.mountRunningSha256 = request.MountRunningSha256()
+	req.mountIsExternalPlugin = request.MountIsExternalPlugin()
+
+	return req, nil
+}
+
+// cloneResponse deep copies a Response struct.
+func cloneResponse(response *Response) (*Response, error) {
+	// If response is nil, there's nothing to clone.
+	if response == nil {
+		return nil, nil
+	}
+
+	resp, err := clone[*Response](response)
+	if err != nil {
+		return nil, fmt.Errorf("unable to clone response: %w", err)
+	}
+
+	return resp, nil
+}