-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Audit logging panic investigation #478
Comments
This is also not something I'm likely to get to in a reasonable time; feel free to tackle this one if you're interested. |
this one looks like a good place to start. @cipherboy can you assign it to me? |
fyi: @cipherboy The serialization/deserialation approach to creating the copy has a problem. Some of the hash code is driven by looking at the subtypes of the data structure being hashed. That type info diseappears with the deserialization. Without that info, the hashing won't work according to spec. Here are examples I've found so far of type checking in the hash code: openbao/audit/hashstructure.go Line 89 in 7fca5c0
openbao/audit/hashstructure.go Lines 282 to 285 in 7fca5c0
I'm considering alternatives now. Let know if you have any thoughts. |
@cipherboy said:
Because of the reason in my comment above, I don't think we can correctly hash the json unmarshaled copy. So I was starting to consider how to create the "addressable copy" you refer to above. I'm not sure there is an easy way to do that. But I'm not that familiar with golang reflection, so maybe I'm missing something. What was the approach you were thinking of to do this? |
\o hey @GeorgeJahad, sorry, been a busy time :-) The Time is a good counter example since it marshals into strings. I think this means a parallel walk of both the original and marshaled structs for typing information? If X in the original struct is a type alias of Time, do not HMAC it, but otherwise if it is a string, we should. Then we modify the marshalled struct (with HMACs) but keep the original around for reference. |
Describe the bug
A well-known problem with the audit logging subsystem today is that, because it rewrites the response in-place using reflection, we end up being unable to audit certain types, causing panics like #97.
In particular, the root cause in this case is that the struct is referenced from a map by value, rather than by reference:
when using
structs.New(role).Map()
, we do get amap[string]interface{}
out, but this is not recursive: the interior type of thevhost
key is stillmap[string]vhostPermission
. When we go to walk the response for auditing, we panic because the struct field is not addressable because it comes from a value and not a pointer reference.In this case, we have two options:
resp.Data
must be serializable), and audit log the de-serialized form.While 1 is likely strictly faster (because it only incurs cost in the few places where structs are responded with due to this bug), 2 is likely more robust and allows us to avoid the dependency on an unmaintained package (as
mitchellh
has archived many of his repositories, some without replacement). Thus, I'd personally lean towards two (deserializing intomap[string]interface{}
).We should also probably remove the [last usages of
structs.New(..)
](https://github.com/search?q=repo%3Aopenbao%2Fopenbao%20structs.New(&type=code) and replace it with custom serializers in all places so it isn't seen as a pattern for others. This is also a deprecated package.To Reproduce
See #97 and makes sure you are prior to the fixed commit in #224 (such as 8d42b5f), then run the reproducer test from that issue.
Expected behavior
Audit should be able to audit any response structure of any kind.
Environment:
n/a
Additional context
n/a
The text was updated successfully, but these errors were encountered: