lake-wg / edhoc

Ephemeral Diffie-Hellman Over COSE (EDHOC)
Other
7 stars 12 forks source link

Sean Turner's review of -12 #217

Closed gselander closed 2 years ago

gselander commented 2 years ago

https://mailarchive.ietf.org/arch/msg/lake/01_qDnQqJlsem3KT58F6XNVbSQ4/


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?

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]

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

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?

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

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.

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 …

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

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.

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.

s3.5.2 (question): related to comment on s3.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].

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

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.

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.

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.

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

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?

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?

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

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

}

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.

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? How does the peer know it was discontinued if an error is not sent?

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.

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?

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.

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

s6.3.2/s5.2.1 (nit): Might be worth noting earlier that SUITES_R is not important. Also, s5.2.1, mentioned SUITES_I, but s5.2.2 does not mention SUITES_R.

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

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.

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

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

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

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

emanjon commented 2 years ago

Waiting for Sean to confirm that he is happy with the changes. Will be closed unless Sean has any further comments. If Sean does not comment more we will assume he is happy and close the issue.

emanjon commented 2 years ago

Sean acks