keyfile: write keyfiles to "/run" directory with ".nmconnection" file suffixth/nm-1-14-keyfile-changes

For profiles in "/etc/NetworkManager/system-connections", we did not enforce
that the keyfiles have a special suffix, nor did we generate the
filenames in such a manner. In hindsight, I think that was a mistake.

Recently we added "/run/NetworkManager/system-connections" as additional
keyfile directory. Enforce a suffix and write keyfiles with such a name.

In principle, we could also start writing keyfiles in /etc with the
same suffix. But let's not do that, because we anyway cannot enforce
it.

An ugly part is, that during `nmcli connection load` we need to
determine whether the to-be-loaded connection is under /etc or /run.
Preferably, we would allow any kind of symlinking as what matters
is the file object (inode) and not the path. Anyway, we don't do
that but compare plain paths. That means, paths which are not
in an expected form, will be rejected. In particular, the paths
starting with "/run/..." and "/var/run/..." will be treated differently,
and one of them will be rejected.

Note that ifcfg-rh plugin strictly enforces that the path
starts with IFCFG_DIR as well. So, while this is a breaking
change for keyfile, I think it's reasonable.

(cherry picked from commit 648c256b9014198aac388097e410999c68c4b452)

6 files changed, 125 insertions, 42 deletions

diff --git a/src/settings/plugins/keyfile/nms-keyfile-plugin.c b/src/settings/plugins/keyfile/nms-keyfile-plugin.c
index c9b613b8fd..346b78c0cf 100644
--- a/src/settings/plugins/keyfile/nms-keyfile-plugin.c
+++ b/src/settings/plugins/keyfile/nms-keyfile-plugin.c
@@ -322,7 +322,7 @@ dir_changed (GFileMonitor *monitor,
 	gboolean exists;
 
 	full_path = g_file_get_path (file);
-	if (nms_keyfile_utils_should_ignore_file (full_path)) {
+	if (nms_keyfile_utils_should_ignore_file (full_path, FALSE)) {
 		g_free (full_path);
 		return;
 	}
@@ -428,7 +428,9 @@ _sort_paths (const char **f1, const char **f2, GHashTable *paths)
 }
 
 static void
-_read_dir (GPtrArray *filenames, const char *path)
+_read_dir (GPtrArray *filenames,
+           const char *path,
+           gboolean require_extension)
 {
 	GDir *dir;
 	const char *item;
@@ -442,7 +444,7 @@ _read_dir (GPtrArray *filenames, const char *path)
 	}
 
 	while ((item = g_dir_read_name (dir))) {
-		if (nms_keyfile_utils_should_ignore_file (item))
+		if (nms_keyfile_utils_should_ignore_file (item, require_extension))
 			continue;
 		g_ptr_array_add (filenames, g_build_filename (path, item, NULL));
 	}
@@ -465,8 +467,8 @@ read_connections (NMSettingsPlugin *config)
 
 	filenames = g_ptr_array_new_with_free_func (g_free);
 
-	_read_dir (filenames, NM_CONFIG_KEYFILE_PATH_IN_MEMORY);
-	_read_dir (filenames, nms_keyfile_utils_get_path ());
+	_read_dir (filenames, NM_CONFIG_KEYFILE_PATH_IN_MEMORY, TRUE);
+	_read_dir (filenames, nms_keyfile_utils_get_path (), FALSE);
 
 	alive_connections = g_hash_table_new (nm_direct_hash, NULL);
 
@@ -521,13 +523,61 @@ get_connections (NMSettingsPlugin *config)
 }
 
 static gboolean
