[Ace] WGLC review of draft-ietf-ace-mqtt-tls-profile-07

[Francesca Palombini <[e-mail]>]

Hi,

Thank you for this work! Here is my review of the document.

Thanks,
Francesca

  The response includes the
   parameters described in Section 5.6.2 of the ACE framework
   [I-D.ietf-ace-oauth-authz].

The fact that the profile parameter with value "mqtt_tls" is included in this response must be specified here.

--

" The Client and the AS MUST
   perform mutual authentication."

I am not sure this is a testable statement. What about specifying that they MUST use a protocol that provides mutual authentication instead?

--

 The Client requests an access token
   from the AS as described in Section 5.6.1 of the ACE framework
   [I-D.ietf-ace-oauth-authz], however, it MUST set the profile
   parameter to 'mqtt_tls'. 

Why adding this here? This is in practice implementing a negotiation of profile. If this is necessary in this MQTT profile I am wondering why it is defined here and not in the framework. In general, I dislike the profile to contradict what defined in the framework.

--

 When CBOR is used, the
   interactions must implement Section 5.6.3 of the ACE framework
   [I-D.ietf-ace-oauth-authz].

normative MUST?

--

The Client and the Broker MUST perform mutual authentication.

Same as before, is this a testable statement? Maybe this sentence does not need to use normative language, but rather descriptive. The details of what Client and Broker MUST do is in the following sentences.

--

"TLS:Known(RPK/PSK)-MQTT:ace": This option SHOULD NOT be chosen.

Any reason why this is a SHOULD NOT? Can we add motivation of when this would be allowed (although not recommended)

--

 It is RECOMMENDED that the Client follows TLS:Anon-MQTT:ace.

I think it would be helpful to say when this option is recommended vs the others. In the next section, it is described in more details that the communication with authz-info is done with "TLS:Anon-MQTT:None" and then after that TLS:Known(RPK/PSK)-MQTT:none. This seems to not agree with the recommendation, which is why I think the recommendation needs more context.

--

 In this profile, the Broker SHOULD always start with a
   clean session regardless of how these parameters are set.

Does this mean that when the Broker recognize that this spec is used, it SHOULD disregard the parameters value and start a clean session? What are the cases where this is not done ("If necessary" in the next paragraph)?

--

 includes state on client subscriptions, QoS 1 and QoS 2 messages

At this point I don't know what QoS 1 and QoS messages are, they have not been introduced. Only QoS levels have. Maybe a reference or a pointer would help.

--

RE Authentication Data (Sections 2.2.4.1, 2.2.4.2 etc): I haven't read MQTT in details, but I don't really see any encoding of the Authentication Data field in this spec, only that it contains one or more parameters. I would assume that is should be in scope of this doc, but if it isn't, that should be clarified.

--

2.2.6.1.  Unauthorised Request: Authorisation Server Discovery

If the Client does not provide a valid token or omits the
   Authentication Data field, the Broker triggers AS discovery. 

Following discussion about AS discovery in this ML: I think this should change to being possible, not mandatory. That means changing: s/the Broker triggers AS discovery/the Broker MAY trigger AS discovery. I still think it is useful to have this section, I am just suggesting it becomes optional.

Also note that not providing a token or omittion the Authentication Data field are not the possible error messages. A malformed token or Authenticated Data are also possible.

--

 Figure 5: AIF-MQTT data model

Need to reference CDDL.

--

Section 3:
What does it mean to have an empty array for scope? (as that is allowed)

--

Scope appears in section 2.1 for the first time, but its encoding is only defined in section 3.

--

  If the Will Flag is set, then the Broker MUST check that the
   token allows the publication of the Will message (i.e. the Will Topic
   filter is found in the scope).

This sentence is not super clear to me: "if the Will Flag is set" in which message? How is the Will Topic filter added to scope? This comes from my ignorance of MQTT, so feel free to skip it, but I think an example would have helped me here.

--

Section 4 talks about reauthentication. What described for reauthentication should work also for update of access rights, is that correct? Does that add any considerations? Anyway I think update of access rights should be discussed in this document.

--

If a client's permissions get revoked but the access
   token has not expired, the RS may still grant publish/subscribe to
   revoked topics. 

What does it mean in practice that client's permissions get revoked? (also nit s/permissions get/permission gets) Do you mean that there is no way to do access right revocation? If that's the case, I did not understand "the RS may still grant ..." as being a negative consequence, so maybe it needs some clarification.



=============

Nits:

framework to enable
   authorization in an Message Queuing Telemetry Transport (MQTT)

s/an/a

--

The token may be a reference or JSON Web Token
   (JWT)

add a reference to rfc7519
You could also add a informative reference to 8392 for the CWT.

--

Missing references to CoAP (informative) and HTTPS (or rather references to HTTP and TLS). 

--

 Topic Filters may include wildcards

This might be me, but I would have liked some explanation of what wildcards are in MQTT.

--

AUTH Properties
           include Authentication Method and Authentication Data.

I assume "Properties" is specific term from MQTT terminology, but I don't see it defined in this section, so maybe either generalize ("field/parameter"? or I see you also use "information" for the Will message) or explain it.

--

If the Client is resource-constrained, a Client Authorisation Server
   may carry out the token request on behalf of the Client,

I might have missed this, where is a "Client Authorization Server" defined? I am fine with the terminology used in the figure though, "Client - Authorization Server interface".

--

  If the
   Client authentication uses TLS:Known(RPK/PSK), then the Broker is
   authenticated using the respective method.

s/respective/same? (just to understand better, please disregard if you think it is clear as is)

--

"authz-info" is a public topic, this response is not expected to
   cause confusion.

Maybe s/public/not protected ?

--

 Similarly, the server MUST NOT process
   any packets other than DISCONNECT or an AUTH that is sent in response
   to its AUTH before it has sent a CONNACK.

add "from that client"

--

If the Will Flag is set to 1 and the
   Broker accepts the connection request, the Broker must store the Will
   message

I understand that these are not requirements set by this document but rather inherited by the MQTT spec, so I would suggest remove the use of must etc (here and in other parts of the document) to avoid any question about if this should be normative or not later on.

--

which have have not been completely acknowledged or pending

remove one of the "have"

--

  In case of an invalid
   PoP token, the CONNACK reason code is '0x87 (Not Authorized)'.

This is in the section about success, while there is a separate section for unauthorized request, where imo this would fit better.

--

   To transport the token to the Broker inside the CONNECT message, the
   Client uses the username and password fields.  

This is just a question rather than a nit: is there no way to register new names for these fields in MQTT?