[Gen-art] Genart last call review of draft-ietf-ace-mqtt-tls-profile-15

[Theresa Enghardt via Datatracker <[e-mail]>]

Reviewer: Theresa Enghardt
Review result: Ready with Nits

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://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-ace-mqtt-tls-profile-15
Reviewer: Theresa Enghardt
Review Date: 2022-03-02
IETF LC End Date: 2022-03-03
IESG Telechat date: Not scheduled for a telechat

Summary: This document is fairly clear and concise, and basically ready for
publication. I found a few minor points and nits that should be addressed to
further enhance document clarity.

Major issues: None.

Minor issues:

Section 1:

"The token MAY be a
   reference or JSON Web Token (JWT) [RFC7519]."
What is a reference in this context?

Section 1.3:

"Will
                   If the network connection is not closed normally, […]"
I suggest to make this a bit more specific:
Does "the network connection" refer to a TCP connection, or a TLS session? Or
does it refer to MQTT's notion of "connection"? Does "not closed normally" mean
anything other than a FIN-ACK exchange to close a TCP connection? Or does it
depend on the used transport protocol (however, in this document, it should all
refer to TLS over TCP iiuc?) If the notion of a "network connection is not
closed normally" is a well-defined concept in this context, please provide a
reference if possible.

Section 2

"Whatever protocol is
   used for C-AS and Broker-AS communications must provide mutual
   authentication, confidentiality protection, and integrity protection."
Is this a MUST?

Section 2.1

"The PoP token includes a 'cnf' parameter with a
   symmetric or asymmetric PoP key. "
The 'cnf' (and 'rs_cnf' in Section 2.2.1) parameter is mentioned here and in
some other places, but it is not obvious what it means and why it is
special/important. I suggest to provide a brief explanation or reference.

2.2.2
"If the QoS level is equal to 0, and the token is invalid or the
   claims cannot be obtained in the case of an introspected token, the
   Broker MUST send a DISCONNECT packet with the reason code '0x87 (Not
   authorized)'."
Does this paragraph apply to all packets or is it specific to the PUBLISH
packet (like the next paragraph)? I suggest to make this explicit.

Section 2.2.4.1

"Given that clients provide a token at each connection,"
Does "at each connection" mean in each CONNECT packet, or something else?
Please clarify.

Nits/editorial comments:

Section 1:

"In this profile, Clients and Servers
   (Brokers) use MQTT to exchange Application Messages.  The protocol
   relies on TLS for communication security between entities."
Section 1.3 defines MQTT over TLS as MQTTS, and if I understand correctly, this
document requires MQTT between Clients and Servers over TLS, effectively,
making the document about MQTTS, and it does say "MQTTS" in some places. Short
of substituting all or nearly all "MQTT" with "MQTTS", is it worth mentioning
"MQTTS" once more here in the introduction?

"MQTT v3.1.1 clients"
Here, "clients" is lower case, while it is upper case in most other places in
the doc. Should it be upper case here as well? There are other occurences of
lower case "client" in Section 2.2.3.1, 2.2.3.2, 2.2.4.1, 2.2.5 - should these
all be upper case?

"This
   document does not protect the payload of the PUBLISH packet from the
   Broker."
Maybe this reads better as "The mechanisms specified in this document do not
protect […]"?

Section 2.1:
"The document follows the procedures
   defined in Section 3.2.1 of the DTLS profile
   [I-D.ietf-ace-dtls-authorize] for RPK, and in Section 3.3.1 of the
   same document for PSK."
This is the first occurrence of RPK and PSK, yet, these acronyms are only
expanded later in Section 2.2.1 and 2.2.3. I suggest move the expansion and
maybe a brief explanation to Section 2.1, and then perhaps the acronyms do not
need to be expanded again in Sections 2.2.1 and 2.2.3.

Section 8:
"Broker does not cache
   any invalid tokens."
The broker?