sys/config: config state endpoint #7424
Conversation
command/server/config.go
Outdated
| StatsiteAddr: c.Telemetry.StatsiteAddr, | ||
| StatsdAddr: c.Telemetry.StatsdAddr, | ||
| DisableHostname: c.Telemetry.DisableHostname, | ||
| CirconusAPIToken: "", |
There was a problem hiding this comment.
Wouldn't it be easier to copy c.Telemetry and then overwrite CirconusAPIToken with ""? That would also save us having to update this code any time we add a new field.
There was a problem hiding this comment.
My concern of doing it this way is that we could accidentally output any newly added sensitive fields if we forget to overwrite it here.
| // used for parsing. | ||
| // | ||
| // Specifically, the fields that this method strips are: | ||
| // - Storage.Config |
There was a problem hiding this comment.
It feels like stripping out Storage.Config is extreme. I understand that it's impractical to be more fine-grained in general, but our enterprise customers are mostly going to be using Consul and Raft. Can we special-case those and only strip out the truly sensitive config, e.g. consul token?
There was a problem hiding this comment.
If we were going to show certain values from Config, I think the safer approach would be to have an allow-list rather than a set of values to strip, as they could be easily overlooked/forgotten since it's better to forget adding a non-sensitive value than leaking a sensitive one. I started down this road initially, but stopped after realizing the numerous types of storage backends that we support and the number of fields that we'd need to include for each of those.
I think the cleaner approach would be to have a separate endpoint that displayed the complete configuration file params, including the Config map (and that could be included in the debug bundle in a future release), but didn't want to add this prematurely.
# Conflicts: # http/logical.go
vault/logical_system.go
Outdated
| func (b *SystemBackend) handleConfigStateSanitized(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { | ||
| config := b.Core.SanitizedConfig() | ||
|
|
||
| configMap := structs.New(config).Map() |
There was a problem hiding this comment.
Don't love the fact that we need to use structs and then pull out values that end in Raw. Seems fragile if new values are added to the config struct that don't happen to end in Raw. I wonder if instead we can have b.Core.SanitizedConfig() return a map instead of a config object?
There was a problem hiding this comment.
We could have SanitizedConfig() return a map instead, but we'd still have the same problem of empty Raw* values in the map that will show up in the response so we'd just be removing them there as opposed to here.
Alternatively, we could have structs:"-" tags added to the raw fields in server.Config. However, I could see a case were we might want to have these show up in the future, so I'm not sure if always omitting them is the right call either.
There was a problem hiding this comment.
I've updated it to return a map directly as discussed offline via 26fdcb5. There's room for improvement on how we can curate this better, but this should give us something to work with for debug.
This PR adds a new endpoint for querying the configuration that the current Vault server is using. Since this endpoint is meant to be queried locally, there are checks in place to prevent forwarding on these types of requests.
The endpoint ended up being
sys/config/state/sanitizedsince it offers the most flexibility in terms of allowing proper ACL against the it while keeping the options open to allow other types of config states to be fetched in the future such asconfig/state/raw(which could include raw and sensitive values) orconfig/state/parsed(which could include finalized values used by core).