Hello Ludwig, Thank you for reviewing our draft. We will start working on addressing your comments asap.
Thanks, --Cigdem On Tue, May 15, 2018 at 10:04 AM, Ludwig Seitz <ludwig.se...@ri.se> wrote: > Hello ACE, > > I've reviewed draft-sengul-ace-mqtt-tls-profile-02. I think this is a > very relevant draft, due to the amount of IoT devices that use MQTT. I > would encourage the WG to pick this up as the third profile of > draft-ietf-ace-oauth-authz > > Detailed comments below. > > > /Ludwig > > > > 1. Introduction > > " Section 4 of the ACE framework [I-D.ietf-ace-oauth-authz]" > > If you make that "Section 4 of [I-D.ietf-ace-oauth-authz]" the IETF > html-izer will not turn this into a link to section 4 of your draft (I > think). > See: https://xml2rfc.tools.ietf.org/xml2rfcFAQ.html#anchor18 > > 1.1. Requirements Language > > There is a new recommendation how this section should be worded: > > "The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", > "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and > "OPTIONAL" in this document are to be interpreted as described in BCP > 14 [RFC2119] [RFC8174] when, and only when, they appear in all > capitals, as shown here." > > 1.2. ACE-Related Terminology > > "The term "Resource" is used to refer to an MQTT "topic", which is > defined in Section 1.2." > > This references back to the same section, perhaps this was meant to > reference to some other part of the draft? > > 2.1. Authorizing Connection Establishment > > Figure 1 could clarify that steps E and F are optional. > > 2.1.1. Client Authorization Server (CAS) and Authorization Server (AS) > Interaction > > "and the client is authorized to obtain a token for the indicated > audience (e.g., topics) and scopes (e.g., publish/subscribe > permissions)" > > Note that the audience claim is typically used to identify the RS (so in > this case the MQTT broker), while the scope is intended to identify both > the resource (=topic) and the actions (=publish, subscribe). > See this for how OAuth scopes are typically used: > https://www.brandur.org/oauth-scope > > > "This includes a token, which is assumed to be PoP by default." > > The abbreviation PoP is not defined at this point, I would write it out as > in "... assumed to be a Proof-of-Possession (PoP) token ..." > > 2.1.2. Client connection request to the broker > > " For the basic operation described in this section, > the Username field MUST be set to the token." > > I think this refers to the access token right? I'd suggest to be more > specific, since there is room for confusion (CoAP has also the concept of a > token, and then OAuth has different types of tokens, the access token being > one of them). > > "The Password field MUST be set to the keyed message digest (MAC) or > signature." > > Please make it explicit already here that this is the Proof-of-possession > mechanism for the key associated to the token. > > "(The Username field is a UTF-8 encoded string," > > You need to specify how the (possibly binary) access token is supposed to > be converted into an UTF-8 string. Base64 encoding would be obvious, but it > needs to be explicitly stated. > > General comment: > > An example of how the CONNECT message could look like would be good. > > > 2.1.3. Token validation > > "The broker MAY cache the introspection result because it will need to > decide whether to accept subsequent PUBLISH and SUBSCRIBE messages" > > This could warrant a section in the security considerations about the > trade-off of caching introspection results (freshness vs. connectivity > requirements). > > "If the introspection result is not cached, then the RS needs to > introspect the saved token for each request." > > This is only true if the token is not self-contained. I would rephrase > this statement > > "Note: Scope strings MAY follow ..." > > Note that a scope is a "space-delimited list strings" (OAuth 2.0 3.3.) > so you could include several topics in one scope as in e.g. "connect > publish_topic1 publish_topic2 subsrcibe_topic3" > > The use of 'aud' as described in this paragraph is definitely not the > intended use, and I would not recommend this type of use. See > https://tools.ietf.org/html/rfc7519#section-4.1.3 for the definition of > aud. > > > 2.4. Token expiration > > "The token validation is done either by checking the 'exp' claim of a > CWT/JWT or via performing an introspection request with the > Authorization server as described in Section 8.2 of the ACE framework > [I-D.ietf-ace-oauth-authz]." > > The word "token validation" here could be misconstrued to ignoring crypto > wrappers, token issuers ect. While it is clear from the context (to me) > that you only refer to checking the expiration of a token, the full > validation of a token would include the following steps: > > 1. Check that the issuer of the token (= AS) is acceptable > 2. Check the cryptographic wrapper of the token (may be MAC or signature or > AEAD encryption) > 3. Check that the token contains all expected claims > 4. Check that the "exp" and "nbf" claims are fulfilled if present > 5. Check that the "aud" claim applies to the recipient if present > > > 3.1. Token Transport via Authentication Exchange (AUTH) > > "The first option is that Authentication data contains both the token > and the keyed message digest (MAC) or signature as described in > Section 2.1.2." > > You need to specify how these two items are encoded. I would suggest a > CBOR array since this property expects binary data. An example would also > be very helpful here. > > "... RS responds with a CONNACK reason code '0x87 (Not Authorized)' and > includes a User Property set to the address of the AS." > > You need to specify the format of this user property, in order to mirror > the framework's section 5.1.2. I guess you could use either a JSON map > or binary data containing a CBOR map for this. > > > 4. IANA Considerations > > " This memo includes no request to IANA." > > This should instead register the new profile identifier "mqtt_tls" in the > soon-to-be-created IANA registry for ACE profile identifiers. See the DTLS > profile for example: > > https://tools.ietf.org/html/draft-ietf-ace-dtls-authorize-03#section-7 > > > 5. Security Considerations > > You should add some text here about the security implications of the > limitations of MQTT v3.1 (client disconnect only). > I could imagine that you could perform some attacks on a client that > hasn't realized it was disconnected for example. > > > -- > Ludwig Seitz, PhD > Security Lab, RISE SICS > Phone +46(0)70-349 92 51 > > _______________________________________________ > Ace mailing list > Ace@ietf.org > https://www.ietf.org/mailman/listinfo/ace >
_______________________________________________ Ace mailing list Ace@ietf.org https://www.ietf.org/mailman/listinfo/ace
