Merge pull request #67164 from dekkagaijin/automated-cherry-pick-of-#…

…65799-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #65799: Escape illegal characters in remote extra keys Cherry pick of #65799 on release-1.11. #65799: Escape illegal characters in remote extra keys ```release-note action required: the API server and client-go libraries have been fixed to support additional non-alpha-numeric characters in UserInfo "extra" data keys. Both should be updated in order to properly support extra data containing "/" characters or other characters disallowed in HTTP headers. ```
kubernetes · Aug 21, 2018 · f53fc73 · f53fc73
2 parents 5cb744b + 1db5687
commit f53fc73
Show file tree

Hide file tree

Showing 6 changed files with 309 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,
+}