Subject: [homenet] Alia Atlas' Discuss on draft-ietf-homenet-dncp-09: (with DISCUSS and COMMENT)

From: "Alia Atlas" <[email-masked]>
Date: Wed, 16 Sep 2015 15:18:25 -0700

Alia Atlas has entered the following ballot position for
draft-ietf-homenet-dncp-09: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-homenet-dncp/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

First,  I have a number of specific comments.   Some of these are hazards
to technical interoperability; I've tried
to include those in my discuss - but the general point is that it is very
hard to tell a number of details because the
structure is assumed.   Having read this document, I do not think that I
could properly implement DNCP from scratch.

Obviously, I can guess at the answers - but that doesn't let everyone
interoperate well.   Examples include:

a) What is a topology graph?  When is it created, modified, or destroyed?
 Is it a conceptual artifact constructed
from the various TLVs?  I think a quick paragraph describing it would
help.

b) How are peer relationships discovered and established and destroyed? 
I really can't tell from the draft and even a quick scan of 
RFC 6206 doesn't give any hints.

c) It looks like the TLVs are sent one after another in a datagram or
stream across a socket.  The closest I see to any detail
is "TLVs are sent across the transport as is".  

d) As far as I can tell, Trickle is used to decide basically how often to
send information - but the details of this and the interactions
aren't clear to me.

I suspect that there are dependencies on the HNCP draft that would make
this a lot easier to understand but since we want it to progress
separately, the document does need to stand alone.

Although not noted in the Shepherd's report, I did have a thorough
Routing Directorate review done and the draft was improved from that.

8) In Sec 4.6 "   o  The origination time (in milliseconds) of R's node
data is greater
      than current time in - 2^32 + 2^15."  Since origination time hasn't
yet been introduced, I'm going
on an assumption that it means when R's node data was originated from R. 
So - this requirement is
saying that R's node data must have been generated in the future (but
already known by the local node)???

9) In Sec 4.6 "They MAY be removed immediately after the topology graph
traversal"  The DNCP nodes can
be removed from what??  The topology graph?  From some type of received
TLV??  How would they be
added back in later?

11) In sec 6.1: "Trickle-driven status updates (Section 4.3) provide a
mechanism for
   handling of new peer detection on an endpoint, as well as state
   change notifications."   Nothing in Sec 4.3 talked about a mechanism
for detecting
new peers on an endpoint.  It is entirely possible that Trickle does
provide this (but what
about the modes where Trickle isn't used/needed??) but there needs to be
a description of how
new peer detection is done - even if it's just a pointer to Trickle
RFCs.

12) In Sec 6.1.4: "   On receipt of a Network State TLV (Section 7.2.2)
which is consistent
   with the locally calculated network state hash, the Last contact
   timestamp for the peer MUST be updated."   Could you add some
rationale for why this
is needed?  I suspect that part of my confusion is that the "locally
calculated network state
hash" could mean two different things.  Is it the hash computed by the
local node on the
data received in the Network State TLV to validate that the Network State
TLV is not
corrupted?  Or is it the hash computed by the local node on its
arbitrarily wide 1-hash tree
that represents the local node's network state?   Since the term is never
defined, it's hard
to guess here.  The bottom of Sec 7.2.2 uses "current locally calculated
network state hash"
to refer to, I believe, the latter.

13) In Sec 6.2: "normally form peer relationships with all peers."  Where
is forming a peer
relationship defined?  Is this tied solely to Trickle instances?  What
about with reliable unicast 
where presumably Trickle isn't used between peers as the Overview states
"If used in pure unicast mode with unreliable transport, Trickle is also
used between peers"?

14) In Sec 7: "   For example, type=123 (0x7b) TLV with value 'x' (120 =
0x78) is
   encoded as: 007B 0001 7800 0000.  If it were to have sub-TLV of
   type=124 (0x7c) with value 'y', it would be encoded as 007B 0009 7800
   0000 007C 0001 7900 0000."   In this case, the padding between the
TLV's value
and the sub-TLV is included but the padding after the sub-TLV is not. 
What would
happen if there were multiple sub-TLVs??  Would the padding between those
sub-TLVs
be included?

Also related :"In this case the length field includes the length of the
original TLV, the 
length of the padding that are inserted before the embedded TLVs and the
length of the 
added TLVs."  Here, the phrase "length of the TLV" means different
things.  In the first case,
"length of the original TLV" means the "length of the value in the
encapsulating TLV".  In
the second case, "length of the added TLVs" appears to mean the length of
the sub-TLVs 
including the type/length header.  As I mentioned above, I can't tell
what happens to the 
padding in between sub-TLVs.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

