Re: [OAUTH-WG] AD Review of -22 (part I)

[Eran Hammer <[e-mail]>]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of Stephen Farrell
> Sent: Thursday, October 13, 2011 10:13 AM

> List 1 - Fairly sure these need changes:
> ----------------------------------------
> 
> (1) In 2.3.1 MUST the AS support both HTTP basic and the client_id and
> client_secret in the request schemes? You say it MAY "allow"
> credentials in the request body, but that's different from what the AS coder
> has to implement.  Saying an AS that issues passwords MUST support "basic
> over TLS" and MAY support including credentials in the body would seem
> right here.

Changed 'MAY allow' to 'MAY support'. Clarified the TLS requirement for sending passwords.

> (2) In 2.3.1 (and more generally) what does "MUST require the use of a
> transport-layer security mechanism" really mean? I think you need to say
> explicitly what this means in terms of TLS and which version of TLS and which
> cipher suites etc. Doing that once is fine and you could then have a defined
> phrase for it, but it needs to be specified for each place where TLS is used.
> The text at the end of
> p15 is almost exactly what's needed, and would be if you said which cipher
> suites are MUSTs. I think you might need to pick cipher suites for each
> version if you want some combination of tls1.0 and tls1.2 and need server
> auth at least.

Replaced ' transport-layer' with TLS with reference to the new section.
 
> (3) Having a MUST for TLS1.0 and a SHOULD for TLS1.2 is going to generate a
> DISCUSS or two. I think you really need to justify that in the Document and
> PROTO write up if you want to stick with the current choices. I personally
> would prefer if those were swapped myself, that is have a MUST for the
> latest version of TLS (TLS1.2) and a MAY/SHOULD for TLS 1.0. In addition to
> taking care of process concerns, there is also the recent TLS chosen plaintext
> attack where TLS1.2 has no problem but TLS1.0 does. (This differs from (2)
> above, since the former is almost editorial, but I guess this one needs to go
> to the WG.)

This has been resolved by the chair and adjusted as such.
 
