Re: [OAUTH-WG] Proposed changes to draft-ietf-oauth-dpop-02

Brian Campbell <bcampbell@pingidentity.com> Tue, 08 December 2020 19:16 UTC

Return-Path: <bcampbell@pingidentity.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 6C3763A10F9 for <oauth@ietfa.amsl.com>; Tue, 8 Dec 2020 11:16:41 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.098
X-Spam-Level:
X-Spam-Status: No, score=-2.098 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, 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=pingidentity.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 Li0geKrI_Urg for <oauth@ietfa.amsl.com>; Tue, 8 Dec 2020 11:16:37 -0800 (PST)
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 C0E623A10FE for <oauth@ietf.org>; Tue, 8 Dec 2020 11:16:36 -0800 (PST)
Received: by mail-lj1-x229.google.com with SMTP id t22so21460106ljk.0 for <oauth@ietf.org>; Tue, 08 Dec 2020 11:16:36 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pingidentity.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=O5oTn7M/5rAZa3xpig7RpJuItco0fnjKd7OlrVaqEXI=; b=SphTMxxIvdZK/jVcmcrggNkgq9gJpEfVQ8l1RGHuxdx+LO9j9n3rW/ECRbzTI1mCnL L6zHcYLkpGMFV2n1OwGLeuzG1xASnf5w0u+my1/H/yGUIQ2+7/IkTLG5ZHlyLC7oiODC Z2ELKU4+9SEhOSDDu9x+CvQOH9oLsTOpSdXkIKkURqQjhroLfA8Sz66XzbL+krg1lnYv lRwWAnALRxHVQcnVlQOXwKL+jSo0mercdLW1Q1exHz01Aov+TuWlU3nN3bMwS+H6eIhD kHOqjStzy11Sj8y+J1JK0lHBOAIP/07gh74N3t6QUdjFOXD9J7lYqZ5+MeNqc8nGyfFQ oEiQ==
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=O5oTn7M/5rAZa3xpig7RpJuItco0fnjKd7OlrVaqEXI=; b=rVclPVjIvCXlv4PAPKmUQnuri3+9NQgAnpZmPhfBEHdap9c0k7eZQlgsyJZqRW0iqY IT4t9ub3swlSWJlnAgWYkmfUBntAgFq9TmhEND2XJ2ASBt7kNWukZYaVvmOCc9CwjHLX fcoTpMgNhZA5Y3PrJoA5FUIWzSmJXqz0moeTGH6cwxTEYMvZziKVGPLXcL1Ent6+V3oa zWurV2BHCbitFH6sQ7MT+tUPrZJJLytHIXSTmqWHsuBGfwb7hF51sFu+RvhSiaBySOVs RH8Wp1mk7JHaZ7wnGz0iU2o9YusZ5Vzat9ql/oy7b93q5EeYitU6tEQn6wASq5sQV9G9 feoQ==
X-Gm-Message-State: AOAM531jOxh7+ZsF9boEh+EI7TRvP/YvQLBG/EFk+eqSMBbRVQ/L8fCR mslZaG4wSxrOPeFEYUYF1B6eEBq+BtfiC01+ke2HhDKZXGnkOaGKtiYuyMJszsQjuFclIj223Tn aPfN/q6DtjA4fxQ==
X-Google-Smtp-Source: ABdhPJxZd9Aj6B4QrxnfnlEeJQGlieSdP/p9G9SE2l+T2r7/KJe7giG9Kso43DhgNXVcGV/+2FLQGM7FYv5UYL+MVVg=
X-Received: by 2002:a2e:321a:: with SMTP id y26mr11348214ljy.293.1607454994645; Tue, 08 Dec 2020 11:16:34 -0800 (PST)
MIME-Version: 1.0
References: <5233025a-a958-a5c0-238c-ea6f1df371bc@free.fr>
In-Reply-To: <5233025a-a958-a5c0-238c-ea6f1df371bc@free.fr>
From: Brian Campbell <bcampbell@pingidentity.com>
Date: Tue, 8 Dec 2020 12:16:07 -0700
Message-ID: <CA+k3eCQYtMosNnvWm2VtRK-5P4x9wxBfUF00bxUtFNmh2Q-g0w@mail.gmail.com>
To: Denis <denis.ietf@free.fr>
Cc: oauth <oauth@ietf.org>
Content-Type: multipart/alternative; boundary="000000000000f2e2cf05b5f8c77e"
Archived-At: <https://mailarchive.ietf.org/arch/msg/oauth/VDAFrjPK5rFQqVUw9KWc3GhpIbs>
Subject: Re: [OAUTH-WG] Proposed changes to draft-ietf-oauth-dpop-02
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: Tue, 08 Dec 2020 19:16:42 -0000

