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

From: Theresa Enghardt via Datatracker <[email-masked]>
Date: Wed, 02 Mar 2022 12:03:43 -0800

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?

---

From: Cigdem Sengul <[email-masked]>
Date: Fri, 4 Mar 2022 22:23:50 +0000

Dear Theresa,
Thank you very much for the comments.

I have prepared a revised version, where the changes can be found in the
following pull request:
 https://github.com/ace-wg/mqtt-tls-profile/pull/99

If you are happy with these changes, I can submit a new ID.
I also explain the changes below, inline.

> What is a reference in this context?

[CS: Added: "opaque reference to authorization information"]

> closed normally" is a well-defined concept in this context, please provide

[CS: Introduced a formal definition of Network Connection to MQTT-related
terminology - as defined in MQTT standard.
To the Will definition, added the situations when the connection is
considered not to have closed normally.
Question: Normal disconnection is DISCONNECT with reason code is 0x00,
according to MQTT standard - is this definition also needed?"

>    authentication, confidentiality protection, and integrity protection."

[CS: Changed to MUST].

> [CS: Added for cnf:
The AS includes a 'cnf' parameter to the PoP token,
   to declare that the Client possesses a particular key and RS can
   cryptographically confirm that the Client has possession of that key.
   The 'cnf' parameter is REQUIRED if a symmetric key is used, and MAY
   be present for asymmetric proof-of-possession keys, as described in
   [I-D.ietf-ace-oauth-params].

rs_cnf:
Otherwise, to authenticate the Broker, the Client MUST validate a public
key from a
   X.509 certificate or an RPK from the Broker against the 'rs_cnf'
parameter in the token response, which contains information about the
   public key used by the RS to authenticate if the token type is "pop"
   and asymmetric keys are used as defined in [I-D.ietf-ace-oauth-params].

> packet (like the next paragraph)? I suggest to make this explicit.

[CS: Moved the last sentence of the previous paragraph to underline that we
are talking about the PUBLISH packet.
So, it is now:
Depending on the QoS level of the PUBLISH packet, the Broker returns
   the error response as a PUBACK, PUBREC, or DISCONNECT packet.  If the
   QoS level is equal to 0, ....
]

> Does "at each connection" mean in each CONNECT packet, or something else?

[CS: Clarified as:
 "Given that the Broker MUST associate the Client with a valid token, a
Client will only send or receive messages to its authorized topics.."
(This is becuase the Broker may have saved a token for Client, and Client
may have not provided a Token in Connect, but passed the challenge/response
hence, proving that the stored token is for itself]
]

> "MQTTS" once more here in the introduction?

[CS: Decided MQTTS is colloquial - as MQTT Standard doesn't refer to it.
So, removed its definition and fixed all instances of MQTTS as MQTT over
TLS]

> lower case "client" in Section 2.2.3.1, 2.2.3.2, 2.2.4.1, 2.2.5 - should

[CS: changed client->Client if speaking of an MQTT client. ]

> Maybe this reads better as "The mechanisms specified in this document do
[CS: Fixed]

> need to be expanded again in Sections 2.2.1 and 2.2.3.

[CS: Moved to where RPK/PSK first mentioned.  Kept the expansions in two
places, removed others.]

> The broker?

[CS: Fixed]

Thanks again,
--Cigdem

---

From: Theresa Enghardt <[email-masked]>
Date: Fri, 4 Mar 2022 18:42:07 -0800

Dear Cigdem,

Thank you for preparing the revised version, it looks pretty good to me.

Some replies inline:

On 3/4/22 14:23, Cigdem Sengul wrote:
> [CS: Introduced a formal definition of Network Connection to 
> MQTT-related terminology - as defined in MQTT standard.

[TE] So "not closed normally" means any way to terminate the Network 
Connection, other than DISCONNECT with reason code 0x00? If so, I think 
this would be a good addition to the definition, either as its own 
definition or added to the "Will" definition.

> Otherwise, to authenticate the Broker, the Client MUST validate a 
> public key from a

[TE] These explanations already help, thanks! However, and this might 
just be me, but I keep wondering what 'cnf' stands for, i.e., if it is 
an acronym for something, and if it is, if it makes sense to expand the 
acronym. But it might just be a string that comes from "somewhere", 
which is fine with me, too. :)

Best,
Reese (aka Theresa)

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 4 Mar 2022 18:46:59 -0800

Hi Theresa,

> definition or added to the "Will" definition.

I think that's right, but Cigdem knows MQTT better than me and she should
confirm.

> which is fine with me, too. :)

I think the lineage of "cnf" can be traced back to at least RFC 7800, so at
this point it's probably a fairly well established part of the greater
OAuth ecosystem.

Which is not to say that we can't try to make the document more accessible
to new readers, of course.  The ACE framework itself relies pretty heavily
on proof of possession semantics for JWT/CWT tokens, so perhaps the
implicit reliance on draft-ietf-ace-oauth-authz and its terminology would
suffice.  Happy to hear further thoughts.

-Ben

---

From: Cigdem Sengul <[email-masked]>
Date: Sat, 5 Mar 2022 13:40:02 +0000

Hello,
I've created a new pull request for the changes:
https://github.com/ace-wg/mqtt-tls-profile/pull/101
Explanations are below.

> I think that's right, but Cigdem knows MQTT better than me and she should
[CS: Yes, added the DISCONNECT packet definition. I also took the MQTT
standard text to specify more formally
situations for sending a Will, which included a definition for normal
disconnection. I reduced the Will-specific text in the
main document, as this definition now is comprehensive.]

> suffice.  Happy to hear further thoughts.

[CS: Added   --> 'cnf' (confirmation) claim in its first instance. We are
referencing the params document as well for both confirmation related
claims (cnf, rs_cnf). Would this be enough?]

Kind regards,
--Cigdem

---

From: Lars Eggert <[email-masked]>
Date: Mon, 7 Mar 2022 11:31:46 +0200

Theresa, thank you for your review. I have entered a No Objection ballot for this document.

Lars

> On 2022-3-2, at 22:03, Theresa Enghardt via Datatracker <[email-masked]> wrote:
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art

---

From: Reese Enghardt <[email-masked]>
Date: Mon, 7 Mar 2022 09:44:16 -0800

Hi,

Thank you for the changes and additional context.

This looks good to me!

Best,
Reese (aka Theresa)

On 3/5/22 05:40, Cigdem Sengul wrote:
> [CS: Added   --> 'cnf' (confirmation) claim in its first instance. We 
> are referencing the params document as well for both confirmation related