[PATCH RFC 2/2] babel: Add HMAC support

[Toke Høiland-Jørgensen]

Ondrej Zajicek <santiago at crfreenet.org> writes:

> Hi
>
> Finally got to finish the review. The code looks OK, comments below.

Thank you for the review! The draft was just accepted by the babel
working group, so I expect there will be a new version along soon. I'll
fix the issues you point out and submit a proper patch once I'm
reasonably confident that the draft is not going to change from under
us... :)

A few points for discussion below:

> Babel does not use something like security association ID to match
> keys, like in OSPF or RIP? That is unseemly.

No, it does not; currently, at least. This has, however, been discussed;
see here, and the thread that follows:

https://mailarchive.ietf.org/arch/msg/babel/i-XOBD9yoJlX_Fkv5GgBgy1B45o

I personally don't have a strong opinion about it; feel free to weigh in
on the babel list if you do :)

>> +static int
>> +babel_hmac_check_state(struct babel_iface *ifa, struct babel_parse_state *state)
>> +{
>> +  struct babel_proto *p = ifa->proto;
>> +  union babel_msg msg = {};
>> +  int ret;
>> +
>> +  if (state->hmac_challenge_len)
>> +  {
>> +    union babel_msg resp = {};
>> +    TRACE(D_PACKETS, "Sending HMAC challenge response to %I", state->saddr);
>> +    resp.type = BABEL_TLV_CHALLENGE_RESP;
>> +    resp.challenge.nonce_len = state->hmac_challenge_len;
>> +    resp.challenge.nonce = state->hmac_challenge;
>> +    babel_send_unicast(&resp, ifa, state->saddr);
>> +  }
>> +
>> +  msg.type = BABEL_TLV_HMAC;
>> +  msg.hmac.sender = state->saddr;
>> +  msg.hmac.pc = state->hmac_pc;
>> +  msg.hmac.pc_seen = state->hmac_pc_seen;
>> +  msg.hmac.index_len = state->hmac_index_len;
>> +  msg.hmac.index = state->hmac_index;
>> +  msg.hmac.challenge_seen = state->hmac_challenge_resp_seen;
>> +  memcpy(msg.hmac.challenge,
>> +	 state->hmac_challenge_resp,
>> +	 BABEL_HMAC_NONCE_LEN);
>> +
>> +  ret = babel_handle_hmac(&msg, ifa);
>
> We are synthetizing virtual TLV here just to call babel_handle_hmac()
> and split the code to two functions?

Yeah, so the reason I did it this way was to keep the parts that touch
internal state in babel.c, and have only code that operate directly on
packet data in packet.c. You may be right that it complicates things
rather than simplify; I'll take another look :)

> IMHO the draft describes this order:
>
> - parse trailer
> - preparse for PC and index
> - verify authentication (HMAC and PC/index)
> - regular parse and processing
>
> You are doing
>
> - parse trailer
> - verify HMAC
> - regular parse
> - verify PC/index
> - regular processing
>
> I would prefer if we just keep whole packet verification as a separate
> phase like described in the draft and as a separate function called
> from babel_process_packet(), even if that means two iterations through
> TLVs.

Why? The way it is described in the draft in influenced by the babeld
parser, which will do full processing (including updating internal
state) of TLVs as they are parsed. Since we already have a separate
parsing step, verifying the HMAC between that and the stateful checks
seems like the natural thing to do, rather than introduce a third round?

-Toke