GNOME Bugzilla – Bug 762114
[RFE] MACSEC support in NetworkManager
Last modified: 2017-01-17 13:08:53 UTC
Kernel soon gets support for it.
Initial implementation in branch bg/macsec-bgo762114. It requires a version of hostapd/wpa_supplicant not yet upstreamed [1]. [1] https://github.com/qsn/hostapd
now with nm-1-4 branched off, it seems all the "Since: 1.4" and NM_AVAILABLE_IN_1_4 should be updated. Same for libnm/libnm.ver >> core,libnm: introduce NMDeviceMacsec +typedef NMDevice NMDeviceMacsec; +typedef NMDeviceClass NMDeviceMacsecClass; yeah, some classes follow this pattern. I guess there are some people who like this style, fair enough. IMO a device class should look like this: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=652dc4b37928f99161fd275c2b28ded49aa9422c >> libnm-core: add NMSettingMacsec libnm-core/nm-setting-macsec.h lacks NM_AVAILABLE_IN_1_6 >> supplicant: add support for macsec options [1] https://github.com/qsn/hostapd/ do you have some better links? >> core: support macsec connections + union { + struct { + guint8 mac[6]; + guint16 port; + } s; + guint64 u; + } sci maybe: _nm_packed? the actual logic is quite complicated, but doesn't look wrong to me.
> libnm-core: add NMSettingMacsec +verify (NMSetting *setting, NMConnection *connection, GError **error) +{ + NMSettingMacsecPrivate *priv = NM_SETTING_MACSEC_GET_PRIVATE (setting); + NMSettingConnection *s_con; + NMSettingWired *s_wired; + NMSetting8021x *s_8021x; + + if (connection) { + s_con = nm_connection_get_setting_connection (connection); + s_wired = nm_connection_get_setting_wired (connection); + s_8021x = nm_connection_get_setting_802_1x (connection); + } else { + s_con = NULL; + s_wired = NULL; + s_8021x = NULL; + } Why not just initialize s_con/s_wired/etc to NULL when declaring them? Then we don't need an else. +static void +finalize (GObject *object) +{ + NMSettingMacsec *setting = NM_SETTING_MACSEC (object); + NMSettingMacsecPrivate *priv = NM_SETTING_MACSEC_GET_PRIVATE (setting); + + g_free (priv->parent); + g_free (priv->mka_cak); + g_free (priv->mka_ckn); If some of these are secrets, should we memset() them to 0 when freeing so they aren't around in memory? > supplicant: add support for macsec options + * FIXME: wpa_supplicant doesn't have yet a way to specify the macsec + * interface name, which is hardcoded to 'macsec0' for now. Ugh, that's awful. We need to fix that. > core: support macsec connections Can we get rid of supplicant_iface_connection_error_cb_handler() and just use nm_device_queue_state()? I'm pretty sure this NMDeviceEthernet 802.1x code predates nm_device_queue_state().
(In reply to Dan Williams from comment #3) > > supplicant: add support for macsec options > > + * FIXME: wpa_supplicant doesn't have yet a way to specify the macsec > + * interface name, which is hardcoded to 'macsec0' for now. > > Ugh, that's awful. We need to fix that. Now wpa_supplicant tries to create an interface with the given SCI and no predefined name (which would result in a 'macsecX' name), but since NM already created the link with that SCI wpa_supplicant reuses it and the name is correct. Fixed the rest and repushed.
>> supplicant: add an enum to specify the driver commit does not compile
(In reply to Thomas Haller from comment #5) > >> supplicant: add an enum to specify the driver > > commit does not compile Fixed now.
shouldn't we correct NLA_PUT_U64 (nlmsg, IFLA_MACSEC_SCI, props->sci); for endianness? + _CMP_FIELD_BOOL (a, b, include_sci); _CMP_FIELD_BOOL() is basically if ((!!(a)->include_sci) != (!!(b)->include_sci)) ... that is, checking whether both are either 0 or both non-equal to 0. As the type of include_sci is "bool:1", that is not necessary and _CMP_FIELD() is fine. I guess, in the end it doesn't really matter :) seems NMPlatformLnkMacsec.parent_ifindex is unused? >> core,libnm: introduce NMDeviceMacsec TODO: src/devices/nm-device-macsec.c should have the changes from bug 776719. DEPRECATED. Use the standard "PropertiesChanged" signal from "org.freedesktop.DBus.Properties" instead which exists since version NetworkManager 1.2.0. maybe we shouldn't add those signals for the new objects. Are they needed?
(In reply to Thomas Haller from comment #7) > shouldn't we correct > NLA_PUT_U64 (nlmsg, IFLA_MACSEC_SCI, props->sci); > for endianness? Fixed. > + _CMP_FIELD_BOOL (a, b, include_sci); > > _CMP_FIELD_BOOL() is basically > > if ((!!(a)->include_sci) != (!!(b)->include_sci)) > ... > > that is, checking whether both are either 0 or both non-equal to 0. > As the type of include_sci is "bool:1", that is not necessary and > _CMP_FIELD() is fine. > I guess, in the end it doesn't really matter :) Ok. > >> core,libnm: introduce NMDeviceMacsec > > TODO: src/devices/nm-device-macsec.c should have the changes from bug 776719. Done. > DEPRECATED. Use the standard "PropertiesChanged" signal from > "org.freedesktop.DBus.Properties" instead which exists since version > NetworkManager 1.2.0. > > maybe we shouldn't add those signals for the new objects. Are they needed? Yes, NMDevices are treated in a special way in nm-exported-object.c and require a PropertiesChanged signal. We could add an exception to the exception, but I prefer to keep things like now.
>> libnm-core: add NMSettingMacsec I don't think any of the properties should be G_PARAM_CONSTRUCT. Why are they? Since you also don't overwrite constructed()/constructor(), it makes almost no difference either. rest lgtm
(In reply to Thomas Haller from comment #9) > >> libnm-core: add NMSettingMacsec > > I don't think any of the properties should be G_PARAM_CONSTRUCT. Why are > they? > Since you also don't overwrite constructed()/constructor(), it makes almost > no difference either. obj_properties[PROP_PORT] = g_param_spec_int (NM_SETTING_MACSEC_PORT, "", "", 1, 65534, 1, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | NM_SETTING_PARAM_INFERRABLE | G_PARAM_STATIC_STRINGS); Without G_PARAM_CONSTRUCT the property is not initialized to 1 when the object is constructed.
>> fixup! supplicant: add support for macsec options if (!nm_supplicant_config_add_option (self, "mka_ckn", g_bytes_get_data (bytes, NULL), g_bytes_get_size (bytes), - TRUE, + NULL, error)) shouldn't it be "<hidden>"?
(In reply to Thomas Haller from comment #11) > shouldn't it be "<hidden>"? The CAK is considered a secret, but the CKN is not: https://w1.fi/cgit/hostap/tree/wpa_supplicant/wpas_kay.c#n359 https://w1.fi/cgit/hostap/tree/wpa_supplicant/wpas_kay.c#n370
Looks very good to me overall. > + * Copyright 2016 Red Hat, Inc. 2016, 2017 :) > 4bf6209 shared: add 64bit byte order conversion utilities Why do we need this? le64toh() from <endian.h> should be sufficient? > 46a3629 ethernet: simplify supplicant error path > 945ff4f platform: add support for macsec links Please add this interface to docs/api/Makefile.am and docs/api/network-manager-docs.xml too. (Perhaps we could generate these...)
(In reply to Lubomir Rintel from comment #13) > > 4bf6209 shared: add 64bit byte order conversion utilities > > Why do we need this? > le64toh() from <endian.h> should be sufficient? Yep, I missed that. Fixed all and repushed.
LGTM
branch merged to NetworkManager's master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=1a24f528c8645148e9612adff34d6edf70fdbb34