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

dekkagaijin · 2018-07-04T04:53:06Z

This percent-encodes characters in X-Remote-Extra- and Impersonate-Extra- keys which aren't valid for header names per RFC 7230 (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:

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.

idealhack · 2018-07-04T05:11:06Z

/ok-to-test
/sig auth

dekkagaijin · 2018-07-04T05:38:45Z

FYI @mikedanese @liggitt I'm %-encoding "%" characters in addition to the characters which are illegal under RFC 7230 so that we can also correctly propagate "%"-containing keys. This way, new client -> old server requests are garbled, and new client -> new server requests are not, rather than the other way around:

encoded %s

String Charset	Old Client, Old API server	Old Client, New API server	New Client, Old API server	New Client, New API server
legal	OK	potentially misinterpreted	potentially garbled	OK
illegal	request fails	request fails	garbled	OK

unencoded %s

String Charset	Old Client, Old API server	Old Client, New API server	New Client, Old API server	New Client, New API server
legal	OK	potentially misinterpreted	OK	potentially misinterpreted
illegal	request fails	request fails	garbled	potentially misinterpreted

mikedanese · 2018-07-09T17:57:41Z

staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go

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


why does PathUnescape work here?

PathEscape is under-aggressive for our needs, but PathUnescape blindly converts %-encoded bytes back to regular bytes: https://golang.org/src/net/url/url.go?s=5075:5118#L173

Can you show some examples where url.PathEscape/url.QueryEscape are lacking?

PathEscape is under-aggressive for our needs

that's unfortunate... what characters does it not escape for us?

Looks like it's "@", "=", and ":" are unreserved in path components but forbidden in header keys. I'll add explicit tests for that.
QueryEscape is lacking because we want to maximize compatibility with (i.e. minimize escaping of) existing legal header key strings. We don't want to be over-aggressivley escaping legal strings (which unnecessarily garbles legal keys keys being sent from a new client to an old API server).

I set up a playground to test the interactions between new/old clients/servers with various escaping/unescaping algorithms:
https://play.golang.org/p/eors6oEbRpT

Also, can we offload bulk of escaping to Path/QueryEscape and post-process the output to catch any extra characters we care about?

Does QueryEscape's aggressive escaping break anything?

The '+' -> ' ' seems problematic

The '+' -> ' ' seems problematic

That would be for Unescape, right? QueryEscape is the other way around, which seems fine?

Does QueryEscape's aggressive escaping break anything?
I'm very skeptical about having to maintain complex encoding logic for the sake of making headers less garbled/readable.

Yes, anyone currently using non-alphanumeric strings and mismatched client/server versions.

Also, can we offload bulk of escaping to Path/QueryEscape and post-process the output to catch any extra characters we care about?

Only if it's acceptable to permanently garble keys that are percent-encoded by the caller.
Double-encoding seems more complicated/fragile than simply whitelisting all header chars.

The '+' -> ' ' seems problematic

That would be for Unescape, right? QueryEscape is the other way around, which seems fine?

If it's acceptable to additionally break old clients sending '+'-containing keys to new API servers, sure.

mikedanese · 2018-07-10T18:42:44Z

staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go

+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.


The why of this comment is not obvious to me.

From the discussion in the original issue, it seemed like it was decided to always record these extra values (even when 'malformed'), so I made it a no-op on error and added test cases for strings like "foo%xxbar" which can't be PathUnescaped.

mikedanese · 2018-07-11T07:14:32Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+	return fmt.Sprintf("%%%x", b)
+}
+
+func percentEncodeRune(r rune) string {


A rune is always a byte so you can make this just be

return fmt.Sprintf("%%%x", byte(r))

runes are int32s, but if we are OK with restricting extra key strings to the ascii subset of utf-8, that'd be OK. I deliberately encoded entire runes here, rather than bytes, to avoid extra-garbled wire-format strings.

Ok, for some reason I thought go strings were UTF-8. Let's encode the whole int32.

Go strings are utf-8, but utf-8 runes can be 1-4 bytes long ;)

