core: let nm_utils_secret_key_read() handle failures internally

and add nm_utils_secret_key_get() to cache the secret-key, to only
obtain it once.

nm_utils_secret_key_read() is not expected to fail. However, in case
of an unexpected error, don't propagate the error to the caller,
but instead handle it internally.

That means, in case of error:
  - log a warning within nm_utils_secret_key_read() itself.
  - always return a generated secret-key. In case of error, the
    key won't be persisted (obviously). But the caller can ignore
    the error and just proceed with an in-memory key.

Hence, also add nm_utils_secret_key_get() to cache the key. This way,
we only try to actually generate/read the secret-key once.  Since that
might fail and return an in-memory key, we must for future invocations
return the same key, without generating a new one.

2 files changed, 93 insertions, 50 deletions

diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c
index 9697b9bc38..50df2b5c2e 100644
--- a/src/nm-core-utils.c
+++ b/src/nm-core-utils.c
@@ -2797,57 +2797,103 @@ nm_utils_file_get_contents (int dirfd,
 
 /*****************************************************************************/
 
-guint8 *
-nm_utils_secret_key_read (gsize *out_key_len, GError **error)
+static gboolean
+_secret_key_read (guint8 **out_secret_key,
+                  gsize *out_key_len)
 {
-	guint8 *secret_key = NULL;
+	guint8 *secret_key;
+	gboolean success = TRUE;
 	gsize key_len;
-
-	/* out_key_len is not optional, because without it you cannot safely
-	 * access the returned memory. */
-	*out_key_len = 0;
+	gs_free_error GError *error = NULL;
 
 	/* Let's try to load a saved secret key first. */
-	if (g_file_get_contents (NMSTATEDIR "/secret_key", (char **) &secret_key, &key_len, NULL)) {
-		if (key_len < 16) {
-			g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-			                     "Key is too short to be usable");
-			key_len = 0;
-		}
+	if (g_file_get_contents (NMSTATEDIR "/secret_key", (char **) &secret_key, &key_len, &error)) {
+		if (key_len >= 16)
+			goto out;
+
+		/* the secret key is borked. Log a warning, but proceed below to generate
+		 * a new one. */
+		nm_log_warn (LOGD_CORE, "secret-key: too short secret key in \"%s\" (generate new key)", NMSTATEDIR "/secret_key");
+		nm_clear_g_free (&secret_key);
 	} else {
-		mode_t key_mask;
+		if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) {
+			nm_log_warn (LOGD_CORE, "secret-key: failure reading secret key in \"%s\": %s (generate new key)",
+			             NMSTATEDIR "/secret_key", error->message);
+		}
+		g_clear_error (&error);
+	}
 
-		/* RFC7217 mandates the key SHOULD be at least 128 bits.
-		 * Let's use twice as much. */
-		key_len = 32;
-		secret_key = g_malloc (key_len + 1);
+	/* RFC7217 mandates the key SHOULD be at least 128 bits.
+	 * Let's use twice as much. */
+	key_len = 32;
+	secret_key = g_malloc (key_len + 1);
 
-		if (!nm_utils_random_bytes (secret_key, key_len)) {
-			g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-			             "Can't get random data to generate secret key");
-			key_len = 0;
-			goto out;
-		}
+	/* the secret-key is binary. Still, ensure that it's NULL terminated, just like
+	 * g_file_set_contents() does. */
+	secret_key[32] = '\0';
 
-		/* the secret-key is binary. Still, ensure that it's NULL terminated, just like
-		 * g_file_set_contents() does. */
-		secret_key[32] = '\0';
+	if (!nm_utils_random_bytes (secret_key, key_len)) {
+		nm_log_warn (LOGD_CORE, "secret-key: failure to generate good random data for secret-key (use non-persistent key)");
+		success = FALSE;
+		goto out;
+	}
 
-		key_mask = umask (0077);
-		if (!g_file_set_contents (NMSTATEDIR "/secret_key", (char *) secret_key, key_len, error)) {
-			g_prefix_error (error, "Can't write " NMSTATEDIR "/secret_key: ");
-			key_len = 0;
-		}
-		umask (key_mask);
+	if (!nm_utils_file_set_contents (NMSTATEDIR "/secret_key", (char *) secret_key, key_len, 0077, &error)) {
+		nm_log_warn (LOGD_CORE, "secret-key: failure to persist secret key in \"%s\" (%s) (use non-persistent key)",
+		             NMSTATEDIR "/secret_key", error->message);
+		success = FALSE;
+		goto out;
 	}
 
 out:
