Subject: [babel] [PATCH 2/4] address IETF-97 slides section I point 2

From: Denis Ovsienko <[email-masked]>
Date: Sun, 11 Dec 2016 23:34:17 +0000

"The specification should explicitly state that the “green” TLVs must be
ignored unless they come from a valid neighbour..."

This change at the end of "Bidirectional Reachability Detection" adds
respective normative text.
---
 draft-ietf-babel-rfc6126bis/draft-ietf-babel-rfc6126bis.xml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/draft-ietf-babel-rfc6126bis/draft-ietf-babel-rfc6126bis.xml b/draft-ietf-babel-rfc6126bis/draft-ietf-babel-rfc6126bis.xml
index 9fa18c4..d4dd853 100644
--- a/draft-ietf-babel-rfc6126bis/draft-ietf-babel-rfc6126bis.xml
+++ b/draft-ietf-babel-rfc6126bis/draft-ietf-babel-rfc6126bis.xml
@@ -746,6 +746,15 @@ a small multiple of the value received in the IHU.</t>
 neighbour's cost (<xref target="cost-computation"/>) and runs the route
 selection procedure (<xref target="route-selection"/>).</t>
 
+<t>The bidirectional reachability detection is a prerequisite for the
+routing information exchange and processing. Specifically, a node MUST ignore:
+<list style="symbols">
+  <t>any TLV except Hello if received from a neighbour that does not have an
+  entry in the neighbour table, and</t>
+  <t>any TLV except Hello and IHU if received from a neighbour that has an
+  entry in the neighbour table with txcost equal to infinity.</t>
+</list></t>
+
 </section>
 
 <section title="Cost Computation" anchor="cost-computation">
-- 
2.7.4

-- 
    Denis Ovsienko

---

From: Juliusz Chroboczek <[email-masked]>
Date: Mon, 12 Dec 2016 17:56:48 +0100

> +<t>A node that receives a Hello TLV from an unknown neighbour creates a new
> +entry in the neighbour table and sets its txcost to infinity.

[...]

> +  entry in the neighbour table with txcost equal to infinity.</t>

I am of somewhat mixed mind on this change.  While this is certainly what
all existing implementations do, I am not sure that it belongs in the base
protocol.  I can certainly envision an implementation that uses any
packet, not only one that contains a Hello, to create a neighbour
association.  I could even envision an extension that signals
neighbourship at the link layer (think Babel over Bluetooth Low Energy).

Here, at Babel towers, our policy has been to only add constraints to the
protocol in order to solve a problem.  So what problem are you trying to
solve with this addition?

  1. A problem in the base protocol?
  2. A problem in HMAC-based authentication?
  3. No problem?

I understand that you're trying to fix a problem in HMAC-based
authentication.  If I'm correct, then perhaps this addition belongs in the
HMAC document, something like

  If a node implements this extension, then it MUST NOT create a neighbour
  entry before receiving a Hello TLV, and MUST silently ignore etc.

If I'm wrong, and you're actually fixing an issue in the base protocol,
than your additions must explain to the reader why this is essential, not
just add a seemingly arbitrary requirement.

-- Juliusz

---

From: David Schinazi <[email-masked]>
Date: Mon, 12 Dec 2016 13:21:01 -0800

This change constrains the protocol more, and is not what *all* implementations do,
as I have one that doesn't do this.  An implementation could still learn useful information
from updates from a host that it has not heard Hellos for. Those routes must have an
infinite metric but once we do get the hello we could immediately start using them
instead of waiting for another update or asking for one explicitly.

Unless what I'm describing breaks some of the properties of Babel, I would advise
against constraining the protocol in this way. I agree with Juliusz that if this only
impacts the authentication extension, then it should be defined alongside the
extension and not the core protocol.

David

> On Dec 12, 2016, at 08:56, Juliusz Chroboczek <[email-masked]> wrote:
> 
> _______________________________________________
> babel mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/babel

---

From: Denis Ovsienko <[email-masked]>
Date: Mon, 12 Dec 2016 23:38:05 +0000

---- On Mon, 12 Dec 2016 16:56:48 +0000 Juliusz Chroboczek  wrote ---- 
>> +entry in the neighbour table and sets its txcost to infinity. 

Thank you for the comments. Please note you are now discussing two independent changes at once. Let me try to comment on each one separately.

The above proposed change is related to section I point 1 and the commit message is as follows:

