core: refactor loading machine-id and cache it

Previously, whenever we needed /etc/machine-id we would re-load it
from file. The are 3 downsides of that:

 - the smallest downside is the runtime overhead of repeatedly
   reading the file and parse it.

 - as we read it multiple times, it may change anytime. Most
   code in NetworkManager does not expect or handle a change of
   the machine-id.
   Generally, the admin should make sure that the machine-id is properly
   initialized before NetworkManager starts, and not change it. As such,
   a change of the machine-id should never happen in practice.
   But if it would change, we would get odd behaviors. Note for example
   how generate_duid_from_machine_id() already cached the generated DUID
   and only read it once.
   It's better to pick the machine-id once, and rely to use the same
   one for the remainder of the program.
   If the admin wants to change the machine-id, NetworkManager must be
   restarted as well (in case the admin cares).
   Also, as we now only load it once, it makes sense to log an error
   (once) when we fail to read the machine-id.

 - previously, loading the machine-id could fail each time. And we
   have to somehow handle that error. It seems, the best thing what we
   anyway can do, is to log an error once and continue with a fake
   machine-id. Here we add a fake machine-id based on the secret-key
   or the boot-id. Now obtaining a machine-id can no longer fail
   and error handling is no longer necessary.

Also, ensure that a machine-id of all zeros is not valid.

Technically, a machine-id is not an RFC 4122 UUID. But it's
the same size, so we also use NMUuid data structure for it.

While at it, also refactor caching of the boot-id and the secret
key. In particular, fix the thread-safety of the double-checked
locking implementations.

8 files changed, 265 insertions, 92 deletions

diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c
index d527a49371..1ac85827c5 100644
--- a/src/devices/nm-device-ethernet.c
+++ b/src/devices/nm-device-ethernet.c
@@ -1455,7 +1455,6 @@ new_default_connection (NMDevice *self)
 	const char *uprop = "0";
 	gs_free char *defname = NULL;
 	gs_free char *uuid = NULL;
-	gs_free char *machine_id = NULL;
 	guint i, n_connections;
 
 	if (nm_config_get_no_auto_default_for_device (nm_config_get (), self))
@@ -1479,12 +1478,10 @@ new_default_connection (NMDevice *self)
 	if (!defname)
 		return NULL;
 
-	machine_id = nm_utils_machine_id_read ();
-
 	/* Create a stable UUID. The UUID is also the Network_ID for stable-privacy addr-gen-mode,
 	 * thus when it changes we will also generate different IPv6 addresses. */
 	uuid = _nm_utils_uuid_generate_from_strings ("default-wired",
-	                                             machine_id ?: "",
+	                                             nm_utils_machine_id_str (),
 	                                             defname,
 	                                             perm_hw_addr,
 	                                             NULL);
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index c4c3365901..d5a58f4a74 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -37,7 +37,6 @@
 #include <linux/if_arp.h>
 #include <linux/rtnetlink.h>
 #include <linux/pkt_sched.h>
-#include <uuid/uuid.h>
 
 #include "nm-utils/nm-dedup-multi.h"
 #include "nm-utils/nm-random-utils.h"
