Re: [OAUTH-WG] Genart last call review of draft-ietf-oauth-native-apps-10

William Denniss <wdenniss@google.com> Fri, 19 May 2017 20:04 UTC

Return-Path: <wdenniss@google.com>
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 D4966129AB6 for <oauth@ietfa.amsl.com>; Fri, 19 May 2017 13:04:10 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.701
X-Spam-Level:
X-Spam-Status: No, score=-2.701 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RP_MATCHES_RCVD=-0.001, SPF_PASS=-0.001] autolearn=unavailable autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=google.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id N-tEwwKHb5xc for <oauth@ietfa.amsl.com>; Fri, 19 May 2017 13:04:07 -0700 (PDT)
Received: from mail-io0-x22d.google.com (mail-io0-x22d.google.com [IPv6:2607:f8b0:4001:c06::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 116A11294AB for <oauth@ietf.org>; Fri, 19 May 2017 13:04:06 -0700 (PDT)
Received: by mail-io0-x22d.google.com with SMTP id o12so53889852iod.3 for <oauth@ietf.org>; Fri, 19 May 2017 13:04:06 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=DIxZijMyoosEBEnHAb1cHOE1AsjPqASahl8Rbj8RQ4A=; b=eemJyYjZYqgg/TNT9am0JhnOyxmz8/RWKf4TjTavffIoW1fgPndpX0603zHamsvKVh Pvw306xN/jGz74WTbiQDe+qMjU9k7ahJScAbVZxyghVB9Ml4txZ3ufYKAHQ0nsNeCIa1 29zSZLzThrQJ0ucwmR9f2fIsu7/2gZW8SlnnlA2gwFjn6U9nu0SD/u3vniZ4CEdg7LEX Bdt3IXgpfcOa6RXqqqp0w8WvJRZ1zVKCwHmeuzzabEV9rCxYy8rsrB7WSnlIgN5IXjF1 nxLOzF0PgvTeKijy2t5nC53FbEy9/5psT3mjkpcN8+knWsEH218LcCWoJ8O3NLFS/9LB mAJQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=DIxZijMyoosEBEnHAb1cHOE1AsjPqASahl8Rbj8RQ4A=; b=PWPMSlr+40+1jBd0KQnqLEbWJXWqTKkY+9QEgktuu8AxzHuDH0OX0hOG1qbxn/ycD9 8B3VwNPgS9F1j3jy43p+bU2XfFf5z6l9Aohn0RBqoMQlaStw3V5rLtav0+o5VcUMKxsi GkQwfpDrQaJjf8TB2TvEWrXYxkc5/tNAZsqWc6ib9Jz+qNjCGhA1DBqCJy29fKip/vAU vOpyWhXGziZfqrtCjPug2IrL1P9rpd/ghPrAB0NeTgyH3M4kHxJN+OS0neah3nRwYJX3 E716UdAOQUOs/wnClGEBntgeuXiv6q5PTsBMqJ5NC3aBYt4bex7RLTGktcxWkRn0K/k9 3A5g==
X-Gm-Message-State: AODbwcCRti4e2FerOs7XkDIKJjIT1kixoYIIbf0/ms4uYKmZ1MhZFWvY pi6pm9Uv5B6h9q/8T3MeM0jdOtKtYyyI
X-Received: by 10.107.25.203 with SMTP id 194mr11591604ioz.182.1495224245104; Fri, 19 May 2017 13:04:05 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.79.35.37 with HTTP; Fri, 19 May 2017 13:03:44 -0700 (PDT)
In-Reply-To: <149489171188.11868.13336890952697460283@ietfa.amsl.com>
References: <149489171188.11868.13336890952697460283@ietfa.amsl.com>
From: William Denniss <wdenniss@google.com>
Date: Fri, 19 May 2017 13:03:44 -0700
Message-ID: <CAAP42hBvma8ZijjQ3+88-pqv7EObHtJcF-cn91zbFhSG9KzFew@mail.gmail.com>
To: Elwyn Davies <elwynd@dial.pipex.com>
Cc: gen-art@ietf.org, draft-ietf-oauth-native-apps.all@ietf.org, ietf@ietf.org, "oauth@ietf.org" <oauth@ietf.org>
Content-Type: multipart/alternative; boundary="001a113fe2dafe2c0d054fe6056b"
Archived-At: <https://mailarchive.ietf.org/arch/msg/oauth/rYcU6gPSDQX0kirJo149WB9-s7g>
Subject: Re: [OAUTH-WG] Genart last call review of draft-ietf-oauth-native-apps-10
X-BeenThere: oauth@ietf.org
X-Mailman-Version: 2.1.22
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: <https://mailarchive.ietf.org/arch/browse/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: Fri, 19 May 2017 20:04:11 -0000

Elwyn,

Thank you for your detailed review.

Comments inline, my changes appear in version 11
<https://datatracker.ietf.org/doc/html/draft-ietf-oauth-native-apps-11>.

On Mon, May 15, 2017 at 4:41 PM, Elwyn Davies <elwynd@dial.pipex.com> wrote:

> Reviewer: Elwyn Davies
> Review result: Almost Ready
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-oauth-native-apps-10
> Reviewer: Elwyn Davies
> Review Date: 2017-05-15
> IETF LC End Date: 2017-05-16
> IESG Telechat date: 2017-05-25
>
> Summary: Almost ready.  A couple of simple minor issues could do with
> addressing.
>
> Major issues:
> None.
>
> Minor issues:
> s3: "browser":  The browser that acts as the Oauth user-agent is
> conflated with the user's choice of default browser.  Firstly this is
> not something that is discussed in RFC 6749.  Secondly, the concept of
> 'default browser' would normally be thought of by users as the browser
> that is used to display the content associated with hyperlinks rather
> than providing Oauth services.  I suggest that the implication in the
> body of the draft that 'the browser' is the user selected or system
> selected default browser needs to be at least discussed explicitly
> rather than buried in the terminology definitions in s3.  I wonder
> whether ths connection is something that should be made by a separate
> OS setting or a setting in each native app rather than conflated with
> the default browser.  The term "designated browser" might be useful.
> In all cases there might be secuity implications if a bad actor could
> subvert the designated browser setting.
>

I reworded the definition, please take another look.

s8.1, Requirement of use of PKCE in some cases:  This strict
> requirement really needs to be introduced in the body of the
> discussion rather than buried in the seurity considerations.
>

I reworked Section 6 to include this requirement there.

Nits/editorial comments:
>
> General: s/i.e./i.e.,/ (3 places)
>

Done.

Title and Abstract: s/apps/applications/g  (uses before we get to
> terminology in s3)
>

After consulting some dictionaries, I'm convinced that app is a bonafide
synonym for application, and not shorthand (even if it started out that
way). So I've reworded our definition, but am otherwise planning to retain
the use of "app". The primary use-case for this BCP are "apps", I believe
replacing this usage would introduce more confusion that it could
potentially solve.

s1, para 1:  Suggest the following to make it clear that the
> definition is in RFC 6749 rather than here.
> OLD:
>  The OAuth 2.0 [RFC6749] authorization framework documents two
> approaches in Section 9 for native apps to interact with the
> authorization endpoint: an embedded user-agent, and an external user-
> agent.
> NEW:
> The OAuth 2.0 [RFC6749] authorization framework defines "native
> applications" in Section 9 of RFC 6749 (see also Section 3 below) and
> documents two approaches by whch they can interact with the
> authorization endpoint: an embedded user-agent, and an external
> user-agent.
> END
>

I feel like this is implied due to the reference to Section 9 of 6749 in
the opening sentence.

s1, para 2: s/apps/applications/(2places) .  For second case: s/native
> apps/native applications (shortened to "native apps" or just "apps"
> hereafter)/
>
> s3, "native app": s/app/application/g in the definition.  After that
> in the document "[native] app" is fine except for the definitions
> mentioned in the next comment. Worth repeating the link to Section 9
> of RFC 6749.
>
> s3, All definitions after "app"; s/app/application/g in the
> definitions as these are not restricted to (native) apps as defined
> here.
>

Per earlier comment.

s3, "embedded user-agent": s/modify/modifyng/
>

Done.

s4, last para: s/emcompasses/encompasses/
>

Done.


> s4, last para: s/inter-process/inter-app/ (since this term is
> defined)
>

Done.


> s4, last para: Might be worth pointing to the 'SHOULD' about client
> type assumptions in s2.1 of RFC 6749 withe reference to servers that
> do make assumptions.
>

This is covered in detail in Section 8.4. I'd like to keep this
introduction light, and not get into the details just yet.  Let me know if
you think 8.4 is inadequate.

s4.1, para below figure 1: s/system browser/browser/ (or maybe
> "designated browser").
>

Reworded.

s5, paras 1 and  2: Reword to clarify and remove 'we gain' usage (not
> allowed in RFCs):
> OLD:
>    Just as URIs are used for OAuth 2.0 [RFC6749] on the web to
> initiate
>    the authorization request and return the authorization response to
>    the requesting website, URIs can be used by native apps to
> initiate
>    the authorization request in the device's browser and return the
>    response to the requesting native app.
>
>    By applying the same principles from the web to native apps, we
> gain
>    benefits seen on the web, like the usability of a single sign-on
>    session and the security of a separate authentication context.  It
>    also reduces the implementation complexity by reusing similar
> flows
>    as the web, and increases interoperability by relying on
> standards-
>    based web flows that are not specific to a particular platform.
> NEW:
>    Just as URIs are used for OAuth 2.0 [RFC6749] in the HTTP protocol
> on the web to initiate
>    the authorization request and return the authorization response to
>    the requesting website, URIs can be used by native apps to
> initiate
>    the authorization request in the device's browser and return the
>    response to the requesting native app.
>
>    By extending the techniques from the web to native apps, the
>    benefits gained in the web context will also be reaped when using
>    OAuth with native apps; benefits include the usability of a single
> sign-on
>    session and the security of a separate authentication context.  Use
> of
>    the techniques also reduces implementation complexity by reusing
> similar flows
>    to those employed on the web, and increases interoperability by
> relying on standards-
>    based web flows that are not specific to a particular platform.
> END
>

Re-worded to remove "we gain", thanks for your suggestion.

s5, para 3: Suggest prefixing this para with: "To conform to this best
> practise," - the MUST is not derived from RFC 6749.
>

Re-worded taking into account this suggestion.

s7.1, last para: s/URI like it/URI as it/; s/like normal/as it would
> normally/
>

Done.


> s7.2, next to last para:
> OLD:
> Due to this reason, they SHOULD be used over the other redirect
> choices for native apps where possible.
> NEW:
> For this reason, they SHOULD be used in preference to the other
> redirect options for native apps where possible.
> END
>

Done.

s7.2, last para: s/it REQUIRED/it is REQUIRED/
>

Done.

s8.1, para 2: Need to expand acronym PKCE at first use (currently
> expanded in para 4).
>

PKCE now defined earlier in the doc.


> s8.1, para 4: s/sends data/send data/
>

Done.

s8.2: It would be more consistent with RFC 6749 to refer to "Implicit
> Flow" as "Implicit Grant authorization flow" at least for the title
> and first occurrence.   The second and third occurrences in para 1
> should s/Implicit Flow/implicit flow/ for consistency with para 2.
>

Done.


> s8.2, para 2: s/code flow/Authorization Code Grant flow/
>

Done.

s8.3, last para: Is a reminder to choose the 'right' type of IP
> literal (IPv4 or v6) desirable?  Doing an address lookup on
> "localhost" presumably tells you which one to use! [perhaps?]
>

Good point, but I think this is an implementation consideration. I've added
a note to 7.3.

s8.7, para 4: s/like/such as/
>

Done.


> s8.8; need to expand CSRF (Cross Site Request Forgery)


Done.


> and maybe
> explain a it how CSRGF and the cross-app case are related
>

Great point, done.