Subject: Re: [Lake] spt review of echoc-12

From: Göran Selander <[email-masked]>
Date: Tue, 11 Jan 2022 08:55:57 +0000

Hi Sean,

Thanks for the review, sorry for the delayed response!

Your comments are recorded as issue #217 and the proposed update are in PR #225.

Comments below.

From: Lake <[email-masked]> on behalf of Sean Turner <[email-masked]>
Date: Wednesday, 15 December 2021 at 07:35
To: LAKE List <[email-masked]>
Subject: [Lake] spt review of echoc-12
Like others I agreed to review the edhoc I-D at the last meeting. I did not have a chance to make sure that I am not repeating comments from others. To save the WG some time, I tried to categorize these along the lines of questions/nits where discuss might need some f2f time and nits, I believe, can be dealt with by the authors as they see fit.

Disclaimers: 0) I am not a cryptographer or security researcher. 1) It is entirely possible I am entirely missing the plot/point.

Apologies: I ran out of time to submit these as Issues/PRs. I can do so after the interim, including those that got rejected at the 12/15 interim for posterity sake.

s1.2 (nit): If s1.2 is supposed to be an applicability statement in the context of RFC 2026, can we call it that. i.e., r/Use of EDHOC/Applicability Statement.

revised: And then, I see section 3.9. Maybe instead of renaming s1.2, a forward pointer to s3.9 is all that is needed to say that an applicability statement is required?

[GS: Section 1.2 is now merged with 1.1 following Stephen's review, so "Use of EDHOC" is removed. The term Applicability Statement is not used in the context of RFC 2026, so I changed name to Applicability Template.
]

s2, 2nd para (nit): refer to both because it’s (D)TLS and not just TLS?  I.e.,
s/(D)TLS 1.3 [RFC8446]/(D)TLS 1.3 [RFC8446] [I-D.ietf-tls-dtls13]

[GS: Done]

s2, 2nd para (nit): I think you can safely drop the 2nd reference to IKEv2
s/like IKEv2 [RFC7296],/like IKEv2,

[GS: Done]

s3.2/s3.5.2 (question): Smarter people than I have devised plans to allow mixing of different METHODs. How does this impact the key usage extension present in certificates? In other words, are METHODS 1 and 2 suggesting that a DS and DH key be used together and does that violate any kind of restrictions on the key usage settings for certificates?

[GS: Note that the METHODs are not mixed, and that a given METHOD has fixed authentication method for each endpoint (i.e. Initiator authenticate in one way, R in another). Since there is a fixed authentication method in each direction I don't think this causes any problem: Key usage remains well-defined for each peer. Or perhaps I misunderstood the comment?
]

s3.4 (nit): Is transport of error messages, as noted by s6 (2nd para), needed in the list.

[GS: Rephrased sentence before list.]

