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.til...@ri.se> 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.til...@ri.se> <marco.til...@ri.se> > > > > 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 > >
_______________________________________________ Ace mailing list -- ace@ietf.org To unsubscribe send an email to ace-le...@ietf.org
