Re: Benjamin Kaduk's No Objection on draft-ietf-bfd-multipoint-active-tail-09: (with COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Tue, Jul 03, 2018 at 10:58:31AM -0700, Greg Mirsky wrote:
> Hi Benjamin,
> thank you for the review and your kind words. Please find my answers
> in-line and tagged GIM>>.
> The new working version and the diff are attached.
> 
> Regards,
> Greg
> 
> On Mon, Jul 2, 2018 at 1:34 PM, Benjamin Kaduk <[e-mail]> wrote:
> 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-bfd-multipoint-active-tail-09: No Objection
> >
> > 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-bfd-multipoint-active-tail/
> >
> >
> >
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > Thanks for the clear explanations throughout; they made this document
> > pretty easy to read.
> >
> > When the head sends both a multipoint poll and a unicast poll, does the
> > MultipointClient session have a way to tell whether a received reply is
> > replying to the multipoint message or the unicast one?
> >
> GIM>> There's no information that a tail includes in the Final poll message
> to distinguish between the Poll from the head. I believe that the head
> paces the messages apart to guarantee that the response to the earlier have
> arrived or must be considered as lost.

It would probably be good to double-check that that requirement on the head
is written down somewhere.

> >
> > For variables that "MUST be initialized based on configuration", do we need
> > to provide a default value?
> >
> GIM>> Interesting question, thank you. I think that the default for:
> 
>    - bfd.SilentTail may be 1 so that enabling responses to polling requires
>    explicit configuration action;
>    - bfd.ReportTailDown may be 1, as that is the goal of using active tail
>    mode.
> 
> Others certainly will have different preferences. It may be that leaving
> the defaults for these two local variables to implementation is acceptable.
> What do you think?

In light of, e.g., Mirja's DISCUSS on the other document, we should
probably default to SilentTail of 1.  I suppose we could leave
ReportTailDown to implementations, though it's maybe a little odd to only
specify one default but not the other.  A default of 1 for ReportTailDown
seems fine.

> >
> > Section 4
> >
> >    A head may wish to be alerted to the tails' connectivity (or lack
> >    thereof), there are a number of options.
> >
> > This is a comma splice and should be reworded.
> >
> GIM>>
> OLD TEXT:
>    A head may wish to be alerted to the tails' connectivity (or lack
>    thereof), there are a number of options.
> NEW TEXT:
>    A head may wish to be alerted to the tails' connectivity (or lack
>    thereof), and there are a number of options to achieve that.

Sure.

> >
> > Section 5.1
> >
> >    [...]  This mode emulates the behavior
> >    described in [I-D.ietf-bfd-multipoint].  In this mode for
> >    bfd.SessionType is MultipointTail, variable bfd.SilentTail (see
> >    Section 6.3.1) MUST be set to 1.
> >
> > nit: I think the "for" is spurious (and could maybe even be replaced by a
> > comma)?  The existing comma could be replaced by a semicolon or "and" to
> > avoid a comma splice.
> >
> GIM>> Modified to:
>  In this mode, bfd.SessionType is MultipointTail and the variable
> bfd.SilentTail (see Section 6.3.1) MUST be set to 1.

Looks good.

> >
> > Section 5.2
> >
> >    o  if bfd.SessionType is MultipointTail, variable bfd.SilentTail (see
> >       Section 6.3.1) MUST be set to 0;
> >
> > nit: "the variable"
> >
> GIM>> Thanks, done.
> 
> >
> > Section 6.3.1
> >
> >    Few state variables are added in support of Multipoint BFD active
> >    tail.
> >
> > nit: "A few"
> >
> GIM>> Done.
> 
> >
> >       bfd.UnicastRcvd
> >
> >          Set to 1 if a tail receives a unicast BFD Control packet from
> >          the head.  This variable MUST be set to zero if the session
> >          transitions from Up state to some other state.
> >
> > Is there ambiguity about "the session" being MultipointHead/MultipointTail
> > vs. MultipointClient/MultipointTail (i.e., multipoint or unicast)?
> >
> GIM>> I had to refresh my memory of how the value of bfd.UnicastRcvd
> controlled. My understanding of text in 6.13.1 is that more accurate
> definition of bfd.UnicastRcvd may be with the following text:
>       bfd.UnicastRcvd
> 
>          Set to 1 if a tail has received a unicast BFD Control packet
>          from the head while being in Up state.  This variable MUST be
>          set to zero if the session transitions from Up state to some
>          other state.