[GS: firsta a preliminary note on 3.5; Stephen's review included a proposal to shorten this section and issue #223 also comments on this section, so there will be subsequent changes besides what is proposed here]

s3.5 para after the bullets (nit): I think you are trying to say the credentials have to be the same type, i.e., you can’t have PGP one one side and x.509 on the other. I am not sure that the following is the best wording:

s/Identical authentication credentials need to be established in both endpoints to be able to verify integrity./The same type of authentication credentials are needed baby the endpoints to be able to verify integrity.

[GS: It is actually necessary to establish identical authentication credentials, since the credential is included in the integrity calculation. I added reference to Figure 2 where this is shown.
]

s3.5.1, 1st para (nit): At first I thought for sure this had to be a MUST, but it says or application so I am not sure:

The EDHOC implementation or the application must enforce information about the intended endpoint …

[GS: Yes, as the application is mentioned here it cannot be MUST. We will reconsider this formulation when updating section 3.5.
]

s3.5.1 (nit): expand on first use NAI & EUI

[GS: Done
]

s3.5.1, 1st bullet (nit): Often we refer to this a Proof-of-Possession so maybe you could say that:

s/EDHOC provides
      proof that the other party possesses the private authentication
      key corresponding to the public authentication key in its
      certificate./EDHOC provides
      proof that the other party possesses the private authentication
      key corresponding to the public authentication key in its
      certificate, which is also known as proof-of-possesion.

[GS: Done (but I skipped "which is", hope that is still good English).
]

s3.5.1, 1st bullet (question): This seems like the “mixing” discussed in s3.2, relies on the identity provide in the certificate?

    The certification path provides proof that the
    subject of the certificate owns the public key in the certificate.

[GS: I didn't understand this comment. As mentioned in the response to the comment on s3.2, mixing is not with respect to a single endpoint. With that in mind, does the comment still apply?
]

s3.5.2 (question): related to comment on s3.2.

[GS: Same answer/counter-question as 3.2]

s3.5.3, 2nd para (nit): Could point to RFC 3279/5480 for some key usages for ECDSA and RFC 8410 for x25519/x448.

In
   X.509 and C509 certificates, signature keys typically have key usage
   "digitalSignature" and Diffie-Hellman public keys typically have key
   usage "keyAgreement" [RFC3279][RFC8410].

[GS: Done]

s3.6 (nit): refer to both?:
s/TLS 1.3 [RFC8446]/
(D)TLS 1.3 [RFC8446] [I-D.ietf-tls-dtls13]

[GS: Done]

s3.5.3 (question): EDHOC seems to assume you only provide the reference for the initiator's/responder’s. Was there any thought it to providing references for the rest of the certification path up to but not including the Root CA’s certificate? I mean technically they can also be in there certificate provided so not necessarily needed.

[GS: I'm not sure I understand the comment. I assume the question is about initiator's/responder’s *end-entity certificate* (since you speak of "the rest of the certification path" in the next sentence). Note that references can be to the entire certificate chain, see x5u in:

https://cose-wg.github.io/X509/draft-ietf-cose-x509.html#name-x509-cose-header-parameters

(Incidently, this third option is missing from the latest submitted version of that draft!)
]

s3.6, CNSA ref (nit): I made a similar comment against the ZRTP spec when they said their alg suites were compliant with Suite B … probably best to say Suite 24 and 25 use algorithms in CNSA and leave the word “compatible” for “them” to say.

[GS: Done]

s3.8 (comment): Wow .. so you can send in the CSR in EAD, if it works then you can use the cert in the later messages (e.g., kind of like EAP)? Sure hope there’s no kind of CSR-related error. And, you know that the CA gets to decide the name right. Sorry just trying to digest it all.

[GS: We discussed this in the LAKE interim where you participated, but just for the record: Yes, the intent is to be able to send CSR in EAD. This is expected to be detailed in an accompanying specification. One example would be the enrolment of an "operational" certificate as an add-on to initial authentication, the authentication of which could be performed using a manufacturer certificate included in the device from factory.
]

s4.3 (nit): s/kan/can

[GS: Done in another review update]

s4.4/s5.1 (question): Do you need to provide advice on when to delete the old PRK_4x3m? I.e., does the peer that sent this need to wait for some kind of confirmation before deleting it?

[GS: This is a good note to make. I added a sentence and pointed to KUDOS (draft-ietf-core-oscore-key-update) which is using the KeyUpdate method in a lockstep manner.
]

s5.1 (question): Is a state diagram needed? One thing people clamored for from TLS was a state machine. Maybe a diagram isn’t needed because there are so few states?

[GS: We raised this in the interim meeting but didn't get any comment, but there was also little time to do so.  To not forget this question, I opened issue #224.
]

s5.1 (comment, no action required): Cute. Deduplication is how to deal with message reply ;)

[GS: :-)
]

s5.2.1, s.5.3.1, s5.4.1, s5.5.1 (nit): is the “,” before the closing bracket right? e.g.,

OLD:

    ? EAD_1 : ead,
}

NEW:

    ? EAD_1 : ead
}

[GS: This is accepted by CDDL syntax so we kept it for simplicity. Previously I used to trim off these last commas, but when we changed order of parameters or added parameters we always forgot some comma. Less so if all lines end with one. I propose to keep this unless someone has a strong opinion not to.
]

s5.2.2 (question): If this this initiator processing section, how does the initiator know to truncate?

  …, truncated after the cipher suite selected for this
  session.

