audit: log invalid wrapping token request/response #6541
Conversation
Co-Authored-By: calvn <cleung2010@gmail.com>
Co-Authored-By: calvn <cleung2010@gmail.com>
|
My overall comment is it feels like we ought to be able to avoid calling ValidateWrappingToken twice on every action with a wrapping token. Before we called it once; now it's being split into two different places but being called both in both cases. |
|
Calling |
http/logical.go
Outdated
| case "sys/wrapping/lookup", "sys/wrapping/rewrap", "sys/wrapping/unwrap": | ||
| r = r.WithContext(newCtx) | ||
| if err := wrappingVerificationFunc(r.Context(), core, req); err != nil { |
There was a problem hiding this comment.
I think we should put this back, no? the logic feels cleaner here instead of in handleRequest and handleLoginRequest
vault/wrapping.go
Outdated
| NonHMACReqDataKeys: nonHMACReqDataKeys, | ||
| } | ||
| if err != nil { | ||
| logInput.OuterErr = errwrap.Wrapf("error validating wrapping token: {{err}}", err) |
There was a problem hiding this comment.
Are we confident that {{err}} will always be safe to include un-HMACed in the audit log? I think it's probably safer to omit the cause.
vault/wrapping.go
Outdated
| // the wrapping token | ||
| if err != nil || !valid { | ||
| // Get non-HMAC'ed request data keys | ||
| var nonHMACReqDataKeys []string |
There was a problem hiding this comment.
This isn't necessary is it? The mount entry will always be sys, which isn't user-mountable or tuneable, thus it will never have non-hmac keys.
| } | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
Why do we test the same thing twice? And should we test the happy path, i.e. that no audit record is written for a valid token?
Closes #6491