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

[Cigdem Sengul <[e-mail]>]

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.

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

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



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

[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?"


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

[CS: Changed to 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.
>
> [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].




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

[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, ....
]


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

[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]
]

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

[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]


>
> "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?
>

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


>
> "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 […]"?
>
[CS: Fixed]


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

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

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

[CS: Fixed]

Thanks again,
--Cigdem