Re: [OAUTH-WG] secdir review of draft-ietf-oauth-v2

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

I have not seen any follow up to the open issues and will be closing them on my editor's list. This doesn't mean they are closed, just that I will not be addressing them without someone raising them again on the list. Since none of them are in the tracker, this email is the only record I know of listing them (and their status/response).

EHL

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of Eran Hammer-Lahav
> Sent: Tuesday, September 20, 2011 3:13 PM
> To: Leif Johansson; OAuth WG
> Subject: Re: [OAUTH-WG] secdir review of draft-ietf-oauth-v2
>
> (+OAuth WG, -everyone else)
>
> Thanks Leif.
>
> See comments inline. Whatever issues are still open will be addressed along
> with the rest of the IETF LC feedback.
>
> EHL
>
>
> > -----Original Message-----
> > From: Leif Johansson [mailto:[e-mail]]
> > Sent: Monday, September 12, 2011 11:31 AM
>
> > ** General observations:
> >
> > POST and/or GET
> >
> > Examples are sometimes POST and sometimes GET. In many cases it is not
> > clear to me from the surrounding text if both POST and GET are allowed
> > or if only one is mandated. Illustrating with both a GET _and_ POST
> > example in the cases where both are supported would help or make the
> > method explicit in the text before the example.
> >
> > The P-word
> >
> > The term 'password' is sprinkled throughout the document, sometimes as
> > in "client password" or "resource owner password credentials" and I
> > suspect that sometimes it is password as in 'an example of a
> > credential type' and in other cases it is password as in 'plain old
> > password'. This needs to be cleared up throughout (I've included some
> examples below).
> >
> > Normative Language
> >
> > I've often found myself wanting more normative language often to
> > replace existing but less precise text. I've called out some important cases
> below.
> >
> > Unknown parameters
> >
> > The sentence "The client SHOULD ignore unrecognized response
> > parameters"
> > occurs in several places. First of all I would argue that this has to
> > be a MUST if you want to be able to guarantee extensibility. Secondly
> > there are some error responses that are triggered by unsupported
> > request parameters. This seems like an inconsistency.
> >
> > ** Specifics
> >
> > 1.1 Roles
> >
> > Forward references to the sections where the roles are defined would
> > improve readability.
>
> What sections? That's the only place these roles are defined.
>
> > As an illustration of an alternative model for the authorization
> > server maybe an informative reference to UMA would be illustrative here.
>
> A reference to UMA would be confusing in this document.
>
> > 1.2 Protocol Flow
> >
> > (A) talks about 'an intermediary such as an authorization server.' it
> > isn't clear it me if this refers to a separate authorization server or
> > if it is the same authorization server that is referenced in (B).
>
> Changed to:
>
>               (A) The client requests authorization from the resource owner. The
> authorization request
>               can be made directly to the resource owner (as shown), or preferably
> indirectly via
>               the authorization server as an intermediary.
>
> > 1.3.3 Resource Owner Password Credentials
> >
> > When I first read this I thought - why not talk about Resource Owner
> > Credentials - in fact there is a parenthesis there "(e.g a username
> > and password)" that seems to indicate that other credentials could be
> supported.
> >
> > This needs to be cleared up. Either its username and password and
> > nothing else or there is a way to support things like Negotiate or
> > X.509-based credentials in which case it should really be Resource
> > Owner Credentials with no reference to passwords other than as as an
> > example of one possible credential type.
>
> Changed to:
>
>         (i.e. username and password)
>
> This is explicitly just for username and password. Other types require an
> extension.
>
> > 1.4 Access Token
> >
> > first paragraph: "The string is usually opaque". This should be
> > reformulated as normative language. In fact I would have liked
> > guidance on generating those tokens (which has been brought up on the
> > list) possibly as part of an implementation-guide.
>
> There is not requirement for the string to be opaque, but the general
> architecture assumes it is. Otherwise, please suggest language.
>
> > 1.5 Refresh Token
> >
> > Why is the refresh token not simply treated as an access token for the
> > access token resource in the authorization server? This would seem to
> > simplify the model a bit by removing a type of token whose semantic
> > (at least to this
> > reviewer) is a duplication of a normal access token for a particular
> > type of resource.
>
> Simpler architecture but probably more confusing to many developers.
>
> > Also in the first paragraph: "(access tokens may have a shorter
> > lifetime and fewer permissions". Why not try to write normative
> > language here - there are security implications of allowing a refresh
> > token to get more permissions
> > (say) than the original access token.
>
> This was discussed before and we could not reach consensus.
>
> > 2.1 Client types
> >
> > I find the terminology public/confidential somewhat counter-intuitive.
> > An app you have on your personal device is 'public' while a shared
> > cloud service is 'confidential'...hmm...
>
> These are the best terms we could come up with. Not intuitive but well
> defined.
>
> > This section also lacks normative language which isn't good. There
> > should be language defining the behavior of public and confidential
> > client aswell as the three profiles.
> >
> > For instance under native application we have "It is assumed that any
> > client authentication credentials included in the application can be
> > extracted". This should really be normative language. Some of what I
> > am looking for can be found in section 2.3 but it isn't clear to me
> > that it covers the definition of the profiles.
>
> Please suggest text.
>
> > 2.3.1 Client Password
> >
> > There is also some confusion here about what the client must implement
> > and what the server must implement wrt the use of client_id. I read
> > the text as trying to say that Servers SHOULD support both and clients
> > SHOULD only do Basic.
>
> We could not reach consensus beyond this. This language was carefully
> crafted to work around the disagreements about what is required and what
> is not.
>
> > This section also seems to have been the product of a partial
> > s/password/credential/g operation. Either OAUTH is only meant to use
> > Basic and passwords or we want to cover all/most HTTP authentication
> > methods and credentials and then section 2.3.2 (which feels like an
> > afterthought) needs to get folded into 2.3.1 and the normative
> > language (and examples!) need some work to cover at least X.509s and
> > perhaps even Negotiate.
>
> When we say password we mean 'plain old password'. Any other types of
> credentials are not covered by design.
>
> > Personally I would try to fold 2.3.1 and 2.3.2 into one section and
> > say that servers MUST support Basic and client_id+client_password.
> > Servers MAY support any HTTP authentication method with a mapping to
> client_id.
> > If a client supports username+passwords it SHOULD do Basic and if it
> > supports other HTTP authentication methods it MUST NOT use
> > client_password for anything and MUST follow normal HTTP
> > authentication method negotiation.
> >
> > Finally, everywhere there is negotiation there must imho be some
> > mention/discussion/protection of downgrade attacks.
>
> Downgrade attacks security section sounds useful. Text?
>
> > 3.1 Authorization Endpoints
> >
> > 6th paragraph: "The authorization server SHOULD ignore unrecognized
> > request parameters". This formulation returns in several places in the
> > document and I don't understand why it isn't a MUST - after all
> > doesn't extensibility depend on this?
>
> I don't have an objection, but extensibility isn't currently required.
>
> > 3.1.1 Response Type
> >
> > The response_type parameter is REQURED but its absence SHOULD result
> > in an error. Why not MUST?
>
> This is an OAuth 1.0 legacy of not mandating a particular error behavior.
> MUST seems like the right language - any objections?
>
> > 3.1.2 Redirection Endpoint
> >
> > There should be a clear normative specification for how to  match
> endpoints.
> > There are traces of this in various parts of the document when
> > matching is discussed. I recommend making a concise definition in one
> > place (namely
> > here) and referencing this throughout. Since this comparison has
> > security implications it should be a priority for the specification to be air-
> tight.
>
> This has been discussed by the WG and no consensus was reached. We can
> reconsider if someone submits proposed text.
>
> > 3.1.2.2 Registration Requirements
> >
> > "(the client MAY use the state request parameter to achieve
> > per-request customization)". Doesn't this overload its use for CSRF-
> protection?
>
> Yep. That's intentional.
>
> > 3.1.2.4 Invalid Endpoint
> >
> > "The authorization server SHOULD NOT redirect...". Why isn't this a
> > MUST NOT?
>
> I'm ok with MUST NOT - any objections?
>
> > 3.1.2.5 Endpoint Content
> >
> > This section basically seems to say "Serve with server-side code not
> > with html or client-side code". If this is the case then I suggest
> > reformulate to say precisely that using normative language.
> >
> > The use of the word "script" could perhaps also be made less ambiguous
> > since in this case it could refer to both server-side code aswell as
> > in-browser code.
>
> This is only about the HTML response sent back to the user-agent. This is
> about including something like 'Share on Twitter/Facebook' widget on a page
> with an access token in the URI fragment (which has caused tokens to leak to
> Twitter in some cases).
>
> > 3.2.1 Client Authentication
> >
> > The phrase "clients issued client credentials" could be rephrased to
> > make less violence on English - perhaps "clients that have been issued
> > with client credentials", unless that is not the intended meaning in
> > which case I argue for something easier to understand ;-)
>
> Ok.
>
> > The second bullet: Using client credentials more often also exposes
> > them more which might be worth mentioning aswell.
>
> Proposed text?
>
> > 4. Obtaining Authorization
> >
> > Perhaps not critical but the term 'password credentials' occurs in the
> > first paragraph and this doesn't seem compatible with resource owner
> > authentication being out of scope. It could just be that it should
> > read 'resource owner credentials' but it could also signal an
> > underlying assumption about username and password being used for
> > resource owner authentication. In either case I thing its best to
> > avoid the use of the word 'password' to avoid any confusion.
>
> Password is intentional. This is a special case for passwords.
>
> > 4.1 Authorization Code
> >
> > (C) - "using the redirection URI provided earlier" should perhaps read
> > "using the redirection URI provided earlier or associated with the
> > client in client registration"
>
> Changed to:
>
>               Assuming the resource owner grants access, the authorization server
> redirects the
>               user-agent back to the client using the redirection URI provided earlier
> (in the
>               request or during client registration). The redirection URI includes an
> authorization
>               code and any local state provided by the client earlier.
>
> > 4.1.1 and 4.1.2 Authorization Request/Authorization Response
> >
> > The redirect_uri is listed as OPTIONAL with a reference to 3.1.2 but
> > there is no mention in 4.1.2 how to handle the case when the
> > redirect_uri is not present. I believe the assumption is that the
> > redirect_uri is either resent or part of client registration but that needs to
> be made explicit in that case.
>
> 3.1.2 describe how to handle this parameter.
>
> > This may apply to other uses of the redirect_uri parameter eg in 4.2.1.
> >
> > Furthermore in 4.2.2 "code" I suggest the following re-formulatation
> > of the last sentence: "The client MUST NOT use an authorization code
> > for more than one request. If an authorization code is re-used, the
> > authorization server should treat that as a replay attack and SHOULD
> > revoke all tokens associated with the client." (i.e loose the "attempt"
> > bit which carries no real meaning)
>
> Changed to server MUST not allow reuse based on previous feedback. Not
> sure treating it as an attack is the right language, but the normative language
> is the same either way (about revoking tokens).
>
> > Also note that this is potentially a DOS attack should a single authz code
> leak.
>
> Explain?
>
> > 4.1.2.1 Error Response
> >
> > First paragraph, last sentence "and MUST NOT automatically redirect".
> > In the light of how willing users normally are to click on links
> > presented to them I would strengthen this to "MUST prevent the user
> > from following the redirect URI"
>
> Not sure what you mean.
>
> > In the definition of the invalid_request parameter I don't understand
> > how unsupported parameters can generate an error since endpoints
> > should ignore unsupported parameters (in order to support extensibility).
>
> Good catch. Dropped 'unsupported parameter' part.
>
> > 4.1.3 Access Token Request
> >
> > "require client authentication for confidential clients or for any
> > client issued client credentials (or with other authentication requirements)"
> >
> > This text seems unnecessarily convoluted. Isn't enough to say that if
> > you have issued credentials to a client you MUST require
> > authentication from that client? This applies equally to the other
> > sections where client authentication is an issue (eg 4.3.2).
>
> I felt it was important to call out confidential clients explicitly, but I agree that
> if we require issuing credentials to such clients, this can be made simpler.
> What do other think? The same end result, but different emphasis.
>
> > Also cf my comment on 3.1.2 for the discussion of matching
> > redirect_uri in the last bullet: ".. and that their values are
> > identical". Is this really meant to mean identical?
>
> Here yes. String comparison.
>
> > 4.2 Implicit Grant
> >
> > The parenthesis "(it does not support the issuance of refresh tokens)"
> > sounds like it should really be normative language, "refresh tokens
> > MUST NOT be issues for implicit grant" because afaiu you could easily
> > extend "fragment-transport" to include a refresh-token, so it isn't a
> > technically motivated constraint, right?
>
> Instead I added this to 4.2.2:
>
>             The authorization server MUST NOT issue a refresh token.
>
> > In (D) I would like to have a normative reference to a document that
> > specifies browser behavior for URL fragments since this is a critical
> > security dependency for this grant type.
>
> That's RFC2616. Seems odd to reference it here.
>
> > 4.4 Client Credentials
> >
> > I think the text should note that this model effectively implies that
> > the client is able to impersonate all users which has the potential
> > for worse security problems than if the client has access to individual user
> passwords.
>
> Worse how? You can't use this with resource owner password.
>
> > 6 Refreshing an Access Token
> >
> > scope - The term "lesser than" is intuitive but imprecise. I suggest
> > "MUST NOT include any scope not originally granted by the resource
> owner".
>
> Ok.
>
> > 7.1 and 8.1 Access Token Types
> >
> > The section 7.1 lists two definitions of access token types and
> > provides a couple of examples but doesn't provide any constraints on
> > future development of access tokens except in 8.1 where it is implied
> > that not all access token types would be associated with HTTP
> > authentication mechanisms. Are there really no constraints on access
> > token types that should be captured?
>
> None suggested.
>
> > 10.6 Authorization Code Redirection URI Manipulation
> >
> > Suggest replace the word 'familiar' with 'trusted' in the first
> > sentence of the second paragraph. The notion of trust opens up several
> > boat loads of worm but it is the correct term here I think.
>
> Ok.
>
> > In the third paragraph "the same as" wrt redirection URIs occur and
> > this needs to be defined (cf comments on 3.1.2 above).
>
> Changed to identical.
>
> > Finally "The authorization server MUST require public clients and
> > SHOULD require confidential clients to register their redirection
> > URI". I am missing a discussion of why the two types of client differ wrt this
> attack vector.
>
> Public clients rely on the registration to provide a minimal level of client
> identification while confidential clients are later authenticated which can
> mitigate redirection URI manipulation.
>
> > 10.10 Credentials Guessing Attack
> >
> > I found myself wanting implementation advice for how to generate good
> > tokens at this point. This has been raised on the list too. The same
> > goes for how to generate good CSRF cookies where the "(eg a hash of
> > the session cookie..." feels like it is implementation advice yearning
> > to come out of the closet.
>
> Need someone to suggest text.
>
> EHL
>
> _______________________________________________
> OAuth mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/oauth