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

[Stephen Farrell <[e-mail]>]

As before,
Thanks
S

On 21 Jan 2012, at 02:53, Eran Hammer <[e-mail]> wrote:

>> -----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
> <winmail.dat>