From b680d64b47b979f9984aafa18599247a70cff8d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 28 Feb 2019 16:55:26 +0100 Subject: [PATCH 01/14] wireguard: accept all-zero private-key, public-key and preshared-key - For PSK, an all-zero PSK means to don't do symmetric encryption. As such, at first it seems a bit odd when the user sets - preshared-key-flags != "4 (not-required)" - preshared-key = AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= Here the user indicates that a PSK is required, but then provides an all-zero PSK that effectively disables it. Still, we should not reject such a configuration. This has the benefit that it allos the user for being prompted for a PSK, only to disable it by entering the all-zero key. - For the private-key (and consequently the public-key), "public-key-flags=4" is rejected by libnm. A private key is always required for NetworkManager to configure the link. However, let's not care for all-zero keys either. If the user configures that, we just set that key. It's a valid setting as far as WireGuard (the kernel module) is concerned, so we shouldn't reject it. (cherry picked from commit 78dccb8bb9123142773e824ea4cb1a1341b0992f) --- libnm-core/nm-utils.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 3e685abd0e..46dd1eba13 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -6707,11 +6707,6 @@ _nm_utils_wireguard_decode_key (const char *base64_key, return FALSE; } - if (nm_utils_memeqzero (bin_arr, required_key_len)) { - /* an all zero key is not valid either. That is used to represet an unset key */ - return FALSE; - } - if (out_key) memcpy (out_key, bin_arr, required_key_len); From 92b27a4f88cbfc62d23914f05d8044abf878cfce Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Thu, 28 Feb 2019 16:23:26 +0100 Subject: [PATCH 02/14] shared: add nm_utils_memeqzero_secret() [thaller@redhat.com: the code is effectively key_is_zero() by (LGPL2.1+). I took it into our source tree and adjusted it to our style] (cherry picked from commit 6234e41153150cee4742360af1f5009c03cf2f77) --- shared/nm-utils/nm-secret-utils.c | 27 +++++++++++++++++++++++++++ shared/nm-utils/nm-secret-utils.h | 2 ++ 2 files changed, 29 insertions(+) diff --git a/shared/nm-utils/nm-secret-utils.c b/shared/nm-utils/nm-secret-utils.c index 65f99c65d9..ec5cc6b1b3 100644 --- a/shared/nm-utils/nm-secret-utils.c +++ b/shared/nm-utils/nm-secret-utils.c @@ -17,6 +17,7 @@ * Boston, MA 02110-1301 USA. * * (C) Copyright 2018 Red Hat, Inc. + * (C) Copyright 2015 - 2019 Jason A. Donenfeld . All Rights Reserved. */ #include "nm-default.h" @@ -132,3 +133,29 @@ nm_secret_buf_to_gbytes_take (NMSecretBuf *secret, gssize actual_len) _secret_buf_free, secret); } + +/*****************************************************************************/ + +/** + * nm_utils_memeqzero_secret: + * @data: the data pointer to check (may be %NULL if @length is zero). + * @length: the number of bytes to check. + * + * Checks that all bytes are zero. This always takes the same amount + * of time to prevent timing attacks. + * + * Returns: whether all bytes are zero. + */ +gboolean +nm_utils_memeqzero_secret (gconstpointer data, gsize length) +{ + const guint8 *const key = data; + volatile guint8 acc = 0; + gsize i; + + for (i = 0; i < length; i++) { + acc |= key[i]; + asm volatile("" : "=r"(acc) : "0"(acc)); + } + return 1 & ((acc - 1) >> 8); +} diff --git a/shared/nm-utils/nm-secret-utils.h b/shared/nm-utils/nm-secret-utils.h index 1bd518704e..034ef7bd33 100644 --- a/shared/nm-utils/nm-secret-utils.h +++ b/shared/nm-utils/nm-secret-utils.h @@ -173,4 +173,6 @@ GBytes *nm_secret_buf_to_gbytes_take (NMSecretBuf *secret, gssize actual_len); /*****************************************************************************/ +gboolean nm_utils_memeqzero_secret (gconstpointer data, gsize length); + #endif /* __NM_SECRET_UTILS_H__ */ From 028d2575373bc4737595fd0570cd1b2c409549e2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 28 Feb 2019 16:26:12 +0100 Subject: [PATCH 03/14] core: use nm_utils_memeqzero_secret() for printing WireGuard key (cherry picked from commit 7451a6a6490d178bd68cd6639c33a22469c6a983) --- src/platform/nm-platform.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 5d998f4475..fe0cb662a2 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -39,6 +39,7 @@ #include "nm-core-internal.h" #include "nm-utils/nm-dedup-multi.h" #include "nm-utils/nm-udev-utils.h" +#include "nm-utils/nm-secret-utils.h" #include "nm-core-utils.h" #include "nm-platform-utils.h" @@ -5658,7 +5659,7 @@ nm_platform_wireguard_peer_to_string (const NMPWireGuardPeer *peer, char *buf, g "%s" /* persistent-keepalive */ "%s", /* allowed-ips */ public_key_b64, - nm_utils_memeqzero (peer->preshared_key, sizeof (peer->preshared_key)) + nm_utils_memeqzero_secret (peer->preshared_key, sizeof (peer->preshared_key)) ? "" : " preshared-key (hidden)", s_endpoint, @@ -5704,7 +5705,7 @@ nm_platform_lnk_wireguard_to_string (const NMPlatformLnkWireGuard *lnk, char *bu ? " public-key " : "", public_b64 ?: "", - nm_utils_memeqzero (lnk->private_key, sizeof (lnk->private_key)) + nm_utils_memeqzero_secret (lnk->private_key, sizeof (lnk->private_key)) ? "" : " private-key (hidden)", lnk->listen_port, From a695acfd289a3d1af2782e8b3c37c760e8434008 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 09:32:45 +0100 Subject: [PATCH 04/14] platform/wireguard: fix WGPEER_A_LAST_HANDSHAKE_TIME to use int64 typed timespec structure The netlink API changed for WireGuard. Adjust for that. https://git.zx2c4.com/WireGuard/commit/?id=c870c7af53f44a37814dfc76ceb8ad88e290fcd8 (cherry picked from commit 4e399d82ac850ef38252f0b2ce9bc646da174ca6) --- src/platform/nm-linux-platform.c | 6 ++++-- src/platform/nmp-object.h | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 4bf8555802..2f5c75b055 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -2019,8 +2019,10 @@ _wireguard_update_from_peers_nla (CList *peers, if (tb[WGPEER_A_PERSISTENT_KEEPALIVE_INTERVAL]) peer_c->data.persistent_keepalive_interval = nla_get_u16 (tb[WGPEER_A_PERSISTENT_KEEPALIVE_INTERVAL]); - if (tb[WGPEER_A_LAST_HANDSHAKE_TIME]) - nla_memcpy (&peer_c->data.last_handshake_time, tb[WGPEER_A_LAST_HANDSHAKE_TIME], sizeof (peer_c->data.last_handshake_time)); + if (tb[WGPEER_A_LAST_HANDSHAKE_TIME]) { + if (nla_len (tb[WGPEER_A_LAST_HANDSHAKE_TIME]) >= sizeof (peer_c->data.last_handshake_time)) + nla_memcpy (&peer_c->data.last_handshake_time, tb[WGPEER_A_LAST_HANDSHAKE_TIME], sizeof (peer_c->data.last_handshake_time)); + } if (tb[WGPEER_A_RX_BYTES]) peer_c->data.rx_bytes = nla_get_u64 (tb[WGPEER_A_RX_BYTES]); if (tb[WGPEER_A_TX_BYTES]) diff --git a/src/platform/nmp-object.h b/src/platform/nmp-object.h index 32775efa83..525e144001 100644 --- a/src/platform/nmp-object.h +++ b/src/platform/nmp-object.h @@ -31,6 +31,18 @@ struct udev_device; /*****************************************************************************/ +/* "struct __kernel_timespec" uses "long long", but we use gint64. In practice, + * these are the same types. */ +G_STATIC_ASSERT (sizeof (long long) == sizeof (gint64)); + +typedef struct { + /* like "struct __kernel_timespec". */ + gint64 tv_sec; + gint64 tv_nsec; +} NMPTimespec64; + +/*****************************************************************************/ + typedef union { struct sockaddr sa; struct sockaddr_in in; @@ -72,7 +84,8 @@ typedef struct { typedef struct _NMPWireGuardPeer { NMSockAddrUnion endpoint; - struct timespec last_handshake_time; + NMPTimespec64 last_handshake_time; + guint64 rx_bytes; guint64 tx_bytes; From c5a247c4c081836f5995674a20a55f3266cabd2b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 08:55:30 +0100 Subject: [PATCH 05/14] cli: add nmc_complete_strv() which takes a string array for completion that may contain NULL This will allow for a convenient calling pattern when some elements should be printed optionally. (cherry picked from commit 62b939de4e2288d0a9d305706db4cf47c34122d5) --- clients/cli/common.c | 34 +++++++++++++++++++++++++--------- clients/cli/common.h | 4 +++- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/clients/cli/common.c b/clients/cli/common.c index 50dd0eb5cb..3c1c315d5e 100644 --- a/clients/cli/common.c +++ b/clients/cli/common.c @@ -1349,23 +1349,39 @@ nmc_do_cmd (NmCli *nmc, const NMCCommand cmds[], const char *cmd, int argc, char /** * nmc_complete_strings: * @prefix: a string to match - * @...: a %NULL-terminated list of candidate strings + * @nargs: the number of elements in @args. Or -1 if @args is a NULL terminated + * strv array. + * @args: the argument list. If @nargs is not -1, then some elements may + * be %NULL to indicate to silently skip the values. * * Prints all the matching candidates for completion. Useful when there's * no better way to suggest completion other than a hardcoded string list. */ void -nmc_complete_strings (const char *prefix, ...) +nmc_complete_strv (const char *prefix, gssize nargs, const char *const*args) { - va_list args; - const char *candidate; + gsize i, n; - va_start (args, prefix); - while ((candidate = va_arg (args, const char *))) { - if (!*prefix || matches (prefix, candidate)) - g_print ("%s\n", candidate); + if (prefix && !prefix[0]) + prefix = NULL; + + if (nargs < 0) { + nm_assert (nargs == -1); + n = NM_PTRARRAY_LEN (args); + } else + n = (gsize) nargs; + + for (i = 0; i < n; i++) { + const char *candidate = args[i]; + + if (!candidate) + continue; + if ( prefix + && !matches (prefix, candidate)) + continue; + + g_print ("%s\n", candidate); } - va_end (args); } /** diff --git a/clients/cli/common.h b/clients/cli/common.h index 71734acc98..688f2819f6 100644 --- a/clients/cli/common.h +++ b/clients/cli/common.h @@ -88,7 +88,9 @@ typedef struct { void nmc_do_cmd (NmCli *nmc, const NMCCommand cmds[], const char *cmd, int argc, char **argv); -void nmc_complete_strings (const char *prefix, ...) G_GNUC_NULL_TERMINATED; +void nmc_complete_strv (const char *prefix, gssize nargs, const char *const*args); + +#define nmc_complete_strings(prefix, ...) nmc_complete_strv ((prefix), NM_NARG (__VA_ARGS__), (const char *const[]) { __VA_ARGS__ }) void nmc_complete_bool (const char *prefix); From 4a137f919b452255eef985ef1eee52c277599dd0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 08:45:02 +0100 Subject: [PATCH 06/14] cli: fix completion for `nmcli connection import` If we already specified "type" or "file", don't offer it for completion again. $ nmcli connection import type openvpn file type (cherry picked from commit fea0f4a5eaa069b9019151ca9a43b6ee25a7f837) --- clients/cli/connections.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index 63cb0bb90c..856848f8fa 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -8888,8 +8888,11 @@ do_connection_import (NmCli *nmc, int argc, char **argv) } while (argc > 0) { - if (argc == 1 && nmc->complete) - nmc_complete_strings (*argv, "type", "file", NULL); + if (argc == 1 && nmc->complete) { + nmc_complete_strings (*argv, + type ? NULL : "type", + filename ? NULL : "file"); + } if (strcmp (*argv, "type") == 0) { argc--; From 76828262299729e718924d5c14a69cfdc5cf48fa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 15:52:19 +0100 Subject: [PATCH 07/14] libnm: change nm_wireguard_peer_set_preshared_key() API to allow validation This is an API break since 1.16-rc1. The functions like _nm_utils_wireguard_decode_key() are internal API and not accessible to a libnm user. Maybe this should be public API, but for now it is not. That makes it cumbersome for a client to validate the setting. The client could only reimplement the validation (bad) or go ahead and set invalid value. When setting an invalid value, the user can afterwards detect it via nm_wireguard_peer_is_valid(), but at that point, it's not clear which exact property is invalid. First I wanted to keep the API conservative and not promissing too much. For example, not promising to do any validation when setting the key. However, libnm indeed validates the key at the time of setting it instead of doing lazy validation later. This makes sense, so we can keep this promise and just expose the validation result to the caller. Another downside of this is that the API just got more complicated. But it not provides a validation API, that we previously did not have. (cherry picked from commit d7bc1750c1bcc5082528d1f277e09454b2cbf1c2) --- examples/python/gi/nm-wg-set | 4 ++-- libnm-core/nm-keyfile.c | 5 ++--- libnm-core/nm-setting-wireguard.c | 36 ++++++++++++++++++++++--------- libnm-core/nm-setting-wireguard.h | 5 +++-- libnm-core/tests/test-setting.c | 6 ++++-- 5 files changed, 37 insertions(+), 19 deletions(-) diff --git a/examples/python/gi/nm-wg-set b/examples/python/gi/nm-wg-set index fc60f069e6..308e4c74ca 100755 --- a/examples/python/gi/nm-wg-set +++ b/examples/python/gi/nm-wg-set @@ -355,11 +355,11 @@ def do_set(nm_client, conn, argv): if peer and argv[idx] == 'preshared-key': psk = argv_get_one(argv, idx + 1, None, idx) if psk == '': - peer.set_preshared_key(None) + peer.set_preshared_key(None, True) if peer_secret_flags is not None: peer_secret_flags = NM.SettingSecretFlags.NOT_REQUIRED else: - peer.set_preshared_key(wg_read_private_key(psk)) + peer.set_preshared_key(wg_read_private_key(psk), True) if peer_secret_flags is not None: peer_secret_flags = NM.SettingSecretFlags.NONE idx += 2 diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index d756a17733..b82b6bf020 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -2935,13 +2935,12 @@ _read_setting_wireguard_peer (KeyfileReaderInfo *info) key = NM_WIREGUARD_PEER_ATTR_PRESHARED_KEY; str = nm_keyfile_plugin_kf_get_string (info->keyfile, info->group, key, NULL); if (str) { - if (!_nm_utils_wireguard_decode_key (str, NM_WIREGUARD_SYMMETRIC_KEY_LEN, NULL)) { + if (!nm_wireguard_peer_set_preshared_key (peer, str, FALSE)) { if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("key '%s.%s' is not not a valid 256 bit key in base64 encoding"), info->group, key)) return; - } else - nm_wireguard_peer_set_preshared_key (peer, str); + } nm_clear_g_free (&str); } diff --git a/libnm-core/nm-setting-wireguard.c b/libnm-core/nm-setting-wireguard.c index ad01d4eaff..c1881240a0 100644 --- a/libnm-core/nm-setting-wireguard.c +++ b/libnm-core/nm-setting-wireguard.c @@ -346,6 +346,9 @@ nm_wireguard_peer_get_preshared_key (const NMWireGuardPeer *self) * @self: the unsealed #NMWireGuardPeer instance * @preshared_key: (allow-none) (transfer none): the new preshared * key or %NULL to clear the preshared key. + * @accept_invalid: whether to allow setting the key to an invalid + * value. If %FALSE, @self is unchanged if the key is invalid + * and if %FALSE is returned. * * Reset the preshared key. Note that if the preshared key is valid, it * will be normalized (which may or may not modify the set value). @@ -358,28 +361,41 @@ nm_wireguard_peer_get_preshared_key (const NMWireGuardPeer *self) * * It is a bug trying to modify a sealed #NMWireGuardPeer instance. * + * Returns: %TRUE if the preshared-key is valid, otherwise %FALSE. + * %NULL is considered a valid value. + * If the key is invalid, it depends on @accept_invalid whether the + * previous value was reset. + * * Since: 1.16 */ -void +gboolean nm_wireguard_peer_set_preshared_key (NMWireGuardPeer *self, - const char *preshared_key) + const char *preshared_key, + gboolean accept_invalid) { char *preshared_key_normalized = NULL; + gboolean is_valid; - g_return_if_fail (NM_IS_WIREGUARD_PEER (self, FALSE)); + g_return_val_if_fail (NM_IS_WIREGUARD_PEER (self, FALSE), FALSE); if (!preshared_key) { nm_clear_pointer (&self->preshared_key, nm_free_secret); - return; + return TRUE; } - self->preshared_key_valid = _nm_utils_wireguard_normalize_key (preshared_key, - NM_WIREGUARD_SYMMETRIC_KEY_LEN, - &preshared_key_normalized); - nm_assert (self->preshared_key_valid == (preshared_key_normalized != NULL)); + is_valid = _nm_utils_wireguard_normalize_key (preshared_key, + NM_WIREGUARD_SYMMETRIC_KEY_LEN, + &preshared_key_normalized); + nm_assert (is_valid == (preshared_key_normalized != NULL)); + if ( !is_valid + && !accept_invalid) + return FALSE; + + self->preshared_key_valid = is_valid; nm_free_secret (self->preshared_key); self->preshared_key = preshared_key_normalized ?: g_strdup (preshared_key); + return is_valid; } /** @@ -1543,7 +1559,7 @@ _peers_dbus_only_set (NMSetting *setting, } if (g_variant_lookup (peer_var, NM_WIREGUARD_PEER_ATTR_PRESHARED_KEY, "&s", &cstr)) - nm_wireguard_peer_set_preshared_key (peer, cstr); + nm_wireguard_peer_set_preshared_key (peer, cstr, TRUE); if (g_variant_lookup (peer_var, NM_WIREGUARD_PEER_ATTR_PRESHARED_KEY_FLAGS, "u", &u32)) nm_wireguard_peer_set_preshared_key_flags (peer, u32); @@ -1873,7 +1889,7 @@ update_one_secret (NMSetting *setting, continue; peer = nm_wireguard_peer_new_clone (pd->peer, FALSE); - nm_wireguard_peer_set_preshared_key (peer, cstr); + nm_wireguard_peer_set_preshared_key (peer, cstr, TRUE); if (!_peers_set (priv, peer, pd->idx, FALSE)) nm_assert_not_reached (); diff --git a/libnm-core/nm-setting-wireguard.h b/libnm-core/nm-setting-wireguard.h index 17fb4664c3..6f6fc0f0b4 100644 --- a/libnm-core/nm-setting-wireguard.h +++ b/libnm-core/nm-setting-wireguard.h @@ -67,8 +67,9 @@ void nm_wireguard_peer_set_public_key (NMWireGuardPeer *self, NM_AVAILABLE_IN_1_16 const char *nm_wireguard_peer_get_preshared_key (const NMWireGuardPeer *self); NM_AVAILABLE_IN_1_16 -void nm_wireguard_peer_set_preshared_key (NMWireGuardPeer *self, - const char *preshared_key); +gboolean nm_wireguard_peer_set_preshared_key (NMWireGuardPeer *self, + const char *preshared_key, + gboolean accept_invalid); NM_AVAILABLE_IN_1_16 NMSettingSecretFlags nm_wireguard_peer_get_preshared_key_flags (const NMWireGuardPeer *self); diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index e59bc4668c..6d089f1ab8 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2067,7 +2067,8 @@ _rndt_wg_peers_create (void) peer = nm_wireguard_peer_new (); nm_wireguard_peer_set_public_key (peer, public_key); - nm_wireguard_peer_set_preshared_key (peer, nmtst_rand_select (NULL, preshared_key)); + if (!nm_wireguard_peer_set_preshared_key (peer, nmtst_rand_select (NULL, preshared_key), TRUE)) + g_assert_not_reached (); nm_wireguard_peer_set_preshared_key_flags (peer, nmtst_rand_select (NM_SETTING_SECRET_FLAG_NONE, NM_SETTING_SECRET_FLAG_NOT_SAVED, @@ -2245,7 +2246,8 @@ _rndt_wg_peers_fix_secrets (NMSettingWireGuard *s_wg, g_assert (NM_IN_SET (nm_wireguard_peer_get_preshared_key_flags (a), NM_SETTING_SECRET_FLAG_AGENT_OWNED, NM_SETTING_SECRET_FLAG_NOT_SAVED)); b_clone = nm_wireguard_peer_new_clone (b, TRUE); - nm_wireguard_peer_set_preshared_key (b_clone, nm_wireguard_peer_get_preshared_key (a)); + if (!nm_wireguard_peer_set_preshared_key (b_clone, nm_wireguard_peer_get_preshared_key (a), TRUE)) + g_assert_not_reached (); nm_setting_wireguard_set_peer (s_wg, b_clone, i); b = nm_setting_wireguard_get_peer (s_wg, i); g_assert (b == b_clone); From 6452d2d0e54cbb4a4acb380d5f8a4a76326bb700 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 15:52:19 +0100 Subject: [PATCH 08/14] libnm: change nm_wireguard_peer_set_public_key() API to allow validation This is an API break since 1.16-rc1. Similar to previous commit. (cherry picked from commit 7962653918fbfd66b549d389a1cb2cf96ae0d3eb) --- examples/python/gi/nm-wg-set | 2 +- libnm-core/nm-keyfile.c | 2 +- libnm-core/nm-setting-wireguard.c | 33 +++++++++++++++++++++---------- libnm-core/nm-setting-wireguard.h | 5 +++-- libnm-core/tests/test-setting.c | 3 ++- 5 files changed, 30 insertions(+), 15 deletions(-) diff --git a/examples/python/gi/nm-wg-set b/examples/python/gi/nm-wg-set index 308e4c74ca..9ee61c7dc4 100755 --- a/examples/python/gi/nm-wg-set +++ b/examples/python/gi/nm-wg-set @@ -343,7 +343,7 @@ def do_set(nm_client, conn, argv): else: peer_idx = None peer = NM.WireGuardPeer() - peer.set_public_key(public_key) + peer.set_public_key(public_key, True) wg_peer_is_valid(peer, 'public key "%s" is invalid' % (public_key)) peer_remove = False idx += 2 diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index b82b6bf020..4f5ec46260 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -2929,7 +2929,7 @@ _read_setting_wireguard_peer (KeyfileReaderInfo *info) info->group); return; } - nm_wireguard_peer_set_public_key (peer, cstr); + nm_wireguard_peer_set_public_key (peer, cstr, TRUE); nm_clear_g_free (&str); key = NM_WIREGUARD_PEER_ATTR_PRESHARED_KEY; diff --git a/libnm-core/nm-setting-wireguard.c b/libnm-core/nm-setting-wireguard.c index c1881240a0..154cc8a798 100644 --- a/libnm-core/nm-setting-wireguard.c +++ b/libnm-core/nm-setting-wireguard.c @@ -280,34 +280,48 @@ nm_wireguard_peer_get_public_key (const NMWireGuardPeer *self) * @self: the unsealed #NMWireGuardPeer instance * @public_key: (allow-none) (transfer none): the new public * key or %NULL to clear the public key. + * @accept_invalid: if %TRUE and @public_key is not %NULL and + * invalid, then do not modify the instance. * * Reset the public key. Note that if the public key is valid, it * will be normalized (which may or may not modify the set value). * * It is a bug trying to modify a sealed #NMWireGuardPeer instance. * + * Returns: %TRUE if the key was valid or %NULL. Returns + * %FALSE for invalid keys. Depending on @accept_invalid + * will an invalid key be set or not. + * * Since: 1.16 */ -void +gboolean nm_wireguard_peer_set_public_key (NMWireGuardPeer *self, - const char *public_key) + const char *public_key, + gboolean accept_invalid) { char *public_key_normalized = NULL; + gboolean is_valid; - g_return_if_fail (NM_IS_WIREGUARD_PEER (self, FALSE)); + g_return_val_if_fail (NM_IS_WIREGUARD_PEER (self, FALSE), FALSE); if (!public_key) { nm_clear_g_free (&self->public_key); - return; + return TRUE; } - self->public_key_valid = _nm_utils_wireguard_normalize_key (public_key, - NM_WIREGUARD_PUBLIC_KEY_LEN, - &public_key_normalized); - nm_assert (self->public_key_valid == (public_key_normalized != NULL)); + is_valid = _nm_utils_wireguard_normalize_key (public_key, + NM_WIREGUARD_PUBLIC_KEY_LEN, + &public_key_normalized); + nm_assert (is_valid == (public_key_normalized != NULL)); + if ( !is_valid + && !accept_invalid) + return FALSE; + + self->public_key_valid = is_valid; g_free (self->public_key); self->public_key = public_key_normalized ?: g_strdup (public_key); + return is_valid; } void @@ -1532,8 +1546,7 @@ _peers_dbus_only_set (NMSetting *setting, } peer = nm_wireguard_peer_new (); - nm_wireguard_peer_set_public_key (peer, cstr); - if (!peer->public_key_valid) { + if (!nm_wireguard_peer_set_public_key (peer, cstr, TRUE)) { if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_STRICT)) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_MISSING_PROPERTY, _("peer #%u has invalid public-key"), diff --git a/libnm-core/nm-setting-wireguard.h b/libnm-core/nm-setting-wireguard.h index 6f6fc0f0b4..5cf5c1f3c4 100644 --- a/libnm-core/nm-setting-wireguard.h +++ b/libnm-core/nm-setting-wireguard.h @@ -61,8 +61,9 @@ gboolean nm_wireguard_peer_is_sealed (const NMWireGuardPeer *self); NM_AVAILABLE_IN_1_16 const char *nm_wireguard_peer_get_public_key (const NMWireGuardPeer *self); NM_AVAILABLE_IN_1_16 -void nm_wireguard_peer_set_public_key (NMWireGuardPeer *self, - const char *public_key); +gboolean nm_wireguard_peer_set_public_key (NMWireGuardPeer *self, + const char *public_key, + gboolean accept_invalid); NM_AVAILABLE_IN_1_16 const char *nm_wireguard_peer_get_preshared_key (const NMWireGuardPeer *self); diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index 6d089f1ab8..bbcff8778f 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2065,7 +2065,8 @@ _rndt_wg_peers_create (void) s_endpoint = _create_random_ipaddr (AF_UNSPEC, TRUE); peer = nm_wireguard_peer_new (); - nm_wireguard_peer_set_public_key (peer, public_key); + if (!nm_wireguard_peer_set_public_key (peer, public_key, TRUE)) + g_assert_not_reached (); if (!nm_wireguard_peer_set_preshared_key (peer, nmtst_rand_select (NULL, preshared_key), TRUE)) g_assert_not_reached (); From f617d5e8b4c421f0812d4fc9bf6780cc51a74d90 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 15:52:19 +0100 Subject: [PATCH 09/14] libnm: change nm_wireguard_peer_set_endpoint() API to allow validation This is an API break since 1.16-rc1. Similar to previous commit. (cherry picked from commit 8ae9aa2428c10aafb837c692fb39bfd04ef4235c) --- examples/python/gi/nm-wg-set | 2 +- libnm-core/nm-keyfile.c | 8 ++----- libnm-core/nm-setting-wireguard.c | 36 +++++++++++++++++++++++++------ libnm-core/nm-setting-wireguard.h | 5 +++-- libnm-core/tests/test-setting.c | 3 ++- 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/examples/python/gi/nm-wg-set b/examples/python/gi/nm-wg-set index 9ee61c7dc4..32097f83f3 100755 --- a/examples/python/gi/nm-wg-set +++ b/examples/python/gi/nm-wg-set @@ -369,7 +369,7 @@ def do_set(nm_client, conn, argv): idx += 2 continue if peer and argv[idx] == 'endpoint': - peer.set_endpoint(argv_get_one(argv, idx + 1, None, idx)) + peer.set_endpoint(argv_get_one(argv, idx + 1, None, idx), True) idx += 2 continue if peer and argv[idx] == 'persistent-keepalive': diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index 4f5ec46260..3633281a3d 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -2972,16 +2972,12 @@ _read_setting_wireguard_peer (KeyfileReaderInfo *info) key = NM_WIREGUARD_PEER_ATTR_ENDPOINT; str = nm_keyfile_plugin_kf_get_string (info->keyfile, info->group, key, NULL); if (str && str[0]) { - nm_auto_unref_sockaddrendpoint NMSockAddrEndpoint *ep = NULL; - - ep = nm_sock_addr_endpoint_new (str); - if (!nm_sock_addr_endpoint_get_host (ep)) { + if (!nm_wireguard_peer_set_endpoint (peer, str, FALSE)) { if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, _("key '%s.%s' is not not a valid endpoint"), info->group, key)) return; - } else - _nm_wireguard_peer_set_endpoint (peer, ep); + } } nm_clear_g_free (&str); diff --git a/libnm-core/nm-setting-wireguard.c b/libnm-core/nm-setting-wireguard.c index 154cc8a798..6785bf4a4b 100644 --- a/libnm-core/nm-setting-wireguard.c +++ b/libnm-core/nm-setting-wireguard.c @@ -524,26 +524,50 @@ _nm_wireguard_peer_set_endpoint (NMWireGuardPeer *self, * nm_wireguard_peer_set_endpoint: * @self: the unsealed #NMWireGuardPeer instance * @endpoint: the socket address endpoint to set or %NULL. + * @allow_invalid: if %TRUE, also invalid values are set. + * If %FALSE, the function does nothing for invalid @endpoint + * arguments. * * Sets or clears the endpoint of @self. * * It is a bug trying to modify a sealed #NMWireGuardPeer instance. * + * Returns: %TRUE if the endpoint is %NULL or valid. For an + * invalid @endpoint argument, %FALSE is returned. Depending + * on @allow_invalid, the instance will be modified. + * * Since: 1.16 */ -void +gboolean nm_wireguard_peer_set_endpoint (NMWireGuardPeer *self, - const char *endpoint) + const char *endpoint, + gboolean allow_invalid) { NMSockAddrEndpoint *old; + NMSockAddrEndpoint *new; + gboolean is_valid; - g_return_if_fail (NM_IS_WIREGUARD_PEER (self, FALSE)); + g_return_val_if_fail (NM_IS_WIREGUARD_PEER (self, FALSE), FALSE); + + if (!endpoint) { + nm_clear_pointer (&self->endpoint, nm_sock_addr_endpoint_unref); + return TRUE; + } + + new = nm_sock_addr_endpoint_new (endpoint); + + is_valid = (nm_sock_addr_endpoint_get_host (new) != NULL); + + if ( !allow_invalid + && !is_valid) { + nm_sock_addr_endpoint_unref (new); + return FALSE; + } old = self->endpoint; - self->endpoint = endpoint - ? nm_sock_addr_endpoint_new (endpoint) - : NULL; + self->endpoint = new; nm_sock_addr_endpoint_unref (old); + return is_valid; } /** diff --git a/libnm-core/nm-setting-wireguard.h b/libnm-core/nm-setting-wireguard.h index 5cf5c1f3c4..017eb1f6d3 100644 --- a/libnm-core/nm-setting-wireguard.h +++ b/libnm-core/nm-setting-wireguard.h @@ -87,8 +87,9 @@ void nm_wireguard_peer_set_persistent_keepalive (NMWireGuardPeer *self, NM_AVAILABLE_IN_1_16 const char *nm_wireguard_peer_get_endpoint (const NMWireGuardPeer *self); NM_AVAILABLE_IN_1_16 -void nm_wireguard_peer_set_endpoint (NMWireGuardPeer *self, - const char *endpoint); +gboolean nm_wireguard_peer_set_endpoint (NMWireGuardPeer *self, + const char *endpoint, + gboolean allow_invalid); NM_AVAILABLE_IN_1_16 guint nm_wireguard_peer_get_allowed_ips_len (const NMWireGuardPeer *self); diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index bbcff8778f..2011273a2b 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2078,7 +2078,8 @@ _rndt_wg_peers_create (void) nm_wireguard_peer_set_persistent_keepalive (peer, nmtst_rand_select ((guint32) 0, nmtst_get_rand_int ())); - nm_wireguard_peer_set_endpoint (peer, nmtst_rand_select (s_endpoint, NULL)); + if (!nm_wireguard_peer_set_endpoint (peer, nmtst_rand_select (s_endpoint, NULL), TRUE)) + g_assert_not_reached (); n_aip = nmtst_rand_select (0, nmtst_get_rand_int () % 10); for (i_aip = 0; i_aip < n_aip; i_aip++) { From 506a59b62e442f31857d954bca12f76202d0640c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 2 Mar 2019 15:38:01 +0100 Subject: [PATCH 10/14] libnm: fix return value for nm_wireguard_peer_append_allowed_ip() According to documentation, this returns a boolean indicating whether the value is valid. Previously, it was indicating whether the instance was modified. Together with the @accept_invalid argument, both behaviors make some sense. Change it, because that is also how the other setters behave. (cherry picked from commit f3ac8c6fe83a18c99fbbd32100924402a360e16e) --- libnm-core/nm-setting-wireguard.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libnm-core/nm-setting-wireguard.c b/libnm-core/nm-setting-wireguard.c index 6785bf4a4b..7629bfef4e 100644 --- a/libnm-core/nm-setting-wireguard.c +++ b/libnm-core/nm-setting-wireguard.c @@ -647,6 +647,7 @@ _peer_append_allowed_ip (NMWireGuardPeer *self, int prefix; NMIPAddr addrbin; char *str; + gboolean is_valid = TRUE; nm_assert (NM_IS_WIREGUARD_PEER (self, FALSE)); nm_assert (allowed_ip); @@ -662,6 +663,7 @@ _peer_append_allowed_ip (NMWireGuardPeer *self, return FALSE; /* mark the entry as invalid by having a "X" prefix. */ str = g_strconcat (ALLOWED_IP_INVALID_X_STR, allowed_ip, NULL); + is_valid = FALSE; } else { char addrstr[NM_UTILS_INET_ADDRSTRLEN]; @@ -679,7 +681,7 @@ _peer_append_allowed_ip (NMWireGuardPeer *self, self->allowed_ips = g_ptr_array_new_with_free_func (g_free); g_ptr_array_add (self->allowed_ips, str); - return TRUE; + return is_valid; } /** From 0d178a968d7cbc4360a3c5274d21f7caa47a86d3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 2 Mar 2019 17:10:25 +0100 Subject: [PATCH 11/14] libnm: rename and expose nm_utils_base64secret_decode() in libnm A NetworkManager client requires an API to validate and decode a base64 secret -- like it is used by WireGuard. If we don't have this as part of the API, it's inconvenient. Expose it. Rename it from _nm_utils_wireguard_decode_key(), to give it a more general name. Also, rename _nm_utils_wireguard_normalize_key() to nm_utils_base64secret_normalize(). But this one we keep as internal API. The user will care more about validating and decoding the base64 key. To convert the key back to base64, we don't need a public API in libnm. This is another ABI change since 1.16-rc1. (cherry picked from commit e46ba0186720bfe5e31dcad9a5001a415deb9ce6) --- libnm-core/nm-core-internal.h | 10 +++------- libnm-core/nm-keyfile.c | 2 +- libnm-core/nm-setting-wireguard.c | 24 ++++++++++++------------ libnm-core/nm-utils.c | 20 +++++++++++--------- libnm-core/nm-utils.h | 4 ++++ libnm/libnm.ver | 1 + src/devices/nm-device-wireguard.c | 18 +++++++++--------- 7 files changed, 41 insertions(+), 38 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 168ae9978e..6e2a5b5e95 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -765,13 +765,9 @@ gboolean _nm_connection_find_secret (NMConnection *self, #define nm_auto_unref_wgpeer nm_auto(_nm_auto_unref_wgpeer) NM_AUTO_DEFINE_FCN_VOID0 (NMWireGuardPeer *, _nm_auto_unref_wgpeer, nm_wireguard_peer_unref) -gboolean _nm_utils_wireguard_decode_key (const char *base64_key, - gsize required_key_len, - guint8 *out_key); - -gboolean _nm_utils_wireguard_normalize_key (const char *base64_key, - gsize required_key_len, - char **out_base64_key_norm); +gboolean nm_utils_base64secret_normalize (const char *base64_key, + gsize required_key_len, + char **out_base64_key_norm); /*****************************************************************************/ diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index 3633281a3d..05c6bf9710 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -2920,7 +2920,7 @@ _read_setting_wireguard_peer (KeyfileReaderInfo *info) nm_assert (g_str_has_prefix (info->group, NM_KEYFILE_GROUPPREFIX_WIREGUARD_PEER)); cstr = &info->group[NM_STRLEN (NM_KEYFILE_GROUPPREFIX_WIREGUARD_PEER)]; - if ( !_nm_utils_wireguard_normalize_key (cstr, NM_WIREGUARD_PUBLIC_KEY_LEN, &str) + if ( !nm_utils_base64secret_normalize (cstr, NM_WIREGUARD_PUBLIC_KEY_LEN, &str) || !nm_streq0 (str, cstr)) { /* the group name must be identical to the normalized(!) key, so that it * is uniquely identified. */ diff --git a/libnm-core/nm-setting-wireguard.c b/libnm-core/nm-setting-wireguard.c index 7629bfef4e..8c5b25a5b6 100644 --- a/libnm-core/nm-setting-wireguard.c +++ b/libnm-core/nm-setting-wireguard.c @@ -309,9 +309,9 @@ nm_wireguard_peer_set_public_key (NMWireGuardPeer *self, return TRUE; } - is_valid = _nm_utils_wireguard_normalize_key (public_key, - NM_WIREGUARD_PUBLIC_KEY_LEN, - &public_key_normalized); + is_valid = nm_utils_base64secret_normalize (public_key, + NM_WIREGUARD_PUBLIC_KEY_LEN, + &public_key_normalized); nm_assert (is_valid == (public_key_normalized != NULL)); if ( !is_valid @@ -397,9 +397,9 @@ nm_wireguard_peer_set_preshared_key (NMWireGuardPeer *self, return TRUE; } - is_valid = _nm_utils_wireguard_normalize_key (preshared_key, - NM_WIREGUARD_SYMMETRIC_KEY_LEN, - &preshared_key_normalized); + is_valid = nm_utils_base64secret_normalize (preshared_key, + NM_WIREGUARD_SYMMETRIC_KEY_LEN, + &preshared_key_normalized); nm_assert (is_valid == (preshared_key_normalized != NULL)); if ( !is_valid @@ -1128,9 +1128,9 @@ again: return pd; } if ( try_with_normalized_key - && _nm_utils_wireguard_normalize_key (public_key, - NM_WIREGUARD_PUBLIC_KEY_LEN, - &public_key_normalized)) { + && nm_utils_base64secret_normalize (public_key, + NM_WIREGUARD_PUBLIC_KEY_LEN, + &public_key_normalized)) { public_key = public_key_normalized; try_with_normalized_key = FALSE; goto again; @@ -2299,9 +2299,9 @@ set_property (GObject *object, guint prop_id, nm_clear_pointer (&priv->private_key, nm_free_secret); str = g_value_get_string (value); if (str) { - if (_nm_utils_wireguard_normalize_key (str, - NM_WIREGUARD_PUBLIC_KEY_LEN, - &priv->private_key)) + if (nm_utils_base64secret_normalize (str, + NM_WIREGUARD_PUBLIC_KEY_LEN, + &priv->private_key)) priv->private_key_valid = TRUE; else { priv->private_key = g_strdup (str); diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 46dd1eba13..d276cfe62e 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -6673,21 +6673,23 @@ nm_utils_version (void) /*****************************************************************************/ /** - * _nm_utils_wireguard_decode_key: + * nm_utils_base64secret_decode: * @base64_key: the (possibly invalid) base64 encode key. * @required_key_len: the expected (binary) length of the key after * decoding. If the length does not match, the validation fails. - * @out_key: (allow-none): an optional output buffer for the binary + * @out_key: (allow-none): (out): an optional output buffer for the binary * key. If given, it will be filled with exactly @required_key_len * bytes. * * Returns: %TRUE if the input key is a valid base64 encoded key * with @required_key_len bytes. + * + * Since: 1.16 */ gboolean -_nm_utils_wireguard_decode_key (const char *base64_key, - gsize required_key_len, - guint8 *out_key) +nm_utils_base64secret_decode (const char *base64_key, + gsize required_key_len, + guint8 *out_key) { gs_free guint8 *bin_arr = NULL; gsize base64_key_len; @@ -6715,9 +6717,9 @@ _nm_utils_wireguard_decode_key (const char *base64_key, } gboolean -_nm_utils_wireguard_normalize_key (const char *base64_key, - gsize required_key_len, - char **out_base64_key_norm) +nm_utils_base64secret_normalize (const char *base64_key, + gsize required_key_len, + char **out_base64_key_norm) { gs_free guint8 *buf_free = NULL; guint8 buf_static[200]; @@ -6729,7 +6731,7 @@ _nm_utils_wireguard_normalize_key (const char *base64_key, } else buf = buf_static; - if (!_nm_utils_wireguard_decode_key (base64_key, required_key_len, buf)) { + if (!nm_utils_base64secret_decode (base64_key, required_key_len, buf)) { NM_SET_OUT (out_base64_key_norm, NULL); return FALSE; } diff --git a/libnm-core/nm-utils.h b/libnm-core/nm-utils.h index 34aae560e3..2b5baba4cd 100644 --- a/libnm-core/nm-utils.h +++ b/libnm-core/nm-utils.h @@ -263,6 +263,10 @@ NMSriovVF *nm_utils_sriov_vf_from_str (const char *str, GError **error); NM_AVAILABLE_IN_1_12 gint64 nm_utils_get_timestamp_msec (void); +NM_AVAILABLE_IN_1_16 +gboolean nm_utils_base64secret_decode (const char *base64_key, + gsize required_key_len, + guint8 *out_key); G_END_DECLS diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 3af360667e..ece9686efa 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1479,6 +1479,7 @@ global: nm_setting_wireguard_set_peer; nm_team_link_watcher_get_vlanid; nm_team_link_watcher_new_arp_ping2; + nm_utils_base64secret_decode; nm_wifi_p2p_peer_connection_valid; nm_wifi_p2p_peer_filter_connections; nm_wifi_p2p_peer_get_flags; diff --git a/src/devices/nm-device-wireguard.c b/src/devices/nm-device-wireguard.c index 34fe1e1fff..ab30cd3174 100644 --- a/src/devices/nm-device-wireguard.c +++ b/src/devices/nm-device-wireguard.c @@ -729,9 +729,9 @@ _peers_get_platform_list (NMDeviceWireGuardPrivate *priv, NMPWireGuardPeer *plp = &plpeers[i_good]; NMSettingSecretFlags psk_secret_flags; - if (!_nm_utils_wireguard_decode_key (nm_wireguard_peer_get_public_key (peer_data->peer), - sizeof (plp->public_key), - plp->public_key)) + if (!nm_utils_base64secret_decode (nm_wireguard_peer_get_public_key (peer_data->peer), + sizeof (plp->public_key), + plp->public_key)) continue; *plf = NM_PLATFORM_WIREGUARD_CHANGE_PEER_FLAG_NONE; @@ -754,9 +754,9 @@ _peers_get_platform_list (NMDeviceWireGuardPrivate *priv, LINK_CONFIG_MODE_REAPPLY)) { psk_secret_flags = nm_wireguard_peer_get_preshared_key_flags (peer_data->peer); if (!NM_FLAGS_HAS (psk_secret_flags, NM_SETTING_SECRET_FLAG_NOT_REQUIRED)) { - if ( !_nm_utils_wireguard_decode_key (nm_wireguard_peer_get_preshared_key (peer_data->peer), - sizeof (plp->preshared_key), - plp->preshared_key) + if ( !nm_utils_base64secret_decode (nm_wireguard_peer_get_preshared_key (peer_data->peer), + sizeof (plp->preshared_key), + plp->preshared_key) && config_mode == LINK_CONFIG_MODE_FULL) goto skip; } @@ -1128,9 +1128,9 @@ link_config (NMDeviceWireGuard *self, wg_lnk.fwmark = nm_setting_wireguard_get_fwmark (s_wg), wg_change_flags |= NM_PLATFORM_WIREGUARD_CHANGE_FLAG_HAS_FWMARK; - if (_nm_utils_wireguard_decode_key (nm_setting_wireguard_get_private_key (s_wg), - sizeof (wg_lnk.private_key), - wg_lnk.private_key)) { + if (nm_utils_base64secret_decode (nm_setting_wireguard_get_private_key (s_wg), + sizeof (wg_lnk.private_key), + wg_lnk.private_key)) { wg_lnk_clear_private_key = NM_SECRET_PTR_ARRAY (wg_lnk.private_key); wg_change_flags |= NM_PLATFORM_WIREGUARD_CHANGE_FLAG_HAS_PRIVATE_KEY; } else { From a6ee43d1c19bd7bb0905ea990b7f16a2aacf43b6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 28 Feb 2019 14:22:35 +0100 Subject: [PATCH 12/14] cli/wireguard: add import functionality for WireGuard Support importing ".conf" files as `wg-quick up` supports it. `wg-quick` parses several options under "[Interface]" and passes the remainder to `wg setconf`. The PreUp/PreDown/PostUp/PostDown options are of course not supported. "Table" for the moment behaves different. (cherry picked from commit a3a8583c315da45ace75f36c40bea9fa55b59ae3) --- clients/cli/connections.c | 40 ++- clients/common/nm-client-utils.h | 7 + clients/common/nm-vpn-helpers.c | 559 ++++++++++++++++++++++++++++++ clients/common/nm-vpn-helpers.h | 3 + shared/nm-utils/nm-io-utils.c | 4 +- shared/nm-utils/nm-shared-utils.c | 2 + 6 files changed, 598 insertions(+), 17 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index 856848f8fa..6db44f8760 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -8902,8 +8902,13 @@ do_connection_import (NmCli *nmc, int argc, char **argv) NMC_RETURN (nmc, NMC_RESULT_ERROR_USER_INPUT); } - if (argc == 1 && nmc->complete) - complete_option ((const NMMetaAbstractInfo *) nm_meta_property_info_vpn_service_type, *argv, NULL); + if ( argc == 1 + && nmc->complete) { + nmc_complete_strings (*argv, "wireguard"); + complete_option ((const NMMetaAbstractInfo *) nm_meta_property_info_vpn_service_type, + *argv, + NULL); + } if (!type) type = *argv; @@ -8943,21 +8948,26 @@ do_connection_import (NmCli *nmc, int argc, char **argv) NMC_RETURN (nmc, NMC_RESULT_ERROR_USER_INPUT); } - service_type = nm_vpn_plugin_info_list_find_service_type (nm_vpn_get_plugin_infos (), type); - if (!service_type) { - g_string_printf (nmc->return_text, _("Error: failed to find VPN plugin for %s."), type); - NMC_RETURN (nmc, NMC_RESULT_ERROR_UNKNOWN); + if (nm_streq (type, "wireguard")) + connection = nm_vpn_wireguard_import (filename, &error); + else { + service_type = nm_vpn_plugin_info_list_find_service_type (nm_vpn_get_plugin_infos (), type); + if (!service_type) { + g_string_printf (nmc->return_text, _("Error: failed to find VPN plugin for %s."), type); + NMC_RETURN (nmc, NMC_RESULT_ERROR_UNKNOWN); + } + + /* Import VPN configuration */ + plugin = nm_vpn_get_editor_plugin (service_type, &error); + if (!plugin) { + g_string_printf (nmc->return_text, _("Error: failed to load VPN plugin: %s."), + error->message); + NMC_RETURN (nmc, NMC_RESULT_ERROR_UNKNOWN); + } + + connection = nm_vpn_editor_plugin_import (plugin, filename, &error); } - /* Import VPN configuration */ - plugin = nm_vpn_get_editor_plugin (service_type, &error); - if (!plugin) { - g_string_printf (nmc->return_text, _("Error: failed to load VPN plugin: %s."), - error->message); - NMC_RETURN (nmc, NMC_RESULT_ERROR_UNKNOWN); - } - - connection = nm_vpn_editor_plugin_import (plugin, filename, &error); if (!connection) { g_string_printf (nmc->return_text, _("Error: failed to import '%s': %s."), filename, error->message); diff --git a/clients/common/nm-client-utils.h b/clients/common/nm-client-utils.h index 9d818873c0..a5bc05fab0 100644 --- a/clients/common/nm-client-utils.h +++ b/clients/common/nm-client-utils.h @@ -24,6 +24,13 @@ #include "nm-active-connection.h" #include "nm-device.h" + +#define nm_auto_unref_ip_address nm_auto (_nm_ip_address_unref) +NM_AUTO_DEFINE_FCN0 (NMIPAddress *, _nm_ip_address_unref, nm_ip_address_unref) + +#define nm_auto_unref_wgpeer nm_auto (_nm_auto_unref_wgpeer) +NM_AUTO_DEFINE_FCN0 (NMWireGuardPeer *, _nm_auto_unref_wgpeer, nm_wireguard_peer_unref) + const NMObject **nmc_objects_sort_by_path (const NMObject *const*objs, gssize len); const char *nmc_string_is_valid (const char *input, const char **allowed, GError **error); diff --git a/clients/common/nm-vpn-helpers.c b/clients/common/nm-vpn-helpers.c index a2e7dc0374..2353b7691e 100644 --- a/clients/common/nm-vpn-helpers.c +++ b/clients/common/nm-vpn-helpers.c @@ -25,7 +25,13 @@ #include "nm-vpn-helpers.h" +#include +#include + +#include "nm-client-utils.h" #include "nm-utils.h" +#include "nm-utils/nm-io-utils.h" +#include "nm-utils/nm-secret-utils.h" /*****************************************************************************/ @@ -247,3 +253,556 @@ nm_vpn_openconnect_authenticate_helper (const char *host, return TRUE; } +static gboolean +_wg_complete_peer (GPtrArray **p_peers, + NMWireGuardPeer *peer_take, + gsize peer_start_line_nr, + const char *filename, + GError **error) +{ + nm_auto_unref_wgpeer NMWireGuardPeer *peer = peer_take; + gs_free_error GError *local = NULL; + + if (!peer) + return TRUE; + + if (!nm_wireguard_peer_is_valid (peer, TRUE, TRUE, &local)) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("Invalid peer starting at %s:%zu: %s"), + filename, + peer_start_line_nr, + local->message); + return FALSE; + } + + if (!*p_peers) + *p_peers = g_ptr_array_new_with_free_func ((GDestroyNotify) nm_wireguard_peer_unref); + g_ptr_array_add (*p_peers, g_steal_pointer (&peer)); + return TRUE; +} + +static gboolean +_line_match (char *line, const char *key, gsize key_len, const char **out_key, char **out_value) +{ + nm_assert (line); + nm_assert (key); + nm_assert (strlen (key) == key_len); + nm_assert (!strchr (key, '=')); + nm_assert (out_key && !*out_key); + nm_assert (out_value && !*out_value); + + /* Note that `wg-quick` (linux.bash) does case-insensitive comparison (shopt -s nocasematch). + * `wg setconf` does case-insensitive comparison too (with strncasecmp, which is locale dependent). + * + * We do a case-insensitive comparison of the key, however in a locale-independent manner. */ + + if (g_ascii_strncasecmp (line, key, key_len) != 0) + return FALSE; + + if (line[key_len] != '=') + return FALSE; + + *out_key = key; + *out_value = &line[key_len + 1]; + return TRUE; +} + +#define line_match(line, key, out_key, out_value) \ + _line_match ((line), ""key"", NM_STRLEN (key), (out_key), (out_value)) + +static gboolean +value_split_word (char **line_remainder, char **out_word) +{ + char *str; + + if ((*line_remainder)[0] == '\0') + return FALSE; + + *out_word = *line_remainder; + + str = strchrnul (*line_remainder, ','); + if (str[0] == ',') { + str[0] = '\0'; + *line_remainder = &str[1]; + } else + *line_remainder = str; + return TRUE; +} + +NMConnection * +nm_vpn_wireguard_import (const char *filename, + GError **error) +{ + nm_auto_clear_secret_ptr NMSecretPtr file_content = NM_SECRET_PTR_INIT (); + char ifname[IFNAMSIZ]; + gs_free char *uuid = NULL; + gboolean ifname_valid = FALSE; + const char *cstr; + char *line_remainder; + gs_unref_object NMConnection *connection = NULL; + NMSettingConnection *s_con; + NMSettingIPConfig *s_ip4; + NMSettingIPConfig *s_ip6; + NMSettingWireGuard *s_wg; + gs_free_error GError *local = NULL; + enum { + LINE_CONTEXT_INIT, + LINE_CONTEXT_INTERFACE, + LINE_CONTEXT_PEER, + } line_context; + gsize line_nr; + gsize current_peer_start_line_nr = 0; + nm_auto_unref_wgpeer NMWireGuardPeer *current_peer = NULL; + gs_unref_ptrarray GPtrArray *data_dns_v4 = NULL; + gs_unref_ptrarray GPtrArray *data_dns_v6 = NULL; + gs_unref_ptrarray GPtrArray *data_addr_v4 = NULL; + gs_unref_ptrarray GPtrArray *data_addr_v6 = NULL; + gs_unref_ptrarray GPtrArray *data_peers = NULL; + const char *data_private_key = NULL; + gint64 data_table; + guint data_listen_port = 0; + guint data_fwmark = 0; + guint data_mtu = 0; + int is_v4; + guint i; + + g_return_val_if_fail (filename, NULL); + g_return_val_if_fail (!error || !*error, NULL); + + /* contrary to "wg-quick", we never interpret the filename as "/etc/wireguard/$INTERFACE.conf". + * If the filename has no '/', it is interpreted as relative to the current working directory. + * However, we do require a suitable filename suffix and that the name corresponds to the interface + * name. */ + cstr = strrchr (filename, '/'); + cstr = cstr ? &cstr[1] : filename; + if (NM_STR_HAS_SUFFIX (cstr, ".conf")) { + gsize len = strlen (cstr) - NM_STRLEN (".conf"); + + if (len > 0 && len < sizeof (ifname)) { + memcpy (ifname, cstr, len); + ifname[len] = '\0'; + + if (nm_utils_is_valid_iface_name (ifname, NULL)) + ifname_valid = TRUE; + } + } + if (!ifname_valid) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, + _("The WireGuard config file must be a valid interface name followed by \".conf\"")); + return FALSE; + } + + if (nm_utils_file_get_contents (-1, + filename, + 10*1024*1024, + NM_UTILS_FILE_GET_CONTENTS_FLAG_SECRET, + &file_content.str, + &file_content.len, + error) < 0) + return NULL; + + /* We interpret the file like `wg-quick up` and `wg setconf` do. + * + * Of course the WireGuard scripts do something fundamentlly different. They + * perform actions to configure the WireGuard link in kernel, add routes and + * addresses, and call resolvconf. It all happens at the time when the script + * run. + * + * This code here instead generates a NetworkManager connection profile so that + * NetworkManager will apply a similar configuration when later activating the profile. */ + +#define _TABLE_AUTO ((gint64) -1) +#define _TABLE_OFF ((gint64) -2) + + data_table = _TABLE_AUTO; + + line_remainder = file_content.str; + line_context = LINE_CONTEXT_INIT; + line_nr = 0; + while (line_remainder[0] != '\0') { + const char *matched_key = NULL; + char *value = NULL; + char *line; + char ch; + gint64 i64; + + line_nr++; + + line = line_remainder; + line_remainder = strchrnul (line, '\n'); + if (line_remainder[0] != '\0') + (line_remainder++)[0] = '\0'; + + /* Drop all spaces and truncate at first '#'. + * See wg's config_read_line(). + * + * Note that wg-quick doesn't do that. + * + * Neither `wg setconf` nor `wg-quick` does a strict parsing. + * We don't either. Just try to interpret the file (mostly) the same as + * they would. + */ + { + gsize l, n; + + n = 0; + for (l = 0; (ch = line[l]); l++) { + if (g_ascii_isspace (ch)) { + /* wg-setconf strips all whitespace before parsing the content. That means, + * *[I nterface]" will be accepted. We do that too. */ + continue; + } + if (ch == '#') + break; + line[n++] = line[l]; + } + if (n == 0) + continue; + line[n] = '\0'; + } + + if (g_ascii_strcasecmp (line, "[Interface]") == 0) { + if (!_wg_complete_peer (&data_peers, + g_steal_pointer (¤t_peer), + current_peer_start_line_nr, + filename, + error)) + return FALSE; + line_context = LINE_CONTEXT_INTERFACE; + continue; + } + + if (g_ascii_strcasecmp (line, "[Peer]") == 0) { + if (!_wg_complete_peer (&data_peers, + g_steal_pointer (¤t_peer), + current_peer_start_line_nr, + filename, + error)) + return FALSE; + current_peer_start_line_nr = line_nr; + current_peer = nm_wireguard_peer_new (); + line_context = LINE_CONTEXT_PEER; + continue; + } + + if (line_context == LINE_CONTEXT_INTERFACE) { + + if (line_match (line, "Address", &matched_key, &value)) { + char *value_word; + + while (value_split_word (&value, &value_word)) { + GPtrArray **p_data_addr; + NMIPAddr addr_bin; + int addr_family; + int prefix_len; + + if (!nm_utils_parse_inaddr_prefix_bin (AF_UNSPEC, + value_word, + &addr_family, + &addr_bin, + &prefix_len)) + goto fail_invalid_value; + + p_data_addr = (addr_family == AF_INET) + ? &data_addr_v4 + : &data_addr_v6; + + if (!*p_data_addr) + *p_data_addr = g_ptr_array_new_with_free_func ((GDestroyNotify) nm_ip_address_unref); + + g_ptr_array_add (*p_data_addr, + nm_ip_address_new_binary (addr_family, + &addr_bin, + prefix_len == -1 + ? ((addr_family == AF_INET) ? 32 : 128) + : prefix_len, + NULL)); + } + continue; + } + + if (line_match (line, "MTU", &matched_key, &value)) { + i64 = _nm_utils_ascii_str_to_int64 (value, 0, 0, G_MAXUINT32, -1); + if (i64 == -1) + goto fail_invalid_value; + + /* wg-quick accepts the "MTU" value, but it also fetches routes to + * autodetect it. NetworkManager won't do that, we can only configure + * an explict MTU or no autodetection will be performed. */ + data_mtu = i64; + continue; + } + + if (line_match (line, "DNS", &matched_key, &value)) { + char *value_word; + + while (value_split_word (&value, &value_word)) { + char addr_s[NM_CONST_MAX (INET_ADDRSTRLEN, INET6_ADDRSTRLEN)]; + GPtrArray **p_data_dns; + NMIPAddr addr_bin; + int addr_family; + + if (!nm_utils_parse_inaddr_bin (AF_UNSPEC, + value_word, + &addr_family, + &addr_bin)) + goto fail_invalid_value; + + p_data_dns = (addr_family == AF_INET) + ? &data_dns_v4 + : &data_dns_v6; + if (!*p_data_dns) + *p_data_dns = g_ptr_array_new_with_free_func (g_free); + + inet_ntop (addr_family, &addr_bin, addr_s, sizeof (addr_s)); + g_ptr_array_add (*p_data_dns, g_strdup (addr_s)); + } + continue; + } + + if (line_match (line, "Table", &matched_key, &value)) { + + if (nm_streq (value, "auto")) + data_table = _TABLE_AUTO; + else if (nm_streq (value, "off")) + data_table = _TABLE_OFF; + else { + /* we don't support table names from /etc/iproute2/rt_tables + * But we accept hex like `ip route add` would. */ + i64 = _nm_utils_ascii_str_to_int64 (value, 0, 0, G_MAXINT32, -1); + if (i64 == -1) + goto fail_invalid_value; + data_table = i64; + } + continue; + } + + if ( line_match (line, "PreUp", &matched_key, &value) + || line_match (line, "PreDown", &matched_key, &value) + || line_match (line, "PostUp", &matched_key, &value) + || line_match (line, "PostDown", &matched_key, &value)) { + /* we don't run any scripts. Silently ignore these paramters. */ + continue; + } + + if (line_match (line, "SaveConfig", &matched_key, &value)) { + /* we ignore the setting, but enforce that it's either true or false (like + * wg-quick. */ + if (!NM_IN_STRSET (value, "true", "false")) + goto fail_invalid_value; + continue; + } + + if (line_match (line, "ListenPort", &matched_key, &value)) { + /* we don't use getaddrinfo(), unlike `wg setconf`. Just interpret + * the port as plain decimal number. */ + i64 = _nm_utils_ascii_str_to_int64 (value, 10, 0, 0xFFFF, -1); + if (i64 == -1) + goto fail_invalid_value; + data_listen_port = i64; + continue; + } + + if (line_match (line, "FwMark", &matched_key, &value)) { + if (nm_streq (value, "off")) + data_fwmark = 0; + else { + i64 = _nm_utils_ascii_str_to_int64 (value, 0, 0, G_MAXINT32, -1); + if (i64 == -1) + goto fail_invalid_value; + data_fwmark = i64; + } + continue; + } + + if (line_match (line, "PrivateKey", &matched_key, &value)) { + if (!nm_utils_base64secret_decode (value, NM_WIREGUARD_PUBLIC_KEY_LEN, NULL)) + goto fail_invalid_secret; + data_private_key = value; + continue; + } + + goto fail_invalid_line; + } + + + if (line_context == LINE_CONTEXT_PEER) { + + if (line_match (line, "Endpoint", &matched_key, &value)) { + if (!nm_wireguard_peer_set_endpoint (current_peer, value, FALSE)) + goto fail_invalid_value; + continue; + } + + if (line_match (line, "PublicKey", &matched_key, &value)) { + if (!nm_wireguard_peer_set_public_key (current_peer, value, FALSE)) + goto fail_invalid_value; + continue; + } + + if (line_match (line, "AllowedIPs", &matched_key, &value)) { + char *value_word; + + while (value_split_word (&value, &value_word)) { + if (!nm_wireguard_peer_append_allowed_ip (current_peer, + value_word, + FALSE)) + goto fail_invalid_value; + } + continue; + } + + if (line_match (line, "PersistentKeepalive", &matched_key, &value)) { + if (nm_streq (value, "off")) + i64 = 0; + else { + i64 = _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT16, -1); + if (i64 == -1) + goto fail_invalid_value; + } + nm_wireguard_peer_set_persistent_keepalive (current_peer, i64); + continue; + } + + if (line_match (line, "PresharedKey", &matched_key, &value)) { + if (!nm_wireguard_peer_set_preshared_key (current_peer, value, FALSE)) + goto fail_invalid_secret; + nm_wireguard_peer_set_preshared_key_flags (current_peer, NM_SETTING_SECRET_FLAG_NONE); + continue; + } + + goto fail_invalid_line; + } + +fail_invalid_line: + nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, + _("unrecognized line at %s:%zu"), + filename, line_nr); + return FALSE; +fail_invalid_value: + nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, + _("invalid value for '%s' at %s:%zu"), + matched_key, filename, line_nr); + return FALSE; +fail_invalid_secret: + nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, + _("invalid secret '%s' at %s:%zu"), + matched_key, filename, line_nr); + return FALSE; + } + + if (!_wg_complete_peer (&data_peers, + g_steal_pointer (¤t_peer), + current_peer_start_line_nr, + filename, + error)) + return FALSE; + + connection = nm_simple_connection_new (); + s_con = NM_SETTING_CONNECTION (nm_setting_connection_new ()); + nm_connection_add_setting (connection, NM_SETTING (s_con)); + s_ip4 = NM_SETTING_IP_CONFIG (nm_setting_ip4_config_new ()); + nm_connection_add_setting (connection, NM_SETTING (s_ip4)); + s_ip6 = NM_SETTING_IP_CONFIG (nm_setting_ip6_config_new ()); + nm_connection_add_setting (connection, NM_SETTING (s_ip6)); + s_wg = NM_SETTING_WIREGUARD (nm_setting_wireguard_new ()); + nm_connection_add_setting (connection, NM_SETTING (s_wg)); + + uuid = nm_utils_uuid_generate (); + + g_object_set (s_con, + NM_SETTING_CONNECTION_ID, + ifname, + NM_SETTING_CONNECTION_UUID, + uuid, + NM_SETTING_CONNECTION_TYPE, + NM_SETTING_WIREGUARD_SETTING_NAME, + NM_SETTING_CONNECTION_INTERFACE_NAME, + ifname, + NULL); + + g_object_set (s_wg, + NM_SETTING_WIREGUARD_PRIVATE_KEY, + data_private_key, + NM_SETTING_WIREGUARD_LISTEN_PORT, + data_listen_port, + NM_SETTING_WIREGUARD_FWMARK, + data_fwmark, + NM_SETTING_WIREGUARD_MTU, + data_mtu, + NULL); + + if (data_peers) { + for (i = 0; i < data_peers->len; i++) + nm_setting_wireguard_append_peer (s_wg, data_peers->pdata[i]); + } + + for (is_v4 = 0; is_v4 < 2; is_v4++) { + const char *method_disabled = is_v4 ? NM_SETTING_IP4_CONFIG_METHOD_DISABLED : NM_SETTING_IP6_CONFIG_METHOD_IGNORE; + const char *method_manual = is_v4 ? NM_SETTING_IP4_CONFIG_METHOD_MANUAL : NM_SETTING_IP6_CONFIG_METHOD_MANUAL; + NMSettingIPConfig *s_ip = is_v4 ? s_ip4 : s_ip6; + GPtrArray *data_dns = is_v4 ? data_dns_v4 : data_dns_v6; + GPtrArray *data_addr = is_v4 ? data_addr_v4 : data_addr_v6; + + if (data_dns && !data_addr) { + /* When specifying "DNS", we also require an "Address" for the same address + * family. That is because a NMSettingIPConfig cannot have @method_disabled + * and DNS settings at the same time. + * + * We don't have addresses. Silently ignore the DNS setting. */ + data_dns = NULL; + } + + g_object_set (s_ip, + NM_SETTING_IP_CONFIG_METHOD, + data_addr ? method_manual : method_disabled, + NULL); + + if (data_addr) { + for (i = 0; i < data_addr->len; i++) + nm_setting_ip_config_add_address (s_ip, data_addr->pdata[i]); + } + if (data_dns) { + for (i = 0; i < data_dns->len; i++) + nm_setting_ip_config_add_dns (s_ip, data_dns->pdata[i]); + } + + if (data_table == _TABLE_AUTO) { + /* in the "auto" setting, wg-quick adds peer-routes automatically to the main + * table. NetworkManager will do that too, but there are differences: + * + * - NetworkManager (contrary to wg-quick) does not check whether the peer-route is necessary. + * It will always add a route for each allowed-ips range, even if there is already another + * route that would ensure packets to the endpoint are routed via the WireGuard interface. + * If you don't want that, disable "wireguard.peer-routes", and add the necessary routes + * yourself to "ipv4.routes" and "ipv6.routes". + * + * - With "auto", wg-quick also configures policy routing to handle default-routes (/0) to + * avoid routing loops. That is not yet solved by NetworkManager, you need to configure + * that explicitly (for example, by adding a direct route to the gateway on the interface + * that has the default-route, or by using a script (possibly dispatcher script). + */ + } else if (data_table == _TABLE_OFF) { + if (is_v4) { + g_object_set (s_wg, + NM_SETTING_WIREGUARD_PEER_ROUTES, + FALSE, + NULL); + } + } else { + g_object_set (s_ip, + NM_SETTING_IP_CONFIG_ROUTE_TABLE, + (guint) data_table, + NULL); + } + } + + if (!nm_connection_normalize (connection, NULL, NULL, &local)) { + nm_utils_error_set (error, NM_UTILS_ERROR_INVALID_ARGUMENT, + _("Failed to create WireGuard connection: %s"), + local->message); + return FALSE; + } + + return g_steal_pointer (&connection); +} diff --git a/clients/common/nm-vpn-helpers.h b/clients/common/nm-vpn-helpers.h index 4c15faa1b8..686687ba4d 100644 --- a/clients/common/nm-vpn-helpers.h +++ b/clients/common/nm-vpn-helpers.h @@ -39,4 +39,7 @@ gboolean nm_vpn_openconnect_authenticate_helper (const char *host, int *status, GError **error); +NMConnection *nm_vpn_wireguard_import (const char *filename, + GError **error); + #endif /* __NM_VPN_HELPERS_H__ */ diff --git a/shared/nm-utils/nm-io-utils.c b/shared/nm-utils/nm-io-utils.c index ce1fee6862..513127480e 100644 --- a/shared/nm-utils/nm-io-utils.c +++ b/shared/nm-utils/nm-io-utils.c @@ -268,8 +268,8 @@ nm_utils_fd_get_contents (int fd, * @flags: %NMUtilsFileGetContentsFlags for reading the file. * @contents: the output buffer with the file read. It is always * NUL terminated. The buffer is at most @max_length long, including - * the NUL byte. That is, it reads only files up to a length of - * @max_length - 1 bytes. + * the NUL byte. That is, it reads only files up to a length of + * @max_length - 1 bytes. * @length: optional output argument of the read file size. * * A reimplementation of g_file_get_contents() with a few differences: diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index 0e427a24c6..6a43c67063 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -672,6 +672,8 @@ nm_utils_parse_inaddr_prefix_bin (int addr_family, return FALSE; if (slash) { + /* For IPv4, `ip addr add` supports the prefix-length as a netmask. We don't + * do that. */ prefix = _nm_utils_ascii_str_to_int64 (slash + 1, 10, 0, addr_family == AF_INET ? 32 : 128, From bf365e9762dfcb2cd0f9f557b19c8cf541854de1 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 6 Mar 2019 16:59:53 +0100 Subject: [PATCH 13/14] clients/tests: add wireguard import tests (cherry picked from commit c152ca37efb67543b34868d5415bf8e03052e34e) --- Makefile.am | 7 +++ clients/common/tests/test-general.c | 96 +++++++++++++++++++++++++++++ clients/common/tests/wg-test0.conf | 18 ++++++ clients/common/tests/wg-test1.conf | 3 + clients/common/tests/wg-test2.conf | 8 +++ clients/common/tests/wg-test3.conf | 3 + 6 files changed, 135 insertions(+) create mode 100644 clients/common/tests/wg-test0.conf create mode 100644 clients/common/tests/wg-test1.conf create mode 100644 clients/common/tests/wg-test2.conf create mode 100644 clients/common/tests/wg-test3.conf diff --git a/Makefile.am b/Makefile.am index b5dc5f9440..b180466adc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4004,6 +4004,13 @@ clients_common_tests_test_general_LDADD = \ $(clients_common_tests_test_general_OBJECTS): $(libnm_core_lib_h_pub_mkenums) +EXTRA_DIST += \ + clients/common/tests/wg-test0.conf \ + clients/common/tests/wg-test1.conf \ + clients/common/tests/wg-test2.conf \ + clients/common/tests/wg-test3.conf \ + $(NULL) + ############################################################################### # clients/cli ############################################################################### diff --git a/clients/common/tests/test-general.c b/clients/common/tests/test-general.c index 8f96eb262f..c39901bae8 100644 --- a/clients/common/tests/test-general.c +++ b/clients/common/tests/test-general.c @@ -20,6 +20,7 @@ #include "nm-default.h" #include "nm-meta-setting-access.h" +#include "nm-vpn-helpers.h" #include "nm-utils/nm-test-utils.h" @@ -145,6 +146,96 @@ test_client_meta_check (void) /*****************************************************************************/ +static void +test_client_import_wireguard_test0 (void) +{ + gs_unref_object NMConnection *connection; + NMSettingWireGuard *s_wg; + NMWireGuardPeer *peer; + gs_free_error GError *error = NULL; + + connection = nm_vpn_wireguard_import (NM_BUILD_SRCDIR"/clients/common/tests/wg-test0.conf", + &error); + + g_assert_no_error (error); + + g_assert_cmpstr (nm_connection_get_id (connection), ==, "wg-test0"); + g_assert_cmpstr (nm_connection_get_interface_name (connection), ==, "wg-test0"); + g_assert_cmpstr (nm_connection_get_connection_type (connection), ==, NM_SETTING_WIREGUARD_SETTING_NAME); + + s_wg = NM_SETTING_WIREGUARD (nm_connection_get_setting (connection, NM_TYPE_SETTING_WIREGUARD)); + + g_assert_cmpint (nm_setting_wireguard_get_listen_port (s_wg), ==, 51820); + g_assert_cmpstr (nm_setting_wireguard_get_private_key (s_wg), ==, "yAnz5TF+lXXJte14tji3zlMNq+hd2rYUIgJBgB3fBmk="); + + g_assert_cmpint (nm_setting_wireguard_get_peers_len (s_wg), ==, 3); + + peer = nm_setting_wireguard_get_peer (s_wg, 0); + g_assert_cmpstr (nm_wireguard_peer_get_public_key (peer), ==, "xTIBA5rboUvnH4htodjb6e697QjLERt1NAB4mZqp8Dg="); + g_assert_cmpstr (nm_wireguard_peer_get_endpoint (peer), ==, "192.95.5.67:1234"); + g_assert_cmpint (nm_wireguard_peer_get_allowed_ips_len (peer), ==, 2); + g_assert_cmpstr (nm_wireguard_peer_get_allowed_ip (peer, 0, NULL), ==, "10.192.122.3/32"); + g_assert_cmpstr (nm_wireguard_peer_get_allowed_ip (peer, 1, NULL), ==, "10.192.124.1/24"); + + peer = nm_setting_wireguard_get_peer (s_wg, 1); + g_assert_cmpstr (nm_wireguard_peer_get_public_key (peer), ==, "TrMvSoP4jYQlY6RIzBgbssQqY3vxI2Pi+y71lOWWXX0="); + g_assert_cmpstr (nm_wireguard_peer_get_endpoint (peer), ==, "[2607:5300:60:6b0::c05f:543]:2468"); + g_assert_cmpint (nm_wireguard_peer_get_allowed_ips_len (peer), ==, 2); + g_assert_cmpstr (nm_wireguard_peer_get_allowed_ip (peer, 0, NULL), ==, "10.192.122.4/32"); + g_assert_cmpstr (nm_wireguard_peer_get_allowed_ip (peer, 1, NULL), ==, "192.168.0.0/16"); + + peer = nm_setting_wireguard_get_peer (s_wg, 2); + g_assert_cmpstr (nm_wireguard_peer_get_public_key (peer), ==, "gN65BkIKy1eCE9pP1wdc8ROUtkHLF2PfAqYdyYBz6EA="); + g_assert_cmpstr (nm_wireguard_peer_get_endpoint (peer), ==, "test.wireguard.com:18981"); + g_assert_cmpint (nm_wireguard_peer_get_allowed_ips_len (peer), ==, 1); + g_assert_cmpstr (nm_wireguard_peer_get_allowed_ip (peer, 0, NULL), ==, "10.10.10.230/32"); +} + +static void +test_client_import_wireguard_test1 (void) +{ + gs_free_error GError *error = NULL; + + nm_vpn_wireguard_import (NM_BUILD_SRCDIR"/clients/common/tests/wg-test1.conf", &error); + g_assert_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT); + g_assert (g_str_has_prefix (error->message, "invalid secret 'PrivateKey'")); + g_assert (g_str_has_suffix (error->message, "wg-test1.conf:2")); +} + +static void +test_client_import_wireguard_test2 (void) +{ + gs_free_error GError *error = NULL; + + nm_vpn_wireguard_import (NM_BUILD_SRCDIR"/clients/common/tests/wg-test2.conf", &error); + + g_assert_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT); + g_assert (g_str_has_prefix (error->message, "unrecognized line at")); + g_assert (g_str_has_suffix (error->message, "wg-test2.conf:5")); +} + +static void +test_client_import_wireguard_test3 (void) +{ + gs_free_error GError *error = NULL; + + nm_vpn_wireguard_import (NM_BUILD_SRCDIR"/clients/common/tests/wg-test3.conf", &error); + g_assert_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT); + g_assert (g_str_has_prefix (error->message, "invalid value for 'ListenPort'")); + g_assert (g_str_has_suffix (error->message, "wg-test3.conf:3")); +} + +static void +test_client_import_wireguard_missing (void) +{ + gs_free_error GError *error = NULL; + + nm_vpn_wireguard_import (NM_BUILD_SRCDIR"/clients/common/tests/wg-missing.conf", &error); + g_assert_error (error, G_FILE_ERROR, G_FILE_ERROR_NOENT); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -153,6 +244,11 @@ main (int argc, char **argv) nmtst_init (&argc, &argv, TRUE); g_test_add_func ("/client/meta/check", test_client_meta_check); + g_test_add_func ("/client/import/wireguard/test0", test_client_import_wireguard_test0); + g_test_add_func ("/client/import/wireguard/test1", test_client_import_wireguard_test1); + g_test_add_func ("/client/import/wireguard/test2", test_client_import_wireguard_test2); + g_test_add_func ("/client/import/wireguard/test3", test_client_import_wireguard_test3); + g_test_add_func ("/client/import/wireguard/missing", test_client_import_wireguard_missing); return g_test_run (); } diff --git a/clients/common/tests/wg-test0.conf b/clients/common/tests/wg-test0.conf new file mode 100644 index 0000000000..61438c2942 --- /dev/null +++ b/clients/common/tests/wg-test0.conf @@ -0,0 +1,18 @@ +[Interface] +PrivateKey = yAnz5TF+lXXJte14tji3zlMNq+hd2rYUIgJBgB3fBmk= +ListenPort = 51820 + +[Peer] +PublicKey = xTIBA5rboUvnH4htodjb6e697QjLERt1NAB4mZqp8Dg= +Endpoint = 192.95.5.67:1234 +AllowedIPs = 10.192.122.3/32, 10.192.124.1/24 + +[Peer] +PublicKey = TrMvSoP4jYQlY6RIzBgbssQqY3vxI2Pi+y71lOWWXX0= +Endpoint = [2607:5300:60:6b0::c05f:543]:2468 +AllowedIPs = 10.192.122.4/32, 192.168.0.0/16 + +[Peer] +PublicKey = gN65BkIKy1eCE9pP1wdc8ROUtkHLF2PfAqYdyYBz6EA= +Endpoint = test.wireguard.com:18981 +AllowedIPs = 10.10.10.230/32 diff --git a/clients/common/tests/wg-test1.conf b/clients/common/tests/wg-test1.conf new file mode 100644 index 0000000000..ceb267acba --- /dev/null +++ b/clients/common/tests/wg-test1.conf @@ -0,0 +1,3 @@ +[Interface] +PrivateKey = bad +ListenPort = 51820 diff --git a/clients/common/tests/wg-test2.conf b/clients/common/tests/wg-test2.conf new file mode 100644 index 0000000000..f691c2af65 --- /dev/null +++ b/clients/common/tests/wg-test2.conf @@ -0,0 +1,8 @@ +[Interface] +PrivateKey = yAnz5TF+lXXJte14tji3zlMNq+hd2rYUIgJBgB3fBmk= +ListenPort = 51820 + +[Beer] +PublicKey = xTIBA5rboUvnH4htodjb6e697QjLERt1NAB4mZqp8Dg= +Endpoint = 192.95.5.67:1234 +AllowedIPs = 10.192.122.3/32, 10.192.124.1/24 diff --git a/clients/common/tests/wg-test3.conf b/clients/common/tests/wg-test3.conf new file mode 100644 index 0000000000..9956e4004d --- /dev/null +++ b/clients/common/tests/wg-test3.conf @@ -0,0 +1,3 @@ +[Interface] +PrivateKey = yAnz5TF+lXXJte14tji3zlMNq+hd2rYUIgJBgB3fBmk= +ListenPort = 666666666 From 7864bb84eeed1b7f1c7e02acdf6a8daeec76b84c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Mar 2019 09:02:20 +0100 Subject: [PATCH 14/14] wireguard: update TODO list for WireGuard devices (cherry picked from commit 3990c92fbf4789d8de2264b39f9d1b690f5dd4d5) --- src/devices/nm-device-wireguard.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/devices/nm-device-wireguard.c b/src/devices/nm-device-wireguard.c index ab30cd3174..0fc12817df 100644 --- a/src/devices/nm-device-wireguard.c +++ b/src/devices/nm-device-wireguard.c @@ -37,9 +37,6 @@ _LOG_DECLARE_SELF(NMDeviceWireGuard); /*****************************************************************************/ -/* TODO: ensure externally-managed works. Both after start of NM and - * when adding a wg link with NM running. */ - /* TODO: activate profile with peer preshared-key-flags=2. On first activation, the secret is * requested (good). Enter it and connect. Reactivate the profile, now there is no password * prompt, as the secret is cached (good??). */ @@ -47,7 +44,15 @@ _LOG_DECLARE_SELF(NMDeviceWireGuard); /* TODO: unlike for other VPNs, we don't inject a direct route to the peers. That means, * you might get a routing sceneraio where the peer (VPN server) is reachable via the VPN. * How we handle adding routes to external gateway for other peers, has severe issues -* as well. I think the only solution is https://www.wireguard.com/netns/#improving-the-classic-solutions */ + * as well. We may use policy-routing like wg-quick does. See also disussions at + * https://www.wireguard.com/netns/#improving-the-classic-solutions */ + +/* TODO: honor the TTL of DNS to determine when to retry resolving endpoints. */ + +/* TODO: when we get multiple IP addresses when resolving a peer endpoint. We currently + * just take the first from GAI. We should only accept AAAA/IPv6 if we also have a suitable + * IPv6 address. The problem is, that we have to recheck that when IP addressing on other + * interfaces changes. This makes it almost too cumbersome to implement. */ /*****************************************************************************/