[OAUTH-WG] Partial set of last call comments on OAuth draft 20 from Yaron Goland

[Mike Jones <[e-mail]>]

1.4. Authorization Grant: Change "resource owner authorization" to "resource owner's authorization".

1.4.1. Authorization Code:  Comment: "It seems odd that we can discuss the authorization code without also discussing the security issues it raises (e.g. passing secure information via a browser) and thus motivating the introduction of the refresh token. I would expect this to be referred to here.  Having read the security considerations section I can find no coherent discussion there either of the relationship between the authorization code and the refresh token and why one motivated the other. I think this is something important for implementers to understand."

1.4.2.  Implicit:  Comment: "I find this section confusing. I think an example is all but mandatory here if the reader is to understand what is intended.  For example, when I first read this section I thought it was some kind of short cut used in cases where the client and authorization server had a close relationship and could share information such as the client's identity so no access code was needed.  No where in here is any hint that this is about browser based clients who don't have servers and who need a way to get tokens. Seriously. Try to read this section as someone who knows nothing about OAuth and tell me that you get the right idea back from it. It needs to be written to be both explicit as to its goals and clearer as to its means."

1.4.2.  Implicit:  Change "authenticate the client and the" to "authenticate the client directly. The".

1.4.2.  Implicit:  Change "weighted" to "weighed".

1.4.3.  Resource Owner Password Credentials: Comment on "(when combined with a refresh token)": "This is the first time that refresh tokens are mentioned in the spec. And yet there is no explanation of what they are. I suspect they should anyway be introduced in section 1.4.1 (as previously noted) and then their use here will make sense.  If that isn't possible then it would be good to have a forward reference to section 1.5 below so the reader has some idea of what's going on."

1.4.4.  Client Credentials:  Comment on "(the client is also the resource owner)": "I think this is misleading. Imagine something like Office where a client has been granted access to a particular resource by the resource owner. The client can then ask for an access token to the resource using the client credentials and no access code or refresh token. Just having the address of the intended resource (probably provided via SCOPE) along with the client credentials is more than enough to decide if an access token should be issued.

My point is that the client credentials scenario is fully applicable to cases where the client is not the resource owner but in which the resource URI provides all the context needed to determine if the client should be able to get an access token. I think the current text would imply otherwise.

So I would propose changing the sentence to "Client credentials are used as an authorization grant when the client is acting on its own behalf (the client is also the resource owner) or when the authorization server has enough context to determine from the access token request if the client has authorization to access the requested resource.""

1.4.5. Extensions: Comment: "I would change this sentence to read "Additional grant types may be defined to support new approaches to authentication/authorization as well as to provide a bridge between OAuth and other protocols.""

1.5. Refresh Token: Comment on "Refresh tokens are credentials used to obtain access tokens.": "This sentence presumes that refresh tokens are self-contained credentials. In other words that one can get an access token just using a refresh token and nothing else. This presumption is rebutted later in the document but I think it's confusing to imply one thing here and state otherwise later on."

1.5. Refresh Token: Change "a protected resource requests" to "a protected resource request".

1.5. Refresh Token: Comment on "(G)  The client requests a new access token by authenticating with the authorization server and presenting the refresh token.": "Hum... now the text seems to contradict itself. Above it seemed like you could use the refresh token by itself to get an access token and I had to ask for language that made it clear that you could use a refresh token with other credentials.  But the text of point G now implies the exact opposite, that refresh tokens can only be used with other credentials and not by themselves. (Even though the picture in figure 2 for step G implies the opposite). I would edit this language to say "The client requests a new access token. Depending on the authorization server's policies this request might be made with the refresh token by itself or with a combination of the refresh token and other client credentials.""

2.1. Client Types: Comment on "user-agent-based application": "Give the poor reader a hint and mention 'browser' somewhere in the paragraph below. For example "A user-agent-based application is a public client (such as a web browser) in which the....""

