Update security environment variables (#617)

ruffsl · mikaelarguedas · web-flow · commit 1a74f8fa5d65 · 2020-04-16T13:19:42.000-03:00
Signed-off-by: ruffsl &lt;roxfoxpox@gmail.com&gt;
Co-authored-by: Mikael Arguedas &lt;mikael.arguedas@gmail.com&gt;
diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h
@@ -27,12 +27,12 @@ extern "C"
 #include "rcl/visibility_control.h"
 #include "rmw/security_options.h"
 
-#ifndef ROS_SECURITY_DIRECTORY_OVERRIDE
-# define ROS_SECURITY_DIRECTORY_OVERRIDE "ROS_SECURITY_DIRECTORY_OVERRIDE"
+#ifndef ROS_SECURITY_ENCLAVE_OVERRIDE
+# define ROS_SECURITY_ENCLAVE_OVERRIDE "ROS_SECURITY_ENCLAVE_OVERRIDE"
 #endif
 
-#ifndef ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME
-# define ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "ROS_SECURITY_ROOT_DIRECTORY"
+#ifndef ROS_SECURITY_KEYSTORE_VAR_NAME
+# define ROS_SECURITY_KEYSTORE_VAR_NAME "ROS_SECURITY_KEYSTORE"
 #endif
 
 #ifndef ROS_SECURITY_STRATEGY_VAR_NAME
@@ -96,21 +96,21 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy);
 /**
  * Return the security directory associated with the enclave name.
  *
- * The value of the environment variable `ROS_SECURITY_ROOT_DIRECTORY` is used as a root.
+ * The value of the environment variable `ROS_SECURITY_KEYSTORE` is used as a root.
  * The specific directory to be used, is found from that root using the `name` passed.
  * E.g. for a context named "/a/b/c" and root "/r", the secure root path will be
  * "/r/a/b/c", where the delimiter "/" is native for target file system (e.g. "\\" for _WIN32).
  *
- * However, this expansion can be overridden by setting the secure directory override environment
- * (`ROS_SECURITY_DIRECTORY_OVERRIDE`) variable, allowing users to explicitly specify the exact secure
- * root directory to be utilized.
+ * However, this expansion can be overridden by setting the secure enclave override environment
+ * (`ROS_SECURITY_ENCLAVE_OVERRIDE`) variable, allowing users to explicitly specify the exact enclave
+ * `name` to be utilized.
  * Such an override is useful for applications where the enclave is non-deterministic
  * before runtime, or when testing and using additional tools that may not otherwise be easily
  * provisioned.
  *
  * \param[in] name validated name (a single token)
  * \param[in] allocator the allocator to use for allocation
- * \returns Machine specific (absolute) secure root path or NULL on failure.
+ * \returns Machine specific (absolute) enclave directory path or NULL on failure.
  *  Returned pointer must be deallocated by the caller of this function
  */
 RCL_PUBLIC
diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c
@@ -106,13 +106,13 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy)
 
 char * exact_match_lookup(
   const char * name,
-  const char * ros_secure_root_env,
+  const char * ros_secure_keystore_env,
   const rcl_allocator_t * allocator)
 {
   // Perform an exact match for the enclave name in directory <root dir>.
   char * secure_root = NULL;
   char * enclaves_dir = NULL;
-  enclaves_dir = rcutils_join_path(ros_secure_root_env, "enclaves", *allocator);
+  enclaves_dir = rcutils_join_path(ros_secure_keystore_env, "enclaves", *allocator);
   // "/" case when root namespace is explicitly passed in
   if (0 == strcmp(name, "/")) {
     secure_root = enclaves_dir;
@@ -132,58 +132,74 @@ char * rcl_get_secure_root(
   const char * name,
   const rcl_allocator_t * allocator)
 {
-  bool ros_secure_directory_override = true;
+  bool ros_secure_enclave_override = true;
 
   // find out if either of the configuration environment variables are set
   const char * env_buf = NULL;
   if (NULL == name) {
     return NULL;
   }
-
-  if (rcutils_get_env(ROS_SECURITY_DIRECTORY_OVERRIDE, &env_buf)) {
+  const char * get_env_error_str = NULL;
+  // check if enclave override environment variable is empty
+  get_env_error_str = rcutils_get_env(ROS_SECURITY_ENCLAVE_OVERRIDE, &env_buf);
+  if (NULL != get_env_error_str) {
+    RCUTILS_LOG_ERROR("rcutils_get_env failed: %s\n", get_env_error_str);
     return NULL;
   }
   if (!env_buf) {
     return NULL;
   }
   if (0 == strcmp("", env_buf)) {
-    // check root directory if override directory environment variable is empty
-    if (rcutils_get_env(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME, &env_buf)) {
-      return NULL;
-    }
-    if (!env_buf) {
-      return NULL;
-    }
-    if (0 == strcmp("", env_buf)) {
-      return NULL;  // environment variable was empty
-    }
-    ros_secure_directory_override = false;
+    ros_secure_enclave_override = false;
   }
+  char * ros_secure_enclave_override_env = rcutils_strdup(env_buf, *allocator);
 
-  // found a usable environment variable, copy into our memory before overwriting with next lookup
-  char * ros_secure_root_env = rcutils_strdup(env_buf, *allocator);
+  // check if keystore environment variable is empty
+  get_env_error_str = rcutils_get_env(ROS_SECURITY_KEYSTORE_VAR_NAME, &env_buf);
+  if (NULL != get_env_error_str) {
+    RCUTILS_LOG_ERROR("rcutils_get_env failed: %s\n", get_env_error_str);
+    allocator->deallocate(ros_secure_enclave_override_env, allocator->state);
+    return NULL;
+  }
+  if (!env_buf) {
+    allocator->deallocate(ros_secure_enclave_override_env, allocator->state);
+    return NULL;
+  }
+  if (0 == strcmp("", env_buf)) {
+    allocator->deallocate(ros_secure_enclave_override_env, allocator->state);
+    return NULL;  // environment variable was empty
+  }
+  char * ros_secure_keystore_env = rcutils_strdup(env_buf, *allocator);
 
+  // given usable environment variables, overwrite with next lookup
   char * secure_root = NULL;
-  if (ros_secure_directory_override) {
-    secure_root = rcutils_strdup(ros_secure_root_env, *allocator);
+  if (ros_secure_enclave_override) {
+    secure_root = exact_match_lookup(
+      ros_secure_enclave_override_env,
+      ros_secure_keystore_env,
+      allocator);
   } else {
-    secure_root = exact_match_lookup(name, ros_secure_root_env, allocator);
+    secure_root = exact_match_lookup(
+      name,
+      ros_secure_keystore_env,
+      allocator);
   }
 
   if (NULL == secure_root || !rcutils_is_directory(secure_root)) {
     // Check secure_root is not NULL before checking directory
     if (NULL == secure_root) {
       RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
         "SECURITY ERROR: unable to find a folder matching the name '%s' in '%s'. ",
-        name, ros_secure_root_env);
+        name, ros_secure_keystore_env);
     } else {
       RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
         "SECURITY ERROR: directory '%s' does not exist.", secure_root);
     }
-    allocator->deallocate(ros_secure_root_env, allocator->state);
+    allocator->deallocate(ros_secure_enclave_override_env, allocator->state);
+    allocator->deallocate(ros_secure_keystore_env, allocator->state);
     allocator->deallocate(secure_root, allocator->state);
     return NULL;
   }
-  allocator->deallocate(ros_secure_root_env, allocator->state);
+  allocator->deallocate(ros_secure_keystore_env, allocator->state);
   return secure_root;
 }
diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp
@@ -38,8 +38,10 @@
 # define PATH_SEPARATOR "\\"
 #endif
 
-#define TEST_ENCLAVE_MULTIPLE_TOKENS \
-  "/group1" PATH_SEPARATOR TEST_ENCLAVE
+#define TEST_ENCLAVE_MULTIPLE_TOKENS_ABSOLUTE \
+  "/group1" TEST_ENCLAVE_ABSOLUTE
+#define TEST_ENCLAVE_MULTIPLE_TOKENS_DIR \
+  "group1" PATH_SEPARATOR TEST_ENCLAVE
 
 char g_envstring[512] = {0};
 
@@ -74,8 +76,8 @@ class TestGetSecureRoot : public ::testing::Test
     rcl_reset_error();
 
     // Always make sure the variable we set is unset at the beginning of a test
-    unsetenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME);
-    unsetenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE);
+    unsetenv_wrapper(ROS_SECURITY_KEYSTORE_VAR_NAME);
+    unsetenv_wrapper(ROS_SECURITY_ENCLAVE_OVERRIDE);
     unsetenv_wrapper(ROS_SECURITY_STRATEGY_VAR_NAME);
     unsetenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME);
     allocator = rcl_get_default_allocator();