-------------------------------------
Some of the thinking made in Appendix A.1 looks normative and could
make Section 3 (Protocol Operation) more complete if moved there."

This change in the middle of "Reverse Reachability Detection" adds one
specific piece that was missing: the document did specify updating and
flushing of a neighbour entry, now it also specifies initialization.
-------------------------------------

Let me add some detail to the above. So long as the neighbour table is one of the protocol data structures currently in the document, it is important to specify how the entries are added, modified and deleted. "Bidirectional Reachability Detection" defines the modification (one upon receiving an IHU TLV and another on a missed timer). Appendix A.1 defines when to delete an entry (perhaps not the best disposition as I tried to explain before but at least the wording is in the document, so can be looked at later). But there is no text in the document that tells when to add an entry to the table and how to set txcost. The specification is plainly incomplete without this and the proposed change solves this particular problem.

>> + entry in the neighbour table with txcost equal to infinity.</t> 

The above proposed change is related to section I point 2 (as stated in the message subject) and the commit message is as follows:
-------------------------------------
"The specification should explicitly state that the “green” TLVs must be
ignored unless they come from a valid neighbour..."

This change at the end of "Bidirectional Reachability Detection" adds
respective normative text.
-------------------------------------

This thinking is based on the slide with the FSM, the one that has in the title: "Should it be like this?". If it should be (like it seems to me), then the proposed change just spells the norm, however much obvious, as that is the purpose of a specification. For a protocol definition it is better to keep a trivial statement around than to allow implementations that are formally compliant but broken.

If the inferred FSM in the slides is wrong and one or more TLVs are actually due for processing even without a 2-way neighbour or even without a 1-way neighbour, then this needs to be specified too. Whichever way is intended, it needs to be clearly defined regardless of the particular reasoning behind.

>neighbourship at the link layer (think Babel over Bluetooth Low Energy). 

As far as I can currently imagine it, Babel without Hello or implied Hello would be a notably different protocol because without Hello sequence numbers the neighbour table would be different and neighbour history would have to be based on something else to keep things working. One thing I would expect is however the other potential protocol would compare to Babel with Hello, it would cost substantial design time anyway.

The mention of extensions is a good point and it gives me an idea on specifying the requirement better. What do you think about the following wording?

<t>The bidirectional reachability detection is a prerequisite for the 
routing information exchange and processing performed by the protocol
instance defined in this document. Specifically, a node MUST ignore
the following TLVs:
<list style="symbols"> 
<t>Acknowledgment Request, Acknowledgment, IHU, Router-Id, Next Hop,
Update, Route Request, and Seqno Request if received from a neighbour
that does not have an entry in the neighbour table, and</t>
<t>Acknowledgment Request, Acknowledgment, Router-Id, Next Hop, Update,
Route Request, and Seqno Request if received from a neighbour that has
an entry in the neighbour table with txcost equal to infinity.</t>
</list>
Babel protocol extensions that introduce additional TLVs SHOULD
specify whether to ignore those TLVs under the conditions stated above.
</t> 

> 2. A problem in HMAC-based authentication? 

Section I item 1: 1=yes, 2=no, 3=no.
Section I item 2: 1=yes, 2=yes, 3=no.

> entry before receiving a Hello TLV, and MUST silently ignore etc. 

Yes and no. I was looking how to fix it and in the course of it I had found issues elsewhere. Usually it helps to resolve the smaller problems first to make it easier to focus on the big ones.

It seems to me the authentication mechanism should not reach into the main protocol body as far as described above. There should be a better solution.

-- 
    Denis Ovsienko

---

From: Denis Ovsienko <[email-masked]>
Date: Tue, 13 Dec 2016 00:29:08 +0000

---- On Mon, 12 Dec 2016 21:21:01 +0000 David Schinazi<[email-masked]> wrote ---- 
 > extension and not the core protocol. 

Thank you for being specific, David. I understand what you do but I am not competent right now to confirm if this always works as expected from the routing point of view.

If all is fine, the specification should say that an Update TLV may be processed in this specific way in the absence of a normal 2-way neighbour. All the other TLVs still stand and may occur in an input packet and the specification should have the behaviour defined.

Another issue here may be, accepting any information that comes without a proof of freshness (like IHU without the sequence number as discussed before) leaves an exposure to a replay attack. A two-way exchange seems to be essential for protocol security, wherever performed.

-- 
    Denis Ovsienko

---

