core,libnm: introduce NMDeviceWireguard #162

jbeta · 2018-07-09T22:16:28Z

For now, the device only exposes partial link status (not including
peers). It cannot create new links.

This PR is mostly sanity check / request for comments. I'm also looking for guidance on how to model Peers/AllowedIPs on the D-Bus interface: would these be object paths to separate instances, a complex a(ss...qta(...)) D-Bus signature that maps to a NMWireguardPeer array, neither...?

vhumpa · 2018-07-09T22:21:06Z

NM CI: Please confirm test execution over this PR. Comment .ok\W+to\W+test. to confirm and/or .add\W+to\W+whitelist. to add the PR author in whitelist for future.

lkundrak

Thank you, this looks rather reasonable.

I've suggested some changes, mostly nitpicks.
Also, please split the commit into the server part and the libnm part. The introspection/ should either go into the server part or a separate commit.

The brand new checkpatch.pl seems rather happy too, with two OCD warnings.

lkundrak · 2018-07-10T11:36:42Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+    <property name="ListenPort" type="u" access="read"/>
+
+    <!--
+        Fwmark:


Wouldn't "FirewallMark" be a better name?

Maybe. fwmark is what wg(8) and WG APIs use, however - I'd like to stick to the upstream naming where possible.

lkundrak · 2018-07-10T11:37:13Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+    <!--
+        Fwmark:
+
+        Firewall mark.


Please add a sentence or two explaining what a Firewall mark is.

Thought I had added it somewhere actually! Will do.

lkundrak · 2018-07-10T11:39:29Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+
+        32-byte private WireGuard key.
+    -->
+    <property name="PrivateKey" type="v" access="read"/>


"v" signature ? Wouldn't "ay" make more sense for a byte array?

Also, the PrivateKey probably shouldn't be visible on the D-Bus API, since it has not access control and is visible to all local users. Is it necessary to have this?

Well, right now keys are shown as base64 strings when queried via D-Bus. The purpose is to play nice with wg config files and commandline, with both take base64 strings.

Well, it still doesn't have to be base64 on D-Bus API -- it may make more sense to encode it in nmcli and possibly the keyfile plugin. @thom311, what do you think?

I also would think that the private key is either a binary array (ay) or a string (s).

Maybe string is nicer, because then we can have there the base64 key directly. And if we some day in the future a different format (but why would we?), then we possibly could even extend the string with a "prefix:" (since ':' cannot be in plain base64).

If we settle on base64, then it the documentation comment should state that it's base64-encoded.

If we settle on base64, then it the documentation comment should state that it's base64-encoded.

That's fine.

Also, I hadn't looked into the access control for the D-Bus API yet. If indeed it's readable by all non-root users then of course we can't have the private key there :)

lkundrak · 2018-07-10T11:40:10Z

libnm/libnm.ver

@@ -1383,6 +1383,11 @@ global:
 	nm_connection_get_setting_6lowpan;
 	nm_connection_get_setting_wpan;
 	nm_device_6lowpan_get_type;
+	nm_device_wireguard_get_public_key;


Please keep this sorted alphabetically.

lkundrak · 2018-07-10T11:41:05Z

libnm/nm-device-wireguard.c

