[OAUTH-WG] Comments on draft-ietf-oauth-v2-1-00 (The OAuth 2.1 Authorization Framework)

Karsten Meyer zu Selhausen <karsten.meyerzuselhausen@hackmanit.de> Thu, 06 August 2020 10:48 UTC

Return-Path: <karsten.meyerzuselhausen@hackmanit.de>
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 8038E3A101E for <oauth@ietfa.amsl.com>; Thu, 6 Aug 2020 03:48:42 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.898
X-Spam-Level:
X-Spam-Status: No, score=-1.898 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=hackmanit-de.20150623.gappssmtp.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 Y12cc-9WNglM for <oauth@ietfa.amsl.com>; Thu, 6 Aug 2020 03:48:39 -0700 (PDT)
Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) (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 CB6693A101B for <oauth@ietf.org>; Thu, 6 Aug 2020 03:48:36 -0700 (PDT)
Received: by mail-ed1-x52b.google.com with SMTP id v22so24015314edy.0 for <oauth@ietf.org>; Thu, 06 Aug 2020 03:48:36 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hackmanit-de.20150623.gappssmtp.com; s=20150623; h=cc:from:subject:autocrypt:to:message-id:date:user-agent :mime-version:content-language; bh=Cz11XR0sdbBgSgtPiYn4AvTL+aCJEPfhBDp7/E9gqIA=; b=VkJctib3fZoyyWy67YB/AKOD4BBdSHbNg0rev62xT6TIuL9c3CFnwHfRT2dKgM/fqZ yDvwt/VLrOAiNlV9a8j55vl5QBuIGxraiwH096peRE5g+Vxr0u+QtSQvjY7xlsVvGUb0 MZpRBDFaBuv74iOoKyddWDbpuvItb6+ocS6cY7k0fQSeRvwRVlyMvRhRgoggl9BIYGSy zo96xWBYkQMl5OV2SBNCZ9RaA4wxF1wlyH6Cusy01dQxirnE7maIuGC2U/QdX8/N5E/b cMXTNtsx3Y+pRJBheLGckwHloWjLDGOIVV3k7A7u6LX0Oni7HE9bmS7Z2kuWXKHbUBDk YzrA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:cc:from:subject:autocrypt:to:message-id:date :user-agent:mime-version:content-language; bh=Cz11XR0sdbBgSgtPiYn4AvTL+aCJEPfhBDp7/E9gqIA=; b=py4nN7Goqzf32QGK4lYh/xgjgLLlY26Y53WtgyJ6xIw0zrrEkTzycITGskxDqfkU6k 53S+z+Hkqt8ziDmmcT92fHlREdoKGMzDTXqFUGlhx8kf3oEdg/2FpfnoCx2WCaAiEzYX u2qC5tK1B6gagoCUBNrAycTZCgJTlmUeJX6u6LPQdntOH+5d0Alerh5shcCq+yKtNnVZ Oi7eD3sptlEbDqexk43sZZCtJfjcDdmNene0HEWhc23SbhaHhS60HJlC0VOLiOiq0l7Z f4t7PzeLuQDMTGozfFdY0ETI1NYTopUO6LndAedgd7gjBD9lb++wLUY5na9D2kkpQsKw IIDw==
X-Gm-Message-State: AOAM531iBHBeuE6N9DRT3QAj7gsTRR0DJTS11fAUZgXzcUIz4zFKeuGq Fu7sqAjO8oxqjWEq6OWxcsn6EPOHZm0=
X-Google-Smtp-Source: ABdhPJxeheUKNCQbWlibV+bpuuegUs2vKx8BeH2gfY1E7PYSyoOIutTduTbiEgK0d5RorGoN1vKm7Q==
X-Received: by 2002:aa7:d9d7:: with SMTP id v23mr3628770eds.112.1596710914731; Thu, 06 Aug 2020 03:48:34 -0700 (PDT)
Received: from [192.168.178.22] (b2b-37-24-87-133.unitymedia.biz. [37.24.87.133]) by smtp.gmail.com with ESMTPSA id e8sm3138925edy.68.2020.08.06.03.48.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Aug 2020 03:48:33 -0700 (PDT)
Cc: Christian Mainka <christian.mainka@hackmanit.de>
From: Karsten Meyer zu Selhausen <karsten.meyerzuselhausen@hackmanit.de>
Autocrypt: addr=karsten.meyerzuselhausen@hackmanit.de; keydata= mQINBFh1IBMBEADV73c10lB7zeFy6/ezLFzOBp8z6Zy1zUyIrf6RoBk1GQWREcGEGeaL90Pj F5plZeASVJdsEYnYXdgcIPE0tlBq6al6OYoWtH/VbFPWEPLVhA3rL1iXVJveD3J40OzSYP8N G7bla3zQ2+TXOB3iDPPsHZUdHCLASkIIWQK6+fE1C2epAdPtnsLsb++1d080jfXXwgyUUh4y bimcy9Jg5oZ4QMwnSq3Y+x38PNb+nTgjDi1X/89/WsNd7Bdh4Zvw3CAuc/W58CFaDjb7liUD YRoAp6ysnjPKEUSnAnMpgaiXJc1gFoL+ahdKJ3D9XTn28NTjUrvOkVidsuKbyxnXP9I6BO6i 2jzjrH6TEAfIYMjZlYTyPZTt271SW5iAHYwvPZWlqQTBT2P/d4gHl0To5b4e+UXxjQgxqUyi QIcxh3Ris21Kx4lKQKDXYWiwNTZzx8AdqrcxCWfK+MRpFyk0B+4uDMm7Apm5ZWwDKN/JnVsJ yokkkrrHs/elRCUGtN9NyhJQf3VnE87862Pej8PVvQJr3uVnoNX2yieTvJZftIOBG1b9ta6Z BcYyn3un1rSn7lBPg+RSnPemposVorQpjGwT+Dhg13Bpv5q0JfSc//js/nB6A4iq5YssdtQ7 35QBWLLaF1oCxalvrQVDD4Sh06eAUQsga9xeE0yv7sxqdsozdwARAQABtEJLYXJzdGVuIE1l eWVyIHp1IFNlbGhhdXNlbiA8a2Fyc3Rlbi5tZXllcnp1c2VsaGF1c2VuQGhhY2ttYW5pdC5k ZT6JAj8EEwEIACkFAlh1IBMCGyMFCQlmAYAHCwkIBwMCAQYVCAIJCgsEFgIDAQIeAQIXgAAK CRBFNcDn2xbxSK7sEAC5hk0VQHo2+fMV3b4TgSt4qSPLz6EnWwoqcEzUGYHErQXy7tCENpqS rsZsFphpgvWo1tcQdpyQTFm0dry4ASJD78lEiYC/8Hedp0fIaJTGwxrSLpRxV/Wb+iqkbgz8 /Qydl3QyupSqznSHQMd0uhvzHLxoYvHAIKy52gCK0T9gmxcCIh7UEjDfm+kqHp+oU4sbNe+2 ZEtJLuCKW+amNyqnHXr7ehAIaYmTdKOEcUb2UM7Yzp9g4kSkg1GbPlAn6yjyAqJ96sobKFXX S3rkXksRTxkGKW278Nrs4UBO+OIu32kIXCM2m3fKaUK777jAQu1e8sdj2nL0sPWQvMikZRx6 0dy+wVuH8gGHZsd7rW201Sv5pAhSAK4l58GS3xSLId6smXCend9Vu+tcYA+Bb+45943LmoPA PrdIUeI+zC9pjGwm+x+jFiCxbChqAiJF7RyYv9crziEYnTQ70gHGNOTOTIS5t0ufc9D4wD4O IkkrPQYg3KcAqP2Kyj1uHcqdk7XEhV1fdTXdeEt1e7auWPh0d3Fo+BTtiGXfNMuORArE0El6 ky8eUOqZEJ8rYpEGDLt0JFkJM5AhX4PrQWekjaMhQ5yl/+M+Ss0V0JkImagSgWdvUn1+eAs0 zEuVxTc6ON69mIyMalQ5d4ofvPnKr3GNVmEiXAVDMGUZHoeabfgSBbkCDQRYdSATARAAsp2V mr3N7iNND8+M/OyA/OwcDQ6utZh+m4TnKsOVdiNLGpu2U3/2Qg3yrbjic2dWx1CsS6VH2/oO 1e/a4FlxA93wFv/OZjiUjHtEvdIJeHWlCvWOUlMsqyGDc3Q75fNjFw6DGKkiOu9lZaBs6naS BmkvAMGjV5bNKLyIL5j7Im1pCdZ2lCjD7eVwR3RQQKobTmu916htX8g1cB9yFmquu37X+ZBl A4GLJi63Kw0L2r8i8iO1NqDLOfT8IeNkOroEm3SDAuEApGAubKLSPBJ1khQ7kDhpdfzSYKUF tiIHpGWVOImDjqf4JIcF7OIdRPQfFPlwoPnsyBAS8znQJvmqbbMowgFZe3UMLAN78CETZHGM OLBPB873oWyZ07Ar4v/SL5/aD+FRj2VnYEcGwt0HMmMyaN6ed8Udj4OTNZ7ceZA1Tw8/lZuI KCamj0XfJIK6376RCGnqjsEfS65P1KWZXfWphCKWp2c7uWKtau1q8pgiVRoBSAmjvfXRrIvK LhhQyNOiCUDKrvEWpoeq9y5GTrY27ncLov8nSR/SUPOw5HwJmzdFjhOF9XAOtiND/QRH886O IohdlnUu668mwLCmL2ROe7XWcTkFQWLDg+5b0bC9dgfL+HHpWGUdQPG3CCyPG5LfDmnmuXkE eU1kSD27kFe1kM6pfqpCydJW66DuwoMAEQEAAYkCJQQYAQgADwUCWHUgEwIbDAUJCWYBgAAK CRBFNcDn2xbxSAAbEACeIsfrsq2tlyigZv+bwkiVP1oKtWfXN1e3K3lDOBqPJaPXWFOopq/1 9osk58PFtVEaDlYPlN/NP6Jq5nTTC8QyLG3swAdo4ZJXWEg1NTRu8ddYUvZWuRHWRghaq7qh eW5lVPqilCndSG7bkDPU/Vyd93nPKnKTKKs/Nd7ePneWA0JQohEg5gO/GU0v5SN3YfTxG1LV Cxu3HHHFodDLK4KITSYmt1+g0WCADeclwm5+L5lIvgKQvcIpjpMGNK1wj2E3exsLlgo/ZEyS AslOPXyQw2yfYLrcfGpvWa3e+AvU7eLVBgihskpibJg53yw31B0CXAJBbjg7AsxR8UE5pl6h 2gTjN2t++GvqefGtw/bPvx2RzFsorh1/RYaFgcaFyefghmpi55iiIhgEOiSIct0LoYl3cmH8 DGYKhSskpSDgfE41Esk/P2odeax9SmJuv4mnqkiGFPpTwCfUka2k0mCpBDpfTdECWUFhreGS qFbrvJDZRBiyaVyCjOvOc0v6Z0/iIRgHWTjITpqaQh69kqAtt9GQWV6i3THnpHFlIC2ecvdc YCagneZdoLEHCS8Nois/uDbp5qZwZcF5zKMI+T7u6Qf8EGdvxCB1fp0Sdlmeto0c6/gnFUix 4J/tozBwSXSg7JCxTrUdnJtcQAJzosOUZTVO/ZZR/n0+904kud6o3w==
To: oauth@ietf.org
Message-ID: <a5c9ee0e-5b41-c4d3-ebff-d4a7985b663f@hackmanit.de>
Date: Thu, 6 Aug 2020 12:48:31 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="------------C632C9223B4FCCE2A57344C0"
Content-Language: en-US
Archived-At: <https://mailarchive.ietf.org/arch/msg/oauth/eu3c9t99rqVHiqcsef4svwCjPrc>
Subject: [OAUTH-WG] Comments on draft-ietf-oauth-v2-1-00 (The OAuth 2.1 Authorization Framework)
X-BeenThere: oauth@ietf.org
X-Mailman-Version: 2.1.29
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: Thu, 06 Aug 2020 10:48:43 -0000

Hi all,

great to see that the OAuth 2.1 draft was adopted by the WG.
We appreciate the efforts to create a single document which contains the
current state of the art in terms of OAuth and all security best
practices very much. It will make it a lot easier for developers and
others to get started with OAuth.

Christian and I reviewed the current draft
(https://www.ietf.org/id/draft-ietf-oauth-v2-1-00.html) and would like
give some feedback.

You can find our comments below; they are sorted based on the section
they affect. I divided the comments into two parts: The first one
consists of suggestions and improvements to generally clarify the draft
and increase the security of deployments implementing OAuth 2.1. The
second part contains typos and other minor mistakes. There should be no
need for further discussion on the list regarding these comments.

We are happy to hear your thoughts on our suggestions.

Best regards,
Karsten

Comments:
Part 1: Suggestions and Improvements

Section 1. Introduction
- 1.4: "The string is opaque to the client, but depending on the
authorization server, may be parseable by the resource server."
    - What does opaque mean in this context? The client is technically
able to decode an AT if it is, for example, a JWT.
    - For refresh tokens the wording is not as strong: "The string is
**usually** opaque to the client." (see section 1.5)
- 1.5: In step 2 and 8 the AS "authenticates the client" but in step 7
the client provides "client authentication if it has been issued
credentials". Should the possibility of public clients and credentialed
clients (which cannot be authenticated) not be taken into account in
step 2 and 8, as well?
- 1.7 (and in general): HTTP status code 303 shoudl be used instead of
302 in this section and generally in all examples due to the
recommendation to use 303 in section 9.7.2.

Section 2. Client Registration
- 2.3 (and in general): Is "client authentication" the right term for
credentialed clients? We think the term "authentication" is confusing
because their credentials should be considered to be public. Therefore,
the AS can not be sure about their identity when they present their
client credentials.
- 2.3.1: An AS MUST NOT accept requests with ambiguous client
information (both HTTP Basic Authentication and client_id/client_secret
body parameters).
    - Different application logics could extract and use a different
client_id from the request otherwise. For example, the HTTP header is
validated to pass client authentication but the client_id parameter is
used to check if code/RT was issued for this client.
    - Should also be considered in 4.1.3.
    - Implicitly included in 5.2 Error Response: ""invalid_request": The
request ... includes multiple credentials, utilizes more than one
mechanism for authenticating the client …"

Section 3. Protocol Endpoints
- 3.1.2: "The authorization server MUST compare the two URIs using
simple string comparison as defined in [RFC3986], Section 6.2.1." We
think the term "two URIs" is ambiguous and suggest to name the
parameters here directly.
- 3.1.2.4: "If an authorization request fails validation due to a
missing, invalid, or mismatching redirect URI, the authorization server
SHOULD inform the resource owner of the error and MUST NOT automatically
redirect the user-agent to the invalid redirect URI." We think the term
"missing" is inaccurate because it might indicate that a redirect URI
should always be present. Maybe the paragraph could be changed to
something like "If an authorization request fails validation due to **an
invalid, a mismatching, or a missing (if it was required**) redirect
URI, the authorization server SHOULD inform the resource owner of the
error and MUST NOT automatically redirect the user-agent to the invalid
redirect URI.".

Section 4. Obtaining Authorization
- 4.1.2.1: ""invalid_request": The request is missing a required
parameter, includes an invalid parameter value, includes a parameter
more than once" Is this only relevant for the parameters defined in this
RFC or also for extensions? For example, RFC8707 (Resource Indicators)
allows to use multiple "resource" parameters in the authorization
request ("Multiple resource parameters MAY be used to indicate that the
requested token is intended to be used at multiple resources.",
https://www.rfc-editor.org/rfc/rfc8707.html#section-2-2.2).
- 4.2.2: Combine "The client MUST authenticate with the authorization
server as described in Section 3.2.1." and "The authorization server
MUST authenticate the client." below the example. The second sentence
looks oddly placed below the example.

Section 6. Refreshing an Access Token
- 6.1: Is token binding supported by any major browser? AFAIK it has
been removed recently, so it could be removed here as well.

Section 7. Accessing Protected Resources
- 7.2.1: The RS MUST NOT accept resource requests which use more than
one method to transmit the access token. This should be stated more
clearly here and not be included only in 7.2.3 Error Codes
"invalid_request".
- 7.4.2:
    - "As a further defense against token disclosure, the client MUST
validate the TLS certificate chain when making requests to protected
resources, including checking the Certificate Revocation List (CRL)
[RFC5280]." The paragraph should also mention OCSP [RFC 6960] as an
alternative to CRLs.
    - "Bearer tokens MUST NOT be stored in cookies that can be sent in
the clear". Maybe state explicitly that only cookies with secure flag
are allowed?
- 7.4.3.4: Maybe add recommended cookie flags (secure, httpOnly,
sameSite) with note "or equivalent measures" (to ensure that future
cookie flags / alternatives are allowed). This is also discussed in this
thread:
https://mailarchive.ietf.org/arch/msg/oauth/f18vynrMXC4dj4tqck7SZIl4xp4/

Section 9. Security Considerations
- 9.2: "Authorization servers MUST require clients to register" should
be clarified to "Authorization servers MUST require **native app**
clients to register" although this is already present in the headline of
the section.
- 9.7: Explicitly state that code_challenge / nonce must be bound to
user agent, as well?
- 9.15: Adjust text according to CSRF part in 9.7.
- 9.21: Remove the section as the only present recommendation is
redundant and already covered in 9.6.

Appendix:
- Appendix C:
    - Sort Extensions either alphabetically or depending on the RFC
number (drafts below the RFCs).
    - Should RFC7592 (Dynamic Client Management) really be included as
an "well-established extension"? It is only categorized as
"experimental" RFC.

General Comments:
- Unify the order of client types: In 2.1 and 9 it is web application,
browser-based application, native application, but the main section
about native applications (10) is in front of the main section about
browser-based applications (11). This should also be taken into account
when sections are added to 9, 9.1, or 9.3. Currently 9.1.1, 9.2, and
9.3.1 are about native applications and sections about browser-based
applications should be added in front of them (if there will be sections
specifically about browser-based applications).
- Is there any statement that it is allowed to add additional parameters
in the authorization/token request/response?


Part 2: Minor Mistakes (typos etc.)

Section 1. Introduction
- 1.5: "If the authorization server issues a refresh token, it is
included when issuing an access token (i.e., step (4) in Figure 2)." ->
"If the authorization server issues a refresh token, it is included when
issuing an access token (i.e., step (**2**) in Figure 2)."

Section 4. Obtaining Authorization
- 4.1.2: Note "(with extra line breaks for display purposes only)" is
missing in front of example.
- 4.2.2: Note "(with extra line breaks for display purposes only)"
present but there are no extra line breaks. The note is not present in
4.1.2.1, 4.1.4, and 4.2.3, for example.

Section 5. Issuing an Access Token
- 5.2: "The provided authorization grant (e.g., authorization code,
resource owner credentials)" -> "The provided authorization grant (e.g.,
authorization code, **client** credentials)"

Section 6. Refreshing an Access Token
- 6: Note "(with extra line breaks for display purposes only)" present
but there are no extra line breaks.

Section 7. Accessing Protected Resources
- 7.2.2: Note "(with extra line breaks for display purposes only)" is
missing in front of last example.

Section 9. Security Considerations
- 9.1.1: "public native app**s** clients" -> "public native app clients"
- 9.4.2: Missing reference to "(#pop_tokens)".
- 9.5: Missing reference to "(#refresh_token_protection)".
- 9.7:
    - Missing references to "(#insufficient_uri_validation)",
"(#open_redirector_on_client)", "(#csrf_countermeasures)", and
"(#mix_up)" (twice).
    - "Clients MUST store the authorization server they sent an
authorization request to and bind this information to the user agent and
check that the authorization request was received from the correct
authorization server." -> "Clients MUST store the authorization server
they sent an authorization request to and bind this information to the
user agent and check that the authorization **response** was received
from the correct authorization server."
- 9.7.2 (and in general): Replace "user agent" (used 18 times) with
"user-agent" (used 64 times).
- 9.8: Missing reference to "(#secmodel)".
- 9.18.1: Missing reference to "(#redir_uri_open_redir)".
- 9.21: Missing reference to "(#client_impersonating)".

-- 
Karsten Meyer zu Selhausen
IT Security Consultant
Phone:	+49 (0)234 / 54456499
Web:	https://hackmanit.de | IT Security Consulting, Penetration Testing, Security Training

Unsere nächste Live Online-Schulung zur Sicherheit von OAuth und OpenID Connect am 24.09 + 25.09:
https://hackmanit.de/de/schulungen/109-live-online-schulung-single-sign-on-sicherheit-oauth-openid-connect-am-24-und-25-09-2020

Hackmanit GmbH
Universitätsstraße 60 (Exzenterhaus)
44789 Bochum

Registergericht: Amtsgericht Bochum, HRB 14896
Geschäftsführer: Prof. Dr. Jörg Schwenk, Prof. Dr. Juraj Somorovsky, Dr. Christian Mainka, Dr. Marcus Niemietz