[Ace] Re: Opsdir last call review of draft-ietf-ace-revoked-token-notification-06

[Marco Tiloca <[e-mail]>]

Hello Dhruv,

Thanks a lot for your review! Please find in line below our detailed 
replies to your comments.

A Github PR where we have addressed your comments is available at [PR].

Unless any concern is raised, we plan to soon merge this PR (and the 
other ones related to other received reviews), and to submit the result 
as version -07 of the document.

Thanks,
/Marco

[PR] https://github.com/ace-wg/ace-revoked-token-notification/pull/9

On 2024-04-01 19:51, Dhruv Dhody via Datatracker wrote:
> Reviewer: Dhruv Dhody
> Review result: Has Issues
>
> # OPSDIR review of draft-ietf-ace-revoked-token-notification-06
>
> I have reviewed this document as part of the Operational directorate's ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments were written with the intent of improving the operational aspects of
> the IETF drafts. Comments that are not addressed in the last call may be
> included in AD reviews during the IESG review.  Document editors and WG chairs
> should treat these comments just like any other last-call comments.
>
> The document provides a mechanism for the Authorization Server to notify about
> revoked access tokens via access to a Token Revocation List (TRL) on the
> Authorization Server via CoAP. The document is clear and well-written. The
> motivation is described well. The document is almost ready for publication.
>
> ## Operational Concern
> - I am worried that the admin authorization is kept out of scope. I could not
> find how it is handled in the ACE framework and if this is acceptable. At the
> very least some hint or example can be provided... ````
>     *  Administrator: entity authorized to get full access to the TRL at
>        the AS, and acting as a requester towards the TRL endpoint.  An
>        administrator is not necessarily a registered device as defined
>        above, i.e., a Client requesting access tokens or an RS consuming
>        access tokens.  How the administrator authorization is established
>        and verified is out of the scope of this specification.
> ````

==>MT

The ACE framework (RFC 9200) does not define the concept of 
"administrator", as it is not part of the main workflow where a Client 
requests an access token from an AS to be consumed by an RS.

The concept of "administrator" was introduced here as an entity that can 
have full access to the TRL, i.e., all the access tokens that the AS 
issues pertain to an administrator.

On the topic of authorized administrators, the document has some text, 
but admittedly quite short and buried in Section 13.1 of the Security 
Considerations, i.e.:

 > Therefore, the AS MUST ensure that, other than registered devices 
accessing their own pertaining subset of the TRL, only authorized and 
authenticated administrators can retrieve the full TRL. To this end, the 
AS may rely on an access control list or similar.


In order to address this comment, we have made the following changes:


* In the quoted definition of "administrator" in Section 1.1, we have 
removed the sentence "How the administrator authorization is established 
and verified is out of the scope of this specification."


* In Section 5, we have extended the third paragraph as follows:

    OLD:
    > The AS MUST implement measures to prevent access to the TRL 
endpoint by entities other than registered devices and authorized 
administrators.

    NEW:
    > The AS MUST implement measures to prevent access to the TRL 
endpoint by entities other than registered devices and authorized 
administrators (see Section 9).


* In Section 9, we have added the following new paragraph, before the 
current last paragraph "Further details about ...":

    NEW:
    > When communicating with one another, the registered devices and 
the AS have to use a secure communication association and be mutually 
authenticated (see Section 5 of [RFC9200]).
    >
    > In the same spirit, it MUST be ensured that communications between 
the AS and an administrator are mutually authenticated, encrypted and 
integrity protected, as well as protected against message replay.
    >
    > Before starting its registration process at the AS, an 
administrator has to establish such a secure communication association 
with the AS, if they do not share one already. In particular, mutual 
authentication is REQUIRED during the establishment of the secure 
association. To this end, the administrator and the AS can rely, e.g., 
on establishing a TLS or DTLS secure session with mutual authentication 
[RFC8446][RFC9147], or an OSCORE Security Context [RFC8613] by running 
the authenticated key establishment protocol EDHOC [RFC9528].
    >
    > When receiving authenticated requests from the administrator for 
accessing the TRL endpoint, the AS can always check whether the 
requester is authorized to take such a role, i.e., to access the full TRL.
    >
    > To this end, the AS may rely on a local access control list or 
similar, which specifies the authentication credentials of trusted, 
authorized administrators. In particular, the AS verifies the requester 
to the TRL endpoint as an authorized administrator, only if the access 
control list includes the same authentication credential used by the 
requester when establishing the mutually-authenticated secure 
communication association with the AS.


* In Section 13.1, we have made the following rephrasing:

   OLD:
   > Therefore, the AS MUST ensure that, other than registered devices 