+const guchar*
+nm_device_wireguard_get_private_key (NMDeviceWireguard *device)
+{
+	g_return_val_if_fail (NM_IS_DEVICE_WIREGUARD (device), 0);


You probably meant NULL instead of 0.

Indeed. Thanks!

lkundrak · 2018-07-10T11:50:17Z

libnm/nm-device-wireguard.h

+/**
+ * NMDeviceWireguard:
+ */
+struct _NMDeviceWireguard {


For new devices, we like to keep these in the .c file, be cause they don't need to be part of the public API. See (nm-device-wpan for an example).

Also, when the object structure itself is not public, there's no need for the NMDeviceWireguardPrivate structure, since all NMDeviceWireguard fields would be private. (This is left for your consideration -- you may still want to keep the private structure for clarity.)

Understood. NMDeviceWireguardPrivate I was using while playing around with a PROP_PEERS implementation - but yeah I can drop it for now.

lkundrak · 2018-07-10T11:51:30Z

src/devices/nm-device-wireguard.c

+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.


I'm wondering if I could ask you to license this under LGPL, the same as the libnm part? We sometimes move things around between libnm and core, and the inconsistent licensing has caused us trouble.

This also applies to some other GPL-licensed files.

Ah - don't worry, it's just a case of lazy copy-paste licensing. Happy to mirror the licenses for similar files.

lkundrak · 2018-07-10T11:54:05Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+    -->
+    <property name="PublicKey" type="v" access="read"/>
+
+    <!--


Please just drop this, none of your properties emit the legacy signal.

lkundrak · 2018-07-10T11:54:19Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+        32-byte public WireGuard key.
+    -->
+    <property name="PublicKey" type="v" access="read"/>
+


Please include a "Parent" property too.

This is a question I meant to ask too - what is the purpose of the parent device?

On the second thought -- I have no idea if this is relevant for Wireguard. Maybe not.

Links that are associated with some other interface have this. E.g. VLANs, Tunneling devices or Veth pairs. In general it mirrors the IFLA_LINK property of the rtnetlink link. (In other words, if it says "link1@link0:" in "ip link", then link0 is the parent device.)

In that case, I don't think it's relevant, no. Wireguard is layer 3, it doesn't care about another particular link.

lkundrak · 2018-07-10T11:55:30Z

src/devices/nm-device-wireguard.h

+#define NM_IS_DEVICE_WIREGUARD_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass),  NM_TYPE_DEVICE_WIREGUARD))
+#define NM_DEVICE_WIREGUARD_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj),  NM_TYPE_DEVICE_WIREGUARD, NMDeviceWireguardClass))
+
+/* defined in the parent class, but exposed on D-Bus by the subclass. */


Well, actually not exposed. But I guess it should be.

True. I'll drop this too.

lkundrak · 2018-07-13T11:01:22Z

libnm/nm-device-wireguard.c

+ * @device: a #NMDeviceWireguard
+ *
+ * Gets the private key for this interface
+ *


Please document that it's a base64-encoded here too. I still find it somewhat odd.
Also, the "32-byte" length might be somewhat impractical -- it would be better if it was NUL terminated. g_base64_encode() doesn't actually guarantee any particular length, since base64 allows using whitespace pretty arbitrarily.

Please document that it's a base64-encoded here too.

Well, for this particular function it ~~isn't~~ shouldn't have been base64-encoded - whoops. I'll update this PR.

Also, the "32-byte" length might be somewhat impractical -- it would be better if it was NUL terminated. g_base64_encode() doesn't actually guarantee any particular length, since base64 allows using whitespace pretty arbitrarily.

I probably didn't make this clear - WG keys are fixed-size 32-byte binary data (NULs allowed). base64 is only used to portably move them around and we're not really interested in the length of the base64 strings at this level.

My intention was to use base64 only in interfaces that are expected to be directly visible to end-users, as this is the standard across WG utilities. This includes NM logs (e.g. nm_platform_lnk_wireguard_to_string), and any CLI components. Honestly I'm not sure about the D-Bus interface, maybe it doesn't fall under this category in which case I agree base64 would be odd.

For all the other internal and external APIs we simply need to pass around fixed-size 32-byte buffers that can hold arbitrary data.

My intention was to use base64 only in interfaces that are expected to be directly visible to end-users, as this is the standard across WG utilities. This includes NM logs (e.g. nm_platform_lnk_wireguard_to_string), and any CLI components. Honestly I'm not sure about the D-Bus interface, maybe it doesn't fall under this category in which case I agree base64 would be odd.

That makes sense. D-Bus is an API that's not exposed to the user, and I believe it shouldn't use base48. Agreed that logs/cli should.

I'll update this PR.

Looking forward. I think this already looks pretty well and almost ready to e merged.

thom311 · 2018-07-17T16:06:28Z

ok to test

zx2c4 · 2018-07-19T11:39:45Z

Makefile.am

@@ -234,6 +234,8 @@ introspection_sources = \
 	introspection/org.freedesktop.NetworkManager.Device.Vlan.h \
 	introspection/org.freedesktop.NetworkManager.Device.Vxlan.c \
 	introspection/org.freedesktop.NetworkManager.Device.Vxlan.h \
+	introspection/org.freedesktop.NetworkManager.Device.Wireguard.c \
+	introspection/org.freedesktop.NetworkManager.Device.Wireguard.h \


Wireguard -> WireGuard

(similar to WiMax.h below)

zx2c4 · 2018-07-19T11:39:55Z

docs/api/Makefile.am

@@ -74,6 +74,7 @@ content_files = \
 	dbus-org.freedesktop.NetworkManager.Device.Veth.xml \
 	dbus-org.freedesktop.NetworkManager.Settings.xml \
 	dbus-org.freedesktop.NetworkManager.Device.Wired.xml \
+	dbus-org.freedesktop.NetworkManager.Device.Wireguard.xml \


zx2c4 · 2018-07-19T11:40:02Z

docs/api/network-manager-docs.xml

@@ -203,8 +203,10 @@
      <xi:include href="dbus-org.freedesktop.NetworkManager.Device.Veth.xml"/>
      <xi:include href="dbus-org.freedesktop.NetworkManager.Device.Vlan.xml"/>
      <xi:include href="dbus-org.freedesktop.NetworkManager.Device.Vxlan.xml"/>
+      <xi:include href="dbus-org.freedesktop.NetworkManager.Device.Wireguard.xml"/>


zx2c4 · 2018-07-19T11:40:11Z

introspection/meson.build

@@ -34,6 +34,7 @@ ifaces = [
  'org.freedesktop.NetworkManager.Device.Wpan',
  'org.freedesktop.NetworkManager.Device.Wired',
  'org.freedesktop.NetworkManager.Device.Wireless',
+  'org.freedesktop.NetworkManager.Device.Wireguard',


Wireguard -> WireGuard

zx2c4 · 2018-07-19T11:40:17Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<node name="/">
+  <!--
+      org.freedesktop.NetworkManager.Device.Wireguard:


Wireguard -> WireGuard

zx2c4 · 2018-07-19T11:40:22Z

introspection/org.freedesktop.NetworkManager.Device.Wireguard.xml

+      @short_description: WireGuard Device
+
+  -->
+  <interface name="org.freedesktop.NetworkManager.Device.Wireguard">


Wireguard -> WireGuard

zx2c4 · 2018-07-19T11:40:30Z

libnm-core/nm-dbus-interface.h

@@ -73,6 +73,7 @@
 #define NM_DBUS_INTERFACE_DEVICE_MACVLAN       NM_DBUS_INTERFACE_DEVICE ".Macvlan"
 #define NM_DBUS_INTERFACE_DEVICE_PPP           NM_DBUS_INTERFACE_DEVICE ".Ppp"
 #define NM_DBUS_INTERFACE_DEVICE_VXLAN         NM_DBUS_INTERFACE_DEVICE ".Vxlan"
+#define NM_DBUS_INTERFACE_DEVICE_WIREGUARD     NM_DBUS_INTERFACE_DEVICE ".Wireguard"


Wireguard -> WireGuard

zx2c4 · 2018-07-19T11:40:42Z

libnm/nm-device-wireguard.c

+	NMDeviceClass parent;
+} NMDeviceWireguardClass;
+
+G_DEFINE_TYPE (NMDeviceWireguard, nm_device_wireguard, NM_TYPE_DEVICE)


Wireguard -> WireGuard

zx2c4 · 2018-07-19T11:40:45Z

libnm/nm-device-wireguard.c

+
+typedef struct {
+	NMDeviceClass parent;
+} NMDeviceWireguardClass;


Wireguard -> WireGuard

thom311 · 2018-07-21T15:52:01Z

@zx2c4 if the spelling is "WireGuard" with camel-case, then our style requires a separator at non-camel-case places. For example

./nm-device-wire-guard.h
nm_device_wire_guard_get_fwmark()
NM_DBUS_INTERFACE_DEVICE_WIRE_GUARD
NM_DEVICE_WIRE_GUARD

thom311 · 2018-07-21T16:37:09Z

@jbeta

I wasn't available this week, sorry for the delay.

Would you consider fixup commits from branch https://github.com/NetworkManager/NetworkManager/tree/th/review/pr/162 for inclusion? If you happen to agree, just squash the patches. Thanks.

After that, it looks good to me. The last remaining question is the naming.

zx2c4 · 2018-07-26T20:27:47Z

./nm-device-wire-guard.h
nm_device_wire_guard_get_fwmark()
NM_DBUS_INTERFACE_DEVICE_WIRE_GUARD
NM_DEVICE_WIRE_GUARD

Oh, ugh, no, please no. That will cause way more confusion and hurt obvious source searches and really isn't nice. That doesn't make sense for so many reasons.

WiMax is /org.freedesktop.NetworkManager.WiMax and nm-device-wimax.h. Please keep WireGuard as "WireGuard" and "wireguard" and "WIREGUARD", depending on context, but never "Wireguard" or "WIRE_GUARD", both of which are very wrong.

thom311 · 2018-07-27T06:00:25Z

WiMax is /org.freedesktop.NetworkManager.WiMax and nm-device-wimax.h

I am aware of that. WiMax is also deprecated. The older the code, the less it conforms to our current style (which, slightly adjusts over time).

Anyway, I think it doesn't have so strict, and

WireGuard
/org.freedesktop.NetworkManager.WireGuard
nm-device-wireguard.h
nm_device_wireguard_get_fwmark()

is fine with me.

@lkundrak WDYT?

jbeta · 2018-07-28T12:35:51Z

I wasn't available this week, sorry for the delay.

No worries - this week it was my turn :)

Would you consider fixup commits from branch https://github.com/NetworkManager/NetworkManager/tree/th/review/pr/162 for inclusion? If you happen to agree, just squash the patches. Thanks.

Thanks for this. Most fixups I agree with and I'll update the PR with them. However taking into account the above feedback from @lkundrak, I think the public key should also become an array on both libnm and D-Bus APIs.

Anyway, I think it doesn't have so strict, and

WireGuard
/org.freedesktop.NetworkManager.WireGuard
nm-device-wireguard.h
nm_device_wireguard_get_fwmark()
is fine with me.

That looks sane to me. Wireguard instead of WireGuard was just an oversight on my part, and breaking the name into two words is wrong for many reasons including practical. So if @zx2c4 also agrees let's go for this.

thom311 · 2018-08-02T07:35:40Z

Thanks for this. Most fixups I agree with and I'll update the PR with them. However taking into account the above feedback from @lkundrak, I think the public key should also become an array on both libnm and D-Bus APIs.

Hi, I don't fully understand this reply. My suggestion (the fixup commits) do not affect that the public key is exposed on libnm and D-Bus API. It however removes unused API for the private key. I agree with how the public key is treated.

Respect WireGuard canonical capitalization on identifiers. As per discussion on: NetworkManager#162

For now, the device only exposes partial link status (not including peers). It cannot create new links.

jbeta · 2018-08-02T22:30:11Z

Hi, I don't fully understand this reply. My suggestion (the fixup commits) do not affect that the public key is exposed on libnm and D-Bus API. It however removes unused API for the private key. I agree with how the public key is treated.

Correct. I was just pointing out that as per previous discussion with @lkundrak we should also remove base64 from the non-user-visible APIs. That's done now.

Updated the PR with:

extra initial commit renaming the existing code (s/Wireguard/WireGuard)
review changes from @thom311
switch to "ay" signature for D-Bus API (tested with NUL bytes in the public key, no issues)

thom311 · 2018-08-03T06:29:30Z

Hi,

switch to "ay" signature for D-Bus API (tested with NUL bytes in the public key, no issues)

I personally don't think this change was a good idea, because a base64 string is easier to work with. Anyway, the change is fine with me.

However,

const char *
nm_device_wireguard_get_public_key (NMDeviceWireguard *device)
{
    g_return_val_if_fail (NM_IS_DEVICE_WIREGUARD (device), NULL);

    return NM_DEVICE_WIREGUARD_GET_PRIVATE (device)->public_key;
}

public_key is no longer a NUL terminated string. You cannot expose it with such an API. It must be instead something like:

const guint8 *nm_device_wireguard_get_public_key (NMDeviceWireguard *device, guint *out_length);

or better:

GBytes *nm_device_wireguard_get_public_key (NMDeviceWireguard *device);

Also,

static GVariant *
get_public_key_as_variant (NMDeviceWireGuard *device)
{
     NMDeviceWireGuardPrivate *priv;

     g_return_val_if_fail (NM_IS_DEVICE_WIREGUARD (device), NULL);
     priv = NM_DEVICE_WIREGUARD_GET_PRIVATE (device);

     return g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE,
                                       priv->public_key, sizeof (priv->public_key), 1);
}

