Last Call Review of draft-ietf-ace-key-groupcomm-17
review-ietf-ace-key-groupcomm-17-genart-lc-fossati-2023-10-13-00

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://wiki.ietf.org/en/group/gen/GenArtFAQ>.

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

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

* `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
```
?

Editorial comments:

* terms and concept that readers are expected to be familiar with should
  also include RFC8610

* 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.)

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://github.com/ace-wg/ace-key-groupcomm/pull/156