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

[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

> Original list of nits:
> ----------------------
> 
> - Intro: Maybe add some ascii-art showing the roles of the user, browser,
> client etc. The term client as used here is still commonly confusing and
> especially worth going the extra mile to clarify, before it is first used. What I
> think needs to be said early is that what is here called a client is normally
> (running on) a web server.

Happy to take suggestions, but can't come up with a useful diagram myself.

Added to the client definition:

              The term client does not imply any particular implementation
              characteristics (e.g. whether the application executes on a server, a desktop, or
              other devices).

> - p4: Maybe s/a string/an octet string/ in the token descriptions, unless the
> access token encoding is constrained to what'd be understood as a string.

Just a string.

> - p8: Seems odd to speak of "issuing" an implicit grant - wouldn't that make
> something explicit? Maybe say "With an implicit grant..."
> instead in the 2nd para of 1.3.2?

Changed to 'When issuing an access token during the implicit grant flow'.

> - p8: I don't get what "its device operating system" means. Do you mean the
> client is an already-trusted part of the resource owner's device OS?

Changed to 'the client is part of the device operating system'.
 
> - p9 - "Issuing a refresh token is optional" - might be better to say that it's the
> authorization server's choice and there's no way for the client to signal a
> request for one.

Changed to 'Issuing a refresh token is optional at the discretion of the authorization server.'

> - p10 - In figure 2, why is the Refresh token shown as optional in step (H) but
> not in step (B)? I think it'd be clearer for the figure to reflect the protocol
> options and say that the refresh token is optional in (B) as well.

Because this is the refresh token flow. If it is optional, there is no flow...

> - p12 - the confidential/public terminology isn't great, did the WG consider
> "authenticating" vs. "non-authenticating"? Trying again to find better terms
> would maybe be worthwhile.

Didn't try this particular alternative, but it doesn't flow off my tongue. 

> Also, it may be worth explicitly saying that it
> doesn't matter how hard to guess a secret the client has nor how good a
> MAC algorithm you use with that secret - if anyone can get the secret then
> the client is still public. It nearly says that, but not quite and given that many
> developers just don't apparently still think that a hardcoded secret
> embedded into a binary is good enough, I'd argue its worth adding a bit more
> here.

This seems to be cross the line as to what the server defines as confidential, but I'm happy to take text proposals.

> - p12/13 in the application profile descriptions - is "resource owner's device"
> correct? That seems to rule out kiosks, which may be correct, but which isn't
> obvious. Maybe you mean "device being used by the resource owner" with
> no implication as to ownership of the device?

Changed to 'the device used by the resource owner' throughout.

> - p13 - client application: typos:
> 
> s/such access tokens/such as access tokens/
> 
> s/which...interact with/with which the application may interact/

Ok.

> - p13, "establishing trust with the client or its developer" is badly phrased, I
> think you mean the AS SHOULD NOT just accept the client type unless it has
> some external reason to do so. Trusting the developer might be one such.
> Being paid is another, and maybe more likely;-)

Changed to:

          The authorization server SHOULD NOT make assumptions about the client type, nor accept the
          type information provided by the client developer without first establishing trust.

> - p13, 2.3 has 2119 language both about the things established at registration
> time and things done in the protocol defined here - would it be better to
> identify those separately in different sections or maybe move the
> registration time stuff into 2.2 with a possible renaming of 2.2.

Tweaked a bit, but overall it reads fine to me.

> - 3.1.2.1 has a SHOULD for TLS which I think generated a lot of mail on the list.
> I think saying why this is not a MUST would be useful, since it's the kind of
> thing that might get revised later on if the situation changes.

              This specification does not mandate the use of TLS because at the
              time of this writing, requiring clients to deploy TLS is a significant hurdle for most
              client developers.

> - Figure 3, (p21) has multiple lines labeled A,B & C - saying why might reduce
> some confusion. Or maybe, say that the labels reflect rough event timing or
> something. It'd also be good if subsequent sections said which parts of figure
> 3 they're addressing, e.g.
> 4.1.1 maps to (A), right? It's hard to tell.

Added clarification about the broken lines.

> -p25, 4.1.3, what does "assigned" "authentication requirements"
> mean? Suggest deleting the parenthetical clause since "confidential client"
> should be sufficiently well-defined to cover that.

Ok.

> -p28, the description of step (D) isn't clear to me - who does what with the
> URI fragment?

Seems pretty clear to me.

> -p30, why refer to "HTTP client implementations" when you're previously
> said UA? If there's a substantive difference it'd be good to be clear about
> that. Same para: s/such client/such clients/

Changed to UA.

> -p33 - Just checking - 4.3.1 says the client MUST discard the credentials once
> an access token has been obtained - why not before though, e.g. once an
> access token has been requested?  Is there a re-tx thing that the client might
> do?

Nope.

> -p38, is "token_type":"example" a valid value? If not, better to use one that
> is.

I refuse to use Bearer as an example and I expect others to take issue with using MAC :-)

> -p40, s/client it was issued/client to which it was issued/?'

Ok.

> -p40, s/require/REQUIRE/ in the 2nd last bullet on the page

Require isn't an uppercase word (REQUIRED is).

> -p43, s/native clients may require/native clients require/ I'd say the "may" is
> worth deleting both to avoid 2119 language and because we do know that
> these require special consideration.

Ok.

> -p46, s/such malicious clients/such potentially malicious clients/?
> Not all unauthenticated clients are bad, though all of them could be bad.

Ok.

> -p47, s/Access token (as well as.../Access tokens (as well as.../

Ok.

> -p50, 10.8 seems to just repeat stuff from earlier in 10.3 & 10.4, is that
> deliberate?

The requirements are but the context is different.

This concludes this review and all other -22 feedback.

EHL