Re: [Ace] WGLC feedback on draft-ietf-ace-cwt-proof-of-possession-02

[Samuel Erdtman <[e-mail]>]

Se comments inline

On Wed, May 23, 2018 at 7:40 PM, Roman Danyliw <[e-mail]> wrote:

> Hello!
> (chair hat off)
>
> I reviewed draft-ietf-ace-cwt-proof-of-possession-02 in WGLC and have the
> following feedback:
>
> (1) (Editorial) Page 3, "The value of the 'cnf' claim is a CBOR map and
> the ...".  Isn't it more precise to say "The _representation_ of the 'cnf'
> claim is a CBOR map ..."?
>

I have no strong opinion here, what does the other authors say?


>
> (2) (Editorial) Page 3,  Why is the sentence "(In some applications, the
> subject identifier ..." in parenthesis?
>

This is copied from RFC7800.


>
> (3) (Editorial) Page 4, Section 3.0, I read to the end of this section by
> which point there has been discussion of "sub" or "iss".  I was left
> wondering about how to interpret the case where both are present and none
> are.
>

I agree it could be clearer especially with regards to non being present.
One case for when both could be present is when the authorization is the
issuer signing an access token that is valid for a specific client and
subject is used to look up the PoP key at the receiver (I think this case
relates to Jims comment about using kid as cnf, by combining kid with
subject it would be unique).
When non is present the application context would have to enable the
receiver to identify the user of the token if needed. one might want
anonymity.
With that said some additional text is needed, but the text will be looked
over based on Jims comment so we should address this too then.



>
> (4) (Editorial) Page 4, Section 3.1, Per " At most one of the "COSE_Key"
> and    "Encrypted_COSE_Key" confirmation values defined below may be
> present."  Defined below where specifically?  I recommend citing Figure 1
> explicitly by s/below/in Figure 1/.
>

I agree (PR submitted)


>
> (5) Page 5, Typo in Section 3.2, s/diagonstic/diagnostic/
>

Thanks (PR submitted)


> (6) (Editorial) Page 5, Section 2.3, Per "If the CWT is not encrypted, the
> symmetric key MUST be encrypted as described below."  Described where
> below?  I recommend citing Section 3.3 by s/below/In Section 3.3/
>

I agree (PR submitted)


>
> (7) Page 6, Section 3.3, The sentence "The COSE_Key could, for instance,
> be encrypted using a COSE_Encrypt0 representation using the
> AES-CCM-16-64-128 algorithm" seems out of place.  What is this text
> explaining relative to the examples?
>

It is a bit unclear, but it more or less says what the example below says.
I think the section as a whole could be clearer. (issue created
https://github.com/cwt-cnf/i-d/issues/10)


>
> (8) Page 7, Section 3.4, Per "The proof-of-possession key can also be
> identified by the use of a Key ID instead of communicating the actual key,
> provided the recipient is able to obtain the identified key using the Key
> ID."  How would the sender know whether the recipient can "obtain the key
> using the Key ID"?  Additionally, I would recommend adding language that
> states that this retrieval process is out of scope for this draft.
>

The sender needs to know from the application context what cnf method to
use. I think we should add both this and a comment saying that retrieval
process is out of scope


>
> (9) (Editorial) Page 7, Section 3.4, Per "In this case, the issuer of a
> CWT declares that the presenter possesses a particular key and that the
> recipient can cryptographically  confirm proof of possession of the key by
> the presenter by including a "cnf" claim in the CWT whose value is a CBOR
> map with the CBOR map containing a "kid" member identifying the key."  I
> recommend s/is a CBOR map with the CBOR map/is a CBOR map/
>

I would like to go even further and change
"In this case, the issuer of a CWT declares that the presenter possesses a
particular key and that the recipient can cryptographically  confirm proof
of possession of the key by the presenter by including a "cnf" claim in the
CWT whose value is a CBOR map with the CBOR map containing a "kid" member
identifying the key."
to
"In this case, the issuer of a CWT declares that the presenter possesses a
particular key and that the recipient can cryptographically confirm proof
of possession of the key by the presenter by including the "kid" member
identifying the key in the "cnf" claim in the CWT. "

It is stated earlier that cnf is a CBOR map so i think we could skip it
here.



>
> (10) Page 8, Section 4.  RFC 2119 capitalizations seem more appropriate in
> these sentences:
>
> (MUST) "Appropriate means *must* be used to ensure that unintended parties
> do not learn private key or symmetric key values."
>

I´m not sure what is best practice, but since  "appropriate means" could
mean different things depending on context I think the upper case MUST
would not add value but rather confusion about what is it I MUST do.
Opposed to cases when it is clear e.g. value MUST be of type X.


>
> (SHOULD) "Applications utilizing proof of possession *should* also utilize
> audience restriction, as described in Section 4.1.3 of [JWT], as it
> provides different protections."
>

I agree (issue created https://github.com/cwt-cnf/i-d/issues/8)


> (MUST) "Applications that require the proof-of-possession keys
> communicated with it to be understood and processed *must* ensure that the
> parts of this specification that they use are implemented."
>

I agree (issue created https://github.com/cwt-cnf/i-d/issues/8)


>
> (11) Page 8, Section 4. Per   "Applications utilizing proof of possession
> should also utilize audience restriction, as described in Section 4.1.3 of
> [JWT], as it provides different protections."  I recommend
> s/different/additional/
>

I agree (PR submitted)


> (12) (Editorial) Page 8, Section 4, Per
>
>    Applications utilizing proof of possession should also utilize
>    audience restriction, as described in Section 4.1.3 of [JWT], as it
>    provides different protections.  Proof of possession can be used by
>    recipients to reject messages from unauthorized senders.  Audience
>    restriction can be used by recipients to reject messages intended for
>    different recipients.
>
> The flow of this sentence is off for me.  Sentence #1 asserts the need for
> audience restrictions.  Sentence #3 describes the benefit of Sentence #1.
> Sentence #2 interjects with something that seems unrelated to this
> need-benefit pairing.
>

I agree (Issue create https://github.com/cwt-cnf/i-d/issues/9)


> (13) Page 8, Section 4, Per "Applications that require the
> proof-of-possession keys communicated with it to be understood and
> processed must ensure that the parts of this    specification that they use
> are implemented."  How is this being ensured?  What happens if the needed
> parts aren't specified?  Also, it seems like the "must" here should be
> "MUST".
>




>
> (14) (Editorial)  Page 8, Section 4, Per "Replay can also be avoided if a
> sub-key is derived from a shared secret that is specific to the instance of
> the PoP demonstration."  PoP is spelled out everywhere else in this draft
> but here.  Yes, the acronym is defined, but for readability, I recommend
> against it using it and consistently spelling it out here too.
>

I could go either way, changing all other place or this one, but for
simplicity lets change this one place for now. (PR submitted)


>
> (15) Page 8, Section 4, Per "Special care has to be applied when carrying
> symmetric keys inside the CWT since those not only require integrity
> protection but also  confidentiality protection."  What is that care?
>

I would say that "care" in this case comes down to either having an signed
and encrypted CWT or a signed CWT with an encrypted cnf key


>
> (16) Page 8, Section 4.  Per "Proof-of-possession signatures made with
> keys not meeting the application's trust criteria MUST NOT not be relied
> upon.", remove the double "not" by s/MUST NOT not/MUST NOT/.
>

Thanks (PR submitted)


>
> Roman
>
>
> _______________________________________________
> Ace mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/ace
>