Just to check my understanding, this is the unicast session's transition
that we're talking about?  (This does match my understanding of how it
works, not that we should be trusting my understanding terribly far.)

> >
> > Section 6.4
> >
> >    If the head wishes to request a notification from the tails when they
> >    lose connectivity, it sets bfd.ReportTailDown to 1 in either the
> >    MultipointHead session (if such notification is desired from all
> >    tails) or in the MultipointClient session (if notification is desired
> >    from a particular tail).  Note that the setting of this variable in a
> >    MultipointClient session for a particular tail overrides the setting
> >    in the MultipointHead session.
> >
> > It seems like this property (MultipointClient overrides MultipointHead) is
> > fairly general and would apply to other variables as well.  Should it be
> > stated in a more general location?
> >
> GIM>> I agree and section 6.1 Multipoint Client Session seems to me as the
> appropriate place for such statement. A new sentense to append the
> paragraph:
>    Note that the settings of all BFD
>    variables in a MultipointClient session for a particular tail
>    override the corresponding settings in the MultipointHead session.

Looks good.

> >
> > Section 6.7
> >
> >    When the tails send BFD Control packets to the head from the
> >    MultipointTail session, the contents of Your Discr (the discriminator
> >    received from the head) will not be sufficient for the head to
> >    demultiplex the packet, since the same value will be received from
> >    all tails on the multicast tree.  In this case, the head MUST
> >    demultiplex packets based on the source address and the value of Your
> >    Discr, which together uniquely identify the tail and the multipoint
> >    path.
> >
> > I just want to double-check my understanding: is My Discr used at all for
> > BFD Control messages from the tail to the head?
> >
> GIM>> The spec does not specify how the head uses My Discriminator from a
> tail.

Thanks

> >
> > Section 6.8
> >
> >    The value of Required Min Rx Interval received by a tail in a unicast
> >    BFD Control packet, if any, always takes precedence over the value
> >    received in Multipoint BFD Control packets.
> >
> > Do we need to scope this down to the "associated" sessions?  (If so,
> > probably someone should review the whole draft with an eye for it, as I
> > have not done so.)
> >
> GIM>> A tail receives all BFD poll messages, multicast or unicast, on the
> same MultipointTail session. The head, on the other hand, sends multicast
> poll messages from MultipointHead session and unicast polls - from
> associated MultipointClient session(s).

Okay.  I may have been confused about the precise scope of a "session" on
the tail.  (That said, this sentence does now seem redundant with the
general note we added in Section 6.1.)

> >
> > Section 6.9, 6.10
> >
> > In "If the head wishes to [...] session MAY send [...]", the 2119 MAY does
> > not seem especially appropriate?
> >
> GIM>> s/MAY/can/ on both occasions?

Sure

> >
> > Section 6.13.1
> >
> >    [...] In
> >    addition to that, if tail tracking is desired by the head, below
> >    procedure MUST be applied.
> >
> > nit: "the following procedure"
> >
> GIM>> Thanks, done.
> 
> >
> > Section 6.13.2
> >
> > Do we need to say if this "addition" happens at the beginning or end of the
> > bfd-multipoint procedure, or is it supposed to replace part of it, or what?
> >
> GIM>> My understanding of this is that a tail must delay sending any packet
> to the head during mpBFD session coming up (yes, it is vague). As this is
> relevant only for active tail, it is addition rather than replacement of
> any procedure in mpBFD spec.

My understanding is that if I run the bfd-multipoint procedure first,
there's a "discard the packet" action that can trigger, where if I ran this
document's procedure first, I would take some action and not discard the
packet.  But I don't have a concrete scenario handy as an example.

> >
> > Section 6.13.3
> >
> >    If bfd.SessionType value is MultipointTail and periodic the
> >    transmission of BFD Control packets is just starting (due to Demand
> >    mode not being active on the remote system), the first packet to be
> >    transmitted MUST be delayed by a random amount of time between zero
> >    and (0.9 * bfd.RemoteMinRxInterval).
> >
> > Should "periodic the" be "the periodic"?
> >
> GIM>> Right, done.
> 
> >
> > Also, this seems like a situation where cryptographic randomness is not
> > required; it may be appropriate to note this.
> >
> GIM>> Sorry, missed the point. Could you please clarify RE: " cryptographic
> randomness".

