Subject: [Anima] TSV-ART review of draft-ietf-anima-grasp-11

From: Martin Stiemerling <[email-masked]>
Date: Tue, 23 May 2017 11:05:05 +0200

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

---

From: Brian E Carpenter <[email-masked]>
Date: Wed, 24 May 2017 08:53:40 +1200

Martin,

Thanks for the thorough review. First reactions:

> This is made obvious in the abstract, but nonetheless, the title is 

I really don't see that. It's very general title.

> requirements here? Couldn’t and shouldn’t they be listed in a separate 

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.

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

It isn't an escape hatch, it's an explicit statement:
"Required External Security Mechanism"

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

> protocol exchange (persistent vs temporary connections). 

They can all be temporary. We can add that.

> What happens if 

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.

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

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

> first bullet point? All anima devices or every single element of the 

All autonomic devices; this could be clarified.

> can for sure add forward references or backward references in the 

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

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

> calling interfaces interfaces and GRASP interfaces if the are used? or 

That's been tweaked in the -12 draft already.

> requirements. Wrong place in the document?

Sorry, I don't see any requirements there; it describes decisions IMHO.
> 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.
 
> is this saying other than it is complicated? Aren’t there any tangible 

I don't really understand your point there.
 
> This is implementation specific and does not belong in a protocol 

Disagree. I think an implementer needs to keep this in mind.

> * Section 3.5.1, page 17: „virtualized over the ACP“ — what does this 

See the ACP draft.

> options listed for this case, but which is used when?

I think section 3.5.2 covers this clearly.

> interoperable implementation? This is extremely underspecified.

See the ACP draft or in 3.5.2.
 
> document and specifically here.

OK will clarify
> 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

> mentioned in this place or I cannot correlate it.

Will check

> How bad is this that TLS and DTLS are RECOMMENDED but the usage is not 

Because we decided it was out of scope for now. 

> datagrams have really been sent on the same link? I haven’t seen any 

As far as I can tell that's an implementation detail that depends on
the socket API.
> 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.

> what the missing details are?

Again, implementation details that are very likely o/s dependent.

> and do not exceed the MTU?

By magic? I think this needs rephrasing.
> value not communicated by the initiator within GRASP?

As I replied to Mirja, we do need a little more work on this text.
> to be sent over TCP or nor? This is not clear.

Will clarify
> provides reliable data transfer…

Right, but the other end might die.

> Again, talking about the implementation. This is not a protocol 

Again, an area where implementers need to pay attention.

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

Maybe need to mention that in the terminology section.

> What is a reasonable value?

I'm not sure we can put a number on it. It is surely technology-dependent.
> 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-masked]
> https://www.ietf.org/mailman/listinfo/anima
>

---

From: Joe Touch <[email-masked]>
Date: Thu, 25 May 2017 11:34:20 -0700

Hi, Brian,

On 5/23/2017 1:53 PM, Brian E Carpenter wrote:
> M_INVALID message was added so 
Version support is recommended by IANA (see RFC7506, Sec 7.5).

Extensibility is a separate issue and doesn't replace the benefit of
version support.

The goal is to avoid needing to assign a new port number for GRASPv2 in
the future.

IMO, all messages (including NOOP) should start with a version number,
and I'd suggest at least 2 bits (if not 4).

Joe

---

From: Michael Richardson <[email-masked]>
Date: Thu, 25 May 2017 15:37:15 -0400

> and I'd suggest at least 2 bits (if not 4).

Everything is CBOR encoded, so we have no dependancy upon encoded bits on the
wire changing.
{Should CBOR change in an incompatible way, we'd need a new port number, true}

Otherwise, we could obsolete any of the M_ operations trivially.
We currently expect to encode the "first" layer as a CBOR array (which
creates the record boundary for TCP as well).

We could even decide that "version 2" would start with [M_GRASP2, [stuff]]
costing us perhaps two bytes, which is about the same as inserting a version
would cost, but I'm still not convinced we'd ever need to do that.

-- 
Michael Richardson <[email-masked]>, Sandelman Software Works
 -= IPv6 IoT consulting =-

---

From: Brian E Carpenter <[email-masked]>
Date: Fri, 26 May 2017 08:09:28 +1200

> would cost, but I'm still not convinced we'd ever need to do that.

You're right. If we had stuck to the original TLV model I would have agreed
that a version number is needed, but the nature of CBOR means that it
really isn't useful. Even the limit MESSAGE_TYPE = 0..255 in the syntax
isn't restrictive; a message type >255 or <0 would just kick back an M_INVALID
with old code, and would be happily accepted by new code, if we increased
the valid range.

    Brian

---

From: Joe Touch <[email-masked]>
Date: Thu, 25 May 2017 15:10:53 -0700

> with old code, and would be happily accepted by new code, if we increased
AOK - thanks for the explanation.
Joe