consul icon indicating copy to clipboard operation
consul copied to clipboard
verified publisher icon hashicorp

fix: deregister service id in url format

Open huikang opened this issue 3 years ago • 0 comments

Description

  • When deregistering a service with an ID, which has an URL format (e.g., http://myservice.com), the http handler will return 301. This PR move the service id from path to the url’s query so that it can be passed to the agent endpoint’s handler.

Testing & Reproduction steps

  1. Register an service with ID in URL format
  2. Use cli to deregister the service and will get error

Links

fix https://github.com/hashicorp/consul/issues/13913

PR Checklist

  • [x] updated test coverage
  • [ ] external facing docs updated
  • [x] not a security concern

huikang avatar Aug 04 '22 15:08 huikang

@mkeeler : is this something we should change generally for all endpoints that accept unvalidated input in path parameters?

We could still merge just this PR first, but I'm concerned this will pop up for other endpoints and, while it's fresh, it might be best to address the others as well.

jkirschner-hashicorp avatar Aug 17 '22 12:08 jkirschner-hashicorp

@mkeeler , thanks so much for your feedback.

@jkirschner-hashicorp , I did a quick search of strings.TrimPrefix in our external facing endpoint files; I will sift the results to make sure we won't miss anything else and report back. Thanks.

./agent/acl_endpoint.go:	policyID := strings.TrimPrefix(req.URL.Path, "/v1/acl/policy/")
./agent/acl_endpoint.go:	policyName := strings.TrimPrefix(req.URL.Path, "/v1/acl/policy/name/")
./agent/acl_endpoint.go:	tokenID := strings.TrimPrefix(req.URL.Path, "/v1/acl/token/")
./agent/acl_endpoint.go:	roleID := strings.TrimPrefix(req.URL.Path, "/v1/acl/role/")
./agent/acl_endpoint.go:	roleName := strings.TrimPrefix(req.URL.Path, "/v1/acl/role/name/")
./agent/acl_endpoint.go:	bindingRuleID := strings.TrimPrefix(req.URL.Path, "/v1/acl/binding-rule/")
./agent/acl_endpoint.go:	methodName := strings.TrimPrefix(req.URL.Path, "/v1/acl/auth-method/")
./agent/agent_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/")
./agent/agent_endpoint.go:	addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/join/")
./agent/agent_endpoint.go:	addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/force-leave/")
./agent/agent_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/deregister/")
./agent/agent_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/pass/")
./agent/agent_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/warn/")
./agent/agent_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/fail/")
./agent/agent_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/update/")
./agent/agent_endpoint.go:	serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/id/")
./agent/agent_endpoint.go:	serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/name/")
./agent/agent_endpoint.go:	serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/deregister/")
./agent/agent_endpoint.go:	serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/maintenance/")
./agent/agent_endpoint.go:	target := strings.TrimPrefix(req.URL.Path, "/v1/agent/token/")
./agent/agent_endpoint.go:	serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/")
./agent/catalog_endpoint.go:	args.ServiceName = strings.TrimPrefix(req.URL.Path, pathPrefix)
./agent/catalog_endpoint.go:	args.Node = strings.TrimPrefix(req.URL.Path, "/v1/catalog/node/")
./agent/catalog_endpoint.go:	args.Node = strings.TrimPrefix(req.URL.Path, "/v1/catalog/node-services/")
./agent/catalog_endpoint.go:	args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/catalog/gateway-services/")
./agent/config_endpoint.go:	kindAndName := strings.TrimPrefix(req.URL.Path, "/v1/config/")
./agent/config_endpoint.go:	kindAndName := strings.TrimPrefix(req.URL.Path, "/v1/config/")
./agent/coordinate_endpoint.go:	node := strings.TrimPrefix(req.URL.Path, "/v1/coordinate/node/")
./agent/discovery_chain_endpoint.go:	args.Name = strings.TrimPrefix(req.URL.Path, "/v1/discovery-chain/")
./agent/event_endpoint.go:	event.Name = strings.TrimPrefix(req.URL.Path, "/v1/event/fire/")
./agent/federation_state_endpoint.go:	datacenterName := strings.TrimPrefix(req.URL.Path, "/v1/internal/federation-state/")
./agent/health_endpoint.go:	args.State = strings.TrimPrefix(req.URL.Path, "/v1/health/state/")
./agent/health_endpoint.go:	args.Node = strings.TrimPrefix(req.URL.Path, "/v1/health/node/")
./agent/health_endpoint.go:	args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/health/checks/")
./agent/health_endpoint.go:	args.ServiceName = strings.TrimPrefix(req.URL.Path, prefix)
./agent/intentions_endpoint.go:			peerName := strings.TrimPrefix(ss[0], "peer:")
./agent/intentions_endpoint.go:			peerName := strings.TrimPrefix(ss[0], "peer:")
./agent/intentions_endpoint.go:	id := strings.TrimPrefix(req.URL.Path, "/v1/connect/intentions/")
./agent/kvs_endpoint.go:	args.Key = strings.TrimPrefix(req.URL.Path, "/v1/kv/")
./agent/peering_endpoint.go:	name := strings.TrimPrefix(req.URL.Path, "/v1/peering/")
./agent/prepared_query_endpoint.go:	id := strings.TrimPrefix(path, "/v1/query/")
./agent/session_endpoint.go:	args.Session.ID = strings.TrimPrefix(req.URL.Path, "/v1/session/destroy/")
./agent/session_endpoint.go:	args.SessionID = strings.TrimPrefix(req.URL.Path, "/v1/session/renew/")
./agent/session_endpoint.go:	args.SessionID = strings.TrimPrefix(req.URL.Path, "/v1/session/info/")
./agent/session_endpoint.go:	args.Node = strings.TrimPrefix(req.URL.Path, "/v1/session/node/")
./agent/ui_endpoint.go:	args.Node = strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/node/")
./agent/ui_endpoint.go:	args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/gateway-services-nodes/")
./agent/ui_endpoint.go:	args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/service-topology/")
./agent/ui_endpoint.go:	name := strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/gateway-intentions/")
./agent/ui_endpoint.go:	subPath := strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/metrics-proxy")

huikang avatar Aug 17 '22 14:08 huikang

  • [ ] ./agent/acl_endpoint.go: policyID := strings.TrimPrefix(req.URL.Path, "/v1/acl/policy/")
  • [ ] ./agent/acl_endpoint.go: policyName := strings.TrimPrefix(req.URL.Path, "/v1/acl/policy/name/")
  • [ ] ./agent/acl_endpoint.go: tokenID := strings.TrimPrefix(req.URL.Path, "/v1/acl/token/")
  • [ ] ./agent/acl_endpoint.go: roleID := strings.TrimPrefix(req.URL.Path, "/v1/acl/role/")
  • [ ] ./agent/acl_endpoint.go: roleName := strings.TrimPrefix(req.URL.Path, "/v1/acl/role/name/")
  • [ ] ./agent/acl_endpoint.go: bindingRuleID := strings.TrimPrefix(req.URL.Path, "/v1/acl/binding-rule/")
  • [ ] ./agent/acl_endpoint.go: methodName := strings.TrimPrefix(req.URL.Path, "/v1/acl/auth-method/")
  • [ ] ./agent/agent_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/")
  • [ ] ./agent/agent_endpoint.go: addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/join/")
  • [ ] ./agent/agent_endpoint.go: addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/force-leave/")
  • [ ] ./agent/agent_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/deregister/")
  • [ ] ./agent/agent_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/pass/")
  • [ ] ./agent/agent_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/warn/")
  • [ ] ./agent/agent_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/fail/")
  • [ ] ./agent/agent_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/agent/check/update/")
  • [ ] ./agent/agent_endpoint.go: serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/id/")
  • [ ] ./agent/agent_endpoint.go: serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/name/")
  • [ ] ./agent/agent_endpoint.go: serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/deregister/")
  • [ ] ./agent/agent_endpoint.go: serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/maintenance/")
  • [ ] ./agent/agent_endpoint.go: target := strings.TrimPrefix(req.URL.Path, "/v1/agent/token/")
  • [ ] ./agent/agent_endpoint.go: serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/")
  • [ ] ./agent/catalog_endpoint.go: args.ServiceName = strings.TrimPrefix(req.URL.Path, pathPrefix)
  • [ ] ./agent/catalog_endpoint.go: args.Node = strings.TrimPrefix(req.URL.Path, "/v1/catalog/node/")
  • [ ] ./agent/catalog_endpoint.go: args.Node = strings.TrimPrefix(req.URL.Path, "/v1/catalog/node-services/")
  • [ ] ./agent/catalog_endpoint.go: args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/catalog/gateway-services/")
  • [ ] ./agent/config_endpoint.go: kindAndName := strings.TrimPrefix(req.URL.Path, "/v1/config/")
  • [ ] ./agent/config_endpoint.go: kindAndName := strings.TrimPrefix(req.URL.Path, "/v1/config/")
  • [ ] ./agent/coordinate_endpoint.go: node := strings.TrimPrefix(req.URL.Path, "/v1/coordinate/node/")
  • [ ] ./agent/discovery_chain_endpoint.go: args.Name = strings.TrimPrefix(req.URL.Path, "/v1/discovery-chain/")
  • [ ] ./agent/event_endpoint.go: event.Name = strings.TrimPrefix(req.URL.Path, "/v1/event/fire/")
  • [ ] ./agent/federation_state_endpoint.go: datacenterName := strings.TrimPrefix(req.URL.Path, "/v1/internal/federation-state/")
  • [ ] ./agent/health_endpoint.go: args.State = strings.TrimPrefix(req.URL.Path, "/v1/health/state/")
  • [ ] ./agent/health_endpoint.go: args.Node = strings.TrimPrefix(req.URL.Path, "/v1/health/node/")
  • [ ] ./agent/health_endpoint.go: args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/health/checks/")
  • [ ] ./agent/health_endpoint.go: args.ServiceName = strings.TrimPrefix(req.URL.Path, prefix)
  • [ ] ./agent/intentions_endpoint.go: peerName := strings.TrimPrefix(ss[0], "peer:")
  • [ ] ./agent/intentions_endpoint.go: peerName := strings.TrimPrefix(ss[0], "peer:")
  • [ ] ./agent/intentions_endpoint.go: id := strings.TrimPrefix(req.URL.Path, "/v1/connect/intentions/")
  • [ ] ./agent/kvs_endpoint.go: args.Key = strings.TrimPrefix(req.URL.Path, "/v1/kv/")
  • [ ] ./agent/peering_endpoint.go: name := strings.TrimPrefix(req.URL.Path, "/v1/peering/")
  • [ ] ./agent/prepared_query_endpoint.go: id := strings.TrimPrefix(path, "/v1/query/")
  • [ ] ./agent/session_endpoint.go: args.Session.ID = strings.TrimPrefix(req.URL.Path, "/v1/session/destroy/")
  • [ ] ./agent/session_endpoint.go: args.SessionID = strings.TrimPrefix(req.URL.Path, "/v1/session/renew/")
  • [ ] ./agent/session_endpoint.go: args.SessionID = strings.TrimPrefix(req.URL.Path, "/v1/session/info/")
  • [ ] ./agent/session_endpoint.go: args.Node = strings.TrimPrefix(req.URL.Path, "/v1/session/node/")
  • [ ] ./agent/ui_endpoint.go: args.Node = strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/node/")
  • [ ] ./agent/ui_endpoint.go: args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/gateway-services-nodes/")
  • [ ] ./agent/ui_endpoint.go: args.ServiceName = strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/service-topology/")
  • [ ] ./agent/ui_endpoint.go: name := strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/gateway-intentions/")
  • [ ] ./agent/ui_endpoint.go: subPath := strings.TrimPrefix(req.URL.Path, "/v1/internal/ui/metrics-proxy")

huikang avatar Aug 25 '22 01:08 huikang

@jkirschner-hashicorp , @mkeeler , I think we do need to handle some of the endpoints as there is no restriction on the url path to these endpoint. For example

curl -X GET --header 'X-Consul-Token: <my bootstrap token>'    http://localhost:8500/v1/acl/policy/http://myservice.com

<a href="/v1/acl/policy/http:/myservice.com">Moved Permanently</a>.

The expected output should be

curl -X GET --header 'X-Consul-Token: <my bootstrap token>'    http://localhost:8500/v1/acl/policy/http://myservice.com
failed acl policy lookup: index error: UUID must be 36 characters%

I will update the PR to address all those endpoints.

huikang avatar Aug 25 '22 02:08 huikang