accessing their own pertaining subset of the TRL, only authorized and 
authenticated administrators can retrieve the full TRL. To this end, the 
AS may rely on an access control list or similar.

   NEW:
   > Therefore, the AS MUST ensure that, other than registered devices 
accessing their own pertaining subset of the TRL, only authorized and 
authenticated administrators can retrieve the full TRL (see Section 9).

<==

> - Section 5.1, if the TRL maintains MAX_N items only and if the diff is larger
> than that wouldn't some of the diffs be lost when the requester finally makes a
> request? I would have assumed that in such a case we indicate it that to the
> requester and it could issue a full query instead. But MAX_N is applicable for
> full query as well. Perhaps that is okay for most deployments, but let's be
> explicit about that in the document.

==>MT

First, we would like to clarify a few details.

The TRL maintains token hashes. The number of token hashes stored in the 
TRL is capped by the number of access tokens that are issued by the AS 
and are not expired yet.

Also, MAX_N is not related to full queries, since it plays a role only 
for diff queries.

That is, MAX_N is a single, constant value at the AS, and indicates the 
maximum number of TRL updates that the AS maintains for each different 
administrator or registered device (i.e., the updates occurred to the 
portion of the TRL pertaining to that administrator or registered 
device). These updates are maintained in a dedicated, per-device update 
collection, whose maximum size is MAX_N.

Eventually, the number of occurred updates pertaining to a device can 
indeed become more than MAX_N. From then on, due to the size of that 
device's update collection limited to MAX_N, the AS will discard an 
update from the update collection when that update becomes the oldest 
one in the collection. This process is defined in step 5 of Section 5.1, 
as part of updating an update collection with the insertion of a new 
series item:

 > 5. If the update collection associated with the requester currently 
includes MAX_N series items, the AS MUST delete the oldest series item 
in the update collection.
 >
 >    This occurs when the number of TRL updates pertaining to the 
requester and currently stored at the AS is equal to MAX_N.


That said, the following focuses on the raised issue.

If a diff query request does not use the "Cursor" extension [0] (e.g., 
the extension is not supported by the device or the AS), then indeed the 
device may never notice through diff queries that an update has occurred 
and has been lost forever. For example, the following can happen:

1. An access token TOKEN is issued as pertaining to a device DEVICE. 
Let's say that TOKEN has an expiration time that is very far in the future.

2. After some time, DEVICE sends a diff query request to the AS. The 
diff query request does not use the "Cursor" extension, and is not a 
CoAP Observation request. In the follow-up response, the AS provides 
DEVICE with the current items from the related update collection.

3. TOKEN gets revoked, and the AS adds to the update collection of 
DEVICE an item ITEM that reflects the revocation of TOKEN (i.e., it 
reflects the addition of the token hash to the TRL).

4. Due to several revocations and follow-up expiration of access tokens 
pertaining to DEVICE, the AS adds a lot of new series items to the 
update collection related to DEVICE.

5. Eventually, the update collection reaches size MAX_N. Later on, ITEM 
becomes the oldest series item. Further later, a new series item is 
added to the update collection, which results in deleting ITEM from the 
update collection.

6. DEVICE sends a new diff query request to the AS. By doing so, DEVICE 
retrieves all the current series items in its related update collection, 
but does not learn that TOKEN was revoked and thus keeps storing it.

[0] See "Case A" in Section 8.2.3. If the diff query request uses the 
"Cursor" extension and its 'cursor' query parameter has a value that 
refers to a very old, deleted series item, then the follow-up response 
from the TRL informs the requester that what is being requested was 
deleted and cannot be provided. After that, as also stated in Section 
8.2.3, the requester should indeed send a full query request to the TRL 
endpoint.


In order to address your comment and situations like the above, we have 
added the following new text at the end of Section 10.0 "Notification of 
Revoked Access Tokens".

 > As specified in Section 5.1, an AS supporting diff queries maintains 
an update collection of maximum MAX_N series items for each 
administrator or registered device, hereafter referred to as requester. 
In particular, if an update collection includes MAX_N series items, 
adding a further series item to that update collection results in 
deleting the oldest series item from that update collection.
 >
 > From then on, the requester associated with the update collection 
will not be able to retrieve the deleted series item, when sending a new 
diff query request to the TRL endpoint. If that series item reflected 
the revocation of an access token pertaining to the requester, then the 
requester will not learn about that when receiving the corresponding 
diff query response from the AS.
 >
 > Sending a diff query request specifically as an Observation request, 
and thus relying on Observe notifications, largely reduces the chances 
for a requester to miss updates occurred to its associated update 
collection altogether. In turn, this relies on the requester 
successfully receiving the Observe notification responses from the TRL 
(see also Section 13.3).
 >
 > In order to limit the amount of time during which the requester is 
