Hi Cigdem, Oof, has it really been two months since you sent this? I am sorry to have let it linger for so long.
I've gone through the published -13 and have a handful or two of comments left (which I will send separately), but let me just reply here to a few things first (inline). On Mon, Oct 18, 2021 at 03:23:12PM +0100, Cigdem Sengul wrote: > Hello Ben, > I thought I should comment on your original review to have the same order > you initially planned. > I went through all the comments, and our discussions of it. > The comparison with Editor's copy and github draft is here > <https://tools.ietf.org/rfcdiff?url1=https://tools.ietf.org/id/draft-ietf-ace-mqtt-tls-profile.txt&url2=https://ace-wg.github.io/mqtt-tls-profile/draft-ietf-ace-mqtt-tls-profile.txt> > . > > In summary, I made the following changes: > (1) Kepts dtls profile informative (going through it, I brought all that > applied to this draft but kept out the ones that didn't apply). For that, I > introduced new sections that explains how we support TLS - PSK and RPK for > client authentication (2.2.3.1 and 2.2.3.2). Aimed to clarify all > TLS-related stuff e.g. added recommended references for using TLS in > constrained environments, followed the cipher suite requirements from these > references. I think I see what you're trying to do here, and it makes sense in a certain way ... but if I go and compare side-by-side the text we have here and what's in the DTLS profile, the DTLS profile goes into a lot more detail. In particular, the DTLS profile mentions some things that implementations need to do in order to avoid vulnerabilities, and I'm not sure that we want to go into so much detail in this document when it's already recorded elsewhere. I'm willing to let the document advance to IETF LC and IESG review with this part as-is (but will not be surprised if other reviewers raise the topic), but just wanted to check if there's a particular reason to want the DTLS profile to not be a normative reference. I think I don't understand that part of things very well, so to me there's not much harm in making it a normative reference -- maybe I'm missing something! > (2) Added more to the glossary about the MQTT data formats so that it is > clear the data is prefixed with a length field, or how variable length > fields > are computed. > (3) Fixed the MQTT CONNECT figures to show more details, and better > alignment of fields. > (4) Revised the document to explain the Clean Session better. > (5) Revised the document to explain how Will topics are verified to be > authorised, and how the Broker doesn't accept CONNECT if they are not. > (6) Added examples on scopes and wildcard matching for SUBSCRIBE. > (7) Added a section on 2.3 on token scope to explain the scope data > structure. I think the restructuring to handle this stuff came out quite well -- thanks for taking the wholistic approach. > The detailed changes, and references to Github issues/commits are below. > I've SNIPped the ones that required no action or no > change based on our discussion e-mails. > > I will submit a new version - should I wait for your perusal of the changes > (detailed also below)? > > Kind regards, > --Cigdem > > On Thu, Aug 5, 2021 at 11:39 PM Benjamin Kaduk <ka...@mit.edu> wrote: > > > Hi all, > > > > Sorry to have taken so long to get back to this, and thank you for > > continuing to make updates in response to the changes in the framework and > > other profiles. > > > > In general, the protocol mechansisms defined here are in good shape; > > thank you! > > > > I made a github PR with some changes that seemed easier to phrase in the > > form of a patch than a prose comment: > > https://github.com/ace-wg/mqtt-tls-profile/pull/77 > > > [Cigdem: Pulled and merged.] > > > > > > > I did find a couple of significant issues that need to be addressed > > before IETF LC, but I think any needed changes will be pretty localized. > > > > Specifically, there's no requirement for ACE access tokens to be > > self-deliniating, so we can't actually programmatically tell whether > > there's content after the token in a CONNECT message; the mechanisms in > > Sections 2.2.4.1/2.2.4.2 seem to assume that we can do so for > > determining whether the CONNECT is just providing a token or also > > providing PoP over a TLS exporter value. I think that this just means > > we need to have an explicit "token length" field (or similar). > > > > [CS: This was discussed; no changes as the text already had a "token > length".] > > > > [SNIP] > > > > A few other general notes before the section-by-section notes: > > > > There is very little in this document about the HTTP-based interactions > > with the AS. I think the intent is to defer to the core framework for > > that, but being a little more explicit about what is being pulled in and > > how would be helpful. > > > [CS: I assumed this comment was for the token and introspection endpoints. > Added some text to refer to the core document more explicitly. > commit 8a789e024b4576b06a9bccc6673b53ab6a709ab8 > <https://github.com/ace-wg/mqtt-tls-profile/commit/8a789e024b4576b06a9bccc6673b53ab6a709ab8> > > > > > > If we're using TLS Exporters and allow TLS-not-1.3, we need to make some > > additional requirements on TLS usage in order for the exporter values to > > be safe. Typically this takes the form of requiring the extended master > > secret extension along with guidance on what cipher suites to use; I > > guess RFC 7925 (rather than 7525) would be the default reference for > > other TLS usage. > > > > [CS: Added a new section 2.2.3 Client Authentication over TLS that makes > these recommendations.] > > > > > This document seems to mostly use British English. AFAIR, that's okay, > > but if it's inconsistent the RFC Editor will prefer American English. I > > didn't attempt to check for this (though there are tools like > > https://github.com/larseggert/ietf-reviewtool that will do so). > > > > [CS: haven't used this particular tool, but I believe inconsistencies are > now fixed.] > > > > > > > Section 1 > > > > their subscribers. In the rest of the document the terms "RS", "MQTT > > Server" and "Broker" are used interchangeably. > > > > We will probably get a reviewer asking why we can't pick a single term > > and standardize on it. However, I expect that there are places where we > > want to emphasize on one aspect or another of its behavior, so don't > > think we should actually do so. Similarly for the places where we > > mention that CoAP can be used (but don't reference a concrete > > specification). > > > [CS: I've tried to use the Broker more consistently, but happy to switch to > one term as we discussed in later > e-mails. I have not added more specific CoAP references; I refer > to RFC7252 as the core document does.] > > > > > > Clients use MQTT PUBLISH message to publish to a topic. This > > document does not protect the payload of the PUBLISH message from the > > Broker. Hence, the payload is not signed or encrypted specifically > > for the subscribers. This functionality MAY be implemented using the > > proposal outlined in the ACE Pub-Sub Profile > > [I-D.ietf-ace-pubsub-profile]. > > > > I suggest s/MAY/may/, which would avoid any need to make > > ace-pubsub-profile a normative reference. > > > [CS: Done] > > > > > > reference or JSON Web Token (JWT) [RFC7519]. For JWTs, this document > > follows [RFC7800] for PoP semantics for JWTs. The Client-AS and RS- > > AS MAY also use protocols other than HTTP, e.g. Constrained > > Application Protocol (CoAP) [RFC7252] or MQTT; it is recommended that > > TLS is used to secure these communication channels between Client-AS > > and RS-AS. Implementations MAY also use "application/ace+cbor" > > content type, and CBOR encoding [RFC8949], and CBOR Web Token (CWT) > > [RFC8392] and associated PoP semantics to reduce the protocol memory > > and bandwidth requirements. For more information, see Proof-of- > > Possession Key Semantics for CBOR Web Tokens (CWTs) [RFC8747]. > > > > One thing that can be surprising to readers not versed in the ecosystem > > is that RFCs 7800 and 8747 only talk about the token claims that are > > used to build the PoP system, and don't actually define mechanisms for > > providing the proof of possession. We might want a forward-reference to > > § 2 where we do actually specify mechansisms to prove possession of the > > indicated key. > > > > [CS: Done] > > > > > > Section 2.2.1 > > > > The way we name these authentication options with specific (quoted) > > strings suggests that they will be used as a protocol element. But > > where? Is the literal string "Known(RPK/PSK)" used in both cases (vs. > > having distinct strings for RPK and PSK)? > > > Also, the hyphen character tends to more often be used as a joiner than > > a separator, so it's easy to misread this as a triple of "TLS", > > "Anon-MQTT", "None"/etc.. (I originally was going to ask why these all > > had a "TLS" prefix...) It might be better to use a semicolon or comma > > instead of hyphen. > > > > [CS: Changed to the style discussed e.g., TLS:Anon,MQTT:None] > > > > > > o "TLS:Anon-MQTT:None": This option is used only for the topics that > > do not require authorization, including the "authz-info" topic. > > > > Are there topics other than "authz-info" that don't require > > authorization? We might need to add some heding language to the earlier > > statement that "Client and the Broker MUST perform mutual > > authentication" if so. > > > > [CS: Revised. Unless the Client publishes and subscribes to only public > topics, > the Client and the Broker MUST perform mutual authentication.] > > > > > o "TLS:Anon-MQTT:ace": The token is transported inside the CONNECT > > message, and MUST be validated using one of the methods described > > in Section 2.2.2. This option also supports a tokenless > > connection request for AS discovery. > > > > We should probably look carefully at the "for AS discovery" phrasing, in > > light of the late changes in how the framework talks about the AS > > Request Creation Hints. > > > > [CS: Follows the core document already.] I think I wanted to emphasize that this discovery mechanism is unauthenticated and that clients need to have some other way to confirm that the provided AS URI is authorized to be an AS used by the client. I will put some proposed text in my pending PR. > > > > > o "TLS:Known(RPK/PSK)-MQTT:none": For the RPK, the token MUST have > > been published to the "authz-info" topic. For the PSK, the token > > MAY be, alternatively, provided as an "identity" in the > > "identities" field in the client's "pre_shared_key" extension in > > TLS 1.3. The TLS session set-up is as described in DTLS profile > > for ACE [I-D.ietf-ace-dtls-authorize]. > > > > A couple notes here: First, the DTLS profile primarily uses (D)TLS 1.2 > > terminology, with the DTLS 1.3 flow as an addendum. I'm actually happy > > for us to only or primarily talk about the TLS 1.3 idiom here in this > > document, but we may want to think about what phrasing we use for the > > relationship between our usage and the DTLS profile. Second, the DTLS > > profile is for, well, DTLS, not TLS. We should acknowledge that and > > state the (lack of) effect on the procedures to follow. > > > > [CS: The DTLS profile is now only used informative. We cannot support DTLS, > as it would require a completely different > MQTT protocol - MQTT-SN] > > > > > > It is RECOMMENDED that the Client implements "TLS:Anon-MQTT:ace" as a > > first choice when working with protected topics. However, depending > > on the Client capability, Client MAY implement "TLS:Known(RPK/PSK)- > > MQTT:none", and consequently "TLS:Anon-MQTT:None" to submit its token > > to "authz-info". > > > > It's good that we provide guidance on which of the authentication > > schemes are preferred (since we offer both ACE-layer and TLS-layer > > schemes). However, we will surely be asked to defend why there are two > > possible ways of doing it instead of just one, and this text doesn't > > really do a good job of that. What might cause a need to implement > > TLS:Known(RPK/PSK)-MQTT:none? > > > > [CS: Clarified. > It is RECOMMENDED that the Client implements TLS:Anon,MQTT:ace as the > first choice when working with protected topics. However, MQTT > v3.1.1 clients that do not prefer to overload username and password > fields for ACE (as described in Section 6) MAY implement > TLS:Known(RPK/PSK),MQTT:none, and consequently TLS:Anon,MQTT:None to > submit their token to "authz-info".] > > The Broker MUST support "TLS:Anon-MQTT:ace". To support Clients with > > different capabilities, the Broker MAY provide multiple client > > authentication options, e.g. support "TLS:Known(RPK)-MQTT:none" and > > "TLS:Anon-MQTT:None", to enable RPK-based client authentication, but > > fall back to "TLS:Anon-MQTT:ace" if the Client does not send a client > > certificate (i.e. it sends an empty Certificate message) during the > > TLS handshake. > > > > [Just a potential nit about the wording: "fall back" implies some > > ordering requirements, but IIRC use of RPK requires sending the token to > > authz-info before starting the new TLS connection, which doesn't quite > > match the steps as ordered in this description.] > > > > [CS: Revised. > > The Broker MUST support TLS:Anon,MQTT:ace. To support Clients with > different capabilities, the Broker MAY provide multiple client > authentication options, e.g. support TLS:Known(RPK),MQTT:none and > TLS:Anon,MQTT:None, to enable RPK-based client authentication. > ] > > > > [SNIP] > > > > Section 2.2.2 > > > > The Broker MUST verify the validity of the token (i.e. through local > > validation or introspection, if the token is a reference) as > > described in Section 2.2.5. If the token is not valid, the Broker > > MUST discard the token. Depending on the QoS level of the PUBLISH > > > > I see that this is covered in the framework (and we reference the > > appropriate section already), but I'd consider reiterating that "not > > valid" includes "the Broker is not an intended audience of the token". > > > > [CS: Added. > The Broker MUST check if the access token is still valid, > if it is the intended destination (i.e., the audience) > of the token, and if the token was issued by an authorized > authorization server. > ] > > > > [SNIP] > > > > Section 2.2.3 > > > > Similarly, the Broker MUST NOT process any packets before it has sent > > a CONNACK. The only exceptions are DISCONNECT or an AUTH response > > from the Client. > > > > nit: in some pedantic sense, these two sentences are in conflict, since > > the latter violates the "MUST NOT". Who cares and how much has varied > > over time, especially on the IESG, so it's not clear that we actually > > need to change anything right now. > > > > [CS: Fixed. > Similarly, except for a DISCONNECT and AUTH response from the Client, > the Broker MUST NOT process any packets before sending a CONNACK. > ] > > > > > > > +------------------------------------------------------+ > > |CPT=1 | Rsvd.|Remaining len.| Protocol name len. = 4 | > > +------------------------------------------------------+ > > > > Figure 2 appears to show that the Remaining Length field of the fixed > > header occupies a single octet, but IIUC it's encoded as a variable byte > > integer. Fixing that would also let us put the separator that appears > > after it on a proper byte boundary. > > > > [CS: The figure is revised.] > > > > > > | 'M' 'Q' 'T' 'T' | > > +------------------------------------------------------+ > > > > This figure does not show the two-byte length prefix for the string. > > > [CS: The previous field Protocol name len: 4 shows that] > > [SNIP] > > > > > > In MQTT v5.0, the Client signals a clean session (i.e. the session > > does not continue an existing session), by setting the Clean Start > > Flag to 1 and, the Session Expiry Interval to 0 in the CONNECT > > message. [...] > > > > I don't understand why setting the Session Expiry Interval to 0 is > > needed to produce a clean session. As I understand it, setting this > > interval to 0 merely directs the server to not store session state after > > the client disconnects from this session, which is unrelated to whether > > or not this is a new session or a reused session. > > > > [CS: Revised > In MQTT v5.0, the Client signals a clean session (i.e., that the session > does not continue an existing session) > by setting the Clean Start Flag to 1 in the CONNECT > packet. In this profile, the Client SHOULD always > start with a clean session. The Broker MAY also signal that it does not > support > session continuation by setting Session Expiry Interval to 0 in the CONNACK. > If the Broker starts a clean session, the Broker MUST set the Session > Present flag to 0 in the CONNACK packet to signal > this to the Client. > ] > > In this profile, the Broker SHOULD always start with a > > clean session regardless of how these parameters are set. Starting a > > clean session helps the Broker avoid keeping unnecessary session > > state for unauthorised clients. If the Broker starts a clean > > > > This SHOULD seems highly problematic to me. It looks like it > > contradicts a hard requirement of MQTT 5.0 ("If a CONNECT packet is > > received with Clean Start set to 0 and there is a Session associated > > with the Client Identifier, the Server MUST resume communications with > > the Client based on state from the existing Session"). If we want to > > recommend that the broker does not maintain session state, that should > > be implemented by setting the Session Expiry Interval in the CONNACK, > > not as part of the CONNECT processing. > > > > [CS: Revised as above.] > > > > When reconnecting to a Broker that supports session continuation, the > > Client MUST still provide a token, in addition to using the same > > Client identifier, setting the Clean Start to 0 and supplying a > > Session Expiry interval in the CONNECT message. The Broker MUST > > > > (As above, setting a Session Expiry interval seems to relate to the > > subsequent connection, not the current one, so this feels > > over-specified.) > > > > [CS: Revised and the part about Session Expiry Interval removed.] > > > > > > Note that, according to the MQTT standard, the Broker uses the Client > > identifier to identify the session state. In the case of a Client > > identifier collision, a client may take over another client's > > session. [...] > > > > Just to confirm: the ACE token is not used to provide authorization to > > use a given client identifer; the client identifier is just used as an > > unauthenticated identifier? We might consider calling that out > > explicitly. > > > [CS: Clarified. > It must be noted that the Client Identifier is an unauthenticated identifier > used within the MQTT protocol and so is not bound to the access token.] > > > > > [SNIP] > > > > Section 2.2.4.1 > > > > For this option, the Authentication Data MUST contain the two-byte > > integer token length, the token, and the keyed message digest (MAC) > > or the Client signature (as shown in Figure 4). [...] > > > > Does this go in the AUTH (which is what §2.2.4 claims to cover) or the > > initial CONNECT? (Hint: later in the paragraph the broker replies to it > > with a CONNACK.) > > > > [CS: Clarified. > "For this option, the Authentication Data inside the client's CONNECT > MUST contain the two-byte integer token length, the token, and the > keyed message digest (MAC) or the Client signature (as shown in > Figure 4). " > ] > > > > > > Section 7.5 of [RFC8446]). This content is exported from the TLS > > session using the exporter label 'EXPORTER-ACE-MQTT-Sign-Challenge', > > an empty context, and length of 32 bytes. [...] > > > > While an empty context should provide ample protection, it seems to me > > that we could consider using the client identifier as the context. > > There can also be value in incorporating information on the server > > identity in the output. If the SNI extension is used, that information > > would already be included in the key schedule, though we do not > > currently seem to mandate SNI usage. Current best practices for new > > deployments are to always use SNI and always use ALPN, so we should > > probably consider both of those. > > > > [CS: Kept empty context, as the Client Identifier is in the application > layer message > and not known to the Broker before the TLS handshake. Or I am > misunderstanding something. Ah, good point about the broker not knowing the client ID at this point. So we need to keep the empty context (as you did). > Added references for SNI and ALPN. > "Additionally, TLS 1.2 implementations SHOULD use "Extended Master Secret" > [RFC7627] to > include parts of the handshake transcript into the master secret, and > SNI (Server Name Indication) [RFC6066] and APLN (Application-Layer > Protocol Negotiation) [RFC7301] extensions." > > > > > > a CONNACK with the appropriate response code. The client cannot > > reauthenticate using this method during the same session ( see > > Section 4). > > > > Depending on what "session" means, this restriction may be too strict. > > We should probably be more clear about what "session" means...if it's > > the MQTT session, I think it's okay to use this method when > > re-connecting to take over the session. > > > > > [Revised. > "The Client cannot reauthenticate using this method during the same TLS > session (see Section 4)." > > > > [SNIP] > > > > Section 2.2.5 > > > > To authenticate the Client, the RS validates the signature or the > > MAC, depending on how the PoP protocol is implemented. HS256 (HMAC- > > SHA-256) [RFC6234] and Ed25519 [RFC8032] are mandatory to implement > > depending on the choice of symmetric or asymmetric validation. > > > > I think there is a decent argument (and that it's likely some other AD > > will make it) that we need to make both HS256 and Ed25519 mandatory to > > implement for the Broker, leaving only clients with the choice. > > Otherwise we can get into scenarios where interop is impossible. > > > [CS: Revised. > "HS256 (HMAC-SHA-256) [RFC6234] and Ed25519 [RFC8032] are mandatory to > implement for > the Broker. The Client MUST implement at least one of them depending > on the choice of symmetric or asymmetric validation." > > > > > > Validation of the signature or MAC MUST fail if the signature > > algorithm is set to "none", when the key used for the signature > > algorithm cannot be determined, or the computed and received > > signature/MAC do not match. > > > > Where would the "none" appear? We haven't said anything about a COSE > > encoding for the signature or MAC value, or anything like that...I > > assumed it was going to be the "raw" output from the relevant primitive > > (EdDSA, HMAC, etc.). > > > [CS: Now clarified in several places. > "For self-contained tokens, the Broker MUST process the security protection > of > the token first, as specified by the respective token format, i.e. a > CWT token uses COSE, while a JWT token uses JOSE. " > "A JWT token uses JOSE, while a CWT > token uses COSE [RFC8152] for security protection." > ] > > > > > Section 2.2.6.2 > > > > On success, the reason code of the CONNACK is "0x00 (Success)". The > > AS informs the client that selected profile is "mqtt_tls" using the > > "ace_profile" parameter in the token response. If the Broker starts > > > > The line about the AS returning "mqtt_tls" as the selected profile feels > > out of place here, where we're discussing successful MQTT > > authorization. > > > > [CS: Removed] > > > > a new session, it MUST also set Session Present to 0 in the CONNACK > > packet to signal a clean session to the Client. Otherwise, it MUST > > set Session Present to 1. > > > > (As above,) my understanding is that the Broker does not have agency > > over whether a new session is started, and must honor the client's > > request. So these "MUST set" seem out of place. > > > > [CS: Fixed as detailed above] > > > > > > If the Broker accepts the connection, it MUST store the token until > > the end of the connection. On Client or Broker disconnection, the > > Client is expected to transport a token again on the next connection > > attempt. > > > > This seems to deviate somewhat from the framework, that expects the RS > > to be prepared to store at least one token for future use, and > > recommends storing one token per PoP key. > > > > [CS: Revised. Support added and text added to several places.] > "Accepting the connection, the Broker MUST be prepared to store the > token during the connection and after disconnection for future use." > "The Broker SHOULD still be prepared to store the Client access > token for future use (regardless of the method of transport)." > And in 2.2.4.2.2. Proof-of-Possession via Broker-generated > Challenge/Response > " The Broker also uses this method if the Authentication Data does not > contain a token, but the Broker has a token stored for the connecting > client." > > > > The Broker > > SHOULD also use a cache time out to introspect tokens regularly. > > > > We will surely be asked to provide guidance on what timescale > > "regularly" indicates, if we do not proactively provide some guidance. > > > > [CS: I have not prescribed a time out value but added: > "The timeout value is application-specific and SHOULD be chosen to reduce > the risk of using stale introspection responses. > ] > > > > > > Section 3 > > > > The scope field contains the publish and subscribe permissions for > > the Client. The scope is a JSON array, each item following the > > Authorization Information Format (AIF) for ACE [I-D.ietf-ace-aif]. > > Using the Concise Data Definition Language (CDDL) [RFC8610], the > > specific data model for MQTT is: > > > > This seems a little dicey, since we claim to allow JWT tokens as well as > > CWT. JWT "scope" is pretty tightly nailed down to be a space-separated > > list (though CWT gives much greater freedom). We probably need to have > > some text about this situation, with a phrase about how "in order to be > > compatible with the JWT scope format, we use a single scope value with > > internal structure", that this structure is also compatible with the CWT > > rules, and noting that our AIF usage prevents any internal spaces, so > > the interpretation of the scope value is unambiguous even when > > unmodified JWT libraries are used. > > > [In the new section 2.3, this is clarified: > "The scope in the token is a single value. For a JWT, the single > scope has an internal structure of a JSON array, and for a CWT, this > information is represented in CBOR following the Authorization > Information Format (AIF) for ACE [I-D.ietf-ace-aif]." I have more to say here but will defer it to my other comments. Everything else looks great; well done! Thanks again, Ben > > > > > 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 in the scope array). > > > > We might want to say a little more about when this check happens. My > > intuition is that it occurs during the CONNECT processing where the > > actual Will message is provided, and that the connection would be > > rejected if the Will message is unauthorized. But this section is > > titled "Authorizing PUBLISH and SUBSCRIBE Messages", so one might be > > forgiven for assuming that CONNECT is out of scope... > > > [CS: Section 2.3 clarifies this: > "During the CONNECT, if the Will Flag is set to 1, the Broker MUST > authorize the publication of the Will Topic and message using the > token's scope field. The token or its introspection result MUST also > be cached to allow a Client's future PUBLISH and SUBSCRIBE packets." > > "The Broker uses the scope to match against the Topic > Name in a PUBLISH packet (including Will Topic in the CONNECT) or a > Topic Filter in a SUBSCRIBE packet." > > "The non-empty scope > used to authorize the Will Topic, if provided, in the CONNECT packet, > during connection setup, and if the connection request succeeds, the > Topic Names or Topic Filters requested in the future PUBLISH and > SUBSCRIBE packets. For the authorization to succeed, the Broker MUST > verify that the topic name or filter in question is either an an > exact match to or a subset of at least one 'topic_filter' in the > scope." > > "If the Client does not provide a valid token or omits the > Authentication Data field and the Broker has no token stored for the > Client or the token or Authentication data are malformed, or if the > Will flag is set, the authorization checks for the Will topic fails, > authentication fails. The Broker responds with the CONNACK reason > code "0x87 (Not Authorized)" or any other applicable reason code." > ] > > > > > > Section 3.2 > > > > subscription to the particular topic). The Broker sends a PUBLISH > > message with the Topic name to all the valid subscribers. > > > > (nit) It seems a little strange to mention specifically the Topic name > > but say nothing about there being a payload along with it. > > > [CS: Revised. > " To forward PUBLISH packets to the subscribing Clients, the Broker > identifies all the subscribers that have valid matching topic > subscriptions to the Topic name of the PUBLISH packet (i.e., the > tokens are valid, and token scopes allow a subscription to this > particular Topic name). The Broker forwards the PUBLISH packet to > all the valid subscribers." > > > > > > Section 3.3 > > > > On receiving the SUBSCRIBE message, the Broker MUST use the type of > > message (i.e. SUBSCRIBE) and the Topic Filter in the message header > > to match against the scope field of the stored token or introspection > > result. The Topic Filters MUST be equal or a subset of at least one > > of the 'topic_filter' fields in the scope array found in the Client's > > token. > > > > I suggest being very explicit about whether the wildcards in the token > > scope are expanded as part of matching the topic filter in the request, > > or if the SUBSCRIBE message must use topic filters that match > > byte-for-byte the permissions granted by the token. > > > > [CS: Revised. > " On receiving the SUBSCRIBE packet, the Broker MUST use the type of > packet (i.e. SUBSCRIBE) and the Topic Filter in the packet header to > match against the scope field of the stored token or introspection > result. The Topic Filters MUST be an exact match to or be a subset > of at least one of the 'topic_filter' fields in the scope array found > in the Client's token. For example, if a client sends a subscription > request for topic "a/b/*", and has a token token permits "a/*", this > is a valid subscription request, as "a/b/*" is a subset of "a/*". > (The process is similar to a Broker matching the Topic Name in a > PUBLISH packet against the Subscriptions known to the Server.)"] > > > > > > Section 4 > > > > Authentication Data. The Broker accepts reauthentication requests if > > the Client has already submitted a token (may be expired) and > > validated via the challenge-response PoP. Otherwise, the Broker MUST > > deny the request. If the reauthentication fails, the Broker MUST > > send a DISCONNECT with the reason code "0x87 (Not Authorized)". > > > > Is this correct? It seems to say that if the initial CONNECT used the > > TLS exporter for PoP, then it's forbidden to use the challenge-response > > method for PoP and thus impossible to reauthenticate on that connection. > > I don't understand why such a limitation would be needed or useful. > > > > [CS: Revised. > "To re-authenticate, the > Client MUST NOT use Proof-of-Possession using a challenge from the > TLS session during the same TLS session to avoid re-using the same > challenge value from the TLS-Exporter. For re-authentications in the > same TLS-session, the Client MUST use the challenge-response PoP as > defined in Section 2.2.4.2.2. The Broker accepts reauthentication > requests if the Client has already submitted a token (may be > expired), for which it performed proof-of-possession." > > > > > Section 5 > > > > In the case of a Client DISCONNECT, the Broker deletes all the > > session state but MUST keep the retained messages. By setting a > > > > As written, this seems to imply that all client DISCONNECT messages > > result in the loss of session state. I didn't (quickly) find a clear > > statement one way or the other in the MQTT spec, but it does seem that > > it's allowed to send a nonzero session expiry interval in a DISCONNECT, > > which seems to imply that such DISCONNECT messages do not cause all > > session state to be discarded. > > > > [CS: Revised. > "In the case of a Client DISCONNECT, if the Session Expiry Interval is > set to 0, the Broker doesn't maintain session state but MUST keep the > retained messages. If the Broker maintains session state, the state > MAY include token and its introspection result (for reference tokens) > in addition to the MQTT session state." > > > > > [SNIP] > > > > In case of disconnections due to network errors or server > > disconnection due to a protocol error (which includes authorization > > errors), the Will message is sent if the Client supplied a Will in > > the CONNECT message. The Client's token scope array MUST include the > > > > This "if the Client supplied a Will in the CONNECT message" implies that > > authorization checks are performed at time of CONNECT... > > > > [CS: Revised as explained above for will authorization.] > > > > > > Will Topic. The Will message MUST be published to the Will Topic > > > > ... but "scope array MUST include the Will Topic" suggests an > > authorization check when the Will message is actually sent. Is it one, > > the other, or both checks that must pass? > > > > [CS: Revised as explained above for will authorization.] > > > > > > Section 6.1 > > > > [My earlier comments about the MQTT header layout apply to Figure 11 as > > well.] > > > > [CS: Fixed] > > > > > The Broker SHOULD NOT accept session continuation. To this end, the > > Broker ignores how the Clean Session Flag is set, and on connection > > success, the Broker MUST set the Session Present flag to 0 in the > > CONNACK packet to indicate a clean session to the Client. [...] > > > > As above, this seems to violate the MQTT normative requirements (but I > > mostly only read about MQTT 5, not 3.1.1). > > > [CS: Fixed as discussed above.] > > > > > > The CONNECT in MQTT v3.1.1 does not have a field to indicate the > > authentication method. To signal that the Username field contains an > > ACE token, this field MUST be prefixed with 'ace' keyword, which is > > followed by the access token. [...] > > > > An example of what this looks like would be helpful. It sounds like we > > just take the first three bytes for the sentinel and then go directly to > > the access token, vs having some kind of "separator" as part of the > > sentinel value. > > > > [CS: Added: > "The CONNECT in MQTT v3.1.1 does not have a field to indicate the > authentication method. To signal that the Username field contains an > ACE token, this field MUST be prefixed with 'ace' keyword, i.e., the > Username field is a concatenation of 'a', 'c', 'e' and the access > token represented as: > > 'U+0061'||'U+0063'||'U+0065'||UTF-8(access token)" > > > > > In MQTT v3.1.1, the MQTT Username is a UTF-8 encoded string (i.e. is > > prefixed by a 2-byte length field followed by UTF-8 encoded character > > data) and may be up to 65535 bytes. Therefore, an access token that > > is not a valid UTF-8 MUST be Base64 [RFC4648] encoded. (The MQTT > > Password allows binary data up to 65535 bytes.) > > > > Data-dependent encoding transformation without explicit signaling is a > > really bad idea. I think we need to always base64-encode the token. > > We should also specify a section reference to RFC 4648 (for plain base64 > > vs base64url) and probably make a statement about whether padding > > characters are retained or omitted. > > > [CS: Fixed. > "To this end, the access token MUST be base64url encoded, omitting the > '=' padding characters [RFC4648]." > > > > > > Section 6.2 > > > > o RS-Client PUBLISH authorization failure: When RS is forwarding > > PUBLISH messages to the subscribed Clients, it may discover that > > some of the subscribers are no more authorized due to expired > > tokens. These token expirations SHOULD lead to disconnecting the > > Client rather than silently dropping messages. > > > > (I'm not actually sure how much the MQTT spec says about this type of > > scenario, and thus whether the "SHOULD" is the right term to use.) > > > [CS: Switched to MUST. > "These token expirations MUST lead to disconnecting the > Client rather than silently dropping messages." > > > > > > Section 7.1 > > > > 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]. > > > > We need to specify the other columns in the registry. > > I think we can have: > > DTLS-OK: Y > > Recommended: Y > > Reference: [this document] > > > [CS: This was registered as DTLS-OK: N, and Recommended: N] > > > > [SNIP] > > > > Section 7.3 > > > > The following registrations are done for the ACE OAuth Profile > > Registry following the procedure specified in > > [I-D.ietf-ace-oauth-authz]. > > [...] > > o CBOR Value: > > > > I think it's clearer for reviewers if we put "TBA" or "to be assigned by > > IANA" here. > > > [CS: Done] > > > > > > o Description: Profile for delegating Client authentication and > > authorization using MQTT as the application protocol and TLS For > > transport layer security. > > > > It seems like it might be preferred to talk about using MQTT for the > > C/RS interactions and HTTP for the interactions with the AS. > > Separately, mention that TLS is used for confidentiality/integrity > > protection and server authentication; also, client authentication can be > > provided either via TLS or using in-band proof of possession at the MQTT > > application layer. > > > [CS: Revised. > "Description: Profile for delegating Client authentication and > authorization using MQTT for the Client and Broker (RS) > interactions, and HTTP for the AS interactions. TLS is used for > confidentiality and integrity protection and server > authentication. Client authentication can be provided either via > TLS or using in-band proof-of-possession at the MQTT application > layer." > > > > > > Section 8 > > > > revoked topics. If the RS caches the token introspection responses, > > then the RS SHOULD use a reasonable cache timeout to introspect > > > > ("reasonable cache timeout" again) > > > [CS: Tried to clarify as: > "The timeout value is > application-specific and SHOULD be chosen to reduce the risk of using > stale introspection responses."] > > > > > > If the RS supports the public > > "authz-info" topic, described in Section 2.2.2, then this may be > > vulnerable to a DDoS attack, where many Clients use the "authz-info" > > public topic to transport fictitious tokens, which RS may need to > > store indefinitely. > > > > We do say that the RS only stores "valid" tokens, which includes being > > generated by a trusted AS and having RS as the audience. So it's not > > clear that this statement is accurate if the attack is to involve > > "fictitious" tokens. Similar attacks are possible, though. > > > > [CS: Revised as > "If the Broker supports the public > "authz-info" topic, described in Section 2.2.2, then this may be > vulnerable to a DDoS attack, where many Clients use the "authz-info" > public topic to transport tokens that are not meant to be used, and > which the Broker may need to store until the tokens expire."] > > > > Section 10.1 > > > > [SNIP] > > > > RFC 8447 probably does not need to be normative; it is just mentioned as > > the thing that is the reference for the IANA registry of TLS exporter > > values. > > > [CS: Fixed] > > > > > Section 10.2 > > > > I think the DTLS profile needs to be a normative reference, since we > > defer to it for TLS session set-up. > > > [CS: Kept informative as explained above.] > > > > > > Likewise, RFC 6234 and RFC 8032 specify MTI algorithms for > > authentication, which would make them normative. > > > [CS: Fixed.] > > > > > Appendix A > > > > I think we need to update the checklist to match the current template > > from > > > > https://datatracker.ietf.org/doc/html/draft-ietf-ace-oauth-authz-43#appendix-C > > [CS: Revised according to template] > > > > > > > > -Ben > > _______________________________________________ Ace mailing list Ace@ietf.org https://www.ietf.org/mailman/listinfo/ace
