Re: [RFC PATCH 2/2] platform: add support for WireGuard links

[Javier Arteaga <jarteaga jbeta is>]

Hi Thomas,

Thanks a lot for your feedback! Comments inline.

On Wed, Mar 14, 2018 at 3:15 PM, Thomas Haller <thaller redhat com> wrote:
[...]
> > +     NMP_OBJECT_TYPE_LNK_WIREGUARD,
> > +
> > +     NMP_OBJECT_TYPE_WIREGUARD_PEER,
> > +     NMP_OBJECT_TYPE_WIREGUARD_ALLOWEDIP,
>
> You ask whether peer/allowed-ip should be their own NMPObject
> implementations. Maybe, but it's not stringend.
[...]
> I tend to think they should not be own objects. If they are their own
> NMPObject instances, they maybe should not be linked via an embedded
> CList, beause it means they cannot be shared, which is a bit unusual
> for these objects.
>
> When receiving these objects, you also commonly get them all in one go,
> like for the VLAN egress-qos-map. That is another reason why it doesn't
> make much sense to have them individual objects. You can just allocate
> one large array and put all the parsed structs inside.

That looks simpler. I'll follow the example of egress-qos-map for v2.

> > +     allowedip->family = tba[IFLA_WG_ALLOWEDIP_FAMILY] ?
> > nla_get_u16 (tba[IFLA_WG_ALLOWEDIP_FAMILY]) : 0;
> > +
> > +     addr_len = (allowedip->family == AF_INET)
>
> check that the family is at least AF_INET6, otherwise error out.

Will fix.