unaware of pertaining access tokens that have been revoked but are not 
expired yet, a requester SHOULD NOT rely solely on diff query requests. 
In particular, a requester SHOULD also regularly send a full query 
request to the TRL endpoint according to a related application policy.


Furthermore, we have also turned a "should" into a normative "SHOULD", 
in Section 8.2.3 when specifying the "Case A" mentioned above. That is:

OLD:
 > When receiving this diff query response, the requester should send a 
new full query request to the AS.

NEW:
 > When receiving this diff query response, the requester SHOULD send a 
new full query request to the AS.

<==

>
> ## Minor
> - Thanks for including the description of "endpoint" in Section 1.1 but the
> term is used in the Abstract and Introduction where it confused me on first
> reading. I suggest adding a forward reference in the Introduction to avoid
> confusion. Also in section 1.1, it would be better if we have a reference to
> the OAuth document where the term originates from.

==>MT

We have rephrased as follows:

OLD (Section 1.0):
 > ... and it does not require any additional endpoints on the 
registered devices.

NEW (Section 1.0):
 > ... and it does not require the registered devices to support any 
additional endpoints (see Section 1.1).

OLD (Section 1.1):
 > Note that, unless otherwise indicated, the term "endpoint" is used 
here following its OAuth definition, aimed at ...

NEW (Section 1.1):
 > Note that the term "endpoint" is used here following its OAuth 
definition [RFC6749], aimed at ...

<==

> - Is there a suitable
> reference for "CBOR diagnostic notation"?

==>MT

Indeed. As part of a planned clarification on this point, we have 
rephrased the last paragraph of Section 1.1 as follows:

OLD:
 > Examples throughout this document are expressed in CBOR diagnostic 
notation without the tag and value abbreviations.

NEW
 > Examples throughout this document are expressed in CBOR diagnostic 
notation as defined in Section 8 of [RFC8949] and Appendix G of 
[RFC8610]. Diagnostic notation comments are often used to provide a 
textual representation of the numeric parameter names and values.

<==

> - Section 3, I suggest mentioning
> that 0x58 identifies byte string with one-byte for n.

==>MT

When taking into account a comment from the GENART review, we have 
revised this section and updated the definition of HASH_INPUT.

The updated design considers the **value** of the CBOR byte string 
conveyed by the parameter 'access_token' in Figure 2.

Consequently, from the example in Figure 2, the bytes (0xd0 0x83 ...) 
are mentioned as relevant to understand how to build HASH_INPUT, hence 
the byte 0x58 is not mentioned anymore.

<==

> - Section 5.1 "The AS
> SHOULD provide requesters with the value of MAX_N, upon their registration",
> how about we make it a MUST and quality it with if diff is supported?

==>MT

This change looks good.

The use of SHOULD was paired with the implicit exception "unless 
registered devices are pre-configured with the value of MAX_N and the AS 
is aware of that".

We have made the following changes:

OLD (Section 5.1):
 > The AS SHOULD provide requesters with the value of MAX_N, upon their 
registration (see Section 9).

NEW (Section 5.1):
 > If the AS supports diff queries, the AS MUST provide requesters with 
the value of MAX_N, upon their registration (see Section 9).

OLD (Section 9):
 > * Optionally, a positive integer MAX_N, if the AS supports diff 
queries of the TRL (see Section 5.1 and Section 7).

NEW (Section 9):
 > * A positive integer MAX_N, if the AS supports diff queries of the 
TRL (see Section 5.1 and Section 7).

<==

> - Section
> 5.1.1 "If supporting the "Cursor" extension, the AS SHOULD provide registered
> devices and administrators with the value of MAX_DIFF_BATCH...", why SHOULD? I
> think this should be a MUST!

==>MT

This change looks good.

Like for the previous comment about MAX_N, the use of SHOULD was paired 
with the implicit exception "unless registered devices are 
pre-configured with the value of MAX_DIFF_BATCH and the AS is aware of 
that".

Please note that the functionality and correctness of the protocol do 
not depend on the registered devices being aware of the value of 
MAX_DIFF_PATCH or not. Its impact only influences the number of update 
entries included in a same response from the TRL endpoint at the AS.

However, in the same spirit of the change above for MAX_N, providing 
registered devices with MAX_DIFF_BATCH is also an explicit indication of 
the AS supporting the "Cursor" extension of diff queries, and sets 
expectations about the number of update entries to expect in a same 
response from the TRL endpoint.

We have made the following changes:

OLD (Section 5.1):
 > If supporting the "Cursor" extension, the AS SHOULD provide 
registered devices and administrators with the value of MAX_DIFF_BATCH, 
upon their registration (see Section 9).

NEW (Section 5.1):
 > If supporting the "Cursor" extension, the AS MUST provide registered 
