Use '.module' extension on module configs

 * And want alphanumeric/_.- filenames
 * Currently this is just a warning, soon it will be enforced
 * The name of a module does not include the extension

Andreas Metzler and Ubuntu both worked on this patch, and I've made
some more changes.

See https://bugs.launchpad.net/ubuntu/+source/p11-kit/+bug/911436

https://bugs.freedesktop.org/show_bug.cgi?id=52158

10 files changed, 82 insertions, 20 deletions

diff --git a/doc/p11-kit-config.xml b/doc/p11-kit-config.xml
index 11fb41f..a10cb7e 100644
--- a/doc/p11-kit-config.xml
+++ b/doc/p11-kit-config.xml
@@ -113,8 +113,11 @@ critical: yes
 		<title>Module Configuration</title>
 
 		<para>Each configured PKCS#11 module has its own config file. These files
-		can be <link linkend="config-locations">placed in various locations</link>.
-		Most importantly each config file specifies the path of the PKCS#11 module to
+		can be <link linkend="config-locations">placed in various locations</link>.</para>
+		<para>The filename of the configuration file may consist of upper and lowercase letters
+		underscore, comma, dash and dots. The first characters needs to be an alphanumeric,
+		the filename should end with a <literal>.module</literal> extension.</para>
+		<para>Most importantly each config file specifies the path of the PKCS#11 module to
 		load. A module config file has the following fields:</para>
 
 		<variablelist>
diff --git a/p11-kit/conf.c b/p11-kit/conf.c
index fdb591d..db730a7 100644
--- a/p11-kit/conf.c
+++ b/p11-kit/conf.c
@@ -458,6 +458,46 @@ finished:
 	return result;
 }
 
+static char *
+calc_name_from_filename (const char *fname)
+{
+	/* We eventually want to settle on .module */
+	static const char *suffix = ".module";
+	static size_t suffix_len = 7;
+	const char *c = fname;
+	size_t fname_len;
+	size_t name_len;
+	char *name;
+
+	assert (fname);
+
+	/* Make sure the filename starts with an alphanumeric */
+	if (!isalnum(*c))
+		return NULL;
+	++c;
+
+	/* Only allow alnum, _, -, and . */
+	while (*c) {
+		if (!isalnum(*c) && *c != '_' && *c != '-' && *c != '.')
+			return NULL;
+		++c;
+	}
+
+	/* Make sure we have one of the suffixes */
+	fname_len = strlen (fname);
+	if (suffix_len >= fname_len)
+		return NULL;
+	name_len = (fname_len - suffix_len);
+	if (strcmp (fname + name_len, suffix) != 0)
+		return NULL;
+
+	name = malloc (name_len + 1);
+	return_val_if_fail (name != NULL, NULL);
+	memcpy (name, fname, name_len);
+	name[name_len] = 0;
+	return name;
+}
+
 static int
 load_config_from_file (const char *configfile, const char *name, hashmap *configs)
 {
@@ -468,20 +508,28 @@ load_config_from_file (const char *configfile, const char *name, hashmap *config
 
 	assert (configfile);
 
+	key = calc_name_from_filename (name);
+	if (key == NULL) {
+		_p11_message ("invalid config filename, will be ignored in the future: %s", configfile);
+		key = strdup (name);
+		return_val_if_fail (key != NULL, -1);
+	}
+
 	config = _p11_conf_parse_file (configfile, 0);
-	if (!config)
+	if (!config) {
+		free (key);
 		return -1;
+	}
 
-	prev = _p11_hash_get (configs, name);
+	prev = _p11_hash_get (configs, key);
 	if (prev == NULL) {
-		key = strdup (name);
-		return_val_if_fail (key != NULL, -1);
 		if (!_p11_hash_set (configs, key, config))
 			return_val_if_reached (-1);
 		config = NULL;
 	} else {
 		if (_p11_conf_merge_defaults (prev, config) < 0)
 			error = errno;
+		free (key);
 	}
 
 	/* If still set */
diff --git a/tests/conf-test.c b/tests/conf-test.c
index eb04c71..92f7205 100644
--- a/tests/conf-test.c
+++ b/tests/conf-test.c
@@ -246,6 +246,15 @@ test_load_globals_user_sets_invalid (CuTest *tc)
 	_p11_hash_free (config);
 }
 
+static int
+assert_msg_contains (const char *msg,
+                     const char *text)
+{
+	if (msg == NULL)
+		return 0;
+	return strstr (msg, text) ? 1 : 0;
+}
+
 static void
 test_load_modules_merge (CuTest *tc)
 {
@@ -258,14 +267,14 @@ test_load_modules_merge (CuTest *tc)
 	                                  SRCDIR "/files/system-modules",
 	                                  SRCDIR "/files/user-modules");
 	CuAssertPtrNotNull (tc, configs);
-	CuAssertStrEquals (tc, NULL, p11_kit_message ());
+	CuAssertTrue (tc, assert_msg_contains (p11_kit_message (), "invalid config filename"));
 
 	config = _p11_hash_get (configs, "one");
 	CuAssertPtrNotNull (tc, config);
 	CuAssertStrEquals (tc, "mock-one.so", _p11_hash_get (config, "module"));
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "user1");
 
