Re: [Ace] AD review of draft-ietf-ace-oscore-profile-08

[Benjamin Kaduk <[e-mail]>]

Hi Francesca,

Sorry to cut this so close to the wire...I was on vacation and came back to
a sizeable inbox.

On Mon, Feb 17, 2020 at 03:55:29PM +0000, Francesca Palombini wrote:
> Hi Ben,
> 
> I am now done with updating the draft following most of your review comments. Out of the 105, I have implemented 88 in the draft, while those left need agreement in the wg as previously mentioned, or require your OK (see inline).
> 
> You can see the result of the update in the PR here: https://github.com/ace-wg/ace-oscore-profile/pull/25  If you give me the go-ahead, I will merge this PR and submit a new version, and then we can continue working on the open issues.

I will have to download the changes locally to view them, as the in-browser
viewer does not do well with the long lines.

> These open issues are:
> 
> * The mechanism of letting the RS pick the identifier of the client is not worth the additional complexity.
> 	6, 7, 32, 61, 65,
> * Recommendation about length of nonces N1 and N2 to use.
> 	5, 52
> * Define and register 2 new ACE parameters to transport the nonces used in the exchange, instead of using "cnonce".
> 	3,  53, 60
> * Check with IANA
> 	102
> 
> I hope to discuss these in the mailing list (continuing on the separate thread), and at the interim. 
> 
> I will cut and paste the rest (46 , 47,  58 , 67 , 92 + a couple of more) which I have answered below, whatever is not reported below I found no issue in doing the modifications you suggested, or is covered by the open points I mentioned. Please do bring any of those I do not touch on up again if you feel they were not solved in the PR.
> 
> Thanks again for your extensive review!
> Francesca
> 
> On 03/02/2020, 05:06, "Benjamin Kaduk" <[e-mail]> wrote:
>     > 29.       The AS MUST also assign an identifier to the RS (serverId), MAY
>     >        assign an identifier to the client (clientId), and MAY assign an
>     >        identifier to the context (contextId).  These identifiers are then
>     >        used as Sender ID, Recipient ID and ID Context in the OSCORE context
>     >        as described in section 3.1 of [I-D.ietf-core-object-security].  The
>     >        couple (client identifier, context identifier) MUST be unique in the
>     >        set of all clients on a single RS.  Moreover, when assigned,
>     >        serverId, clientId and contextId MUST be included in the
>     >        OSCORE_Security_Context, as defined in Section 3.2.1.
>     >     
>     >     When certain fields are optional, we typically have to ask whether it's
>     >     possible for the two parties to disagree about whether the field is
>     >     present.  IIUC, the OSCORE context derivation procedure includes enough
>     >     information to prevent that possibility, but it would be good if someone
>     >     would confirm that.
>     > 
>     > FP: That is correct, a number of fields have default values when non present. Hkdf, alg, salt, contextId, rpl. On the other hand, ms, clientId and serverId need to be set.
>     
>     Hmm, so there is not a great mechanism to distinguish "absent" from
>     "present with default value", but the practical consequences of such an
>     attack seem quite limited.
> 
> FP: That is right. I did not add any text about this, as this is more OSCORE implication.

That's okay.

>     >     
>     > 34.             "access_token" : h'a5037674656d7053656e73 ...'
>     >               (remainder of access token omitted for brevity)',
>     >     
>     >     In addition to the invalid-CBOR-diagnostic-notation-syntax comment, we
>     >     also have unbalanced quotes.
>     > 
>     > FP: Right, but this is not valid CBOR diagnostic notation syntax anyway, do you have a better idea? I do not think it would be useful to have the full token here. Will remove the unbalanced quote.
>     
>     This is probably manageable, though we do often see people put a disclaimer
>     before the example that <certain fields> are truncated for readability;
>     some other ADs may have other suggestions, too.
> 
> FP: Thanks for the input text :) Now added.
> 
>     ...
> 
>     > 43.       hkdf:  This parameter identifies the OSCORE HKDF Algorithm.  For more
>     >           information about this field, see section 3.1 of
>     >           [I-D.ietf-core-object-security].  The values used MUST be
>     >           registered in the IANA "COSE Algorithms" registry and MUST be
>     >           HMAC-based HKDF algorithms.  The value can either be the integer
>     >     
>     >     It's a little unfortunate that we basically are relying on "I know it
>     >     when I see it" for determining which algorithms are HKDF algorithms.
>     > 
>     > FP: Agreed, but we don’t really have another way to say this. If you or COSE experts (Jim) have any idea please let us know.    
>     
>     I think we inherited this from JOSE :-/
> 
> FP: That would make sense. I have not done any modifications here, still waiting for more input if any.

I don't know of any modifications that would help.

>     >     Also, "HKDF" expands to "HMAC-based Extract-and-Expand Key Derivation
>     >     Function", i.e., is definitionally HMAC-based.  So this should be
>     >     reworded to reflect the actual intent.
>     > 
>     > FP: Right, that is because COSE also defines HKDF using AES-MAC, see Table 12 of RFC8152, so we needed to specify that only HMAC-based can be used.
>     
>     Oof, rather unfortunate IMO that the terminology was overloaded by RFC
>     8152.
> 
> FP: Agreed. Again no modifications following this point (or the analogous 44. About AEAD alg).

