Thanks Marco for taking my comments into consideration and providing detailed feedback! LGTM!
On Fri, May 10, 2024 at 7:04 PM Marco Tiloca <marco.tiloca@xxxxx> wrote:
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 <marco.tiloca@xxxxx>
>
> 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: +46 (0)70 60 46 501 RISE Research Institutes of Sweden AB Box 1263 164 29 Kista (Sweden) Division: Digital Systems Department: Computer Science Unit: Cybersecurity https://www.ri.se
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
