Subject: [art] Artart last call review of draft-ietf-ace-mqtt-tls-profile-14

From: Jean Mahoney via Datatracker <[email-masked]>
Date: Fri, 04 Mar 2022 13:40:54 -0800

Reviewer: Jean Mahoney
Review result: Ready with Issues

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.

---

From: Cigdem Sengul <[email-masked]>
Date: Tue, 8 Mar 2022 18:36:49 +0000

Dear Jean,
Thank you for your review.
I implemented changes and prepared a pull request at:
https://github.com/ace-wg/mqtt-tls-profile/pull/102

Below is a summary of how I revised the text according to your
suggestions, and corrected references for this document (removing unused
references due to changes of text etc.) I have still kept MQTT as
normative, as the document is about MQTT, but is it expected to be
informative when the reference is a non-RFC?

Kind regards,
--Cigdem

>    communication channels between Client-AS and RS-AS.

[CS: Done.]

>    8-byte RS nonce as a challenge for the Client (Figure 6).

[CS: Revised as:
The Broker continues authentication using an AUTH packet that contains the
Authentication Method
and the Authentication Data. The Authentication Method MUST be set to
"ace", and the Authentication Data MUST NOT be empty and contain
an 8-byte RS nonce as a challenge for the Client.
]

>    challenge value from the TLS-Exporter.

[CS: Revised:
If re-authenticating during the current TLS session, the Client MUST NOT
use the method
   described in Section 2.2.4.2.1, Proof-of-Possession using a challenge
   from the TLS session, to avoid re-using the same challenge value from
   the TLS-Exporter.
]

>    necessary policies to limit misuse.

[CS: Revised as:
"However, stored Session state can be discarded as a result
of administrator action or policies (e.g. defining an automated
response based on storage capabilities), and Brokers SHOULD implement the
necessary policies to limit
misuse."

Would this work?
]

>      but no explicit reference was found in the text

[CS: This is fixed based on the revision for genart review, and the ID is
referenced.]

>      reference was found in the text

[CS: Reference removed; added RFC 8442 for the PSK cipher reference]

>      reference was found in the text

[CS: Now cited]

>      reference was found in the text
[CS: Removed as does not apply to the current text]

>   -- Possible downref: Non-RFC (?) normative reference: ref.

[CS: OK for these two, I feel on the fence as the document is about MQTT.
Is the downref suggested because this is a Non-RFC and a standard coming
from OASIS]

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

[CS: Moved to Informative references]

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

[CS: Reference removed]

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

 [CS: Moved to informative]

>      draft-ietf-ace-pubsub-profile-01

[CS: Fixed.]

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

[CS: Added:
"Based on the requirements on profiles for the ACE framework
   [I-D.ietf-ace-oauth-authz], this document fulfills the following: "

>    [RFC8949], and CBOR Web Token (CWT) [RFC8392] with its associated PoP

[CS: Fixed]

>    For TLS 1.2, however, a client MUST support ...

[CS: Fixed]

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

[CS: Fixed]

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

[CS: the conditions are separated at each "or", so revised as:
"Authentication can fail for the following reasons:

   o  If the Client does not provide a valid token,

   o  the Client omits the Authentication Data field and the Broker has
      no token stored for the Client,

   o  the token or Authentication data are malformed, or

   o  if the Will flag is set, the authorization checks for the Will
      topic fails."

> Section 3.3: "has a token token permits" / "has a token that permits"
[CS: Fixed]

>      Session will end and Session State will be discarded.

[CS: Done]

>    use by MQTT v5.0 clients.
[CS: Done]

> Figure 11: endoded / encoded

[CS: Fixed]

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

[CS: Fixed]

>               "no more authorized" / "no longer authorized"
[CS: Fixed]

> Section 7.2: The spacing after the colons is inconsistent.

[CS: Fixed to have all with a single space after the colon.]

> longer needs to be expanded).
[CS: Is it OK that we leave things as they are, as this will affect title
etc.]

> "RS", "MQTT Server" and "Broker" are used interchangeably.)

[CS: Revised as much as possible so that Broker is used when we refer to
the entity defined in this profile,
and RS is used more for generic OAuth interactions]

>    '0x18 (Cont. Authentication)' vs "0x18 (Continue Authentication)"

[CS: Switched to double quotes, but removed quotation marks around reason
codes as in the MQTT standard.]

> [CS: Done for PoP, was done for PSK/RPK for genart review.]

---

From: "A. Jean Mahoney" <[email-masked]>
Date: Wed, 9 Mar 2022 17:17:59 -0600

Cigdem,

Thank you for reply and for incorporating the feedback into a PR. I have 
few comments and questions below.

On 3/8/22 12:36 PM, Cigdem Sengul wrote:
> <https://github.com/ace-wg/mqtt-tls-profile/pull/102>
> 

