openbao / openbao

OpenBao exists to provide a software solution to manage, store, and distribute sensitive data including secrets, certificates, and keys.
https://openbao.org/
Mozilla Public License 2.0
2.82k stars 111 forks source link

Audit logging panic investigation #478

Open cipherboy opened 3 weeks ago

cipherboy commented 3 weeks ago

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 particular 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 sitll 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 8d42b5f684f35dccac9ad5822f77e87277a3f366), 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 commented 3 weeks ago

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.