Hello Joerg,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/7 On 2024-04-08 22:13, Joerg Ott via Datatracker wrote:
Reviewer: Joerg Ott Review result: Ready with Nits This document has been reviewed as part of the transport area review team's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors and WG to allow them to address any issues raised and also to the IETF discussion list for information. When done at the time of IETF Last Call, the authors should consider this review as part of the last-call comments they receive. Please always CC tsv-...@ietf.org if you reply to or forward this review. This document defines the interactions and message format for TRL endpoints to inquire token revocation lists (TRLs) or be notified about such from/by an authorisation server (AS). The specification defines the message exchanges on top of established application protocols such as CoAP. It supports retrieving full TRLs or (recent) changes to such TRLs. Being based upon existing protocols and carefully addressing resource constraints that endpoints might have, the protocol does not appear to introduce new transport layer issues to be considered. Looks good to me from this perspective. Found a few minor nits: sect 1, 4th para: "if they are defined to use in the ACE framework" doesn't quite seem to parse.
==>MT We have rephrased as follows: OLD> Other underlying protocols than CoAP are not prohibited from being supported in the future, if they are defined to use in the ACE framework for Authentication and Authorization.
NEW (emphasis mine)> Other underlying protocols than CoAP are not prohibited from being supported in the future, if they are defined **to be used** in the ACE framework for Authentication and Authorization.
<==
sect 5, 2nd para: "the AS sends as response use Content-Format"
==>MT We have rephrased as follows: OLD> Following a request to the TRL endpoint, the messages defined in this document that the AS sends as response use Content-Format "application/ace-trl+cbor".
NEW (emphasis mine)> Following a request to the TRL endpoint, **the corresponding response messages sent by** the AS use Content-Format "application/ace-trl+cbor".
<==
sect 5, 8th para: "ii) they were added to or removed from that update." The previous part of this para talks about multiple updates, the previous para about the most recent one. What does "that update" refer to?
==>MTIt is an update to the TRL, which becomes relevant if specifically affecting the subset of the TRL pertaining the requester.
We have rephrased the 7th and 8th paragraphs as follows. OLD> Diff query: the AS returns a list of diff entries. Each diff entry is related to one of the most recent updates, in the subset of the TRL pertaining to the requester.
>> The entry associated with one of such updates contains a list of token hashes, such that: i) the corresponding revoked access tokens pertain to the requester; and ii) they were added to or removed from the TRL at that update.
NEW (emphasis mine)> Diff query: the AS returns a list of diff entries. Each diff entry is related to one of the most recent updates **to the TRL, with such an update performed** in the subset of the TRL pertaining to the requester.
>> The entry associated with one of such updates contains a list of token hashes, such that: i) the corresponding revoked access tokens pertain to the requester; and ii) they were added to or removed from the TRL **when performing that update to the TRL**.
<==
The discussion of the diff update procedure in sect 5.1.1 may not be that easy to follow, but maybe it is better understandable if one writes code.
==>MTPlease note that Section 5.1.1 considers the optional extension "Cursor" of the diff queries, and introduces the corresponding constants 'MAX_INDEX' and 'MAX_DIFF_BATCH', as well as the variables 'index' and 'last_index'.
The actual, related processing for the AS to produce a response is specified later in Section 8.
<==
sect 5.2: 11th para: "The 4.00 Bad Request ..." -- move this para after the indented bullet list? Seems to break the reading flow here.
==>MT Quoting the whole paragraph for context:> The 4.00 (Bad Request) response MUST have Content-Format "application/ace-trl+cbor". The payload of the response MUST be a CBOR map, which MUST include the 'error' parameter and MAY include the 'error_description' parameter to provide additional context.
We would prefer to keep the current structure, since each point in the bullet list refers to the 'error' parameter in the payload and specifies how to fill that parameter. So it's better if the payload format is reminded before the bullet list with the different cases.
The same holds also when switching to the Concise Problem Details (RFC 9290) format for error responses, now in the Pull Request at https://github.com/ace-wg/ace-revoked-token-notification/pull/6
<==
sect 7, last para before sect 8 refers to I-D.bormann-t2trg-stp. This draft seems to be expired, it is from a research group; does this reference help here and add value?
==>MT Quoting the whole paragraph for context:> Appendix A discusses how performing a diff query of the TRL is in fact a usage example of the Series Transfer Pattern defined in [I-D.bormann-t2trg-stp].
The intent in Section 7 is have a forward pointer to Appendix A (which, otherwise, would not be referred to in the document body) and to anticipate its content.
We believe that Appendix A provides additional information by positioning the present proposal in the context of the more general Series Transfer Pattern defined in draft-bormann-t2trg-stp, which is in fact an informative reference. We are referring to its latest version -03 and, even if that draft is currently expired, we think that its version -03 well serves the purpose intended in Appendix A of the present document.
<==
sect 9: is there a need to discuss any constraints of permutations of MAX_N and MAX_DIFF_BATCH?
==>MTAs explained below, no particular discussion is needed in Section 9, but it's better to improve the text in Section 5.1.0 to avoid confusion (see further below).
On the mentioned parameters:* There is a single, constant value MAX_N at the AS. That same value is used with all the registered devices and administrators. That is, for each registered device or administrator, the corresponding update collection includes at most MAX_N items.
* Instead, there is one constant value MAX_DIFF_BATCH (with MAX_DIFF_BATCH <= MAX_N) for each different registered device or administrator. The value might be different for different registered devices or administrators. As stated in Section 5.1.1, the specific value of MAX_DIFF_BATCHÂ used for a registered device or administrator "depends on the specific registered device or administrator associated with the update collection in question."
* The table in Appendix B provides an overview of these parameters, and highlights their nature as single-instance (like N_MAX) or per-requester (like MAX_DIFF_BATCH).
Admittedly, the text in Section 5.1.0 is not clear enough and might suggest that the AS uses one value of MAX_N for each registered device or administrators. Instead, as explained above and summarized in Appendix B, the intent is to have a single-instance value of MAX_N to use for all the registered devices and administrators.
We have clarified in Section 5.1.0: OLD> For each requester, the AS maintains an update collection of maximum MAX_N series items, where MAX_N is a pre-defined, constant positive integer.
NEW> The AS defines the single, constant positive integer MAX_N >= 1. For each requester, the AS maintains an update collection of maximum MAX_N series items.
For editorial consistency, a sentence in Section 5.1.1 has been updated as follows.
OLD> The AS defines the constant, unsigned integer MAX_INDEX <= ((2^64) - 1), ...
NEW (emphasis mine)> The AS defines the **single, constant** unsigned integer MAX_INDEX <= ((2^64) - 1), ...
<==
sect. 10, 1st para: "Once completed the registration procedure at the AS, the administrator..." doesn't quite parse.
==>MTBuilding on the simpler formulation in the first paragraph of Section 1, we have rephrased as follows.
OLD> Once completed the registration procedure at the AS, the administrator or registered device can send a GET request to the TRL endpoint at the AS.
NEW> Once registered at the AS, the administrator or registered device can send a GET request to the TRL endpoint at the AS.
<==
sect 10.1, 2ns para: "if the corresponding token hash is among the currently stored ones" [add: 'in the TRL"?]
==>MT Quoting the text for context.> When receiving a response from the TRL endpoint, a registered device MUST expunge every stored access token associated with a token hash specified in the response. In case the registered device is an RS, it MUST store the token hash.
>> An RS MUST NOT accept and store an access token, if the corresponding token hash is among the currently stored ones.
The addition of "in the TRL" is not pertinent to the RS here, since the TRL is a resource specifically at the AS. The RS is free to store the mentioned token hash as it see fits, e.g., within a data structure associated with its local token repository.
<==
Appendix C: I believe to have understood that one could have a retrieve operation without Observe. Should this include such a simple example, too?
==>MT Indeed one can access the TRL resource without using Observe. The examples are already showing that case. See in particular:* In Appendix C.3: the final request-response exchange, also mentioned in the third paragraph of the appendix.
* In Appendix C.4: the two final request-response exchanges, also mentioned in the fourth and fifth paragraphs of the appendix.
* In Appendix C.5: the two final request-response exchanges, also mentioned starting from the fourth paragraph of the appendix.
<==
-- 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
OpenPGP_0xEE2664B40E58DA43.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
_______________________________________________ Ace mailing list -- ace@ietf.org To unsubscribe send an email to ace-le...@ietf.org