attempts at replies are inline

On Wed, Dec 2, 2020 at 8:42 AM Denis <denis.ietf@free.fr> wrote:

> I have reviewed the whole draft and you will find comments below starting
> with five editorials comments. Every other comment is numbered.
> Let us start with five typos where there is a duplication of the word
> "the":
>

Will fix, thank you.

1. Section 1. Introduction
>
> The text states:
>
>      The value of the header is a JWT [RFC7519] that enables the
> authorization server to bind issued tokens to the public part of the
> client’s key pair.
>
> A client may use different key pairs.
>
> Change :
>
> "the client’s key pair"
>
> into :
>
> " a client’s key pair".
>

Okay, sure.

2. Section 2.  Objectives
>
> The text states:
>
>     The primary aim of DPoP is to prevent unauthorized or illegitimate
> parties from using leaked or stolen access tokens by binding a token to a
> public key
>     upon issuance and requiring that the client demonstrate possession of
> the corresponding private key when using the token.
>
> The objective needs to be described in terms of what ,the mechanism does
> and not yet at this time for which reasons.
>
> Change into:
>
>      The aim of DPoP is first to require a client to demonstrate
> possession of a public key when requesting an access token or a refresh
> token to an AS
>      where the AS will then bind that public key upon a token issuance
> and, then to demonstrate the possession of a public key included into a
> token
>      when presenting a refresh token to an AS or an access token to a RS.
>
I'm of the opinion that the current text is fine.

3. The next sentence looks odd as far as the English grammar is considered.
>
>     This constrains the legitimate sender of the token to only the party
> with access to the private key and gives the server receiving the token
> added assurances
>     that the sender is legitimately authorized to use it.
>
> With the proposed change above, this sentence looks unnecessary and could
> be deleted.
>

Grammar has admittedly never been my strong suit but, even after rereading
the text a few times, I don't see reason to change it. Perhaps the RFC
editor will help out later in the process, if need be.


4. Section 3 Concepts Page 5
>
> In order to make crystal clear that they are to different DPoP Proofs,
> (DPoP Proof 1) and (DPoP Proof 2) should be shown on the figure as proposed
> below:
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> *+--------+                                          +---------------+
> |        |--(A)-- Token Request ------------------->|               | |
> Client |        (DPoP Proof 1)                    | Authorization |
> |        |                                          |     Server    |
> |        |<-(B)-- DPoP-bound Access Token ----------|               |
> |        |        (token_type=DPoP)                 +---------------+
> |        | |        | |        |
> +---------------+ |        |--(C)-- DPoP-bound Access Token
> --------->|               | |        |        (DPoP Proof
> 2)                    |    Resource   | |
> |                                          |     Server    | |
> |<-(D)-- Protected Resource ---------------|               | |
> |                                          +---------------+ +--------+
>                       Figure 1: Basic DPoP Flow *
> 5. The text states:
>
>      The basic steps of an OAuth flow with DPoP are shown in Figure 1:
>
>      *  (A) In the Token Request, the client sends an authorization grant
> (e.g., an authorization code, refresh token, etc.) to the authorization
> server
>         in order to obtain an access token (and potentially a refresh
> token).  The client attaches a DPoP proof to the request in an HTTP
> header.
>
> At the end of the sentence, add : (DPoP Proof 2).
>

