Fwd: [PATCH] Changes to make MKA actor Priority configurable

[Badrish Adiga H R]

On Tue, Dec 13, 2016 at 4:03 PM, Sabrina Dubroca <sd at queasysnail.net> wrote:
> 2016-12-05, 21:47:13 +0530, Badrish Adiga H R wrote:
>> From: Badrish Adiga H R <badrish.adigahr at gmail.com>
>>
>> Signed-off-by: Badrish Adiga H R <badrish.adigahr at gmail.com>
>
> That's quite similar to the patch I was working on, except for some comments below.
>
>
> [...]
>> diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c
>> index 2120a6e..b446f84 100644
>> --- a/wpa_supplicant/config.c
>> +++ b/wpa_supplicant/config.c
>> @@ -2127,6 +2127,7 @@ static const struct parse_data ssid_fields[] = {
>>         { INT_RANGE(macsec_policy, 0, 1) },
>>         { INT_RANGE(macsec_integ_only, 0, 1) },
>>         { INT_RANGE(macsec_port, 1, 65534) },
>> +       { INT_RANGE(mka_actor_priority, 0, 255) },
>>         { FUNC_KEY(mka_cak) },
>>         { FUNC_KEY(mka_ckn) },
>>  #endif /* CONFIG_MACSEC */
>
> I have an extra chunk to set the default value:
>
> @@ -2617,6 +2621,9 @@ void wpa_config_set_network_defaults(struct wpa_ssid *ssid)
>  #ifdef CONFIG_IEEE80211W
>         ssid->ieee80211w = MGMT_FRAME_PROTECTION_DEFAULT;
>  #endif /* CONFIG_IEEE80211W */
> +#ifdef CONFIG_MACSEC
> +       ssid->mka_priority = DEFAULT_PRIO_NOT_KEY_SERVER;
> +#endif /* CONFIG_MACSEC */
>         ssid->mac_addr = -1;
>  }
>
>
Thanks for looking into. I am okay with this change...

>
>> diff --git a/wpa_supplicant/config_ssid.h b/wpa_supplicant/config_ssid.h
>> index fe0f7fa..ac5d28e 100644
>> --- a/wpa_supplicant/config_ssid.h
>> +++ b/wpa_supplicant/config_ssid.h
>> @@ -751,6 +751,11 @@ struct wpa_ssid {
>>         int macsec_port;
>>
>>         /**
>> +        * mka_actor_priority - Priority of MKA Actor
>> +        */
>> +       u8 mka_actor_priority;
>
> That doesn't work. The way wpa_supplicant parses configuration, it
> will store a full int (32 bits), so it overwrites part of the CKN
> after it. You need to make this an 'int'.
>
A good catch and thanks again...

>> +
>> +       /**
>>          * mka_ckn - MKA pre-shared CKN
>>          */
>>  #define MACSEC_CKN_LEN 32
>> diff --git a/wpa_supplicant/wpa_cli.c b/wpa_supplicant/wpa_cli.c
>> index f11028a..d0b584c 100644
>> --- a/wpa_supplicant/wpa_cli.c
>> +++ b/wpa_supplicant/wpa_cli.c
>> @@ -1392,6 +1392,7 @@ static const char *network_fields[] = {
>>         "macsec_policy",
>>         "macsec_integ_only",
>>         "macsec_port",
>> +       "mka_actor_priority",
>>  #endif /* CONFIG_MACSEC */
>>  #ifdef CONFIG_HS20
>>         "update_identifier",
>> diff --git a/wpa_supplicant/wpa_supplicant.conf
>> b/wpa_supplicant/wpa_supplicant.conf
>> index edb230d..3ad2c44 100644
>> --- a/wpa_supplicant/wpa_supplicant.conf
>> +++ b/wpa_supplicant/wpa_supplicant.conf
>> @@ -901,13 +901,16 @@ fast_reauth=1
>>  # Port component of the SCI
>>  # Range: 1-65534 (default: 1)
>>  #
>> -# mka_cak and mka_ckn: IEEE 802.1X/MACsec pre-shared authentication mode
>> -# This allows to configure MACsec with a pre-shared key using a (CAK,CKN) pair.
>> -# In this mode, instances of wpa_supplicant can act as peers, one of
>> -# which will become the key server and start distributing SAKs.
>> +# mka_cak, mka_ckn and mka_actor_priority: IEEE 802.1X/MACsec pre-shared
>
> I wouldn't mention 'mka_actor_priority' here. You're in the
> "pre-shared auth" section, actor priority doesn't have anything to do
> with that.
>
I had put this in "pre-shared auth" section, because I felt this is
applicable to PSK mode only. In EAP_EXCHANGE mode, always
Authenticator should be Key Server right?

>> +# authentication mode; This allows to configure MACsec with a pre-shared key
>> +# using a (CAK,CKN) pair. In this mode, instances of wpa_supplicant can act as
>> +# MACsec peers. The peer with lower actor priority will become the key server
>> +# and start distributing SAKs.
>>  # mka_cak (CAK = Secure Connectivity Association Key) takes a
>> 16-bytes (128 bit)
>>  # hex-string (32 hex-digits)
>>  # mka_ckn (CKN = CAK Name) takes a 32-bytes (256 bit) hex-string (64
>> hex-digits)
>> +# mka_actor_priority (Priority of MKA Actor) is in 1..255 range with 255 being
>> +# default priority.
>
> You defined the valid range as 0..255.
>
> FWIW, here's the doc I have (I called it mka_priority instead of
> mka_actor_priority in my version, I'm fine either way):
>
> # mka_priority: IEEE 802.1X/MACsec priority
> # This allows to configure the priority of the wpa_supplicant instance
> # in the key server election process. Lower values indicate higher
> # priority, i.e. more likely to be elected.
> # Recommended ranges are specified in Table 9-2 (Key Server Priority
> # values) of the IEEE 802.1X-2010 standard.
> # Range: 0-255 (default: 255)
>
>
>>  #
>>  # mixed_cell: This option can be used to configure whether so called mixed
>>  # cells, i.e., networks that use both plaintext and encryption in the same
>> diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c
>> index d3fefda..cc41c74 100644
>> --- a/wpa_supplicant/wpas_kay.c
>> +++ b/wpa_supplicant/wpas_kay.c
>> @@ -233,7 +233,8 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant
>> *wpa_s, struct wpa_ssid *ssid)
>>         kay_ctx->disable_transmit_sa = wpas_disable_transmit_sa;
>>
>>         res = ieee802_1x_kay_init(kay_ctx, policy, ssid->macsec_port,
>> -                                 wpa_s->ifname, wpa_s->own_addr);
>> +                                 ssid->mka_actor_priority, wpa_s->ifname,
>> +                                 wpa_s->own_addr);
>>         if (res == NULL) {
>>                 os_free(kay_ctx);
>>                 return -1;
>
> --
> Sabrina

Regards,
Badrish