mikedanese · 2018-07-11T07:15:15Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+func headerEscape(key string) string {
+	encoded := ""
+	for _, r := range key {
+		encoded += percentEncodeIfIllegal(r)


Does this require a lot of allocations? Should we use a byte buffer?

potentially...
I deliberately chose string concatenation because bytes.Buffer.WriteFoo() returns an error, and I thought that it would be weird to do error handling when such a case does not arise from string concatenation (which I assume panics when malloc or something fails)

Scratch that, the docs are explicit in that Write(Byte|Rune|String) always returns a nil error. I'll use the buffer.

Use strings.Builder

liggitt · 2018-07-11T17:08:54Z

staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go

+			groupHeaders:       []string{"X-Remote-Group"},
+			extraPrefixHeaders: []string{"X-Remote-Extra-"},
+			requestHeaders: http.Header{
+				"X-Remote-User":                                            {"Bob"},


can you add a test specifically with the + character, to make sure that round trips correctly?

added one in the roundtripper test, but I can do so here, too

awly · 2018-07-12T00:07:27Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+		b := key[i]
+		if shouldEscape(b) {
+			buf.WriteByte('%')
+			buf.WriteByte("0123456789abcdef"[b>>4])


This needs an explanatory comment

awly · 2018-07-12T00:08:22Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+
+func headerKeyEscape(key string) string {
+	buf := strings.Builder{}
+	buf.Grow(headerKeyEscapedLen(key))


You can just write to buf without pre-scanning key to allocate memory

I realize that this is a micro-optimization, but it was done in response to feedback earlier about repetitive string concatenations being malloc-heavy.

awly · 2018-07-12T00:10:10Z

staging/src/k8s.io/client-go/transport/round_trippers.go

@@ -422,3 +422,130 @@ func (rt *debuggingRoundTripper) RoundTrip(req *http.Request) (*http.Response, e
 func (rt *debuggingRoundTripper) WrappedRoundTripper() http.RoundTripper {
 	return rt.delegatedRoundTripper
 }
+
+func isLegalHeaderKey(key string) bool {


this is no longer used

dekkagaijin · 2018-07-12T00:14:14Z

/retest

awly · 2018-07-12T00:32:24Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+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 == '%'


How about removing % from legalHeaderKeyBytes and remove this func?

That's what I originally had.
I feel that explicitly declaring '%' as header-key-legal, referencing the relevant RFC and appropriated code, and then explicitly encoding it anyway is more readable and less surprising.

awly · 2018-07-12T00:33:53Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+	for i := 0; i < len(key); i++ {
+		b := key[i]
+		if shouldEscape(b) {
+			// %-encode bytes that should be escaped.


Sorry, I meant explain how the magic string indexing and shifting below works.

awly · 2018-07-12T01:15:32Z

staging/src/k8s.io/client-go/transport/round_trippers.go

+		if shouldEscape(b) {
+			// %-encode bytes that should be escaped:
+			// https://tools.ietf.org/html/rfc3986#section-2.1
+			buf.WriteByte('%')


I'd prefer fmt.Fprintf(buf, "%%%x", b) here.

In order to ensure a proper encoding, it'd have to be fmt.Fprintf(buf, "%%%x%x", b>>4, b&15):
https://play.golang.org/p/gVydKNv7qq-

FWIW the current scheme is how things are done in the golang stdlib: https://golang.org/src/net/url/url.go?s=7512:7544#L304

Try this https://play.golang.org/p/JU_qx6yt-Pw

awly · 2018-07-12T01:16:47Z

LGTM but someone else should comment on how breaking this is.

dekkagaijin · 2018-07-16T18:05:56Z

@liggitt @mikedanese ping

mikedanese · 2018-07-16T18:24:04Z

Can you squash? This looks good to me.

/approve

Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin · 2018-07-16T18:45:54Z

squashed

dekkagaijin · 2018-07-16T23:34:11Z

/test pull-kubernetes-e2e-kops-aws

dekkagaijin · 2018-07-19T21:43:27Z

@liggitt ping
Would also be good to know of someone that could review the release note

dekkagaijin · 2018-07-26T02:32:09Z

@sttts @deads2k @lavalamp any approvers that could take a look?

liggitt · 2018-07-27T20:27:46Z

/lgtm

k8s-ci-robot · 2018-07-27T20:27:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dekkagaijin, liggitt, mikedanese

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-07-27T20:28:39Z

/test all

Tests are more than 96 hours old. Re-running tests.

liggitt · 2018-07-27T20:36:02Z

thanks for all the work on this. I updated the release note, please take a look. would you mind opening a doc PR against the impersonation and requestheader documentation describing the safe set of characters to use, and against the webhook authenticator doc referencing the safe set of characters to use up through v1.11.x?

dekkagaijin · 2018-07-27T21:38:30Z

@liggitt thanks, will do

k8s-github-robot · 2018-07-27T23:42:09Z

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 here.

liggitt · 2018-08-08T04:25:05Z

just noticed the doc PR referenced 1.11.2, but this hasn't been picked back to 1.11.x, has it?

dekkagaijin · 2018-08-08T20:22:49Z

@liggitt whoops... had a few changes in flight and only cherry picked one of them. I'll revise and send you the PR

…65799-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #65799: Escape illegal characters in remote extra keys Cherry pick of #65799 on release-1.10. #65799: Escape illegal characters in remote extra keys

…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. ```

…65799-upstream-release-1.9 Automated cherry pick of #65799: Escape illegal characters in remote extra keys

k8s-ci-robot requested review from liggitt and sttts July 4, 2018 04:53

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 4, 2018

mikedanese self-assigned this Jul 9, 2018

mikedanese reviewed Jul 9, 2018

View reviewed changes

mikedanese reviewed Jul 11, 2018

View reviewed changes

mikedanese assigned awly Jul 11, 2018

liggitt reviewed Jul 11, 2018

View reviewed changes

dekkagaijin force-pushed the fix-headers branch from c32a68b to f3a2579 Compare July 11, 2018 22:35

awly reviewed Jul 12, 2018

View reviewed changes

dekkagaijin force-pushed the fix-headers branch from ade1c69 to 44cf243 Compare July 12, 2018 17:24

Escape illegal characters in remote extra keys

f35e3d0

Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin force-pushed the fix-headers branch from 44cf243 to f35e3d0 Compare July 16, 2018 18:45

mikedanese assigned lavalamp, sttts and deads2k Jul 26, 2018

k8s-ci-robot assigned liggitt Jul 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2018

liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Jul 27, 2018

k8s-github-robot merged commit 6715f13 into kubernetes:master Jul 27, 2018

dekkagaijin deleted the fix-headers branch August 1, 2018 04:30

dekkagaijin mentioned this pull request Aug 1, 2018

Document the wire format for X-Remote-Extra- and Impersonate-Extra- keys kubernetes/website#9698

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 10, 2018

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

da2670a

…65799-upstream-release-1.9 Automated cherry pick of #65799: Escape illegal characters in remote extra keys

dekkagaijin mentioned this pull request Apr 23, 2019

REQUEST: New membership for @dekkagaijin kubernetes/org#760

Closed

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

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

Conversation

dekkagaijin commented Jul 4, 2018 • edited by liggitt

idealhack commented Jul 4, 2018

dekkagaijin commented Jul 4, 2018 • edited

encoded %s

unencoded %s

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Jul 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Jul 12, 2018

dekkagaijin commented Jul 16, 2018

mikedanese commented Jul 16, 2018

dekkagaijin commented Jul 16, 2018

dekkagaijin commented Jul 16, 2018

dekkagaijin commented Jul 19, 2018

dekkagaijin commented Jul 26, 2018

liggitt commented Jul 27, 2018

k8s-ci-robot commented Jul 27, 2018

k8s-github-robot commented Jul 27, 2018

liggitt commented Jul 27, 2018

dekkagaijin commented Jul 27, 2018

k8s-github-robot commented Jul 27, 2018

liggitt commented Aug 8, 2018

dekkagaijin commented Aug 8, 2018

dekkagaijin commented Jul 4, 2018 •

edited by liggitt

dekkagaijin commented Jul 4, 2018 •

edited

awly Jul 11, 2018 •

edited

dekkagaijin Jul 11, 2018 •

edited

dekkagaijin Jul 11, 2018 •

edited

dekkagaijin Jul 11, 2018 •

edited

dekkagaijin Jul 12, 2018 •

edited