[GS: This part is rewritten following Stephen's review

OLD
The Initiator SHALL compose message_1 as follows:
•  SUITES_I contains a list of supported cipher suites, in order of preference, truncated after the cipher suite selected for this session.
o  ...

NEW
The Initiator SHALL compose message_1 as follows:
•  Construct SUITES_I complying with the definition in Section 5.2.1<https://lake-wg.github.io/edhoc/updates-following-review-by-Stephen/draft-ietf-lake-edhoc.html#asym-msg1-form>, and furthermore:
  o  ...

Have a look and see if it makes better sense now. Clarifications are made both in the message processing the cipher suite negotiation:
https://lake-wg.github.io/edhoc/updates-following-review-by-Stephen/draft-ietf-lake-edhoc.html#name-edhoc-message-1
https://lake-wg.github.io/edhoc/updates-following-review-by-Stephen/draft-ietf-lake-edhoc.html#name-wrong-selected-cipher-suite
]

s5.2.3, s5.3.3, s5.4.3, s5.5.3, last sentence (question - probably being pedantic): If there is an error but an error message is not sent, is the session discontinued?

[GS: As discussed in referenced section 8.7, there may be reason to ignore errors and continue processing.
]

How does the peer know it was discontinued if an error is not sent?

[GS: Good question. Even if errors are sent they may be lost, so the peer needs to be prepared for the protocol being discontinued in different ways. The comment was added to the related issue #208.
]

s5.2.3, s5.3.3, s5.4.3, s5.5.3 (nit, which I am sure the RFC editor fix better than I could, but I couldn’t help myself): Instead of the e.g., in middle of the MAY sentence maybe:

OLD:

   Sending error
   messages is essential for debugging but MAY e.g., be skipped if a
   session cannot be found or due to denial-of-service reasons, see
   Section 8.6.

NEW:

   Sending error
   messages is essential for debugging but MAY be skipped if, for
   example, a
   session cannot be found or due to denial-of-service reasons, see
   Section 8.6.

[GS: Done.
]

s6.1 (nit): To stop loops, should any peer that receives an ERR_CODE=0 discontinue the session? I.e., is that worth stating in the I-D?

[GS: I added that;

If an endpoint receives an error message with error code 0, then it MUST discontinue the protocol and MUST NOT send an error message.
]

s6.2 (nit): It’s really more “Freeform” than unspecified right? I mean the string is required so it’s definitely not unspecified per se.

[GS: Hmm. The error format is specified but the actual error is not specified. I changed to "Unspecified Error", is that better?
]

s6.3.2, 2nd to last para (nit): s/shall/SHALL

[GS: Fixed in Stefan's review  (which is now merged with master) and now stated 2nd to last para in 6.3.1 instead:
https://lake-wg.github.io/edhoc/draft-ietf-lake-edhoc.html#name-wrong-selected-cipher-suite
]

s6.3.2/s5.2.1 (nit): Might be worth noting earlier that SUITES_R is not important.

[GS: I guess you mean that *the order of suites* in SUITES_R is not important, right? :-) This is noted at the end of section 6.3.0 (i.e. before 6.3.1) which is the paragraph where SUITES_R is introduced, so couldn't really be noted earlier unless we also move the description of SUITES_R. But I made that statement into a standalone paragraph to highlight that it applies to all cases when SUITES_R is used: "In contrast to SUITES_I, the order of the cipher suites in SUITES_R has no significance."
]

Also, s5.2.1, mentioned SUITES_I, but s5.2.2 does not mention SUITES_R.

[GS: It is now mentioned in s5.2.2, but explanation is deferred to s6.3, which is referenced.
]

s7, last para (nit): s/To enable as much interoperability as we can
   reasonably achieve,/To enable as much interoperability as possible,

[GS: Done.
]

s8 (question): Isn’t some kind of consideration needed to address the use of PSKs shared by more than two? See RFC 8773 for some considerations wrt to group key sharing and that sharing impact on authentication.

[GS: I didn't understand this comment. PSK is explicitly out of scope of EDHOC (only mentioned when that is stated). Were you thinking of group communication protected with OSCORE? In that case, EDHOC is not used but there is the body of work in ACE including draft-ietf-ace-key-groupcomm-oscore.
]

s8.3 (nit): You can informatively refer to RFC 6194 to slam the door on SHA-1.

[GS: Done.
]

s9 and onwaard (apologies): I ran out of steam.

[GS: No worries :-)
]

s? (question): Is a peer supposed to check status information for its peer’s certificate/key?