Okay.

>     > 46.       rpl:  This parameter is used to carry the OSCORE value, encoded as a
>     >           bstr.  This parameter identifies the OSCORE Replay Window Size and
>     >           Type value, which is a byte string.  For more information about
>     >           this field, see section 3.1 of [I-D.ietf-core-object-security].
>     >           In JSON, the "rpl" value is a Base64 encoded byte string.  In
>     >           CBOR, the "rpl" type is bstr, and has label 8.
>     >     
>     >     I tried to follow the reference, but I couldn't really find a
>     >     description of the encoding for this as a byte string (especially
>     >     noteworthy since we claim that it represents both size and type).
>     >     (Relatedly, I'm not sure why it's allowed to be either bstr or int.)
>     >     
>     > FP: Right, this is an old reference, where we actually gave a default encoding for rpl, I believe. Now this is entirely up to the application. I would keep this as bstr and specify that it is any format wrapped in a bstr.
>     
>     "Up to the application" meaning known by prior agreement among all three of
>     C, AS, and RS?
>     
> FP: There is no need for the 3 nodes to be in agreement on the replay window on the RS. After thinking a bit more about this, I think I'd like to remove this parameter completely from this object. The idea here was that the AS is able to tell the RS what type and size of replay window to use, but I do not see a reason why the AS should send this info to the RS. Window type and size is implementation related, and each RS node can have its own. 

It's not even just the AS sending this to the RS, though, right?  This
section is for the token response from AS to C, and the
OSCORE_Security_Context object is for cleartext information that the AS
gives to the C alongside the token.  (It might also be represented in the
token as well, but this is where the text in question occurs.)  I may be
forgetting something, but the replay windows sounds like it could just be
entirely local to the RS (and not something the client needs to know the
implementation of), so removing this parameter seems to make sense.  (And
after all, we have a registry now for the OSCORE_Security_Context contents,
so we could allocate the codepoint later if needed.)

>     > Also I need to fix a couple of mistakes: “OSCORE value” is wrong; “Replay Window” and not "Size and Type". Also, I think we need to define a new IANA registry for rpl with 0 as default, so that applications can define their format.
>     
>     I'm not entirely sure I understand that the registry is for (i.e., what you
>     propose).
> 
> FP: disregard that. I was thinking we might want to have a registry for different types of replay windows, but considering the comment above I do not think it's needed anymore.
>     
>     >     Section 4
>     >     
>     > 47.    Can we get some CDDL or similar for both C-to-RS and RS-to-C (in the
>     >     respective subsections)?
>     >     
>     > FP: I am not sure what you’d like to see in CDDL in this subsection, could you expand?
>     
>     I am not sure what the body of the POST and response thereto are formatted
>     as.  Maybe CDDL is not quite right, if we only want to provide an example
>     with a particular content-format but allow many formats.
>     
> FP: I think this should be covered in the examples of those 2 messages, see figures 11 and 12. Anything else needed?

I would prefer to have something that looks more like a formal syntax than
just relying on examples, though I don't insist upon it for now.  (Some
other ADs might, though.)

