Thanks Richard, IMO, no changes are required as result of this review. I did mention one change that's missing from the current draft -25 which was decided during the IETF meeting period and was not push out yet (my appologized) - it is not significant. EH > -----Original Message----- > From: Richard L.Barnes [mailto:rbarnes@xxxxxxx] > Sent: Tuesday, April 10, 2012 6:11 AM > 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. The use of HTTP Basic is limited to client credentials, not end-user credentials. In both cases, the credentials are sent over a secure channel via TLS. The use of HTTP Digest was discussed and ruled out by WG consensus due to its complexity and limited adoption. As a general rule, the WG has opted to rely on TLS when transmitting plaintext credentials (as well as not to transmit them in the URI query). I believe section 10.7 Resource Owner Password Credentials sufficiently addresses these concerns when it comes to end-user 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? Addressed in section 2: the client type is provided by the client developer during registration. BTW, there is one change approved by the WG which has not yet been applied to -25 (due to timing issues with the IESG review and IETF meeting feeze). The text in section 2.1 beginning with "A client application consisting of" is to be replaced with: --- A clients may be implemented as a distributed set of components, each with a different client type and security context (e.g. a distributed client with both a confidential server-based component and a public browser-based component). If the authorization server does not provide support for such clients, or does not provide guidance with regard to their registration, the client SHOULD register each component as a separate client. --- > 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. 302 is only used in two examples but the normative text only discussed "redirects" and "directs" which are open to any implementation alternatives. > 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. Client authentication is intentionally not included in protected resources requests. This is a change made from 1.0 because of significant deployment issues. I can add a warning but there is nothing the resource server can do given the protocol design. > 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. I find the current organization with each item featured in a separate section easier to implement. Also, while some require secure transmission, it is still optional for everything else. Adding such a table might give the wrong impression that this list is normative and restrictive. > 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. This is only used with the Token Endpoint (3.2) which is limited to POST only. > 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). > " We did not have WG consensus to mandate redirection URI registration for all clients. > 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. It does include the client identifier via the client authentication requirements specified for the Token endpoint. The example shows that. Notice the text: If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1. The reason for including the redirect_uri is explained here: http://hueniverse.com/2011/06/oauth-2-0-redirection-uri-validation/ > 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. Same as above. > 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? It's not. The examples show the header but there is nothing to prevent using other means. In fact, the bearer draft offers such mechanism (IIRC). > 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. The resource endpoint isn't really part of the authorization protocol. > 3.2: It's not clear why only POST is allowed here. A couple of words of > explanation would help. To prevent credentials from leaking via the URI query. I don't think we need to explain the reason as we do not explain any other similar choices made. > 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. Because it is linked to the HTTP authentication scheme name which is case insensitive. EH