Subject: [babel] Shepherd Review of draft-ietf-babel-source-specific-04

From: Donald Eastlake <[email-masked]>
Date: Tue, 9 Apr 2019 23:11:46 -0400

Hi,

Possible technical issues:
===================
Section 6, page 7, first parenthetical:
   I think instead of "and all of its extensions" it should say something a
bit more limited. How about "and all previously standardized extensions"?

Section 7, page 8, 2nd paragraph:
   Generally, cases where different routers can interpret routing protocol
messages differently make me very nervous. It is almost always possible to
construct a situation where this causes persistent looping. It seems to me
that you need to unambiguously specify what is done if there is a malformed
TLV with multiple Source Prefix sub-TLVs. Such TLVs could always be ignored
or the first or the last Source Prefix sut-TLV could be the effective
sub-TLV with other Source Prefix sub-TLVs ignored. But whatever is
specified, it needs to be mandatory unambiguous behavior.

Section 7.1, Page 9:
   What do you do when you get a malformed Source Prefix sub-TLV with a
zero Source Plen? I guess this is answered below where it says the TLV is
ignored if the sub-TLV is malformed. (I guess it could also be malformed by
Source Plen being too large.) Perhaps it should say "This MUST NOT be 0. If
it is 0, the enclosing TLV is ignored."
   For completeness, whether the Source Prefix is left-justified or
right-justified should be specified.

Section 7.3, Page 9:
   Continuing the theme of some of the comments above, what do you do with
a wildcard request that does contain a Source Prefix sub-TLV?

Typos:
=====
Section 5.2, page 7, top line:
   "that is has" -> "that it has"

Section 7.2, Page 9, third line:
   "than" -> "as"

Minor textural comments:
===================
   These are just minor cases where I think the draft would read slightly
better with a wording change.

Abstract:
OLD
source address. This document describes an extension for source-
specific routing to the Babel routing protocol.
NEW
source address. This document describes an extension to the Babel
routing protocol for source-specific routing .

Section 1, page 3, 1st paragraph, next to last line:
   "in case of equality" -> "in case of equally specific destination
addresses"

Section 5.1, page 6, last line:
    "sub-TLV" -> "source specific sub-TLV"

Section 6.2, pag 8, first line:
   ", discarding source specific" -> ", the discarding of source-specific"

Thanks,
Donald
===============================
 Donald E. Eastlake 3rd   +1-508-333-2270 (cell)
 1424 Pro Shop Court, Davenport, FL 33896 USA
 [email-masked]

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 10 Apr 2019 12:42:11 +0200

> It seems to me that you need to unambiguously specify what is done if
> there is a [...] TLV with multiple Source Prefix sub-TLVs. Such TLVs
> could always be ignored or the first or the last Source Prefix sut-TLV
> could be the effective sub-TLV with other Source Prefix sub-TLVs
> ignored. But whatever is specified, it needs to be mandatory unambiguous
> behavior.

[...]

> What do you do when you get a malformed Source Prefix sub-TLV with a zero
> Source Plen?

[...]

> a wildcard request that does contain a Source Prefix sub-TLV?

Toke, I could use some advice here.  Please have a look at your code and
let me know whether requiring (MUST) dropping the enclosing TLV in all of
these cases complicates your implementation or not.  (I'll do the same on
my side.)

Thanks,

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 10 Apr 2019 13:08:23 +0200

> these cases complicates your implementation or not.  (I'll do the same on

Dropping the enclosing TLV is no problem, skipping the source TLV would
need patches... For reference, Bird currently does the following[0]:

- Malformed sub-TLV or Source prefix plen == 0: Parse error, drop the whole packet.

- Source sub-TLV on wildcard update or route request: Ignore enclosing TLV.

- Source sub-TLV on any other TLV: Parse error, drop the whole packet.

- Source-specific routing not configured: Ignore enclosing TLV.

- Duplicate source prefix sub-TLV: Ignore enclosing TLV.

-Toke

[0]: https://gitlab.labs.nic.cz/labs/bird/blob/master/proto/babel/packets.c#L998

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 10 Apr 2019 13:34:29 +0200

> need patches... For reference, Bird currently does the following[0]:

I see.  So your current behaviour can be described by

  MUST drop the enclosing TLV and MAY drop the whole packet

May the MUST be a SHOULD, or must it be a MUST?

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 10 Apr 2019 13:41:50 +0200

> May the MUST be a SHOULD, or must it be a MUST?

If it's a SHOULD, what is one supposed to do instead if deciding not to
drop?

-Toke

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 10 Apr 2019 17:54:22 +0200

> If it's a SHOULD, what is one supposed to do instead if deciding not to

It's not required to detect the situation -- we're in nasal daemons land.

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 10 Apr 2019 23:13:54 +0200

> It's not required to detect the situation -- we're in nasal daemons

That sorta feels under-specified to me?

-Toke

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 10 Apr 2019 23:57:13 +0200

> That sorta feels under-specified to me?

Babel is a distributed algorithm, and its correctness relies on all nodes
in a routing domain behaving correctly.  There are many ways in which
a buggy or malicious node can disrupt a Babel network, and an
implementation is not required to detect most of them.  (The most common
in production is a node that announces a route but doesn't actually
forward packets, e.g. due to a mis-configured firewall.)

What Donald is requesting here is that we add a requirement that every
implementation should detect one particular (and rather unlikely) instance
of incorrect behaviour, and work around it.  I don't necessarily agree
with this requirement, but I am willing to put it in if the implementation
cost is low enough.

Assuming we put that requirement in, should we mandate (MUST) that all
implementations detect that incorrect situation and work around it, or
should we merely recommend (SHOULD) that they do?  Please keep in mind
that the situation identified by Donald is rather unlikely to occur in
practice, while the implementation cost will be carried by all implementations.

-- Juliusz

---

From: Donald Eastlake <[email-masked]>
Date: Wed, 10 Apr 2019 22:14:23 -0400

Hi,

On Wed, Apr 10, 2019 at 5:57 PM Juliusz Chroboczek <[email-masked]> wrote:

> ...

> forward packets, e.g. due to a mis-configured firewall.)

