diff --git a/audit/entry_formatter.go b/audit/entry_formatter.go index 6b1c2b5f3388..811c0a1f8a1f 100644 --- a/audit/entry_formatter.go +++ b/audit/entry_formatter.go @@ -8,19 +8,20 @@ import ( "crypto/tls" "errors" "fmt" + "reflect" + "runtime/debug" "strings" "time" - "github.com/jefferai/jsonx" - - "github.com/hashicorp/vault/helper/namespace" - "github.com/hashicorp/vault/sdk/logical" - "github.com/go-jose/go-jose/v3/jwt" + "github.com/hashicorp/eventlogger" + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/internal/observability/event" "github.com/hashicorp/vault/sdk/helper/jsonutil" - - "github.com/hashicorp/eventlogger" + "github.com/hashicorp/vault/sdk/logical" + "github.com/jefferai/jsonx" ) var ( @@ -30,13 +31,22 @@ var ( // NewEntryFormatter should be used to create an EntryFormatter. // Accepted options: WithPrefix. -func NewEntryFormatter(config FormatterConfig, salter Salter, opt ...Option) (*EntryFormatter, error) { +func NewEntryFormatter(name string, config FormatterConfig, salter Salter, logger hclog.Logger, opt ...Option) (*EntryFormatter, error) { const op = "audit.NewEntryFormatter" + name = strings.TrimSpace(name) + if name == "" { + return nil, fmt.Errorf("%s: name is required: %w", op, event.ErrInvalidParameter) + } + if salter == nil { return nil, fmt.Errorf("%s: cannot create a new audit formatter with nil salter: %w", op, event.ErrInvalidParameter) } + if logger == nil || reflect.ValueOf(logger).IsNil() { + return nil, fmt.Errorf("%s: cannot create a new audit formatter with nil logger: %w", op, event.ErrInvalidParameter) + } + // We need to ensure that the format isn't just some default empty string. if err := config.RequiredFormat.validate(); err != nil { return nil, fmt.Errorf("%s: format not valid: %w", op, err) @@ -48,9 +58,11 @@ func NewEntryFormatter(config FormatterConfig, salter Salter, opt ...Option) (*E } return &EntryFormatter{ - salter: salter, config: config, + salter: salter, + logger: logger, headerFormatter: opts.withHeaderFormatter, + name: name, prefix: opts.withPrefix, }, nil } @@ -67,7 +79,7 @@ func (*EntryFormatter) Type() eventlogger.NodeType { // Process will attempt to parse the incoming event data into a corresponding // audit Request/Response which is serialized to JSON/JSONx and stored within the event. -func (f *EntryFormatter) Process(ctx context.Context, e *eventlogger.Event) (*eventlogger.Event, error) { +func (f *EntryFormatter) Process(ctx context.Context, e *eventlogger.Event) (_ *eventlogger.Event, retErr error) { const op = "audit.(EntryFormatter).Process" select { @@ -89,6 +101,23 @@ func (f *EntryFormatter) Process(ctx context.Context, e *eventlogger.Event) (*ev return nil, fmt.Errorf("%s: cannot audit event (%s) with no data: %w", op, a.Subtype, event.ErrInvalidParameter) } + // Handle panics + defer func() { + r := recover() + if r == nil { + return + } + + f.logger.Error("panic during logging", + "request_path", a.Data.Request.Path, + "audit_device_path", f.name, + "error", r, + "stacktrace", string(debug.Stack())) + + // Ensure that we add this error onto any pre-existing error that was being returned. + retErr = multierror.Append(retErr, fmt.Errorf("%s: panic generating audit log: %q", op, f.name)).ErrorOrNil() + }() + // Take a copy of the event data before we modify anything. data, err := a.Data.Clone() if err != nil { diff --git a/audit/entry_formatter_test.go b/audit/entry_formatter_test.go index 62f91b2fcc51..7b52f26c3f1e 100644 --- a/audit/entry_formatter_test.go +++ b/audit/entry_formatter_test.go @@ -13,6 +13,8 @@ import ( "time" "github.com/hashicorp/eventlogger" + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/internal/observability/event" "github.com/hashicorp/vault/sdk/helper/jsonutil" @@ -25,20 +27,42 @@ import ( // TestNewEntryFormatter ensures we can create new EntryFormatter structs. func TestNewEntryFormatter(t *testing.T) { tests := map[string]struct { + Name string UseStaticSalt bool + Logger hclog.Logger Options []Option // Only supports WithPrefix IsErrorExpected bool ExpectedErrorMessage string ExpectedFormat format ExpectedPrefix string }{ + "empty-name": { + Name: "", + IsErrorExpected: true, + ExpectedErrorMessage: "audit.NewEntryFormatter: name is required: invalid parameter", + }, + "spacey-name": { + Name: " ", + IsErrorExpected: true, + ExpectedErrorMessage: "audit.NewEntryFormatter: name is required: invalid parameter", + }, "nil-salter": { + Name: "test", UseStaticSalt: false, IsErrorExpected: true, ExpectedErrorMessage: "audit.NewEntryFormatter: cannot create a new audit formatter with nil salter: invalid parameter", }, + "nil-logger": { + Name: "juan", + UseStaticSalt: true, + Logger: nil, + IsErrorExpected: true, + ExpectedErrorMessage: "audit.NewEntryFormatter: cannot create a new audit formatter with nil logger: invalid parameter", + }, "static-salter": { + Name: "test", UseStaticSalt: true, + Logger: hclog.NewNullLogger(), IsErrorExpected: false, Options: []Option{ WithFormat(JSONFormat.String()), @@ -46,12 +70,16 @@ func TestNewEntryFormatter(t *testing.T) { ExpectedFormat: JSONFormat, }, "default": { + Name: "test", UseStaticSalt: true, + Logger: hclog.NewNullLogger(), IsErrorExpected: false, ExpectedFormat: JSONFormat, }, "config-json": { + Name: "test", UseStaticSalt: true, + Logger: hclog.NewNullLogger(), Options: []Option{ WithFormat(JSONFormat.String()), }, @@ -59,7 +87,9 @@ func TestNewEntryFormatter(t *testing.T) { ExpectedFormat: JSONFormat, }, "config-jsonx": { + Name: "test", UseStaticSalt: true, + Logger: hclog.NewNullLogger(), Options: []Option{ WithFormat(JSONxFormat.String()), }, @@ -67,7 +97,9 @@ func TestNewEntryFormatter(t *testing.T) { ExpectedFormat: JSONxFormat, }, "config-json-prefix": { + Name: "test", UseStaticSalt: true, + Logger: hclog.NewNullLogger(), Options: []Option{ WithPrefix("foo"), WithFormat(JSONFormat.String()), @@ -77,7 +109,9 @@ func TestNewEntryFormatter(t *testing.T) { ExpectedPrefix: "foo", }, "config-jsonx-prefix": { + Name: "test", UseStaticSalt: true, + Logger: hclog.NewNullLogger(), Options: []Option{ WithPrefix("foo"), WithFormat(JSONxFormat.String()), @@ -100,7 +134,7 @@ func TestNewEntryFormatter(t *testing.T) { cfg, err := NewFormatterConfig(tc.Options...) require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss, tc.Options...) + f, err := NewEntryFormatter(tc.Name, cfg, ss, tc.Logger, tc.Options...) switch { case tc.IsErrorExpected: @@ -123,7 +157,7 @@ func TestEntryFormatter_Reopen(t *testing.T) { cfg, err := NewFormatterConfig() require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss) + f, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) require.NotNil(t, f) require.NoError(t, f.Reopen()) @@ -135,7 +169,7 @@ func TestEntryFormatter_Type(t *testing.T) { cfg, err := NewFormatterConfig() require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss) + f, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) require.NotNil(t, f) require.Equal(t, eventlogger.NodeTypeFormatter, f.Type()) @@ -278,7 +312,7 @@ func TestEntryFormatter_Process(t *testing.T) { cfg, err := NewFormatterConfig(WithFormat(tc.RequiredFormat.String())) require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss) + f, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) require.NotNil(t, f) @@ -343,7 +377,7 @@ func BenchmarkAuditFileSink_Process(b *testing.B) { cfg, err := NewFormatterConfig() require.NoError(b, err) ss := newStaticSalt(b) - formatter, err := NewEntryFormatter(cfg, ss) + formatter, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(b, err) require.NotNil(b, formatter) @@ -453,7 +487,7 @@ func TestEntryFormatter_Process_JSON(t *testing.T) { for name, tc := range cases { cfg, err := NewFormatterConfig(WithHMACAccessor(false)) require.NoError(t, err) - formatter, err := NewEntryFormatter(cfg, ss, WithPrefix(tc.Prefix)) + formatter, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger(), WithPrefix(tc.Prefix)) require.NoError(t, err) in := &logical.LogInput{ @@ -612,7 +646,7 @@ func TestEntryFormatter_Process_JSONx(t *testing.T) { WithFormat(JSONxFormat.String()), ) require.NoError(t, err) - formatter, err := NewEntryFormatter(cfg, tempStaticSalt, WithPrefix(tc.Prefix)) + formatter, err := NewEntryFormatter("test", cfg, tempStaticSalt, hclog.NewNullLogger(), WithPrefix(tc.Prefix)) require.NoError(t, err) require.NotNil(t, formatter) @@ -661,7 +695,7 @@ func TestEntryFormatter_Process_NoMutation(t *testing.T) { cfg, err := NewFormatterConfig() require.NoError(t, err) ss := newStaticSalt(t) - formatter, err := NewEntryFormatter(cfg, ss) + formatter, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) require.NotNil(t, formatter) @@ -711,6 +745,66 @@ func TestEntryFormatter_Process_NoMutation(t *testing.T) { require.NotEqual(t, a2, a) } +// TestEntryFormatter_Process_Panic tries to send data into the EntryFormatter +// which will currently cause a panic when a response is formatted due to the +// underlying hashing that is done with reflectwalk. +func TestEntryFormatter_Process_Panic(t *testing.T) { + t.Parallel() + + // Create the formatter node. + cfg, err := NewFormatterConfig() + require.NoError(t, err) + ss := newStaticSalt(t) + formatter, err := NewEntryFormatter("juan", cfg, ss, hclog.NewNullLogger()) + require.NoError(t, err) + require.NotNil(t, formatter) + + // The secret sauce, create a bad addr. + // see: https://github.com/hashicorp/vault/issues/16462 + badAddr, err := sockaddr.NewSockAddr("10.10.10.2/32 10.10.10.3/32") + require.NoError(t, err) + + in := &logical.LogInput{ + Auth: &logical.Auth{ + ClientToken: "foo", + Accessor: "bar", + EntityID: "foobarentity", + DisplayName: "testtoken", + NoDefaultPolicy: true, + Policies: []string{"root"}, + TokenType: logical.TokenTypeService, + }, + Request: &logical.Request{ + Operation: logical.UpdateOperation, + Path: "/foo", + Connection: &logical.Connection{ + RemoteAddr: "127.0.0.1", + }, + WrapInfo: &logical.RequestWrapInfo{ + TTL: 60 * time.Second, + }, + Headers: map[string][]string{ + "foo": {"bar"}, + }, + Data: map[string]interface{}{}, + }, + Response: &logical.Response{ + Data: map[string]any{ + "token_bound_cidrs": []*sockaddr.SockAddrMarshaler{ + {SockAddr: badAddr}, + }, + }, + }, + } + + e := fakeEvent(t, ResponseType, in) + + e2, err := formatter.Process(namespace.RootContext(nil), e) + require.Error(t, err) + require.Contains(t, err.Error(), "audit.(EntryFormatter).Process: panic generating audit log: \"juan\"") + require.Nil(t, e2) +} + // hashExpectedValueForComparison replicates enough of the audit HMAC process on a piece of expected data in a test, // so that we can use assert.Equal to compare the expected and output values. func (f *EntryFormatter) hashExpectedValueForComparison(input map[string]any) map[string]any { diff --git a/audit/entry_formatter_writer.go b/audit/entry_formatter_writer.go index adace5496877..44d510e2b660 100644 --- a/audit/entry_formatter_writer.go +++ b/audit/entry_formatter_writer.go @@ -10,6 +10,7 @@ import ( "io" "strings" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/sdk/helper/salt" "github.com/hashicorp/vault/sdk/logical" ) @@ -96,7 +97,7 @@ func NewTemporaryFormatter(requiredFormat, prefix string) (*EntryFormatterWriter return nil, err } - eventFormatter, err := NewEntryFormatter(cfg, &nonPersistentSalt{}, WithPrefix(prefix)) + eventFormatter, err := NewEntryFormatter("sys/audit/test", cfg, &nonPersistentSalt{}, hclog.NewNullLogger(), WithPrefix(prefix)) if err != nil { return nil, err } diff --git a/audit/entry_formatter_writer_test.go b/audit/entry_formatter_writer_test.go index 8357fec2b01e..095252a9c9b4 100644 --- a/audit/entry_formatter_writer_test.go +++ b/audit/entry_formatter_writer_test.go @@ -8,6 +8,7 @@ import ( "io" "testing" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/salt" "github.com/hashicorp/vault/sdk/logical" @@ -127,7 +128,7 @@ func TestNewEntryFormatterWriter(t *testing.T) { var f Formatter if !tc.UseNilFormatter { - tempFormatter, err := NewEntryFormatter(cfg, s) + tempFormatter, err := NewEntryFormatter("test", cfg, s, hclog.NewNullLogger()) require.NoError(t, err) require.NotNil(t, tempFormatter) f = tempFormatter @@ -192,7 +193,7 @@ func TestEntryFormatter_FormatRequest(t *testing.T) { ss := newStaticSalt(t) cfg, err := NewFormatterConfig() require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss) + f, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) var ctx context.Context @@ -259,7 +260,7 @@ func TestEntryFormatter_FormatResponse(t *testing.T) { ss := newStaticSalt(t) cfg, err := NewFormatterConfig() require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss) + f, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) var ctx context.Context @@ -361,7 +362,7 @@ func TestElideListResponses(t *testing.T) { formatResponse := func(t *testing.T, config FormatterConfig, operation logical.Operation, inputData map[string]interface{}, ) { - f, err := NewEntryFormatter(config, &tfw) + f, err := NewEntryFormatter("test", config, &tfw, hclog.NewNullLogger()) require.NoError(t, err) formatter, err := NewEntryFormatterWriter(config, f, &tfw) require.NoError(t, err) diff --git a/audit/types.go b/audit/types.go index af5a5830dfae..fcd638beadcf 100644 --- a/audit/types.go +++ b/audit/types.go @@ -9,8 +9,8 @@ import ( "time" "github.com/hashicorp/eventlogger" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/sdk/helper/salt" - "github.com/hashicorp/vault/sdk/logical" ) @@ -96,9 +96,11 @@ type HeaderFormatter interface { // EntryFormatter should be used to format audit requests and responses. type EntryFormatter struct { + config FormatterConfig salter Salter + logger hclog.Logger headerFormatter HeaderFormatter - config FormatterConfig + name string prefix string } @@ -313,6 +315,9 @@ type BackendConfig struct { // MountPath is the path where this Backend is mounted MountPath string + + // Logger is used to emit log messages usually captured in the server logs. + Logger hclog.Logger } // Factory is the factory function to create an audit backend. diff --git a/audit/writer_json_test.go b/audit/writer_json_test.go index 822f26851be9..b0227f81d8f5 100644 --- a/audit/writer_json_test.go +++ b/audit/writer_json_test.go @@ -12,11 +12,11 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/jsonutil" "github.com/hashicorp/vault/sdk/logical" + "github.com/stretchr/testify/require" ) func TestFormatJSON_formatRequest(t *testing.T) { @@ -100,7 +100,7 @@ func TestFormatJSON_formatRequest(t *testing.T) { var buf bytes.Buffer cfg, err := NewFormatterConfig(WithHMACAccessor(false)) require.NoError(t, err) - f, err := NewEntryFormatter(cfg, ss) + f, err := NewEntryFormatter("test", cfg, ss, hclog.NewNullLogger()) require.NoError(t, err) formatter := EntryFormatterWriter{ Formatter: f, diff --git a/audit/writer_jsonx_test.go b/audit/writer_jsonx_test.go index 3533174e8321..797e251af55f 100644 --- a/audit/writer_jsonx_test.go +++ b/audit/writer_jsonx_test.go @@ -12,11 +12,11 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/salt" "github.com/hashicorp/vault/sdk/logical" + "github.com/stretchr/testify/require" ) func TestFormatJSONx_formatRequest(t *testing.T) { @@ -119,7 +119,7 @@ func TestFormatJSONx_formatRequest(t *testing.T) { WithFormat(JSONxFormat.String()), ) require.NoError(t, err) - f, err := NewEntryFormatter(cfg, tempStaticSalt) + f, err := NewEntryFormatter("test", cfg, tempStaticSalt, hclog.NewNullLogger()) require.NoError(t, err) writer := &JSONxWriter{Prefix: tc.Prefix} formatter, err := NewEntryFormatterWriter(cfg, f, writer) diff --git a/builtin/audit/file/backend.go b/builtin/audit/file/backend.go index 4cc82c15ce85..508becf24054 100644 --- a/builtin/audit/file/backend.go +++ b/builtin/audit/file/backend.go @@ -10,6 +10,7 @@ import ( "io" "os" "path/filepath" + "reflect" "strconv" "strings" "sync" @@ -31,10 +32,15 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool if conf.SaltConfig == nil { return nil, fmt.Errorf("nil salt config") } + if conf.SaltView == nil { return nil, fmt.Errorf("nil salt view") } + if conf.Logger == nil || reflect.ValueOf(conf.Logger).IsNil() { + return nil, fmt.Errorf("nil logger") + } + path, ok := conf.Config["file_path"] if !ok { path, ok = conf.Config["path"] @@ -123,7 +129,11 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool b.salt.Store((*salt.Salt)(nil)) // Configure the formatter for either case. - f, err := audit.NewEntryFormatter(b.formatConfig, b, audit.WithHeaderFormatter(headersConfig), audit.WithPrefix(conf.Config["prefix"])) + formatterOpts := []audit.Option{ + audit.WithHeaderFormatter(headersConfig), + audit.WithPrefix(conf.Config["prefix"]), + } + f, err := audit.NewEntryFormatter(conf.MountPath, b.formatConfig, b, conf.Logger, formatterOpts...) if err != nil { return nil, fmt.Errorf("error creating formatter: %w", err) } diff --git a/builtin/audit/file/backend_test.go b/builtin/audit/file/backend_test.go index e0ba06319ca5..177c8930ae39 100644 --- a/builtin/audit/file/backend_test.go +++ b/builtin/audit/file/backend_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/salt" @@ -35,6 +36,8 @@ func TestAuditFile_fileModeNew(t *testing.T) { SaltConfig: &salt.Config{}, SaltView: &logical.InmemStorage{}, Config: config, + Logger: hclog.NewNullLogger(), + MountPath: "test", }, false, nil) if err != nil { t.Fatal(err) @@ -74,6 +77,8 @@ func TestAuditFile_fileModeExisting(t *testing.T) { Config: config, SaltConfig: &salt.Config{}, SaltView: &logical.InmemStorage{}, + Logger: hclog.NewNullLogger(), + MountPath: "test", }, false, nil) if err != nil { t.Fatal(err) @@ -114,6 +119,8 @@ func TestAuditFile_fileMode0000(t *testing.T) { Config: config, SaltConfig: &salt.Config{}, SaltView: &logical.InmemStorage{}, + Logger: hclog.NewNullLogger(), + MountPath: "test", }, false, nil) if err != nil { t.Fatal(err) @@ -148,6 +155,8 @@ func TestAuditFile_EventLogger_fileModeNew(t *testing.T) { SaltConfig: &salt.Config{}, SaltView: &logical.InmemStorage{}, Config: config, + Logger: hclog.NewNullLogger(), + MountPath: "test", }, true, nil) if err != nil { t.Fatal(err) @@ -170,6 +179,8 @@ func BenchmarkAuditFile_request(b *testing.B) { Config: config, SaltConfig: &salt.Config{}, SaltView: &logical.InmemStorage{}, + Logger: hclog.NewNullLogger(), + MountPath: "test", }, false, nil) if err != nil { b.Fatal(err) diff --git a/builtin/audit/socket/backend.go b/builtin/audit/socket/backend.go index 866113806720..579728e5cf55 100644 --- a/builtin/audit/socket/backend.go +++ b/builtin/audit/socket/backend.go @@ -8,14 +8,14 @@ import ( "context" "fmt" "net" + "reflect" "strconv" "sync" "time" - "github.com/hashicorp/go-secure-stdlib/parseutil" - "github.com/hashicorp/eventlogger" "github.com/hashicorp/go-multierror" + "github.com/hashicorp/go-secure-stdlib/parseutil" "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/internal/observability/event" "github.com/hashicorp/vault/sdk/helper/salt" @@ -26,10 +26,15 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool if conf.SaltConfig == nil { return nil, fmt.Errorf("nil salt config") } + if conf.SaltView == nil { return nil, fmt.Errorf("nil salt view") } + if conf.Logger == nil || reflect.ValueOf(conf.Logger).IsNil() { + return nil, fmt.Errorf("nil logger") + } + address, ok := conf.Config["address"] if !ok { return nil, fmt.Errorf("address is required") @@ -85,11 +90,6 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool return nil, err } - opts := []audit.Option{ - audit.WithHeaderFormatter(headersConfig), - audit.WithPrefix(conf.Config["prefix"]), - } - b := &Backend{ saltConfig: conf.SaltConfig, saltView: conf.SaltView, @@ -101,7 +101,11 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool } // Configure the formatter for either case. - f, err := audit.NewEntryFormatter(b.formatConfig, b, opts...) + formatterOpts := []audit.Option{ + audit.WithHeaderFormatter(headersConfig), + audit.WithPrefix(conf.Config["prefix"]), + } + f, err := audit.NewEntryFormatter(conf.MountPath, b.formatConfig, b, conf.Logger, formatterOpts...) if err != nil { return nil, fmt.Errorf("error creating formatter: %w", err) } diff --git a/builtin/audit/syslog/backend.go b/builtin/audit/syslog/backend.go index f1b7f8179045..d64c8ab0caed 100644 --- a/builtin/audit/syslog/backend.go +++ b/builtin/audit/syslog/backend.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "fmt" + "reflect" "strconv" "sync" @@ -27,6 +28,10 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool return nil, fmt.Errorf("nil salt view") } + if conf.Logger == nil || reflect.ValueOf(conf.Logger).IsNil() { + return nil, fmt.Errorf("nil logger") + } + // Get facility or default to AUTH facility, ok := conf.Config["facility"] if !ok { @@ -90,7 +95,11 @@ func Factory(ctx context.Context, conf *audit.BackendConfig, useEventLogger bool } // Configure the formatter for either case. - f, err := audit.NewEntryFormatter(b.formatConfig, b, audit.WithHeaderFormatter(headersConfig), audit.WithPrefix(conf.Config["prefix"])) + formatterOpts := []audit.Option{ + audit.WithHeaderFormatter(headersConfig), + audit.WithPrefix(conf.Config["prefix"]), + } + f, err := audit.NewEntryFormatter(conf.MountPath, b.formatConfig, b, conf.Logger, formatterOpts...) if err != nil { return nil, fmt.Errorf("error creating formatter: %w", err) } diff --git a/changelog/25605.txt b/changelog/25605.txt new file mode 100644 index 000000000000..87cd770186c6 --- /dev/null +++ b/changelog/25605.txt @@ -0,0 +1,3 @@ +```release-note:bug +audit: Handle a potential panic while formatting audit entries for an audit log +``` \ No newline at end of file diff --git a/helper/testhelpers/corehelpers/corehelpers.go b/helper/testhelpers/corehelpers/corehelpers.go index ff9de7bac811..f5c2cc2ec397 100644 --- a/helper/testhelpers/corehelpers/corehelpers.go +++ b/helper/testhelpers/corehelpers/corehelpers.go @@ -16,12 +16,11 @@ import ( "sync" "time" - "github.com/hashicorp/vault/internal/observability/event" - "github.com/hashicorp/eventlogger" "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/builtin/credential/approle" + "github.com/hashicorp/vault/internal/observability/event" "github.com/hashicorp/vault/plugins/database/mysql" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/consts" @@ -238,28 +237,32 @@ func NewNoopAudit(config map[string]string) (*NoopAudit, error) { return nil, err } - n := &NoopAudit{ - Config: &audit.BackendConfig{ - SaltView: view, - SaltConfig: &salt.Config{ - HMAC: sha256.New, - HMACType: "hmac-sha256", - }, - Config: config, + cfg := &audit.BackendConfig{ + SaltView: view, + SaltConfig: &salt.Config{ + HMAC: sha256.New, + HMACType: "hmac-sha256", }, + Config: config, + MountPath: "noop/", + Logger: hclog.NewNullLogger(), + } + + n := &NoopAudit{ + Config: cfg, } - cfg, err := audit.NewFormatterConfig() + formatterCfg, err := audit.NewFormatterConfig() if err != nil { return nil, err } - f, err := audit.NewEntryFormatter(cfg, n) + f, err := audit.NewEntryFormatter(cfg.MountPath, formatterCfg, n, cfg.Logger) if err != nil { return nil, fmt.Errorf("error creating formatter: %w", err) } - fw, err := audit.NewEntryFormatterWriter(cfg, f, &audit.JSONWriter{}) + fw, err := audit.NewEntryFormatterWriter(formatterCfg, f, &audit.JSONWriter{}) if err != nil { return nil, fmt.Errorf("error creating formatter writer: %w", err) } diff --git a/vault/audit.go b/vault/audit.go index bdbae6471464..595cc6cdd365 100644 --- a/vault/audit.go +++ b/vault/audit.go @@ -522,12 +522,15 @@ func (c *Core) newAuditBackend(ctx context.Context, entry *MountEntry, view logi return nil, fmt.Errorf("unable to parse feature flag: %q: %w", featureFlagDisableEventLogger, err) } + auditLogger := c.baseLogger.Named("audit") + be, err := f( ctx, &audit.BackendConfig{ SaltView: view, SaltConfig: saltConfig, Config: conf, MountPath: entry.Path, + Logger: auditLogger, }, !disableEventLogger, c.auditedHeaders) @@ -538,9 +541,6 @@ func (c *Core) newAuditBackend(ctx context.Context, entry *MountEntry, view logi return nil, fmt.Errorf("nil backend returned from %q factory function", entry.Type) } - auditLogger := c.baseLogger.Named("audit") - c.AddLogger(auditLogger) - switch entry.Type { case "file": key := "audit_file|" + entry.Path @@ -570,6 +570,7 @@ func (c *Core) newAuditBackend(ctx context.Context, entry *MountEntry, view logi } } + c.AddLogger(auditLogger) return be, err } diff --git a/vault/audit_broker.go b/vault/audit_broker.go index f53956036fa3..b345d8df46d2 100644 --- a/vault/audit_broker.go +++ b/vault/audit_broker.go @@ -166,17 +166,25 @@ func (a *AuditBroker) LogRequest(ctx context.Context, in *logical.LogInput, head var retErr *multierror.Error defer func() { - if r := recover(); r != nil { - a.logger.Error("panic during logging", "request_path", in.Request.Path, "error", r, "stacktrace", string(debug.Stack())) - retErr = multierror.Append(retErr, fmt.Errorf("panic generating audit log")) - } + if a.broker == nil { + if r := recover(); r != nil { + a.logger.Error("panic during logging", "request_path", in.Request.Path, "error", r, "stacktrace", string(debug.Stack())) + retErr = multierror.Append(retErr, fmt.Errorf("panic generating audit log")) + } - ret = retErr.ErrorOrNil() - failure := float32(0.0) - if ret != nil { - failure = 1.0 + ret = retErr.ErrorOrNil() + failure := float32(0.0) + if ret != nil { + failure = 1.0 + } + metrics.IncrCounter([]string{"audit", "log_request_failure"}, failure) + } else { + metricVal := float32(0.0) + if ret != nil { + metricVal = 1.0 + } + metrics.IncrCounter([]string{"audit", "log_request_failure"}, metricVal) } - metrics.IncrCounter([]string{"audit", "log_request_failure"}, failure) }() headers := in.Request.Headers @@ -247,18 +255,25 @@ func (a *AuditBroker) LogResponse(ctx context.Context, in *logical.LogInput, hea var retErr *multierror.Error defer func() { - if r := recover(); r != nil { - a.logger.Error("panic during logging", "request_path", in.Request.Path, "error", r, "stacktrace", string(debug.Stack())) - retErr = multierror.Append(retErr, fmt.Errorf("panic generating audit log")) - } - - ret = retErr.ErrorOrNil() + if a.broker == nil { + if r := recover(); r != nil { + a.logger.Error("panic during logging", "request_path", in.Request.Path, "error", r, "stacktrace", string(debug.Stack())) + retErr = multierror.Append(retErr, fmt.Errorf("panic generating audit log")) + } - failure := float32(0.0) - if ret != nil { - failure = 1.0 + ret = retErr.ErrorOrNil() + failure := float32(0.0) + if ret != nil { + failure = 1.0 + } + metrics.IncrCounter([]string{"audit", "log_response_failure"}, failure) + } else { + metricVal := float32(0.0) + if ret != nil { + metricVal = 1.0 + } + metrics.IncrCounter([]string{"audit", "log_response_failure"}, metricVal) } - metrics.IncrCounter([]string{"audit", "log_response_failure"}, failure) }() headers := in.Request.Headers diff --git a/vault/audit_test.go b/vault/audit_test.go index a7dd44539ab6..ad3ded114e8b 100644 --- a/vault/audit_test.go +++ b/vault/audit_test.go @@ -12,13 +12,12 @@ import ( "testing" "time" - "github.com/hashicorp/vault/helper/testhelpers/corehelpers" - "github.com/hashicorp/errwrap" log "github.com/hashicorp/go-hclog" uuid "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/helper/namespace" + "github.com/hashicorp/vault/helper/testhelpers/corehelpers" "github.com/hashicorp/vault/sdk/helper/jsonutil" "github.com/hashicorp/vault/sdk/helper/logging" "github.com/hashicorp/vault/sdk/logical"