> > +static NMPObject *
> > +_parse_lnk_wireguard (const char *kind, const char *ifname)
> > +{
> > +     nm_auto_nmpobj NMPObject *obj = NULL;
> > +     NMPObject *obj_result = NULL;
> > +     struct nl_sock *sock;
> > +     nm_auto_nlmsg struct nl_msg *msg = NULL;
> > +     struct nl_cb cb = {
> > +             .valid_cb = _wireguard_parse_getdevice,
> > +     };
> > +     static int family_id;
> > +
> > +     if (!nm_streq0 (kind, "wireguard"))
> > +             goto err;
> > +
> > +     sock = nl_socket_alloc ();
> > +     if (!sock)
> > +             goto err;
> > +
> > +     if (nl_connect (sock, NETLINK_GENERIC))
>
> I think the genl socket should be cached and re-used by
> NMLinuxPlatform. Likewise, family_id should be cached in
> NMLinuxPlatformPrivate.

I'll look into it.

> > +             goto err_socket;
>
> It's a bit odd that _parse_lnk_wireguard() opens a genl socket to
> request additional data. Commonly the entire parsing of
> _new_from_nl_link() is only supposed to get information that is present
> in the netlink message. They only thing that they might do, is look
> into the platform cache, and re-use information from there, for example
> at
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nm-linux-platform.c?id=167e42a87e97ed7fb26a4263c22f1774716ac51b#n785
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nm-linux-platform.c?id=167e42a87e97ed7fb26a4263c22f1774716ac51b#n1833
>
> Basically, for every RTM_NEWLINK event, it re-fetches all wireguard
> information from genetlink. That seems a bit heavy. Do we get a
> notification on (ge)netlink when something about this interface
> changes?
>
> Otherwise, you could just look into the platform cache, see that the
> information is already, and re-use it from there.

Yeah it does feel like an abuse of _new_from_nl_link().
I don't yet know if there are any options to subscribe to link changes.
I'll keep digging.

> > +     if (!family_id)
> > +             family_id = genl_ctrl_resolve (sock, "wireguard");
> > +     if (family_id <= 0)
> > +             goto err_socket;
> > +
> > +     msg = nlmsg_alloc ();
> > +
> > +     if (!genlmsg_put (msg, NL_AUTO_PORT, NL_AUTO_SEQ, family_id,
> > +                       0, NLM_F_DUMP, WG_CMD_GET_DEVICE, 1))
> > +             goto err_socket;
> > +
> > +     if (nla_put_string (msg, IFLA_WG_DEVICE_IFNAME, ifname) < 0)
>
> can we avoid lookup by ifname? The ifname can be changed, only ifindex
> is the unchangable, unique ID (mostly, not really either :( )

Checking WireGuard code now - yes we can lookup by ifindex.
I'll make that change.

> > +const char *
> > +nm_platform_lnk_wireguard_to_string (const NMPlatformLnkWireguard
> > *lnk, char *buf, gsize len)
> > +{
> > +     gchar *public_b64;
> > +     if (!nm_utils_to_string_buffer_init_null (lnk, &buf, &len))
> > +             return buf;
> > +
> > +     public_b64 = g_base64_encode (lnk->public_key, sizeof (lnk-
> > > public_key));
> > +
> > +     g_snprintf (buf, len,
> > +                 "wireguard "
> > +                 "public_key %s "
> > +                 "private_key (hidden) "
> > +                 "listen_port %u "
> > +                 "fwmark 0x%x",
> > +                 public_b64,
> > +                 lnk->listen_port,
> > +                 lnk->fwmark);
> > +
> > +     g_free (public_b64);
>
> don't explicitly free. Use one of the gs_* or nm_auto_* macros.
> In this case, gs_free.

OK.

> Commonly, the to-string methods should print *all* aspects of the
> object. It's right to hide the private-key, I am talking about the
> Peers/AllowedIPs.
>
> The sole purpose of the to-string methods if for debugging. Currently,
> the peers/allowed-ips are never printed, maybe they should.
>
> Note also that nmp_object_to_string() has an arguemnt to_string_mode,
> for printing common fields (NMP_OBJECT_TO_STRING_PUBLIC) vs. all
> (NMP_OBJECT_TO_STRING_ALL).
> I think at least for NMP_OBJECT_TO_STRING_ALL, NMPObjectLnkWireguard
> should print all peers/allowed-ips. This means to implement
> "cmd_obj_to_string".
> See 
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/platform/nmp-object.c?id=167e42a87e97ed7fb26a4263c22f1774716ac51b#n666

Sure. What about:

NMP_OBJECT_TO_STRING_PUBLIC:
* Device: public key, port, fwmark, peers...
* Each peer: public key, endpoint, keepalive interval, allowed IPs..
* Each allowed IP: CIDR notation address

NMP_OBJECT_TO_STRING_ALL:
* All of the above plus every single field (including all stats).

> > +const char *
> > +nm_platform_wireguard_peer_to_string (const NMPlatformWireguardPeer
> > *peer, char *buf, gsize len)
> > +{
> > +     gchar *public_b64;
> > +     char s_endpoint[NI_MAXHOST + NI_MAXSERV + sizeof("endpoint
> > []:") + 1];
> > +
> > +     if (!nm_utils_to_string_buffer_init_null (peer, &buf, &len))
> > +             return buf;
> > +
> > +     if (peer->endpoint.addr.sa_family == AF_INET || peer-
> > > endpoint.addr.sa_family == AF_INET6) {
> > +             char host[NI_MAXHOST];
> > +             char service[NI_MAXSERV];
> > +             socklen_t addr_len = 0;
> > +
> > +             if (peer->endpoint.addr.sa_family == AF_INET)
> > +                     addr_len = sizeof (struct sockaddr_in);
> > +             else if (peer->endpoint.addr.sa_family == AF_INET6)
> > +                     addr_len = sizeof (struct sockaddr_in6);
> > +             if (!getnameinfo (&peer->endpoint.addr, addr_len,
> > host, sizeof(host), service, sizeof(service), NI_DGRAM |
> > NI_NUMERICSERV | NI_NUMERICHOST)) {
>
> I don't think we should resolve the addresses just for logging.
> Just convert them to string with nm_utils_inet_ntop().

I understood getnameinfo() with NI_NUMERIC* didn't do resolution.
Anyhow, I can use that utils function if you prefer.

> > +typedef struct {
> > +     guint8 private_key[NM_WG_PUBLIC_KEY_LEN];
> > +     guint8 public_key[NM_WG_PUBLIC_KEY_LEN];
> > +     guint16 listen_port;
> > +     guint32 fwmark;
> > +
> > +     CList peers_lst_head;
>
> The public part of the NMPObjects (NMPlatformLnkWireguard vs.
> NMPObjectLnkWireguard) is supposed to be copyable directly. That means,
> it cannot have pointers (except for example NMPlatformLink.kind, which
> is however a static string, so it's safe to copy).
>
> This means, the CList part must go into NMPObjectLnkWireguard.
>
>
> Together with I said above about nm_platform_lnk_wireguard_to_string()
> to print all fields, it means, the list of peers/allowed-ips is not
> accessible to NMPlatformLnkWireguard, so there is nothing to print.
> But it certainly should implement cmd_obj_to_string().

Got it - I'll go the NMPObjectLnkVlan route instead as you suggest.

> > +     [NMP_OBJECT_TYPE_LNK_WIREGUARD - 1] = {
> > +             .parent                             =
> > DEDUP_MULTI_OBJ_CLASS_INIT(),
> > +             .obj_type                           =
> > NMP_OBJECT_TYPE_LNK_WIREGUARD,
> > +             .sizeof_data                        = sizeof
> > (NMPObjectLnkWireguard),
> > +             .sizeof_public                      = sizeof
> > (NMPlatformLnkWireguard),
> > +             .obj_type_name                      = "wireguard",
> > +             .lnk_link_type                      =
> > NM_LINK_TYPE_WIREGUARD,
> > +             .cmd_plobj_to_string                = (const char
> > *(*) (const NMPlatformObject *obj, char *buf, gsize len))
>
> the types that are put inside the platform cache (like
> NMPObjectLnkWireguard), they must implement equality/hash operators.
>
> NMPObjectLnkWireguard is not unlike NMPObjectLnkVlan. See what
> functions are implemented there.

Will do.

(Actually had these functions in before, following the macsec example,
but then I thought I'd just make the patch simpler until I was sure what
the wireguard data structures would look like.)

On Wed, Mar 14, 2018 at 5:20 PM, Thomas Haller <thaller redhat com> wrote:
[...]
> > +static NMPObject *
> > +_parse_lnk_wireguard (const char *kind, const char *ifname)
> > +{
> > +     nm_auto_nmpobj NMPObject *obj = NULL;
> > +     NMPObject *obj_result = NULL;
> > +     struct nl_sock *sock;
> > +     nm_auto_nlmsg struct nl_msg *msg = NULL;
> > +     struct nl_cb cb = {
> > +             .valid_cb = _wireguard_parse_getdevice,
> > +     };
> > +     static int family_id;
>
> Hi,
>
> all the "parsing" done here is basically requesting/querying everyhing
> via genl.
>
> It would be really nice, to only request new data when necessary, and
> otherwise keep the current from the cache.
>
> But how to notice changes?
>
> Maybe the wireguard netlink API should expose a version id both on rtnl
> and genl. Then, if we receive a new (different) version id on rtnl, we
> know we need to refetch everything.
>
> Or does the wireguard API already provide some functionality about
> this? Should we instead register to signals on genl? That would be
> doable, but cumbersome.

Not sure, I'd wait for Jason's answer on this one. I can see a
device_update_gen counter within struct wireguard_device, but I don't
think that is meant to be reported via rtnl.

Again, thanks for your time!