From b5ec7f543a7266c6e6287f8ee6573f3c6bdba601 Mon Sep 17 00:00:00 2001 From: Yicheng Qin Date: Fri, 14 Aug 2015 16:14:23 -0700 Subject: [PATCH 1/3] client: use canonical url path in request The main change is that it keeps the trailing slash. This helps auth feature to judge path permission accurately. --- client/keys.go | 13 +++++++++++-- client/keys_test.go | 20 +++++++++++++++++--- pkg/pathutil/path.go | 38 ++++++++++++++++++++++++++++++++++++++ pkg/pathutil/path_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 pkg/pathutil/path.go create mode 100644 pkg/pathutil/path_test.go diff --git a/client/keys.go b/client/keys.go index 411cf44f7..064a78356 100644 --- a/client/keys.go +++ b/client/keys.go @@ -20,12 +20,12 @@ import ( "fmt" "net/http" "net/url" - "path" "strconv" "strings" "time" "github.com/coreos/etcd/Godeps/_workspace/src/golang.org/x/net/context" + "github.com/coreos/etcd/pkg/pathutil" ) const ( @@ -445,7 +445,16 @@ func (hw *httpWatcher) Next(ctx context.Context) (*Response, error) { // provided endpoint's path to the root of the keys API // (typically "/v2/keys"). func v2KeysURL(ep url.URL, prefix, key string) *url.URL { - ep.Path = path.Join(ep.Path, prefix, key) + // We concatenate all parts together manually. We cannot use + // path.Join because it does not reserve trailing slash. + // We call CanonicalURLPath to further cleanup the path. + if prefix != "" && prefix[0] != '/' { + prefix = "/" + prefix + } + if key != "" && key[0] != '/' { + key = "/" + key + } + ep.Path = pathutil.CanonicalURLPath(ep.Path + prefix + key) return &ep } diff --git a/client/keys_test.go b/client/keys_test.go index 1d00e6ba4..0dc05211c 100644 --- a/client/keys_test.go +++ b/client/keys_test.go @@ -80,6 +80,20 @@ func TestV2KeysURLHelper(t *testing.T) { key: "/baz", want: url.URL{Scheme: "https", Host: "example.com", Path: "/foo/bar/baz"}, }, + // Prefix is joined to path + { + endpoint: url.URL{Scheme: "https", Host: "example.com", Path: "/foo"}, + prefix: "/bar", + key: "", + want: url.URL{Scheme: "https", Host: "example.com", Path: "/foo/bar"}, + }, + // Keep trailing slash + { + endpoint: url.URL{Scheme: "https", Host: "example.com", Path: "/foo"}, + prefix: "/bar", + key: "/baz/", + want: url.URL{Scheme: "https", Host: "example.com", Path: "/foo/bar/baz/"}, + }, } for i, tt := range tests { @@ -269,7 +283,7 @@ func TestSetAction(t *testing.T) { act: setAction{ Key: "/foo/", }, - wantURL: "http://example.com/foo", + wantURL: "http://example.com/foo/", wantBody: "value=", }, @@ -460,7 +474,7 @@ func TestCreateInOrderAction(t *testing.T) { act: createInOrderAction{ Dir: "/foo/", }, - wantURL: "http://example.com/foo", + wantURL: "http://example.com/foo/", wantBody: "value=", }, @@ -555,7 +569,7 @@ func TestDeleteAction(t *testing.T) { act: deleteAction{ Key: "/foo/", }, - wantURL: "http://example.com/foo", + wantURL: "http://example.com/foo/", }, // Recursive set to true diff --git a/pkg/pathutil/path.go b/pkg/pathutil/path.go new file mode 100644 index 000000000..35d753d1f --- /dev/null +++ b/pkg/pathutil/path.go @@ -0,0 +1,38 @@ +// Copyright 2015 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pathutil + +import "path" + +// CanonicalURLPath returns the canonical url path for p, which follows the rules: +// 1. the path always starts with "/" +// 2. replace multiple slashes with a single slash +// 3. replace each '.' '..' path name element with equivalent one +// 4. keep the trailing slash +func CanonicalURLPath(p string) string { + if p == "" { + return "/" + } + if p[0] != '/' { + p = "/" + p + } + np := path.Clean(p) + // path.Clean removes trailing slash except for root, + // put the trailing slash back if necessary. + if p[len(p)-1] == '/' && np != "/" { + np += "/" + } + return np +} diff --git a/pkg/pathutil/path_test.go b/pkg/pathutil/path_test.go new file mode 100644 index 000000000..6d3d803cf --- /dev/null +++ b/pkg/pathutil/path_test.go @@ -0,0 +1,38 @@ +// Copyright 2015 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pathutil + +import "testing" + +func TestCanonicalURLPath(t *testing.T) { + tests := []struct { + p string + wp string + }{ + {"/a", "/a"}, + {"", "/"}, + {"a", "/a"}, + {"//a", "/a"}, + {"/a/.", "/a"}, + {"/a/..", "/"}, + {"/a/", "/a/"}, + {"/a//", "/a/"}, + } + for i, tt := range tests { + if g := CanonicalURLPath(tt.p); g != tt.wp { + t.Errorf("#%d: canonical path = %s, want %s", i, g, tt.wp) + } + } +} From fab3feab66d39359b6131b15989ad2a3a7a6cfbd Mon Sep 17 00:00:00 2001 From: Yicheng Qin Date: Tue, 18 Aug 2015 08:13:17 -0700 Subject: [PATCH 2/3] etcdctl/role: reject non-canonical permission path Non-canonical permission path is useless because the path received by auth is always canonical, which is due to our ServeMux always redirects request to canonical path(). This helps users to detect path permission setting error early. Ref: http://godoc.org/net/http#ServeMux --- etcdctl/command/role_commands.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/etcdctl/command/role_commands.go b/etcdctl/command/role_commands.go index bbd90864e..96777886d 100644 --- a/etcdctl/command/role_commands.go +++ b/etcdctl/command/role_commands.go @@ -23,6 +23,7 @@ import ( "github.com/coreos/etcd/Godeps/_workspace/src/github.com/codegangsta/cli" "github.com/coreos/etcd/Godeps/_workspace/src/golang.org/x/net/context" "github.com/coreos/etcd/client" + "github.com/coreos/etcd/pkg/pathutil" ) func NewRoleCommands() cli.Command { @@ -152,6 +153,10 @@ func roleGrantRevoke(c *cli.Context, grant bool) { fmt.Fprintln(os.Stderr, "No path specified; please use `-path`") os.Exit(1) } + if pathutil.CanonicalURLPath(path) != path { + fmt.Fprintf(os.Stderr, "Not canonical path; please use `-path=%s`\n", pathutil.CanonicalURLPath(path)) + os.Exit(1) + } read := c.Bool("read") write := c.Bool("write") From 4778d780a8e00cd6f6fc2c92545d4a9d6254f012 Mon Sep 17 00:00:00 2001 From: Yicheng Qin Date: Tue, 18 Aug 2015 11:48:22 -0700 Subject: [PATCH 3/3] pkg/pathutil: change copyright for path.go The file only contains the function that is borrowed from std http lib, so we use their copyright. --- pkg/pathutil/path.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/pkg/pathutil/path.go b/pkg/pathutil/path.go index 35d753d1f..82fd1db39 100644 --- a/pkg/pathutil/path.go +++ b/pkg/pathutil/path.go @@ -1,16 +1,6 @@ -// Copyright 2015 CoreOS, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +// Copyright 2009 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. package pathutil @@ -21,6 +11,7 @@ import "path" // 2. replace multiple slashes with a single slash // 3. replace each '.' '..' path name element with equivalent one // 4. keep the trailing slash +// The function is borrowed from stdlib http.cleanPath in server.go. func CanonicalURLPath(p string) string { if p == "" { return "/"