Re: [Gen-art] Gen-ART Last Call review of draft-ietf-ace-dtls-authorize-12

Benjamin Kaduk <kaduk@mit.edu> Mon, 27 July 2020 19:11 UTC

Return-Path: <kaduk@mit.edu>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 7CEC83A0B91; Mon, 27 Jul 2020 12:11:03 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.919
X-Spam-Level:
X-Spam-Status: No, score=-1.919 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_MSPIKE_H4=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
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 irKFAOjwVtD6; Mon, 27 Jul 2020 12:11:01 -0700 (PDT)
Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B898D3A0B35; Mon, 27 Jul 2020 12:10:58 -0700 (PDT)
Received: from kduck.mit.edu ([24.16.140.251]) (authenticated bits=56) (User authenticated as kaduk@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id 06RJAqRh002537 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 27 Jul 2020 15:10:54 -0400
Date: Mon, 27 Jul 2020 12:10:52 -0700
From: Benjamin Kaduk <kaduk@mit.edu>
To: Paul Kyzivat <pkyzivat@alum.mit.edu>
Cc: draft-ietf-ace-dtls-authorize.all@ietf.org, General Area Review Team <gen-art@ietf.org>
Message-ID: <20200727191052.GI41010@kduck.mit.edu>
References: <8c2725a3-f89f-7ea1-dda9-681edd463a32@alum.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <8c2725a3-f89f-7ea1-dda9-681edd463a32@alum.mit.edu>
User-Agent: Mutt/1.12.1 (2019-06-15)
Archived-At: <https://mailarchive.ietf.org/arch/msg/gen-art/59ouWy6beg8DpBLRjCzCTkWxIHE>
Subject: Re: [Gen-art] Gen-ART Last Call review of draft-ietf-ace-dtls-authorize-12
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 27 Jul 2020 19:11:10 -0000

Hi Paul,

Just a couple notes in addition to what Jim already mentioned.

On Sun, Jul 19, 2020 at 04:23:46PM -0400, Paul Kyzivat wrote:
> 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-ace-dtls-authorize-12
> Reviewer: Paul Kyzivat
> Review Date: 2020-07-19
> IETF LC End Date: 2020-07-20
> IESG Telechat date: ?
> 
> Summary:
> 
> This draft is on the right track but has open issues, described in the 
> review.
> 
> General:
> 
> TBD
> 
> Issues:
> 
> Major: 2
> Minor: 1
> Nits:  1
> 
> 1) MAJOR: Management of token storage in RS
> 
> There seems to be an expectation that when the client uploads an access 
> token that the RS will retain it until the client attempts to establish 
> a DTLS association. This seems to require some sort of management of 
> token lifetime in the RS. The only discussion I can find of this issue 
> is the following in section 7:
> 
>     ... A similar issue exists with the
>     unprotected authorization information endpoint where the resource
>     server needs to keep valid access tokens until their expiry.
>     Adversaries can fill up the constrained resource server's internal
>     storage for a very long time with interjected or otherwise retrieved
>     valid access tokens.
> 
> This seems to imply a normative requirement to keep tokens until their 
> expiry. But I find no supporting normative requirements about this. And, 
> this section only presents it as a DoS attack, rather than a potential 
> problem with valid usage.
> 
> ISTM that there is an implied requirement that the RS have the capacity 
> to store one access token for every PoP key of every authorized client. 
> If so, that ought to be stated. If not, then some other way of bounding 
> storage ought to be discussed.
> 
> 2) MAJOR: Missing normative language
> 
> I found several places where the text seems to suggest required behavior 
> but fails to do so using normative language:
> 
> * In section 3.3:
> 
>     ... Instead of
>     providing the keying material in the access token, the authorization
>     server includes the key identifier in the "kid" parameter, see
>     Figure 7.  This key identifier enables the resource server to
>     calculate the symmetric key used for the communication with the
>     client using the key derivation key and a KDF to be defined by the
>     application, for example HKDF-SHA-256.  The key identifier picked by
>     the authorization server needs to be unique for each access token
>     where a unique symmetric key is required.
>     ...
>     Use of a unique (per resource server) "kid" and the use of a key
>     derivation IKM that is unique per authorization server/resource
>     server pair as specified above will ensure that the derived key is
>     not shared across multiple clients.
> 
> The uniqueness seems to be a requirement. Perhaps "needs to be unique" 
> should be "MUST be unique". (And something similar for the IKM.)
> 
> * Also in section 3.3:
> 
>     All CBOR data types are encoded in CBOR using preferred serialization
>     and deterministic encoding as specified in Section 4 of
>     [I-D.ietf-cbor-7049bis].  This implies in particular that the "type"
>     and "L" components use the minimum length encoding.  The content of
>     the "access_token" field is treated as opaque data for the purpose of
>     key derivation.
> 
> IIUC the type of serialization and encoding is a requirement. Will need 
> some rewording to make it so.

