Ondrej Zajicek <santiago@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@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