-	config = _p11_hash_get (configs, "two");
+	config = _p11_hash_get (configs, "two.badname");
 	CuAssertPtrNotNull (tc, config);
 	CuAssertStrEquals (tc, "mock-two.so", _p11_hash_get (config, "module"));
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system2");
@@ -290,14 +299,14 @@ test_load_modules_user_none (CuTest *tc)
 	                                  SRCDIR "/files/system-modules",
 	                                  SRCDIR "/files/user-modules");
 	CuAssertPtrNotNull (tc, configs);
-	CuAssertStrEquals (tc, NULL, p11_kit_message ());
+	CuAssertTrue (tc, assert_msg_contains (p11_kit_message (), "invalid config filename"));
 
 	config = _p11_hash_get (configs, "one");
 	CuAssertPtrNotNull (tc, config);
 	CuAssertStrEquals (tc, "mock-one.so", _p11_hash_get (config, "module"));
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system1");
 
-	config = _p11_hash_get (configs, "two");
+	config = _p11_hash_get (configs, "two.badname");
 	CuAssertPtrNotNull (tc, config);
 	CuAssertStrEquals (tc, "mock-two.so", _p11_hash_get (config, "module"));
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system2");
@@ -327,7 +336,7 @@ test_load_modules_user_only (CuTest *tc)
 	CuAssertStrEquals (tc, _p11_hash_get (config, "module"), NULL);
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "user1");
 
-	config = _p11_hash_get (configs, "two");
+	config = _p11_hash_get (configs, "two.badname");
 	CuAssertPtrEquals (tc, NULL, config);
 
 	config = _p11_hash_get (configs, "three");
@@ -350,14 +359,14 @@ test_load_modules_no_user (CuTest *tc)
 	                                  SRCDIR "/files/system-modules",
 	                                  SRCDIR "/files/non-existant");
 	CuAssertPtrNotNull (tc, configs);
-	CuAssertStrEquals (tc, NULL, p11_kit_message ());
+	CuAssertTrue (tc, assert_msg_contains (p11_kit_message (), "invalid config filename"));
 
 	config = _p11_hash_get (configs, "one");
 	CuAssertPtrNotNull (tc, config);
 	CuAssertStrEquals (tc, "mock-one.so", _p11_hash_get (config, "module"));
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system1");
 
-	config = _p11_hash_get (configs, "two");
+	config = _p11_hash_get (configs, "two.badname");
 	CuAssertPtrNotNull (tc, config);
 	CuAssertStrEquals (tc, "mock-two.so", _p11_hash_get (config, "module"));
 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system2");
diff --git a/tests/files/system-modules/one b/tests/files/system-modules/one.module
index 3620869..3620869 100644
--- a/tests/files/system-modules/one
+++ b/tests/files/system-modules/one.module
diff --git a/tests/files/system-modules/two b/tests/files/system-modules/two
deleted file mode 100644
index f391757..0000000
--- a/tests/files/system-modules/two
+++ /dev/null
@@ -1,5 +0,0 @@
-
-module: mock-two.so
-setting: system2
-
-disable-in: test-disable, test-other
\ No newline at end of file
diff --git a/tests/files/system-modules/two-duplicate b/tests/files/system-modules/two-duplicate.module
index 907aa75..907aa75 100644
--- a/tests/files/system-modules/two-duplicate
+++ b/tests/files/system-modules/two-duplicate.module
diff --git a/tests/files/system-modules/two.badname b/tests/files/system-modules/two.badname
new file mode 100644
index 0000000..b6d0f7f
--- /dev/null
+++ b/tests/files/system-modules/two.badname
@@ -0,0 +1,7 @@
+# This module doesn't have a .module extension, but p11-kit doesn't yet
+# enforce the naming, just warns, so it should still be loaded
+
+module: mock-two.so
+setting: system2
+
+disable-in: test-disable, test-other
\ No newline at end of file
diff --git a/tests/files/user-modules/one b/tests/files/user-modules/one.module
index c371e4a..c371e4a 100644
--- a/tests/files/user-modules/one
+++ b/tests/files/user-modules/one.module
diff --git a/tests/files/user-modules/three b/tests/files/user-modules/three.module
index 00caab5..00caab5 100644
--- a/tests/files/user-modules/three
+++ b/tests/files/user-modules/three.module
diff --git a/tests/test-modules.c b/tests/test-modules.c
index 1debed0..f755298 100644
--- a/tests/test-modules.c
+++ b/tests/test-modules.c
@@ -142,7 +142,7 @@ test_disable (CuTest *tc)
 	 */
 
 	modules = initialize_and_get_modules (tc);
-	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two") != NULL);
+	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two.badname") != NULL);
 	finalize_and_free_modules (tc, modules);
 
 	/*
@@ -155,7 +155,7 @@ test_disable (CuTest *tc)
 	p11_kit_set_progname ("test-disable");
 
 	modules = initialize_and_get_modules (tc);
-	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two") == NULL);
+	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two.badname") == NULL);
 	finalize_and_free_modules (tc, modules);
 
 	p11_kit_set_progname (NULL);