2.1. Client Types: web application: Change "access tokens or refresh tokens, can receive an acceptable level" to "access tokens or refresh tokens, can, in some cases, receive an acceptable level".

2.4. Client Authentication:  Add a period after "etc".

2.4.1. Client Password: Comment on "charset=UTF-8": "The use of UTF-8 with x-www-form-urlencoded is explicitly banned by HTML 4.01 section 17.13.1. If we want to use UTF-8 then we have to use multipart/form-data, also defined by HTML 4.01 (section 17.13.4). But since nobody would actually want to do that I would suggest putting in a reference to section 4.2 of RFC 5552 which actually defines how to use UTF-8 with x-www-form-urlencoded and specifying that the 5552 extension MUST be supported by OAuth participants."

3.1.  Authorization Endpoint: Comment on first sentence:  "There is a third option - none of the above.  It would be a shame if the text of this draft explicitly prohibits that pattern."

3.1.  Authorization Endpoint:  Comment on "[RFC3986] section 3": "Shouldn't we point directly to section 3.4 which exactly defines the query component?"

3.1.  Authorization Endpoint:  Comment on "MUST be retained when adding additional query
   parameters": "HIGH IMPORTANCE - This specification is incomplete. Section 3.4 of RFC 3986 simply asserts the existence of a query component. It says nothing about the syntax of that component. The spec assumes that the component is using x-www-form-urlencoded but that is not in any way mandated by RFC 3986. So there is a normative sentence missing here which states that any query parameter on the authorization endpoint's URI MUST be defined as specified in x-www-form-urlencoded."

3.1.  Authorization Endpoint:  Comment on "The authorization server SHOULD ignore unrecognized request parameters.": "Although that is normal behavior for application protocols it seems rather dangerous for a security protocol. After all what if the client put in a parameter specifying something security related about the request thinking that the authorization endpoint would honor it and then the authorization endpoint silently ignores it?  Again, this is a security protocol, not a generic application protocol, it seems to be that unrecognized parameters should cause a failure."

3.1.2.  Redirection Endpoint: Comment on "when initiating the authorization request": "I would suggest more explicit language "or as specified in the redirection URI value in the authorization request.""

3.1.2.2. Registration Requirements: Comment on last word "path": "Huh? Scheme, authorization and path is a complete URI minus a query component.  Is the goal to specifically mandate that only the query component can vary and the rest of the URI MUST be static? If so that should be stated explicitly rather than implicitly as it is now.  But I think, if that is the intent, it goes too far. We should also allow for additional path segments to be added to the URI path beyond the registered prefix. Assuming we can address the security problem in the next comment."

3.1.2.3. Dynamic Configuration: Comment on "section 6":  "The reference to section 6 is incomplete. Section 6 defines no less than 6 different axis's on which URI comparisons can vary. Furthermore as recently documented in draft-thaler-iab-identifier-comparison-00.txt it is trivially easy to screw up URI comparisons and the security implications are pretty dire. Personally I think that the only sane approach given the issues in the previous link is to adopt section 6.2.1 of RFC 3986.

I am a bit scared of how to handle partial matches. It's tempting to argue that so long as the schema has an authority and at least one path segment then we can just use a partial string match but boy oh boy do I see people screwing that one up royally. This is scary enough that I think I can be argued into saying that partial pre-registered URIs MUST only differ by the query component.

In any case this issues needs to be explicitly called out for all URI comparisons required by the spec and normatively defined. Otherwise we will end up with all the security issues in Dave's ID."

3.1.2.4.  Invalid Endpoint: Comment on "open redirector": "How many people even know what the heck an open redirector is? I think we need a section in the security considerations section that defines what an open redirector is and why it's bad. Alternatively a normative reference to a complete definition somewhere else is also fine."

3.2.1. Client Authentication: Change "Recovery" to "Recovering".

3.2.1. Client Authentication: Change "credentials, by preventing an attacker from abusing" to "credentials. This prevents an attacker from abusing"

3.2.1. Client Authentication: Change "periodic credentials rotation" to "periodic credential rotation".