I don't think having the proofs labeled with 1 & 2 detracts more than it
adds. And that would be the wrong one in the context of that sentence, if
they were labeled.


>
>
> 6. The text states:
>
>      *  (C) To use the access token the client has to prove possession of
> the private key by, again, adding a header to the request that carries the
> DPoP proof.
>
> Change into:
>
>      *  (C) To use the access token the client has to prove possession of
> the private key by using a header to the request that carries another DPoP
> proof (DPoP Proof 2).
>

Will add/adjust some text to try and make it more clear that the proof is
specific and unique to that request.


>
> 7. Section 4.  DPoP Proof JWTs
>
> The text states:
>
>      A valid DPoP proof demonstrates to the server that the client holds
> the private key that was used to sign the JWT.
>
> For avoiding misunderstandings, it would be better to say that this
> applies to DPoP Proof JWTs and that it applicable for both ASs and RSs.
>
> Proposed change:
>
>      A valid DPoP proof demonstrates to a server, (i.e. an AS or a RS),
> that the client holds the private key that was used to sign a DPoP Proof
> JWT.
>

What other server could there be in the context of this work? I think AS/RS
there just clutters the text. And please try and use language and
abbreviations that are consistent with the draft when you propose text -
the AS and RS abbreviations are not used currently. Otherwise it's even
more difficult to consider/incorporate suggestions.

The JWT in question in that sentence seemed rather self evident but I can
make it more explicit in an effort at avoiding misunderstandings.

8. The text states:
>
>      This enables authorization servers to bind issued tokens to the
> corresponding public key (as described in Section 5) and for resource
> servers to verify
>      the key-binding of tokens that it receives (see Section 7.1), which
> prevents said tokens from being used by any entity that does not have
> access to the private key.
>
> The end of the sentence is using the wording "(...) which prevents said
> tokens from being used by any entity that does not have access to the
> private key".
>
> a) It is proposed to remove the end of the sentence, i.e.: "), which
> prevents said tokens from being used by any entity that does not have
> access to the private key".
>     A new section 8.2 is being proposed (DPoP private key usage). See
> comment 17 which addresses this issue.
>
Here, as is/was the case with many other drafts, we are not going to make
changes to the draft to cover something that is not a reasonable
expectation of this work or OAuth in general. I believe WG consensus has
been consistent about this general issue and I expect the draft(s) to
reflect that (unless that WG consensus shifts, which is possible but I'm
not seeing anything to suggest that's the case).


At this time, it would be adequate to add that a DPoP Proof JWT can only be
> used once.
>
> b) Additional proposed text:
>
>      A DPoP proof JWT is intended to be only usable once: it includes
> claims that allows an AS or a RS to detect replays.
>
This is something that I expected would be understood from the general
context of the document. But perhaps that was too much of an assumption.
I'll add something to that effect.