is wrong. sizeof(priv->public_key) is the size of a pointer (4 or 8 bytes).

Also, in libnm's NMDeviceWireGuard, I think that PROP_PUBLIC_KEY should be of type g_param_spec_boxed(..., G_TYPE_BYTES, ...) instead of GVariant.

jbeta · 2018-08-03T06:41:41Z

I personally don't think this change was a good idea, because a base64 string is easier to work with.

Hopefully it's the other way around and we save consumers of these APIs a base64 decoding step. I'm OK with this being reverted later if turns out not to be the case.

public_key is no longer a NUL terminated string. You cannot expose it with such an API. It must be instead something like:

The key size is always 32 bytes, so in practice you can. But I'll follow your advice and use GBytes.

is wrong. sizeof(priv->public_key) is the size of a pointer (4 or 8 bytes).

True. Thanks for spotting this.

Also, in libnm's NMDeviceWireGuard, I think that PROP_PUBLIC_KEY should be of type g_param_spec_boxed(..., G_TYPE_BYTES, ...) instead of GVariant.

Ah, I didn't realize that was an option. That's easier then.

Thanks again for reviewing!

thom311 · 2018-08-03T06:46:31Z

See NM_ACCESS_POINT_SSID as an example for a "ay" array in libnm.

I'm OK with this being reverted later if turns out not to be the case.

