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

[Ondrej Zajicek]

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. I would like to have better
fail mode in case of bugs, but not sure if that could be reasonably done.

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?


> > 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.

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."