Last Call Review of draft-ietf-bfd-multipoint-16
review-ietf-bfd-multipoint-16-tsvart-lc-briscoe-2018-05-21-00

Request Review of draft-ietf-bfd-multipoint
Requested rev. no specific revision (document currently at 19)
Type Last Call Review
Team Transport Area Review Team (tsvart)
Deadline 2018-05-21
Requested 2018-05-07
Draft last updated 2018-05-21
Completed reviews Rtgdir Last Call review of -16 by Michael Richardson (diff)
Tsvart Last Call review of -16 by Bob Briscoe (diff)
Genart Last Call review of -16 by Francis Dupont (diff)
Genart Telechat review of -18 by Francis Dupont (diff)
Tsvart Telechat review of -18 by Bob Briscoe (diff)
Assignment Reviewer Bob Briscoe
State Completed
Review review-ietf-bfd-multipoint-16-tsvart-lc-briscoe-2018-05-21
Reviewed rev. 16 (document currently at 19)
Review result Not Ready
Review completed: 2018-05-21

Review
review-ietf-bfd-multipoint-16-tsvart-lc-briscoe-2018-05-21

Altho this is a TSV-ART review, I did not find many transport-related issues to focus on, except a need to justify why lack of information for adapting the transmit interval is not an issue. 

Nonetheless, I did find a few other non-trivial technical issues, including 2 security issues, enumerated below (I mis-spent some of my early research career working on a multicast session control and security, for which we used beaconing control channels). However, I only have passing prior knowledge of BFD, so my critique might be off-beam.


==Main Technical Concerns===

1/ Mandatory return path?
RFC5880 is the base RFC that this draft updates. RFC5880 says that "unidirectional links" are in scope, but only as long as there is a return path.

The introduction of this bfd-multipoint draft seems to contradict that, making a return path optional:
"
   As an option, the tail may notify the head of the lack of multipoint
   connectivity.  Details of tail notification to the head are outside
   the scope of this document.
" 
It's allowable for irrelevant details to be outside the scope, but surely it needs to be clear whether at least the existence of a return path is mandatory.

2/ Mechanism for verifying connectivity, or not?
The introduction seems to contradict itself:
"
   As multipoint transmissions are inherently unidirectional, this
   mechanism purports only to verify this unidirectional connectivity.
"
"
   Term "connectivity" in this document is not being used in the context
   of connectivity verification in transport network but as an
   alternative to "continuity", i.e. existence of a forwarding path
   between the sender and the receiver.
"
How can this mechanism verify connectivity, but not be used in the context of connectivity verification in the transport network?

3/ Use case
The introduction seems to be written rather academically. Surely, in cases where there is never a return path, only the tails will ever be able to verify connectivity. The head could continue transmitting BFD packets (and data packets) for years without ever knowing whether it is connected to anything. Knowledge of connectivity is surely of little use if it excludes the link sender, which is the node that always controls routing.

If there are scenarios where it is useful for tails but not the head to be able to verify connectivity, can you please give a concrete example?

4/ Interval adaptation
Text is needed to describe why it is not an issue for the head to be unaware whether it needs to adapt its transmit interval. Otherwise, this seems potentially problematic.

5/ Inability to authenticate the sender with symmetric keys
In unicast scenarios, symmetric keys can be used for message authentication, because each end knows there is only one other node with the shared key. But, in multipoint scenarios, all the tails would share the key, so a shared key does not authenticate who sent the message - any tail can spoof the head from the viewpoint of the other tails.

Therefore text is needed to say that:
* multipoint message authentication is limited to cases where all tails are trusted not to spoof the head, if shared keys are used.
* otherwise asymmetric message authentication would be needed, e.g. TESLA [RFC4082]

A related nit: Section 5 says all tails are assumed to have a common authentication key. In cases with symmetric message authentication, surely the head also needs the same key.

6/ Source address spoofing
A 3-way handshake makes a protocol robust against simple source address spoofing. Without a 3WHS, surely the spec. needs to highlight this vulnerability or discuss ways to address it or why it is not an issue.

7/ Scope
On eight occasions an issue is raised, but resolution is stated as outside the scope of this document. It is OK to limit the scope of a spec, for example to allow for multiple solutions to each issue. But at least one solution must already exist for each of these eight issues. So, at least one example solution ought to be cited in each case. If any issues are open, then this should not be on the standards track.

It would be more useful to state why each issue is out of scope. This would be helped by stating from the start what the scope of the document is. 

There is also one issue that is "for further discussion". Does this mean the document is not ready yet?

8/ Incremental deployment

Section 4.4.1.  "New State Variable Values" defines bfd.SessionType = PointToPoint as well as a couple of Multipoint alternatives. Presumably this spec does not require all existing PointToPoint systems to support this state value. Is the implication that only Multipoint systems that happen to be in PointToPoint mode should use this state? 




==Nits==

* Sometimes 'tree' is used to mean a multipoint path in general. I suspect 'path' was intended.

4.8.  Packet consumption on tails
s/Node/Nodes/
s/packet/packets/
s/demultiplex/demultiplexed/

4.9.  Bringing Up and Shutting Down Multipoint BFD Service
"
   a newly
   started head (that does not have any previous state information
   available) SHOULD start with...
"
...
"
   ... (so long as the restarted head
   is using the same or a larger value of bfd.DesiredMinTxInterval than
   it did previously).
"
If it has no state available, how can it know whether a value is larger than previously?

4.9.  Bringing Up and Shutting Down Multipoint BFD Service
There are a number of "SHOULD"s and "SHOULD NOT"s that do not state or give examples of circumstances in which the "SHOULD" would not be appropriate. If there are none, "MUST" would be more appropriate.

4.10.  Timer Manipulation
"
   Because of the one-to-many mapping, a session of type MultipointHead
   SHOULD NOT initiate a Poll Sequence in conjunction with timer value
   changes.  However, to indicate a change in the packets,
   MultipointHead session MUST send packets with the P bit set.
   MultipointTail session MUST NOT reply if the packet has M and P bits
   set and bfd.RequiredMinRxInterval set to 0.
"
The initial "SHOULD NOT" needs to be written another way. Perhaps 
"
   ...a session of type MultipointHead
   does not initiate a Poll Sequence
"
The head's normative action is defined by the following "MUST", then the tail's "MUST NOT reply" is what prevents the poll sequence happening.

4.11.  Detection Times
Delete "in the calculation" (repetition).

4.13.1.  Reception of BFD Control Packets
Some actions seem to be only relevant to PointToPoint sessions, but they are stated for all session types. Specifically:
"the transmission of Echo packets, if any, MUST cease."
"the Poll Sequence MUST be terminated."
"MUST cease the periodic transmission of BFD Control packets"
"MUST send periodic BFD Control packets"

"
If bfd.SessionType is PointToPoint, update the Detection Time as
      described in section 6.8.4 of [RFC5880].  If bfd.SessionType is
      MultipointTail, 
"
The second sentence above ought to start on a new line as an Else if.


4.13.2.  Demultiplexing BFD Control Packets
"
   This section is part of the replacement for [RFC5880] section 6.8.6,
   separated for clarity.
"
Do you mean "This section replaces the sentence: "If the Multipoint (M) bit is nonzero, the packet MUST be discarded." in [RFC5880] section 6.8.6?

The statements under "If the Multipoint (M) bit is set" are not formatted like the rest of the if-else logic, and I think an Else is missing at the start of the statement after the nested "If".