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?
_______________________________________________
Ace mailing list
Ace@ietf.org
https://www.ietf.org/mailman/listinfo/ace
