oauth2: cannot fetch token: 401 Unauthorized Response: {"code":null,"message":"Bad credentials"}

[slowblow]

13449ad

The commit 'internal: urlencode client id and secret in header' with ID: 13449ad
throws an error 'oauth2: cannot fetch token: 401 Unauthorized' when the clientID or the clienteSecret contains a special characters.

See line 191 of the internal/token.go
req.SetBasicAuth(clientID, clientSecret) is changed into req.SetBasicAuth(url.QueryEscape(clientID), url.QueryEscape(clientSecret))

Our client consists of implementation of get oauth2 token:

func GetOAuth2Token() (*oauth2.Token, error) {
        ctx := xnetcontext.Background()
	conf := &clientcredentials.Config{
		ClientID:     GetOAuth2ClientID(),
		ClientSecret: GetOAuth2ClientSecret(),
		Scopes:       GetOAuth2Scopes(),
		TokenURL:     GetOAuth2AccessTokenURL(),
	}

	token, err := conf.Token(ctx)

	if err != nil {
		fmt.Println("ERROR: ", err)
	} else {
		//fmt.Println("ACCESS TOKEN: ", token.AccessToken)
	}

	return token, err
}

For example, we are developing a client oauth2 with client_credentials as grant_type and clientServer was something like 'word1/word2'. So clientSecret != url.QueryEscape(clientSecret)
'word1/word2' != word1%2Fword2 get a response {"code":null,"message":"Bad credentials"}

[ty2]

The client id and secret should be url encoded in the basic auth header.

You may decode the clientSecret and clientID in the server side to solved the problem.

2.3.1.  Client Password

   Clients in possession of a client password MAY use the HTTP Basic
   authentication scheme as defined in [RFC2617] to authenticate with
   the authorization server.  The client identifier is encoded using the
   "application/x-www-form-urlencoded" encoding algorithm per
   Appendix B, and the encoded value is used as the username; the client
   password is encoded using the same algorithm and used as the
   password.  The authorization server MUST support the HTTP Basic
   authentication scheme for authenticating clients that were issued a
   client password.

   For example (with extra line breaks for display purposes only):

     Authorization: Basic czZCaGRSa3F0Mzo3RmpmcDBaQnIxS3REUmJuZlZkbUl3

   Alternatively, the authorization server MAY support including the
   client credentials in the request-body using the following
   parameters:

   client_id
         REQUIRED.  The client identifier issued to the client during
         the registration process described by Section 2.2.

   client_secret
         REQUIRED.  The client secret.  The client MAY omit the
         parameter if the client secret is an empty string.

https://tools.ietf.org/html/rfc6749#section-2.3.1

[heikoettelbruecksap]

This also affects the combination with Cloud Foundry UAA and apparently quite some other OAuth servers, see cloudfoundry/uaa#778. The reason is that certain RFCs require encoding of the basic authentication header content, while others don't.
Proposed solution, without the risk to break compatibility with currently supported OAuth servers: OAuth also allows to transfer the client ID and secret as POST parameters, and this is clearly specified regarding encoding of special characters. => Please change passing client credentials from basic authentication header to POST parameters.
Details: https://tools.ietf.org/html/rfc6749#section-2.3: "Alternatively, the authorization server MAY support including the client credentials in the request-body using the following parameters (...)"

[aeijdenberg]

UAA is most certainly doing the wrong thing by having their server not URL decode these, nor their various clients URL encode them. The RFC is unambiguous on this. I've pinged them multiple on multiples issues about this, however they seem quite content to not contemplate addressing the problem.

That said, I also believe it would be better for an OAuth library use the POST parameters by default for 3 reasons:

