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

Dick Hardt <dick.hardt@gmail.com> Thu, 06 August 2020 21:23 UTC

Return-Path: <dick.hardt@gmail.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 66BFB3A0F2A for <oauth@ietfa.amsl.com>; Thu, 6 Aug 2020 14:23:21 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -0.853
X-Spam-Level:
X-Spam-Status: No, score=-0.853 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_FONT_LOW_CONTRAST=0.001, HTML_MESSAGE=0.001, NORMAL_HTTP_TO_IP=0.001, NUMERIC_HTTP_ADDR=1.242, 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=gmail.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 kPleBHEM1VKW for <oauth@ietfa.amsl.com>; Thu, 6 Aug 2020 14:23:19 -0700 (PDT)
Received: from mail-lj1-x229.google.com (mail-lj1-x229.google.com [IPv6:2a00:1450:4864:20::229]) (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 BFE2C3A0F26 for <oauth@ietf.org>; Thu, 6 Aug 2020 14:23:18 -0700 (PDT)
Received: by mail-lj1-x229.google.com with SMTP id w25so17944890ljo.12 for <oauth@ietf.org>; Thu, 06 Aug 2020 14:23:18 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TY/yrcvdv6mQkuqP9INU8r2GFGaGhDvKWNeMQvnsuk0=; b=ozoA59TjdiOsLsVo/R3Tfc6ymYIrS05Bp41a9PoYOvXRsuIjXV7iVAPP/AHm4CbGyq 6urcxGkxTviJtLbjmIDscSG8kdAnFTBeVOyEDYw5JUJgdgp0Ms1R+uNLqOmpWeoY1QF8 PgxL5beVgrccGrslqC2L+ocg4NNvhZeYpsLeyu9wTLPwMnNmvGESMT9fwUb5IIZTfSQg mVWt5tlNbO+xdmcrBsd5B6lsOoUMzr1GLLJc6jLkbvbo1hdve6EbPrBhhnQ8G03Paopi diZ2sfejOr/wKF7365R7TKBKU6y4nMJWhyG0lvG15oagoFWTN7O3qLBRZjBiRa1Bc+XY ++zg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TY/yrcvdv6mQkuqP9INU8r2GFGaGhDvKWNeMQvnsuk0=; b=d9J9t5YwTcoH4O6Rv+PBDNSQwNohsGXhbig9PNBXwC/Mtp3bQD2Rw0leKbCiG/WY3o h6nQojypICabDaBYJ0Y7LQADLxNZfDJgrn8bgfINQydaI0sf+r9/Kb8P/Fqr9xINwGWU pJ66DNeHjW2hgoaQtIDdhlj1KYSb7T/FU95zQaNCjWDxUEH8V8yQZirk94I+vtfPM1v8 I7hV0Yhjn3RUxkH4p6492/a2Zfg2IdUplg/yz9uuEkzPiLF1d+WDhDpgWwVxrTAHV7c7 5Qh/+KHKQyPhMQzh39pmDLc4RUsH+BSmLYNeT5vvP1oO6xvgYHlO8VH5CGVZcte7eyDR k8iA==
X-Gm-Message-State: AOAM531SugU7iYRs1ZMAlNo3ZOPdhAsN4KfMW6eKBiiYX5RuyjZRMhQw vSl4npuOskOvXT4RmT+eW0W9Hma2FIGXn068XTnV2QQLkgA=
X-Google-Smtp-Source: ABdhPJyxTfsa5Sf4X5eXdjshGFsFYCLoi8JtqBVrje0qMnM0Tjed0Yvmj5qommXQrJbiL0pK57vWPArdMcUK2LDEtZM=
X-Received: by 2002:a2e:2283:: with SMTP id i125mr4539512lji.142.1596748996594; Thu, 06 Aug 2020 14:23:16 -0700 (PDT)
MIME-Version: 1.0
References: <a5c9ee0e-5b41-c4d3-ebff-d4a7985b663f@hackmanit.de>
In-Reply-To: <a5c9ee0e-5b41-c4d3-ebff-d4a7985b663f@hackmanit.de>
From: Dick Hardt <dick.hardt@gmail.com>
Date: Thu, 06 Aug 2020 14:22:40 -0700
Message-ID: <CAD9ie-vgEOp9DhmLFfu8Z-q+oNB833WjtVXisVTdZ23jjU2k1w@mail.gmail.com>
To: Karsten Meyer zu Selhausen <karsten.meyerzuselhausen@hackmanit.de>
Cc: oauth@ietf.org, Christian Mainka <christian.mainka@hackmanit.de>
Content-Type: multipart/alternative; boundary="000000000000bcfafb05ac3c1803"
Archived-At: <https://mailarchive.ietf.org/arch/msg/oauth/oRIO2wgIKw7-z9wFm4wSpUdfQLE>
Subject: Re: [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 21:23:21 -0000

Karsten / Christian: thank you for reviewing the draft and all of your
feedback. Greatly appreciated!

Responses inline ... if no comment, I (or another author) will have to
review to respond.

On Thu, Aug 6, 2020 at 3:48 AM Karsten Meyer zu Selhausen <
karsten.meyerzuselhausen@hackmanit.de> wrote:

> 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.
>

More clarity on what opaque means is a good suggestion. To answer your
question, the Client should treat the AT as a string. The token format
could be anything the AS and RS want, and could change at any time. This
does not mean the Client cannot discern information from the token, and an
AS should ensure that a Client cannot violate any privacy objectives by
gleaning information from the AT.


>     - For refresh tokens the wording is not as strong: "The string is
> **usually** opaque to the client." (see section 1.5)
>

I'm not sure why "usually" is in there.


> - 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.
>


It is unfortunate that credentials can be interpreted a couple of ways. One
way is how I think you are intending, which is a public statement about an
entity.

In this case, we are thinking of credentials as a mechanism to
authenticate such as a password or private key.


> - 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.
>

Edge still supports token binding AFAIK.


>
> 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
>
> _______________________________________________
> OAuth mailing list
> OAuth@ietf.org
> https://www.ietf.org/mailman/listinfo/oauth
>
ᐧ