From 31baa89f75ee0b6432dd8c7d6ed2efa1df763cdf Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Fri, 12 Jan 2024 14:47:29 +0000 Subject: [PATCH] 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 | 22 +++--- audit/entry_formatter_test.go | 8 +-- sdk/logical/audit.go | 126 ++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 12 deletions(-) diff --git a/audit/entry_formatter.go b/audit/entry_formatter.go index 6937949db3..a9b4880d32 100644 --- a/audit/entry_formatter.go +++ b/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) diff --git a/audit/entry_formatter_test.go b/audit/entry_formatter_test.go index 784d4be13f..b383df9c18 100644 --- a/audit/entry_formatter_test.go +++ b/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, diff --git a/sdk/logical/audit.go b/sdk/logical/audit.go index 12b8bed1cb..5423fd4e2a 100644 --- a/sdk/logical/audit.go +++ b/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 +}