Martin, Thanks for the thorough review. First reactions:
On 23/05/2017 21:05, Martin Stiemerling wrote: > Hi all, > > Please find below my review of draft-ietf-anima-grasp-11. > The comments below are about -11 of the draft, as I did work on this > version and did unfortunately not have time to go through -12. > > > I've reviewed this document as part of the transport area review team's > ongoing effort to review key IETF documents. These comments were written > primarily for the transport area directors, but are copied to the > document's authors for their information and to allow them to address > any issues raised. When done at the time of IETF Last Call, the authors > should consider this review together with any other last-call comments > they receive. Please always CC tsv-art@… if you reply to or forward this > review. > > Summary: > This draft has serious issues, described in the review, and needs to be > rethought. > > > General comments: > * The title is partially misleading as this document is setting > requirements for the protocol and is additionally defining the protocol. > This is made obvious in the abstract, but nonetheless, the title is > misleading I really don't see that. It's very general title. > > * Document structure: I am not sure why the requirements are listed in > Section 2. The are never ever used in the document to motivate any > design decision or reference to. So what is the purpose of having these > requirements here? Couldn’t and shouldn’t they be listed in a separate > document? We had specific instructions from our original AD not to do separate requirements analysis. We've also asked the WG a couple of times whether to move the requirements to an Appendix and never got a Yes on that. > > * Security: There is no security proposed or discussed for GRASP in this > document. There are solely statements about generic security, such as in > Section 1 „a secure and strongly authenticated environment“ or Section > 3.5.2.1. "Messages MUST be authenticated and encryption MUST be > implemented“. So how must they be authenticated and encrypted and why? > What is the standard set of protocols to be used for this and what is > the minimal set of crypto algorithms to be implemented to make this work? We're on version 12 now but I'm at a loss to know what is unclear about the statement in https://tools.ietf.org/html/draft-ietf-anima-grasp-12#section-3.5.1 (and the ACP is now a normative reference). > ************************************************************ > This document is always using the escape hatch when it comes to security > in any respect! > ************************************************************ It isn't an escape hatch, it's an explicit statement: "Required External Security Mechanism" > * Usage of UDP: This document is not discussing any of the aspects in > RFC 8085. Every usage of UDP is required by IETF consensus to review RFC > 8085 and to address at least the applicable subset of issues listed in > RFC 8085 (or the predecessor RFC 5405). The somewhat weak language about unicast UDP has gone in version 12: https://tools.ietf.org/html/draft-ietf-anima-grasp-12#section-3.5.3 We now say "Use of an unreliable transport protocol is therefore NOT RECOMMENDED." As far as multicast goes, https://tools.ietf.org/html/rfc8085#section-4.1 is new work but at a quick glance I don't think there is any concern. We use link-local multicast with precautions against excessive rates or loops. RFC8085 seems to be worrying about completely different types of usage. Of course we can add a statement about that. > > * Starting with UDP and switching to TCP for the data transfer looks > like the right do. However, UDP should be really only used to discover > other devices, but not piggy back further protocol mechanics. However, > this document is not really specific on how to make use of TCP, for > instance, how long are TCP connections kept open or closed down after a > protocol exchange (persistent vs temporary connections). They can all be temporary. We can add that. > What happens if > a TCP connection is shutdown by one end or is forcefully closed, e.g., > by a reset? What's to say? The operation fails, and then it's up to the user (the autonomic service agent) what to do next. If that isn't quite obvious, we can certainly add it. > > * There is no versioning support in GRASP. Why are we still developing > protocols that do not have versioning support in it? Because the protocol is indefinitely extensible by adding message types and options. We really don't see any need for a version number. The M_INVALID message was added so that old and new code can discover what works and what doesn't. > > * IPv4/IPv6 simultaneous use: I believe the email describing the -12 > update of the draft says that IPv4 and IPv6 should not be mixed, so I > can skip over this. > > Detailed comments: > > * Section 2.1: These are not protocol requirements but system > requirements. It would be really good to separate them in the list of > requirements. > > * Section 2.1, Requirement D2: What is this requirements saying? Is it > „it should just work whatever?“ Yes. There should be no uncaught exceptions. That's a pretty general rule in autonomic systems, so it must also be true in the protocol itself. > > * Section 2.1, Requirement D7: What is the rest of the network in the > first bullet point? All anima devices or every single element of the > network? All autonomic devices; this could be clarified. > > * Section 2.2, Requirement SN7: I am not sure if any of these points in > this requirement related to any part of the GRAPS protocol. If so, you > can for sure add forward references or backward references in the > protocol specification. That's hard to do. It's really saying (computer science hat on) that GRASP needs to be powerful enough to express all kinds of semantics that you might need. > > * Section 2.3, T1 & T2 are good, but how is this fulfilled by the protocol? T1 is in practice fulfilled by the choice of CBOR and its ability to express almost any kind of data structure. T2, by the ability to add message types and options in future. (But getting back to our original instructions from Benoit, do we really need to map requirements:features ?) > * Section 3.2, end of page 12, „appropriate global-scope address“: so > GRASP is not intended to run networks that use private RFC 1918 addresses? Well, we tend to think in IPv6 terms, where anything that isn't link-local has global scope. And we *really* hope that GRASP itself will always run over IPv6 even if it's managing an IPv4 data plane. But you're right, it should be phrased to cover the RFC1918 case. > > * Section 3.2, head of page 13: the discussion about interfaces. I am > not sure that calling GRASP interfaces just interfaces helps. Why not > calling interfaces interfaces and GRASP interfaces if the are used? or > active interfaces, etc? That's been tweaked in the -12 draft already. > > * Section 3.3: This section is again bringing up a number of > requirements. Wrong place in the document? Sorry, I don't see any requirements there; it describes decisions IMHO. > > * Section 3.3, page 14: the unsolicited flooding mode sounds really bad. > Has this aspect looked at in terms of how many GRAPS nodes are expected > to live on in a single link-local multicast domain (hoping that non > link-local multicast is excluded per se). It is a trade-off, but we have use cases (such as distributing Intent to every autonomic node, a low frequency operation) where flooding really does look like a necessary tool. > * Section 3.3: last bullet on page 14 and first bullet on page 15: what > is this saying other than it is complicated? Aren’t there any tangible > details to this? I don't really understand your point there. > * Section 3.4: > " An instance of GRASP is expected to run as a separate core module, > providing an API (such as [I-D.liu-anima-grasp-api]) to interface to > various ASAs. These ASAs may operate without special privilege, > unless they need it for other reasons (such as configuring IP > addresses or manipulating routing tables).“ > This is implementation specific and does not belong in a protocol > specification. Disagree. I think an implementer needs to keep this in mind. > * Section 3.5.1, page 17: „virtualized over the ACP“ — what does this > mean and how it is done? See the ACP draft. > > * Section 3.5.1, page 17: „If there is no ACP, one“ there are two > options listed for this case, but which is used when? I think section 3.5.2 covers this clearly. > > * Section 3.5.1, page 17: „strong authentication“ — what is strong > authentication and what needs to be supported by a minimal, but > interoperable implementation? This is extremely underspecified. See the ACP draft or in 3.5.2. > * Section 3.5.1, page 17: "Network interfaces could be at different > security levels,“ what is a security level in the context of this > document and specifically here. OK will clarify > > * Section 3.5.1, page 17: "discovery messages MUST be secured, with one > exception mentioned in“ — how secured? This is extremely underspecified. See the ACP draft or in 3.5.2. > > * Section 3.5.2 s/GRAPS subject/GRAPS are subject/ OK > > * Section 3.5.2.1., page 17: "As mentioned in Section 3.3“ this isn’t > mentioned in this place or I cannot correlate it. Will check > > * Section 3.5.2.1., page 17 > " implemented. TLS [RFC5246] and DTLS [RFC6347] based on a Public Key > Infrastructure (PKI) [RFC5280] are RECOMMENDED for this purpose. > Further details are out of scope for this document.“ > How bad is this that TLS and DTLS are RECOMMENDED but the usage is not > specified? Because we decided it was out of scope for now. > > * Section 3.5.2.2: Are there any measures in the protocol to ensure that > datagrams have really been sent on the same link? I haven’t seen any > measures. As far as I can tell that's an implementation detail that depends on the socket API. > > * Section 3.5.2.3.: > " by TLS. A separate instance of GRASP is used, with its own copy of > all GRASP data structures. This instance is nicknamed SONN - Secure > Only Neighbor Negotiation.“ > I am not sure why this document is trying to mandate how implementers > are organizing their implementation and how instances are named? Because this is how we can specify a minimal amount of security during the bootstrap process that depends on GRASP. > * Section 3.5.2.3, page 19, end of bullet list: "Further details are out > of scope for this document.“ What further details? Is the WG not sure > what the missing details are? Again, implementation details that are very likely o/s dependent. > * Section 3.5.3.: > "They MUST NOT be fragmented, and therefore MUST > NOT exceed the link MTU size.“ > How should the protocol ensure that such datagrams are not fragmented > and do not exceed the MTU? By magic? I think this needs rephrasing. > > * Section 3.5.3.: " Use of an unreliable transport protocol is therefore > NOT RECOMMENDED.“ — very good guidance — thank you! :-) > > * Section 3.5.3., end of page 19: > > * Section 3.5.4.4: > "Since the relay device is unaware of the timeout set by the original > initiator it SHOULD set a timeout at least equal to GRASP_DEF_TIMEOUT > milliseconds.“ > > The timeout set by the initiator seems to be important, so why is this > value not communicated by the initiator within GRASP? As I replied to Mirja, we do need a little more work on this text. > > * Section 3.5.5: It is unclear to me if the messages here are expected > to be sent over TCP or nor? This is not clear. Will clarify > > * Section 3.5.5, page 24 and also Section 3.5.6.1: > " request is sent (see Section 3.8.6). If no reply message of any kind > is received within a reasonable timeout, the negotiation request MAY > be repeated, with a newly generated Session ID (Section 3.7). An > exponential backoff SHOULD be used for subsequent repetitions.“ > What is a reasonable time out? And why is this need if TCP is used? TCP > provides reliable data transfer… Right, but the other end might die. > > * Section 3.5.5, page 24: > " about different objectives. Thus, GRASP is expected to be used in a > multi-threaded mode. Certain negotiation objectives may have > restrictions on multi-threading, for example to avoid over-allocating > resources. > „ > Again, talking about the implementation. This is not a protocol > specification, isn’t it? Again, an area where implementers need to pay attention. > > * Section 3.5.6.2 (and other places in the document): What is the GRASP > core? Maybe need to mention that in the terminology section. > > * Section 3.5.6.2, page 26: > „ the result is zero. Also, it MUST limit the total rate at which it > relays Flood Synchronization messages to a reasonable value, in order > to mitigate possible denial of service attacks. It MUST cache the > „ > What is a reasonable value? I'm not sure we can put a number on it. It is surely technology-dependent. > > * Message encoding: What is the encoding to be supported for "reason = > text ;optional error message“ Is there are any minimal standard to be > supported, such as 7 bit ASCII or UTF-8 or whatever? It is intended to be UTF-8, if that isn't clear it needs to be written. Regards, Brian (in a rush, on vacation) > > Best regards, > > Martin Stiemerling > > > _______________________________________________ > Anima mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/anima > _______________________________________________ Anima mailing list [email protected] https://www.ietf.org/mailman/listinfo/anima
