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 -18 of the document.
Thanks,
/Marco
[PR] https://github.com/ace-wg/ace-key-groupcomm/pull/158
On 2023-10-13 14:17, Thomas Fossati via
Datatracker wrote:
Reviewer: Thomas Fossati Review result: Ready with Issues I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please treat these comments just like any other last call comments. For more information, please see the FAQ at <https://eur05.safelinks.protection.outlook.com/?url="">. Document: draft-ietf-ace-key-groupcomm-?? Reviewer: Thomas Fossati Review Date: 2023-10-13 IETF LC End Date: 2023-10-20 IESG Telechat date: Not scheduled for a telechat Summary: This document describes how to do secure group communication management using ACE. The document does not directly define a protocol. Rather, it provides a framework - in terms of message formats and a comprehensive REST API (plus the associated IANA machinery) - that individual secure group communication protocols are supposed to instantiate for their purposes, and according to their specific needs. The document is very clearly written. Kudos to the editors. From a Gen-ART perspective this work is basically ready, except a couple of minor CDDL-related issues described below. IANA-wise, all registrations to existing registries as well as the creation of new registries look good. DE instructions are also excellent, thanks for that. One question I have related to error handling: Have you considered reusing the RFC9290 framework instead of putting `error` and `error_description` in application/ace-groupcomm+cbor? * it is (quite) common practice to use a separate "problem details" format (e.g., see ACME, JMAP, SCITT, and CT) because of the advantage of using a single interface for *all* 4/5.xx responses, not just those that are protocol-specific * it is similarly compact (but allows extra information to be added, if needed) * it is not limited to English/ASCII text
==>MT
Thank you for this, Thomas! Good point. Just to give you a reasoning on why this hadn't been done before:
* When publication of this document was requested in February 2022, RFC 9290 (back then draft-ietf-core-problem-details) had been dormant for a long while and was focused only on CoRAL.
* For expressing errors, we reapplied the same rationale used by the ACE framework and in turn built on OAuth (see RFC 9200).
However we agree with you that, having this now, it makes sense to use it.
So we'll make the change that error responses will use application/concise-problem-details+cbor (ID 257) defined in RFC 9290, as you suggest.
As a consequence, we will also register a new IANA parameter in the “Custom Problem Detail Keys” registry of RFC 9290, and use that parameter to carry the information that is now in the Error Identifiers (Section 9).
Please see the PR: https://github.com/ace-wg/ace-key-groupcomm/pull/158
<==
Major issues:
None
Minor issues:
Two small things related to the CDDL:
* RFC 9237 has `AIF-Generic` rather than `AIF_Generic`
* also maybe add `;# include rfc9237` to each CDDL fragment to make it
self-consistent - or add the definition of AIF-Generic at the top
==>MT
Indeed, thank you.
Now fixed: https://github.com/ace-wg/ace-key-groupcomm/commit/f25853c477a1f82439194ce61c1fdc1ecd04ac1f
<==
* `scope = << [ + scope_entry ] >>` is not valid CDDL (`<< >>` is for embedding CBOR items AFAIK, not CDDL.) Do you mean instead: ``` scope_entries = [ + scope_entry ] scope = bstr .cbor scope_entries ``` ?
==>MT
Good catch!
Now fixed: https://github.com/ace-wg/ace-key-groupcomm/commit/bae81ff88d0caf4d58434cd1c1f8383c9d24de5b
<==
Editorial comments: * terms and concept that readers are expected to be familiar with should also include RFC8610
==>MT
Now added: https://github.com/ace-wg/ace-key-groupcomm/commit/88e62933c490588fb590536e116e23ec1cdcbf38
<==
* there are a few occurrences of this pattern: "the invariant once established identifier [...]", which may be easier to parse if rewritten as: "the identifier [...]. Once established, it never changes / it is invariant." (unless I have completely missed the meaning.)
==>MT
You have not missed the meaning, now changed: https://github.com/ace-wg/ace-key-groupcomm/commit/2a3065a526818cb688914ca10c27ffe5e5874e23
<==
Nits: A bunch of typos fixed in [1] - which also has (in a separate commit) a SVG-ised version of the architectural diagram for fancier rendering in HTML and PDF :-) [1] https://eur05.safelinks.protection.outlook.com/?url="">
==>MT
Thank you very much! Now merged.
<==
-- 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
Attachment:
OpenPGP_0xEE2664B40E58DA43.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
