From 420ae3df698b84942bc31f57563a3dea6fd0b3da Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Mon, 18 Jun 2018 20:37:00 +0100 Subject: [PATCH] Limit proxy telemetry config to only be visible with authenticated with a proxy token --- agent/agent.go | 31 +++++++++-------- agent/agent_endpoint.go | 66 +++++++++++++++++++++--------------- agent/agent_endpoint_test.go | 66 +++++++++++++++++++++++++++++++++++- 3 files changed, 120 insertions(+), 43 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index bf4f88c2b7..987a063c2c 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2212,19 +2212,22 @@ func (a *Agent) RemoveProxy(proxyID string, persist bool) error { // The given token may be a local-only proxy token or it may be an ACL token. We // will attempt to verify the local proxy token first. // -// The effective ACL token is returned along with any error. In the case the -// token matches a proxy token, then the ACL token used to register that proxy's -// target service is returned for use in any RPC calls the proxy needs to make -// on behalf of that service. If the token was an ACL token already then it is -// always returned. Provided error is nil, a valid ACL token is always returned. -func (a *Agent) verifyProxyToken(token, targetService, targetProxy string) (string, error) { +// The effective ACL token is returned along with a boolean which is true if the +// match was against a proxy token rather than an ACL token, and any error. In +// the case the token matches a proxy token, then the ACL token used to register +// that proxy's target service is returned for use in any RPC calls the proxy +// needs to make on behalf of that service. If the token was an ACL token +// already then it is always returned. Provided error is nil, a valid ACL token +// is always returned. +func (a *Agent) verifyProxyToken(token, targetService, + targetProxy string) (string, bool, error) { // If we specify a target proxy, we look up that proxy directly. Otherwise, // we resolve with any proxy we can find. var proxy *local.ManagedProxy if targetProxy != "" { proxy = a.State.Proxy(targetProxy) if proxy == nil { - return "", fmt.Errorf("unknown proxy service ID: %q", targetProxy) + return "", false, fmt.Errorf("unknown proxy service ID: %q", targetProxy) } // If the token DOESN'T match, then we reset the proxy which will @@ -2247,32 +2250,32 @@ func (a *Agent) verifyProxyToken(token, targetService, targetProxy string) (stri // represents the existence of a local service. target := a.State.Service(proxy.Proxy.TargetServiceID) if target == nil { - return "", fmt.Errorf("proxy target service not found: %q", + return "", false, fmt.Errorf("proxy target service not found: %q", proxy.Proxy.TargetServiceID) } if target.Service != targetService { - return "", acl.ErrPermissionDenied + return "", false, acl.ErrPermissionDenied } // Resolve the actual ACL token used to register the proxy/service and // return that for use in RPC calls. - return a.State.ServiceToken(proxy.Proxy.TargetServiceID), nil + return a.State.ServiceToken(proxy.Proxy.TargetServiceID), true, nil } // Doesn't match, we have to do a full token resolution. The required - // permission for any proxy-related endpont is service:write, since + // permission for any proxy-related endpoint is service:write, since // to register a proxy you require that permission and sensitive data // is usually present in the configuration. rule, err := a.resolveToken(token) if err != nil { - return "", err + return "", false, err } if rule != nil && !rule.ServiceWrite(targetService, nil) { - return "", acl.ErrPermissionDenied + return "", false, acl.ErrPermissionDenied } - return token, nil + return token, false, nil } func (a *Agent) cancelCheckMonitors(checkID types.CheckID) { diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index f71ea7df4e..4b1c2ff004 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -952,7 +952,7 @@ func (s *HTTPServer) AgentConnectCALeafCert(resp http.ResponseWriter, req *http. // Verify the proxy token. This will check both the local proxy token // as well as the ACL if the token isn't local. - effectiveToken, err := s.agent.verifyProxyToken(qOpts.Token, serviceName, "") + effectiveToken, _, err := s.agent.verifyProxyToken(qOpts.Token, serviceName, "") if err != nil { return nil, err } @@ -1019,7 +1019,7 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http } // Validate the ACL token - _, err := s.agent.verifyProxyToken(token, target.Service, id) + _, isProxyToken, err := s.agent.verifyProxyToken(token, target.Service, id) if err != nil { return "", nil, err } @@ -1063,35 +1063,45 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http target.Port) } - // Add telemetry config. Copy the global config so we can customize the - // prefix. - telemetryCfg := s.agent.config.Telemetry - telemetryCfg.MetricsPrefix = telemetryCfg.MetricsPrefix + ".proxy." + target.ID + // Only merge in telemetry config from agent if the requested is + // authorized with a proxy token. This prevents us leaking potentially + // sensitive config like Circonus API token via a public endpoint. Proxy + // tokens are only ever generated in-memory and passed via ENV to a child + // proxy process so potential for abuse here seems small. This endpoint in + // general is only useful for managed proxies now so it should _always_ be + // true that auth is via a proxy token but inconvenient for testing if we + // lock it down so strictly. + if isProxyToken { + // Add telemetry config. Copy the global config so we can customize the + // prefix. + telemetryCfg := s.agent.config.Telemetry + telemetryCfg.MetricsPrefix = telemetryCfg.MetricsPrefix + ".proxy." + target.ID - // First see if the user has specified telemetry - if userRaw, ok := config["telemetry"]; ok { - // User specified domething, see if it is compatible with agent - // telemetry config: - var uCfg lib.TelemetryConfig - dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - Result: &uCfg, - // Make sure that if the user passes something that isn't just a - // simple override of a valid TelemetryConfig that we fail so that we - // don't clobber their custom config. - ErrorUnused: true, - }) - if err == nil { - if err = dec.Decode(userRaw); err == nil { - // It did decode! Merge any unspecified fields from agent config. - uCfg.MergeDefaults(&telemetryCfg) - config["telemetry"] = uCfg + // First see if the user has specified telemetry + if userRaw, ok := config["telemetry"]; ok { + // User specified domething, see if it is compatible with agent + // telemetry config: + var uCfg lib.TelemetryConfig + dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + Result: &uCfg, + // Make sure that if the user passes something that isn't just a + // simple override of a valid TelemetryConfig that we fail so that we + // don't clobber their custom config. + ErrorUnused: true, + }) + if err == nil { + if err = dec.Decode(userRaw); err == nil { + // It did decode! Merge any unspecified fields from agent config. + uCfg.MergeDefaults(&telemetryCfg) + config["telemetry"] = uCfg + } } + // Failed to decode, just keep user's config["telemetry"] verbatim + // with no agent merge. + } else { + // Add agent telemetry config. + config["telemetry"] = telemetryCfg } - // Failed to decode, just keep user's config["telemetry"] verbatim - // with no agent merge. - } else { - // Add agent telemetry config. - config["telemetry"] = telemetryCfg } reply := &api.ConnectProxyConfig{ diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index f90c53fe01..b7d7c2755a 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -3274,13 +3274,16 @@ func TestAgentConnectProxyConfig_ConfigHandling(t *testing.T) { Check: structs.CheckType{ TTL: 15 * time.Second, }, - Connect: &structs.ServiceConnect{}, + Connect: &structs.ServiceConnect{ + // Proxy is populated with the definition in the table below. + }, } tests := []struct { name string globalConfig string proxy structs.ServiceDefinitionConnectProxy + useToken string wantMode api.ProxyExecMode wantCommand []string wantConfig map[string]interface{} @@ -3502,6 +3505,58 @@ func TestAgentConnectProxyConfig_ConfigHandling(t *testing.T) { }, }, }, + { + name: "reg passed through with no agent config added if not proxy token auth", + useToken: "foo", // no actual ACLs set so this any token will work but has to be non-empty to be used below + globalConfig: ` + bind_addr = "0.0.0.0" + connect { + enabled = true + proxy { + allow_managed_api_registration = true + } + proxy_defaults = { + exec_mode = "daemon" + daemon_command = ["daemon.sh"] + script_command = ["script.sh"] + config = { + connect_timeout_ms = 1000 + } + } + } + ports { + proxy_min_port = 10000 + proxy_max_port = 10000 + } + telemetry { + statsite_address = "localhost:8989" + } + `, + proxy: structs.ServiceDefinitionConnectProxy{ + ExecMode: "script", + Command: []string{"foo.sh"}, + Config: map[string]interface{}{ + "connect_timeout_ms": 2000, + "bind_address": "127.0.0.1", + "bind_port": 1024, + "local_service_address": "127.0.0.1:9191", + "telemetry": map[string]interface{}{ + "metrics_prefix": "foo", + }, + }, + }, + wantMode: api.ProxyExecModeScript, + wantCommand: []string{"foo.sh"}, + wantConfig: map[string]interface{}{ + "bind_address": "127.0.0.1", + "bind_port": float64(1024), + "local_service_address": "127.0.0.1:9191", + "connect_timeout_ms": float64(2000), + "telemetry": map[string]interface{}{ // No defaults merged + "metrics_prefix": "foo", + }, + }, + }, } for _, tt := range tests { @@ -3522,7 +3577,16 @@ func TestAgentConnectProxyConfig_ConfigHandling(t *testing.T) { require.Equal(200, resp.Code, "body: %s", resp.Body.String()) } + proxy := a.State.Proxy("test-id-proxy") + require.NotNil(proxy) + require.NotEmpty(proxy.ProxyToken) + req, _ := http.NewRequest("GET", "/v1/agent/connect/proxy/test-id-proxy", nil) + if tt.useToken != "" { + req.Header.Set("X-Consul-Token", tt.useToken) + } else { + req.Header.Set("X-Consul-Token", proxy.ProxyToken) + } resp := httptest.NewRecorder() obj, err := a.srv.AgentConnectProxyConfig(resp, req) require.NoError(err)