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

From: Paul Kyzivat <[email-masked]>
Date: Sun, 19 Jul 2020 16:23:46 -0400

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.

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

* 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"?

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.

---

From: Jim Schaad <[email-masked]>
Date: Sun, 19 Jul 2020 15:29:48 -0700

> -----Original Message-----
> From: Paul Kyzivat <[email-masked]>
> Sent: Sunday, July 19, 2020 1:24 PM
> To: [email-masked]
> Cc: General Area Review Team <[email-masked]>
> Subject: Gen-ART Last Call review of draft-ietf-ace-dtls-authorize-12
> 
> 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.
> 
review.
> 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
> 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.
storage
> ought to be discussed.

In section 5.8.1 of draft-ietf-ace-oauth-authz is the sentence "The RS MUST
be prepared to store at least one access token for future use."   When this
was put in, this is exactly what we were discussing.  There is no
requirement that an RS needs to store two access tokens for future use.  I
think this means that there is a strongly bounded requirement on storage.

Authors - It might be worthwhile to re-iterate this requirement in both of
the profile documents.

> 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
some
> rewording to make it so.

The rule would be MUST implement not MUST offer.  A client could offer a
completely different ciphersuite that is consistently used in the group and
that would be fine.  

> There are a couple of places where what seem to be requirements are stated
> too vaguely to be implemented consistently:
> 
> 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.

I am seeing this in section 3.4 - did I miss something?

> * The section begins with discussion of the interaction between the client
and
> the AS.
client
> and the RS.
isn't easily
> discerned from the TOC.
subsections,
> one covering the AS interactions and the other the RS interactions. IOW,
put the
> material covering the AS interactions into a new subsection.

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 27 Jul 2020 12:10:52 -0700

Hi Paul,

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

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

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

> 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

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 28 Jul 2020 11:22:45 -0400

Jim,

Sorry - I forgot to respond sooner.

Please see comments inline.

	Thanks,
	Paul

> think this means that there is a strongly bounded requirement on storage.

When I read that, it seems that the stated requirement is to store one 
token *in total*, yet the context implies that it ought to be one token 
*per* client.

The procedures defined aren't going to work unless there is one per client.

> completely different ciphersuite that is consistently used in the group and

That implies that the client be configured to *know* that the server 
supports something else. I guess that is ok.

> I am seeing this in section 3.4 - did I miss something?

I'm sorry. This issue should have referred to that passage in 3.4, not 
3.3.1.

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 28 Jul 2020 11:36:48 -0400

Hi Ben,

Please find my comments inline.

> discussion of the example behavior.

OK. That makes sense.

> this could just be removed.

It gets simpler if you always require PoP tokens. Does it state that 
normatively somewhere?

The "unless" construct opens a can of worms about how things are to work 
in that case.

> framework that is merely suggested and not mandatory.

I commented on this in my reply to Jim.

	Thanks,
	Paul

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 29 Jul 2020 03:16:39 -0700

Hi Paul,

Also inline.

> The "unless" construct opens a can of worms about how things are to work 

Hmm, I didn't find a clear and unambiguous statement in a quick check.
Similar language about "in this document the access token is assumed to be
a PoP token unless specified otherwise" in the core framework document,
draft-ietf-ace-oauth-authz.  But that document uses "access token" some
130-odd times, and while I didn't see any mention of
non-proof-of-possession in there, I may have missed one.  (There is
discussion of not using symmetric proof-of-possession keys in a
group-audience context, which is supposed to mean use asymmetric proof of
possesion keys in that case.)

Assuming that I remember correctly (and the WG should correct me if I'm
wrong), it might be easiest to change both this document and the core
framework to flatly assert that PoP tokens are always required.

> I commented on this in my reply to Jim.

Okay, I'll follow that thread.

Thanks again,

Ben