[OAUTH-WG] Fwd: Gen-ART Telechat review of draft-ietf-oauth-v2-25

[Stephen Farrell <[e-mail]>]

FYI in case some aren't on [e-mail]

Having responses to these before Thursday would be good. Its
often the case that some AD will turn some of these points into
a DISCUSS so good to know what can be cleared up easily and
what might need further discussion before the telechat.

S

-------- Original Message --------
Subject: Gen-ART Telechat review of draft-ietf-oauth-v2-25
Date: Tue, 10 Apr 2012 09:11:19 -0400
From: Richard L. Barnes <[e-mail]>
To: General Area Review Team <[e-mail]>, 
[e-mail], IETF-Discussion list <[e-mail]>

I am the assigned Gen-ART reviewer for this draft. For background on 
Gen-ART, please see the FAQ at 
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-oauth-v2.txt
Reviewer:  Richard Barnes
Review Date:  10 Apr 2012
IETF LC End Date:
IESG Telechat Date: 12 Apr 2012

Summary: Almost ready, but with some gaps that need to be addressed


MAJOR:

General: At several points, the document seems to deal in plain-text 
usernames and passwords, ranging from the requirement for HTTP Basic 
authentication (2.3.1) to the explicit username and password fields one 
of the grant types (4.3.2, 10.7).  In these cases, it seems like it 
would encourage better security practices to use some sort of digest or 
other secure scheme for proving ownership of passwords.  Otherwise, 
there's a risk that proxies, caches, etc. will have access to users' 
plaintext credentials.

General: The document requires the Authorization Server to distinguish 
between confidential and public clients at several points.  It's not 
clear, however, how the server is supposed to tell which is which. 
Perhaps Section 2.1 could elaborate a little more on this?

4: Throughout the document, the assumption is that the client is moved 
from one URI to another via HTTP redirects (302 responses).  Could the 
protocol also accommodate other techniques of moving clients around, 
e.g., in JavaScript (setting window.location).  It seems like this could 
make deployment much easier in some cases.

10.3: This section seems to ignore the risk of client impersonation at 
the resource server.  Just as with client credentials in Section 10.2, 
if an access token is compromised, then it could be presented by another 
party in order to access the protected resource.  So the Resource Server 
needs to authenticate the client and validate that the token goes with 
the client, in addition to just checking the validity of the token.

10: Throughout this section, there are mentions of which parameters 
require secure transmission/storage and which do not.  It would be 
helpful to summarize these requirements, say in a table at the end of 
the section.


MINOR:

2.3.1: When you say "request body" in this section, do you actually mean 
"query"?  I don't see a prohibition on GET here, in which case the 
parameters would be in the URI query section.

3.1.2.2: It seems like an explicit requirement would be helpful here, e.g.:
"
The Authorization Server MUST verify that either no redirect_uri is 
provided (in which case it uses the registered URI), or else that the 
provided redirect_uri matches the registered redirection URI for the 
authenticated client_id (where the match can be based on a full or 
partial registered URI).
"

4.1.3: Why does this request use redirect_uri and not client_id to 
identify the client?  (Or both?)  It seems like involving the client_id 
would better support the goal of client authentication, in order to 
mitigate the risk of client impersonation.

4.3.2: Shouldn't there be a client authentication here? (In particular, 
a client_id.)  Otherwise, it seems like this is essentially the same as 
the  flow in 4.4.

7. It seems like it would be helpful to have a way to pass access tokens 
in the request URI / query in some deployment cases (e.g., a JavaScript 
based client).  Is there any particular reason this was excluded?


EDITORIAL:

3. Would be helpful to note here that these endpoints are in addition to 
the resource endpoint (the one started out to protect), which is handled 
in Section 7.

3.2: It's not clear why only POST is allowed here.  A couple of words of 
explanation would help.

5.1: Why bother with a  case-insensitive token_type here?  It doesn't 
look like anything else in the whole document has this property.