| Message ID | CAGNNFCYs+LDaHkGjJX5dYE4s1B7k-5FAWkn8Zv_xr9JQP9UAaw@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
2016-11-24, 20:46:51 +0530, Badrish Adiga H R wrote: > Plz Ignore the above patch.... > > consider this one It doesn't apply, you have some extra '\n' characters. You're going to need to provide a commit message explaining these changes, and probably split them up in separate patches. You'll also need a sign-off line, see CONTRIBUTIONS. > diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c > index 3a495ca..0baa5d3 100644 > --- a/src/pae/ieee802_1x_kay.c > +++ b/src/pae/ieee802_1x_kay.c > @@ -1548,7 +1548,7 @@ ieee802_1x_mka_decode_dist_sak_body( > ieee802_1x_cp_connect_authenticated(kay->cp); > ieee802_1x_cp_sm_step(kay->cp); > wpa_printf(MSG_WARNING, "KaY:The Key server advise no MACsec"); > - participant->to_use_sak = TRUE; > + participant->to_use_sak = FALSE; > return 0; > } What incorrect behavior do you observe without this? > @@ -3071,7 +3071,8 @@ static void kay_l2_receive(void *ctx, const u8 > *src_addr, const u8 *buf, > */ > struct ieee802_1x_kay * > ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, > - u16 port, const char *ifname, const u8 *addr) > + u16 port, const char *ifname, const u8 *addr, > + enum mka_created_mode mode) > { > struct ieee802_1x_kay *kay; > > @@ -3094,7 +3095,12 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx > *ctx, enum macsec_policy policy, > os_strlcpy(kay->if_name, ifname, IFNAMSIZ); > os_memcpy(kay->actor_sci.addr, addr, ETH_ALEN); > kay->actor_sci.port = host_to_be16(port ? port : 0x0001); > - kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; > + > + if (mode == PSK) { > + kay->actor_priority = DEFAULT_PRIO_INFRA_PORT; > + } else { > + kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; > + } I'm not a big fan of this change. The reason is that if I connect my laptop to a MACsec switch, I'd like the switch to be the key server, not my laptop. I thought about making the priority configurable instead, just haven't implemented it yet. > /* While actor acts as a key server, shall distribute sakey */ > kay->dist_kn = 1; > diff --git a/src/pae/ieee802_1x_kay.h b/src/pae/ieee802_1x_kay.h > index ea5a0dd..c0b0ade 100644 > --- a/src/pae/ieee802_1x_kay.h > +++ b/src/pae/ieee802_1x_kay.h > @@ -233,7 +233,8 @@ struct ieee802_1x_kay { > > struct ieee802_1x_kay * > ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, > - u16 port, const char *ifname, const u8 *addr); > + u16 port, const char *ifname, const u8 *addr, > + enum mka_created_mode mode); > void ieee802_1x_kay_deinit(struct ieee802_1x_kay *kay); > > struct ieee802_1x_mka_participant * > diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c > index d3fefda..fb0f26d 100644 > --- a/wpa_supplicant/wpas_kay.c > +++ b/wpa_supplicant/wpas_kay.c > @@ -186,6 +186,7 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant > *wpa_s, struct wpa_ssid *ssid) > { > struct ieee802_1x_kay_ctx *kay_ctx; > struct ieee802_1x_kay *res = NULL; > + enum mka_created_mode mode; > enum macsec_policy policy; > > ieee802_1x_dealloc_kay_sm(wpa_s); > @@ -232,8 +233,11 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant > *wpa_s, struct wpa_ssid *ssid) > kay_ctx->enable_transmit_sa = wpas_enable_transmit_sa; > kay_ctx->disable_transmit_sa = wpas_disable_transmit_sa; > > + if ((ssid->mka_psk_set & MKA_PSK_SET) == MKA_PSK_SET) { > + mode = PSK; > + } > res = ieee802_1x_kay_init(kay_ctx, policy, ssid->macsec_port, > - wpa_s->ifname, wpa_s->own_addr); > + wpa_s->ifname, wpa_s->own_addr, mode); mode is used without being initialized here. I'm surprised gcc doesn't complain about that, looks like a compiler bug. > if (res == NULL) { > os_free(kay_ctx); > return -1; >
On Fri, Nov 25, 2016 at 8:59 PM, Sabrina Dubroca <sd@queasysnail.net> wrote: > 2016-11-24, 20:46:51 +0530, Badrish Adiga H R wrote: >> Plz Ignore the above patch.... >> >> consider this one > > It doesn't apply, you have some extra '\n' characters. > > You're going to need to provide a commit message explaining these > changes, and probably split them up in separate patches. You'll also > need a sign-off line, see CONTRIBUTIONS. > Thanks.. I will send new patch-set then... > >> diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c >> index 3a495ca..0baa5d3 100644 >> --- a/src/pae/ieee802_1x_kay.c >> +++ b/src/pae/ieee802_1x_kay.c >> @@ -1548,7 +1548,7 @@ ieee802_1x_mka_decode_dist_sak_body( >> ieee802_1x_cp_connect_authenticated(kay->cp); >> ieee802_1x_cp_sm_step(kay->cp); >> wpa_printf(MSG_WARNING, "KaY:The Key server advise no MACsec"); >> - participant->to_use_sak = TRUE; >> + participant->to_use_sak = FALSE; >> return 0; >> } > > What incorrect behavior do you observe without this? Nothing, it is just that value is incorrect... > > >> @@ -3071,7 +3071,8 @@ static void kay_l2_receive(void *ctx, const u8 >> *src_addr, const u8 *buf, >> */ >> struct ieee802_1x_kay * >> ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, >> - u16 port, const char *ifname, const u8 *addr) >> + u16 port, const char *ifname, const u8 *addr, >> + enum mka_created_mode mode) >> { >> struct ieee802_1x_kay *kay; >> >> @@ -3094,7 +3095,12 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx >> *ctx, enum macsec_policy policy, >> os_strlcpy(kay->if_name, ifname, IFNAMSIZ); >> os_memcpy(kay->actor_sci.addr, addr, ETH_ALEN); >> kay->actor_sci.port = host_to_be16(port ? port : 0x0001); >> - kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; >> + >> + if (mode == PSK) { >> + kay->actor_priority = DEFAULT_PRIO_INFRA_PORT; >> + } else { >> + kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; >> + } > > I'm not a big fan of this change. The reason is that if I connect my > laptop to a MACsec switch, I'd like the switch to be the key server, > not my laptop. > > I thought about making the priority configurable instead, just haven't > implemented it yet. > Making priority configurable is better alternative. in case laptop is connected to MACsec switch, I would prefer to make switch as authenticator and my laptop as Supplicant and use EAP mode MACsec which is more secure as compared to PSK. > >> /* While actor acts as a key server, shall distribute sakey */ >> kay->dist_kn = 1; >> diff --git a/src/pae/ieee802_1x_kay.h b/src/pae/ieee802_1x_kay.h >> index ea5a0dd..c0b0ade 100644 >> --- a/src/pae/ieee802_1x_kay.h >> +++ b/src/pae/ieee802_1x_kay.h >> @@ -233,7 +233,8 @@ struct ieee802_1x_kay { >> >> struct ieee802_1x_kay * >> ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, >> - u16 port, const char *ifname, const u8 *addr); >> + u16 port, const char *ifname, const u8 *addr, >> + enum mka_created_mode mode); >> void ieee802_1x_kay_deinit(struct ieee802_1x_kay *kay); >> >> struct ieee802_1x_mka_participant * >> diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c >> index d3fefda..fb0f26d 100644 >> --- a/wpa_supplicant/wpas_kay.c >> +++ b/wpa_supplicant/wpas_kay.c >> @@ -186,6 +186,7 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant >> *wpa_s, struct wpa_ssid *ssid) >> { >> struct ieee802_1x_kay_ctx *kay_ctx; >> struct ieee802_1x_kay *res = NULL; >> + enum mka_created_mode mode; >> enum macsec_policy policy; >> >> ieee802_1x_dealloc_kay_sm(wpa_s); >> @@ -232,8 +233,11 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant >> *wpa_s, struct wpa_ssid *ssid) >> kay_ctx->enable_transmit_sa = wpas_enable_transmit_sa; >> kay_ctx->disable_transmit_sa = wpas_disable_transmit_sa; >> >> + if ((ssid->mka_psk_set & MKA_PSK_SET) == MKA_PSK_SET) { >> + mode = PSK; >> + } >> res = ieee802_1x_kay_init(kay_ctx, policy, ssid->macsec_port, >> - wpa_s->ifname, wpa_s->own_addr); >> + wpa_s->ifname, wpa_s->own_addr, mode); > > mode is used without being initialized here. I'm surprised gcc doesn't > complain about that, looks like a compiler bug. > I uploaded wrong patch, so submitted another patch asking to ignore previous one. >> if (res == NULL) { >> os_free(kay_ctx); >> return -1; >> > > -- > Sabrina
diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c index 3a495ca..0baa5d3 100644 --- a/src/pae/ieee802_1x_kay.c +++ b/src/pae/ieee802_1x_kay.c @@ -1548,7 +1548,7 @@ ieee802_1x_mka_decode_dist_sak_body( ieee802_1x_cp_connect_authenticated(kay->cp); ieee802_1x_cp_sm_step(kay->cp); wpa_printf(MSG_WARNING, "KaY:The Key server advise no MACsec"); - participant->to_use_sak = TRUE; + participant->to_use_sak = FALSE; return 0; } @@ -3071,7 +3071,8 @@ static void kay_l2_receive(void *ctx, const u8 *src_addr, const u8 *buf, */ struct ieee802_1x_kay * ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, - u16 port, const char *ifname, const u8 *addr) + u16 port, const char *ifname, const u8 *addr, + enum mka_created_mode mode) { struct ieee802_1x_kay *kay; @@ -3094,7 +3095,12 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, os_strlcpy(kay->if_name, ifname, IFNAMSIZ); os_memcpy(kay->actor_sci.addr, addr, ETH_ALEN); kay->actor_sci.port = host_to_be16(port ? port : 0x0001); - kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; + + if (mode == PSK) { + kay->actor_priority = DEFAULT_PRIO_INFRA_PORT; + } else { + kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; + } /* While actor acts as a key server, shall distribute sakey */ kay->dist_kn = 1; diff --git a/src/pae/ieee802_1x_kay.h b/src/pae/ieee802_1x_kay.h index ea5a0dd..c0b0ade 100644 --- a/src/pae/ieee802_1x_kay.h +++ b/src/pae/ieee802_1x_kay.h @@ -233,7 +233,8 @@ struct ieee802_1x_kay { struct ieee802_1x_kay * ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, - u16 port, const char *ifname, const u8 *addr); + u16 port, const char *ifname, const u8 *addr, + enum mka_created_mode mode); void ieee802_1x_kay_deinit(struct ieee802_1x_kay *kay); struct ieee802_1x_mka_participant * diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c index d3fefda..fb0f26d 100644 --- a/wpa_supplicant/wpas_kay.c +++ b/wpa_supplicant/wpas_kay.c @@ -186,6 +186,7 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant *wpa_s, struct wpa_ssid *ssid) { struct ieee802_1x_kay_ctx *kay_ctx; struct ieee802_1x_kay *res = NULL; + enum mka_created_mode mode; enum macsec_policy policy;
Plz Ignore the above patch.... consider this one ieee802_1x_dealloc_kay_sm(wpa_s); @@ -232,8 +233,11 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant *wpa_s, struct wpa_ssid *ssid) kay_ctx->enable_transmit_sa = wpas_enable_transmit_sa; kay_ctx->disable_transmit_sa = wpas_disable_transmit_sa; + if ((ssid->mka_psk_set & MKA_PSK_SET) == MKA_PSK_SET) { + mode = PSK; + } res = ieee802_1x_kay_init(kay_ctx, policy, ssid->macsec_port, - wpa_s->ifname, wpa_s->own_addr); + wpa_s->ifname, wpa_s->own_addr, mode); if (res == NULL) { os_free(kay_ctx); return -1; On Thu, Nov 24, 2016 at 7:44 PM, Badrish Adiga H R <badrish.adigahr@gmail.com> wrote: > Fix 1: ieee802_1x_mka_decode_dist_sak_body is wrongly setting > to_use_sak flag to TRUE when body_len of distributed SAK is 0 > > Fix 2: if mode is PSK, default actor_priority should be DEFAULT_PRIO_INFRA_PORT. > ----------------------------------------------------- > > > diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c > index 3a495ca..986d2cb 100644 > --- a/src/pae/ieee802_1x_kay.c > +++ b/src/pae/ieee802_1x_kay.c > @@ -1548,7 +1548,7 @@ ieee802_1x_mka_decode_dist_sak_body( > ieee802_1x_cp_connect_authenticated(kay->cp); > ieee802_1x_cp_sm_step(kay->cp); > wpa_printf(MSG_WARNING, "KaY:The Key server advise no MACsec"); > - participant->to_use_sak = TRUE; > + participant->to_use_sak = FALSE; > return 0; > } > > @@ -3094,7 +3094,12 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx > *ctx, enum macsec_policy policy, > os_strlcpy(kay->if_name, ifname, IFNAMSIZ); > os_memcpy(kay->actor_sci.addr, addr, ETH_ALEN); > kay->actor_sci.port = host_to_be16(port ? port : 0x0001); > - kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; > + > + if (mode == PSK) { > + kay->actor_priority = DEFAULT_PRIO_INFRA_PORT; > + } else { > + kay->actor_priority = DEFAULT_PRIO_NOT_KEY_SERVER; > + } > > /* While actor acts as a key server, shall distribute sakey */ > kay->dist_kn = 1;