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

[Francesca Palombini <[e-mail]>]

Hi Cigdem,

Thanks for the quick reply! Answers inline.

To Ludwig and the WG, which might not otherwise see the comment: Cigdem and I have a different interpretations of what the framework allows to put in the “ace_profile” parameter in the request from Client to AS (See (***) below). I think this might require clarifications in the framework either way.

Thanks,
Francesca

From: Cigdem Sengul <[e-mail]>
Date: Sunday, 20 September 2020 at 23:02
To: Francesca Palombini <[e-mail]>
Cc: Ace Wg <[e-mail]>, “[e-mail]” <[e-mail]>
Subject: Re: WGLC review of draft-ietf-ace-mqtt-tls-profile-07




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 <
rancesca.palombini@ericsson<mailto:>.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.”.
]

FP: Right. However I think that the framework’s requirement is ok even if it is not implementable because the framework is not supposed to be implemented by itself. So the framework’s requirement is more a requirement on what the profile must specify. What I was looking for in the profile was some indication on how to perform the mutual authentication, hence the suggested rephrasing. It’s not a major change though, so feel free to disregard.

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

(***)
FP: Your interpretation was surprising to me. I always understood that the ace_profile parameter can only be included in the request with the null value, so that the client can indicate to the AS that it wants to receive the ace_profile back. I went through the relevant sections again, and the following text in 5.6.1. "Client-to-AS Request" is what I based my understanding on.

o  The client can send an empty (null value) "ace_profile" parameter
      to indicate that it wants the AS to include the "ace_profile"
      parameter in the response.  See Section 5.6.4.3.

And in 5.6.4.3. "Profile":

   Clients that want the AS to provide them with the "ace_profile"
   parameter in the access token response can indicate that by sending a
   ace_profile parameter with a null value (for CBOR-based interactions)
   or an empty string (for JSON based interactions) in the access token
   request.

There is no text about allowing any other value than the null value in the ace_profile, or how the AS should react to receiving such a parameter with non-null value (what if it does not support the profile? That at least should be documented) so I just assumed that it was not supported.

I'd like to get the opinion of Ludwig and the WG on this, but I think this requires some clarifications in the framework in both cases:
* if your interpretation is correct, there should be more text in the framework about how that works
* if my interpretation is correct, it should be clarified that in the request the only acceptable value for the "ace_profile" is null.


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

FP: What I did following Ben's review of the OSCORE Profile was to un-capitalize the normative statements from the framework (i.e. s/MUST/must when it was a requirement inherited from the framework). But again, your decision here.

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

FP: No, what I meant is that for example when publishing to authz-info the client MUST use TLS:Anon-MQTT:None, as described in 2.2.2, so the "RECOMMENDED that the Client follows TLS:Anon-MQTT:ace" does not apply to that particular communication. I was just hoping you could add some context about when it is recommended to use TLS:Anon-MQTT:ace (this is basically just an editorial/clarification).

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

FP: Ok, thanks for the clarification. Changing "If necessary" to "If the Broker provider requires it" (or something of the sort) would have helped me in this case, as that removes the vagueness of "necessary".

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

FP: Ah ok. Then I would suggest s/QoS/QoS level.

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

FP: Right, what I was wondering is for the "that number of bytes", so the actual data part of the Authentication Data. For example in 2.2.4.1, you write:

the Authentication Data MUST contain the two-byte
   integer token length, the token, and the keyed message digest (MAC)
   or the Client signature.

I assume you are concatenating the bytes string of length, token and MAC? That should be specified, because "contain" is not clear enough.

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

FP: This comment comes from my interpretation of "valid token", which to me comes from the framework, where the token is validated to make sure that it is integrity protected, comes from the AS and covers the resource requested (that also includes not expired).  What I was asking you to add is the case where the token cannot be validated, because it is malformed so it cannot be parsed or validated, and same for the Authentication data.

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

FP: I was just noting that the empty array is a possible option according to the draft's CDDL (i.e. a Broker would not reject with error such an empty array), and I was wondering how that would be interpreted. If it's no permission, it would be good to clarify. Otherwise you could add a requirement to the AS that it MUST NOT be the empty array.

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

FP: Yes.

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

FP: Thanks!

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

FP: Will you add a sentence about that in the spec?

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

FP: Ah of course. My confusion came from the "gets revoked" as for some reason I assumed it meant that "it got revoked at the RS", so the RS knew in some way (maybe with introspection). Nevermind, thanks for the answer.

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

FP: These are definitions of multi-level and single-level wildcard, I was talking about wildcard in general. It could also be that I am not a native English speaking person, feel free to disregard if you think that is clear.

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

FP: Thanks! Looks good.

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

FP: I see. I suggest using "Client-Authorisation Server interface", which to me is more intuitive.

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

FP: Again this comment stems from my familiarity with ACE terminology. Public might be clearer for MQTT people. Feel free to disregard, or use both.

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

FP: Or you could just point to the section of the spec where these are inherited from. This would have been enough for me. Otherwise it's also fine with just explaining future reviewers why they are lower case.

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