Re: Expand the use of check_canonical_path() for more GUCs

[Michael Paquier]

On Tue, May 19, 2020 at 09:32:15AM -0400, Tom Lane wrote:
> Hm, I'm pretty certain that data_directory does not need this because
> canonicalization is done elsewhere; the most that you could accomplish
> there is to cause problems.  Dunno about the rest.

Hmm.  I missed that this is getting done in SelectConfigFiles() first
by the postmaster so that's not necessary, which also does the work
for hba_file and ident_file.  config_file does not need that either as
AbsoluteConfigLocation() does the same work via ParseConfigFile().  So
perhaps we could add a comment or such about that?  Attached is an
idea.

The rest is made of PromoteTriggerFile, pg_krb_server_keyfile,
ssl_cert_file, ssl_key_file, ssl_ca_file, ssl_crl_file and
ssl_dh_params_file where loaded values are taken as-is, so applying
canonicalization would be helpful there, no?
--
Michael

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 2f3e0a70e0..99b66aa2a7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3781,7 +3781,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&PromoteTriggerFile,
 		"",
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{
@@ -3903,7 +3903,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&pg_krb_server_keyfile,
 		PG_KRB_SRVTAB,
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{
@@ -4188,7 +4188,8 @@ static struct config_string ConfigureNamesString[] =
 	{
 		/*
 		 * Can't be set by ALTER SYSTEM as it can lead to recursive definition
-		 * of data_directory.
+		 * of data_directory.  check_canonical_path() is not needed here as
+		 * canonicalization is done when loading configuration files.
 		 */
 		{"data_directory", PGC_POSTMASTER, FILE_LOCATIONS,
 			gettext_noop("Sets the server's data directory."),
@@ -4201,6 +4202,10 @@ static struct config_string ConfigureNamesString[] =
 	},
 
 	{
+		/*
+		 * check_canonical_path() is not needed here as canonicalization
+		 * is done when loading the configuration file.
+		 */
 		{"config_file", PGC_POSTMASTER, FILE_LOCATIONS,
 			gettext_noop("Sets the server's main configuration file."),
 			NULL,
@@ -4212,6 +4217,10 @@ static struct config_string ConfigureNamesString[] =
 	},
 
 	{
+		/*
+		 * check_canonical_path() is not needed here as canonicalization
+		 * is done when loading the configuration file.
+		 */
 		{"hba_file", PGC_POSTMASTER, FILE_LOCATIONS,
 			gettext_noop("Sets the server's \"hba\" configuration file."),
 			NULL,
@@ -4223,6 +4232,10 @@ static struct config_string ConfigureNamesString[] =
 	},
 
 	{
+		/*
+		 * check_canonical_path() is not needed here as canonicalization
+		 * is done when loading the configuration file.
+		 */
 		{"ident_file", PGC_POSTMASTER, FILE_LOCATIONS,
 			gettext_noop("Sets the server's \"ident\" configuration file."),
 			NULL,
@@ -4266,7 +4279,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&ssl_cert_file,
 		"server.crt",
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{
@@ -4276,7 +4289,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&ssl_key_file,
 		"server.key",
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{
@@ -4286,7 +4299,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&ssl_ca_file,
 		"",
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{
@@ -4296,7 +4309,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&ssl_crl_file,
 		"",
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{
@@ -4369,7 +4382,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&ssl_dh_params_file,
 		"",
-		NULL, NULL, NULL
+		check_canonical_path, NULL, NULL
 	},
 
 	{

signature.asc
Description: PGP signature