[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 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! :)

> I found a bug with getentropy() handling on BSD (it uses different return
> value than getrandom(), so it ended in infinite loop).
>
>
> Also, this part in babel_auth_check_pc() seems to me as buggy:
>
> +  /* If index differs, send challenge */
> +  if ((n->auth_index_len != msg->index_len ||
> +      memcmp(n->auth_index, msg->index, msg->index_len)) &&
> +      n->auth_next_challenge <= current_time())
> +  {
> +    LOG_PKT_AUTH("Index mismatch from %I on %s; sending challenge",
> +                 msg->sender, ifa->ifname);
> +    babel_auth_send_challenge(ifa, n);
> +    return 1;
> +  }
>
> When the condition fails due to rate limiting (auth_next_challenge),
> the execution continues in the babel_auth_check_pc(), which now assumes
> that the index matches. IMHO it should be more like:
>
>   if ((n->auth_index_len != msg->index_len) ||
>       memcmp(n->auth_index, msg->index, msg->index_len))
>   {
>     LOG_PKT_AUTH("Index mismatch from %I on %s",
>                  msg->sender, ifa->ifname);
>
>     if (n->auth_next_challenge <= current_time())
>       babel_auth_send_challenge(ifa, n);
>
>     return 1;
>   }
>
> Comments?

Yup, agreed - nice catch!

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

-Toke