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

Stephen Farrell <stephen.farrell@cs.tcd.ie> Sat, 21 January 2012 10:21 UTC

Return-Path: <stephen.farrell@cs.tcd.ie>
X-Original-To: oauth@ietfa.amsl.com
Delivered-To: oauth@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E1DC621F85D4 for <oauth@ietfa.amsl.com>; Sat, 21 Jan 2012 02:21:09 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -101.203
X-Spam-Level:
X-Spam-Status: No, score=-101.203 tagged_above=-999 required=5 tests=[BAYES_00=-2.599, MIME_QP_LONG_LINE=1.396, USER_IN_WHITELIST=-100]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id TW4jyE1bEz9g for <oauth@ietfa.amsl.com>; Sat, 21 Jan 2012 02:21:05 -0800 (PST)
Received: from scss.tcd.ie (hermes.cs.tcd.ie [IPv6:2001:770:10:200:889f:cdff:fe8d:ccd2]) by ietfa.amsl.com (Postfix) with ESMTP id 3C58721F85BD for <oauth@ietf.org>; Sat, 21 Jan 2012 02:21:04 -0800 (PST)
Received: from localhost (localhost [127.0.0.1]) by hermes.scss.tcd.ie (Postfix) with ESMTP id 760A5171C24; Sat, 21 Jan 2012 10:21:02 +0000 (GMT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cs.tcd.ie; h=date :subject:from:x-mailer:message-id:content-type :content-transfer-encoding:mime-version:in-reply-to:references :received:received:x-virus-scanned; s=cs; t=1327141261; bh=iilvb ekD1LlGKHrDFmK+fhqW/2jWE1PvAm1IFvMNXe8=; b=NgNbb8N6nJd5ezFouQny8 J1/1Wd4ihenbNL66ttrO3u68bfi2n5S6MGpjtLYx5i5IKG68O4qPe+/VfJ1gfAuU yEF5AyPvNlZekplGJSIATfLTRtwL3lOaajIIKG2f21m3c/1PXCo/0UKh/2kYqbG7 B9uj2q5EVMxvyxpdcHuKUBwgcsl2ek+scLfOmU+DFuOYEQ/3fci4ZqBbxvMAjwZS zNG32ZJ1XlpXQ5g7uRjBvFaNsPZX0zm4oOaswBK8d6r5+3rxxAD1fKCvpD7GJOi2 UTMcr/r6/kAQ9ToUfgW6xBpWJbpanpLmdtBc9jMbKqrLQORDMdip4dECCZ/5fTr+ Q==
X-Virus-Scanned: Debian amavisd-new at scss.tcd.ie
Received: from scss.tcd.ie ([127.0.0.1]) by localhost (scss.tcd.ie [127.0.0.1]) (amavisd-new, port 10027) with ESMTP id beslv9EXGgaD; Sat, 21 Jan 2012 10:21:01 +0000 (GMT)
Received: from [10.87.48.5] (unknown [86.46.23.151]) by smtp.scss.tcd.ie (Postfix) with ESMTPSA id 65618171C23; Sat, 21 Jan 2012 10:21:00 +0000 (GMT)
References: <90C41DD21FB7C64BB94121FBBC2E723453AAB96548@P3PW5EX1MB01.EX1.SECURESERVER.NET>
In-Reply-To: <90C41DD21FB7C64BB94121FBBC2E723453AAB96548@P3PW5EX1MB01.EX1.SECURESERVER.NET>
Mime-Version: 1.0 (1.0)
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=us-ascii
Message-Id: <B3745DE3-3397-4B6F-A598-C0193076DA5C@cs.tcd.ie>
X-Mailer: iPhone Mail (9A405)
From: Stephen Farrell <stephen.farrell@cs.tcd.ie>
Date: Sat, 21 Jan 2012 10:20:58 +0000
To: Eran Hammer <eran@hueniverse.com>
Cc: OAuth WG <oauth@ietf.org>
Subject: Re: [OAUTH-WG] AD Review of -22 (part III)
X-BeenThere: oauth@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: OAUTH WG <oauth.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/oauth>, <mailto:oauth-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/oauth>
List-Post: <mailto:oauth@ietf.org>
List-Help: <mailto:oauth-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/oauth>, <mailto:oauth-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 21 Jan 2012 10:21:10 -0000

As before,
Thanks
S

On 21 Jan 2012, at 02:53, Eran Hammer <eran@hueniverse.com> wrote:

>> -----Original Message-----
>> From: oauth-bounces@ietf.org [mailto:oauth-bounces@ietf.org] 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>