1) In Sec 4.1, I think it would be clearer if you moved the sentence
"These leaves are ordered in ascending
   order of the respective node identifiers. " to after the first
sentence with appropriate text changes.  I was left
confused by why the leaf would be represented by a node's sequence
number.  I think it's that a leaf represents
 a node and is ordered based upon that node's identifier.   The value of
a leaf is a tuple of the node's sequence
number and the hash value...  

2) In Sec 4.2, it says "As multicast is used only to identify potential
new DNCP
   nodes and to send status messages which merely notify that a unicast
   exchange should be triggered, the multicast transport does not have
   to be secured."   There aren't attacks from processing fake potential
new DNCP node
  or triggering lots of unneeded/unterminated unicast exchanges?

3) In Sec 4.3, it says "Imin, Imax and k.   Imin and Imax represent the
minimum and maximum values for I"
I believe this isn't quite accurate.  Imax is a max multiplier of Imin
for I and not a maximum value.
I've seen this error in another draft also; I think it's important to be
correct & clear here.

4) There are multiple references to different unintroduced TLVs.  There
is also the idea that
each DNCP protocol can have its own TLVs.  It'd be very useful in the
Introduction to simply state
what TLVs are required by DNCP and conceptually what they are for.

5) In Sec 4.3, it says "The Trickle state for all Trickle instances is
considered
   inconsistent and reset if and only if the locally calculated network
   state hash changes."  but I have no idea yet what a Trickle instance
is, when
   a Trickle instance is created or associated with a node?  an
interface? or something else?

6) I think it would be more useful to describe generally at least what is
in a DNCP profile earlier in the
document.  This may be bringing Section 9 forward or it may be listing it
earlier.  I'm seeing numerous
forward references.

7) Start of Sec 4.6 talks about the topology graph - but there's been
absolutely no introduction of
what the topology graph is or how it was created, updated, etc.

10) In Sec 5:  This is a helpful section.  It tells me that a Trickle
instance is per interface.  I don't see anything
like a topology graph in it.  It clarifies origination time slightly.  It
talks about "For each remote (peer, endpoint) 
pair detected on a local endpoint" but I still have no ideas how that
detection is done.  It implies some policy
restrictions "Set of addresses: the DNCP nodes from which connections are
accepted." but has no details on whether
that is created via multicast messaging or via local configuration or
something else.

15) Also, in Sec. 4.6, the terminology for the fields of the Peer TLV is
different than defined
in Sec 7.3.1 - just "Endpoint Identifier" instead of "Local Endpoint
Identifier".

16) In Sec 7.3.2: "   Endpoint identifier is used to identify the
particular endpoint for
   which the interval applies.  If 0, it applies for ALL endpoints for
   which no specific TLV exists."   Is this the Local Endpoint Identifier
or the remote?
   The same question applies for Sec 7.2.1.

---

From: Steven Barth <[email-masked]>
Date: Thu, 17 Sep 2015 17:10:49 +0200

Hello Alia,

please find replies inline.

Cheers,

Steven & Markus

Please note that an independent DNCP (or more specifically an HNCP)
implementation has been successfully developed based on
an earlier version of this draft and has been shown to be
interoperable with the reference implementation of the draft authors.
We used the implementer’s feedback afterwards to further refine the draft
to avoid possible ambiguities.

> from the various TLVs?  I think a quick paragraph describing it would

The term “topology graph” is defined in the Terminology Section and based
on bidirectional peer reachability which is defined right afterwards. In the
latter definition it is mentioned that the process is solely based on published
TLVs so the topology graph is to my understanding already unambiguously
defined. It is solely up to the implementer if this is implemented as an actual
graph or not, as long as the outcome is consistent with what is described.

If you still think it is ambiguous please provide some ideas on how this
could be worded better.

> RFC 6206 doesn't give any hints.

This is explained in section “4.5 Adding and Removing Peers”. As for
methods for discovery, this is actually mentioned multiple times across
the document, e.g. in the second paragraph of the Overview or in the
terminology.

Could you please be more specific on what exactly is unclear here in your
opinion?

> is "TLVs are sent across the transport as is".

Section “4.2 Data Transport” says
   TLVs are sent across the transport as is, and they SHOULD be sent
   together where, e.g., MTU considerations do not recommend sending
   them in multiple batches.  TLVs in general are handled individually
   and statelessly, with one exception …

Could you please be more specific on what is unclear?

The section states that TLV handling is stateless except for the one exception
which is explained, so otherwise  it does not really matter in what order
they are sent or how you split them up onto datagrams (if that is your
transport).

> send information - but the details of this and the interactions

