Re: Tsvart last call review of draft-ietf-bfd-multipoint-16

[Greg Mirsky <[e-mail]>]

Hi Bob,
thank you for the thorough review, detailed questions and helpful comments.
Please find my answers in-line and tagged GIM>>.
I've updated the working version of the draft based on your comments and
suggestions. Appreciate your feedback whether all questions have been
addressed.
Attached please find the diff of -16 and the working version and the copy
of the working version of the draft.

Regards,
Greg

On Mon, May 21, 2018 at 5:20 PM, Bob Briscoe <[e-mail]> wrote:

> Reviewer: Bob Briscoe
> Review result: Not Ready
>
> 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.
>
GIM>> Thank you for highlighting this issue. I think that the second
paragraph of Introduction is the appropriate place to note that this
mechanism does not require existence of a return path from tails to the
head. Would the following be acceptable:
NEW TEXT:
   Use of BFD in
   Demand mode enables a tail monitor availability of a multipoint path
   even without the existence of some kind of a return path to the head.

>
> 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?
>
GIM>> This draft defines the base specification for multipoint BFD. In
order for multipoint BFD to support the transport-like connectivity
verification we need to do work similar to described in RFC 6428.

>
> 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?
>
GIM>> One example could be a multicast system with 1+1 protection. Without
multipoint BFD tails would not be able to detect the failure of the
muticast path from the head. Other examples discussed in several drafts:

   - BESS WG draft MVPN fast upstream failover
   <https://tools.ietf.org/html/draft-ietf-bess-mvpn-fast-failover-03>
   - Individual draft BFD for Multipoint Networks and VRRP Use Case
   <https://tools.ietf.org/html/draft-mirsky-bfd-p2mp-vrrp-use-case-01>
   - Individual draft BFD for Multipoint Networks and PIM-SM Use Case
   <https://tools.ietf.org/html/draft-mirsky-pim-bfd-p2mp-use-case-00>

I am not sure how references to non-WG drafts affect the progress of this
document. Appreciate your suggestion.


> 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.
>
GIM>> Very interesting, thank you. I wouldn't say that the case when a tail
cannot process incoming mpBFD control packets at the offered rate is
entirely non-issue. Such scenario must be handled by the implementation and
may be controlled by local policy, e.g., close the MultipointTail session.
Where would you suggest to add the text?