@@ -104,7 +106,7 @@ class TestGetSecureRoot : public ::testing::Test
   {
     base_lookup_dir_fqn = rcutils_join_path(
       resource_dir, resource_dir_name, allocator);
-    std::string putenv_input = ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=";
+    std::string putenv_input = ROS_SECURITY_KEYSTORE_VAR_NAME "=";
     putenv_input += base_lookup_dir_fqn;
     memcpy(
       g_envstring, putenv_input.c_str(),
@@ -124,7 +126,7 @@ TEST_F(TestGetSecureRoot, failureScenarios) {
     (char *) NULL);
   rcl_reset_error();
 
-  putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
+  putenv_wrapper(ROS_SECURITY_KEYSTORE_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
 
   /* Security directory is set, but there's no matching directory */
   /// Wrong enclave
@@ -136,7 +138,7 @@ TEST_F(TestGetSecureRoot, failureScenarios) {
 
 TEST_F(TestGetSecureRoot, successScenarios_local_root_enclave) {
   putenv_wrapper(
-    ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "="
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
     TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
 
   secure_root = rcl_get_secure_root("/", &allocator);
@@ -148,7 +150,7 @@ TEST_F(TestGetSecureRoot, successScenarios_local_root_enclave) {
 
 TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) {
   putenv_wrapper(
-    ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "="
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
     TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
 
   secure_root = rcl_get_secure_root(TEST_ENCLAVE_ABSOLUTE, &allocator);
@@ -161,42 +163,43 @@ TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) {
 
 TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch_multipleTokensName) {
   putenv_wrapper(
-    ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "="
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
     TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
 
   secure_root = rcl_get_secure_root(
-    TEST_ENCLAVE_MULTIPLE_TOKENS, &allocator);
+    TEST_ENCLAVE_MULTIPLE_TOKENS_ABSOLUTE, &allocator);
   ASSERT_NE(nullptr, secure_root);
   std::string secure_root_str(secure_root);
   ASSERT_STREQ(
     TEST_ENCLAVE,
     secure_root_str.substr(secure_root_str.size() - strlen(TEST_ENCLAVE)).c_str());
 }
 
-TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) {
-  /* Specify a valid directory */
-  putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY);
+TEST_F(TestGetSecureRoot, nodeSecurityEnclaveOverride_validEnclave) {
+  putenv_wrapper(
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
+    TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
+
+  /* Specify a valid enclave */
+  putenv_wrapper(ROS_SECURITY_ENCLAVE_OVERRIDE "=" TEST_ENCLAVE_ABSOLUTE);
   root_path = rcl_get_secure_root(
     "name shouldn't matter", &allocator);
-  ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY);
+  ASSERT_STREQ(
+    TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME
+    PATH_SEPARATOR "enclaves" PATH_SEPARATOR TEST_ENCLAVE,
+    root_path);
 }
 
-TEST_F(
-  TestGetSecureRoot,
-  nodeSecurityDirectoryOverride_validDirectory_overrideRootDirectoryAttempt) {
-  /* Setting root dir has no effect */
-  putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY);
-  root_path = rcl_get_secure_root("name shouldn't matter", &allocator);
-  putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
-  ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY);
-}
+TEST_F(TestGetSecureRoot, nodeSecurityEnclaveOverride_invalidEnclave) {
+  putenv_wrapper(
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
+    TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
 
-TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) {
   /* The override provided should exist. Providing correct node/namespace/root dir won't help
    * if the node override is invalid. */
   putenv_wrapper(
-    ROS_SECURITY_DIRECTORY_OVERRIDE
-    "=TheresN_oWayThi_sDirectory_Exists_hence_this_should_fail");
+    ROS_SECURITY_ENCLAVE_OVERRIDE
+    "=TheresN_oWayThi_sEnclave_Exists_hence_this_should_fail");
   EXPECT_EQ(
     rcl_get_secure_root(TEST_ENCLAVE_ABSOLUTE, &allocator),
     (char *) NULL);
@@ -215,20 +218,26 @@ TEST_F(TestGetSecureRoot, test_get_security_options) {
 
   putenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME "=true");
   putenv_wrapper(ROS_SECURITY_STRATEGY_VAR_NAME "=Enforce");
+  putenv_wrapper(
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
+    TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
 
   putenv_wrapper(
-    ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY);
+    ROS_SECURITY_ENCLAVE_OVERRIDE "=" TEST_ENCLAVE_MULTIPLE_TOKENS_ABSOLUTE);
   ret = rcl_get_security_options_from_environment(
     "doesn't matter at all", &allocator, &options);
   ASSERT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str;
   EXPECT_EQ(RMW_SECURITY_ENFORCEMENT_ENFORCE, options.enforce_security);
-  EXPECT_STREQ(TEST_RESOURCES_DIRECTORY, options.security_root_path);
+  EXPECT_STREQ(
+    TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME
+    PATH_SEPARATOR "enclaves" PATH_SEPARATOR TEST_ENCLAVE_MULTIPLE_TOKENS_DIR,
+    options.security_root_path);
   EXPECT_EQ(RMW_RET_OK, rmw_security_options_fini(&options, &allocator));
 
   options = rmw_get_zero_initialized_security_options();
-  unsetenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE);
+  unsetenv_wrapper(ROS_SECURITY_ENCLAVE_OVERRIDE);
   putenv_wrapper(
-    ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "="
+    ROS_SECURITY_KEYSTORE_VAR_NAME "="
     TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
   ret = rcl_get_security_options_from_environment(
     TEST_ENCLAVE_ABSOLUTE, &allocator, &options);