[JM] I think that is okay. The document shepherd indicated in his 
writeup that he thinks the MQTT reference should be normative 
(https://datatracker.ietf.org/doc/draft-ietf-ace-mqtt-tls-profile/shepherdwriteup/). 

> 
> Kind regards,
> --Cigdem
> 
> On Fri, 4 Mar 2022 at 21:40, Jean Mahoney via Datatracker 
> <[email-masked] <mailto:[email-masked]>> wrote:
> 
>     Reviewer: Jean Mahoney
> The Broker continues authentication using an AUTH packet that contains 
> the Authentication Method

[JM] This is better, but the last half of the last sentence could be 
interpreted as saying "MUST NOT be empty and MUST NOT contain...". 
Perhaps it could just say "the Authentication Data MUST contain an 
8-byte RS nonce..."?

>     from the TLS session, to avoid re-using the same challenge value from

[JM] Much better, thanks!

> response based on storage capabilities), and Brokers SHOULD implement 
> the necessary policies to limit
> misuse."
> 
> Would this work?
> ]

[JM] I like the added example. As someone unfamiliar with MQTT, I was 
wondering what "necessary policies" might look like. That is, if I were 
to build a Broker implementation, what sort of things SHOULD my 
implementation be doing here? An example or two might clarify.

>     (https://datatracker.ietf.org/doc/downref
>     <https://datatracker.ietf.org/doc/downref>).
> 
> [CS: This is fixed based on the revision for genart review, and the ID 
> is referenced.]
> 
> 

[JM] The idnits script calls out any normative reference that is not a 
Standards Track RFC. Documents from other standards bodies can be 
approved by the IESG to be listed in the Normative References section. 
The MQTT refs shouldn't be an issue. More info about downrefs can be 
found in RFC 8067.

> [CS: Moved to Informative references]

[JM] RFC 6234 should be okay as a normative reference because it has 
been normatively referenced in other RFCs 
(https://datatracker.ietf.org/doc/rfc6234/referencedby/) and the 
following sentence says "mandatory to implement":

    HS256 (HMAC-SHA-256) [RFC6234] and Ed25519 [RFC8032] are mandatory
    to implement for the Broker.

> [CS: Reference removed] >
> 

[JM] Same as RFC 6234, RFC 8032 can be normative.

https://datatracker.ietf.org/doc/rfc8032/referencedby/

>     Section 2.2.3.2 <http://2.2.3.2>: Would it clarify the normative
>     text to split this sentence
>     into two?
> 
>     Current:
> [CS: Done] >
> [CS: Is it OK that we leave things as they are, as this will affect 
> title etc.]

[JM] That's fine. I was just highlighting that the expansion wasn't 
necessary.

> and RS is used more for generic OAuth interactions]

[JM] Thanks for working on this!

> [CS: Switched to double quotes, but removed quotation marks around 
> reason codes as in the MQTT standard.]
> 
> 

[JM] Thank you for addressing my comments!

Best regards,
Jean

> 
> _______________________________________________
> art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/art

---

From: Francesca Palombini <[email-masked]>
Date: Wed, 9 Mar 2022 23:27:40 +0000

Jean: thank you very much for your thoughtful review! Cigdem, thank you for addressing it, I do believe it improves the document.

Just one note: for the downref to informative documents (for those documents that were actually included in the text), please revert the change – RFC 6234 and RFC 8032 were correctly referenced as normative, since they are mandatory to implement. MQTT standards are also correctly referenced normatively. Jean was only noting these appear as downrefs, but downrefs are not “mistakes”, they just require more attention – see https://www.rfc-editor.org/info/bcp97 for more information. Downrefs are usually Last Called to make sure the community is aware of them, except for those that have appeared as downref for many documents and don’t need to be Last Called explicitly anymore: https://datatracker.ietf.org/doc/downref/.

Thanks,
Francesca

From: last-call <[email-masked]> on behalf of Cigdem Sengul <[email-masked]>
Date: Tuesday, 8 March 2022 at 19:37
To: Jean Mahoney <[email-masked]>
Cc: [email-masked] <[email-masked]>, [email-masked] <[email-masked]>, [email-masked] <[email-masked]>, Ace Wg <[email-masked]>
Subject: Re: [Last-Call] Artart last call review of draft-ietf-ace-mqtt-tls-profile-14
Dear Jean,
Thank you for your review.
I implemented changes and prepared a pull request at: https://github.com/ace-wg/mqtt-tls-profile/pull/102<https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-194406ae4b662912&q=1&e=122819e3-4d79-46e2-95cc-1afbf78f235b&u=https%3A%2F%2Fgithub.com%2Face-wg%2Fmqtt-tls-profile%2Fpull%2F102>

Below is a summary of how I revised the text according to your suggestions, and corrected references for this document (removing unused references due to changes of text etc.) I have still kept MQTT as normative, as the document is about MQTT, but is it expected to be informative when the reference is a non-RFC?

Kind regards,
--Cigdem

On Fri, 4 Mar 2022 at 21:40, Jean Mahoney via Datatracker <[email-masked]<mailto:[email-masked]>> wrote:
Reviewer: Jean Mahoney
Review result: Ready with Issues

Minor issues:

Current:

[CS: Done.]

Current:

[CS: Revised as:
The Broker continues authentication using an AUTH packet that contains the Authentication Method
and the Authentication Data. The Authentication Method MUST be set to "ace", and the Authentication Data MUST NOT be empty and contain
an 8-byte RS nonce as a challenge for the Client.
]

Current:
   To re-authenticate, the

[CS: Revised:
If re-authenticating during the current TLS session, the Client MUST NOT use the method
   described in Section 2.2.4.2.1, Proof-of-Possession using a challenge
   from the TLS session, to avoid re-using the same challenge value from
   the TLS-Exporter.
]

Current:

[CS: Revised as:
"However, stored Session state can be discarded as a result
of administrator action or policies (e.g. defining an automated
response based on storage capabilities), and Brokers SHOULD implement the necessary policies to limit
misuse."

Would this work?
]

[CS: This is fixed based on the revision for genart review, and the ID is referenced.]

[CS: Now cited]

[CS: OK for these two, I feel on the fence as the document is about MQTT.
Is the downref suggested because this is a Non-RFC and a standard coming from OASIS]

[CS: Reference removed]

[CS: Fixed.]

[CS: Added:

Nits:

Current:
   Implementations

Perhaps:

[CS: Fixed]

Section 2.2.3.2<https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-dd7b08654dc4d689&q=1&e=122819e3-4d79-46e2-95cc-1afbf78f235b&u=http%3A%2F%2F2.2.3.2%2F>: Would it clarify the normative text to split this sentence
into two?

Current:

Perhaps:

[CS: Fixed]

[CS: Fixed]

Current:

Perhaps:

Current:

Perhaps:

[CS: Done]

Current:

Perhaps:

[CS: Fixed]

[CS: Fixed]

According to Wikipedia, MQTT is no longer considered an acronym (i.e., it no
longer needs to be expanded).
[CS: Is it OK that we leave things as they are, as this will affect title etc.]

[CS: Revised as much as possible so that Broker is used when we refer to the entity defined in this profile,
and RS is used more for generic OAuth interactions]

[CS: Switched to double quotes, but removed quotation marks around reason codes as in the MQTT standard.]

---

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 9 Mar 2022 15:39:44 -0800

> Just one note: for the downref to informative documents (for those documents that were actually included in the text), please revert the change – RFC 6234 and RFC 8032 were correctly referenced as…

Exactly so.  Thanks for the quick clarification, Francesca!

-Ben

---

From: Cigdem Sengul <[email-masked]>
Date: Wed, 9 Mar 2022 23:41:51 +0000

Hello Jean,

Thank you for your feedback on the changes.
I have updated the pull request with a new commit:
https://github.com/ace-wg/mqtt-tls-profile/pull/102/commits/75ac0c0a86812f359471a63f6b481b0b80482b97

Responses to questions/comments are inline below.

On Wed, 9 Mar 2022 at 23:18, A. Jean Mahoney <[email-masked]> wrote:

> (
> https://datatracker.ietf.org/doc/draft-ietf-ace-mqtt-tls-profile/shepherdwriteup/).
>
>
> [CS: OK, will leave as is.]

> Perhaps it could just say "the Authentication Data MUST contain an

[CS: True, revised as your suggestion.]

> implementation be doing here? An example or two might clarify.

[CS: The example regarding defining an automated response based on storage
capabilities come from the MQTT standard.
The standard itself does not define policies explicitly. I changed the
wording from "necessary policies" to "administrative policies" to avoid
misunderstanding. Such policies may include setting quotas on storage,
connection rates etc, but since they are not explained as such in the MQTT
standard, I would like to avoid specifying them here as well]

> The MQTT refs shouldn't be an issue. More info about downrefs can be

[CS: Thanks, then I will keep things as they are.]

>     to implement for the Broker.
[CS: OK, moved back to normative]

> https://datatracker.ietf.org/doc/rfc8032/referencedby/

[CS: Moved back to normative]

> Thanks again.
Kind regards,
--Cigdem

---

From: Cigdem Sengul <[email-masked]>
Date: Wed, 9 Mar 2022 23:43:31 +0000

Fixed back in the new commit. Thank you! I wasn't sure how to react to
downref comment.

On Wed, 9 Mar 2022 at 23:39, Benjamin Kaduk <[email-masked]> wrote:

> > Just one note: for the downref to informative documents (for those
> documents that were actually included in the text), please revert the
> change – RFC 6234 and RFC 8032 were correctly referenced as normative,
> since they are mandatory to implement. MQTT standards are also correctly
> referenced normatively. Jean was only noting these appear as downrefs, but
> downrefs are not “mistakes”, they just require more attention – see
> https://www.rfc-editor.org/info/bcp97 for more information. Downrefs are
> usually Last Called to make sure the community is aware of them, except for
> those that have appeared as downref for many documents and don’t need to be
> Last Called explicitly anymore: https://datatracker.ietf.org/doc/downref/.
>