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

[Joerg Ott <[e-mail]>]

Hi Marco,

thank much for the updates and clarifications.  Looks good to me.

Best,
Jörg

On 10.05.24 17:01, Marco Tiloca wrote:
> 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
>> [e-mail]  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?
> 
> ==>MT
> 
> It 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.
> 
> ==>MT
> 
> Please 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?
> 
> ==>MT
> 
> As 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.
> 
> ==>MT
> 
> Building 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: [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
>