After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762114 - [RFE] MACSEC support in NetworkManager
[RFE] MACSEC support in NetworkManager
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Beniamino Galvani
NetworkManager maintainer(s)
Depends on:
Blocks: nm-next
 
 
Reported: 2016-02-15 23:36 UTC by Thomas Haller
Modified: 2017-01-17 13:08 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thomas Haller 2016-02-15 23:36:49 UTC
Kernel soon gets support for it.
Comment 1 Beniamino Galvani 2016-07-22 19:26:51 UTC
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
Comment 2 Thomas Haller 2016-08-17 18:49:48 UTC
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.
Comment 3 Dan Williams 2016-08-25 20:34:31 UTC
> 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().
Comment 4 Beniamino Galvani 2017-01-02 10:12:51 UTC
(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.
Comment 5 Thomas Haller 2017-01-02 12:59:07 UTC
>>  supplicant: add an enum to specify the driver

commit does not compile
Comment 6 Beniamino Galvani 2017-01-02 13:29:49 UTC
(In reply to Thomas Haller from comment #5)
> >>  supplicant: add an enum to specify the driver
> 
> commit does not compile

Fixed now.
Comment 7 Thomas Haller 2017-01-02 16:50:36 UTC
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?
Comment 8 Beniamino Galvani 2017-01-08 13:50:03 UTC
(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.
Comment 9 Thomas Haller 2017-01-09 14:17:41 UTC
>> 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
Comment 10 Beniamino Galvani 2017-01-10 15:37:50 UTC
(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.
Comment 11 Thomas Haller 2017-01-13 10:09:10 UTC
>> 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>"?
Comment 12 Beniamino Galvani 2017-01-16 08:48:42 UTC
(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
Comment 13 Lubomir Rintel 2017-01-16 12:03:31 UTC
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...)
Comment 14 Beniamino Galvani 2017-01-16 15:32:09 UTC
(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.
Comment 15 Lubomir Rintel 2017-01-17 11:22:56 UTC
LGTM
Comment 16 Thomas Haller 2017-01-17 13:08:53 UTC
branch merged to NetworkManager's master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=1a24f528c8645148e9612adff34d6edf70fdbb34