This is explained in detail in section “4.3.  Trickle-Driven Status Updates”.

   The Trickle state for all Trickle instances is considered
   inconsistent and reset if and only if the locally calculated network
   state hash changes.  This occurs either due to a change in the local
   node's own node data, or due to receipt of more recent data from
   another node.  A node MUST NOT reset its Trickle state merely based
   on receiving a Network State TLV (Section 7.2.2) with a network state
   hash which is different from its locally calculated one.

   Every time a particular Trickle instance indicates that an update
   should be sent, the node MUST send a Network State TLV…

Could you please be more specific on what is missing here?

> separately, the document does need to stand alone.

No, there are no dependencies. This was noted already in response to
reviews from Thomas Claussen and Les Ginsberg.

Please refer to section “9.  DNCP Profile-Specific Definitions“ for the
comprehensive list of things we have intentionally left out in DNCP.

> already known by the local node)???

The term “origination time” is actually explained in Section “5. Data Model”,
-10 will have a forward-reference here. About the time itself,
the text says greater than “current time - 2^32+2^15”; that threshold is always
in the past.

> be removed from what??  The topology graph?  From some type of received

Maybe “forgotten (about)” may be a more intuitive term here. Each node stores
each other node’s data and metadata so “removing” the node essentially means
removing that information from the local node’s memory. If it is not removed
immediately the node can still keep it in its memory in case the remote node
becomes bidirectionally reachable again to avoid synchronizing the node data
(if it did not change in the meantime).

Clarified in -10.

> new peer detection is done - even if it's just a pointer to Trickle

As noted in the reply for c) addition is based on TLV reception as noted in 4.3.
Actual discovery is mentioned multiple times in this draft, e.g. 2nd paragraph
of Overview. Summarizing: it boils down to either one node receives that TLV
over multicast or one of the node knows the other’s unicast address based
on configuration or some other protocol then the other node discovers the
former when that TLV is unicasted to it.

Could you please note how this could be improved?

> is needed?

This is to indicate the node is still alive (6.1. is the section on keep-alives).
The Last Contact timestamp is defined in 6.1.1 and 6.1.5 explains what
happens when it reaches a threshold.

> I suspect that part of my confusion is that the "locally
> calculated network state
> corrupted?

Since there is no mention of any hashing to validate generic TLVs anywhere
in the RFC or any such hash values for comparison at all I cannot really follow
how one could come to that conclusion.

> Or is it the hash computed by the local node on its

The term “network state hash” is actually defined in the terminology section and hash tree section 4.1;
if the words “locally calculated” make it really ambiguous please let me know how to improve it.

> "If used in pure unicast mode with unreliable transport, Trickle is also

Please see replies to c) and 11).

> happen if there were multiple sub-TLVs??  Would the padding between those

Yes, that is correct, fixed in -10. The individual sub-TLVs themselves contain
length fields which will not include even their own padding, but the container
TLV’s length ends at the end of the last sub-TLV’s last non-padding byte
(and then padding is inserted if any).

> padding in between sub-TLVs.

Clarified in -10.

Clarified as
“These leaves are ordered in ascending
   order of the node identifiers of the nodes they represent.”
in -10.

>   or triggering lots of unneeded/unterminated unicast exchanges?

This is already covered in the Security Considerations section.

> I've seen this error in another draft also; I think it's important to be

Addressed for -10. 6206 defines it as “The maximum interval size, Imax“,
which probably led to this. (I wonder why it is not maximum interval exponent, or something..)

> what TLVs are required by DNCP and conceptually what they are for.

Section 7 is entirely devoted to that purpose. I do not think that it should
be repeated in the general introduction.

If there are particular forward references that are missing, please let us know.

> interface? or something else?

Please see Section “5. Data Model”

   “For each remote (peer, endpoint) pair detected on a local endpoint, a
   DNCP node has:”
   [...]
   * Trickle instance: the particular peer's Trickle instance with
      parameters I, T, and c (only on an endpoint in Unicast mode, when
      using an unreliable unicast transport) .

we will add an xref there and add “trickle instance” to the terminology.

> earlier.  I'm seeing numerous

There would be numerous forward references in that case too, as it refers
to the protocol behavior and individual TLVs. (There are currently 2 forward
references in the terminology and 2 in the text; we probably cannot move it
before the terminology, and right after the terminology it would introduce
more forward references than it would eliminate.)

> what the topology graph is or how it was created, updated, etc.

See reply to a).

> that is created via multicast messaging or via local configuration or

See previous answers referring to peer detection.

> in Sec 7.3.1 - just "Endpoint Identifier" instead of "Local Endpoint

Addressed in -10.

>    The same question applies for Sec 7.2.1.

Both refer to the endpoint identifier of the (local) sending node. Given the
TLV is published network-wide within the particular node’s node state,
identifying that of remote nodes would be nontrivial and probably not unique
either.