Maybe "ay" + GBytes is indeed better. In the end, there are pros+cons. Leave it as "ay", ok?

The key size is always 32 bytes, so in practice you can.

I think it's better to not commit the API to this. Also, I think the previous API could not be nicely exposed via GObject introspection (for example, the Python bindings wouldn't be correct).

jbeta · 2018-08-03T19:31:19Z

Switched to GBytes + boxed param for libnm now.

zx2c4 · 2018-08-04T14:42:34Z

Looks good to me. Let's merge this so @jbeta can move onto part 3?

thom311 · 2018-08-06T06:39:14Z

merged to master.

Thanks Javier!!

jbeta force-pushed the wireguard-device-basic branch from f510352 to 980e0cc Compare July 9, 2018 22:28

lkundrak requested changes Jul 10, 2018

View reviewed changes

jbeta force-pushed the wireguard-device-basic branch 2 times, most recently from 082c37d to 647c478 Compare July 11, 2018 18:10

lkundrak approved these changes Jul 13, 2018

View reviewed changes

lkundrak reviewed Jul 13, 2018

View reviewed changes

zx2c4 reviewed Jul 19, 2018

View reviewed changes

libnm/nm-device-wireguard.c Outdated

typedef struct {

NMDeviceClass parent;

} NMDeviceWireguardClass;

