Re: [COSE] Review of draft-jones-cose-additional-algorithms-00

[Mike Jones <[e-mail]>]

Thanks for the detailed review, Jim.  See my replies inline...

-----Original Message-----
From: Jim Schaad <[e-mail]> 
Sent: Tuesday, March 12, 2019 9:02 PM
To: [e-mail]
Cc: 'cose' <[e-mail]>
Subject: Review of draft-jones-cose-additional-algorithms-00

Here is a first pass review:

1.  I was surprised that there are two algorithms still in the WebAuthn document that are not included here.  Is that intentional and what is the plan forward for them?

You're correct that the algorithms ED256 and ED512 in https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#sctn-cose-alg-reg aren't yet in the document.  This was a matter of time to produce the draft - not a statement that they aren't important to WebAuthn.  They will be added in future drafts.

2.  The title of the document is not descriptive in terms of a stand alone sentence on what this document is doing.  It should be more specific on the algorithms.

Fair enough.  I'll plan a title change after working group adoption (assuming that happens as a result of the call for adoption).

3.  Abstract:  A list of the algorithms being processed in this document would be reasonable.  Based on the current text, the abstract would need to be re-written as soon as the algorithms are registered because they would then be registered and thus the specification is not necessary.  It would also be slightly odd in the event that the CTAP or WebAuthn protocols are updated to add new algorithms as they would not be in this document.  (See issue #1.)

Thanks.  Will revise accordingly after adoption.

4.  Section 1:  See abstract comment.

Agreed.

5.  Section 2: The recommended column needs to be added to the table.

Will do.

6.  Section 2:  The discussion on what is and is not recommended and why belongs in this section not in the security considerations.

OK

7.  Section 2: You are missing some checks that should be done on signing and verification.  Look at the standard text at the end of sections in RFC
8152 for a pattern.  You might also want to add a check on lengths of keys both for a maximum and a minimum length.

Thanks

8.  Section 3:  This should not be the first use of JOSE and COSE and therefore the expansions here make no sense.  That should be in the introduction.

OK

9. Section 3.1:  I have only rarely seen this curve as P-256K, it is almost always referred to as secp256k1.  I would ask that this be changed in the COSE representation.  I am agnostic about what should be in the JOSE representation.

I'm OK changing the string to "secp256k1" after adoption, assuming others concur.

10. Section 3.1:  I don't understand the reason for the last paragraph in the section.  It seems to me that this should just refer to all of the items defined for EC2 in COSE.

Sure

11. Section 3.1:  Is there a recommendation on using compressed vs non-compressed points?

Uncompressed should always work.  I'd like to try to understand how actual deployments are using this before commenting on whether compressed should be supported as well.  I'll plan to ask this in Prague.

12.  Section 3.1:  You need to talk about if this is going to be IETF
recommended or not.   (And why)

There was a CFRG discussion on this in June.  Dan Brown isn't in love with it but wasn't aware of specific problems with it - see https://mailarchive.ietf.org/arch/msg/cfrg/N-XU09enlfMYEbl3L4IWCM5D8WQ.  Richard Barnes pointed this study of it https://eprint.iacr.org/2014/161.pdf.  This is another thing we should discuss in Prague.

13.  Section 3.2:  I don't understand what this section is doing.  If you are really defining a new signature algorithm, then that should be noted as part of the requirements in section 3.1 on the key.  If you are not defining a new signature algorithm then this section makes zero sense.  (You are not saying what the hash algorithm is.)  Both options are reasonable.

It's defining the algorithm identifiers.  That needs to be somewhere and this is parallel with how it was done in JWA [RFC 7518].

14. Section 3.2:  If this is a new signature algorithm then it should say what curves it operates with.  Else there should be an addition of the algorithms that are legal in section 3.1

It's not a new algorithm.  It's a new set of identifiers.  I can revise to make this clear.

15.  Section 4.1 - The correct term is 'provisional' not 'temporary'.

The term "TEMPORARY" is used in https://www.iana.org/assignments/cose/cose.xhtml#algorithms but I agree that "provisional" is the right term.  I'll revise accordingly.

16. Section 5.4 - So if somebody changes the crv value, is there any way to tell the difference?  If so then that should be described here.

OK

17.  Section 5.3 - This text seems to be self-contradictory.  If you MUST NOT use it, then how can you then use it?  How does an implementation know when it is or is not acceptable practice to use this?

It's saying that COSE implementations MUST NOT use it.  That doesn't mean that we can stop TPMs from using it, and it's in that context that this algorithm identifier is used.

18.  CHAIRS:  You should run the fact that this document is updating JOSE registries as well and make sure that there are going to be no surprises about this being not in charter.

Given that the charter listed draft-jones-webauthn-secp256k1-00 as a starting point and it was already performing JOSE algorithm registrations, I suspect that there's no surprises in us doing so.

Jim

				Thanks again,
				-- Mike