>
> 9. Section 4.2.  DPoP Proof JWT Syntax
>
> The text states:
>
>      The body of a DPoP proof contains at least the following claims:
>
> (...)
>
>      *  "jti": Unique identifier for the DPoP proof JWT (REQUIRED).  The
> value MUST be assigned such that there is a negligible probability that the
> same value
>         will be assigned to any other DPoP proof used in the same context
> during the time window of validity.  Such uniqueness can be accomplished
> by encoding
>         (base64url or any other suitable encoding) at least 96 bits of
> pseudorandom data or by using a version 4 UUID string according to
> [RFC4122].  The "jti" can
>         be used by the server for replay detection and prevention, see
> Section 8.1.
>
> This looks like over engineering. 32 bits of pseudorandom data will be
> sufficient if used in conjunction with the "iat" claim. If the server (AS
> or RS) uses a concatenation
> of both the "iat" and the "jti" this makes 2 ^ 32 possibilities within the
> same second during a time windows of a few minutes (about 120 seconds).
>
> To highlight the fact that "jti" is a complement of "iat", it should be
> placed after "iat" in the description".
>
> Proposed change:
>
>    *  "jti": JWT identifier that complements the "iat" claim to handle
> replay protection for a given server (REQUIRED).
>
>       The value MUST be assigned such that there is a negligible
> probability that the same value will be assigned to any other DPoP proof
> received by the server
>       within the same second during a short time window of validity (i.e.
> a few minutes only) . This MUST be accomplished by encoding 32 bits of
> pseudorandom data.
>
>
> 10. Figure 3.
>
> The end of figure 3 is as follows:
>
> (...)
>      {
>      "jti":"-BwC3ESc6acc2lTc",
>      "htm":"POST",
>      "htu":"https://server.example.com/token"
> <https://server.example.com/token>,
>      "iat":1562262616
>     }
>               Figure 3: Example JWT content of a "DPoP" proof
>
> In order to follow the ordering of the claims, as proposed before, it
> would be more adequate to write it as follows:
>
> (...)
>      {
>      "htm":"POST",
>      "htu":"https://server.example.com/token"
> <https://server.example.com/token>,
>      "iat":1562262616
>      "jti": EE238F3F,
>
>     }
>               Figure 3: Example JWT content of a "DPoP" proof
>
> Please note that the "jti" value is shorter.
>

 The 'jti' value in that example does not conform to the definition of the
claim from RFC 7519 / JWT. It's a relatively minor issue in the scheme of
things but I feel it typifies a larger dynamic at play in some of the WG
exchanges here. Comprehending and responding to comments and suggestions
can be arduous and very time consuming, which is further exacerbated by
outright errors, mischaracterization, or misunderstandings in such
feedback. I'd humbly request that, out of respect for everyone’s time, we
all please be a little more careful and precise in our exchanges. I don't
want to discourage discourse - that's much of the point of a WG after all -
I'd just request that a little more consideration be given for the impact
to others.



> 11. The text below figure 3 states:
>
>       Of the HTTP content in the request, only the HTTP method and URI are
> included in the DPoP JWT, and therefore only these 2 headers of the request
>       are covered by the DPoP proof and its signature.
>
> The four claims should be covered by the signature.
>

The point here is about the HTTP content.  Not the claims.


