VAULT-24013: Audit regression attempting to recover from panic#25605
VAULT-24013: Audit regression attempting to recover from panic#25605
Conversation
peteski22
added
core/audit
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
VioletHynes
left a comment
VioletHynes
left a comment
There was a problem hiding this comment.
This looks good! I'm not the biggest expert in this area but from what I know, it looks like it fixes the bug, and I like the addition of the regression test.
| cfg, err := NewFormatterConfig() | ||
| require.NoError(t, err) | ||
| ss := newStaticSalt(t) | ||
| formatter, err := NewEntryFormatter("juan", cfg, ss, hclog.NewNullLogger()) |
There was a problem hiding this comment.
Why juan? It's just dummy data, but there might be opportunity to use something more descriptive
| // Handle panics | ||
| defer func() { | ||
| r := recover() | ||
| if r == nil { |
There was a problem hiding this comment.
This is a nit, but the following seems to be a much more common pattern, so it might make sense for us to align to that one
if r := recover(); r != nil {
//logging, assigning erroring, etc
}
There was a problem hiding this comment.
I think it's because r is used below in the call to the logger (on line 125).
There was a problem hiding this comment.
That seems to be the case in the other ones I've seen, like this one in audit_broker.go, though maybe I'm totally misunderstanding what you mean :P :
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"))
}
There was a problem hiding this comment.
Nope that's my bad. I totally missed that your original suggestion involved handling logging and errors inside the if statement. Soz for the noise :D
There was a problem hiding this comment.
It's also on me for not posting an example in the initial comment, so no worries at all :D
* Add Logger to BackendConfig * EntryFormatter use logger and recover panics * Added TODO to consider * Add 'name' to entry formatter * Add test for the panic * Fix NoopAudit with update params * emit counter metric even when 0 * Fix vault package tests * changelog * Remove old comment during test writing
3 participants
This PR fixes a regression in recovering Vault when audit related code panics. It appeared with the introduction of the go-eventlogger.
The changes involve continuing to handle the metrics updates in the audit broker, but making an entry formatter node responsible for handling panics that occur during formatting of the audit entry.
Addresses: #16462