@@ -8303,8 +8302,7 @@ generate_duid_uuid (guint8 *data, gsize data_len)
 static GBytes *
 generate_duid_from_machine_id (void)
 {
-	gs_free const char *machine_id_s = NULL;
-	uuid_t uuid;
+	const NMUuid *uuid;
 	GChecksum *sum;
 	guint8 sha256_digest[32];
 	gsize len = sizeof (sha256_digest);
@@ -8313,13 +8311,11 @@ generate_duid_from_machine_id (void)
 	if (global_duid)
 		return g_bytes_ref (global_duid);
 
-	machine_id_s = nm_utils_machine_id_read ();
-	if (!nm_utils_machine_id_parse (machine_id_s, uuid))
-		return NULL;
+	uuid = nm_utils_machine_id_bin ();
 
 	/* Hash the machine ID so it's not leaked to the network */
 	sum = g_checksum_new (G_CHECKSUM_SHA256);
-	g_checksum_update (sum, (const guchar *) &uuid, sizeof (uuid));
+	g_checksum_update (sum, (const guchar *) uuid, sizeof (*uuid));
 	g_checksum_get_digest (sum, sha256_digest, &len);
 	g_checksum_free (sum);
 
diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c
index 21b87c17cc..18e3b59266 100644
--- a/src/dhcp/nm-dhcp-client.c
+++ b/src/dhcp/nm-dhcp-client.c
@@ -28,7 +28,6 @@
 #include <unistd.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <uuid/uuid.h>
 #include <linux/rtnetlink.h>
 
 #include "nm-utils/nm-dedup-multi.h"
diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c
index 3d7047c3c9..fc78a83fdc 100644
--- a/src/nm-core-utils.c
+++ b/src/nm-core-utils.c
@@ -2319,60 +2319,153 @@ nm_utils_is_specific_hostname (const char *name)
 
 /*****************************************************************************/
 
-gboolean
-nm_utils_machine_id_parse (const char *id_str, /*uuid_t*/ guchar *out_uuid)
-{
-	int i;
-	guint8 v0, v1;
-
-	if (!id_str)
-		return FALSE;
-
-	for (i = 0; i < 32; i++) {
-		if (!g_ascii_isxdigit (id_str[i]))
-			return FALSE;
-	}
-	if (id_str[i] != '\0')
-		return FALSE;
+typedef struct {
+	NMUuid bin;
+	char _nul_sentinel; /* just for safety, if somebody accidentally uses the binary in a string context. */
 
-	if (out_uuid) {
-		for (i = 0; i < 16; i++) {
-			v0 = g_ascii_xdigit_value (*(id_str++));
-			v1 = g_ascii_xdigit_value (*(id_str++));
-			out_uuid[i] = (v0 << 4) + v1;
-		}
+	/* depending on whether the string is packed or not (with/without hyphens),
+	 * it's 32 or 36 characters long (plus the trailing NUL).
+	 *
+	 * The difference is that boot-id is a valid RFC 4211 UUID and represented
+	 * as a 36 ascii string (with hyphens). The machine-id technically is not
+	 * a UUID, but just a 32 byte sequence of hexchars. */
+	char str[37];
+	bool is_fake;
+} UuidData;
+
+static UuidData *
+_uuid_data_init (UuidData *uuid_data,
+                 gboolean packed,
+                 gboolean is_fake,
+                 const NMUuid *uuid)
+{
+	nm_assert (uuid_data);
+	nm_assert (uuid);
+
+	uuid_data->bin = *uuid;
+	uuid_data->_nul_sentinel = '\0';
+	uuid_data->is_fake = is_fake;
+	if (packed) {
+		G_STATIC_ASSERT_EXPR (sizeof (uuid_data->str) >= (sizeof (*uuid) * 2 + 1));
+		_nm_utils_bin2hexstr_full (uuid,
+		                           sizeof (*uuid),
+		                           '\0',
+		                           FALSE,
+		                           uuid_data->str);
+	} else {
+		G_STATIC_ASSERT_EXPR (sizeof (uuid_data->str) >= 37);
+		_nm_utils_uuid_unparse (uuid, uuid_data->str);
 	}
-	return TRUE;
+	return uuid_data;
 }
 
-char *
-nm_utils_machine_id_read (void)
+/*****************************************************************************/
+
+static const UuidData *
+_machine_id_get (void)
 {
-	gs_free char *contents = NULL;
-	int i;
+	static const UuidData *volatile p_uuid_data;
+	const UuidData *d;
 
-	/* Get the machine ID from /etc/machine-id; it's always in /etc no matter
-	 * where our configured SYSCONFDIR is.  Alternatively, it might be in
-	 * LOCALSTATEDIR /lib/dbus/machine-id.
-	 */
-	if (   !g_file_get_contents ("/etc/machine-id", &contents, NULL, NULL)
-	    && !g_file_get_contents (LOCALSTATEDIR "/lib/dbus/machine-id", &contents, NULL, NULL))
-		return NULL;
+again:
+	d = g_atomic_pointer_get (&p_uuid_data);
+	if (G_UNLIKELY (!d)) {
+		static gsize lock;
+		static UuidData uuid_data;
+		gs_free char *content = NULL;
+		gboolean is_fake = TRUE;
+		const char *fake_type = NULL;
+		NMUuid uuid;
+
+		/* Get the machine ID from /etc/machine-id; it's always in /etc no matter
+		 * where our configured SYSCONFDIR is.  Alternatively, it might be in
+		 * LOCALSTATEDIR /lib/dbus/machine-id.
+		 */
+		if (   nm_utils_file_get_contents (-1, "/etc/machine-id", 100*1024, 0, &content, NULL, NULL) >= 0
+		    || nm_utils_file_get_contents (-1, LOCALSTATEDIR"/lib/dbus/machine-id", 100*1024, 0, &content, NULL, NULL) >= 0) {
+			g_strstrip (content);
+			if (_nm_utils_hexstr2bin_full (content,
+			                               FALSE,
+			                               FALSE,
+			                               NULL,
+			                               16,
+			                               (guint8 *) &uuid,
+			                               sizeof (uuid),
+			                               NULL)) {
+				if (!nm_utils_uuid_is_null (&uuid)) {
+					/* an all-zero machine-id is not valid. */
+					is_fake = FALSE;
+				}
+			}
+		}
 
-	contents = g_strstrip (contents);
+		if (is_fake) {
+			const guint8 *seed_bin;
+			const char *hash_seed;
+			gsize seed_len;
+
+			if (nm_utils_secret_key_get (&seed_bin, &seed_len)) {
+				/* we have no valid machine-id. Generate a fake one by hashing
+				 * the secret-key. This key is commonly persisted, so it should be
+				 * stable accross reboots (despite having a broken system without
+				 * proper machine-id). */
+				fake_type = "secret-key";
+				hash_seed = "ab085f06-b629-46d1-a553-84eeba5683b6";
+			} else {
+				/* the secret-key is not valid/persistent either. That happens when we fail
+				 * to read/write the secret-key to disk. Fallback to boot-id. The boot-id
+				 * itself may be fake and randomly generated ad-hoc, but that is as best
+				 * as it gets.  */
+				seed_bin = (const guint8 *) nm_utils_get_boot_id_bin ();
+				seed_len = sizeof (NMUuid);
+				fake_type = "boot-id";
+				hash_seed = "7ff0c8f5-5399-4901-ab63-61bf594abe8b";
+			}
 
-	for (i = 0; i < 32; i++) {
-		if (!g_ascii_isxdigit (contents[i]))
-			return NULL;
-		if (contents[i] >= 'A' && contents[i] <= 'F') {
-			/* canonicalize to lower-case */
-			contents[i] = 'a' + (contents[i] - 'A');
+			/* the fake machine-id is based on secret-key/boot-id, but we hash it
+			 * again, so that they are not literally the same. */
+			nm_utils_uuid_generate_from_string_bin (&uuid,
+			                                        (const char *) seed_bin,
+			                                        seed_len,
+			                                        NM_UTILS_UUID_TYPE_VERSION5,
+			                                        (gpointer) hash_seed);
 		}
+
+		if (!g_once_init_enter (&lock))
+			goto again;
+
+		d = _uuid_data_init (&uuid_data, TRUE, is_fake, &uuid);
+		g_atomic_pointer_set (&p_uuid_data, d);
+		g_once_init_leave (&lock, 1);
+
+		if (is_fake) {
+			nm_log_err (LOGD_CORE,
+			            "/etc/machine-id: no valid machine-id. Use fake one based on %s: %s",
+			            fake_type,
+			            d->str);
+		} else
+			nm_log_dbg (LOGD_CORE, "/etc/machine-id: %s", d->str);
 	}
-	if (contents[i] != '\0')
-		return NULL;
 
-	return g_steal_pointer (&contents);
+	return d;
+}
+
+const char *
+nm_utils_machine_id_str (void)
+{
+	return _machine_id_get ()->str;
+}
+
+const NMUuid *
+nm_utils_machine_id_bin (void)
+{
+	return &_machine_id_get ()->bin;
+}
+
+gboolean
+nm_utils_machine_id_is_fake (void)
+{
+	return _machine_id_get ()->is_fake;
 }
 
 /*****************************************************************************/
@@ -2443,9 +2536,10 @@ gboolean
 nm_utils_secret_key_get (const guint8 **out_secret_key,
                          gsize *out_key_len)
 {
-	static volatile const SecretKeyData *secret_key_static;
+	static const SecretKeyData *volatile secret_key_static;
 	const SecretKeyData *secret_key;
 
+again:
 	secret_key = g_atomic_pointer_get (&secret_key_static);
 	if (G_UNLIKELY (!secret_key)) {
 		static gsize init_value = 0;
@@ -2455,20 +2549,15 @@ nm_utils_secret_key_get (const guint8 **out_secret_key,
 		gsize tmp_key_len;
 
 		tmp_success = _secret_key_read (&tmp_secret_key, &tmp_key_len);
-		if (g_once_init_enter (&init_value)) {
-			secret_key_data.secret_key = tmp_secret_key;
-			secret_key_data.key_len = tmp_key_len;
-			secret_key_data.is_good = tmp_success;
-
-			if (g_atomic_pointer_compare_and_exchange (&secret_key_static, NULL, &secret_key_data)) {
-				g_steal_pointer (&tmp_secret_key);
-				secret_key = &secret_key_data;
-			}
+		if (!g_once_init_enter (&init_value))
+			goto again;
 
-			g_once_init_leave (&init_value, 1);
-		}
-		if (!secret_key)
-			secret_key = g_atomic_pointer_get (&secret_key_static);
+		secret_key_data.secret_key = g_steal_pointer (&tmp_secret_key);
+		secret_key_data.key_len = tmp_key_len;
+		secret_key_data.is_good = tmp_success;
+		secret_key = &secret_key_data;
+		g_atomic_pointer_set (&secret_key_static, secret_key);
+		g_once_init_leave (&init_value, 1);
 	}
 
 	*out_secret_key = secret_key->secret_key;
@@ -2494,34 +2583,52 @@ nm_utils_secret_key_get_timestamp (void)
 
 /*****************************************************************************/
 
-const char *
-nm_utils_get_boot_id (void)
+static const UuidData *
+_boot_id_get (void)
 {
-	static const char *boot_id;
+	static const UuidData *volatile p_boot_id;
+	const UuidData *d;
 
-	if (G_UNLIKELY (!boot_id)) {
+again:
+	d = g_atomic_pointer_get (&p_boot_id);
+	if (G_UNLIKELY (!d)) {
+		static gsize lock;
+		static UuidData boot_id;
 		gs_free char *contents = NULL;
+		NMUuid uuid;
+		gboolean is_fake = FALSE;
 
 		nm_utils_file_get_contents (-1, "/proc/sys/kernel/random/boot_id", 0,
 		                            NM_UTILS_FILE_GET_CONTENTS_FLAG_NONE,
 		                            &contents, NULL, NULL);
-		if (contents) {
-			g_strstrip (contents);
-			if (contents[0]) {
-				/* clone @contents because we keep @boot_id until the program
-				 * ends.
-				 * nm_utils_file_get_contents() likely allocated a larger
-				 * buffer chunk initially and (although using realloc to shrink
-				 * the buffer) it might not be best to keep this memory
-				 * around. */
-				boot_id = g_strdup (contents);
-			}
+		if (   !contents
+		    || !_nm_utils_uuid_parse (nm_strstrip (contents), &uuid)) {
+			/* generate a random UUID instead. */
+			is_fake = TRUE;
+			_nm_utils_uuid_generate_random (&uuid);
 		}
-		if (!boot_id)
-			boot_id = nm_utils_uuid_generate ();
+
+		if (!g_once_init_enter (&lock))
+			goto again;
+
+		d = _uuid_data_init (&boot_id, FALSE, is_fake, &uuid);
+		g_atomic_pointer_set (&p_boot_id, d);
+		g_once_init_leave (&lock, 1);
 	}
 
-	return boot_id;
+	return d;
+}
+
+const char *
+nm_utils_get_boot_id_str (void)
+{
+	return _boot_id_get ()->str;
+}
+
+const NMUuid *
+nm_utils_get_boot_id_bin (void)
+{
+	return &_boot_id_get ()->bin;
 }
 
 /*****************************************************************************/
@@ -2850,7 +2957,7 @@ nm_utils_stable_id_parse (const char *stable_id,
 		if (CHECK_PREFIX ("${CONNECTION}"))
 			_stable_id_append (str, uuid);
 		else if (CHECK_PREFIX ("${BOOT}"))
-			_stable_id_append (str, bootid ?: nm_utils_get_boot_id ());
+			_stable_id_append (str, bootid ?: nm_utils_get_boot_id_str ());
 		else if (CHECK_PREFIX ("${DEVICE}"))
 			_stable_id_append (str, deviceid);
 		else if (g_str_has_prefix (&stable_id[i], "${RANDOM}")) {
diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h
index af651a2afb..a811b0f2c4 100644
--- a/src/nm-core-utils.h
+++ b/src/nm-core-utils.h
@@ -269,15 +269,19 @@ gboolean nm_utils_sysctl_ip_conf_is_path (int addr_family, const char *path, con
 
 gboolean nm_utils_is_specific_hostname (const char *name);
 
-char *nm_utils_machine_id_read (void);
-gboolean nm_utils_machine_id_parse (const char *id_str, /*uuid_t*/ guchar *out_uuid);
+struct _NMUuid;
+
+const char *nm_utils_machine_id_str (void);
+const struct _NMUuid *nm_utils_machine_id_bin (void);
+gboolean nm_utils_machine_id_is_fake (void);
+
+const char *nm_utils_get_boot_id_str (void);
+const struct _NMUuid *nm_utils_get_boot_id_bin (void);
 
 gboolean nm_utils_secret_key_get (const guint8 **out_secret_key,
                                   gsize *out_key_len);
 gint64 nm_utils_secret_key_get_timestamp (void);
 
-const char *nm_utils_get_boot_id (void);
-
 /* IPv6 Interface Identifier helpers */
 
 /**
diff --git a/src/systemd/nm-sd-utils.c b/src/systemd/nm-sd-utils.c
index 914b4b448c..c6c4c12328 100644
--- a/src/systemd/nm-sd-utils.c
+++ b/src/systemd/nm-sd-utils.c
@@ -20,9 +20,12 @@
 
 #include "nm-sd-utils.h"
 
+#include "nm-core-internal.h"
+
 #include "nm-sd-adapt.h"
 
 #include "path-util.h"
+#include "sd-id128.h"
 
 /*****************************************************************************/
 
@@ -43,3 +46,16 @@ nm_sd_utils_path_startswith (const char *path, const char *prefix)
 {
 	return path_startswith (path, prefix);
 }
+
+/*****************************************************************************/
+
+NMUuid *
+nm_sd_utils_id128_get_machine (NMUuid *out_uuid)
+{
+	g_assert (out_uuid);
+
+	G_STATIC_ASSERT_EXPR (sizeof (*out_uuid) == sizeof (sd_id128_t));
+	if (sd_id128_get_machine ((sd_id128_t *) out_uuid) < 0)
+		return NULL;
+	return out_uuid;
+}
diff --git a/src/systemd/nm-sd-utils.h b/src/systemd/nm-sd-utils.h
index 8ca920e273..fb41f9fe57 100644
--- a/src/systemd/nm-sd-utils.h
+++ b/src/systemd/nm-sd-utils.h
@@ -29,5 +29,9 @@ const char *nm_sd_utils_path_startswith (const char *path, const char *prefix);
 
 /*****************************************************************************/
 
+struct _NMUuid;
+
+struct _NMUuid *nm_sd_utils_id128_get_machine (struct _NMUuid *out_uuid);
+
 #endif /* __NM_SD_UTILS_H__ */
 
diff --git a/src/tests/test-general.c b/src/tests/test-general.c
index 2ab5f8f735..3b2cccd219 100644
--- a/src/tests/test-general.c
+++ b/src/tests/test-general.c
@@ -28,6 +28,8 @@
 
 #include "NetworkManagerUtils.h"
 #include "nm-core-internal.h"
+#include "nm-core-utils.h"
+#include "systemd/nm-sd-utils.h"
 
 #include "dns/nm-dns-manager.h"
 
@@ -1910,6 +1912,52 @@ test_dns_create_resolv_conf (void)
 
 /*****************************************************************************/
 
+static void
+test_machine_id_read (void)
+{
+	NMUuid machine_id_sd;
+	const NMUuid *machine_id;
+	char machine_id_str[33];
+	gpointer logstate;
+
+	logstate = nmtst_logging_disable (FALSE);
+	/* If you run this test as root, without a valid /etc/machine-id,
+	 * the code will try to get the secret-key (and possibly attempt
+	 * to write it).
+	 *
+	 * That's especially ugly, if you run the test as root and it writes
+	 * a new "/var/lib/NetworkManager/secret_key" file. Another reason
+	 * not to run tests as root. */
+	machine_id = nm_utils_machine_id_bin ();
+	nmtst_logging_reenable (logstate);
+
+	g_assert (machine_id);
+	g_assert (_nm_utils_bin2hexstr_full (machine_id,
+	                                     sizeof (NMUuid),
+	                                     '\0',
+	                                     FALSE,
+	                                     machine_id_str) == machine_id_str);
+	g_assert (strlen (machine_id_str) == 32);
+	g_assert_cmpstr (machine_id_str, ==, nm_utils_machine_id_str ());
+
+	/* double check with systemd's implementation... */
+	if (!nm_sd_utils_id128_get_machine (&machine_id_sd)) {
+		/* if systemd failed to read /etc/machine-id, the file likely
+		 * is invalid. Our machine-id is fake, and we have nothing to
+		 * compare against. */
+
+		/* NOTE: this test will fail, if you don't have /etc/machine-id,
+		 * but a valid "LOCALSTATEDIR/lib/dbus/machine-id" file.
+		 * Just don't do that. */
+		g_assert (nm_utils_machine_id_is_fake ());
+	} else {
+		g_assert (!nm_utils_machine_id_is_fake ());
+		g_assert_cmpmem (&machine_id_sd, sizeof (NMUuid), machine_id, 16);
+	}
+}
+
+/*****************************************************************************/
+
 NMTST_DEFINE ();
 
 int
@@ -1956,6 +2004,8 @@ main (int argc, char **argv)
 	g_test_add_func ("/general/stable-id/parse", test_stable_id_parse);
 	g_test_add_func ("/general/stable-id/generated-complete", test_stable_id_generated_complete);
 
+	g_test_add_func ("/general/machine-id/read", test_machine_id_read);
+
 	g_test_add_func ("/general/test_utils_file_is_in_path", test_utils_file_is_in_path);
 
 	g_test_add_func ("/general/test_dns_create_resolv_conf", test_dns_create_resolv_conf);