Last Call Review of draft-ietf-ace-mqtt-tls-profile-14
review-ietf-ace-mqtt-tls-profile-14-artart-lc-mahoney-2022-03-04-00

This document is ready but has minor issues with wording and references.

Minor issues:

Section 1: The subject seems to be missing in the following sentence.
Should "recommended" be normative?

Current:
   The Client-AS and RS-AS MAY also use protocols other
   than HTTP, e.g.  Constrained Application Protocol (CoAP) [RFC7252] or
   MQTT; it is recommended that TLS is used to secure these
   communication channels between Client-AS and RS-AS.

Perhaps (add "exchanges", split into two sentences):
   The Client-AS and RS-AS exchanges MAY also use protocols other
   than HTTP, e.g., Constrained Application Protocol (CoAP) [RFC7252] or
   MQTT. It is recommended that TLS is used to secure these
   communication channels between Client-AS and RS-AS.

Section 2.2.4.2.2: I'm having difficulty parsing the following normative
statements. Does "MUST" also cover the Authentication Data (i.e., MUST be set
to "ace" and MUST contain Authentication Data)? If the Authentication Data MUST
NOT be empty, MUST it contain the 8-byte RS nonce or could it contain something
else?

Current:
   The AUTH packet to continue authentication includes the
   Authentication Method, which MUST be set to "ace" and Authentication
   Data.  The Authentication Data MUST NOT be empty and contains an
   8-byte RS nonce as a challenge for the Client (Figure 6).

Section 4: I'm having difficulty parsing the following. Is it talking about
using a challenge from the current TLS session?

Current:
   To re-authenticate, the
   Client MUST NOT use Proof-of-Possession using a challenge from the
   TLS session during the same TLS session to avoid re-using the same
   challenge value from the TLS-Exporter.

Section 6.1: Could more guidance or examples of necessary policies be provided
here?

Current:
   However, stored Session state can be discarded as a
   result of administrator policies, and Brokers SHOULD implement the
   necessary policies to limit misuse.

References: idnits identifies the following issues. The three informative RFCs
are already listed in the downrefs registry, though
(https://datatracker.ietf.org/doc/downref).

  == Unused Reference: 'I-D.ietf-ace-oauth-params' is defined on line 1499,
     but no explicit reference was found in the text

  == Unused Reference: 'RFC7251' is defined on line 1563, but no explicit
     reference was found in the text

  == Unused Reference: 'RFC8422' is defined on line 1609, but no explicit
     reference was found in the text

  == Unused Reference: 'RFC8705' is defined on line 1625, but no explicit
     reference was found in the text

  -- Possible downref: Non-RFC (?) normative reference: ref.
     'MQTT-OASIS-Standard'

  -- Possible downref: Non-RFC (?) normative reference: ref.
     'MQTT-OASIS-Standard-v5'

  ** Downref: Normative reference to an Informational RFC: RFC 6234

  ** Downref: Normative reference to an Informational RFC: RFC 7251

  ** Downref: Normative reference to an Informational RFC: RFC 8032

  == Outdated reference: A later version (-04) exists of
     draft-ietf-ace-pubsub-profile-01

Appendix A: Perhaps add a reference to [I-D.ietf-ace-oauth-authz] for the
origin of the checklist.

Nits:

Section 1: The following list is somewhat hard to read.

Current:
   Implementations
   MAY also use "application/ace+cbor" content type, and CBOR encoding
   [RFC8949], and CBOR Web Token (CWT) [RFC8392] and associated PoP
   semantics to reduce the protocol memory and bandwidth requirements.

Perhaps:
   To reduce protocol memory and bandwidth requirements, implementations
   MAY also use 'application/ace+cbor' content type, CBOR encoding
   [RFC8949], and CBOR Web Token (CWT) [RFC8392] with its associated PoP
   semantics.

Section 2.2.3.2: Would it clarify the normative text to split this sentence
into two?

Current:
   ...a client MAY omit support for the cipher suites
  TLS_PSK_WITH_AES_128_CCM_8 and TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8,
   but for TLS 1.2, MUST support TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256
   for PSK and TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 for RPK, as
   recommended in [RFC7525] (and adjusted to be a PSK cipher suite as
   appropriate).

Perhaps:
   ...a client MAY omit support for the cipher suites
   TLS_PSK_WITH_AES_128_CCM_8 and TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8.
   For TLS 1.2, however, a client MUST support ...

Section 2.3: "an an exact" / "an exact"

Section 2.4.1: Does the authentication check fail for any of these cases?

Current:
   If the Client does not provide a valid token or omits the
   Authentication Data field and the Broker has no token stored for the
   Client or the token or Authentication data are malformed, or if the
   Will flag is set, the authorization checks for the Will topic fails,
   authentication fails.

Perhaps:
   Authentication can fail for the following reasons:

   * the Client does not provide a valid token or omits the Authentication
     Data field,

   * the Broker has no token stored for the Client,

   * the token or Authentication data are malformed, or

   * if the Will flag is set, the authorization check for the Will topic fails.

Section 3.3: "has a token token permits" / "has a token that permits"

Section 5: Would the following be clearer as a list?

Current:
   The MQTT session state is
   identified by the Client Identifier and includes state on client
   subscriptions; messages with QoS levels 1 and 2, and which have not
   been completely acknowledged or are pending transmission to the
   Client; and if the Session is currently not connected, the time at
   which the Session will end and Session State will be discarded.

Perhaps:
   The MQTT session state is identified by the Client Identifier and
   includes the following:

   * Client subscription state,

   * messages with QoS levels 1 and 2 that either have not been
     completely acknowledged or are pending transmission to the
     Client, and

   * if the Session is currently not connected, the time at which the
     Session will end and Session State will be discarded.

Section 6: Perhaps switching the order here would improve readability:

Current:
   ... MQTT v5.0 clients are NOT
   RECOMMENDED to use the flows described in this section.

Perhaps:
   ... the flows described in this section are NOT RECOMMENDED for
   use by MQTT v5.0 clients.

Figure 11: endoded / encoded

Section 6.2:  "and not attempt" / "and do not attempt"

              "no more authorized" / "no longer authorized"

Section 7.2: The spacing after the colons is inconsistent.

Formatting and terminology:

According to Wikipedia, MQTT is no longer considered an acronym (i.e., it no
longer needs to be expanded).

Could the terms "RS" and "Broker" be consistently scoped? That is, "RS" is used
when talking about OAuth interactions, and "Broker" is used when talking about
MQTT interactions? (From Section 1: "In the rest of the document, the terms
"RS", "MQTT Server" and "Broker" are used interchangeably.)

Check the use of single and double quotation marks, for instance:

   'application/ace+json' vs "application/ace+cbor"
   '0x18 (Cont. Authentication)' vs "0x18 (Continue Authentication)"

Because the acronym for "proof-of-possession" was given in the introduction,
the rest of the instances can be replaced with "PoP". Same with other acronyms
used, like PSK. Just expand on first use and use the acronym thereafter.