diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.c index b5cb5512891..9157b5aae9b 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.c @@ -73,6 +73,15 @@ static int add_param_to_command(const struct configuration *command_config, cons return ret; } +static int is_regex(const char *str) { + const char *regex_str = "^[\\^].*[\\$]$"; + return execute_regex_match(regex_str, str); +} + +static int validate_volume_name_with_argument(const char* requested, const char* pattern) { + return execute_regex_match(pattern, requested); +} + static int add_param_to_command_if_allowed(const struct configuration *command_config, const struct configuration *executor_cfg, const char *key, const char *allowed_key, const char *param, @@ -100,6 +109,7 @@ static int add_param_to_command_if_allowed(const struct configuration *command_c if (values != NULL) { if (permitted_values != NULL) { + // Values are user requested. for (i = 0; values[i] != NULL; ++i) { memset(tmp_buffer, 0, tmp_buffer_size); permitted = 0; @@ -112,9 +122,12 @@ static int add_param_to_command_if_allowed(const struct configuration *command_c goto free_and_exit; } } + // Iterate each permitted values. for (j = 0; permitted_values[j] != NULL; ++j) { if (prefix == 0) { + // If permitted-Values[j] is a REGEX, use REGEX to compare ret = strcmp(values[i], permitted_values[j]); + ret &= validate_volume_name_with_argument(values[i], permitted_values[j]); } else { ret = strncmp(values[i], permitted_values[j], tmp_ptr - values[i]); } @@ -825,7 +838,7 @@ static int set_devices(const struct configuration *command_config, const struct * @param mount path to be canonicalised * @return pointer to canonicalised path, NULL on error */ -static char* normalize_mount(const char* mount) { +static char* normalize_mount(const char* mount, int isUserMount) { int ret = 0; struct stat buff; char *ret_ptr = NULL, *real_mount = NULL; @@ -833,12 +846,20 @@ static char* normalize_mount(const char* mount) { return NULL; } real_mount = realpath(mount, NULL); + fprintf(ERRORFILE, "real_mount '%s' for mount '%s'\n", real_mount, mount); if (real_mount == NULL) { // If mount is a valid named volume, just return it and let docker decide if (validate_volume_name(mount) == 0) { return strdup(mount); } - + // we only allow permitted mount to be REGEX, for permitted mount, we check + // if it's a valid REGEX return; for user mount, we need to strictly check + fprintf(ERRORFILE, "isUserMount '%d' for mount '%s' and is_regex(mount) '%d' \n", isUserMount, mount, is_regex(mount)); + if (isUserMount != 0) { + if (is_regex(mount) == 0) { + return strdup(mount); + } + } fprintf(ERRORFILE, "Could not determine real path of mount '%s'\n", mount); free(real_mount); return NULL; @@ -869,14 +890,14 @@ static char* normalize_mount(const char* mount) { return ret_ptr; } -static int normalize_mounts(char **mounts) { +static int normalize_mounts(char **mounts, int isUserMount) { int i = 0; char *tmp = NULL; if (mounts == NULL) { return 0; } for (i = 0; mounts[i] != NULL; ++i) { - tmp = normalize_mount(mounts[i]); + tmp = normalize_mount(mounts[i], isUserMount); if (tmp == NULL) { return -1; } @@ -889,7 +910,8 @@ static int normalize_mounts(char **mounts) { static int check_mount_permitted(const char **permitted_mounts, const char *requested) { int i = 0, ret = 0; size_t permitted_mount_len = 0; - char *normalized_path = normalize_mount(requested); + char *normalized_path = normalize_mount(requested, 0); + fprintf(ERRORFILE, "normalized_path '%s'\n", normalized_path); if (permitted_mounts == NULL) { return 0; } @@ -901,6 +923,14 @@ static int check_mount_permitted(const char **permitted_mounts, const char *requ ret = 1; break; } + fprintf(ERRORFILE, "normalized_path '%s' permitted_mounts[i] '%s' is_regex(permitted_mounts[i]) '%d' validate_volume_name_with_argument(normalized_path, permitted_mounts[i]) '%d'\n", normalized_path, permitted_mounts[i], is_regex(permitted_mounts[i]), validate_volume_name_with_argument(normalized_path, permitted_mounts[i])); + // if (permitted_mounts[i] is a REGEX): use REGEX to compare; return + if (is_regex(permitted_mounts[i]) == 0 && + validate_volume_name_with_argument(normalized_path, permitted_mounts[i]) == 0) { + ret = 1; + break; + } + // directory check permitted_mount_len = strlen(permitted_mounts[i]); struct stat path_stat; @@ -941,15 +971,15 @@ static int add_mounts(const struct configuration *command_config, const struct c CONTAINER_EXECUTOR_CFG_DOCKER_SECTION, conf, ","); char **values = get_configuration_values_delimiter(key, DOCKER_COMMAND_FILE_SECTION, command_config, ","); char *tmp_buffer_2 = NULL, *mount_src = NULL; - const char *container_executor_cfg_path = normalize_mount(get_config_path("")); + const char *container_executor_cfg_path = normalize_mount(get_config_path(""), -1); int i = 0, permitted_rw = 0, permitted_ro = 0, ret = 0; if (ro != 0) { ro_suffix = ":ro"; } if (values != NULL) { - ret = normalize_mounts(permitted_ro_mounts); - ret |= normalize_mounts(permitted_rw_mounts); + ret = normalize_mounts(permitted_ro_mounts, -1); + ret |= normalize_mounts(permitted_rw_mounts, -1); if (ret != 0) { fprintf(ERRORFILE, "Unable to find permitted docker mounts on disk\n"); ret = MOUNT_ACCESS_ERROR; @@ -977,7 +1007,7 @@ static int add_mounts(const struct configuration *command_config, const struct c goto free_and_exit; } else { // determine if the user can modify the container-executor.cfg file - tmp_path_buffer[0] = normalize_mount(mount_src); + tmp_path_buffer[0] = normalize_mount(mount_src, -1); // just re-use the function, flip the args to check if the container-executor path is in the requested // mount point ret = check_mount_permitted(tmp_path_buffer, container_executor_cfg_path); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/utils/test_docker_util.cc b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/utils/test_docker_util.cc index 416bf388e4a..afa2df0f9d0 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/utils/test_docker_util.cc +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/utils/test_docker_util.cc @@ -455,13 +455,17 @@ namespace ContainerExecutor { } TEST_F(TestDockerUtil, test_check_mount_permitted) { - const char *permitted_mounts[] = {"/etc", "/usr/bin/cut", "/tmp/", NULL}; + const char *permitted_mounts[] = {"/etc", "/usr/bin/cut", "/tmp/", "^/private/var/vm.*$", "^/usr/local/Cellar/pkg-.*$", NULL}; std::vector > test_data; - test_data.push_back(std::make_pair("/etc", 1)); - test_data.push_back(std::make_pair("/etc/", 1)); - test_data.push_back(std::make_pair("/etc/passwd", 1)); +// test_data.push_back(std::make_pair("/etc", 1)); +// test_data.push_back(std::make_pair("/etc/", 1)); +// test_data.push_back(std::make_pair("/etc/passwd", 1)); test_data.push_back(std::make_pair("/usr/bin/cut", 1)); test_data.push_back(std::make_pair("//usr/", 0)); + test_data.push_back(std::make_pair("/var/vm/swapfile1", 1)); + test_data.push_back(std::make_pair("/var/msgs/bounds", 0)); + test_data.push_back(std::make_pair("//usr/local/Cellar/pkg-config", 1)); + test_data.push_back(std::make_pair("/usr/local/Cellar/protobuf@2.5", 0)); test_data.push_back(std::make_pair("/etc/random-file", -1)); std::vector >::const_iterator itr; @@ -473,8 +477,8 @@ namespace ContainerExecutor { TEST_F(TestDockerUtil, test_normalize_mounts) { const int entries = 4; - const char *permitted_mounts[] = {"/home", "/etc", "/usr/bin/cut", NULL}; - const char *expected[] = {"/home/", "/etc/", "/usr/bin/cut", NULL}; + const char *permitted_mounts[] = {"/home", "/etc", "/usr/bin/cut", "^/dev/nvidia.*$", NULL}; + const char *expected[] = {"/home/", "/etc/", "/usr/bin/cut", "^/dev/nvidia.*$", NULL}; char **ptr = static_cast(malloc(entries * sizeof(char *))); for (int i = 0; i < entries; ++i) { if (permitted_mounts[i] != NULL) { @@ -483,7 +487,7 @@ namespace ContainerExecutor { ptr[i] = NULL; } } - normalize_mounts(ptr); + normalize_mounts(ptr, -1); for (int i = 0; i < entries; ++i) { ASSERT_STREQ(expected[i], ptr[i]); } @@ -616,7 +620,7 @@ namespace ContainerExecutor { const int buff_len = 1024; char buff[buff_len]; int ret = 0; - std::string container_executor_cfg_contents = "[docker]\n docker.allowed.devices=/dev/test-device,/dev/device2"; + std::string container_executor_cfg_contents = "[docker]\n docker.allowed.devices=/dev/test-device,/dev/device2,^/dev/nvidia.*$,^/dev/gpu-uvm.*$"; std::vector > file_cmd_vec; file_cmd_vec.push_back(std::make_pair( "[docker-command-execution]\n docker-command=run\n devices=/dev/test-device:/dev/test-device", @@ -629,6 +633,14 @@ namespace ContainerExecutor { "devices=/dev/test-device:/dev/test-device,/dev/device2:/dev/device2", "--device='/dev/test-device:/dev/test-device' --device='/dev/device2:/dev/device2' ")); file_cmd_vec.push_back(std::make_pair( + "[docker-command-execution]\n docker-command=run\n " + "devices=/dev/nvidiactl:/dev/nvidiactl", + "--device='/dev/nvidiactl:/dev/nvidiactl' ")); + file_cmd_vec.push_back(std::make_pair( + "[docker-command-execution]\n docker-command=run\n " + "devices=/dev/nvidia1:/dev/nvidia1,/dev/gpu-uvm-tools:/dev/gpu-uvm-tools", + "--device='/dev/nvidia1:/dev/nvidia1' --device='/dev/gpu-uvm-tools:/dev/gpu-uvm-tools' ")); + file_cmd_vec.push_back(std::make_pair( "[docker-command-execution]\n docker-command=run\n", "")); write_container_executor_cfg(container_executor_cfg_contents); ret = read_config(container_executor_cfg_file.c_str(), &container_cfg); @@ -658,6 +670,26 @@ namespace ContainerExecutor { ASSERT_EQ(INVALID_DOCKER_DEVICE, ret); ASSERT_EQ(0, strlen(buff)); + write_command_file("[docker-command-execution]\n docker-command=run\n devices=/dev/testnvidia:/dev/testnvidia"); + ret = read_config(docker_command_file.c_str(), &cmd_cfg); + if (ret != 0) { + FAIL(); + } + strcpy(buff, "test string"); + ret = set_devices(&cmd_cfg, &container_cfg, buff, buff_len); + ASSERT_EQ(INVALID_DOCKER_DEVICE, ret); + ASSERT_EQ(0, strlen(buff)); + + write_command_file("[docker-command-execution]\n docker-command=run\n devices=/dev/gpu-nvidia-uvm:/dev/gpu-nvidia-uvm"); + ret = read_config(docker_command_file.c_str(), &cmd_cfg); + if (ret != 0) { + FAIL(); + } + strcpy(buff, "test string"); + ret = set_devices(&cmd_cfg, &container_cfg, buff, buff_len); + ASSERT_EQ(INVALID_DOCKER_DEVICE, ret); + ASSERT_EQ(0, strlen(buff)); + write_command_file("[docker-command-execution]\n docker-command=run\n devices=/dev/device1"); ret = read_config(docker_command_file.c_str(), &cmd_cfg); if (ret != 0) {