Last Call Review of draft-ietf-oauth-native-apps-10
review-ietf-oauth-native-apps-10-genart-lc-davies-2017-05-15-00

Request Review of draft-ietf-oauth-native-apps
Requested rev. no specific revision (document currently at 12)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2017-05-16
Requested 2017-05-02
Other Reviews Secdir Last Call review of -11 by Donald Eastlake (diff)
Opsdir Last Call review of -11 by Zitao Wang (diff)
Genart Telechat review of -11 by Elwyn Davies (diff)
Review State Completed
Reviewer Elwyn Davies
Review review-ietf-oauth-native-apps-10-genart-lc-davies-2017-05-15
Posted at https://mailarchive.ietf.org/arch/msg/gen-art/_AaK7moxWvdKZgJPuczLnTq3mWE
Reviewed rev. 10 (document currently at 12)
Review result Almost Ready
Last updated 2017-05-15

Review
review-ietf-oauth-native-apps-10-genart-lc-davies-2017-05-15

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.

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.

Nits/editorial comments: 

General: s/i.e./i.e.,/ (3 places)

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

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 

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.

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

s4, last para: s/emcompasses/encompasses/

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

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.

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

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

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

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

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

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

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

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

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.

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

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?]

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

s8.8; need to expand CSRF (Cross Site Request Forgery) and maybe explain a it how CSRGF and the cross-app case are related