3.2.1. Client Authentication: Comment on "while rotation of a single set of client credentials is significantly easier": "The spec calls out rotation of credentials as being crucial but then doesn't define how this is to be done? That seems kind of odd. Shouldn't we define an endpoint where one can submit one's old but unexpired credentials for new ones? This still leaves the edge case of what to do if the old credentials have expired and new ones haven't been issued but that is unavoidable and in that case we can require out of scope action."

4.1. Authorization Code: Comment on first "^": "Shouldn't this be a v character and not a ^? This would make it consistent with A which also crosses two boxes and points in the same direction."

4.1. Authorization Code: Change "If valid, responds back with an access token" to "If the request is valid, then the authorization server will responds back with an access token and optionally a refresh token".

4.1.2.  Authorization Response: Comment "The language around scopes in the document is really frustrating. One only finds out much later in the document that it is perfectly allowable for an authorization server to more or less ignore what scopes are submitted and instead to just return whatever the hell they want. This is a fine design choice but it seems like the sort of thing that should be forcefully repeated anywhere in the spec that we allow people to submit scopes so nobody forgets."

4.1.2.  Authorization Response: Comment on "state": "Don't we have to provide some guidance on how large the state value can safely be? Otherwise we are asking clients to rewrite their state mechanisms every time they throw in support for a new protected resource server. We have to set expectations across the industry if we are to hope for actual interoperability."

4.1.2.1. Error Response: Comment on "UTF-8 encoded text": "I think just saying UTF-8 encoded text can be misleading. It's not legal to put UTF-8 characters into a x-www-form-urlencoded used in a GET (as defined by section 17.13.1 of HTML 4.01). So what you have to do is take the UTF-8 String and then URL encode it. Which is fine but we should call this out.  Note that this is a separate issue than UTF-8 support for x-www-form-urlencoded. That issue only applies when the form is in the request body. In this scenario the form is in a URL. There is no question that URLs in HTTP request lists don't support UTF-8."

4.1.3. Access Token Request: Change "For example, the client makes the following HTTP using" to "For example, if the client makes the following HTTP request using"

4.1.3. Access Token Request: Comment on "that their values are identical": "This verbiage isn't much use, for example, if the client can always send the same redirect_uri. So, just to be clear, this text here doesn't in anyway change my previous recommendation regarding the attack previously described."

4.1.4. Access Token Response: Comment on ""token_type":"example"": "Just to prevent any confusion it would be good to actually define the token_type "example" in this document (Probably in section 7.1 and later in the IANA section) and specify that it is reserved for use in examples where one does not wish to be more specific."

4.2.  Implicit Grant: Comment "I have run into multiple people (including myself) who have read the OAuth spec and came away completely confused about when the heck one is supposed to use the implicit grant flow for. It's not immediately obvious that it's a way to support standalone browser based clients. Can we put in an example or something to make it more obvious why it's here?"

4.2.1. Authorization Request: Comment on redirect_uri:  "Given that the only way to identify the client in the implicit grant flow is via the redirect_uri, how can it be optional?"

4.3. Resource Owner Password Credentials: Change "enabling the grant type, and only when other flows" to "enabling the grant type and only use it when other flows".

4.3. Resource Owner Password Credentials: Change "resource owner credentials" to "resource owner's credentials".

4.3.2. Access Token Request:  Comment on "authorization server MUST protect the endpoint against brute force attacks": "Shouldn't the requirement to prevent against brute force attacks apply to all credentials, resource owner or otherwise? In other words in section 2.4.1 we talk about how clients submit their name/password. Shouldn't endpoints accepting client name/passwords have the same protections against brute force attack?"

4.4.3. Access Token Response: Comment on "A refresh token SHOULD NOT be included": "Why isn't this a MUST NOT?"

4.5. Extensions: Comment "Isn't the text in this section and 8.3 redundant with each other? It seems like we should take the text from here and merge it with section 8.3 so all the extension information is recorded in one place.  It's just plain that all other extension points are in section 8 but this one."

