Subject: [Ace] Review of draft-sengul-ace-mqtt-tls-profile-02

From: Ludwig Seitz <[email-masked]>
Date: Tue, 15 May 2018 11:04:51 +0200

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

---

From: Cigdem Sengul <[email-masked]>
Date: Tue, 15 May 2018 21:31:24 +0100

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 <[email-masked]> wrote:

> 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 ..."
>
> 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).
>
> Please make it explicit already here that this is the Proof-of-possession
> mechanism for the key associated to the token.
>
> 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:
>
> 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:
>
> 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:
>
> _______________________________________________
> Ace mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ace
>