>     > 51.       Note that the proof-of-possession required to bind the access token
>     >        to the client is implicitly performed by generating the shared OSCORE
>     >        Security Context using the pop-key as master secret, for both client
>     >        and RS.  An attacker using a stolen token will not be able to
>     >        generate a valid OSCORE context and thus not be able to prove
>     >        possession of the pop-key.
>     >     
>     >     It may be worth saying something roughly equating "proof of possesion
>     >     performed by the RS" and "the RS authenticating itself to the client",
>     >     since there is not an external action for server authentication.
>     >     Also, ISTR getting some pushback on a different document that talked
>     >     about an attacker with a "stolen token", as something that can steal a
>     >     token from the client's local storage might also steal the associated
>     >     key.  I forget if we ended up changing to some other phrasing or not,
>     >     though :(
>     > 
>     > FP: Ok, will rephrase to “the RS authenticating itself” instead of “pop performed by the RS”. Also, what about using eavesdrop here instead?
>     
>     Eavesdrop may be too limited, as I think it's also in scope for the attack
>     when an RS receives (whether legitimately or by deception) the token in
>     question as the target of a CoAP request.
> 
> FP: Ok, did not use the term eavesdrop.
>     
>     > 58.       to claims that the RS cannot process (e.g., an unknown scope), or if
>     >        any of the expected parameters in the OSCORE_Security_Context is
>     >     
>     >     Hmm, "associated to claims that the RS cannot process" sounds like we're
>     >     telling the RS to abort if the token includes any unrecognized claims,
>     >     which IIUC is at odds with the OAuth 2.0 philosophy of ignoring
>     >     unrecognized parameters.  (Hopefully someone will correct me if I'm
>     >     misremembering that.)
>     > 
>     > FP: This paragraph is meant to mirror what specified in 5.8.1.1 of ace-oauth-authz:
>     > If the claims cannot be obtained the RS MUST discard the token
>     >       and, in case of an interaction via the authz-info endpoint, return
>     >       an error message with a response code equivalent to the CoAP code
>     >       4.00 (Bad Request).
>     > Next, the RS MUST verify claims, if present, contained in the access
>     >    token.  Errors are returned when claim checks fail, in the order of
>     >    priority of this list:
>     
>     Ah, maybe the ace framework diverges from oauth here and I misremembered.
>     But is this behavior limited to "any of the four standard claims that have
>     a value that cannot be processed" or does it also include "there was an
>     unrecognized claim"?
> 
> FP: Yes, I checked with Ludwig who confirmed that the framework diverges from oauth and that it's for any unrecognized claims. So I did not do any modifications here.

Okay, thanks for confirming and sorry for misremebmering.

>     > 67.       After sending the 2.01 (Created) response, the RS MUST set the Master
>     >        Salt of the Security Context created to communicate with the client
>     >        to the concatenation of salt, N1, and N2, in this order: Master Salt
>     >        = salt | N1 | N2, where | denotes byte string concatenation, and
>     >     
>     >     [It might be worth breaking this computation off into a standalone piece of
>     >     text that applies to both C and RS, including the edits for injectivity.]
>     >     
>     > FP: OK
> 
> FP: I tried to do this, but did not manage to implement the change in a satisfactory way that would improve readability, so I gave up. The reason is that the text that is actually the same is only "Master Salt is the concatenation of ...", while the surrounding text is very much node-specific. Also, the text is written in a way to sequentially explain client and RS processing. Moving this text around would make the reader jump from a section to another. If you have suggestions on a better way to structure this text, I'll take them. 

Thank you for trying; I don't think we need to go to great lengths to make
this refactoring happen.

>     > 76.       authorization information from the access token associated to the
>     >        Security Context.  The RS then MUST verify that the authorization
>     >        information covers the resource and the action requested.
>     >     
>     >     In the vein of my previous note about an abstract data model, we may not want
>     >     to be overly prescriptive about "retrieves [...] from the access token".
>     >     Per-request introspection is allowed, as is caching the authorization
>     >     information in a non-token local datastore.
>     > 
>     > FP: Ok, I understand but I am not sure how to implement this comment. “Retrieve” may be misinterpreted, but in my mind it can mean both by looking up caching info as well as introspecting. How to make this clearer?
>     
>     Would s/from/using/ work?
> 
> FP: Yes! Thanks.
>     
>     >     
>     > 92.       o  Profile Description: Profile for using OSCORE to secure
>     >           communication between constrained nodes using the Authentication
>     >           and Authorization for Constrained Environments framework.
>     >     
>     >     This is the registry for ACE profiles; "using the Authentication and
>     >     Authorization for Constrained Envirornments framework" feels redundant.
>     >     There's also perhaps something to say about defining "coap_oscore" for all
>     >     three interaction flows even though the procedures for two of the three are
>     >     essentially just "out of the scope of this specification", though having
>     >     *some* identifier for "use OSCORE" is probably still useful.
>     >     
>     > FP: Ok, we can remove the last part. About the interaction flows between nodes, I am not sure what you mean, as by following definition https://tools.ietf.org/html/draft-ietf-ace-oauth-authz-31#section-5.6.4.3 it is not mandatory for a profile to specify C-AS and RS-AS, those flows can be out of scope. Am I missing something?
>     
>     It's not mandatory, though given that we talk about profiles not being able
>     to assume that the same profile is used for any/all of C/RS, C/AS, and
>     RS/AS, I guess I assumed that we use the profile identifier to talk about
>     the last two cases as well as the first one.  Maybe I'm wrong to assume
>     that!
> 
> FP: From my understanding, that's not the case, as I said above. Maybe someone else can confirm.
>     
>     Regardless, I think the question here is whether we want to have people use
>     the "coap_oscore" string to indicate that C/AS and/or RS/AS use OSCORE, or
>     if that information is not expected to be encoded in any protocol element;
>     if the latter, then there's nothing to change.
> 
> FP: As of now, that information is not expected to be encoded in any protocol element, as far as I understand. Again, if anybody can confirm/shout if wrong, please do.

Okay.  I would be happy to hear from the WG if there are other known uses
like this.

>     > 96.    "OSCORE_Security_Context" feels like a somewhat long name in light of the
>     >     previous commentary about shorter names being preferred. :)
>     >     
>     > FP: Typically this whole name should not be sent when CBOR is used (so in constrained environments). But we’ll shorten it, what about "Sec_Ctx"?
>     
>     There's no need to shorten it just on my account; as you note, it's
>     generally going to be the CBOR tag that's used.
> 
> FP: I ended up shortening the name of the CWT parameter to "osc" (same as JWT), even though the CBOR label would be used instead, but kept the "OSCORE_Security_Context" as the name for the CBOR map. I think it's better to separate between the CBOR object and the CWT field's name.

That seems likely to be a good idea; I'll try to remember to look for it
when I'm reading the diff.

Thanks!

-Ben