Skip to content
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

Open
cipherboy opened this issue Aug 20, 2024 · 5 comments
Open

Audit logging panic investigation #478

cipherboy opened this issue Aug 20, 2024 · 5 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@cipherboy
Copy link
Member

cipherboy commented Aug 20, 2024

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:

// Role that defines the capabilities of the credentials issued against it.
// Maps are used because the names of vhosts and exchanges will vary widely.
// VHosts is a map with a vhost name as key and the permissions as value.
// VHostTopics is a nested map with vhost name and exchange name as keys and
// the topic permissions as value.
type roleEntry struct {
    Tags        string                                      `json:"tags" structs:"tags" mapstructure:"tags"`
    VHosts      map[string]vhostPermission                 `json:"vhosts" structs:"vhosts" mapstructure:"vhosts"`
    VHostTopics map[string]map[string]vhostTopicPermission `json:"vhost_topics" structs:"vhost_topics" mapstructure:"vhost_topics"`
}

when using structs.New(role).Map(), we do get a map[string]interface{} out, but this is not recursive: the interior type of the vhost key is still map[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:

  1. Build a parallel, addressable copy of the struct from the parent map (when the current struct is not accessible), and write the modified struct to the map when finished.
  2. Don't copy the struct, but instead, serialize it to JSON (because 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 into map[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

@cipherboy cipherboy added the bug Something isn't working label Aug 20, 2024
@cipherboy
Copy link
Member Author

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.

@cipherboy cipherboy added good first issue Good for newcomers help wanted Extra attention is needed labels Aug 20, 2024
@GeorgeJahad
Copy link

this one looks like a good place to start. @cipherboy can you assign it to me?

@GeorgeJahad
Copy link

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:

if o, ok := v.(logical.OptMarshaler); ok {

// We are looking for time values. If it isn't one, ignore it.
if v.Type() != hashTimeType {
return nil
}

I'm considering alternatives now. Let know if you have any thoughts.

@GeorgeJahad
Copy link

@cipherboy said:

Build a parallel, addressable copy of the struct from the parent map

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?

@cipherboy
Copy link
Member Author

\o hey @GeorgeJahad, sorry, been a busy time :-)

The OptMarshaler/MarshalJSONWithOptions seems entirely unused so I'm not concerned about supporting it.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants