Merge pull request #65799 from dekkagaijin/fix-headers

Automatic merge from submit-queue (batch tested with PRs 66225, 66648, 65799, 66630, 66619). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Percent-encode illegal characters in user.Info.Extra keys

This percent-encodes characters in `X-Remote-Extra-` and `Impersonate-Extra-` keys which aren't valid for header names per [RFC 7230](https://tools.ietf.org/html/rfc7230#section-3.2.6) (plus "%" to avoid breaking keys which contain them). The API server then blindly unescapes these keys.

Reviewer note:
Old clients sending keys which were `%`-escaped by the user will have their values unescaped by new API servers. New clients sending keys containing illegal characters (or "%") to old API servers will not have their values unescaped. This version skew incompatibility is a compromise discussed in #63682.

Fixes #63682

PTAL @mikedanese 

**Release note**:
```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.
```
This commit is contained in:
Kubernetes Submit Queue 2018-07-27 16:42:11 -07:00 committed by GitHub
commit 6715f13929
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 309 additions and 8 deletions

View File

@ -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...)
}
}

View File

@ -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 {

View File

@ -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 {

View File

@ -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{

View File

@ -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,
}

View File

@ -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)
}
})
}
}