+_file_is_in_path (const char *abs_filename,
+                  const char *abs_path)
+{
+	gsize l;
+
+	/* FIXME: ensure that both paths are at least normalized (coalescing ".",
+	 * duplicate '/', and trailing '/'). */
+
+	nm_assert (abs_filename && abs_filename[0] == '/');
+	nm_assert (abs_path && abs_path[0] == '/');
+
+	l = strlen (abs_path);
+	if (strncmp (abs_filename, abs_path, l) != 0)
+		return FALSE;
+
+	abs_filename += l;
+	while (abs_filename[0] == '/')
+		abs_filename++;
+
+	if (!abs_filename[0])
+		return FALSE;
+
+	if (strchr (abs_filename, '/'))
+		return FALSE;
+
+	return TRUE;
+}
+
+static gboolean
 load_connection (NMSettingsPlugin *config,
                  const char *filename)
 {
 	NMSKeyfilePlugin *self = NMS_KEYFILE_PLUGIN ((NMSKeyfilePlugin *) config);
 	NMSKeyfileConnection *connection;
+	gboolean require_extension;
+
+	/* the test whether to require a file extension tries to figure out whether
+	 * the provided filename is inside /etc or /run.
+	 *
+	 * However, on Posix a filename just resolves to an Inode, and there can
+	 * be any kind of paths that point to the same Inode. It's not generally possible
+	 * to check for that (unless, we would stat all files in the target directory
+	 * and see whether their inode matches).
+	 *
+	 * So, when loading the file do something simpler: require that the path
+	 * starts with the well-known prefix. This rejects symlinks or hard links
+	 * which would actually also point to the same file. */
+	if (_file_is_in_path (filename, nms_keyfile_utils_get_path ()))
+		require_extension = FALSE;
+	else if (_file_is_in_path (filename, NM_CONFIG_KEYFILE_PATH_IN_MEMORY))
+		require_extension = TRUE;
+	else
+		return FALSE;
 
-	if (nms_keyfile_utils_should_ignore_file (filename))
+	if (nms_keyfile_utils_should_ignore_file (filename, require_extension))
 		return FALSE;
 
 	connection = update_connection (self, NULL, filename, find_by_path (self, filename), TRUE, NULL, NULL);
diff --git a/src/settings/plugins/keyfile/nms-keyfile-reader.c b/src/settings/plugins/keyfile/nms-keyfile-reader.c
index f417e4325f..580a857a23 100644
--- a/src/settings/plugins/keyfile/nms-keyfile-reader.c
+++ b/src/settings/plugins/keyfile/nms-keyfile-reader.c
@@ -114,6 +114,7 @@ nms_keyfile_reader_from_keyfile (GKeyFile *key_file,
 	};
 	gs_free char *base_dir_free = NULL;
 	gs_free char *profile_filename_free = NULL;
+	gs_free char *filename_id = NULL;
 	const char *profile_filename = NULL;
 
 	nm_assert (filename && filename[0]);
@@ -141,7 +142,14 @@ nms_keyfile_reader_from_keyfile (GKeyFile *key_file,
 	if (!connection)
 		return NULL;
 
-	nm_keyfile_read_ensure_id (connection, filename);
+	if (g_str_has_suffix (filename, NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION)) {
+		gsize l = strlen (filename);
+
+		if (l > NM_STRLEN (NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION))
+			filename_id = g_strndup (filename, l - NM_STRLEN (NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION));
+	}
+
+	nm_keyfile_read_ensure_id (connection, filename_id ?: filename);
 
 	if (!profile_filename) {
 		profile_filename_free = g_build_filename (profile_dir ?: base_dir, filename, NULL);
diff --git a/src/settings/plugins/keyfile/nms-keyfile-utils.c b/src/settings/plugins/keyfile/nms-keyfile-utils.c
index 3912e5d76b..c3bfcdee02 100644
--- a/src/settings/plugins/keyfile/nms-keyfile-utils.c
+++ b/src/settings/plugins/keyfile/nms-keyfile-utils.c
@@ -84,7 +84,7 @@ check_suffix (const char *base, const char *tag)
 #define DER_TAG ".der"
 
 gboolean
-nms_keyfile_utils_should_ignore_file (const char *filename)
+nms_keyfile_utils_should_ignore_file (const char *filename, gboolean require_extension)
 {
 	gs_free char *base = NULL;
 
@@ -104,6 +104,14 @@ nms_keyfile_utils_should_ignore_file (const char *filename)
 	if (check_suffix (base, PEM_TAG) || check_suffix (base, DER_TAG))
 		return TRUE;
 
+	if (require_extension) {
+		gsize l = strlen (base);
+
+		if (   l <= NM_STRLEN (NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION)
+		    || !g_str_has_suffix (base, NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION))
+			return TRUE;
+	}
+
 	return FALSE;
 }
 
@@ -167,14 +175,16 @@ nms_keyfile_utils_check_file_permissions (const char *filename,
 /*****************************************************************************/
 
 char *
-nms_keyfile_utils_escape_filename (const char *filename)
+nms_keyfile_utils_escape_filename (const char *filename,
+                                   gboolean with_extension)
 {
 	GString *str;
 	const char *f = filename;
-	const char ESCAPE_CHAR = '*';
-
 	/* keyfile used to escape with '*', do not change that behavior.
-	 * But for newly added escapings, use '_' instead. */
+	 *
+	 * But for newly added escapings, use '_' instead.
+	 * Also, @with_extension is new-style. */
+	const char ESCAPE_CHAR = with_extension ? '_' : '*';
 	const char ESCAPE_CHAR2 = '_';
 
 	g_return_val_if_fail (filename && filename[0], NULL);
@@ -200,6 +210,9 @@ nms_keyfile_utils_escape_filename (const char *filename)
 	    || check_suffix (str->str, DER_TAG))
 		g_string_append_c (str, ESCAPE_CHAR2);
 
+	if (with_extension)
+		g_string_append (str, NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION);
+
 	return g_string_free (str, FALSE);;
 }
 
diff --git a/src/settings/plugins/keyfile/nms-keyfile-utils.h b/src/settings/plugins/keyfile/nms-keyfile-utils.h
index 13a3eb009b..297dd4eac3 100644
--- a/src/settings/plugins/keyfile/nms-keyfile-utils.h
+++ b/src/settings/plugins/keyfile/nms-keyfile-utils.h
@@ -25,15 +25,17 @@
 
 #define NM_CONFIG_KEYFILE_PATH_IN_MEMORY NMRUNDIR "/system-connections"
 
+#define NMS_KEYFILE_PATH_SUFFIX_NMCONNECTION      ".nmconnection"
+
 #define NMS_KEYFILE_CONNECTION_LOG_PATH(path)  ((path) ?: "in-memory")
 #define NMS_KEYFILE_CONNECTION_LOG_FMT         "%s (%s,\"%s\")"
 #define NMS_KEYFILE_CONNECTION_LOG_ARG(con)    NMS_KEYFILE_CONNECTION_LOG_PATH (nm_settings_connection_get_filename ((NMSettingsConnection *) (con))), nm_settings_connection_get_uuid ((NMSettingsConnection *) (con)), nm_settings_connection_get_id ((NMSettingsConnection *) (con))
 #define NMS_KEYFILE_CONNECTION_LOG_FMTD        "%s (%s,\"%s\",%p)"
 #define NMS_KEYFILE_CONNECTION_LOG_ARGD(con)   NMS_KEYFILE_CONNECTION_LOG_PATH (nm_settings_connection_get_filename ((NMSettingsConnection *) (con))), nm_settings_connection_get_uuid ((NMSettingsConnection *) (con)), nm_settings_connection_get_id ((NMSettingsConnection *) (con)), (con)
 
-gboolean nms_keyfile_utils_should_ignore_file (const char *filename);
+gboolean nms_keyfile_utils_should_ignore_file (const char *filename, gboolean require_extension);
 
-char *nms_keyfile_utils_escape_filename (const char *filename);
+char *nms_keyfile_utils_escape_filename (const char *filename, gboolean with_extension);
 
 const char *nms_keyfile_utils_get_path (void);
 
diff --git a/src/settings/plugins/keyfile/nms-keyfile-writer.c b/src/settings/plugins/keyfile/nms-keyfile-writer.c
index 6b7c0019c5..fc836d7d27 100644
--- a/src/settings/plugins/keyfile/nms-keyfile-writer.c
+++ b/src/settings/plugins/keyfile/nms-keyfile-writer.c
@@ -173,6 +173,7 @@ static gboolean
 _internal_write_connection (NMConnection *connection,
                             const char *keyfile_dir,
                             const char *profile_dir,
+                            gboolean with_extension,
                             uid_t owner_uid,
                             pid_t owner_grp,
                             const char *existing_path,
@@ -229,7 +230,7 @@ _internal_write_connection (NMConnection *connection,
 	if (existing_path != NULL && !rename) {
 		path = g_strdup (existing_path);
 	} else {
-		char *filename_escaped = nms_keyfile_utils_escape_filename (id);
+		char *filename_escaped = nms_keyfile_utils_escape_filename (id, with_extension);
 
 		path = g_build_filename (keyfile_dir, filename_escaped, NULL);
 		g_free (filename_escaped);
@@ -255,7 +256,7 @@ _internal_write_connection (NMConnection *connection,
 			else
 				filename = g_strdup_printf ("%s-%s-%u", id, nm_connection_get_uuid (connection), i);
 
-			filename_escaped = nms_keyfile_utils_escape_filename (filename);
+			filename_escaped = nms_keyfile_utils_escape_filename (filename, with_extension);
 
 			g_free (path);
 			path = g_strdup_printf ("%s/%s", keyfile_dir, filename_escaped);
@@ -351,16 +352,21 @@ nms_keyfile_writer_connection (NMConnection *connection,
                                GError **error)
 {
 	const char *keyfile_dir;
+	gboolean with_extension = FALSE;
 
 	if (save_to_disk)
 		keyfile_dir = nms_keyfile_utils_get_path ();
-	else
+	else {
 		keyfile_dir = NM_CONFIG_KEYFILE_PATH_IN_MEMORY;
+		with_extension = TRUE;
+	}
 
 	return _internal_write_connection (connection,
 	                                   keyfile_dir,
 	                                   nms_keyfile_utils_get_path (),
-	                                   0, 0,
+	                                   with_extension,
+	                                   0,
+	                                   0,
 	                                   existing_path,
 	                                   force_rename,
 	                                   out_path,
@@ -382,7 +388,9 @@ nms_keyfile_writer_test_connection (NMConnection *connection,
 	return _internal_write_connection (connection,
 	                                   keyfile_dir,
 	                                   keyfile_dir,
-	                                   owner_uid, owner_grp,
+	                                   FALSE,
+	                                   owner_uid,
+	                                   owner_grp,
 	                                   NULL,
 	                                   FALSE,
 	                                   out_path,
diff --git a/src/settings/plugins/keyfile/tests/test-keyfile.c b/src/settings/plugins/keyfile/tests/test-keyfile.c
index 80c06fb752..4a0e01b3f3 100644
--- a/src/settings/plugins/keyfile/tests/test-keyfile.c
+++ b/src/settings/plugins/keyfile/tests/test-keyfile.c
@@ -2460,49 +2460,51 @@ test_write_tc_config (void)
 /*****************************************************************************/
 
 static void
-_escape_filename (const char *filename, gboolean would_be_ignored)
+_escape_filename (gboolean with_extension, const char *filename, gboolean would_be_ignored)
 {
 	gs_free char *esc = NULL;
 
 	g_assert (filename && filename[0]);
 
-	if (!!would_be_ignored != !!nms_keyfile_utils_should_ignore_file (filename)) {
+	if (!!would_be_ignored != !!nms_keyfile_utils_should_ignore_file (filename, with_extension)) {
 		if (would_be_ignored)
 			g_error ("We expect filename \"%s\" to be ignored, but it isn't", filename);
 		else
 			g_error ("We expect filename \"%s\" not to be ignored, but it is", filename);
 	}
 
-	esc = nms_keyfile_utils_escape_filename (filename);
+	esc = nms_keyfile_utils_escape_filename (filename, with_extension);
 	g_assert (esc && esc[0]);
 	g_assert (!strchr (esc, '/'));
 
-	if (nms_keyfile_utils_should_ignore_file (esc))
+	if (nms_keyfile_utils_should_ignore_file (esc, with_extension))
 		g_error ("Escaping filename \"%s\" yielded \"%s\", but this is ignored", filename, esc);
 }
 
 static void
 test_nm_keyfile_plugin_utils_escape_filename (void)
 {
-	_escape_filename ("ab", FALSE);
-	_escape_filename (".vim-file.swp", TRUE);
-	_escape_filename (".vim-file.Swp", TRUE);
-	_escape_filename (".vim-file.SWP", TRUE);
-	_escape_filename (".vim-file.swpx", TRUE);
-	_escape_filename (".vim-file.Swpx", TRUE);
-	_escape_filename (".vim-file.SWPX", TRUE);
-	_escape_filename (".pem-file.pem", TRUE);
-	_escape_filename (".pem-file.Pem", TRUE);
-	_escape_filename (".pem-file.PEM", TRUE);
-	_escape_filename (".pem-file.der", TRUE);
-	_escape_filename (".pem-file.Der", TRUE);
-	_escape_filename (".mkstemp.ABCEDF", TRUE);
-	_escape_filename (".mkstemp.abcdef", TRUE);
-	_escape_filename (".mkstemp.123456", TRUE);
-	_escape_filename (".mkstemp.A23456", TRUE);
-	_escape_filename (".#emacs-locking", TRUE);
-	_escape_filename ("file-with-tilde~", TRUE);
-	_escape_filename (".file-with-dot", TRUE);
+	_escape_filename (FALSE, "ab", FALSE);
+	_escape_filename (FALSE, ".vim-file.swp", TRUE);
+	_escape_filename (FALSE, ".vim-file.Swp", TRUE);
+	_escape_filename (FALSE, ".vim-file.SWP", TRUE);
+	_escape_filename (FALSE, ".vim-file.swpx", TRUE);
+	_escape_filename (FALSE, ".vim-file.Swpx", TRUE);
+	_escape_filename (FALSE, ".vim-file.SWPX", TRUE);
+	_escape_filename (FALSE, ".pem-file.pem", TRUE);
+	_escape_filename (FALSE, ".pem-file.Pem", TRUE);
+	_escape_filename (FALSE, ".pem-file.PEM", TRUE);
+	_escape_filename (FALSE, ".pem-file.der", TRUE);
+	_escape_filename (FALSE, ".pem-file.Der", TRUE);
+	_escape_filename (FALSE, ".mkstemp.ABCEDF", TRUE);
+	_escape_filename (FALSE, ".mkstemp.abcdef", TRUE);
+	_escape_filename (FALSE, ".mkstemp.123456", TRUE);
+	_escape_filename (FALSE, ".mkstemp.A23456", TRUE);
+	_escape_filename (FALSE, ".#emacs-locking", TRUE);
+	_escape_filename (FALSE, "file-with-tilde~", TRUE);
+	_escape_filename (FALSE, ".file-with-dot", TRUE);
+
+	_escape_filename (TRUE, "lala", TRUE);
 }
 
 /*****************************************************************************/