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.

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);