code review 97400044: oauth: add Authorization header to exchange token requests

oauth: add Authorization header to exchange token requests

According to http://tools.ietf.org/html/rfc6749 authorization servers MUST
support receiving client_id:client_secret encoded in the Authorization
header. However, receiving the id and secret urlencoded inside the body is
only something the server MAY support.

To increase compatibility, this commit change exchange token requests to
always include id and secret both encoded in the body and in the
Authorization header.

[cezarsa, 2014-05-13 17:30:02]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/goauth2/

[gobot, 2014-05-13 17:33:32]

Replacing golang-dev with golang-codereviews.

To the author of this CL: 

If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail'
instead.

If you did not name golang-dev explicitly and it was still added to the CL,
it means your working copy of the repo has a stale codereview.cfg
(or lib/codereview/codereview.cfg).
Please run 'hg sync' to update your client to the most recent codereview.cfg.
If the most recent codereview.cfg has defaultcc set to golang-dev instead of
golang-codereviews, please send a CL correcting it.

Thanks very much.

[cezarsa, 2014-05-14 17:34:03]

ping

[bradfitz, 2014-05-14 21:55:59]

LGTM but adg owns this, so deferring to him.

[adg, 2014-05-14 23:11:20]

LGTM

+cc jbd who is working on the rewrite. This may is relevant.

[adg, 2014-05-14 23:11:51]

*** Submitted as https://code.google.com/p/goauth2/source/detail?r=696c08849124
***

oauth: add Authorization header to exchange token requests

According to http://tools.ietf.org/html/rfc6749 authorization servers MUST
support receiving client_id:client_secret encoded in the Authorization
header. However, receiving the id and secret urlencoded inside the body is
only something the server MAY support.

To increase compatibility, this commit change exchange token requests to
always include id and secret both encoded in the body and in the
Authorization header.

LGTM=bradfitz, adg
R=golang-codereviews, adg
CC=bradfitz, golang-dev, jbd
https://codereview.appspot.com/97400044

Committer: Andrew Gerrand <adg@golang.org>

[jbd, 2014-05-15 09:46:00]

How does this comply with client that use a different client authentication
method? According to http://tools.ietf.org/html/rfc6749#section-2.3, a client
should be authenticated with a single method.

This is probably breaking Compute Engine's token retrieval flow where a token is
retrieved from a metadata server -- only accessible from GCE context, therefore
client is not configured with a client ID and secret.

[adg, 2014-05-15 09:54:04]

On 15 May 2014 19:46, <jbd@google.com> wrote:

> This is probably breaking Compute Engine's token retrieval flow where a
> token is retrieved from a metadata server -- only accessible from GCE
> context, therefore client is not configured with a client ID and secret.
>

Will it break it? Or will the metadata server just ignore the header?

[jbd, 2014-05-15 11:16:12]

It doesn't affect the package. On goauth2, compute builds it own request
[1], so do my new package. But it may break a regular token exchanging
request if auth is handled differently, according to the spec. We should
rather have two Transports here at
https://github.com/rakyll/oauth2/blob/master/oauth2.go#L59. The new one
will be used to handle token retrieval requests. You can provide whatever
RoundTripper implementation to authenticate these requests.

[1]
https://code.google.com/p/goauth2/source/browse/compute/serviceaccount/servic...


On Thu, May 15, 2014 at 11:53 AM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 15 May 2014 19:46, <jbd@google.com> wrote:
>
>> This is probably breaking Compute Engine's token retrieval flow where a
>> token is retrieved from a metadata server -- only accessible from GCE
>> context, therefore client is not configured with a client ID and secret.
>>
>
> Will it break it? Or will the metadata server just ignore the header?
>



-- 
Burcu Dogan

[cezarsa, 2014-05-15 14:01:05]

The reference to more than one client authentication method described
in http://tools.ietf.org/html/rfc6749#section-2.3 seem to be about
using something other (sec 2.3.2) than the client password (2.3.1) to
identify the client, in the same request.

This patch affects only the method described in 2.3.1 in which the
client can be sure that requests to /token to a compliant server will
understand the Authorization header. Servers probably won't complain
about unwanted args encoded in the body since they are already
something they might support.

If we want to avoid sending extra information to the server, what would
you guys think about adding a flag to the Config struct? This flag 
would control whether we send id and secret in the body or in the
header.

On 2014/05/15 11:16:12, jbd wrote:
> It doesn't affect the package. On goauth2, compute builds it own request
> [1], so do my new package. But it may break a regular token exchanging
> request if auth is handled differently, according to the spec. We should
> rather have two Transports here at
> https://github.com/rakyll/oauth2/blob/master/oauth2.go#L59. The new one
> will be used to handle token retrieval requests. You can provide whatever
> RoundTripper implementation to authenticate these requests.
> 
> [1]
>
https://code.google.com/p/goauth2/source/browse/compute/serviceaccount/servic...
> 
> 
> On Thu, May 15, 2014 at 11:53 AM, Andrew Gerrand <mailto:adg@golang.org>
wrote:
> 
> >
> > On 15 May 2014 19:46, <mailto:jbd@google.com> wrote:
> >
> >> This is probably breaking Compute Engine's token retrieval flow where a
> >> token is retrieved from a metadata server -- only accessible from GCE
> >> context, therefore client is not configured with a client ID and secret.
> >>
> >
> > Will it break it? Or will the metadata server just ignore the header?
> >
> 
> 
> 
> -- 
> Burcu Dogan