From 47bcb6ad0221e8ee53123ae39bb9b6ad67d395c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 5 Feb 2021 16:39:14 +0200 Subject: [PATCH] MXS-3345: Defer path permission checks Doing the directory permission checks after all the values have been set helps avoid problems with intermediate values that aren't valid. This happens when --basedir generates invalid derived paths and the correct path is provided as an argument right after it. The path parameter is read from the configuration file only if it hasn't been modified by a command line option. The case where an invalid command line option is given but a valid configuration parameter would override it is still treated as an error. Also added a clarifying comment into set_dirs to make sure the handling for the two path parameters is not moved inside set_runtime_dirs. Fixed the testing scripts for the REST API and MaxCtrl now that the directory permission checks are done correctly. Previously some paths seem to have been ignored. --- server/core/gateway.cc | 129 ++++++++++++++++--------------- test/maxscale_test.cnf | 4 + test/maxscale_test_secondary.cnf | 3 + test/start_double_maxscale.sh | 2 + 4 files changed, 74 insertions(+), 64 deletions(-) diff --git a/server/core/gateway.cc b/server/core/gateway.cc index dfe4fcdc9..87b14ed9a 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -182,23 +182,24 @@ static bool resolve_maxscale_conf_fname(char** cnf_full_path, const char* home_dir, char* cnf_file_arg); -static char* check_dir_access(char* dirname, bool, bool); -static int set_user(const char* user); -bool pid_file_exists(); -void write_child_exit_code(int fd, int code); -static bool change_cwd(); -static void log_exit_status(); -static bool daemonize(); -static bool sniff_configuration(const char* filepath); -static bool modules_process_init(); -static void modules_process_finish(); -static void disable_module_unloading(const char* arg); -static void enable_module_unloading(const char* arg); -static void enable_statement_logging(const char* arg); -static void disable_statement_logging(const char* arg); -static void redirect_output_to_file(const char* arg); -static bool user_is_acceptable(const char* specified_user); -static bool init_sqlite3(); +static bool check_dir_access(const char* dirname, bool wr, bool rd); +bool check_paths(); +static int set_user(const char* user); +bool pid_file_exists(); +void write_child_exit_code(int fd, int code); +static bool change_cwd(); +static void log_exit_status(); +static bool daemonize(); +static bool sniff_configuration(const char* filepath); +static bool modules_process_init(); +static void modules_process_finish(); +static void disable_module_unloading(const char* arg); +static void enable_module_unloading(const char* arg); +static void enable_statement_logging(const char* arg); +static void disable_statement_logging(const char* arg); +static void redirect_output_to_file(const char* arg); +static bool user_is_acceptable(const char* specified_user); +static bool init_sqlite3(); struct DEBUG_ARGUMENT { @@ -714,51 +715,32 @@ static bool resolve_maxscale_conf_fname(char** cnf_full_path, * @return NULL if directory can be read and written, an error message if either * read or write is not permitted. */ -static char* check_dir_access(char* dirname, bool rd, bool wr) +static bool check_dir_access(const char* dirname, bool rd, bool wr) { - char errbuf[PATH_MAX * 2]; - char* errstr = NULL; - - if (dirname == NULL) - { - errstr = MXS_STRDUP_A("Directory argument is NULL"); - goto retblock; - } + mxb_assert(dirname); + std::ostringstream ss; if (access(dirname, F_OK) != 0) { - snprintf(errbuf, PATH_MAX * 2 - 1, "Can't access '%s'.", dirname); - errbuf[PATH_MAX * 2 - 1] = '\0'; - errstr = MXS_STRDUP_A(errbuf); - goto retblock; + ss << "Can't access '" << dirname << "'."; } - - if (rd && !path_is_readable(dirname)) + else if (rd && access(dirname, R_OK) != 0) { - snprintf(errbuf, - PATH_MAX * 2 - 1, - "MaxScale doesn't have read permission " - "to '%s'.", - dirname); - errbuf[PATH_MAX * 2 - 1] = '\0'; - errstr = MXS_STRDUP_A(errbuf); - goto retblock; + ss << "MaxScale doesn't have read permission to '" << dirname << "'."; } - - if (wr && !path_is_writable(dirname)) + else if (wr && access(dirname, W_OK) != 0) { - snprintf(errbuf, - PATH_MAX * 2 - 1, - "MaxScale doesn't have write permission " - "to '%s'.", - dirname); - errbuf[PATH_MAX * 2 - 1] = '\0'; - errstr = MXS_STRDUP_A(errbuf); - goto retblock; + ss << "MaxScale doesn't have write permission to '" << dirname << "'."; } -retblock: - return errstr; + auto err = ss.str(); + + if (!err.empty()) + { + print_log_n_stderr(true, true, err.c_str(), err.c_str(), errno); + } + + return err.empty(); } static bool init_log() @@ -1427,6 +1409,10 @@ bool set_dirs(const char* basedir) rv = set_runtime_dirs(basedir); + // The two paths here are not inside set_runtime_dirs on purpose: they are set by --basedir but not by + // --runtimedir. The former is used with tarball installations and the latter is used to run multiple + // MaxScale instances on the same server. + if (rv && (rv = handle_path_arg(&path, basedir, MXS_DEFAULT_LIB_SUBPATH, true, false))) { set_libdir(path); @@ -1824,7 +1810,10 @@ int main(int argc, char** argv) break; case '?': - usage(); + if (check_paths()) + { + usage(); + } rc = EXIT_SUCCESS; goto return_main; @@ -2058,6 +2047,12 @@ int main(int argc, char** argv) goto return_main; } + if (!check_paths()) + { + rc = MAXSCALE_BADCONFIG; + goto return_main; + } + if (daemon_mode) { if (!change_cwd()) @@ -2690,22 +2685,28 @@ bool handle_path_arg(char** dest, const char* path, const char* arg, bool rd, bo strcat(pathbuffer, arg); } - if ((errstr = check_dir_access(pathbuffer, rd, wr)) == NULL) - { - *dest = MXS_STRDUP_A(pathbuffer); - rval = true; - } - else - { - print_log_n_stderr(true, true, errstr, errstr, 0); - MXS_FREE(errstr); - errstr = NULL; - } + *dest = MXS_STRDUP_A(pathbuffer); + rval = true; } return rval; } +bool check_paths() +{ + return check_dir_access(get_logdir(), true, false) + && check_dir_access(get_cachedir(), true, true) + && check_dir_access(get_configdir(), true, false) + && check_dir_access(get_module_configdir(), true, false) + && check_dir_access(get_datadir(), true, false) + && check_dir_access(get_langdir(), true, false) + && check_dir_access(get_piddir(), true, true) + && check_dir_access(get_config_persistdir(), true, true) + && check_dir_access(get_connector_plugindir(), true, false) + && check_dir_access(get_libdir(), true, false) + && check_dir_access(get_execdir(), true, false); +} + void set_log_augmentation(const char* value) { // Command line arguments are handled first, thus command line argument diff --git a/test/maxscale_test.cnf b/test/maxscale_test.cnf index 3006e6c45..8eb69cae6 100644 --- a/test/maxscale_test.cnf +++ b/test/maxscale_test.cnf @@ -6,6 +6,10 @@ datadir=@CMAKE_INSTALL_PREFIX@/lib/maxscale cachedir=@CMAKE_INSTALL_PREFIX@/cache/maxscale language=@CMAKE_INSTALL_PREFIX@/lib/maxscale/ piddir=@CMAKE_INSTALL_PREFIX@/run/maxscale/ +persistdir=@CMAKE_INSTALL_PREFIX@/lib/maxscale/maxscale.cnf.d/ +module_configdir=@CMAKE_INSTALL_PREFIX@/etc/maxscale.modules.d/ +# This isn't really correct but it's OK as long as it's an empty directory +connector_plugindir=@CMAKE_INSTALL_PREFIX@/etc/maxscale.modules.d/ admin_auth=false [server1] diff --git a/test/maxscale_test_secondary.cnf b/test/maxscale_test_secondary.cnf index b16a12179..76877cfcd 100644 --- a/test/maxscale_test_secondary.cnf +++ b/test/maxscale_test_secondary.cnf @@ -7,6 +7,9 @@ cachedir=@CMAKE_INSTALL_PREFIX@/secondary/cache/maxscale language=@CMAKE_INSTALL_PREFIX@/secondary/lib/maxscale/ piddir=@CMAKE_INSTALL_PREFIX@/secondary/run/maxscale persistdir=@CMAKE_INSTALL_PREFIX@/secondary/lib/maxscale/maxscale.cnf.d/ +module_configdir=@CMAKE_INSTALL_PREFIX@/secondary/etc/maxscale.modules.d/ +# This isn't really correct but it's OK as long as it's an empty directory +connector_plugindir=@CMAKE_INSTALL_PREFIX@/secondary/etc/maxscale.modules.d/ admin_auth=false admin_port=8990 diff --git a/test/start_double_maxscale.sh b/test/start_double_maxscale.sh index d4fc35e51..2bfcce292 100755 --- a/test/start_double_maxscale.sh +++ b/test/start_double_maxscale.sh @@ -20,10 +20,12 @@ rm -r $maxscaledir/secondary/run/maxscale test -f /tmp/maxadmin.sock && rm /tmp/maxadmin.sock test -f /tmp/maxadmin2.sock && rm /tmp/maxadmin2.sock +mkdir -m 0755 -p $maxscaledir/etc/maxscale.modules.d/ mkdir -m 0755 -p $maxscaledir/lib/maxscale/maxscale.cnf.d mkdir -m 0755 -p $maxscaledir/cache/maxscale mkdir -m 0755 -p $maxscaledir/run/maxscale mkdir -m 0755 -p $maxscaledir/log/maxscale +mkdir -m 0755 -p $maxscaledir/secondary/etc/maxscale.modules.d/ mkdir -m 0755 -p $maxscaledir/secondary/lib/maxscale/maxscale.cnf.d mkdir -m 0755 -p $maxscaledir/secondary/cache/maxscale mkdir -m 0755 -p $maxscaledir/secondary/run/maxscale