From c8c87ec317225ffde13190d976b371e3f25f7056 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Fri, 30 Oct 2020 16:49:54 -0500 Subject: [PATCH] agent: introduce path allow list for requests going through the metrics proxy (#9059) Added a new option `ui_config.metrics_proxy.path_allowlist`. This defaults to `["/api/v1/query", "/api/v1/query_range"]` when the metrics provider is set to `prometheus`. Requests that do not use one of the allow-listed paths (via exact match) get a 403 Forbidden response instead. --- agent/config/builder.go | 42 ++++- agent/config/config.go | 5 +- agent/config/runtime.go | 5 +- agent/config/runtime_test.go | 191 +++++++++++++++++++- agent/ui_endpoint.go | 23 +++ agent/ui_endpoint_test.go | 19 ++ test/integration/connect/envoy/helpers.bash | 2 + 7 files changed, 279 insertions(+), 8 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 5f6292d94..f6cf06d64 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -9,6 +9,7 @@ import ( "net" "net/url" "os" + "path" "path/filepath" "reflect" "regexp" @@ -1102,6 +1103,16 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { return RuntimeConfig{}, fmt.Errorf("cache.entry_fetch_rate must be strictly positive, was: %v", rt.Cache.EntryFetchRate) } + if rt.UIConfig.MetricsProvider == "prometheus" { + // Handle defaulting for the built-in version of prometheus. + if len(rt.UIConfig.MetricsProxy.PathAllowlist) == 0 { + rt.UIConfig.MetricsProxy.PathAllowlist = []string{ + "/api/v1/query", + "/api/v1/query_range", + } + } + } + if err := b.BuildEnterpriseRuntimeConfig(&rt, &c); err != nil { return rt, err } @@ -1180,6 +1191,11 @@ func (b *Builder) Validate(rt RuntimeConfig) error { rt.UIConfig.MetricsProxy.BaseURL) } } + for _, allowedPath := range rt.UIConfig.MetricsProxy.PathAllowlist { + if err := validateAbsoluteURLPath(allowedPath); err != nil { + return fmt.Errorf("ui_config.metrics_proxy.path_allowlist: %v", err) + } + } for k, v := range rt.UIConfig.DashboardURLTemplates { if err := validateBasicName("ui_config.dashboard_url_templates key names", k, false); err != nil { return err @@ -1746,8 +1762,9 @@ func (b *Builder) uiMetricsProxyVal(v RawUIMetricsProxy) UIMetricsProxy { } return UIMetricsProxy{ - BaseURL: b.stringVal(v.BaseURL), - AddHeaders: hdrs, + BaseURL: b.stringVal(v.BaseURL), + AddHeaders: hdrs, + PathAllowlist: v.PathAllowlist, } } @@ -2325,3 +2342,24 @@ func validateRemoteScriptsChecks(conf RuntimeConfig) error { } return nil } + +func validateAbsoluteURLPath(p string) error { + if !path.IsAbs(p) { + return fmt.Errorf("path %q is not an absolute path", p) + } + + // A bit more extra validation that these are actually paths. + u, err := url.Parse(p) + if err != nil || + u.Scheme != "" || + u.Opaque != "" || + u.User != nil || + u.Host != "" || + u.RawQuery != "" || + u.Fragment != "" || + u.Path != p { + return fmt.Errorf("path %q is not an absolute path", p) + } + + return nil +} diff --git a/agent/config/config.go b/agent/config/config.go index 41af5360e..f966297ba 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -797,8 +797,9 @@ type RawUIConfig struct { } type RawUIMetricsProxy struct { - BaseURL *string `json:"base_url,omitempty" hcl:"base_url" mapstructure:"base_url"` - AddHeaders []RawUIMetricsProxyAddHeader `json:"add_headers,omitempty" hcl:"add_headers" mapstructure:"add_headers"` + BaseURL *string `json:"base_url,omitempty" hcl:"base_url" mapstructure:"base_url"` + AddHeaders []RawUIMetricsProxyAddHeader `json:"add_headers,omitempty" hcl:"add_headers" mapstructure:"add_headers"` + PathAllowlist []string `json:"path_allowlist,omitempty" hcl:"path_allowlist" mapstructure:"path_allowlist"` } type RawUIMetricsProxyAddHeader struct { diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 1fd3db430..c65642638 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -1540,8 +1540,9 @@ type UIConfig struct { } type UIMetricsProxy struct { - BaseURL string - AddHeaders []UIMetricsProxyAddHeader + BaseURL string + AddHeaders []UIMetricsProxyAddHeader + PathAllowlist []string } type UIMetricsProxyAddHeader struct { diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 42adf5986..425613a6c 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -4545,6 +4545,189 @@ func TestBuilder_BuildAndValidate_ConfigFlagsAndEdgecases(t *testing.T) { `}, err: `ui_config.metrics_proxy.base_url must be a valid http or https URL.`, }, + { + desc: "metrics_proxy.path_allowlist invalid (empty)", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["", "/foo"] + } + } + `}, + err: `ui_config.metrics_proxy.path_allowlist: path "" is not an absolute path`, + }, + { + desc: "metrics_proxy.path_allowlist invalid (relative)", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["bar/baz", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["bar/baz", "/foo"] + } + } + `}, + err: `ui_config.metrics_proxy.path_allowlist: path "bar/baz" is not an absolute path`, + }, + { + desc: "metrics_proxy.path_allowlist invalid (weird)", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["://bar/baz", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["://bar/baz", "/foo"] + } + } + `}, + err: `ui_config.metrics_proxy.path_allowlist: path "://bar/baz" is not an absolute path`, + }, + { + desc: "metrics_proxy.path_allowlist invalid (fragment)", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["/bar/baz#stuff", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["/bar/baz#stuff", "/foo"] + } + } + `}, + err: `ui_config.metrics_proxy.path_allowlist: path "/bar/baz#stuff" is not an absolute path`, + }, + { + desc: "metrics_proxy.path_allowlist invalid (querystring)", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["/bar/baz?stu=ff", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["/bar/baz?stu=ff", "/foo"] + } + } + `}, + err: `ui_config.metrics_proxy.path_allowlist: path "/bar/baz?stu=ff" is not an absolute path`, + }, + { + desc: "metrics_proxy.path_allowlist invalid (encoded slash)", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["/bar%2fbaz", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["/bar%2fbaz", "/foo"] + } + } + `}, + err: `ui_config.metrics_proxy.path_allowlist: path "/bar%2fbaz" is not an absolute path`, + }, + { + desc: "metrics_proxy.path_allowlist ok", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_proxy": { + "path_allowlist": ["/bar/baz", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_proxy { + path_allowlist = ["/bar/baz", "/foo"] + } + } + `}, + patch: func(rt *RuntimeConfig) { + rt.UIConfig.MetricsProxy.PathAllowlist = []string{"/bar/baz", "/foo"} + rt.DataDir = dataDir + }, + }, + { + desc: "metrics_proxy.path_allowlist defaulted for prometheus", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_provider": "prometheus" + } + }`}, + hcl: []string{` + ui_config { + metrics_provider = "prometheus" + } + `}, + patch: func(rt *RuntimeConfig) { + rt.UIConfig.MetricsProvider = "prometheus" + rt.UIConfig.MetricsProxy.PathAllowlist = []string{ + "/api/v1/query", + "/api/v1/query_range", + } + rt.DataDir = dataDir + }, + }, + { + desc: "metrics_proxy.path_allowlist not overridden with defaults for prometheus", + args: []string{`-data-dir=` + dataDir}, + json: []string{`{ + "ui_config": { + "metrics_provider": "prometheus", + "metrics_proxy": { + "path_allowlist": ["/bar/baz", "/foo"] + } + } + }`}, + hcl: []string{` + ui_config { + metrics_provider = "prometheus" + metrics_proxy { + path_allowlist = ["/bar/baz", "/foo"] + } + } + `}, + patch: func(rt *RuntimeConfig) { + rt.UIConfig.MetricsProvider = "prometheus" + rt.UIConfig.MetricsProxy.PathAllowlist = []string{"/bar/baz", "/foo"} + rt.DataDir = dataDir + }, + }, { desc: "metrics_proxy.base_url http(s)", args: []string{`-data-dir=` + dataDir}, @@ -5459,7 +5642,8 @@ func TestFullConfig(t *testing.T) { "name": "p3nynwc9", "value": "TYBgnN2F" } - ] + ], + "path_allowlist": ["/aSh3cu", "/eiK/2Th"] }, "dashboard_url_templates": { "u2eziu2n_lower_case": "http://lkjasd.otr" @@ -6149,6 +6333,7 @@ func TestFullConfig(t *testing.T) { value = "TYBgnN2F" } ] + path_allowlist = ["/aSh3cu", "/eiK/2Th"] } dashboard_url_templates { u2eziu2n_lower_case = "http://lkjasd.otr" @@ -6942,6 +7127,7 @@ func TestFullConfig(t *testing.T) { Value: "TYBgnN2F", }, }, + PathAllowlist: []string{"/aSh3cu", "/eiK/2Th"}, }, DashboardURLTemplates: map[string]string{"u2eziu2n_lower_case": "http://lkjasd.otr"}, }, @@ -7627,7 +7813,8 @@ func TestSanitize(t *testing.T) { "MetricsProviderOptionsJSON": "", "MetricsProxy": { "AddHeaders": [], - "BaseURL": "" + "BaseURL": "", + "PathAllowlist": [] }, "DashboardURLTemplates": {} }, diff --git a/agent/ui_endpoint.go b/agent/ui_endpoint.go index 842f45cae..603d32436 100644 --- a/agent/ui_endpoint.go +++ b/agent/ui_endpoint.go @@ -589,6 +589,29 @@ func (s *HTTPHandlers) UIMetricsProxy(resp http.ResponseWriter, req *http.Reques // double slashes etc. u.Path = path.Clean(u.Path) + if len(cfg.PathAllowlist) > 0 { + // This could be done better with a map, but for the prometheus default + // integration this list has two items in it, so the straight iteration + // isn't awful. + denied := true + for _, allowedPath := range cfg.PathAllowlist { + if u.Path == allowedPath { + denied = false + break + } + } + if denied { + log.Error("target URL path is not allowed", + "base_url", cfg.BaseURL, + "path", subPath, + "target_url", u.String(), + "path_allowlist", cfg.PathAllowlist, + ) + resp.WriteHeader(http.StatusForbidden) + return nil, nil + } + } + // Pass through query params u.RawQuery = req.URL.RawQuery diff --git a/agent/ui_endpoint_test.go b/agent/ui_endpoint_test.go index 966ac1254..2fec77cc2 100644 --- a/agent/ui_endpoint_test.go +++ b/agent/ui_endpoint_test.go @@ -1631,6 +1631,25 @@ func TestUIEndpoint_MetricsProxy(t *testing.T) { path: endpointPath + "/ok", wantCode: http.StatusNotFound, }, + { + name: "blocked path", + config: config.UIMetricsProxy{ + BaseURL: backendURL, + PathAllowlist: []string{"/some/other-prefix/ok"}, + }, + path: endpointPath + "/ok", + wantCode: http.StatusForbidden, + }, + { + name: "allowed path", + config: config.UIMetricsProxy{ + BaseURL: backendURL, + PathAllowlist: []string{"/some/prefix/ok"}, + }, + path: endpointPath + "/ok", + wantCode: http.StatusOK, + wantContains: "OK", + }, { name: "basic proxying", config: config.UIMetricsProxy{ diff --git a/test/integration/connect/envoy/helpers.bash b/test/integration/connect/envoy/helpers.bash index bbe933a72..fce83cb45 100755 --- a/test/integration/connect/envoy/helpers.bash +++ b/test/integration/connect/envoy/helpers.bash @@ -157,6 +157,7 @@ function assert_envoy_http_rbac_policy_count { local EXPECT_COUNT=$2 GOT_COUNT=$(get_envoy_http_rbac_once $HOSTPORT | jq '.rules.policies | length') + echo "GOT_COUNT = $GOT_COUNT" [ "${GOT_COUNT:-0}" -eq $EXPECT_COUNT ] } @@ -172,6 +173,7 @@ function assert_envoy_network_rbac_policy_count { local EXPECT_COUNT=$2 GOT_COUNT=$(get_envoy_network_rbac_once $HOSTPORT | jq '.rules.policies | length') + echo "GOT_COUNT = $GOT_COUNT" [ "${GOT_COUNT:-0}" -eq $EXPECT_COUNT ] }