[GS: This is part of a larger set of questions, see issues #212 and #223. I added your question to #223.
https://github.com/lake-wg/edhoc/issues/223
]

I-D Nit complaints (not all of them because the DOWNREFs (mostly) looked intentional):

== Missing Reference: 'RFC9052' is mentioned on line 2411, but not defined

[GS: That turned out to not be the right reference so is therefore removed.

Thanks,
Göran

---

From: Sean Turner <[email-masked]>
Date: Mon, 31 Jan 2022 23:02:49 -0500

Hi I pruned out the “done” ones and removed them from this response. I wanted to close the loop on some of the others, which frankly mostly appeared to be my misunderstanding.

> On Jan 11, 2022, at 03:55, Göran Selander <[email-masked]> wrote:
> 

Nope that explains it.

> [GS: firsta a preliminary note on 3.5; Stephen's review included a proposal to shorten this section and issue #223 also comments on this section, so there will be subsequent changes besides what is…

Okay I can re-review when it’s shortened up.

> [GS: I didn't understand this comment. As mentioned in the response to the comment on s3.2, mixing is not with respect to a single endpoint. With that in mind, does the comment still apply?

I think you can safely ignore this one ;)

> [GS: I'm not sure I understand the comment. I assume the question is aboutinitiator's/responder’s *end-entity certificate* (since you speak of "the rest of the certification path" in the next sentence). Note that references can be to the entire certificate chain, see x5u in:

I was thinking about how CMS has a bag of certs and how S/MIME says send up to but not including the root. The x5u, as you noted, serves the same purpose. You can safely ignore this comment.

> [GS: We discussed this in the LAKE interim where you participated, but just for the record: Yes, the intent is to be able to send CSR in EAD. This is expected to be detailed in an accompanying spec…

Yeah I remember I was there. As long as your consider what to do when you get an enrollment error this will be fine to do.

> [GS: This is accepted by CDDL syntax so we kept it for simplicity. Previously I used to trim off these last commas, but when we changed order of parameters or added parameters we always forgot some…

I’d leave it if it works. No strong objection on my part.
> [GS: Hmm. The error format is specified but the actual error is not specified. I changed to "Unspecified Error", is that better?

Yes I think that’s a better way to describe it.
> [GS: I guess you mean that *the order of suites* in SUITES_R is not important, right? :-) This is noted at the end of section 6.3.0 (i.e. before 6.3.1) which is the paragraph where SUITES_R is intr…

And there I missed it ;)

> [GS: I didn't understand this comment. PSK is explicitly out of scope of EDHOC (only mentioned when that is stated). Were you thinking of group communication protected with OSCORE? In that case, ED…

You’re confused. I think it was me. You can safely ignore this one.

Cheers,
spt

---

From: Göran Selander <[email-masked]>
Date: Tue, 1 Feb 2022 07:25:09 +0000

Hi Sean,

Thanks for the follow-up!  Some responses below:

From: Sean Turner <[email-masked]>

> > [GS: firsta a preliminary note on 3.5; Stephen's review included a proposal to shorten this section and issue #223 also > comments on this section, so there will be subsequent changes besides what is proposed here]
>
[GS:] Rewrite in progress, first draft in #240. We'll let you know when it is ready for review.

> > https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-65cece53a7226647&q=1&e=285c7537-d5dd-4a27-84fd-3e81cb5316e0&u=https%3A%2F%2Fcose-wg.github.io%2FX509%2Fdraft-ietf-cose-x509.html%23name-x509-cose-header-parameters
> >
> I was thinking about how CMS has a bag of certs and how S/MIME says send up to but not including the root.
> The x5u, as you noted, serves the same purpose. You can safely ignore this comment.
[GS:] OK. Just a side note that as we rewrite 3.5 in #240 I try to tighten the terminology so "authentication credential" refers specifically to CRED_x whereas "credential" refers to any certificate/chain/bag etc. Hopefully it will be clearer what is referenced.

> Yeah I remember I was there. As long as your consider what to do when you get an enrollment error this will be fine to do.

[GS:] Good point, this applies in general to errors in EAD processing, I added #242.

> Yes I think that’s a better way to describe it.
[GS:] OK, good. Note that this has taken another turn in #208 where we discuss splitting Unspecified Error into two categories.

Thanks,
Göran