[PATCH v4 0/8] Add MAC authentication support to the Babel protocol

[Toke Høiland-Jørgensen]

Ondrej Zajicek <santiago at crfreenet.org> writes:

> On Fri, Jun 04, 2021 at 12:42:54AM +0200, Toke Høiland-Jørgensen wrote:
>> Ondrej Zajicek <santiago at crfreenet.org> writes:
>> 
>> > On Sun, May 30, 2021 at 11:12:04PM +0200, Toke Høiland-Jørgensen wrote:
>> >> >> Toke Høiland-Jørgensen <toke at toke.dk> writes:
>> >> >> 
>> >> >> > This series adds MAC authentication support to the Babel protocol as specified
>> >> >> > in in RFC8967:
>> >> >> >
>> >> >> > https://www.rfc-editor.org/rfc/rfc8967
>> >> >> >
>> >> >> > I have performed basic interoperability testing between this implementation and
>> >> >> > the current babeld HMAC implementation[1]. The two implementations were able to
>> >> >> > successfully exchange authenticated messages with both HMAC-256 and Blake2s-256
>> >> >> > keys.
>> >> >> >
>> >> >> > Given the above, and the fact that the RFC was finally published at the the
>> >> >> > IETF, I believe this series is ready for merging (subject to review, of course).
>> >> >> > For those wanting to test the code, a version of Bird with this series applied
>> >> >> > is available on Github[2] for easy consumption.
>> >> >> >
>> >> >> > [1] https://github.com/jech/babeld/pull/52
>> >> >> > [2] https://github.com/tohojo/bird/tree/babel-mac-04
>> >> 
>> >> Ping? :)
>> >
>> > Hi
>> >
>> > Sorry for let you wait. I did most of the review some time ago, but got
>> > distracted by other issues and not finished it. Now it is done, i did
>> > some minor cleanups, but it is ready for merging.
>> 
>> Awesome! :)
>
> Hi
>
> Merged to master. There are few more issues i noticed during testing, see
> b174cc0abc0a9d7e84cc6fae46d9e19b714fbcfb for details. Two of these issues
> were related to bad value of auth_tx_overhead, which has an ugly fail mode
> where only large route updates had bad/no signature, but small IHU packets
> had good signature, so the link looks like OK.

Awesome! Many thanks, also for the bug fixes :)

> I would like to have better fail mode in case of bugs, but not sure if
> that could be reasonably done.

Hmm, one thought would be to do an explicit sanity check on link
bring-up by padding the initial Hello to the full packet length? That
should at least flush out any bugs inside Bird and (if we also actually
start checking the return value of the socket call) the OS. Big packets
could still be dropped on the wire, of course, but not much we can do
about that unless we want to do very extensive probing...

> One thing that seems confusing is handling of nbr->auth_expiry. It seems
> to serve two purposes - to cleanup nodes that never pass PC/challenge
> check, and to ensure that neighbor auth state got expired after fixed
> time, as required by spec. IMHO the second purpose could be handled
> simply by using existing Hello mechanism (no need to have duplicate
> timeout). We can just have init_expiry initialized in babel_get_neighbor()
> and disabled with first accepted Hello TLV. Is this OK or am i missing
> something?

No, that should be OK. In fact it's explicitly mentioned as a valid
strategy in section 4.4 of RFC8967:
https://www.rfc-editor.org/rfc/rfc8967.html#section-4.4

Not sure why I went with the separate timer; seemed simpler at the time
I guess?

>> > I also changed 'key' config option to 'password' (so it is 'password'
>> > with either ASCII string or hex-string). In future, we should probably
>> > switch to 'key' for both variants, as that is the name generally used for
>> > that. But using different keywords just for different notation of the
>> > same concept seems confusing to me.
>> 
>> OK. But why not just support both 'key' and 'password' for both formats
>> straight away, then?
>
> OK with me. Will change that.

Cool.

-Toke