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

sys/config: config state endpoint #7424

Merged
merged 22 commits into from
Oct 8, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7a45f56
sys/config: initial work on adding config state endpoint
calvn Sep 4, 2019
66afa3c
server/config: add tests, fix Sanitized method
calvn Sep 4, 2019
e612b78
thread config through NewTestCluster's config to avoid panic on dev m…
calvn Sep 4, 2019
15f41ee
properly guard endpoint against request forwarding
calvn Sep 4, 2019
c4a9edb
add http tests, guard against panics on nil RawConfig
calvn Sep 5, 2019
9cfbdb3
ensure non-nil rawConfig on NewTestCluster cores
calvn Sep 5, 2019
d1d2919
Merge remote-tracking branch 'origin/master' into sys-config-state
calvn Sep 5, 2019
048961c
update non-forwarding logic
calvn Sep 16, 2019
0813829
Merge remote-tracking branch 'origin/master' into sys-config-state
calvn Sep 17, 2019
31c2454
fix imports; use no-forward handler
calvn Sep 17, 2019
6567fac
add missing config test fixture; update gitignore
calvn Sep 17, 2019
543f765
Merge remote-tracking branch 'origin/master' into sys-config-state
calvn Sep 20, 2019
26fdcb5
return sanitized config as a map
calvn Sep 20, 2019
7fef0b8
fix test, use deep.Equal to check for equality
calvn Sep 21, 2019
66eb350
fix http test
calvn Sep 21, 2019
507f708
Merge remote-tracking branch 'origin/master' into sys-config-state
calvn Oct 3, 2019
95ab4e0
minor comment fix
calvn Oct 3, 2019
da85f38
config: change Sanitized to return snake-cased keys, update tests
calvn Oct 7, 2019
219d6ee
Merge remote-tracking branch 'origin/master' into sys-config-state
calvn Oct 7, 2019
d5b5a9f
core: hold rlock when reading config; add docstring
calvn Oct 7, 2019
d62aa0f
update docstring
calvn Oct 8, 2019
86a808c
Merge branch 'master' into sys-config-state
calvn Oct 8, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion command/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,7 @@ func (c *ServerCommand) Run(args []string) int {
}

coreConfig := &vault.CoreConfig{
RawConfig: config,
Physical: backend,
RedirectAddr: config.Storage.RedirectAddr,
HAPhysical: nil,
Expand Down Expand Up @@ -966,7 +967,7 @@ CLUSTER_SYNTHESIS_COMPLETE:
}
props["max_request_size"] = fmt.Sprintf("%d", maxRequestSize)