From: David Schinazi <[email-masked]>
Date: Tue, 13 Dec 2016 09:26:01 -0800

Denis,

I'm not sure I understand why the Update TLV is special here, when I haven't seen a
Hello why shouldn't I reply to AckReq? What's wrong with me replying to a RouteReq?

The fact that you need established bidirectional reachability still seems specific to the
HMAC extension to me. I agree with your points regarding proofs of freshness and
replay attacks, however those apply to the HMAC extension, not the core spec.
If I were to run Babel over IPsec/ESP, I would already have these properties without
constraining the Babel spec.

I don't see changes to the Hello structure as a "notably different protocol".
Today the spec lets me compute rxcost/txcost as I see fit, and the requirements
on how to compute metrics are only what is required to ensure routing properties.
If I had a magical oracle that perfectly estimated link quality (I keep it next to my
unicorn), then I could use that instead of maintaining hello history. And I could
even do this in a way compliant with the specs if all nodes have their Hello and IHU
intervals set to 11 minutes (2^16cs). But if I do that then I have no way to quickly
discover new peers when I join the network. This is why I'd like to dissociate
new peer detection from quality estimation.

Your change regarding the initialization of the neighbor table entry is
fine with today's spec, but I'd love for us to move in a direction where the
spec is less multicast focused and where section 3.2.3 (neighbor table)
would be more of an implementation detail than core spec.

David

> On Dec 12, 2016, at 16:29, Denis Ovsienko <[email-masked]> wrote:
>

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 14 Dec 2016 00:47:34 +0100

> If all is fine, the specification should say that an Update TLV may be
> processed in this specific way in the absence of a normal 2-way
> neighbour.

Does the spec not already say that?  I don't see any place where it would
say that updates from non-bidirectional neighbours, or even from
non-neighbours, are dropped.

To the contrary, what the spec fails to make clear is that it is legal to
drop updates before bidirectional reachability has been ascertained.  I'm
not sure that this is worth clarifying more -- none of the three
independent reimplementations had trouble with this particular bit.

Denis, you haven't replied to my question: what problem with the basic
protocol are you trying to solve?

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 14 Dec 2016 00:59:33 +0100

> Today the spec lets me compute rxcost/txcost as I see fit,

This is not going to change.  Earlier drafts of RFC 6126 mandated ETX, and
Joel successfully convinced me that this should become an informative
appendix.

> a magical oracle that perfectly estimated link quality

David Lamparter built one of those, which he uses in his version of IS-IS.
It works very well in IETF presentations, but has some flaws when discussed
over beer (the oracle fails to update the link quality of a neighbour
that's not being used).  More work is needed, perhaps next summer.

OTOH, David's oracle works at the link layer, so I would not recommend
disabling IHU processing even when using it -- probing bidirectional
reachability at the network layer even when the link layer claims
everything is working makes for a more robust implementation.

> But if I do that then I have no way to quickly discover new peers when
> I join the network.

Well, your magical link-layer oracle is probably able to provide
indication of new peers, so you could send a hello (perhaps over unicast)
when it says that a new neighbour has appeared.  Of course, the link-layer
oracle doesn't say a new Babel node has appeared, just that there is a new
link-layer neighbour.

> I'd love for us to move in a direction where the spec is less multicast
> focused and where section 3.2.3 (neighbor table) would be more of an
> implementation detail than core spec.

Hmm... even in the absence of bidirectional reachability and link quality
estimation, you still need to dump the (interface, IP) information
somewhere.  I guess you could put it directly in the route table, and
I don't think that the current spec disallows that -- the neighbour table
is conceptual, after all --, so I don't think it's worth working on
removing the neighbour table from the spec.

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 14 Dec 2016 01:04:31 +0100

> But there is no text in the document that tells when to add an entry to
> the table and how to set txcost. The specification is plainly incomplete
> without this

I partly agree.  Section 3.4.2 says:

    A node receiving an IHU updates the value of
    the sending neighbour's txcost (transmission cost), from its
    perspective, to the value contained in the IHU, and resets this
    neighbour's IHU timer to a small multiple of the value received in
    the IHU.

This is meant to say that when you receive an IHU, you grab the
corresponding neighbour table entry (creating one if it doesn't exist),
and dump the txcost in there.  I agree it's horribly badly written, but
the intent is clear.

Section 3.4.1 doesn't say to create a neighbour entry.  This needs fixing.

-- Juliusz