Message-ID: <1f17e2c5-c844-44a4-8970-64618c6295fb@openvpn.net>
Date: Thu, 12 Sep 2024 10:33:24 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v6 12/25] ovpn: implement packet processing

On 11/09/2024 15:30, Sabrina Dubroca wrote:
> 2024-09-11, 14:52:10 +0200, Antonio Quartulli wrote:
>> On 10/09/2024 15:04, Sabrina Dubroca wrote:
>>> 2024-09-06, 15:19:08 +0200, Antonio Quartulli wrote:
>>>> Therefore, how about having an array large enough to store all key IDs (max
>>>> ID is 7):
>>>
>>> Right, I forgot about that. For 8 keys, sure, array sounds ok. MACsec
>>> has only 4 and I also implemented it as an array
>>> (include/net/macsec.h: macsec_{tx,rx}_sc, sa array).
>>
>> thanks for the pointer!
>>
>>>
>>>> * array index is the key ID (fast lookup on RX);
>>>> * upon SET_KEY we store the provided keys and erase the rest;
>>>
>>> I'm not sure about erasing the rest. If you're installing the
>>> secondary ("next primary") key, you can't just erase the current
>>> primary? ("erase the rest" also means you'd only ever have one key in
>>> the array, which contradicts your last item in this list)
>> Yeah, my point is wrong.
>> I fooled myself assuming SET_KEY would install two keys each time, but this
>> is not the case, so we can't "erase everything else".
>>
>> What we can do is:
>> * if SET_KEY wants to install a key in PRIMARY, then we erase the key marked
>> as "primary" and we mark the new one as such
>> * if SET_KEY wants to install a key in SECONDARY, then we erase the key
>> without any mark.
> 
> Ok, then the DEL_KEY op would not really be needed.

It would be needed only when we want to kick an old key without 
installing a new one.
IMHO something that should not truly happen during the normal lifecycle 
of a peer, but there might be some corner cases where userspace may 
still want to do that (I am checking userspace to understand when this 
can truly happen, if at all)

> 
>>
>> This way we simulate writing into slots.
>>
>>>
>>>> * upon SET_KEY we store in a new field the index/ID of the PRIMARY (fast
>>>> lookup on TX), namely primary_id;
>>
>> FTR: this is what "marking as primary" means
>>
>>>> * upon SWAP we just save in primary_id the "other" index/ID;
>>>
>>> I'm confused by these two. Both SET_KEY and SWAP modify the primary_id?
>>
>> Yes.
>> SET_KEY may want to install a key in the primary slot: in that case the new
>> key has to be marked as primary immediately.
>> This normally happens only upon first SET_KEY after connection, when no
>> other key exists.
>>
>> SWAP will switch primary_id to the other key ID.
>>
>> makes sense?
> 
> Yes, thanks, that fixes up all my confusion around setting primary_id.
> 
>>>> * at any given time we will have only two keys in the array.
>>>
>>> This needs to be enforced in the SET_KEY implementation, otherwise
>>> SWAP will have inconsistent effects.
>>
>> yap. I hope what I wrote above about SET_KEY helps clarifying this part.
>>
>>>
>>>
>>>> It's pretty much like your option 1 and 2, but using an array
>>>> indexed by key ID.
>>>
>>> Are you decoupling TX and RX keys (so that they can be installed
>>> independently) in this proposal? I can't really tell, the array could
>>> be key pairs or separate.
>>
>> I would not decouple TX and RX keys.
>> Each array item should be the same as what we are now storing into each slot
>> (struct ovpn_crypto_key_slot *).
>> I could use two arrays, instead of an array of slots, but that means
>> changing way more logic and I don't see the benefit.
> 
> Avoid changing both keys every time when a link has very asymmetric
> traffic. And the concept of a primary/secondary key makes no sense on
> receive, all you need is keyids, and there's no need to swap slots,
> which avoids the "key not available during SWAP" issue
> entirely. Primary key only makes sense on TX.
> 
> But I'm guessing the keypair concept is also baked deep into openvpn.

Correct. Both keys are recomputed upon each renegotiation, even if one 
key was still usable.

> 
> 
>>>> The concept of slot is a bit lost, but it is not important as long as we can
>>>> keep the API and its semantics the same.
>>>
>>> Would it be a problem for userspace to keep track of key IDs, and use
>>> that (instead of slots) to communicate with the kernel? Make
>>> setkey/delkey be based on keyid, and replace swap with a set_tx
>>> operation which updates the current keyid for TX. That would seem like
>>> a better API, especially for the per-ID array you're proposing
>>> here. The concept of slots would be almost completely lost (only
>>> "current tx key" is left, which is similar to the "primary slot").
>>> (it becomes almost identical to the way MACsec does things, except
>>> possibly that in MACsec the TX and RX keys are not paired at all, so
>>> you can install/delete a TX key without touching any RX key)
>>>
>>
>> unfortunately changing userspace that way is not currently viable.
>> There are more components (and documentation) that attach to this slot
>> abstraction.
> 
> I thought that might be the case. Too bad, it means we have to keep
> the slots API and the kernel will be stuck with it forever (even if
> some day userspace can manage key ids instead of slots).

Yap, but I think there won't be any change like that in the near future.

> 
>> Hence my proposal to keep the API the same, but rework the internal for
>> better implementation.
> 
> Yeah, that makes sense if you have that constraint imposed by the
> existing userspace.
> 
>>
>>>
>>> Maybe you can also rework the current code to look a bit like this
>>> array-based proposal, and not give up the notion of slots, if that's
>>> something strongly built into the core of openvpn:
>>>
>>> - primary/secondary in ovpn_crypto_state become slots[2]
>>> - add a one-bit "primary" reference, used to look into the slots array
>>> - swap just flips that "primary" bit
>>> - TX uses slots[primary]
>>> - RX keeps inspecting both slots (now slot[0] and slot[1] instead of
>>>     primary/secondary) looking for the correct keyid
>>> - setkey/delkey still operate on primary/secondary, but need to figure
>>>     out whether slot[0] or slot[1] is primary/secondary based on
>>>     ->primary
>>>
>>> It's almost identical to the current code (and API), except you don't
>>> need to reassign any pointers to swap keys, so it should avoid the
>>> rekey issue.
>>
>> This approach sounds very close to what I was aiming for, but simpler
>> because we have slots[2] instead of slots[8] (which means that primary_id
>> can be just 'one-bit').
> 
> Yes.
> 
>> The only downside is that upon RX we have to check both keys.
>> However I don't think this is an issue as we have just 2 keys at most.
>>
>> I could even optimize the lookup by starting always from the primary key, as
>> that's the one being used most of the time by the sender.
> 
> Nice.
> 
>> Ok, I will go with this approach you summarized here at the end.
> 
> Ok. I think it should be a pretty minimal set of changes on top of the
> existing code, especially if you hide the primary/secondary accesses
> in inline helpers.

I think so to - working on it.

Thanks a lot!
Cheers,

> 

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists