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

[Eran Hammer-Lahav <[e-mail]>]

This is fantastic feedback. Some parts moved to new threads.

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of Mike Jones
> Sent: Wednesday, August 10, 2011 2:39 PM

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

Ok.

> 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."

This particular relationship has not been discussed on the list before.

The primary justifications of refresh tokens were the need to allow access token rotations because of the weaker nature of bearer tokens, and to support large scale deployments in which access tokens are self-contained and do not require a DB lookup (which implies short-lived, similar to self-contained session cookies).

If we are going to add a discussion, it should cover these as well, however, such a discussion really belongs in the threat model document. We have not included any other motivations in the document and I don't feel this justifies an exception.

Either way, please provide text and we can decide where it belongs.

> 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."

Changed first paragraph to:

            The implicit grant is a simplified authorization code flow optimized for clients
            implemented in a browser using a scripting language such as JavaScript. In the implicit
            flow, instead of issuing the client an authorization code, the client is issued an
            access token directly (as the result of the resource owner authorization). The grant
            type is implicit as no intermediate credentials (such as an authorization code) are
            issued (and later used to obtain an access token).

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

Text already changed.

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

Ok.

> 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."

I removed '(when combined with a refresh token)'. This is actually not true as there is no assumption that access tokens are always short-lived or that refresh tokens will always be issued to native applications using this grant type.

> 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.""

Added:

"or is requesting access to protected resources based on an authorization previously arranged with the authorization server."

To make is more in line with the existing language of 4.4.

> 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.""

Took out the entire section. The introduction to 1.4 already contained all the relevant information.

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

Ok.

> 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: 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.""

Added to (G): "The client authentication requirements are based on the client type and on the authorization server policies."

> 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....""

Ok.

> 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".

This doesn't really add anything.

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

Huh?

> 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."

http://www.w3.org/TR/html401/interact/forms.html#h-17.13.1

Where?

> 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."

Dropped "which is expressed explicitly as an authorization code (later exchanged for an access token), or implicitly by direct issuance of an access token" as it is no longer accurate given the new response type extensibility. Either way, section 3.1.1 covers this.

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

Ok.

> 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."

Changed to:

   The endpoint URI MAY include an "application/x-www-form-urlencoded"
   formatted ([W3C.REC-html401-19991224]) query component ([RFC3986]
   section 3.4), which MUST be retained when adding additional query
   parameters.  The endpoint URI MUST NOT include a fragment component.

> 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."

This is an issue for those defining additional parameters. Basically, you should not define new parameters that when not-understood cause degradation of the base protocol security.

> 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.""

Changed to "during the client registration process or when making the authorization request".

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

Ok.

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

Changed 'by' to 'thus'.

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

Ok.

> 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."

The spec leaves much out of scope. You are welcome to submit a proposal for such a new endpoint.

> 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."

(B) is bi-directional (server serves a login page, user enter password, fetches another page...).

> 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".

Ok.

> 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."

Need proposed text.

> 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."

Whatever value goes into the parameter has to be form-encoded. I think that's pretty clear throughout the specification. We don't put any restrictions on most values we expect to be delivered using form-encoded strings. The expectation is that implementers will use a library that takes some structure with parameter names and values and turn it into a valid URI or body.

> 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"

Already fixed.

> 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."

Huh?

> 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."

Added to 8.1: "The token type 'example' is reserved for use in examples."

> 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?"

I think the text is pretty clear:

   These clients are typically implemented in a browser using a scripting language
   such as JavaScript.

But I'm open to suggestions. Please propose text.

> 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?"

client_id is required.

> 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".

Ok.

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

Ok.

> 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?"

Yes, but in general brute force attacks are more likely on human-friendly passwords than machine-issued client credentials. I'll add it to 2.4.1:

            Since this client authentication method involves a password, the authorization server
            MUST protect any endpoint utilizing it against brute force attacks.

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

Working group consensus. It was discussed and some people expressed use cases for allowing it.

> 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."

This one is about usage white the others are about registration. This is the right place to inform developers of the need to handle URI values.

> 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."

Added: "The order of parameters does not matter and can vary."

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

Yep.

> 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."

Same text added.

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

Ok.

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

Ok.

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

Ok.

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

Ok.

> 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".

Ok.

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

Already fixed.

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

Ok.

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

Actually, responses.

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

Ok.

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

Ok.

> 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)".

Nah. I like the -.

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

Ok.

> 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."

Disagree. Feel free to propose text.

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

Nah.

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

Ok.

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

Ok.

> 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?"

In this case, lack of understanding does not take away from the rest of the prose and a reader can look it up. It is a well-established term and the reason for OAuth in the first place. But feel free to propose text.

> 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."

Yeah... especially with a normative SHOULD. Changed 'restrict' to 'consider' and lowercased the should (I know it doesn't really matter but it reads better now):

         The authorization server should consider the scope
         and lifetime of access tokens issued via this grant type.

> 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?"

It seems we are still discussing this section on another thread. But generally, we cannot be expected to provide a comprehensive web security guide within this protocol. The attack is presented in very general terms and the reader should go and study it if they are not familiar with it. I think the extra prose in the beginning doesn't hurt but I don't mind dropping it and just starting with "A cross-site request forgery (CSRF) allow an attacker to inject..." and drop the entire first paragraph.

No change for now.

> 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."

Discussed on another thread.

> 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?"

Seems pretty clear to me. There is no full solution, but using the header is one way to prevent newer browsers from allowing such an attack. Can't put a MUST here because there is no way to completely close this attack vector.

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

Ok.

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

Ok.

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

Ok.

Thanks!

EHL