[babel] Comments on the MAC authentication for Babel draft

[Antonin Décimo <[e-mail]>]

Hello all,

I have some comments on the MAC authentication for Babel draft.
The first three date from 2020-12-28, the others are new.


1. Key length

This is the change I'm proposing in §7:

 Keys must be chosen in a manner that makes them difficult to guess.
-Ideally, they should have a length of 32 octets (both for HMAC-SHA256
-and Blake2s), and be chosen randomly.
+Ideally, they should be chosen randomly, and have the maximum key
+length supported by the MAC algorithm (32 octets for keyed BLAKE2S)
+or no less than the block size of the MAC algorithm (64 octets for
+HMAC-SHA256).


2. Digest length

Reading the length of the MAC TLV in order to compute a MAC of this
length is not compliant with the protocol (§4.3) since MACs are
computed without looking into the packet.

> Then, for each key configured on the receiving interface, the
> receiver computes the MAC of the packet. It then compares every
> generated MAC against every MAC included in the packet;

Still, it was an error I was almost going to make. I propose the
following change to ensure that we're always using the maximum digest
length of the MAC algorithm.

This is the change I'm proposing in §4.1:

 implementation MUST implement HMAC-SHA256 as defined in
 <xref target="RFC6234"/> and Section 2 of <xref target="RFC2104"/>,
 SHOULD implement keyed BLAKE2s <xref target="RFC7693"/>, and MAY implement
 other MAC algorithms.
+The length of the MAC MUST be the maximum MAC length computable by
+the MAC algorithm (32 octets for both HMAC-SHA256 and keyed
+BLAKE2S).


3. Lazy computation

With the idea that computing a MAC is more costly than comparing MACs,
and that there's a very limited amount of MACs to check, I propose we
lazily compute MACs. Instead of computing the MAC for every key on
packet reception, we compute the MAC with the first key, then check it
against the MACs carried by the packet. If there's a match, the packet
is immediately accepted. If there's no match, we the proceed to the
next key until all keys are exhausted, and the packet is rejected.

This is the change I'm proposing in §4.3:

-Then, for each key configured on the receiving interface, the
-receiver computes the MAC of the packet.  It then compares every
-generated MAC against every MAC included in the packet; if there is
-at least one match, the packet passes the MAC test; if there is none,
-the packet MUST be silently dropped and processing stops at this
-point.
+Then, for each key configured on the receiving interface, the
+receiver computes the MAC of the packet and compares it against every
+MAC included in the packet; if there is at least one match, the
+packet passes the MAC test; if there is none, the receiver proceeds
+with the next key.  If none of the MAC matched, the packet MUST be
+silently dropped and processing stops at this point.


4. Accepting Challenges

When receiving a challenge reply, a node must check if there was a
challenge in progress before validating the challenge.

§ 3.2 Neighbour table
> The Nonce and expiry timer are initially undefined

§ 4.3.1.3. Receiving challenge replies
> If no challenge is in progress, i.e., if there is no Nonce stored in
> the Neighbour Table entry or the Challenge timer has expired, the
> Challenge Reply MUST be silently ignored and the challenge has
> failed.

However the text does not explicitly describe if there are changes
to be made to the overall state when the challenge succeeds.

> Otherwise, the node compares the Nonce contained in the Challenge
> Reply with the Nonce contained in the Neighbour Table entry. If the
> two are equal (they have the same length and content), then the
> challenge has succeeded; otherwise, the challenge has failed.

As it is not possible to have a running or expired timer and an
uninitialised Nonce field, I think that the timer should be
initialised in the "expired" state. The "challenge is in progress"
test can be simplified to "is the timer expired?", and then if the
challenge succeeds, the timer should be set into the "expired" state,
so that duplicated or incorrect challenge replies are rejected.


5. Replying to challenges

Section 4.3.1.2. Replying to challenges, the sentence

> A challenge sent to a multicast address MUST be silently ignored.

could be moved earlier, as it describes a case where the challenge
request should be ignored, but it is after the text indicating how to
construct and send a challenge reply.


6. Timers

I was confused at first between the "challenge expiry timer", the two
rate limits, and the neighbour state expiry timer. I suggest, for
consistency, the following changes:

section 3.2:
- The Nonce and expiry timer are initially undefined
+ The Nonce and challenge expiry timer are initially undefined

section 4.3.1.3
- the Challenge timer has expired
+ the challenge timer has expired


7. TLV names shouldn't be pluralised

Section 4.3
- ignored except PC TLVs, Challenge Requests and Challenge Replies.
+ ignored except PC, Challenge Request, and Challenge Reply TLVs.

- 4.3.1.  Challenge Requests and Replies
+ 4.3.1.  Challenge requests and replies


8. Consistency

Section 6.2
replace "4 octet" with "4-octet" (single dash)

Section 7
- nonces included in challenge requests and responses can
+ nonces included in challenge requests and replies can


9. Relations with the Information Model

The MAC draft defines (amongst other) three optionaly tunable parameters:
- the rate limit for sending Challenge Replies;
- the rate limit for sending Challenge Requests;
- various strategies for expiring neighbour state, with their own
  tunable parameters.
Should they be reflected in the models?


All the best,

-- Antonin