Elwyn,
Thank you for your detailed review.
Comments inline, my changes appear in version 11.
On Mon, May 15, 2017 at 4:41 PM, Elwyn Davies <elwynd@xxxxxxxxxxxxxx> 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.