Subject: [OAUTH-WG] Comments on -18

From: Torsten Lodderstedt <[email-masked]>
Date: Wed, 20 Jul 2011 23:49:21 +0200

section 1.5

"(A)  The client requests an access token by authenticating with the
         authorization server, and presenting an authorization grant."

This statement does not reflect that client authentication is not always 
required.

Proposal:

"The client requests an access token by presenting an authorization 
grant. The authorization server may require the client to authenticate 
as a pre-requisite."

section 4.1

"(D)  The client requests an access token from the authorization
         server's token endpoint by including the authorization code
         received in the previous step.  When making the request, the
         client authenticates with the authorization server."

Same as above. Proposal:

".... When making the request, the
         client may be required to authenticate with the authorization 
server"

"(E)  The authorization server authenticates the client, validates the
         authorization code, and ensures the redirection URI received
         matches the URI used to redirect the client in step (C)."

Same as above.

Additionally, is the URI check also required if the client did not 
passed a redirect uri but relied on its pre-registered redirect_uri?

section 4.1.1

"state" parameter: Would it make sense to elaborate on the usage of this 
parameter and its importance for preventing CSRF attacks (incl. the 
nessessary entropy)? Alternatively, a reference to the security 
consideration section could be added.

section 4.1.3

"If the client type is private or was issued client credentials ..." 
Isn't the later the most important property of a "private" client? If 
so, "or was issued client credentials" is redundant.

section 4.4.3

"A refresh token SHOULD NOT be included." Why not "MUST NOT"?

section 5.2

"The authorization server MAY return an HTTP 401
                (Unauthorized) status code to indicate which HTTP
                authentication schemes are supported."

Given the usage of HTTP authentication schemes is the way to 
authenticated client recommended by the spec, status code 401 should be 
the default status code for this kind of error. Usage of status code 400 
should be the exception.

"unauthorized_client"

So above - status code 403 seems to be a more appropriate default.

section 10

"and furthermore that rotation of the client
       authentication credentials is impractical."

What does this mean?

Please update reference [I-D.lodderstedt-oauth-security] to 
[I-D.ietf-oauth-v2-threatmodel].

section 10.2

last sentence: "... ensure the repeated request comes from an authentic 
client and not an
    impersonator."

The authorization server must ensure that the request comes from the 
same client not just any authentic client. So I would propose to adjust 
the text as follows:

"... ensure the repeated request comes from the original client and not an
    impersonator."

section 10.3

"Access token (as well as any type-specific attributes) MUST ..." does 
"type-specific" refer to access token type specific? If so, please add 
this information.

section 10.6

Please replace the first sentence with the following text:

"Such an attack leverages the authorization code ..."

---

From: Eran Hammer-Lahav <[email-masked]>
Date: Mon, 25 Jul 2011 00:54:54 -0700

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf
> Of Torsten Lodderstedt
> Sent: Wednesday, July 20, 2011 2:49 PM
> To: OAuth WG
> Subject: [OAUTH-WG] Comments on -18
> 
> section 1.5
> 
> "The client requests an access token by presenting an authorization grant.
> The authorization server may require the client to authenticate as a pre-
> requisite."

The general design for fresh tokens does include authentication. Yes, the normative text allows you not to, but this is the general case and I like the simpler text for the introduction.

>          client may be required to authenticate with the authorization server"

Same. Section 4.1.3 makes this clear.
 
> Additionally, is the URI check also required if the client did not passed a
> redirect uri but relied on its pre-registered redirect_uri?

This is the overview. Section 4.1.3 provides the specific requirements.

> nessessary entropy)? Alternatively, a reference to the security consideration
> section could be added.

I think we got this covered. I rather not start pointing to the security section from the sections above.

> Isn't the later the most important property of a "private" client? If so, "or was
> issued client credentials" is redundant.

All private clients must authenticate, but also public clients with issued credentials.

> "A refresh token SHOULD NOT be included." Why not "MUST NOT"?

This was debated a while back and the consensus was that there is no reason to restrict this to a MUST NOT. Someone might even had plans to issue refresh tokens using this flow.

> Given the usage of HTTP authentication schemes is the way to authenticated
> client recommended by the spec, status code 401 should be the default
> status code for this kind of error. Usage of status code 400 should be the
> exception.
>
> "unauthorized_client"
> 

I think this is fine unchanged.

>        authentication credentials is impractical."

I'll take it out. I don't remember who suggested it (Brian?).

> Please update reference [I-D.lodderstedt-oauth-security] to [I-D.ietf-oauth-
> v2-threatmodel].

Done.

> The authorization server must ensure that the request comes from the same
> client not just any authentic client. So I would propose to adjust the text as
> follows:
> 

Ok.

> section 10.3
> 
> "Access token (as well as any type-specific attributes) MUST ..." does "type-
> specific" refer to access token type specific? If so, please add this
> information.

Ok.

> "Such an attack leverages the authorization code ..."

That reads funny. How about 'An attacker can leverage...'

EHL

---

From: Torsten Lodderstedt <[email-masked]>
Date: Mon, 25 Jul 2011 10:24:04 -0400

Hi Eran,
> I think this is fine unchanged.

Can you please give a rationale?

...

> That reads funny. How about 'An attacker can leverage...'

No one said we have to write boring text :-) Your proposal is fine.

regards,
Torsten.

---

From: Eran Hammer-Lahav <[email-masked]>
Date: Mon, 25 Jul 2011 08:53:04 -0700

> From: Torsten Lodderstedt [mailto:[email-masked]]
> Sent: Monday, July 25, 2011 7:24 AM
> To: Eran Hammer-Lahav
> Cc: OAuth WG
> Subject: Re: [OAUTH-WG] Comments on -18
> 
> Hi Eran,
> >> section 5.2
> >>
> >> authenticated client recommended by the spec, status code 401 should
> >> be the default status code for this kind of error. Usage of status
> >> code 400 should be the exception.
> >>
> >> "unauthorized_client"
> >>

The current text keeps things simple by using a single error code 400, but allowing/requiring the use of 401 when client authentication fails. Whether this is the ideal use of HTTP status codes is open for debate, but even the HTTP experts informed us that we can use 400 for cases that might be more accurately described by a 403.

So I rather not change this at this point.

EHL

---

From: Torsten Lodderstedt <[email-masked]>
Date: Mon, 25 Jul 2011 13:22:37 -0400

ok