Both this and the previous item are scoped by the following text:

   The method for how the resource server determines the symmetric key
   from an access token containing only a key identifier is application-
   specific; the remainder of this section provides one example.

So it's not entirely clear that normative language is appropriate in the
discussion of the example behavior.

> * In section 3.3.1:
> 
>     ... To
>     be consistent with the recommendations in [RFC7252] a client is
>     expected to offer at least the ciphersuite TLS_PSK_WITH_AES_128_CCM_8
>     [RFC6655] to the resource server.
> 
> I think "is expected" should be "MUST".
> 
> * Also in section 3.3.1:
> 
>     ... This
>     specification assumes that the access token is a PoP token as
>     described in [I-D.ietf-ace-oauth-authz] unless specifically stated
>     otherwise.
> 
> I think "assumes ... unless" should be "MUST ... unless".

My understanding is that this is just talking about the text in the
document itself.  But as far as I remember we always require PoP tokens, so
this could just be removed.

> * Also in section 3.3.1:
> 
>     ... New access tokens issued by the
>     authorization server are supposed to replace previously issued access
>     tokens for the respective client.
> 
> Is this normative? Should "are supposed to" be "MUST"?

I don't think this is a "MUST"; it refers to some behavior in the core
framework that is merely suggested and not mandatory.

Thanks for the review; it brought up some good points!

-Ben

> 3) MINOR: Insufficient specification
> 
> (I'm waffling whether this is minor or major.)
> 
> There are a couple of places where what seem to be requirements are 
> stated too vaguely to be implemented consistently:
> 
> * In the previously mentioned paragraph in 3.3.1:
> 
>     ... This
>     specification assumes that the access token is a PoP token as
>     described in [I-D.ietf-ace-oauth-authz] unless specifically stated
>     otherwise.
> 
> The "unless specifically stated otherwise" is too vague to be normative. 
> How would the alternative be indicated? Is this an escape hatch for 
> future extensions? If so, it needs more work to make that clear and to 
> open a path for that future work.
> 
> * Also in section 3.3.1:
> 
>     ... The resource server therefore must
>     have a common understanding with the authorization server how access
>     tokens are ordered.
> 
> The last statement ("must have a common understanding") is mysterious. 
> IIUC section 4 is covering the same topic in a less mysterious way.
> 
> 4) NIT: Subsection organization
> 
> Both 3.2 and 3.3 share a common structure:
> 
> * The section begins with discussion of the interaction between the 
> client and the AS.
> 
> * it is followed by a subsection discussing the interaction between the 
> client and the RS.
> 
> It is odd to have a section with a single subsection. And the structure 
> isn't easily discerned from the TOC.
> 
> I suggest it would be clearer if each of these sections had *two* 
> subsections, one covering the AS interactions and the other the RS 
> interactions. IOW, put the material covering the AS interactions into a 
> new subsection.