Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: document that Request.SetBasicAuth doesn't do any additional escaping #31577

Closed
tsuna opened this issue Apr 19, 2019 · 7 comments

Comments

Projects
None yet
5 participants
@tsuna
Copy link
Contributor

commented Apr 19, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

Call req.SetBasicAuth(user, pass) with special characters in either the username or the password, e.g.:

	req, err := http.NewRequest("POST", "http://127.0.0.1/foo", nil)
	req.SetBasicAuth("foo", "bar+baz")
	if err != nil {
		panic(err)
	}
	fmt.Println(req.Header.Get("Authorization"))

What did you expect to see?

The above program should probably print Basic Zm9vOmJhciUyQmJheg==

What did you see instead?

The above program prints Basic Zm9vOmJhcitiYXo=

Discussion

So I spent a long long long time debugging some OpenID Connect authentication issue with kubectl and dex. Long story short, it turns out that the random client secret I had generated contained a + sign and it needed to be URL encoded before being base64-encoded when used with basic auth.

After looking at the code in Kubernetes and various other projects, I realized that nobody ever escapes the arguments passed to SetBasicAuth(). So this issue is extremely widespread. In fact the bug was even present in Go's own OAuth2 implementation until golang/oauth2#237 fixed it.

The godoc doesn't say anything about escaping so it's not clear to me whether the intent was to have users do it themselves (and, clearly, they don't) or, more likely, this was just an unfortunate omission (i.e. a bug) when the library was implemented.

Given how widespread this issue is, I think the standard library should do the URL encoding automagically. Now since there are some callers that already do it (e.g. the OAuth2 library after the fix mentioned above), this means that the code should probably make a pass through the strings it's given and if they contain anything that needs escaping, then escape the whole string, otherwise use the string as-is.

If you guys disagree that the library should take care of escaping the arguments, then at the very least the godoc should be updated to explicitly state that the strings must be URL-safe or URL encoded. But I strongly think that the library should just make it work automatically given that virtually none of the existing code out there does this properly.

Granted this problem is probably not that likely to occur in practice, again I hit it because I used a random string of ASCII characters as a client secret, and there was a + in there. So kubectl worked for a while but once it needed to refresh its token, I'd get a mysterious 401 due to this bug.

@jwatte

This comment has been minimized.

Copy link

commented Apr 20, 2019

Strong passwords are a good thing; I would +2 this change to the library.
Because the library base64-encodes the payload, it should also urlencode it.
The only other behavior that I think would be consistent would be to require that the user provides the full header (i e, user does base64 encoding) but the ergonomics of that are pretty bad, so library-does-both seems like the best solution.

@julieqiu

This comment has been minimized.

Copy link

commented Apr 22, 2019

/cc @bradfitz

@julieqiu julieqiu added this to the Go1.13 milestone Apr 22, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

What says that URL escaping this is the responsibility of this layer? Or what says that URL escaping is standardized for this purpose at any other layer?

I see nothing in RFC 7617 about that.

@tsuna

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Hi @bradfitz, it's true that RFC 7617 (basic auth) says nothing about encoding or escaping, which the RFC even acknowledges:

The original definition of this authentication scheme failed to specify the character encoding scheme used to convert the user-pass into an octet sequence. In practice, most implementations chose either a locale-specific encoding such as ISO-8859-1 ([ISO-8859-1]), or UTF-8 ([RFC3629]). For backwards compatibility reasons, this specification continues to leave the default encoding undefined, as long as it is compatible with US-ASCII (mapping any US-ASCII character to a single octet matching the US-ASCII character code).

Maybe that's why the OAuth2 RFC, which was referenced by the aforementioned OAuth2 bug, explicitly calls for application/x-www-form-urlencoded.

I checked the nginx source code to gather a data point and they don't actually URL decode the string in the standard case (parsing of the header is done here and they later just do a plain string comparison here).

So unfortunately there doesn't seem to be a standard way of doing things here :(

I guess the godoc on the function should probably just mention that the argument may need to be escaped depending on the context? I will file a separate bug report with Kubernetes so they fix the call sites.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Yeah, this appears to be very OAuth2-specific. I can't find anything else that does that.

As one example, online generators like https://www.blitter.se/utils/basic-authentication-header-generator/ don't URL-escape the password.

New docs would be fine.

@bradfitz bradfitz changed the title net/http: SetBasicAuth() doesn't escape its arguments net/http: document that Request.SetBasicAuth doesn't do any additional escaping Apr 22, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Apr 22, 2019

Change https://golang.org/cl/173319 mentions this issue: net/http: Document that Basic Auth requires URL encoding with OAuth2.

@tsuna

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

⬆️
Just a small one-line documentation change to give folks a heads-up about this potential pitfall and, at the same time, make it clear and explicit that no URL encoding is performed.

@gopherbot gopherbot closed this in 415da71 Apr 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.