From 634c72d22f656cb2b3dc9e7d346eb3f45f12516f Mon Sep 17 00:00:00 2001 From: littlestar642 Date: Sat, 16 Oct 2021 00:35:58 +0530 Subject: [PATCH] add path escape and unescape to path params --- .changelog/11335.txt | 3 ++ agent/agent_endpoint.go | 83 +++++++++++++++++++++++++++++++++-------- agent/http.go | 12 ++++++ agent/http_test.go | 39 +++++++++++++++++++ api/agent.go | 24 ++++++------ 5 files changed, 134 insertions(+), 27 deletions(-) create mode 100644 .changelog/11335.txt diff --git a/.changelog/11335.txt b/.changelog/11335.txt new file mode 100644 index 0000000000..26253e038b --- /dev/null +++ b/.changelog/11335.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +api: URL-encode/decode resource names for v1/agent endpoints in API +``` \ No newline at end of file diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 463ba87cec..d4becc95e1 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -380,7 +380,10 @@ func (s *HTTPHandlers) AgentServices(resp http.ResponseWriter, req *http.Request // blocking watch using hash-based blocking. func (s *HTTPHandlers) AgentService(resp http.ResponseWriter, req *http.Request) (interface{}, error) { // Get the proxy ID. Note that this is the ID of a proxy's service instance. - id := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/") + id, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/service/") + if err != nil { + return nil, err + } // Maybe block var queryOpts structs.QueryOptions @@ -400,7 +403,7 @@ func (s *HTTPHandlers) AgentService(resp http.ResponseWriter, req *http.Request) } // need to resolve to default the meta - _, err := s.agent.delegate.ResolveTokenAndDefaultMeta(token, &entMeta, nil) + _, err = s.agent.delegate.ResolveTokenAndDefaultMeta(token, &entMeta, nil) if err != nil { return nil, err } @@ -641,7 +644,10 @@ func (s *HTTPHandlers) AgentJoin(resp http.ResponseWriter, req *http.Request) (i } // Get the address - addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/join/") + addr, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/join/") + if err != nil { + return nil, err + } if wan { if s.agent.config.ConnectMeshGatewayWANFederationEnabled { @@ -701,7 +707,10 @@ func (s *HTTPHandlers) AgentForceLeave(resp http.ResponseWriter, req *http.Reque // Check if the WAN is being queried _, wan := req.URL.Query()["wan"] - addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/force-leave/") + addr, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/force-leave/") + if err != nil { + return nil, err + } if wan { return nil, s.agent.ForceLeaveWAN(addr, prune, entMeta) } else { @@ -788,7 +797,11 @@ func (s *HTTPHandlers) AgentRegisterCheck(resp http.ResponseWriter, req *http.Re } func (s *HTTPHandlers) AgentDeregisterCheck(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - checkID := structs.NewCheckID(types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/deregister/")), nil) + ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/deregister/") + if err != nil { + return nil, err + } + checkID := structs.NewCheckID(types.CheckID(ID), nil) // Get the provided token, if any, and vet against any ACL policies. var token string @@ -822,13 +835,21 @@ func (s *HTTPHandlers) AgentDeregisterCheck(resp http.ResponseWriter, req *http. } func (s *HTTPHandlers) AgentCheckPass(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/pass/")) + ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/pass/") + if err != nil { + return nil, err + } + checkID := types.CheckID(ID) note := req.URL.Query().Get("note") return s.agentCheckUpdate(resp, req, checkID, api.HealthPassing, note) } func (s *HTTPHandlers) AgentCheckWarn(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/warn/")) + ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/warn/") + if err != nil { + return nil, err + } + checkID := types.CheckID(ID) note := req.URL.Query().Get("note") return s.agentCheckUpdate(resp, req, checkID, api.HealthWarning, note) @@ -836,7 +857,11 @@ func (s *HTTPHandlers) AgentCheckWarn(resp http.ResponseWriter, req *http.Reques } func (s *HTTPHandlers) AgentCheckFail(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/fail/")) + ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/fail/") + if err != nil { + return nil, err + } + checkID := types.CheckID(ID) note := req.URL.Query().Get("note") return s.agentCheckUpdate(resp, req, checkID, api.HealthCritical, note) @@ -875,7 +900,12 @@ func (s *HTTPHandlers) AgentCheckUpdate(resp http.ResponseWriter, req *http.Requ return nil, nil } - checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/update/")) + ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/update/") + if err != nil { + return nil, err + } + + checkID := types.CheckID(ID) return s.agentCheckUpdate(resp, req, checkID, update.Status, update.Output) } @@ -958,7 +988,10 @@ func returnTextPlain(req *http.Request) bool { // AgentHealthServiceByID return the local Service Health given its ID func (s *HTTPHandlers) AgentHealthServiceByID(resp http.ResponseWriter, req *http.Request) (interface{}, error) { // Pull out the service id (service id since there may be several instance of the same service on this host) - serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/id/") + serviceID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/health/service/id/") + if err != nil { + return nil, err + } if serviceID == "" { return nil, &BadRequestError{Reason: "Missing serviceID"} } @@ -1017,7 +1050,11 @@ func (s *HTTPHandlers) AgentHealthServiceByID(resp http.ResponseWriter, req *htt // AgentHealthServiceByName return the worse status of all the services with given name on an agent func (s *HTTPHandlers) AgentHealthServiceByName(resp http.ResponseWriter, req *http.Request) (interface{}, error) { // Pull out the service name - serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/name/") + serviceName, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/health/service/name/") + if err != nil { + return nil, err + } + if serviceName == "" { return nil, &BadRequestError{Reason: "Missing service Name"} } @@ -1247,7 +1284,12 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. } func (s *HTTPHandlers) AgentDeregisterService(resp http.ResponseWriter, req *http.Request) (interface{}, error) { - sid := structs.NewServiceID(strings.TrimPrefix(req.URL.Path, "/v1/agent/service/deregister/"), nil) + serviceID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/service/deregister/") + if err != nil { + return nil, err + } + + sid := structs.NewServiceID(serviceID, nil) // Get the provided token, if any, and vet against any ACL policies. var token string @@ -1283,7 +1325,12 @@ func (s *HTTPHandlers) AgentDeregisterService(resp http.ResponseWriter, req *htt func (s *HTTPHandlers) AgentServiceMaintenance(resp http.ResponseWriter, req *http.Request) (interface{}, error) { // Ensure we have a service ID - sid := structs.NewServiceID(strings.TrimPrefix(req.URL.Path, "/v1/agent/service/maintenance/"), nil) + serviceID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/service/maintenance/") + if err != nil { + return nil, err + } + + sid := structs.NewServiceID(serviceID, nil) if sid.ID == "" { resp.WriteHeader(http.StatusBadRequest) @@ -1496,7 +1543,10 @@ func (s *HTTPHandlers) AgentToken(resp http.ResponseWriter, req *http.Request) ( } // Figure out the target token. - target := strings.TrimPrefix(req.URL.Path, "/v1/agent/token/") + target, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/token/") + if err != nil { + return nil, err + } err = s.agent.tokens.WithPersistenceLock(func() error { triggerAntiEntropySync := false @@ -1569,7 +1619,10 @@ func (s *HTTPHandlers) AgentConnectCARoots(resp http.ResponseWriter, req *http.R func (s *HTTPHandlers) AgentConnectCALeafCert(resp http.ResponseWriter, req *http.Request) (interface{}, error) { // Get the service name. Note that this is the name of the service, // not the ID of the service instance. - serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/") + serviceName, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/connect/ca/leaf/") + if err != nil { + return nil, err + } args := cachetype.ConnectCALeafRequest{ Service: serviceName, // Need name not ID diff --git a/agent/http.go b/agent/http.go index a1d8461d0d..cf63c4e420 100644 --- a/agent/http.go +++ b/agent/http.go @@ -1120,3 +1120,15 @@ func (s *HTTPHandlers) parseFilter(req *http.Request, filter *string) { *filter = other } } + +func getPathSuffixUnescaped(path string, prefixToTrim string) (string, error) { + // The suffix may be URL-encoded, so attempt to decode + suffixRaw := strings.TrimPrefix(path, prefixToTrim) + suffixUnescaped, err := url.PathUnescape(suffixRaw) + + if err != nil { + return suffixRaw, fmt.Errorf("failure in unescaping path param %q: %v", suffixRaw, err) + } + + return suffixUnescaped, nil +} diff --git a/agent/http_test.go b/agent/http_test.go index 33e2e78670..b6ead02f07 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -1680,3 +1680,42 @@ func TestRPC_HTTPSMaxConnsPerClient(t *testing.T) { }) } } + +func TestGetPathSuffixUnescaped(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + pathInput string + pathPrefix string + suffixResult string + errString string + }{ + // No decoding required (resource name must be unaffected by the decode) + {"Normal Valid", "/foo/bar/resource-1", "/foo/bar/", "resource-1", ""}, + // This function is not responsible for enforcing a valid URL, just for decoding escaped values. + // If there's an invalid URL segment in the path, it will be returned as is. + {"Unencoded Invalid", "/foo/bar/resource 1", "/foo/bar/", "resource 1", ""}, + // Decode the encoded value properly + {"Encoded Valid", "/foo/bar/re%2Fsource%201", "/foo/bar/", "re/source 1", ""}, + // Fail to decode an invalidly encoded input + {"Encoded Invalid", "/foo/bar/re%Fsource%201", "/foo/bar/", "re%Fsource%201", "failure in unescaping path param"}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + + suffixResult, err := getPathSuffixUnescaped(tc.pathInput, tc.pathPrefix) + + require.Equal(t, suffixResult, tc.suffixResult) + + if tc.errString == "" { + require.NoError(t, err) + } else { + require.Error(t, err) + require.Contains(t, err.Error(), tc.errString) + } + }) + } +} diff --git a/api/agent.go b/api/agent.go index e3b5d362a0..04dd059d99 100644 --- a/api/agent.go +++ b/api/agent.go @@ -707,7 +707,7 @@ func (a *Agent) AgentHealthServiceByNameOpts(service string, q *QueryOptions) (s // agent-local state. That means there is no persistent raft index so we block // based on object hash instead. func (a *Agent) Service(serviceID string, q *QueryOptions) (*AgentService, *QueryMeta, error) { - r := a.c.newRequest("GET", "/v1/agent/service/"+serviceID) + r := a.c.newRequest("GET", "/v1/agent/service/"+url.PathEscape(serviceID)) r.setQueryOptions(q) rtt, resp, err := a.c.doRequest(r) if err != nil { @@ -812,7 +812,7 @@ func (a *Agent) serviceRegister(service *AgentServiceRegistration, opts ServiceR // ServiceDeregister is used to deregister a service with // the local agent func (a *Agent) ServiceDeregister(serviceID string) error { - r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID) + r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+url.PathEscape(serviceID)) _, resp, err := a.c.doRequest(r) if err != nil { return err @@ -827,7 +827,7 @@ func (a *Agent) ServiceDeregister(serviceID string) error { // ServiceDeregisterOpts is used to deregister a service with // the local agent with QueryOptions. func (a *Agent) ServiceDeregisterOpts(serviceID string, q *QueryOptions) error { - r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID) + r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+url.PathEscape(serviceID)) r.setQueryOptions(q) _, resp, err := a.c.doRequest(r) if err != nil { @@ -884,7 +884,7 @@ func (a *Agent) updateTTL(checkID, note, status string) error { default: return fmt.Errorf("Invalid status: %s", status) } - endpoint := fmt.Sprintf("/v1/agent/check/%s/%s", status, checkID) + endpoint := fmt.Sprintf("/v1/agent/check/%s/%s", url.PathEscape(status), url.PathEscape(checkID)) r := a.c.newRequest("PUT", endpoint) r.params.Set("note", note) _, resp, err := a.c.doRequest(r) @@ -932,7 +932,7 @@ func (a *Agent) UpdateTTLOpts(checkID, output, status string, q *QueryOptions) e return fmt.Errorf("Invalid status: %s", status) } - endpoint := fmt.Sprintf("/v1/agent/check/update/%s", checkID) + endpoint := fmt.Sprintf("/v1/agent/check/update/%s", url.PathEscape(checkID)) r := a.c.newRequest("PUT", endpoint) r.setQueryOptions(q) r.obj = &checkUpdate{ @@ -976,7 +976,7 @@ func (a *Agent) CheckDeregister(checkID string) error { // CheckDeregisterOpts is used to deregister a check with // the local agent using query options func (a *Agent) CheckDeregisterOpts(checkID string, q *QueryOptions) error { - r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+checkID) + r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+url.PathEscape(checkID)) r.setQueryOptions(q) _, resp, err := a.c.doRequest(r) if err != nil { @@ -992,7 +992,7 @@ func (a *Agent) CheckDeregisterOpts(checkID string, q *QueryOptions) error { // Join is used to instruct the agent to attempt a join to // another cluster member func (a *Agent) Join(addr string, wan bool) error { - r := a.c.newRequest("PUT", "/v1/agent/join/"+addr) + r := a.c.newRequest("PUT", "/v1/agent/join/"+url.PathEscape(addr)) if wan { r.params.Set("wan", "1") } @@ -1044,7 +1044,7 @@ func (a *Agent) ForceLeavePrune(node string) error { // ForceLeaveOpts is used to have the agent eject a failed node or remove it // completely from the list of members. func (a *Agent) ForceLeaveOpts(node string, opts ForceLeaveOpts) error { - r := a.c.newRequest("PUT", "/v1/agent/force-leave/"+node) + r := a.c.newRequest("PUT", "/v1/agent/force-leave/"+url.PathEscape(node)) if opts.Prune { r.params.Set("prune", "1") } @@ -1108,7 +1108,7 @@ func (a *Agent) ConnectCARoots(q *QueryOptions) (*CARootList, *QueryMeta, error) // ConnectCALeaf gets the leaf certificate for the given service ID. func (a *Agent) ConnectCALeaf(serviceID string, q *QueryOptions) (*LeafCert, *QueryMeta, error) { - r := a.c.newRequest("GET", "/v1/agent/connect/ca/leaf/"+serviceID) + r := a.c.newRequest("GET", "/v1/agent/connect/ca/leaf/"+url.PathEscape(serviceID)) r.setQueryOptions(q) rtt, resp, err := a.c.doRequest(r) if err != nil { @@ -1136,7 +1136,7 @@ func (a *Agent) EnableServiceMaintenance(serviceID, reason string) error { } func (a *Agent) EnableServiceMaintenanceOpts(serviceID, reason string, q *QueryOptions) error { - r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+serviceID) + r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+url.PathEscape(serviceID)) r.setQueryOptions(q) r.params.Set("enable", "true") r.params.Set("reason", reason) @@ -1158,7 +1158,7 @@ func (a *Agent) DisableServiceMaintenance(serviceID string) error { } func (a *Agent) DisableServiceMaintenanceOpts(serviceID string, q *QueryOptions) error { - r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+serviceID) + r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+url.PathEscape(serviceID)) r.setQueryOptions(q) r.params.Set("enable", "false") _, resp, err := a.c.doRequest(r) @@ -1355,7 +1355,7 @@ func (a *Agent) updateTokenFallback(token string, q *WriteOptions, targets ...st } func (a *Agent) updateTokenOnce(target, token string, q *WriteOptions) (*WriteMeta, int, error) { - r := a.c.newRequest("PUT", fmt.Sprintf("/v1/agent/token/%s", target)) + r := a.c.newRequest("PUT", fmt.Sprintf("/v1/agent/token/%s", url.PathEscape(target))) r.setWriteOptions(q) r.obj = &AgentToken{Token: token}