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-art@xxxxxxxx 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: +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
