luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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.
This commit is contained in:
R.B. Boyer 2020-10-30 16:49:54 -05:00 committed by GitHub
parent e69d2c99cf
commit c8c87ec317
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 279 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
agent/config/builder.go
View File

@ -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
}

5
agent/config/config.go
View File

@ -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 {

5
agent/config/runtime.go
View File

@ -1540,8 +1540,9 @@ type UIConfig struct {
}
type UIMetricsProxy struct {
BaseURL string
AddHeaders []UIMetricsProxyAddHeader
BaseURL string
AddHeaders []UIMetricsProxyAddHeader
PathAllowlist []string
}
type UIMetricsProxyAddHeader struct {

191
agent/config/runtime_test.go
View File

@ -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": {}
},

23
agent/ui_endpoint.go
View File

@ -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

19
agent/ui_endpoint_test.go
View File

@ -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{

2
test/integration/connect/envoy/helpers.bash
View File

@ -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 ]
}