Hello Francesca, Thank you for your review. My responses are inside; in the cases I have not understood a comment, I asked for clarifications. Kind regards, --Cigdem
On Tue, Sep 15, 2020 at 2:38 PM Francesca Palombini < francesca.palomb...@ericsson.com> wrote: > > 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. > > [CS: Added to https://github.com/ace-wg/mqtt-tls-profile/issues/65, and will be fixed in the next revision.] > -- > > " 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? > > [CS: I followed the core document statement, which also says: "It is also REQUIRED that the communicating endpoints perform mutual authentication.". ] > -- > > 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. > > [CS: This is because I've interpreted what I read in 5.6.4 as the ace_profile can be used as a parameter in the request. "5.6.4. Request and Response Parameters: This section provides more detail about the new parameters that can be used in access token requests and responses" 5.6.4.3. Profile " So, I did not think that specifying what the profile should be contradicted the document. ] -- > > When CBOR is used, the > interactions must implement Section 5.6.3 of the ACE framework > [I-D.ietf-ace-oauth-authz]. > > normative MUST? > > [CS: Added to https://github.com/ace-wg/mqtt-tls-profile/issues/65, and will be fixed in the next revision.] -- > > 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. > > [CS: My response is the same as above. Since the core document capitalised REQUIRED, I kept MUST.] > -- > > "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) > > [CS: It is overhead as any token validation done in TLS is overwritten by what ever token is provided in ace. Will add a sentence around that. https://github.com/ace-wg/mqtt-tls-profile/issues/70 ] > -- > > 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. > > [CS: I am not sure I understand the comment. The other methods are described, but TLS:Anon-MQTT:ace is recommended. You suggest I reverse the order, and start with recommended version?] > -- > > 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)? > [CS: Yes, if it does not want to support session continuation, it always starts clean session. If necessary was added due to providing potentially support for better QoS, but it may be other reasons. So, instead of prescribing a reason, I opted for "If necessary", where the need is determined by the Broker provider. ] -- > > 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. > > [CS: QoS level is defined in MQTT terminology in Section 1.3. " QoS level The level of assurance for the delivery of an Application Message. The QoS level can be 0-2, where "0" indicates "At most once delivery", "1" "At least once delivery", and "2" "Exactly once delivery"." ] > -- > > 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. > > [CS: Authentication data is defined in 2.2.4 before these subsections as: "The Authentication Method is followed by the Authentication Data, which has a property identifier 22 (0x16) and is binary data. The binary data in MQTT is represented by a two-byte integer length, which indicates the number of data bytes, followed by that number of bytes." ] > -- > > 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. > [CS: Noted. https://github.com/ace-wg/mqtt-tls-profile/issues/71] > 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. > [CS: Not sure I follow the comment. The draft says: " If the Client does not provide a valid token or omits the Authentication Data field], so includes the option of invalid token. I can add to that "does not provide a valid token or authentication data" to make it more clear. ] > > -- > > Figure 5: AIF-MQTT data model > > Need to reference CDDL. > [CS: Added to https://github.com/ace-wg/mqtt-tls-profile/issues/65, and will be fixed in the next revision.] > > -- > > Section 3: > What does it mean to have an empty array for scope? (as that is allowed) > > [CS: Do you mean I should add that as an explanation? It means no permissions?] > -- > > Scope appears in section 2.1 for the first time, but its encoding is only > defined in section 3. > [CS: Do you suggest doing a forward reference to section 3 from section 2.1?] > > -- > > 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. > [CS: This was explained in the 2.2.3 - the Will Flag is set in CONNECT message. The Will Topic filter would be added like any other permission to the scope. Will Topic is set in the CONNECT message. When the time comes to publish the will, the RS checks whether the will topic is covered in the scope permissions. Will is like any other PUBLISH message, but I will add a few more clarifying sentences. https://github.com/ace-wg/mqtt-tls-profile/issues/72 ] > > -- > > 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. > > [CS: Yes, that would update the access rights. ] > -- > > 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. > [CS: When the resource owner changes the access policy for the client at the AS, but the client has still a valid token with old access rights? Revocation would assume there is an AS-> RS channel for pushing notifications. There isn't. The only option is that RS does regular token introspections, and not rely on cached data that much. That sentence was in the context of cached tokens/token introspection responses. The next sentence was: "If the RS caches the token introspection responses, then the RS should use a reasonable cache timeout to introspect tokens regularly." ] > > > > ============= > > 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). > > [CS: Added to https://github.com/ace-wg/mqtt-tls-profile/issues/65, and will be fixed in the next revision.] > -- > > Topic Filters may include wildcards > > This might be me, but I would have liked some explanation of what > wildcards are in MQTT. > > [CS: Wildcards are explained in Section 3 - " The multi-level wildcard, '#', matches any number of levels within a topic, and the single-level wildcard, '+', matches one topic level." Do you mean add it to its definition? ] > -- > > 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. > [CS: Property will mean something specific to MQTT developers. I will add it to MQTT Terminology in section 1.3 "A Property consists of an Identifier which defines its usage and data type, followed by a value. The Identifier is encoded as a Variable Byte Integer. "] > > -- > > 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". > > [CS: Client Authorisation Server was defined in the old actors draft - Daniel made a similar comment. Not sure what terminology the group settled on after the draft expired.] > -- > > 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) > [CS: Respective to mean RPK or PSK, respectively] > > -- > > "authz-info" is a public topic, this response is not expected to > cause confusion. > > Maybe s/public/not protected ? > [CS: I prefer public, but can change if the group prefers 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" > > [CS: Added to https://github.com/ace-wg/mqtt-tls-profile/issues/65, and will be fixed in the next revision.] > -- > > 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. > [CS: Daniel had a similar comment - I was wondering whether I should explain lower case must is referring to MQTT spec, and uppercase for the spec.] > > -- > > 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. > > [CS: Added to https://github.com/ace-wg/mqtt-tls-profile/issues/65, and will be fixed in the next revision.] > -- > > 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? > [CS: Not to my knowledge]
_______________________________________________ Ace mailing list Ace@ietf.org https://www.ietf.org/mailman/listinfo/ace
