Key Server support for Group CAs

[Jouni Malinen]

On Wed, Apr 20, 2022 at 06:16:13PM +0000, Greg Goblirsch wrote:
> Signed-off-by: Greg Goblirsch <gregg at thinklogical.com>

Could you please provide a more complete commit message that would
explain what this is used for and why the particular changes are needed?
Some of the changes seemed to be independent and not really necessarily
targeting the generic "Key Server support for Group CAs" part directly.
It would be be helpful if such changes were separate into individual
patches that each cover one logical set of changes with a description of
the change in the commit message.

> diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c
> @@ -1159,10 +1159,9 @@ static int ieee802_1x_mka_decode_live_peer_body(
>                         continue;

This is whitespace damaged and does not apply since all the tabs have
been converted into spaces.

>                 peer = ieee802_1x_kay_get_peer(participant, peer_mi->mi);
> -               if (peer) {
> -                       peer->mn = peer_mn;
> -               } else if (!ieee802_1x_kay_create_potential_peer(
> -                               participant, peer_mi->mi, peer_mn)) {
> +               if (!peer) {
> +                       if (!ieee802_1x_kay_create_potential_peer(
> +                               participant, peer_mi->mi, peer_mn))
>                         return -1;

Why does this remove updating of peer->mn?

> @@ -1737,6 +1736,12 @@ ieee802_1x_mka_decode_dist_sak_body(
>                 return -1;
>         }
>  
> +       if (!dl_list_empty(&participant->potential_peers)) {
> +               wpa_printf(MSG_ERROR,
> +                       "KaY: I can't accept the distributed SAK as potential peer list is not empty");
> +               return -1;
> +       }
> +

Why is this added? Maybe this should be in a separate patch with clear
justification in the commit message.

> @@ -2142,15 +2147,13 @@ ieee802_1x_kay_generate_new_sak(struct ieee802_1x_mka_participant *participant)
>                 return -1;
>         }
>  
> -       /* FIXME: A fresh SAK not generated until
> +       /* A fresh SAK not generated until
>          * the live peer list contains at least one peer and
>          * MKA life time has elapsed since the prior SAK was first distributed,
>          * or the Key server's potential peer is empty
> -        * but I can't understand the second item, so
> -        * here only check first item and ingore
> -        *   && (!dl_list_empty(&participant->potential_peers))) {
>          */
> -       if ((time(NULL) - kay->dist_time) < MKA_LIFE_TIME / 1000) {
> +       if (((time(NULL) - kay->dist_time) < MKA_LIFE_TIME / 1000) &&
> +           (!dl_list_empty(&participant->potential_peers))) {
>                 wpa_printf(MSG_ERROR,
>                            "KaY: Life time has not elapsed since prior SAK distributed");
>                 return -1;

Is this related to the change above? If so, this should be with it in a
separate patch. If not, this would likely need to be in yet another
patch on its own with the commit message explaining what this does (I'd
assume this is just to address that FIXME comment).

> @@ -2290,9 +2293,6 @@ ieee802_1x_kay_elect_key_server(struct ieee802_1x_mka_participant *participant)
>         /* elect the key server among the peers */
>         dl_list_for_each(peer, &participant->live_peers,
>                          struct ieee802_1x_kay_peer, list) {
> -               if (!peer->is_key_server)
> -                       continue;
> -
>                 if (!key_server) {
>                         key_server = peer;

Why is that check removed?

> @@ -2645,10 +2645,10 @@ static void ieee802_1x_participant_timer(void *eloop_ctx, void *timeout_ctx)
>         }
>  
>         if (participant->new_sak && participant->is_key_server) {
> -               if (!ieee802_1x_kay_generate_new_sak(participant))
> +               if (!ieee802_1x_kay_generate_new_sak(participant)) {
>                         participant->to_dist_sak = true;
> -
> -               participant->new_sak = false;
> +                       participant->new_sak = false;
> +        }
>         }
>  

Maybe this would be clearer in a separate patch.. The commit message
would need to explain why this is needed.

> @@ -3217,8 +3217,6 @@ static int ieee802_1x_kay_decode_mkpdu(struct ieee802_1x_kay *kay,
>         int i;
>         const u8 *pos;
>         bool handled[256];
> -       bool bad_sak_use = false; /* Error detected while processing SAK Use
> -                                  * parameter set */
...

All these ieee802_1x_kay_decode_mkpdu() changes that remove bad_sak_use
and discarding of body_type != MKA_SAK_USE would need be clearly
described in the commit message.

> @@ -3773,21 +3746,28 @@ ieee802_1x_kay_create_mka(struct ieee802_1x_kay *kay,
>  
>         dl_list_add(&kay->participant_list, &participant->list);
>  
> -       usecs = os_random() % (kay->mka_hello_time * 1000);
> -       eloop_register_timeout(0, usecs, ieee802_1x_participant_timer,
> -                              participant, NULL);
> -
>         /* Disable MKA lifetime for PSK mode.
>          * The peer(s) can take a long time to come up, because we
>          * create a "standby" MKA, and we need it to remain live until
>          * some peer appears.
>          */
>         if (mode != PSK) {
> +               usecs = os_random() % (kay->mka_hello_time * 1000);
>                 participant->mka_life = MKA_LIFE_TIME / 1000 + time(NULL) +
>                         usecs / 1000000;
>         }
>         participant->mode = mode;
>  
> +       if (participant->retry_count < MAX_RETRY_CNT ||
> +           participant->mode == PSK) {
> +               ieee802_1x_participant_send_mkpdu(participant);
> +               participant->retry_count++;
> +       }
> +
> +       eloop_register_timeout(kay->mka_hello_time / 1000, 0,
> +                              ieee802_1x_participant_timer,
> +                              participant, NULL);

Why does this remove the randomness from that timer? These changes are
not really clear and need to be explained in the commit message.

-- 
Jouni Malinen                                            PGP id EFC895FA