Skip to main content

Last Call Review of draft-ietf-oauth-security-topics-26
review-ietf-oauth-security-topics-26-secdir-lc-jones-2024-04-28-00

Request Review of draft-ietf-oauth-security-topics
Requested revision No specific revision (document currently at 27)
Type Last Call Review
Team Security Area Directorate (secdir)
Deadline 2024-02-22
Requested 2024-02-08
Authors Torsten Lodderstedt , John Bradley , Andrey Labunets , Daniel Fett
I-D last updated 2024-04-28
Completed reviews Secdir Telechat review of -27 by Michael B. Jones
Artart Last Call review of -25 by Russ Housley (diff)
Secdir Last Call review of -26 by Michael B. Jones (diff)
Genart Last Call review of -25 by Thomas Fossati (diff)
Artart Telechat review of -26 by Russ Housley (diff)
Assignment Reviewer Michael B. Jones
State Completed
Request Last Call review on draft-ietf-oauth-security-topics by Security Area Directorate Assigned
Posted at https://mailarchive.ietf.org/arch/msg/secdir/DlHGcmXYj2EOV-J93lYddHKdN2U
Reviewed revision 26 (document currently at 27)
Result Has issues
Completed 2024-04-28
review-ietf-oauth-security-topics-26-secdir-lc-jones-2024-04-28-00
Comments on substantive issues with the specification:

In 4.15. Client Impersonating Resource Owner, the sentence “If the resource
server cannot properly distinguish between access tokens issued to clients and
access tokens issued to end-users, the client may then be able to access
resource of the end-user.” has two problems.  (1) End-users are people. 
Therefore access tokens are not issued to them.  Please rework so this
paragraph means what you actually intend.  (2) I suspect that you want to
change “the client may then be able to access resource of the end-user” to
something like “the client may then be able to access resources belonging to
the end-user”.

In 4.15.1. Countermeasures, it talks about “the resource server to be unable to
distinguish an access token authorized by a resource owner from an access token
authorized by a client itself”.  I’m sure I don’t understand the distinction or
the scenarios here, and if I don’t, others likely won’t either.  This is
probably related to the confused language cited in the previous paragraph. In
my maybe overly simplistic view, all access tokens are authorized by
authorization servers and issued to clients.  Clients don’t authorize access
tokens.  Please rework these paragraphs so that the distinctions you’re trying
to make will be clear to readers.

Other, mostly editorial, review comments:

In the Abstract, change “It further deprecates some modes of operation” to
“Further, it deprecates some modes of operation”.

In 1. Introduction add “the” before “client” in “OAuth initially assumed static
relationships between client”.

The Introduction says “OAuth 2.1, under developement as [I-D.ietf-oauth-v2-1],
will incorporate the security recommendations of this document.”  Is the plan
for it to incorporate all the security recommendations in this document
verbatim (possibly rendering this document largely obsolete)?  And all the ones
in [RFC6819]?  Or might it be more accurate to say “will incorporate security
recommendations from this document”?  I’d suggest the latter.  Also, correct
“developement” to “development”.

In 2. Best Practices, change “considers best practices” to “considers to be
best practices”.

In 2.1. Protecting Redirect-Based Flows, I’d suggest changing “, see Section
4.1.3” to “(see Section 4.1.3)” to be consistent with the syntax used in the
following sentences.  That, or change it to “, per Section 4.1.3”.

In 2.1. Protecting Redirect-Based Flows, change “(open redirector)” to “(open
redirectors)”.

In 2.1.2. Implicit Grant, change “aka” to “a.k.a.,”.

In 2.6. Other Recommendations, change “However, CORS MUST NOT be supported at
the Authorization Endpoint as the client does not access this endpoint
directly, instead, the client redirects the user agent to it.” to “However,
CORS MUST NOT be supported at the Authorization Endpoint, as the client does
not access this endpoint directly; instead, the client redirects the user agent
to it.”

In 3. The Updated OAuth 2.0 Attacker Model, change “as good as practically
possible” to “as well as practically possible”.

In 3. The Updated OAuth 2.0 Attacker Model, change “See also Section 4.9.2.” to
“Also see Section 4.9.2.”.

In 4.1.1. Redirect URI Validation Attacks on Authorization Code Grant, in “For
a client using the grant type code”, put backquotes around “code”.

In 4.2.1. Leakage from the OAuth Client, change “ (and potentially access
token)” to “ (and potentially access_token)”.

In 4.5.3.1. PKCE, this sentence is awkward “PKCE is a deployed OAuth feature,
although its originally intended use was solely focused on securing native
apps, not the broader use recommended by this document.” and verges on being a
run-on sentence.  Please update.

In 4.5.3.2. Nonce, change “The nonce value is a one-time use” to “The nonce
value is one-time use”.

In 4.5.3.2. Nonce, the meaning of “where the attacker has stolen the respective
authorization code” in “The assumption is that an attacker cannot get hold of
the user agent state on the victim's device, where the attacker has stolen the
respective authorization code.” isn't clear.  This sounds like the attacker
stole the code from the user agent state, but that’s probably not what’s meant.
 Please clarify.

In 4.5.3.3. Other Solutions, this sentence is missing two commas “PKCE is the
most obvious solution for OAuth clients as it is available today (originally
intended for OAuth native apps) whereas nonce is appropriate for OpenID Connect
clients.”

In 4.9.3. Countermeasures, this sentence leaves me wanting more: “Depending on
the severity of the penetration, sender-constrained access tokens will also
prevent replay on the compromised system.”  In particularly, please
characterize when sender-constrained access tokens will and will not prevent
replay on the compromised system.

In 4.10.1. Sender-Constrained Access Tokens, change “The approach as specified
in this document” to “The approach specified in this document”.

In 4.10.2. Audience-Restricted Access Tokens, change “the fingerprint of the
resource server's X.509 certificate as audience value” to “the fingerprint of
the resource server's X.509 certificate as the audience value”.

In 4.10.2. Audience-Restricted Access Tokens, change “crypto” to “cryptography”.

In 4.10.2. Audience-Restricted Access Tokens, given the effective demise of
Token Binding, I suggest changing “[I-D.ietf-oauth-token-binding] has the same
property since different token-binding IDs must be associated with the access
token.” to “[I-D.ietf-oauth-token-binding] had the same property since
different token-binding IDs would be associated with the access token.”

In 4.11.2. Authorization Server as Open Redirector, put backquotes around
“prompt=none”.

In 4.14. Refresh Token Protection, the sentence “Refresh tokens are a
convenient and user-friendly way to obtain new access tokens after the
expiration of access tokens.” is a somewhat inaccurate characterization, since
refresh tokens can also be used to obtain new access tokens before the
expiration of access tokens.  Please revise.

In 4.14.1. Discussion, the description “overall grant”  seemed like it could be
improved.  I don’t think the word “overall” conveys all that you mean readers
to understand about this scenario.  Please revise.

In 4.17. Authorization Server Redirecting to Phishing Site, put backquotes
around “prompt=none”.

There’s a lot of duplicated text between 4.11.2. Authorization Server as Open
Redirector and 4.17. Authorization Server Redirecting to Phishing Site. 
Consider refactoring to eliminate or reduce the duplication.

Update [OpenID.Core] to OpenID Connect Core 1.0 incorporating errata set 2,
December 15, 2023.

Update [OpenID.Discovery] to OpenID Connect Discovery 1.0 incorporating errata
set 2, December 15, 2023.

Update [W3C.WebAuthn] to reference
https://www.w3.org/TR/2021/REC-webauthn-2-20210408/ (including updating
editors, date, etc.).

Change to <?rfc tocdepth="4" ?> (or whatever its equivalent is in Markdown) so
that sections such as 4.5.3.1. PKCE appear in the table of contents.