Nits/Typos/References

[ciseng]

A compilation of all nits:
== Line 497 has weird spacing: '...rotocol  name ...'
  == Line 901 has weird spacing: '...rotocol  name ...'
  == The document seems to lack the recommended RFC 2119 boilerplate, even if
     it appears to use RFC 2119 keywords. ( I think that is an error as I can see the section 1.1.)
  == Unused Reference: 'RFC7250' is defined on line 1135, but no explicit
     reference was found in the text
 -- Unexpected draft version: The latest known version of
     draft-ietf-ace-oauth-authz is -34, but you're referring to -35.
 -- Unexpected draft version: The latest known version of
     draft-ietf-ace-oauth-params is -12, but you're referring to -13.
 -- Unexpected draft version: The latest known version of
     draft-ietf-ace-pubsub-profile is -00, but you're referring to -01.

Cigdem: Could not find the weird spacing; RFC 2119 boilerplate error seems to be bug; cited RFC7250 for raw public keys; the rest of draft versions are correct (it seems like the nits are not updated).

[ciseng]

Change:
"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'. "

To:
"from the AS as described in Section 5.6.1 of the ACE framework
   [I-D.ietf-ace-oauth-authz], with the profile
   parameter set to 'mqtt_tls'. "

Cigdem: Removed that statement, Added: The AS informs the client that selected profile is "mqtt_tls"
using the "ace_profile" parameter in the token response.

From: "The AS uses JSON in the payload of its responses to both to the
   Client and the RS."

To: "The AS uses JSON in the payload of its responses to the
   Client and the RS."

Cigdem: Fixed.

[ciseng]

"The MQTT session state is identified by the Client identifier and
   includes state on client subscriptions, QoS 1 and QoS 2 messages
   which have have not been completely acknowledged or pending
   transmission to the Client,"

Duplicate have.

Cigdem: Fixed

[ciseng]

"AIF-Generic<topic_filter, permissions> = [* [topic_filter,permissions]]"
Missed space after topic_filter

Cigdem:Fixed

[ciseng]

As in MQTT v5.0, The Token MAY either be transported before by
   publishing to the "authz-info" topic, or inside the CONNECT message.

The Token - wrongly capitalised.

Cigdem: Fixed

[ciseng]

"CONNECT without a token: It is not possible to support AS
      discovery via sending a tokenless CONNECT message to the Broker.
      This is because a CONNACK packet in MQTT v3.1.1 does not include a
      means to provide additional information to the Client.  Therefore,
      AS discovery needs to take place out-of-band.  CONNECT attempt
      MUST fail."

Finish last sentence wit Tokenless CONNECT attempt MUST fail.

Cigdem: Fixed

[ciseng]

This document registers 'EXPORTER-ACE-MQTT-Sign-Challenge' from
   Section 2.2.4.1 in the TLS Exporter Label Registry TLS-REGISTRIES
   [RFC8447].

-> Section number should be 12.

Cigdem:Fixed to this: This document registers 'EXPORTER-ACE-MQTT-Sign-Challenge' (introduced in Section 2.2.4.1
in this document) in the TLS Exporter Label Registry [RFC8447].

[ciseng]

General comments from Marco:

• Refer the AIF adopted draft rather than the individual submission.

• Some references are included twice side by side, e.g. RFC 4949 and RFC 7800.

Fixed

[ciseng]

[Section 1]

• Add an inline reference to RFC 8446 for TLS 1.3. I think it's good adding also references to CoAP and CBOR(-bis).

Cigdem:Fixed

[ciseng]

Section 2.2.4.1

• It's worth making it explicit that the PoP key is used to compute the MAC or the client signature. 
  Fixed: The Proof-of-Possession key in the token is used to calculate to the keyed message digest (MAC) or the Client signature based on the contained obtained from the TLS exporter

• s/and, the server/and the server

• Remove the final closed parenthesis.

Fixed

[ciseng]

Section 2.2.4.2

• Like for the comment above for Section 2.2.4.1, it's worth making it explicit that the PoP key is used to compute the MAC or the client signature.
  Fixed as: The Authentication Data in the Client's response is formatted as the client nonce length, the client nonce, and the signature or MAC computed over the RS nonce concatenated with the client nonce using PoP key in the token."

[ciseng]

[Section 2.2.5]

• s/RS MUST verify/the RS MUST verify
  Fixed

• Please, add references for HS256 and Ed25519.

Cited RFC6234, and RFC8032, respectively - but noticed HS256 is not usually provided with a citation in other RFCs using it.

[Section 3]

• s/to all topic3/to all 'topic3'

Fixed

[ciseng]

[Section 6.1]

• s/as a UTF-8/is a UTF-8

Fixed

[ciseng]

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.

Fixed: Added: "and specifically, the "ace_profile" parameter is set to "mqtt_tls" at the end of the sentence.

[ciseng]

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

must ->MUST

Fixed

[ciseng]

Need to reference CDDL.

Fixed: Added RFC8610

[ciseng]

Nits:

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

s/an/a

--
he 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).
Fixed all - added RFC7230 for https

[ciseng]

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"

Fixed

[ciseng]

--

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.

Sentence removed.

[ciseng]

Francesca: 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)

Added: It is RECOMMENDED that the Client uses "TLS:Anon-MQTT:ace" as a first choice when working with protected topics. However, depending on the Client capability, Client MAY use "TLS:Known(RPK/PSK)-MQTT:none", and consequently "TLS:Anon-MQTT:None" to submit its token to "authz-info".

[ciseng]

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

Francesca: > Does this mean that when the Broker recognizes 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".

Added: The Broker MAY support session continuation e.g., if the Broker requires it for QoS reasons.

[ciseng]

includes state on client subscriptions, QoS 1 and QoS 2 messages
Francesca: QoS->QoS level

Changed to: messages with QoS levels 1 and 2

[ciseng]

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

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.

This is covered in the text but re-emphasize.

Reworded as: If the Client does not provide a valid token or omits the Authentication Data field, or the token or Authentication data are malformed, authentication fails.

[ciseng]

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.

Added: If the scope is empty i.e., the JSON array is empty, the RS records no permissions for the client for any topic. In this case, the client is not able to publish or subscribe to any protected topics.

[ciseng]

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.

Done

[ciseng]

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?

Changed the title of the section:Token Expiration, Update and Reauthentication, and reworded slightly.

[ciseng]

[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.
Formally defined CAS