5.1. Successful Response: Comment on "parameter at the highest structure level": "Are there any guarantees about the order of the members in the JSON object? If not we should say so explicitly."

5.2. Error Response: Comment on "multiple client authentications included" in invalid_client: "Shouldn't this be an invalid_request?"

5.2. Error Response: Comment on "Numerical values are included as JSON numbers": "Same issue as before, does ordering matter and if not we need to say that."

8.1. Defining Access Token Types: Change "or use a unique" to "or using a unique".

9.  Native Applications:  Change "an scheme" to "a scheme"

9.  Native Applications:  Change "offer an improved usability" to "offer improved usability"

9.  Native Applications:  Change "inability to keep credentials confidential" to "inability to keep client credentials confidential".

9.  Native Applications:  Change "When using the implicit grant type flow a refresh token is not returned" to "When using the implicit grant type flow a refresh token is not returned so once the access token expires the application will have to repeat the authorization process".

10.2. Client Impersonation:  Change "keep is client" to "keep its client".

10.2. Client Impersonation:  Change "due to the client nature" to "due to the client's nature".

10.2. Client Impersonation:  Change "URI used for receiving authorization" to "URI used for receiving authorization requests"

10.2. Client Impersonation:  Change "engage the resource owner" to "engaging the resource owner".

10.2. Client Impersonation:  Change "and authorize the request" to "and authorize or reject the request".

10.3. Access Tokens: Change "Access token (as well as any access token type-specific attributes)" to "Access tokens (as well as any access token type specific attributes)".

10.3. Access Tokens: Change "guessed to produce" to "guessed so as to prevent unauthorized parties from producing".

10.4. Refresh Tokens: Comment "As mentioned previously we really should explain why we introduced refresh tokens. Without understanding the intent of the mechanism it's unlikely implementers will use them appropriately."

10.4. Refresh Tokens:  Change "storage, and shared only among" to "storage, and are to be shared only among".

10.4. Refresh Tokens:  Change "refresh tokens rotation" to "refresh token rotation".

10.4. Refresh Tokens:  Change "guessed to produce" to "guessed so as to prevent unauthorized parties from producing".

10.6. Authorization Code Leakage: Comment "I fancy myself as being reasonably intelligent and I'm unclear what attack is actually being described here."

10.6. Authorization Code Leakage: Comment on "The authorization server SHOULD require the client to register their redirection URI": "Why is this a should?"

10.7. Resource Owner Password Credentials: Comment on "password anti-pattern": "Is it fair to expect that the reader knows what the password anti-pattern is? Shouldn't some reference to a definition of this pattern be required?"

10.7. Resource Owner Password Credentials: Comment on "The authorization server SHOULD restrict the scope and lifetime of access tokens issued via this grant type": "Restrict in what ways? Based on what criteria? This statement is the equivalent of saying "don't do bad stuff". Perhaps true but not terribly useful."

10.12. Cross-Site Request Forgery: Comment on first paragraph: "I challenge any reasonably intelligent person who does not already know what a CSRF is to read this paragraph and come away any clearer as to what is being discussed. At a minimum isn't some reference to a complete definition of CSRF needed if there is to be any hope of user compliance?"

10.12. Cross-Site Request Forgery: Comment on "The "state" request parameter MUST contain a non-guessable value": "Actually a non-guessable value won't stop the attack discussed in the previous paragraph on its own. What's also needed is a way to uniquely (and unbreakably) associate the state with a particular user so that if an authorization code swap occurs it can be reliably detected."

10.13. Clickjacking: Comment on "x-frame-options": "The recommendation seems confused. Is it o.k. to just rely on x-frame-options or MUST other actions be taken?"

11.1.  The OAuth Access Token Type Registry: Change "toke type" to "token type".

11.1.1.  Registration Template: Change "protected resources requests using access token" to "protected resource requests using access tokens".

11.1.1.  Registration Template:  Change "Reference to document" to "Reference to the document".

________________________________