Copy link

Contributor

zx2c4 Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wireguard -> WireGuard

jbeta added 2 commits August 2, 2018 19:05

platform: rename instances of Wireguard to WireGuard

9a39150

Respect WireGuard canonical capitalization on identifiers. As per discussion on: NetworkManager#162

core: introduce NMDeviceWireGuard

f41c306

For now, the device only exposes partial link status (not including peers). It cannot create new links.

jbeta force-pushed the wireguard-device-basic branch from 647c478 to c5a93fc Compare August 2, 2018 22:17

libnm: introduce NMDeviceWireGuard

37b12b3

jbeta force-pushed the wireguard-device-basic branch from c5a93fc to 37b12b3 Compare August 3, 2018 19:28

thom311 closed this Aug 6, 2018

core,libnm: introduce NMDeviceWireguard #162

core,libnm: introduce NMDeviceWireguard #162

Conversation

jbeta commented Jul 9, 2018

vhumpa commented Jul 9, 2018

lkundrak left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbeta Jul 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbeta Jul 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbeta Jul 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thom311 commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thom311 commented Jul 21, 2018

thom311 commented Jul 21, 2018

zx2c4 commented Jul 26, 2018

thom311 commented Jul 27, 2018

jbeta commented Jul 28, 2018

thom311 commented Aug 2, 2018

jbeta commented Aug 2, 2018

thom311 commented Aug 3, 2018

jbeta commented Aug 3, 2018

thom311 commented Aug 3, 2018

jbeta commented Aug 3, 2018

zx2c4 commented Aug 4, 2018

thom311 commented Aug 6, 2018

lkundrak left a comment •

edited

jbeta Jul 10, 2018 •

edited

jbeta Jul 10, 2018 •

edited

jbeta Jul 14, 2018 •

edited