>
> 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]
>
GIM>> Thank you for the suggested text. Would the Security Considerations
section be appropriate place:
NEW TEXT:
   Use of shared keys to authenticate BFD Control packet in multipoint
   scenarios is limited because tail can spoof the head from the
   viewpoint of the other tails.  Thus, if shared keys are used, all
   tails MUST be trusted not to spoof the head.  Otherwise, asymmetric
   message authentication would be needed, e.g., Timed Efficient Stream
   Loss-Tolerant Authentication (TESLA) as described in [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.
>
GIM>> Thank you. Please check the updated text:
NEW TEXT:
   If authentication is in use, the head and all tails must be
   configured to have a common authentication key in order for the tails
   to validate received the multipoint BFD Control packets.

>
> 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.
>
GIM>> Because mpBFD control packets cannot be demultiplexed by  tail based
on the value of Your Discriminator field as per RFC 5880,
the new procedure outlined in Section 4.7:
   IP and MPLS multipoint tails MUST demultiplex BFD packets based on a
   combination of the source address, My Discriminator and the identity
   of the multipoint tree which the Multipoint BFD Control packet was
   received from.  Together they uniquely identify the head of the
   multipoint path.
and described in details in Section 4.13.2:
      If the Multipoint (M) bit is set

         If the Your Discriminator field is nonzero, the packet MUST be
         discarded.

         Select a session as based on source address, My Discriminator
         and the identity of the multipoint tree which the Multipoint
         BFD Control packet was received.  If a session is found, and
         bfd.SessionType is not MultipointTail, the packet MUST be
         discarded.  If a session is not found, a new session of type
         MultipointTail MAY be created, or the packet MAY be discarded.
         This choice is outside the scope of this specification.

Would you suggest additional text to a use case where the new
demultiplexing is not sufficent to protect from source address spoofing?

>
> 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.
>
GIM>> I've listed all eight occasions with the explanation for each one:

   1. Details of tail notification to the head are outside the scope of
   this document. - Notifications by tails addressed in
   draft-ietf-bfd-multipoint-active-tail
   <https://datatracker.ietf.org/doc/draft-ietf-bfd-multipoint-active-tail/>.
   Will add as informational reference.
   2. Details of how the head keeps track of tails and how tails alert
   their connectivity to the head are outside scope of this document. - Same
   as #1.
   3. Bootstrapping BFD session to multipoint MPLS LSP in case of
   penultimate hop popping is outside the scope of this document. - It may use
   control plane as in MVPN draft. Will add as informational reference.
   4. Use of other types of encapsulation of the BFD control message over
   multipoint LSP is outside the scope of this document. - This in reference
   to ACH encapsulation that is discussed in draft-mirsky-mpls-p2mp-bfd
   <https://tools.ietf.org/html/draft-mirsky-mpls-p2mp-bfd-03>. Should it
   be added as informational reference? What would be the imacpt of
   progressing this specification?
   5. Change in the value of bfd.RequiredMinRxInterval is outside the scope
   of this document. - Same as #1.
   6. If a session is not found, a new session of type MultipointTail MAY
   be created, or the packet MAY be discarded. This choice is outside the
   scope of this specification. - I propose to add "based on local policy" to
   the last sentence.
   7. The exact method of selection is application-specific and is thus
   outside the scope of this specification. - This is copied from RFC 5880:
   "The exact method of selection is application specific and is thus outside
   the scope of this specification." as the section is to replace Section
   6.8.6.
   8. If a matching session is not found, a new session of type
   PointToPoint MAY be created, or the packet MAY be discarded. This choice is
   outside the scope of this specification. - Same as #6.


> There is also one issue that is "for further discussion". Does this mean
> the
> document is not ready yet?
>
GIM>> I think that the question left for further discussion is
non-technical:
   The semantic difference between Down and AdminDown state is for
   further discussion.
I propose to remove the sentence altogether.


> 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?
>
GIM>> You're aboultely right, existing implementations of BFD don't need to
support bfd.SessionType variable. Only implementations that support the
base BFD, single-hop or multi-hop, and this specification, mpBFD, should
support bfd.SessionType and set it to PointToPoint value when BFD is in
single-hop or multi-hop mode. When in mpBFD mode, bfd.SessionType will be
set to either MultipointHead or MultipointClient.

>
> ==Nits==
>
> * Sometimes 'tree' is used to mean a multipoint path in general. I suspect
> 'path' was intended.
>
GIM>> I've found six occasions of "tree" and s/tree/path/

>
> 4.8.  Packet consumption on tails
> s/Node/Nodes/
> s/packet/packets/
> s/demultiplex/demultiplexed/
>
GIM>> Accepted all three.

>
> 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?
>
GIM>> You are right, the BFD system at the head would not know the previous
value of bfd.DesiredMinTxInterval. This text is to caution operator from
decreasing  bfd.DesiredMinTxInterval upon restart of the BFD system.

>
> 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.
>
GIM>> In the first paragraph SHOULD may not be followed if the
implementation can differentiate between the very first start and restarts
of BFD system. If it is the first start of BFD system, the head MAY
directly progress to Up state skipping Down state.
The last paragraph describes graceful shuttdown. The head MAY shut the BFD
mp session abruptly by just stopping transmission of BFD Control packets.

>
> 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.
>
GIM>> A Poll Sequence starts with the initiator setting Poll bit. Unless
the peer sends BFD Control packet with Finl bit set the poll sequence would
continue indefinetely. The initial SHOULD NOT, in my opinion, correctly
points that the mechanism of Poll Sequence not to be used by MultipointHead
when changing transmission interval. I think that MUST in the first
paragraph can be downgraded to MAY because the MultipointHead doesn't need
to use transition period when changing the transmission interval to lower
level, i.e., decreasing frequency. May I propose the following:
OLD TEXT:
   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.
NEW TEXT:
   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 MAY send packets with the P bit set during
transition period.

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

>
> 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.
>
GIM>> Hope I got it right:
      If bfd.SessionType is PointToPoint, update the Detection Time as
      described in section 6.8.4 of [RFC5880].

      Else

         If bfd.SessionType is MultipointTail, then update the Detection
         Time as the product of the last received values of Desired Min
         TX Interval and Detect Mult, as described in Section 5.11 of
         this specification.

>
> 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".
>
GIM>> Agree, the paragraph is not structured properly. How about this
formating:
      If the Multipoint (M) bit is set

         If the Your Discriminator field is nonzero, the packet MUST be
         discarded.

         Select a session as based on source address, My Discriminator
         and the identity of the multipoint path which the Multipoint
         BFD Control packet was received.

         If a session is found, and bfd.SessionType is not
         MultipointTail, the packet MUST be discarded.

         Else

            If a session is not found, a new session of type
            MultipointTail MAY be created, or the packet MAY be
            discarded.  This choice is outside the scope of this
            specification.