Ondrej Zajicek <santiago@crfreenet.org> writes:
On Sun, Feb 23, 2020 at 11:56:33PM +0100, Toke Høiland-Jørgensen wrote:
This series adds MAC authentication support to the Babel protocol as specified in by the IETF Babel working group in draft-babel-hmac-10:
Hi
Some more comments / questions:
1/4:
BIRD_CHECK_GETRANDOM_SYSCALL - direct syscall case seems unnecessary, as we can fallback to /dev/urandom anyways.
BIRD_CHECK_GETRANDOM - just use generic AC_CHECK_FUNCS / AC_SEARCH_LIBS ?
OK.
I think that random_bytes() should not fail.
Preferably not; but we don't really have any guarantees that the syscall will succeed, do we? I guess I can add some sanity checks on startup and bail out if (e.g.) /dev/urandom cannot be opened. It would still be possible for read() or getrandom() to fail later on, though, no?
2/4:
blake2 - We definitely need unit tests here. Ideally there should exist some reference data / hash pairs for blake2. See mac_test.c
Yup, there does seem to be some test vectors in the blake2 repository; will add those.
There are '#if defined(NATIVE_LITTLE_ENDIAN)' in the code, does anybody define these?
Hmm, probably not? The FreeBSD Blake implementation seems to have a #define based on __BYTE_ORDER, so guess we could just add something like that as well?
3/4:
What is point of separating babel_parse_state and babel_read_state?
Why export packet/TLV structures from packets.c?
Well, I did both of these to be able to have all the auth-related code in a separate file, while still reusing the packet parsing macros etc...
General pattern in BIRD (including Babel) is that wire format details is hidden in packets.c and more abstract structures are exported outside (e.g. union babel_msg). Seems to me that it would make sense to have low-level auth code (TLV read/write code, packet signing/verifying) directly in packets.c, while high-level code (challenge response mechanism) in babel.c.
Hmm, I could have sworn that I got the idea of splitting it into its own file from one of the other protocols, but looking at them now that does not actually seem to be the case. So I guess I'll just move everything back into {packets,babel}.c :) -Toke