devices and administrators with the value of MAX_DIFF_BATCH, upon their 
registration (see Section 9).

OLD (Section 9):
 > * Optionally, a positive integer MAX_DIFF_BATCH, if the AS supports 
diff queries of the TRL as well as the related "Cursor" extension (see 
Section 5.1.1 and Section 8).

NEW (Section 9):
 > * A positive integer MAX_DIFF_BATCH, if the AS supports diff queries 
of the TRL as well as the related "Cursor" extension (see Section 5.1.1 
and Section 8).

<==

> - Section 7, "SIZE <= MAX_N" but it is unclear
> what exactly SIZE is, please add explicit text.

==>MT

What follows in the original sentence is the definition of SIZE, i.e.:

 > The AS determines U = min(NUM, SIZE), where SIZE <= MAX_N is the 
number of TRL updates pertaining to the requester and currently stored 
at the AS.

To clarify, we have rephrased as follows:

NEW
 > The AS determines U = min(NUM, SIZE), where SIZE <= MAX_N. In 
particular, SIZE is the number of TRL updates pertaining to the 
requester and currently stored at the AS.

<==

> - Section 14.1, should the
> change controller be IETF instead?

==>MT

Indeed. As already planned for addressing the IANA considerations, we 
have made the following change.

OLD:
 > Author: Marco Tiloca <[e-mail]>
 >
 > Change controller: IESG

NEW:
 > Author/Change controller: IETF
 >
 > Provisional registration: No

<==

>
> ## Nits
> - Don't use "the CoAP protocol", the P in CoAP is already protocol!

==>MT

The document has two instances of "CoAP protocol", both of which are in 
Section 1.1.

We have fixed the instance in the fourth paragraph:

OLD:
 > the CoAP protocol [RFC7252], ...

NEW:
 > CoAP [RFC7252], ...

We have kept the other instance in the fifth paragraph, as it is a 
verbatim quote from Section 1.1 of the CoAP specification (RFC 7252) 
where the quoted definition of "CoAP endpoint" is provided:

 > This document does not use the CoAP definition of "endpoint", which 
is "An entity participating in the CoAP protocol."

<==

> - s/as interested to receive notifications/as interested in receiving
> notifications/ - s/recent updates occurred over the list/recent updates that
> occurred over the list/ - s/with value the token hash of an access token/with
> the value of the token hash of an access token/ - s/The processing of a full
> query and the related response format are defined in/The processing of a full
> query and the related response format is defined in/ - s/other than 0 or than a
> positive integer/other than 0 or a positive integer/ - s/registers at the AS
> until a first update/registers at the AS until the first update/ - s/pertaining
> to the requester occurred to the TRL./pertaining to the requester that occurred
> to the TRL./ - s/registers at the AS until a first update/registers at the AS
> until the first update/ - s/Once completed the registration procedure at the
> AS,/Once the registration procedure is completed at the AS,/ - s/the Client is
> not aware about that yet,/the Client is not aware of that yet,/

==>MT

We have made all the fixes above as suggested, except for the following 
ones:

* As to the suggestion

   > s/with value the token hash of an access token/with the value of 
the token hash of an access token/

   The full sentence is

   > Each element of the array is a CBOR byte string, with value the 
token hash of an access token ...

   We think that the current phrasing is correct. Please note that 
nowhere does the document use the expression "value of the token hash".


* As to the suggestion

   > s/other than 0 or than a positive integer/other than 0 or a 
positive integer/

   We have made a different rephrasing, to avoid any potential 
misunderstanding

   OLD:
   > has a value other than 0 or than a positive integer

   NEW:
   > has a value that is neither 0 nor a positive integer


* As to the suggestion

   > s/Once completed the registration procedure at the AS,/Once the 
registration procedure is completed at the AS,/

   We have made a different rephrasing, to be consistent with what was 
suggested in other reviews:

   OLD:
   > Once completed the registration procedure at the AS, ...

   NEW:
   > Once registered at the AS, ...


* As to the suggestion

   > s/the Client is not aware about that yet,/the Client is not aware 
of that yet,/

   We have made a different rephrasing, to be consistent with what was 
suggested in other reviews:

   OLD:
   > For example, the access token has actually been revoked and the 
Client is not aware about that yet, while the RS has gained knowledge 
about that and has expunged the access token.

   NEW:
   > For example, the access token has been revoked, and the RS has 
become aware of it and has expunged the access token, but the Client is 
not aware of it (yet).

<==

>
>

-- 
Marco Tiloca
Ph.D., Senior Researcher

Phone: [phone]

RISE Research Institutes of Sweden AB
Box 1263
164 29 Kista (Sweden)

Division: Digital Systems
Department: Computer Science
Unit: Cybersecurity

https://www.ri.se