While it is true that the protocol data stores and state machines and other
aspects of behavior at the ends of the wire are important, the IETF has
always had a particularly sharp focus on the bits on the wire. It seems
reasonable to have a specification that won't fall over even if it gets a
"routing" packet that's junk. And it is a traditional desiderata in routing
protocols in the IETF that in a network with mixed implementations, you
can't originate a bad routing packet from a node and have a persistent loop
or conceivably even scattered persistent loops form in your network.

> with this requirement, but I am willing to put it in if the implementation

I don't think I'm necessarily requesting that, at least not in all cases.
For example, if you are parsing a TLV it would be reasonable to set a flag
when you see a valid Source Prefix sub-TLV and just remember the prefix
specified, overwriting any previous source prefix seen in that TLV. Such a
parser can distinguish between no Source Prefix sub-TLVs and one or more
such sub-TLVs and, if there are any, will remember the prefix in the last
one. But it does not "detect" if there are more than one.

In any case, as long as the same packet will be interpreted in the same way
by different routers, I'm fine with whatever reasonable unambiguous
interpretation method is easiest to implement.

Thanks,
Donald

...

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 11 Apr 2019 17:01:46 +0200

[...]

> And it is a traditional desiderata in routing protocols in the
> IETF that in a network with mixed implementations, you can't originate a bad
> routing packet from a node and have a persistent loop or conceivably even
> scattered persistent loops form in your network.

I don't think that ideal is achievable.  For example, in OSPF or IS-IS, if
a router corrupts its LSDB, or fails to synchronise it in a timely manner,
then you will get persistent loops.  A similar thing will happen if a BGP
speaker erroneously or maliciously discards part of the AS-path (the
opposite of prepending, in a sense).

I agree with you, though, that even though we cannot handle all cases, we
should identify the cases that are easy to handle and either require or
recommend that a robust implementation avoid burning down the universe
when it receives an erroneous packet that is easy to detect.  My question
to Toke was whether this is one of the easy cases from the point of view
of his implementation.

> For example, if you are parsing a TLV it would be reasonable to set
> a flag when you see a valid Source Prefix sub-TLV and just remember the
> prefix specified, overwriting any previous source prefix seen in that
> TLV.

I agree, this appears to be a case that's easy to handle.

So -- MUST or SHOULD?  (I prefer SHOULD, both Toke and Donald appear to
prefer MUST, so I'll give myself some more time to think it over.)

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 11 Apr 2019 17:11:58 +0200

> prefer MUST, so I'll give myself some more time to think it over.)

Now that I've implemented it, I think it's simple enough to be a MUST.
Sorry for boring everyone.

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 11 Apr 2019 17:30:09 +0200

>    I think instead of "and all of its extensions" it should say something a bit
> more limited. How about "and all previously standardized extensions"?

Done.

> construct a situation where this causes persistent looping. It seems to me that
> you need to unambiguously specify what is done if there is a malformed TLV with
> multiple Source Prefix sub-TLVs. Such TLVs could always be ignored or the first
> or the last Source Prefix sut-TLV could be the effective sub-TLV with other
> Source Prefix sub-TLVs ignored. But whatever is specified, it needs to be
> mandatory unambiguous behavior.

Done.

>    What do you do when you get a malformed Source Prefix sub-TLV with a zero

Done.  This leads to some repetition (the case of wildcard requests and
retractions was already specified in Section 5.2), but what the hell.

> Typos:

Done.

> Minor textural comments:

Done.

Thanks for your help,

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Fri, 12 Apr 2019 01:13:26 +0200

> Sorry for boring everyone.

Great :)

-Toke