-	if (key_len) {
-		*out_key_len = key_len;
-		return secret_key;
+	/* regardless of success or failue, we always return a secret-key. The
+	 * caller may choose to ignore the error and proceed. */
+	*out_key_len = key_len;
+	*out_secret_key = secret_key;
+	return success;
+}
+
+typedef struct {
+	const guint8 *secret_key;
+	gsize key_len;
+	bool is_good:1;
+} SecretKeyData;
+
+gboolean
+nm_utils_secret_key_get (const guint8 **out_secret_key,
+                         gsize *out_key_len)
+{
+	static volatile const SecretKeyData *secret_key_static;
+	const SecretKeyData *secret_key;
+
+	secret_key = g_atomic_pointer_get (&secret_key_static);
+	if (G_UNLIKELY (!secret_key)) {
+		static gsize init_value = 0;
+		static SecretKeyData secret_key_data;
+		gboolean tmp_success;
+		gs_free guint8 *tmp_secret_key = NULL;
+		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;
+			}
+
+			g_once_init_leave (&init_value, 1);
+		}
+		if (!secret_key)
+			secret_key = g_atomic_pointer_get (&secret_key_static);
 	}
-	g_free (secret_key);
-	return NULL;
+
+	*out_secret_key = secret_key->secret_key;
+	*out_key_len = secret_key->key_len;
+	return secret_key->is_good;
 }
 
 /*****************************************************************************/
@@ -3287,7 +3333,7 @@ _set_stable_privacy (NMUtilsStableType stable_type,
                      const char *ifname,
                      const char *network_id,
                      guint32 dad_counter,
-                     guint8 *secret_key,
+                     const guint8 *secret_key,
                      gsize key_len,
                      GError **error)
 {
@@ -3386,8 +3432,8 @@ nm_utils_ipv6_addr_set_stable_privacy (NMUtilsStableType stable_type,
                                        guint32 dad_counter,
                                        GError **error)
 {
-	gs_free guint8 *secret_key = NULL;
-	gsize key_len = 0;
+	const guint8 *secret_key;
+	gsize key_len;
 
 	g_return_val_if_fail (network_id, FALSE);
 
@@ -3397,9 +3443,7 @@ nm_utils_ipv6_addr_set_stable_privacy (NMUtilsStableType stable_type,
 		return FALSE;
 	}
 
-	secret_key = nm_utils_secret_key_read (&key_len, error);
-	if (!secret_key)
-		return FALSE;
+	nm_utils_secret_key_get (&secret_key, &key_len);
 
 	return _set_stable_privacy (stable_type, addr, ifname, network_id, dad_counter,
 	                            secret_key, key_len, error);
@@ -3535,14 +3579,12 @@ nm_utils_hw_addr_gen_stable_eth (NMUtilsStableType stable_type,
                                  const char *current_mac_address,
                                  const char *generate_mac_address_mask)
 {
-	gs_free guint8 *secret_key = NULL;
-	gsize key_len = 0;
+	const guint8 *secret_key;
+	gsize key_len;
 
 	g_return_val_if_fail (stable_id, NULL);
 
-	secret_key = nm_utils_secret_key_read (&key_len, NULL);
-	if (!secret_key)
-		return NULL;
+	nm_utils_secret_key_get (&secret_key, &key_len);
 
 	return _hw_addr_gen_stable_eth (stable_type,
 	                                stable_id,
diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h
index a9a70038f9..ed66a7f0c8 100644
--- a/src/nm-core-utils.h
+++ b/src/nm-core-utils.h
@@ -283,7 +283,8 @@ gboolean nm_utils_file_set_contents (const gchar *filename,
 char *nm_utils_machine_id_read (void);
 gboolean nm_utils_machine_id_parse (const char *id_str, /*uuid_t*/ guchar *out_uuid);
 
-guint8 *nm_utils_secret_key_read (gsize *out_key_len, GError **error);
+gboolean nm_utils_secret_key_get (const guint8 **out_secret_key,
+                                  gsize *out_key_len);
 
 const char *nm_utils_get_boot_id (void);