Re: [Emu] AD Review: draft-ietf-emu-noob-03

[Roman Danyliw <[e-mail]>]

Hi Tuomas!

Thanks for the inline explanation below and the new text in -04.  This new content addresses my feedback so I advanced the document to IETF LC.

Regards,
Roman

> -----Original Message-----
> From: Aura Tuomas <[e-mail]>
> Sent: Tuesday, March 16, 2021 9:17 AM
> To: Roman Danyliw <[e-mail]>; [e-mail]
> Subject: RE: AD Review: draft-ietf-emu-noob-03
> 
> Hi Roman,
> 
> Thank you for your review. We have made the necessary changes and published
> version -04. I have also explained the changes made in-line below. Hopefully,
> the draft is now ready for the next steps.
> 
> Regards,
> Tuomas
> 
> 
> -------- Forwarded Message --------
> Subject: 	[Emu] AD Review: draft-ietf-emu-noob-03
> Date: 	Sun, 7 Mar 2021 13:33:59 +0000
> From: 	Roman Danyliw <[e-mail]>
> 
> To: 	[e-mail] <[e-mail]>
> 
> 
> 
> Hi!
> 
> I performed an AD review on draft-ietf-emu-noob-03. Thanks for the work on
> this document -- in particular for providing copious examples in the Appendix;
> and co-developing this text with the implementations and the proofs.
> 
> ** Section 3.2.3. Per "The OOB receiver MUST compare the received value of
> the fingerprint Hoob ...", perhaps overly pedantic, it would be worth
> mentioning that this is compared relative to the expected PeerId + Hoob.
> 
> Tuomas: Added  "The OOB receiver MUST compare the received value of the
> fingerprint Hoob (see Section 3.3.2) with a value that it computes locally for
> the PeerID received."
> 
> 
> ** Section 3.4.2 and Section 6.6. I wanted to talk through the expected
> implementation logic around the downgrade protection in the check during the
> cryptosuite upgrade. Specifically:
> 
> (a) Section 3.4.2. The server SHOULD NOT offer and the peer MUST NOT accept
> protocol versions or cryptosuites that it knows to be weaker than the one
> currently in the Cryptosuitep field of the persistent EAP-NOOB association.
> 
> (b) Section 6.6. As long as
> the server or peer saves any information about the other endpoint, it MUST
> also remember the previously negotiated cryptosuite and MUST NOT accept
> renegotiation of any cryptosuite that is known to be weaker than the previous
> one, such as a deprecated cryptosuite.
> 
> To make sure I understand that right, let's say I registered cryptosuite = 3 as
> "ECDHE curve Curve25519 + SHA-512". If the peer initially used this new
> cryptosuite=3 and later tried to negotiate the current cryptosuite=1, this should
> fail because SHA-256 is weaker than SHA-512? What about the situation of
> hypothetical cryptosuite = 4 as "fancy new PQ-resistant algo + SHA-256"?
> No issues with the suggested design, but perhaps we should further caveat
> somewhere in the document by adding language that determining the relative
> strength of the cryptosuites is out of scope and may be managed through local
> policy or configuration.
> 
> Tuomas: Yes, thank you for raising this. We have added: "Determining the
> relative strength of the cryptosuites is out of scope and may be managed
> through local policy or configuration at the peer and server."
> 
> 
> ** Section 4. Per "The EAP Method Type number for EAP-NOOB needs to be
> assigned", can the explicit registry name for this be explicitly named.
> 
> Tuomas: We now call out the registry name explicitly. We realized that all the
> new registries created should also have explicit names. We have made the
> necessary changes.
> 
> 
> ** Section 4.1. Per "public-key format [RFC7517] Section 6.2.1" in both
> cryptosuites, RFC5717 doesn't have a Section 6.2.1.
> 
> Tuomas: Good catch. This is a remanent from when the text was pointing to
> section 6.2.1 of RFC 7518. Fixed.
> 
> 
> ** Section 5.4. Editorial. Please add the model URLs as a reference instead of a
> bare URL
> 
> Tuomas: The URLs are now informational references.
> 
> 
> ** Section 5.4, 6.1, 6.2, 6.6, Appendix E. In the spirit of inclusive language,
> please consider: s/man-in-the-middle/on-path/
> 
> ** Section 6.5. In the spirit of inclusive language, please consider: s/blacklist
> misbehaving peer devices/add misbehaving peer devices to a deny list/
> 
> Tuomas: We have now made the appropriate terminology changes.
> 
> 
> ** Appendix C. Per "Table 11 lists some suggested data fields for ServerInfo.
> Further specifications may specify application- specific ServerInfo and PeerInfo
> contents.":
> 
> -- I would recommend tuning the guidance to make it clear that if these fields
> names are used in any OOB-enabled application their semantics will be as
> defined here (I stumbled over calling these "suggested data fields").
> 
> NEW:
> Table 11 defines commonly used data fields for ServerInfo. Further
> specifications may define additional application-specific ...
> 
> -- Is there an EAP reference to describe handle unknown fields?
> 
> Tuomas: Error types 5002 and 5004 handle the cases where ServerInfo and
> PeerInfo have unknown fields.
> 
> 
> -- Did the WG discuss/consider defining an IANA registry to manage the
> Peer/ServerInfo fields to ensure there are clear pointers to their semantics?
> 
> Tuomas: This is was something briefly eluded to by the IoT directorate review
> of Dave Thaler. Based on Dave's recommendation, we had added a type tag.
> We consulted RFC 8216 again and like your recommendation of making the
> PeerInfo and ServerInfo semantics clearer with an IANA registry. We were
> initially hesitant but 'specification required' is flexible enough to not prevent
> new applications of EAP-NOOB from defining new data fields. PeerInfo and
> ServerInfo are now IANA registries.
> 
> 
> ** Appendix F and Section 3.3.2. The existing examples in this Appendix are
> very helpful. One additional place where I was looking for an illustrative
> example was the JSON input that would get hashed into the Hoob, MACs. Just
> one of them would have been useful.
> 
> Tuomas: The example inputs of Hoob, MACs, MACp, etc. have now been added.
> 
> 
> Regards,
> Roman