On 9/8/20 7:14 AM, Olaf Bergmann wrote:
Hi Paul,

I hope you are doing well. On request from Jim Schaad who acts as the
document shepherd, we have submitted draft-ietf-ace-dtls-authorize-13
including the changes proposed below. Please feel free to contact us if
you do not agree with these proposals.

Thanks for addressing my comments. For the most part the changes address my concerns. My lack of any deep understanding of the subject material limits the extent that I can comprehend if my concerns have been sufficiently addressed. Further comments inline.

Grüße
Olaf

Olaf Bergmann <bergm...@tzi.org> writes:

Thank you very much for your review and the very useful suggestions
therein. Please find our comments inline. All issues that have not been
resolved in the prior discussion threads with Ben and Jim have been
addressed in the editor's copy of the draft.

Paul Kyzivat <pkyzi...@alum.mit.edu> writes:

I am the assigned Gen-ART reviewer for this draft. The General Area
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.

There is no normative requirement to keep tokens until their expiry. A
resource server could dispose of a stored token at any time, forcing a
client to retrieve another access token for subsequent requests directed
to the respective resource server. The only requirement imposed on the
resource server is that it must not send data to the client if there is
no valid access token that authorizes this action (this is described in
Section 3.4).

Usually, a resource server implementation would keep the received access
tokens as long as it deems useful for the intended interaction with the
client, with the expiration time being an upper bound for the storage
time (see Section 3.4).

I think I understand. Discarding tokens while they are still in use would result in sub-optimal behavior (including potential thrashing). But this can be viewed as an optimization or quality of implementation issue.

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.

The ACE framework already states in Section 5.8.1 that the "RS
MUST be prepared to store at least one access token for future use"
and "RECOMMENDS that an RS stores only one token per
proof-of-possession key". This is now re-iterated in this profile
as suggested:

NEW:

   A resource server MUST have the capacity to store one access token
   for every proof-of-possession key of every authorized client.

Sounds good.

Although a reasonable implementation
of this profile would try to provide storage for every PoP key of
every authorized client, this could easily get out of bounds with the
separate upload mechanism to the authz-info resource as pointed out in
the cited paragraph from the security considerations. This now has
been elaborated as follows:

OLD:

   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.

NEW:

   A similar issue exists with the unprotected authorization
   information endpoint when the resource server needs to keep valid
   access tokens for a long time. Adversaries could fill up the
   constrained resource server's internal storage for a very long time
   with interjected or otherwise retrieved valid access tokens.  To
   mitigate against this, the resource server should set a time
   boundary until an access token that has not been used until then
   will be deleted.

Sounds good.

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

We have changed "needs to be unique" to "MUST be unique" in the first
paragraph as suggested, and "the use of a key derivation IKM that is
unique" to "the use of a key derivation IKM that MUST be unique" in the
second paragraph you have mentioned.

Sounds good.

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

I take it that you and Ben have agreed that the example description does
not necessarily need normative language as the description of this key
derivation procedure is meant as an example how the authorization server
and the resource server can securely agree on a shared secret to be used
between the client and the resource server.

This still confuses me. IIUC preferred serialization and deterministic encoding are *optional* in CBOR. The text hear seems to require it, but doesn't use normative language to do so.

If these are required for things to work then you make a normative statement about it. E.g., "The "type" and "L" components MUST use the minimum length encoding."

Or do you intend that some other (non-minimum-length) MAY be used? (In which case both sides would need a side agreement on what encoding is used.)

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

>From [1] I take it that you have been ok with the current wording to
indicate that this means a MUST implement while there may be cases where
a client knows that it can offer something else.

    [1] https://mailarchive.ietf.org/arch/msg/ace/V8WXg8dhE3UkDxbIbOK-D0VmD9U/

Yes.

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

We are happy to mandate PoP tokens. In the beginning when
the protocol was intended to be similar to OAuth, people wanted to be
have some loophole for other token types. The text now has been
changed as proposed in [2] to

   [2] https://mailarchive.ietf.org/arch/msg/ace/ddtl8a4fmvEdXhEWc3HQtv6RQgo/

OLD:

   This specification assumes that the access token is a PoP token as
   described in [I-D.ietf-ace-oauth-authz] unless specifically stated
   otherwise.

NEW:

   This specification implements access tokens as proof-of-possession
   tokens.

Sounds good.

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

The ACE profiles would continue to work if there were multiple
access tokens for a single resource server/client association in
effect at a particular point in time. But neither OAuth nor ACE have
defined algorithms for merging authorization rules, and therefore, the
ACE WG has decided to keep things simple and assume that a newer
access token always replaces the old one.

As a consequence, the framework document and the profiles are written
as if a newer access token always replaces older access tokens for a
respective association between resource server and client.  To make
this assumption more clear, we have removed the "are supposed to". The
text now reads

OLD:

   New access tokens issued by the authorization server are supposed to
   replace previously issued access tokens for the respective client.

NEW:

   New access tokens issued by the authorization server replace
   previously issued access tokens for the respective client.

The above is still non-normative. Perhaps:

      New access tokens issued by the authorization server MUST replace
      previously issued access tokens for the respective client.

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.

See above. Basically, we now require PoP tokens. (An alternative type of
access token would have been indicated with the token_type claim as
defined in the framework document.)

Sounds good.

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

The reason for this seemingly mysterious statement is that it is
very difficult to define a strict order on access tokens that works
for all intended use cases the protocol has initially been designed
for. This was discussed in [3].

   [3] https://mailarchive.ietf.org/arch/msg/ace/x9JFbn0a6CRdsIwmQdKReGu5q1E/

The only resolution the working group had agreed on by then was to treat
use the last token that was received as the newest. There is some text
in the framework that tells how the resource server might be able to
order access tokens. The most general would be to use the cti claim,
therefore the following text has been added to clarify this:

NEW:

   The authorization server may, e.g., specify a "cti" claim for the
   access token (see Section 5.8.3 of [I-D.ietf-ace-oauth-authz] to
   employ a strict order.

Its all getting hazy here for me. I trust you.

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.

Good point, this is indeed very odd. The structure now has been changed
to include a subsection "Access Token Retrieval from the Authorization
Server" for each of the two modes where the interaction between client
and authorization server is described.

I think it seems better now.

        Thanks,
        Paul

_______________________________________________
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art

Reply via email to