1. Workaround the UAA breakage.
2. We've observed Google break the Authorization header path at least once in production, and I'm told it's happened other times (see: https://twitter.com/AdamEijdenberg/status/1045197676759769088).
3. Documentation and examples for other OAuth implementations (such as Google's) tend to give plenty of examples where the client ID / secret are specified via POST parameters, but none with Authorization headers, which suggests to me this is a less common (even though technically correct), and possibly less well tested approach.

[heikoettelbruecksap]

@aeijdenberg Thanks for this feedback :-)
Two minor remarks to complete the picture - not invalidating any of your statements:
Regarding RFCs: It's a bit more difficult since certain RFC are kind of contradicting (see cloudfoundry/uaa#778 (comment)): 1945 is about basic authentication in HTTP in general, and it doesn't mentio URL-encoding (https://tools.ietf.org/html/rfc1945#section-10.2). 6749 is the more specific one for OAuth and requires encoding. So I agree that OAuth servers should follow the more specific RFC and thus encode the credentials. Still it seems to be a pattern that quite some OAuth servers don't correctly implement this one because they use some standard (RFC 1945 compliant) implementation/library/... for basic authentication in HTTP.
Regarding authorization servers that would benefit from the workaround: It's not only UAA, but apparently quite some more.

[aeijdenberg]

@heikoettelbruecksap - I stand by the comment that the RFCs are unambiguous and do not contradict.

RFC6749#2.3.1 - Client Password states:

Clients in possession of a client password MAY use the HTTP Basic
authentication scheme as defined in [RFC2617] to authenticate with
the authorization server. The client identifier is encoded using the
"application/x-www-form-urlencoded" encoding algorithm per
Appendix B, and the encoded value is used as the username; the client
password is encoded using the same algorithm and used as the
password. The authorization server MUST support the HTTP Basic
authentication scheme for authenticating clients that were issued a
client password.

You are correct that neither RFC2617 - HTTP Authentication: Basic and Digest Access Authentication (which obsoletes RFC2069 - An Extension to HTTP : Digest Access Authentication) nor RFC1945 - Hypertext Transfer Protocol -- HTTP/1.0 contemplate URL encoding the username / password, however RFC6749#2.3.1 clearly states that the client identifier is to be encoded and that the encoded value be used as the username, and the same with the password. That is, it describes what is done to map the client ID / secret to the inputs needed by the basic auth encoding.

So neither is wrong or ambiguous - they each correctly describe different layers of protocols.

Of course, now that you have me reading RFCs, that same section also states:

Including the client credentials in the request-body using the two
parameters is NOT RECOMMENDED and SHOULD be limited to clients unable
to directly utilize the HTTP Basic authentication scheme (or other
password-based HTTP authentication schemes).

which is a pretty decent argument to be made this library is already doing the right thing (at least per RFCs) and should not be changed.

[heikoettelbruecksap]

Your last finding is actually interesting and agree it gives some direction of how the ideal solution should behave (i.e. not pass the client credentials via request body).
Now it comes to reality, where apparently we have a significant set of OAuth servers (including, but not limited to UAA) which don't behave according to the OAuth-specific (and clear) definition for transfer of client credentials. Since it's usually an incompatible change for them to change their behavior (=> existing client credentials with special characters would suddenly be rejected), for every single one it's quite hard to become compliant to RFC 6749.
Considering this, I feel it makes sense to apply a mixed strategy:

• In general, keep the implementation as it is, as default behavior.
• To deal with incompatibilites in a pragmatic way, support an optional flag that a component using the library could provide (together with other connection data like URL and client credentials) to request transfer via request body.
  Regarding the "compatibility option": Clients would still need to figure out when to set this flag, so if they don't know, the default will still apply (giving "new" OAuth servers a hint to follow the recommendation). When a client needs to support "non-compliant" servers, it can expose this flag to e.g. the user (or maintain a list of problematic servers, ...).
  Example: In case of kubelogin, which anyhow looks into the kubeconfig YAML, a nice way would be to set this flag in the kubeconfig, together with the other required information. Then this would be a one-time setting, which even could be kept away from the actual user if the basic kubeconfig is already provided from somebody who knows the OAuth server.
  => In short: Would you agree that such a "compatibility option" would make sense?

[aeijdenberg]

@heikoettelbruecksap - I do agree a compatibility options makes sense, and it looks like others do too, as it exists already here: https://godoc.org/golang.org/x/oauth2#RegisterBrokenAuthHeaderProvider