Telechat Review of draft-ietf-anima-grasp-11
review-ietf-anima-grasp-11-tsvart-telechat-stiemerling-2017-05-23-00

Request Review of draft-ietf-anima-grasp
Requested rev. no specific revision (document currently at 15)
Type Telechat Review
Team Transport Area Review Team (tsvart)
Deadline 2017-05-09
Requested 2017-04-23
Authors Carsten Bormann, Brian Carpenter, Bing Liu
Draft last updated 2017-05-23
Completed reviews Intdir Early review of -09 by Charles Perkins (diff)
Opsdir Last Call review of -12 by Joel Jaeggli (diff)
Secdir Last Call review of -09 by Barry Leiba (diff)
Genart Last Call review of -09 by Joel Halpern (diff)
Tsvart Telechat review of -11 by Martin Stiemerling (diff)
Secdir Telechat review of -11 by Barry Leiba (diff)
Genart Telechat review of -11 by Joel Halpern (diff)
Artart Telechat review of -11 by Martin Thomson (diff)
Genart Telechat review of -12 by Joel Halpern (diff)
Assignment Reviewer Martin Stiemerling
State Completed
Review review-ietf-anima-grasp-11-tsvart-telechat-stiemerling-2017-05-23
Reviewed rev. 11 (document currently at 15)
Review result Not Ready
Review completed: 2017-05-23

Review
review-ietf-anima-grasp-11-tsvart-telechat-stiemerling-2017-05-23

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

* 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?

* 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?

************************************************************
This document is always using the escape hatch when it comes to security 
in any respect!
************************************************************

* 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).

* 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). What happens if 
a TCP connection is shutdown by one end or is forcefully closed, e.g., 
by a reset?

* There is no versioning support in GRASP. Why are we still developing 
protocols that do not have versioning support in it?

* 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?“

* 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?

* 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.

* Section 2.3, T1 & T2 are good, but how is this fulfilled by the protocol?

* 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?

* 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?

* Section 3.3: This section is again bringing up a number of 
requirements. Wrong place in the document?

* 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).

* 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?

* 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.

* Section 3.5.1, page 17: „virtualized over the ACP“ — what does this 
mean and how it is done?

* 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?

* 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.

* 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.

* Section 3.5.1, page 17: "discovery messages MUST be secured, with one 
exception mentioned in“ — how secured? This is extremely underspecified.

* Section 3.5.2 s/GRAPS subject/GRAPS are subject/

* 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.

* 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?

* 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.

* 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?

* 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?

* 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?

* 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?

* 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.

* 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…

* 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?

* Section 3.5.6.2 (and other places in the document): What is the GRASP 
core?

* 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?

* 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?

Best regards,

   Martin Stiemerling