We frequently distinguish between "cryptographic-strength random numbers"
(e.g., /dev/random) that are used for encryption keys and the like, and
"non-cryptographic strength random numbers" (e.g., linear feedback shift
registers, rand(3)) whose output is predictable given some sample of the
output.  In the latter, the randomness serves only to spread a distribution
around but does not provide any security properties.  In practice, though,
the need for cryptographically secure randomness is frequently
underestimated (and security vulnerabilities have resulted!), so we
generally recommend that people use the "strong" random numbers just in
case.  Here, though, we just need to desynchronize several independent
actors and there is probably not a "strong" requirement.

> >
> >    [...] A system MAY limit the rate at
> >    which such packets are transmitted.  If rate limiting is in effect,
> >    the advertised value of Desired Min TX Interval MUST be greater than
> >    or equal to the interval between transmitted packets imposed by the
> >    rate limiting function.
> >
> > How does this MUST get enforced?  Is it a matter of a software
> > implementation refusing to allow local configuration to effect such
> > behavior, or is it a global property of the system (e.g., that would
> > require the administrator to enforce the MUST)?
> >
> GIM>> I see it as the property of implementation.

Okay, I just wanted to check that we did not need more text (if it was a
burden on the administrator, we'd want to call that out).

> >
> >    Contents of transmitted packet MUST be as explained in section 5.13.3
> >    of [I-D.ietf-bfd-multipoint].
> >
> > nit: There's a singular/plural mismatch here between "Contents" (plural)
> > and "transmitted packet" (singular).
> >
> GIM>> Since the mpBFD describes only transmission of periodic BFD Control
> packet, the following seems logical:
>     Content of the transmitted packet MUST be as explained in section
>    5.13.3 of [I-D.ietf-bfd-multipoint].

Okay.  (Probably "The content", I suppose.)

> >
> > Section 9
> >
> > "infinite" is, well, infinite.  Implementations that create
> > MultipointClient sessions on demand need to have more reasonable
> > expectations on prevention (the listed points do a better job than this
> > text would indicate).
> >
> GIM>> Would this re-wording stand:
>    Additionally, implementations that create MultpointClient sessions
>    dynamically upon receipt of BFD Control packet from a tail MUST
>    implement protective measures to prevent a number of MultipointClient
>    sessions being created growing out of control.

It would, thanks.

> >
> > As the rtgdir early review, the risk of spoofed packets causing
> > tail-->MultpointClient traffic (including creation of MultipointClient
> > sessions based on the received traffic) should probably be called out more
> > directly.
> 
> GIM>> tail->MultipointClient, i.e.,head traffic is unidiractional. The
> head, I expect sane implementation, will drop all packets that cannot be
> matched to outstanding poll. I don't see it as new atack vector, but, of
> course, I may be missing something.

That traffic is unidrectional, yes, but if it is prompted by a spoofed
multipoint message, wouldn't the unidirectional traffic be sent from all
tails that receive the spoofed message?
Injecting a spoofed multipoint message is of course not a new attack, but
getting traffic reflected back to the head as a result does seem new.

> 
> > It seems like an attacker that can insert spoofed multipoint
> > traffic would be able to effect some level of traffic amplification over
> > time, though the jitter makes it harder to create large spikes.
> 
> GIM>> If understand the scenario you've described, the attacker transmits
> multicast poll messages but tails send unicast packets to the real head. I
> think that will not cause explosion of MultipointClient sessions because
> the head would not have outstanding poll. I expect that all packets will be
> discarded.

We do say that "If no session is found, a new session of type
MultipointClient MAY be created, or the packet MAY be discarded.  The
choice is outside the scope of this specification."  But I think what I had
in mind when I wrote this is basically the same thing as in my previous
paragraph, just getting the 1:1 control traffic sent to the head, and the
amplification factor is just the multipoint spread factor.

-Benjamin

> > (Even
> > on-path attacks are still worth documenting, IMO.)  I see there was some
> > conversation about the other security related points raised by that review,
> > but on a quick read it seemed like maybe there was still some room to add
> > more text to the security considerations.
> >
> >
> >