From f35e3d07c9898f8ec156209a868fa4451eb9afe2 Mon Sep 17 00:00:00 2001 From: Jake Sanders Date: Tue, 3 Jul 2018 21:19:15 -0700 Subject: [PATCH] Escape illegal characters in remote extra keys Signed-off-by: Jake Sanders --- .../request/headerrequest/requestheader.go | 11 +- .../headerrequest/requestheader_test.go | 31 +++++ .../pkg/endpoints/filters/impersonation.go | 11 +- .../endpoints/filters/impersonation_test.go | 34 +++++ .../client-go/transport/round_trippers.go | 111 +++++++++++++++- .../transport/round_trippers_test.go | 119 +++++++++++++++++- 6 files changed, 309 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go b/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go index 38f132b583f..948478b80ed 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go @@ -21,6 +21,7 @@ import ( "fmt" "io/ioutil" "net/http" + "net/url" "strings" "k8s.io/apimachinery/pkg/util/sets" @@ -160,6 +161,14 @@ func allHeaderValues(h http.Header, headerNames []string) []string { return ret } +func unescapeExtraKey(encodedKey string) string { + key, err := url.PathUnescape(encodedKey) // Decode %-encoded bytes. + if err != nil { + return encodedKey // Always record extra strings, even if malformed/unencoded. + } + return key +} + func newExtra(h http.Header, headerPrefixes []string) map[string][]string { ret := map[string][]string{} @@ -170,7 +179,7 @@ func newExtra(h http.Header, headerPrefixes []string) map[string][]string { continue } - extraKey := strings.ToLower(headerName[len(prefix):]) + extraKey := unescapeExtraKey(strings.ToLower(headerName[len(prefix):])) ret[extraKey] = append(ret[extraKey], vv...) } } diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go index d81d4ee64a8..28876dbc43d 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go @@ -152,6 +152,37 @@ func TestRequestHeader(t *testing.T) { }, expectedOk: true, }, + + "escaped extra keys": { + nameHeaders: []string{"X-Remote-User"}, + groupHeaders: []string{"X-Remote-Group"}, + extraPrefixHeaders: []string{"X-Remote-Extra-"}, + requestHeaders: http.Header{ + "X-Remote-User": {"Bob"}, + "X-Remote-Group": {"one-a", "one-b"}, + "X-Remote-Extra-Alpha": {"alphabetical"}, + "X-Remote-Extra-Alph4num3r1c": {"alphanumeric"}, + "X-Remote-Extra-Percent%20encoded": {"percent encoded"}, + "X-Remote-Extra-Almost%zzpercent%xxencoded": {"not quite percent encoded"}, + "X-Remote-Extra-Example.com%2fpercent%2520encoded": {"url with double percent encoding"}, + "X-Remote-Extra-Example.com%2F%E4%BB%8A%E6%97%A5%E3%81%AF": {"url with unicode"}, + "X-Remote-Extra-Abc123!#$+.-_*\\^`~|'": {"header key legal characters"}, + }, + expectedUser: &user.DefaultInfo{ + Name: "Bob", + Groups: []string{"one-a", "one-b"}, + Extra: map[string][]string{ + "alpha": {"alphabetical"}, + "alph4num3r1c": {"alphanumeric"}, + "percent encoded": {"percent encoded"}, + "almost%zzpercent%xxencoded": {"not quite percent encoded"}, + "example.com/percent%20encoded": {"url with double percent encoding"}, + "example.com/今日は": {"url with unicode"}, + "abc123!#$+.-_*\\^`~|'": {"header key legal characters"}, + }, + }, + expectedOk: true, + }, } for k, testcase := range testcases { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go index 8e7e9eaee86..726cbe4d565 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" "github.com/golang/glog" @@ -146,6 +147,14 @@ func WithImpersonation(handler http.Handler, a authorizer.Authorizer, s runtime. }) } +func unescapeExtraKey(encodedKey string) string { + key, err := url.PathUnescape(encodedKey) // Decode %-encoded bytes. + if err != nil { + return encodedKey // Always record extra strings, even if malformed/unencoded. + } + return key +} + // buildImpersonationRequests returns a list of objectreferences that represent the different things we're requesting to impersonate. // Also includes a map[string][]string representing user.Info.Extra // Each request must be authorized against the current user before switching contexts. @@ -175,7 +184,7 @@ func buildImpersonationRequests(headers http.Header) ([]v1.ObjectReference, erro } hasUserExtra = true - extraKey := strings.ToLower(headerName[len(authenticationv1.ImpersonateUserExtraHeaderPrefix):]) + extraKey := unescapeExtraKey(strings.ToLower(headerName[len(authenticationv1.ImpersonateUserExtraHeaderPrefix):])) // make a separate request for each extra value they're trying to set for _, value := range values { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go index 3357e038db1..d309a21098d 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation_test.go @@ -70,6 +70,10 @@ func (impersonateAuthorizer) Authorize(a authorizer.Attributes) (authorized auth return authorizer.DecisionAllow, "", nil } + if len(user.GetGroups()) > 1 && (user.GetGroups()[1] == "escaped-scopes" || user.GetGroups()[1] == "almost-escaped-scopes") { + return authorizer.DecisionAllow, "", nil + } + if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-particular-scopes" && a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "scopes" && a.GetName() == "scope-a" { return authorizer.DecisionAllow, "", nil @@ -224,6 +228,36 @@ func TestImpersonationFilter(t *testing.T) { }, expectedCode: http.StatusOK, }, + { + name: "percent-escaped-userextras", + user: &user.DefaultInfo{ + Name: "dev", + Groups: []string{"wheel", "escaped-scopes"}, + }, + impersonationUser: "system:admin", + impersonationUserExtras: map[string][]string{"example.com%2fescaped%e1%9b%84scopes": {"scope-a", "scope-b"}}, + expectedUser: &user.DefaultInfo{ + Name: "system:admin", + Groups: []string{"system:authenticated"}, + Extra: map[string][]string{"example.com/escapedᛄscopes": {"scope-a", "scope-b"}}, + }, + expectedCode: http.StatusOK, + }, + { + name: "almost-percent-escaped-userextras", + user: &user.DefaultInfo{ + Name: "dev", + Groups: []string{"wheel", "almost-escaped-scopes"}, + }, + impersonationUser: "system:admin", + impersonationUserExtras: map[string][]string{"almost%zzpercent%xxencoded": {"scope-a", "scope-b"}}, + expectedUser: &user.DefaultInfo{ + Name: "system:admin", + Groups: []string{"system:authenticated"}, + Extra: map[string][]string{"almost%zzpercent%xxencoded": {"scope-a", "scope-b"}}, + }, + expectedCode: http.StatusOK, + }, { name: "allowed-users-impersonation", user: &user.DefaultInfo{ diff --git a/staging/src/k8s.io/client-go/transport/round_trippers.go b/staging/src/k8s.io/client-go/transport/round_trippers.go index 459a93760d6..316a5c0d01c 100644 --- a/staging/src/k8s.io/client-go/transport/round_trippers.go +++ b/staging/src/k8s.io/client-go/transport/round_trippers.go @@ -129,7 +129,7 @@ func SetAuthProxyHeaders(req *http.Request, username string, groups []string, ex } for key, values := range extra { for _, value := range values { - req.Header.Add("X-Remote-Extra-"+key, value) + req.Header.Add("X-Remote-Extra-"+headerKeyEscape(key), value) } } } @@ -246,7 +246,7 @@ func (rt *impersonatingRoundTripper) RoundTrip(req *http.Request) (*http.Respons } for k, vv := range rt.impersonate.Extra { for _, v := range vv { - req.Header.Add(ImpersonateUserExtraHeaderPrefix+k, v) + req.Header.Add(ImpersonateUserExtraHeaderPrefix+headerKeyEscape(k), v) } } @@ -422,3 +422,110 @@ func (rt *debuggingRoundTripper) RoundTrip(req *http.Request) (*http.Response, e func (rt *debuggingRoundTripper) WrappedRoundTripper() http.RoundTripper { return rt.delegatedRoundTripper } + +func legalHeaderByte(b byte) bool { + return int(b) < len(legalHeaderKeyBytes) && legalHeaderKeyBytes[b] +} + +func shouldEscape(b byte) bool { + // url.PathUnescape() returns an error if any '%' is not followed by two + // hexadecimal digits, so we'll intentionally encode it. + return !legalHeaderByte(b) || b == '%' +} + +func headerKeyEscape(key string) string { + buf := strings.Builder{} + for i := 0; i < len(key); i++ { + b := key[i] + if shouldEscape(b) { + // %-encode bytes that should be escaped: + // https://tools.ietf.org/html/rfc3986#section-2.1 + fmt.Fprintf(&buf, "%%%02X", b) + continue + } + buf.WriteByte(b) + } + return buf.String() +} + +// legalHeaderKeyBytes was copied from net/http/lex.go's isTokenTable. +// See https://httpwg.github.io/specs/rfc7230.html#rule.token.separators +var legalHeaderKeyBytes = [127]bool{ + '%': true, + '!': true, + '#': true, + '$': true, + '&': true, + '\'': true, + '*': true, + '+': true, + '-': true, + '.': true, + '0': true, + '1': true, + '2': true, + '3': true, + '4': true, + '5': true, + '6': true, + '7': true, + '8': true, + '9': true, + 'A': true, + 'B': true, + 'C': true, + 'D': true, + 'E': true, + 'F': true, + 'G': true, + 'H': true, + 'I': true, + 'J': true, + 'K': true, + 'L': true, + 'M': true, + 'N': true, + 'O': true, + 'P': true, + 'Q': true, + 'R': true, + 'S': true, + 'T': true, + 'U': true, + 'W': true, + 'V': true, + 'X': true, + 'Y': true, + 'Z': true, + '^': true, + '_': true, + '`': true, + 'a': true, + 'b': true, + 'c': true, + 'd': true, + 'e': true, + 'f': true, + 'g': true, + 'h': true, + 'i': true, + 'j': true, + 'k': true, + 'l': true, + 'm': true, + 'n': true, + 'o': true, + 'p': true, + 'q': true, + 'r': true, + 's': true, + 't': true, + 'u': true, + 'v': true, + 'w': true, + 'x': true, + 'y': true, + 'z': true, + '|': true, + '~': true, +} diff --git a/staging/src/k8s.io/client-go/transport/round_trippers_test.go b/staging/src/k8s.io/client-go/transport/round_trippers_test.go index d5ffc6bde30..74d3dc21203 100644 --- a/staging/src/k8s.io/client-go/transport/round_trippers_test.go +++ b/staging/src/k8s.io/client-go/transport/round_trippers_test.go @@ -18,6 +18,7 @@ package transport import ( "net/http" + "net/url" "reflect" "strings" "testing" @@ -125,6 +126,32 @@ func TestImpersonationRoundTripper(t *testing.T) { ImpersonateUserExtraHeaderPrefix + "Second": {"B", "b"}, }, }, + { + name: "escape handling", + impersonationConfig: ImpersonationConfig{ + UserName: "user", + Extra: map[string][]string{ + "test.example.com/thing.thing": {"A", "a"}, + }, + }, + expected: map[string][]string{ + ImpersonateUserHeader: {"user"}, + ImpersonateUserExtraHeaderPrefix + `Test.example.com%2fthing.thing`: {"A", "a"}, + }, + }, + { + name: "double escape handling", + impersonationConfig: ImpersonationConfig{ + UserName: "user", + Extra: map[string][]string{ + "test.example.com/thing.thing%20another.thing": {"A", "a"}, + }, + }, + expected: map[string][]string{ + ImpersonateUserHeader: {"user"}, + ImpersonateUserExtraHeaderPrefix + `Test.example.com%2fthing.thing%2520another.thing`: {"A", "a"}, + }, + }, } for _, tc := range tcs { @@ -159,9 +186,10 @@ func TestImpersonationRoundTripper(t *testing.T) { func TestAuthProxyRoundTripper(t *testing.T) { for n, tc := range map[string]struct { - username string - groups []string - extra map[string][]string + username string + groups []string + extra map[string][]string + expectedExtra map[string][]string }{ "allfields": { username: "user", @@ -170,6 +198,34 @@ func TestAuthProxyRoundTripper(t *testing.T) { "one": {"alpha", "bravo"}, "two": {"charlie", "delta"}, }, + expectedExtra: map[string][]string{ + "one": {"alpha", "bravo"}, + "two": {"charlie", "delta"}, + }, + }, + "escaped extra": { + username: "user", + groups: []string{"groupA", "groupB"}, + extra: map[string][]string{ + "one": {"alpha", "bravo"}, + "example.com/two": {"charlie", "delta"}, + }, + expectedExtra: map[string][]string{ + "one": {"alpha", "bravo"}, + "example.com%2ftwo": {"charlie", "delta"}, + }, + }, + "double escaped extra": { + username: "user", + groups: []string{"groupA", "groupB"}, + extra: map[string][]string{ + "one": {"alpha", "bravo"}, + "example.com/two%20three": {"charlie", "delta"}, + }, + expectedExtra: map[string][]string{ + "one": {"alpha", "bravo"}, + "example.com%2ftwo%2520three": {"charlie", "delta"}, + }, }, } { rt := &testRoundTripper{} @@ -210,9 +266,64 @@ func TestAuthProxyRoundTripper(t *testing.T) { actualExtra[extraKey] = append(actualExtra[key], values...) } } - if e, a := tc.extra, actualExtra; !reflect.DeepEqual(e, a) { + if e, a := tc.expectedExtra, actualExtra; !reflect.DeepEqual(e, a) { t.Errorf("%s expected %v, got %v", n, e, a) continue } } } + +// TestHeaderEscapeRoundTrip tests to see if foo == url.PathUnescape(headerEscape(foo)) +// This behavior is important for client -> API server transmission of extra values. +func TestHeaderEscapeRoundTrip(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + key string + }{ + { + name: "alpha", + key: "alphabetical", + }, + { + name: "alphanumeric", + key: "alph4num3r1c", + }, + { + name: "percent encoded", + key: "percent%20encoded", + }, + { + name: "almost percent encoded", + key: "almost%zzpercent%xxencoded", + }, + { + name: "illegal char & percent encoding", + key: "example.com/percent%20encoded", + }, + { + name: "weird unicode stuff", + key: "example.com/ᛒᚥᛏᛖᚥᚢとロビン", + }, + { + name: "header legal chars", + key: "abc123!#$+.-_*\\^`~|'", + }, + { + name: "legal path, illegal header", + key: "@=:", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + escaped := headerKeyEscape(tc.key) + unescaped, err := url.PathUnescape(escaped) + if err != nil { + t.Fatalf("url.PathUnescape(%q) returned error: %v", escaped, err) + } + if tc.key != unescaped { + t.Errorf("url.PathUnescape(headerKeyEscape(%q)) returned %q, wanted %q", tc.key, unescaped, tc.key) + } + }) + } +}