var maxRequestDuration time.Duration = vault.DefaultMaxRequestDuration
maxRequestDuration := vault.DefaultMaxRequestDuration
if valRaw, ok := lnConfig.Config["max_request_duration"]; ok {
val, err := parseutil.ParseDurationSecond(valRaw)
if err != nil {
Expand Down Expand Up @@ -1387,6 +1388,8 @@ CLUSTER_SYNTHESIS_COMPLETE:
goto RUNRELOADFUNCS
}

core.SetConfig(config)

if config.LogLevel != "" {
configLogLevel := strings.ToLower(strings.TrimSpace(config.LogLevel))
switch configLogLevel {
Expand Down
117 changes: 117 additions & 0 deletions command/server/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -905,3 +905,120 @@ func parseTelemetry(result *Config, list *ast.ObjectList) error {

return nil
}

// Sanitized returns a copy of the config with all values that are considered
// sensitive stripped. It also strips all raw values that are mainly
// used for parsing.
//
// Specifically, the fields that this method strips are:
// - Storage.Config
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@calvn calvn Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// - HAStorage.Config
// - Seals.Config
// - Telemetry.CirconusAPIToken
func (c *Config) Sanitized() *Config {
// Sanitize storage stanza
var sanitizedStorage *Storage
if c.Storage != nil {
sanitizedStorage = &Storage{
Type: c.Storage.Type,
RedirectAddr: c.Storage.RedirectAddr,
ClusterAddr: c.Storage.ClusterAddr,
DisableClustering: c.Storage.DisableClustering,
}
}

// Sanitize HA storage stanza
var sanitizedHAStorage *Storage
if c.HAStorage != nil {
sanitizedHAStorage = &Storage{
Type: c.HAStorage.Type,
RedirectAddr: c.HAStorage.RedirectAddr,
ClusterAddr: c.HAStorage.ClusterAddr,
DisableClustering: c.HAStorage.DisableClustering,
}
}

// Sanitize seals stanza
var sanitizedSeals []*Seal
if len(c.Seals) != 0 {
for _, s := range c.Seals {
cleanSeal := &Seal{
Type: s.Type,
Disabled: s.Disabled,
}
sanitizedSeals = append(sanitizedSeals, cleanSeal)
}
}

// Sanitize telemetry stanza
var sanitizedTelemetry *Telemetry
if c.Telemetry != nil {
sanitizedTelemetry = &Telemetry{
StatsiteAddr: c.Telemetry.StatsiteAddr,
StatsdAddr: c.Telemetry.StatsdAddr,
DisableHostname: c.Telemetry.DisableHostname,
CirconusAPIToken: "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CirconusAPIApp: c.Telemetry.CirconusAPIApp,
CirconusAPIURL: c.Telemetry.CirconusAPIURL,
CirconusSubmissionInterval: c.Telemetry.CirconusSubmissionInterval,
CirconusCheckSubmissionURL: c.Telemetry.CirconusCheckSubmissionURL,
CirconusCheckID: c.Telemetry.CirconusCheckID,
CirconusCheckForceMetricActivation: c.Telemetry.CirconusCheckForceMetricActivation,
CirconusCheckInstanceID: c.Telemetry.CirconusCheckInstanceID,
CirconusCheckSearchTag: c.Telemetry.CirconusCheckSearchTag,
CirconusCheckTags: c.Telemetry.CirconusCheckTags,
CirconusCheckDisplayName: c.Telemetry.CirconusCheckDisplayName,
CirconusBrokerID: c.Telemetry.CirconusBrokerID,
CirconusBrokerSelectTag: c.Telemetry.CirconusBrokerSelectTag,
DogStatsDAddr: c.Telemetry.DogStatsDAddr,
DogStatsDTags: c.Telemetry.DogStatsDTags,
PrometheusRetentionTime: c.Telemetry.PrometheusRetentionTime,
PrometheusRetentionTimeRaw: c.Telemetry.PrometheusRetentionTimeRaw,
StackdriverProjectID: c.Telemetry.StackdriverProjectID,
StackdriverLocation: c.Telemetry.StackdriverLocation,
StackdriverNamespace: c.Telemetry.StackdriverNamespace,
}
}

return &Config{
Listeners: c.Listeners,
Storage: sanitizedStorage,
HAStorage: sanitizedHAStorage,
Seals: sanitizedSeals,

CacheSize: c.CacheSize,
DisableCache: c.DisableCache,
DisableMlock: c.DisableMlock,
DisablePrintableCheck: c.DisablePrintableCheck,

EnableUI: c.EnableUI,

Telemetry: sanitizedTelemetry,

MaxLeaseTTL: c.MaxLeaseTTL,
DefaultLeaseTTL: c.DefaultLeaseTTL,

DefaultMaxRequestDuration: c.DefaultMaxRequestDuration,

ClusterName: c.ClusterName,
ClusterCipherSuites: c.ClusterCipherSuites,

PluginDirectory: c.PluginDirectory,

LogLevel: c.LogLevel,
LogFormat: c.LogFormat,

PidFile: c.PidFile,
EnableRawEndpoint: c.EnableRawEndpoint,

APIAddr: c.APIAddr,
ClusterAddr: c.ClusterAddr,
DisableClustering: c.DisableClustering,

DisablePerformanceStandby: c.DisablePerformanceStandby,

DisableSealWrap: c.DisableSealWrap,

DisableIndexing: c.DisableIndexing,
}
}
66 changes: 66 additions & 0 deletions command/server/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,72 @@ func TestLoadConfigDir(t *testing.T) {
}
}

func TestConfig_Sanitized(t *testing.T) {
config, err := LoadConfigFile("./test-fixtures/config3.hcl")
if err != nil {
t.Fatalf("err: %s", err)
}

expected := &Config{
Listeners: []*Listener{
&Listener{
Type: "tcp",
Config: map[string]interface{}{
"address": "127.0.0.1:443",
},
},
},

Storage: &Storage{
Type: "consul",
RedirectAddr: "top_level_api_addr",
ClusterAddr: "top_level_cluster_addr",
},

HAStorage: &Storage{
Type: "consul",
RedirectAddr: "top_level_api_addr",
ClusterAddr: "top_level_cluster_addr",
DisableClustering: true,
},

Telemetry: &Telemetry{
StatsdAddr: "bar",
PrometheusRetentionTime: prometheusDefaultRetentionTime,
},

Seals: []*Seal{
{
Type: "awskms",
Disabled: false,
},
},

DisableCache: true,
DisableMlock: true,
EnableUI: true,

EnableRawEndpoint: true,

DisableSealWrap: true,

MaxLeaseTTL: 10 * time.Hour,
DefaultLeaseTTL: 10 * time.Hour,
ClusterName: "testcluster",

PidFile: "./pidfile",

APIAddr: "top_level_api_addr",
ClusterAddr: "top_level_cluster_addr",
}

sanitizedConfig := config.Sanitized()

if !reflect.DeepEqual(sanitizedConfig, expected) {
t.Fatalf("expected \n\n%#v\n\n to be \n\n%#v\n\n", sanitizedConfig, expected)
}
}

func TestParseListeners(t *testing.T) {
obj, _ := hcl.Parse(strings.TrimSpace(`
listener "tcp" {
Expand Down
21 changes: 21 additions & 0 deletions http/forwarding_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -582,3 +582,24 @@ func TestHTTP_Forwarding_HelpOperation(t *testing.T) {
testHelp(cores[0].Client)
testHelp(cores[1].Client)
}

func TestHTTP_Forwarding_LocalOnly(t *testing.T) {
cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{
HandlerFunc: Handler,
})
cluster.Start()
defer cluster.Cleanup()
cores := cluster.Cores

vault.TestWaitActive(t, cores[0].Core)

testLocalOnly := func(client *api.Client) {
_, err := client.Logical().Read("sys/config/state/sanitized")
if err == nil {
t.Fatal("expected error")
}
}

testLocalOnly(cores[1].Client)
testLocalOnly(cores[2].Client)
}
20 changes: 14 additions & 6 deletions http/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,14 +514,15 @@ func parseRequest(core *vault.Core, r *http.Request, w http.ResponseWriter, out
// falling back on the older behavior of redirecting the client
func handleRequestForwarding(core *vault.Core, handler http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ns, err := namespace.FromContext(r.Context())
if err != nil {
respondError(w, http.StatusBadRequest, err)
return
}
path := ns.TrimmedPath(r.URL.Path[len("/v1/"):])

// If we are a performance standby we can handle the request.
if core.PerfStandby() {
ns, err := namespace.FromContext(r.Context())
if err != nil {
respondError(w, http.StatusBadRequest, err)
return
}
path := ns.TrimmedPath(r.URL.Path[len("/v1/"):])
switch {
case !perfStandbyAlwaysForwardPaths.HasPath(path) && !alwaysRedirectPaths.HasPath(path):
handler.ServeHTTP(w, r)
Expand Down Expand Up @@ -559,6 +560,13 @@ func handleRequestForwarding(core *vault.Core, handler http.Handler) http.Handle
return
}

// These requests should always be served locally
calvn marked this conversation as resolved.
Show resolved Hide resolved
switch {
case strings.HasPrefix(path, "sys/config/state/"):
respondError(w, http.StatusBadRequest, vault.ErrCannotForwardLocalOnly)
return
}

forwardRequest(core, w, r)
return
})
Expand Down
7 changes: 7 additions & 0 deletions http/logical.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,13 @@ func handleLogicalInternal(core *vault.Core, injectDataIntoTopLevel bool) http.H
// success.
resp, ok, needsForward := request(core, w, r, req)
if needsForward {
// Do not forward these specific requests since they are only
// applicable to the local node.
if strings.HasPrefix(req.Path, "sys/config/state/") {
respondError(w, http.StatusBadRequest, vault.ErrCannotForwardLocalOnly)
return
}

if origBody != nil {
r.Body = origBody
}
Expand Down
70 changes: 70 additions & 0 deletions http/sys_config_state_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package http

import (
"encoding/json"
"net/http"
"reflect"
"strconv"
"strings"
"testing"
"time"

"github.com/fatih/structs"
"github.com/hashicorp/vault/command/server"
"github.com/hashicorp/vault/vault"
)

func TestSysConfigState_Sanitized(t *testing.T) {
var resp *http.Response

core, _, token := vault.TestCoreUnsealed(t)
ln, addr := TestServer(t, core)
defer ln.Close()
TestServerAuth(t, addr, token)

resp = testHttpGet(t, token, addr+"/v1/sys/config/state/sanitized")
testResponseStatus(t, resp, 200)

var actual map[string]interface{}
var expected map[string]interface{}

expectedConfig := new(server.Config)
configResp := structs.New(expectedConfig.Sanitized()).Map()

var nilObject interface{}
// Do some surgery on the expected config to line up the
// types and string the raw fields.
for k, v := range configResp {
if strings.HasSuffix(k, "Raw") {
delete(configResp, k)
continue
}
switch v.(type) {
case int:
configResp[k] = json.Number(strconv.Itoa(v.(int)))
case time.Duration:
configResp[k] = json.Number(strconv.Itoa(int(v.(time.Duration))))
}
}
configResp["HAStorage"] = nilObject
configResp["Storage"] = nilObject
configResp["Telemetry"] = nilObject

expected = map[string]interface{}{
"lease_id": "",
"renewable": false,
"lease_duration": json.Number("0"),
"wrap_info": nil,
"warnings": nil,
"auth": nil,
"data": configResp,
}

testResponseBody(t, resp, &actual)
expected["request_id"] = actual["request_id"]

if !reflect.DeepEqual(actual, expected) {
t.Fatalf("bad mismatch response body:\nexpected:\n%#v\nactual:\n%#v", expected, actual)
}

}
3 changes: 2 additions & 1 deletion vault/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ const (
)

var (
ErrCannotForward = errors.New("cannot forward request; no connection or address not known")
ErrCannotForward = errors.New("cannot forward request; no connection or address not known")
ErrCannotForwardLocalOnly = errors.New("cannot forward local-only request")
)

type ClusterLeaderParams struct {
Expand Down
Loading