> Proposed change:
>
>     Of the HTTP content in the request, only the HTTP method ("htm"
> claim"), the URI ("htu" claim), the issuance time of the DPoP proof JWT
> ("iat" claim)
>     and the DPoP proof JWT identifier ( jti "claim") are included in the
> DPoP JWT, and therefore only these 4 headers of the request are covered by
> the DPoP proof
>     and its signature.
>
> 12. Section 4.3.  Checking DPoP Proofs
>
> The text states:
>
>      To check if a string that was received as part of an HTTP Request is
> a valid DPoP proof, the receiving server MUST ensure that
>
>         1.  the string value is a well-formed JWT,
>         2.  all required claims are contained in the JWT,
>
> It would be wise to indicate what are "all the required claims"
>
> Change the last above sentence into:
>
>    2.  all required claims are contained in the JWT, at least the "htm"
> claim, the "htu" claim, the "iat" claim and the "jti "claim.
>

The required claims are listed in sec 4.2 and I don't believe it's wise to
duplicate that normative requirement here. Perhaps a back reference to that
section would be good.



>
> 13. The text states:
>
>      5.  that the JWT is signed using the public key contained in the
> "jwk" header of the JWT
>
> Typo: Since the sentence above all these conditions already includes the
> word "that" (*the receiving server MUST ensure that*),
> the word "that" in this sentence should be removed.
>
> Change into:
>
>      5.  the JWT is signed using the public key contained in the "jwk"
> header of the JWT
>

Okay. I guess the same would apply to #9 of that list too. The list could
benefit from some tidying up in general.


14. The text continues with:
>
>        8.  the token was issued within an acceptable timeframe (see
> Section 8.1), and
>        9.  that, within a reasonable consideration of accuracy and
> resource utilization, a JWT with the same "jti" value has not previously
> been received
>             at the same URI (see Section 8.1).
>
> References to text located in the "Security considerations" section should
> be avoided.
>

If that's problematic, I expect that it'll come up in
chair/shepherd/AD/IESG reviews. And can be addressed then.


>
> In addition this text needs to be revised in order to take benefit of the
> use of a combination of "iat" and "jti".
>
> Change the two last above sentences into:
>
> [the receiving server MUST ensure that]
>
>        8.  the "iat" time is within an acceptable timeframe (a few
> seconds skew SHOULD be allowed).
>
>            Note: To accommodate for clock offsets, the server SHOULD
> accept DPoP proofs that carry an "iat" time in the reasonably near future
> (e.g., a few seconds in the future).
>
>        9.  and that, in order to accept the DPoP proof JWT, both the
> "iat" claim and the associated "jti" claim have not already been seen
> before during the time window
>             used by the server; otherwise refuse the DPoP proof JWT.
>
>
> 15. Section 5.  DPoP Access Token Request (Page 12)
>
> The text states:
>
>        An authorization server MAY elect to issue access tokens which are
> not DPoP bound, which is signaled to the client with a value of "Bearer" in
> the "token_type" parameter
>        of the access token response per [RFC6750].
>
> This has a consequence for the client which is not mentioned but should be
> mentioned.
>
> Proposed additional text:
>
>        When the client receives an access token, it MUST check whether the
> "token_type" parameter of the access token response contains a value of
> "Bearer" or a value of " DPoP".
>        If a "Bearer" token is received whereas the client indeed wanted a
> "DPoP" token, then the client SHALL discard the response.
>

I'd presumed it was understood that not proceeding was always an option to
the client. But we can mention it more explicitly here.


16. Section 8.1.  DPoP Proof Replay
>
> Replace the current text with:
>
>       In order to prevent the replay of a token at the same endpoint (the
> HTTP endpoint), only a single DPoP proof JWT which contains the same claims
> is accepted during a time window
>       set by the server.  This prevents both legitimate and illegitimate
> clients to use twice a same DPoP proof JWT. A JWT that is still valid can
> be associated with a new DPoP proof JWT
>      and hence can be re-used during its validity period.
>
>       The mechanism which is based on both the "iat" and the "jti" claims
> occupies 64 bits of memory (32 +32 bits) per DPoP proof JWT that has been
> accepted.
>       The entries can be flushed after the end of the time window.
>
>       Clients have no way to know in advance the exact value of a time
> window used by a server.  If they wait too long for using a DPoP proof
> JWT, a DPoP proof JWT / access token pair
>       may be rejected, even it is cryptographically correct.
>
> 17. Add a new section 8.2:
>
>       8.2.  DPoP private key usage
>
>       A legitimate client does not necessarily need to "have access to"
> the private key that is being used to sign a DPoP proof JWT, but can simply
> "use" the private key without knowing its value.
>       This means that it is able to perform cryptographic computations
> either for its own benefit or for the benefit of other users. In the later
> case, an illegitimate client may be given both an access token
>       and a DPoP Proof JWT by a legitimate client.  The fact that a DPoP
> proof JWT can only be used once does not protect against this collaborative
> attack.
>
>
> 18. Currently there is no "Privacy considerations" section, whereas there
> should be one.
>
> This point is addressed in a separate email since it proposes new text.
>
> Denis
>
> _______________________________________________
> OAuth mailing list
> OAuth@ietf.org
> https://www.ietf.org/mailman/listinfo/oauth
>

-- 
_CONFIDENTIALITY NOTICE: This email may contain confidential and privileged 
material for the sole use of the intended recipient(s). Any review, use, 
distribution or disclosure by others is strictly prohibited.  If you have 
received this communication in error, please notify the sender immediately 
by e-mail and delete the message and any file attachments from your 
computer. Thank you._