> (4) The response_type description in 3.1.1 is unclear. You say it MUST be
> "one of" various values, but then that it can be a space separated list of
> values. When >1 value is provided, it doesn't say what that means, it only
> says that the order is irrelevant. (It could mean "any of these" or "all of
> these" for example, both are order independent, but are not the same.) I
> suggest adding a sentence to say that "code token" means "please send
> both" if that's what it means.

Removed the space-delimited text from the end of the response_type parameter definition and added the following immediate after:

            Extension response types MAY contain a space-delimited (%x20) list of values, where the
            order of values does not matter (e.g. response type"a b" is
            the same as "b a"). The meaning of such composite response
            types is defined by their respective specifications.
 
> (5) How does a client implement the MUST in the last sentence of 3.1.2.3? I
> don't see anything here or in 10.15 that says how to do this. I guess ideally,
> you'd just need a reference to somewhere else in the doc or to something
> else, but I do think you need something since you've made this a "MUST
> ensure" rule for clients.
> Adding a sentence/short paragraph here or in 10.15 with some typical/good
> ways to ensure this would be fine.

I took that last paragraph out. Client mitigation isn't really practical here.

> (6) In 7.1 what does it mean to say that the client MUST NOT use the access
> token if it doesn't trust the token type? I don't know what code I'd write
> there in a client. Maybe s/trust/support/ or s/trust/understand/ would fix
> this.

Took 'trust' out.

> (7) Doesn't 7.1 need to say which token types are MTI so that we get
> interop?  I think I'd like to see mac being a MUST and bearer being a MAY but
> regardless of my preference, I don't think you can be silent on this.  One way
> to do this would be to mandate that the client MUST support mac and MAY
> support bearer but to allow the server to choose which token types they
> support.  And as a consequence one or both of the mac/bearer drafts need
> to end up as normative I think.

No change as resolved by the chair.

> (8) 10.3 says access tokens MUST be kept confidential in transit.
> Does that not mean that non-anonymous TLS is a MUST? If so, then saying
> that clearly here would be good. If not, then saying what's really meant
> clearly is needed. (Same point for refresh tokens in
> 10.4.)

Added to each section:

          [Access token credentials | Refresh tokens] MUST only be transmitted using TLS as described
          in <xref target='tls' /> with server authentication as defined by  <xref target='RFC2818' />.

> (9) 10.5 says "effort should be made" to keep codes confidential, but I expect
> that'll generate AD comments - that's just a bit too vague - what do you really
> mean there?

Took that sentence out. Added no value.

> (10) 10.10 has an impossible requirement - you cannot stop/prevent
> attackers guessing, but you can ensure that such guessing is futile. Can you
> not be a bit more specific about a "reasonable"
> level of entropy here? I'd say 10 bits is not enough, 128 is, and there may be a
> good level to at least RECOMMEND (e.g. maybe >N bits if rate limiting can
> effectively prevent 2^(N/2) guesses going undetected? I'm not
> recommending an "N" myself here, but rather that the WG consider picking
> one or else justify not picking one.
> (The same comment applies to the term "non-guessable" as used in
> 10.12 and maybe elsewhere as well.)

Per the requirement in the thread-model document, replaced with the following:

          Generated tokens and other credentials not intended for handling by end-users MUST be
          constructed from a cryptographically strong random or pseudo-random number sequence
          (<xref target='RFC1750' />) generated by the authorization server. The probability of any
          two values being identical MUST be less than or equal to 2^(-128) and SHOULD be less than
          or equal to 2^(-160).

Added a reference to 10.10 from 10.12.

> (11) Section 11 says a couple of times that the apps ADs are the appeal chain -
> shouldn't that be the SEC ADs now?

Fixed.
 
> List 2 - Questions: (not sure whether change needed or not)
> ----------
> 
> (1) p12 - 2.1 says that the client developer declares the type of the client, but
> then says that the definition is up to the authorization server really, so, in
> principle one client might have different types for different authorization
> servers. I think you could make this clearer by adding a sentence saying that
> one client could have >1 type according to different authorization servers or
> that the client type could change over time, e.g. as some vulnerability in an
> OS gets discovered or if a new OS feature is added.  I'm not sure if there's
> any action that could/should be taken if the client type changes, perhaps re-
> registration with a SHOULD for using a new redirection URL and invalidating
> all outstanding tokens or something? Maybe this sounds like something that
> some other document (and RFC or not) can tackle later.

Added to section 2.1:

          A client application consisting of multiple components, each with its own client type
          (e.g. a distributed client with both a confidential server-based component and a public
          browser-based component), MUST register each component separately as a different client
          to ensure proper handling by the authorization server. The authorization server MAY
          provider tools to manage such complex clients through a single administration interface.

> (2) 3.1 - I think you may need more about how passwords are handled, esp.
> when they contain odd characters. Is there really no problem with x-www-
> form-urlencoded when such a password is in a form? Have you checked with
> someone who really cares about such things?

Not sure I understand this, but how passwords are handled is outside the scope. Basically, the user-agent is sent to the endpoint and the server shows some kind of form. How that form is submitted is not part of OAuth is not part of the form encoding described. The form encoding mentioned is just about inputs send to the server before any login prompt.

If this is not clear, please let me know.

> (3) Generally, the specification is a bit unclear about what each component
> MUST do, it's hard to figure that out with the style of describing the 2119
> rules on a per endpoint basis. Not sure what to suggest that'd not be loads of
> work, but I wondered if there's evidence that someone not involved in the
> WG or Oauth1 would find it easy or very hard to implement and get interop?
> I know that there've been some people who've implemented from the
> drafts and I'm told some of those were not involved in the WG at all. If you
> know of any such implementers, it'd be good to note that in the PROTO
> write-up since it'd answer this question, which I'd expect to be posed by
> some AD.

We've got feedback from people who just read the spec without being involved here (most of it in the past couple of months) and it did not raise this issue.
 
> (4) Is it clear that a client always knows when a redirection request will result
> in sensitive data being sent, thus triggering the SHOULD for use of TLS in
> 3.1.2.1? It may well be the case but it wasn't clear to me from reading
> whether I could write code that'd know when it's ok to not use TLS.

Changed to:

              The redirection endpoint SHOULD require the use of TLS as described in
              <xref target='tls' /> when the requested response type is
              "code" or "token", or when the redirection request will result in the transmission of sensitive credentials
              over an open network.

> (5) 3.1.2.2 says AS'es MAY support >1 redirection URI. Why not make that a
> MUST? If a client needed it, then it couldn't interop with an AS that only
> supports 1, and it ought be easy for an AS to support >1 I guess.

Because this is not an interop issue. The number of supported redirection URIs is a matter of configuration. Until we have an spec for interop on that side, we don't need to prescribe it. Most 1.0 sites today support only one.

> (6) What's the case where the AS is ok to redirect to an unregistered or
> untrusted URI that corresponds to the 2nd para of 3.1.2.4?

I assume you are asking why isn't this a MUST NOT?

That sentence is a little bit tongue-in-cheek... In 3.1.2.2 we say that unregistered endpoints SHOULD NOT be allowed and this one is a way of saying why they are bad in a twisted way. After reading it again, I took that line out and added this to 3.1.2.2:

              Lack of a redirection URI registration requirement can enable an attacker to use
              the authorization endpoint as open redirector as described in
              <xref target='open-redirect' />.

> (7) Can a client really ensure that other scripts don't go before its own as
> required in 3.1.2.5?

Yes, for example, by using multiple endpoints chained in the right order.

> (8) What is the impact of the "MUST be taken into consideration" at the end
> of 3.2.1? I can't see how to implement that nor is it clear what checks an AS
> could do at registration time.

Dropped that line. 10.1 already covers this.

> (9) Does the recent list discussion about scope encoding require any changes
> to 3.3?

Yep. Been applied.

> (10) Is there anything that needs saying if the AS ignores the client's
> requested scope but doesn't indicate that in the response?
> Put another way - why is that SHOULD not a MUST? (Just asking that you
> explain, not change.)

No reason. I originally put it in as a SHOULD because scope was more of an nice idea than an important feature. This should probably be changed to MUST. Not going to make this change now, but will if the WG would like to. Will bring up in another thread.

> (11) State and scope variables probably should not contain anything sensitive
> for the client or user, since the AS and UA both get to see them, is that
> stated somewhere? (Maybe with a hint that if you need something sensitive
> in the state, then just generate a symmetric key and encrypt it for yourself.)
> An example of what not to include would be a banking client including a
> user's bank a/c number as the state variable.

Added to 10.8:

          The "state" and "scope" parameters
          SHOULD NOT include sensitive client or resource owner information in plain text as they
          can be transmitted over insecure channels or stored insecurely.

> (12) 4.4.3 says a refresh token SHOULD NOT be included. Seems like the AS
> actions for good requests are fairly variable, which is likely to cause
> developers to do the wrong thing - is there no way to make the AS actions
> more consistent? (Could be that they are consistent though, but I'm just
> getting lost in the text;-)

Yeah...

This specification sucks when it comes to prescribing interoperable behavior. At this point the biggest reason people hate OAuth 1.0 is not the crypto but that every server implementation is different. Wait until they experience 2.0... But unfortunately this is as far as this WG was able to agree. WG participants didn't want to come together and agree but to make sure they will be able to implement their own unique use cases (and screw interop).

I think the only hope moving forward is for someone to publish a profile that is tight and restrictive and see if it gains traction.

Yeah, I'm bitter about the quality of this work after investing 4 years into it, but that's life.

> (13) 10.9 says that the client MUST verify the server's cert which is fine.
> However, does that need a reference to e.g. rfc 6125?
> Also, do you want to be explicit here about the TLS server cert and thereby
> possibly rule out using DANE with the non PKI options that that WG (may)
> produce?

Don't know anything about this. Will bring up in another thread.
 
(To be continued...)

EHL