From 31d3acc7d341468248c41ea95a59713990b3721a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 31 Jan 2020 11:17:06 -0300 Subject: [PATCH 01/30] Rename security_directory.* to security.* Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/{security_directory.h => security.h} | 0 rcl/src/rcl/{security_directory.c => security.c} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename rcl/include/rcl/{security_directory.h => security.h} (100%) rename rcl/src/rcl/{security_directory.c => security.c} (100%) diff --git a/rcl/include/rcl/security_directory.h b/rcl/include/rcl/security.h similarity index 100% rename from rcl/include/rcl/security_directory.h rename to rcl/include/rcl/security.h diff --git a/rcl/src/rcl/security_directory.c b/rcl/src/rcl/security.c similarity index 100% rename from rcl/src/rcl/security_directory.c rename to rcl/src/rcl/security.c From f9aa67e2ea7bdf876ee1eff57979ca8fc6373e59 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 10 Oct 2019 17:14:17 -0300 Subject: [PATCH 02/30] Switch to one participant per context Signed-off-by: Ivan Santiago Paunovic Fix test Signed-off-by: Ivan Santiago Paunovic Addressed comments Signed-off-by: Ivan Santiago Paunovic Increase test_graph timeout Signed-off-by: Ivan Santiago Paunovic --- rcl/CMakeLists.txt | 3 +- rcl/include/rcl/domain_id.h | 52 ++++++++++++++++ rcl/include/rcl/localhost.h | 16 +++-- rcl/include/rcl/node.h | 2 +- rcl/include/rcl/node_options.h | 6 +- rcl/include/rcl/security.h | 40 +++++++++++- rcl/src/rcl/domain_id.c | 56 +++++++++++++++++ rcl/src/rcl/init.c | 38 ++++++++++++ rcl/src/rcl/init_options_impl.h | 2 +- rcl/src/rcl/localhost.c | 30 +++++++-- rcl/src/rcl/node.c | 78 ++++++++---------------- rcl/src/rcl/node_options.c | 1 + rcl/src/rcl/security.c | 50 ++++++++++++++- rcl/test/CMakeLists.txt | 1 + rcl/test/rcl/test_node.cpp | 5 +- rcl/test/rcl/test_security_directory.cpp | 2 +- 16 files changed, 305 insertions(+), 77 deletions(-) create mode 100644 rcl/include/rcl/domain_id.h create mode 100644 rcl/src/rcl/domain_id.c diff --git a/rcl/CMakeLists.txt b/rcl/CMakeLists.txt index cca35b24b..e04afd468 100644 --- a/rcl/CMakeLists.txt +++ b/rcl/CMakeLists.txt @@ -37,6 +37,7 @@ set(${PROJECT_NAME}_sources src/rcl/client.c src/rcl/common.c src/rcl/context.c + src/rcl/domain_id.c src/rcl/event.c src/rcl/expand_topic_name.c src/rcl/graph.c @@ -53,13 +54,13 @@ set(${PROJECT_NAME}_sources src/rcl/publisher.c src/rcl/remap.c src/rcl/rmw_implementation_identifier_check.c + src/rcl/security.c src/rcl/service.c src/rcl/subscription.c src/rcl/time.c src/rcl/timer.c src/rcl/validate_topic_name.c src/rcl/wait.c - src/rcl/security_directory.c ) add_library(${PROJECT_NAME} ${${PROJECT_NAME}_sources}) diff --git a/rcl/include/rcl/domain_id.h b/rcl/include/rcl/domain_id.h new file mode 100644 index 000000000..0259197c4 --- /dev/null +++ b/rcl/include/rcl/domain_id.h @@ -0,0 +1,52 @@ +// Copyright 2019 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef RCL__DOMAIN_ID_H_ +#define RCL__DOMAIN_ID_H_ + +#ifdef __cplusplus +extern "C" +{ +#endif + +#include + +#include "rcl/types.h" +#include "rcl/visibility_control.h" +#include "rmw/domain_id.h" + +#define RCL_DEFAULT_DOMAIN_ID RMW_DEFAULT_DOMAIN_ID + +extern const char * const RCL_DOMAIN_ID_ENV_VAR; + +/// Determine the default domain id, based on the environment. +/** + * Checks the default domain id based on ROS_DOMAIN_ID environment variable, if + * the original `domain_id` was RCL_DEFAULT_DOMAIN_ID. + * If not, the input `domain_id` is not modified. + * + * \param[inout] domain_id Must not be NULL. + * \returns RCL_RET_INVALID_ARGUMENT if an argument is invalid, or, + * \returns RCL_RET_ERROR in case of an unexpected error, or, + * \returns RCL_RET_OK. + */ +RCL_PUBLIC +rcl_ret_t +rcl_domain_id(size_t * domain_id); + +#ifdef __cplusplus +} +#endif + +#endif // RCL__DOMAIN_ID_H_ diff --git a/rcl/include/rcl/localhost.h b/rcl/include/rcl/localhost.h index be41615d8..6d5c04541 100644 --- a/rcl/include/rcl/localhost.h +++ b/rcl/include/rcl/localhost.h @@ -22,18 +22,24 @@ extern "C" #include "rcl/types.h" #include "rcl/visibility_control.h" +#include "rmw/localhost.h" extern const char * const RCL_LOCALHOST_ENV_VAR; /// Determine if the user wants to communicate using loopback only. /** - * Checks if localhost should be used for network communication checking ROS_LOCALHOST_ONLY env - * variable - * \returns true if ROS_LOCALHOST_ONLY is set and is 1, false otherwise. + * Checks if localhost should be used for network communication. + * If `localhost_only` is RCL_LOCALHOST_ONLY_DEFAULT, ROS_LOCALHOST_ONLY env variable is used. + * If not, `localhost_only` is not modified. + * + * \param[inout] localhost_only Must not be NULL. + * \returns RCL_RET_INVALID_ARGUMENT if an argument is invalid, or + * \returns RCL_RET_ERROR if an unexpected error happened, or + * \returns RCL_RET_OK. */ RCL_PUBLIC -bool -rcl_localhost_only(); +rcl_ret_t +rcl_localhost_only(rmw_localhost_only_t * localhost_only); #ifdef __cplusplus } diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index b3a6f7312..fd87e4518 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -336,7 +336,7 @@ rcl_node_get_options(const rcl_node_t * node); * This function returns the ROS domain ID that the node is in. * * This function should be used to determine what `domain_id` was used rather - * than checking the domin_id field in the node options, because if + * than checking the domain_id field in the node options, because if * `RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID` is used when creating the node then * it is not changed after creation, but this function will return the actual * `domain_id` used. diff --git a/rcl/include/rcl/node_options.h b/rcl/include/rcl/node_options.h index 7f0941164..839666f05 100644 --- a/rcl/include/rcl/node_options.h +++ b/rcl/include/rcl/node_options.h @@ -23,8 +23,10 @@ extern "C" #include "rcl/allocator.h" #include "rcl/arguments.h" +#include "rcl/domain_id.h" + /// Constant which indicates that the default domain id should be used. -#define RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID SIZE_MAX +#define RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID RCL_DEFAULT_DOMAIN_ID /// Structure which encapsulates the options for creating a rcl_node_t. typedef struct rcl_node_options_t @@ -66,7 +68,7 @@ typedef struct rcl_node_options_t /** * The default values are: * - * - domain_id = RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID + * - domain_id = RMW_DEFAULT_DOMAIN_ID * - allocator = rcl_get_default_allocator() * - use_global_arguments = true * - arguments = rcl_get_zero_initialized_arguments() diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index e3d8b52f7..56328bd5f 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -12,16 +12,20 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef RCL__SECURITY_DIRECTORY_H_ -#define RCL__SECURITY_DIRECTORY_H_ +#ifndef RCL__SECURITY_H_ +#define RCL__SECURITY_H_ #ifdef __cplusplus extern "C" { #endif +#include + #include "rcl/allocator.h" +#include "rcl/types.h" #include "rcl/visibility_control.h" +#include "rmw/security_options.h" #ifndef ROS_SECURITY_NODE_DIRECTORY_VAR_NAME #define ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "ROS_SECURITY_NODE_DIRECTORY" @@ -35,6 +39,36 @@ extern "C" #define ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "ROS_SECURITY_LOOKUP_TYPE" #endif +#ifndef ROS_SECURITY_STRATEGY_VAR_NAME +#define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY" +#endif + +#ifndef ROS_SECURITY_ENABLE_VAR_NAME +#define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE" +#endif + +/// Check if security has to be used, according to the environment. +/** + * \param use_security[out] Must not be NULL. + * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or + * \returns RCL_RET_ERROR if an unexpected error happened, or + * \returns RCL_RET_OK. + */ +RCL_PUBLIC +rcl_ret_t +rcl_use_security(bool * use_security); + +/// Get security enforcement policy from the environment. +/** + * \param policy[out] Must not be NULL. + * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or + * \returns RCL_RET_ERROR if an unexpected error happened, or + * \returns RCL_RET_OK. + */ +RCL_PUBLIC +rcl_ret_t +rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); + /// Return the secure root directory associated with a node given its validated name and namespace. /** * E.g. for a node named "c" in namespace "/a/b", the secure root path will be @@ -64,4 +98,4 @@ char * rcl_get_secure_root( } #endif -#endif // RCL__SECURITY_DIRECTORY_H_ +#endif // RCL__SECURITY_H_ diff --git a/rcl/src/rcl/domain_id.c b/rcl/src/rcl/domain_id.c new file mode 100644 index 000000000..aa9592749 --- /dev/null +++ b/rcl/src/rcl/domain_id.c @@ -0,0 +1,56 @@ +// Copyright 2019 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "rcl/domain_id.h" + +#include + +#include "rcutils/get_env.h" + +#include "rcl/error_handling.h" +#include "rcl/types.h" + +const char * const RCL_DOMAIN_ID_ENV_VAR = "ROS_DOMAIN_ID"; + +rcl_ret_t +rcl_domain_id(size_t * domain_id) +{ + const char * ros_domain_id; + const char * get_env_error_str; + + if (!domain_id) { + return RCL_RET_INVALID_ARGUMENT; + } + + if (RCL_DEFAULT_DOMAIN_ID != *domain_id) { + return RCL_RET_OK; + } + + get_env_error_str = rcutils_get_env(RCL_DOMAIN_ID_ENV_VAR, &ros_domain_id); + if (NULL != get_env_error_str) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Error getting env var '" RCUTILS_STRINGIFY(RCL_DOMAIN_ID_ENV_VAR) "': %s\n", + get_env_error_str); + return RCL_RET_ERROR; + } + if (ros_domain_id) { + unsigned long number = strtoul(ros_domain_id, NULL, 0); // NOLINT(runtime/int) + if (number == ULONG_MAX) { + RCL_SET_ERROR_MSG("failed to interpret ROS_DOMAIN_ID as integral number"); + return RCL_RET_ERROR; + } + *domain_id = (size_t)number; + } + return RCL_RET_OK; +} diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index d1dc84c80..2cd929b8a 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -24,8 +24,11 @@ extern "C" #include "./context_impl.h" #include "./init_options_impl.h" #include "rcl/arguments.h" +#include "rcl/domain_id.h" #include "rcl/error_handling.h" +#include "rcl/localhost.h" #include "rcl/logging.h" +#include "rcl/security.h" #include "rcutils/logging_macros.h" #include "rcutils/stdatomic_helper.h" #include "rmw/error_handling.h" @@ -144,6 +147,41 @@ rcl_init( rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id); context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id; + // Get actual domain id based on environment variable, if needed. + ret = rcl_domain_id(&context->impl->init_options.impl->rmw_init_options.domain_id); + if (RCL_RET_OK != ret) { + fail_ret = ret; + goto fail; + } + + // Get actual localhost_only value based on environment variable, if needed. + ret = rcl_localhost_only(&context->impl->init_options.impl->rmw_init_options.localhost_only); + if (RCL_RET_OK != ret) { + fail_ret = ret; + goto fail; + } + + // TODO(ivanpauno): Uncomment this when new path discovery magic is ready. + // bool use_security = false; + // ret = rcl_use_security(&use_security); + // if (RCL_RET_OK != ret) { + // fail_ret = ret; + // goto fail; + // } + // RCUTILS_LOG_DEBUG_NAMED( + // ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); + + // ret = rcl_get_enforcement_policy( + // &context->impl->init_options.impl->rmw_init_options.security_options.enforce_security); + // if (RCL_RET_OK != ret) { + // fail_ret = ret; + // goto fail; + // } + + // if (use_security) { + // ... + // } + // Initialize rmw_init. rmw_ret_t rmw_ret = rmw_init( &(context->impl->init_options.impl->rmw_init_options), diff --git a/rcl/src/rcl/init_options_impl.h b/rcl/src/rcl/init_options_impl.h index e03a8e88a..b1d4401e2 100644 --- a/rcl/src/rcl/init_options_impl.h +++ b/rcl/src/rcl/init_options_impl.h @@ -17,7 +17,7 @@ #include "rcl/init_options.h" -#include "rmw/init.h" +#include "rmw/init_options.h" #ifdef __cplusplus extern "C" diff --git a/rcl/src/rcl/localhost.c b/rcl/src/rcl/localhost.c index 1d5945ad3..2165c6cc1 100644 --- a/rcl/src/rcl/localhost.c +++ b/rcl/src/rcl/localhost.c @@ -12,19 +12,39 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include +#include "rcl/localhost.h" #include #include #include "rcutils/get_env.h" +#include "rcl/error_handling.h" +#include "rcl/types.h" + const char * const RCL_LOCALHOST_ENV_VAR = "ROS_LOCALHOST_ONLY"; -bool -rcl_localhost_only() +rcl_ret_t +rcl_localhost_only(rmw_localhost_only_t * localhost_only) { const char * ros_local_host_env_val = NULL; - return rcutils_get_env(RCL_LOCALHOST_ENV_VAR, &ros_local_host_env_val) == NULL && - ros_local_host_env_val != NULL && strcmp(ros_local_host_env_val, "1") == 0; + const char * get_env_error_str = NULL; + + if (!localhost_only) { + return RCL_RET_INVALID_ARGUMENT; + } + + if (RMW_LOCALHOST_ONLY_DEFAULT == *localhost_only) { + return RCL_RET_OK; + } + + get_env_error_str = rcutils_get_env(RCL_LOCALHOST_ENV_VAR, &ros_local_host_env_val); + if (NULL != get_env_error_str) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Error getting env var '" RCUTILS_STRINGIFY(RCL_DOMAIN_ID_ENV_VAR) "': %s\n", + get_env_error_str); + return RCL_RET_ERROR; + } + *localhost_only = ros_local_host_env_val != NULL && strcmp(ros_local_host_env_val, "1") == 0; + return RCL_RET_OK; } diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 80e5a025c..a00ed574c 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -26,12 +26,14 @@ extern "C" #include "rcl/arguments.h" #include "rcl/error_handling.h" +#include "rcl/domain_id.h" #include "rcl/localhost.h" #include "rcl/logging.h" #include "rcl/logging_rosout.h" #include "rcl/rcl.h" #include "rcl/remap.h" -#include "rcl/security_directory.h" +#include "rcl/security.h" + #include "rcutils/filesystem.h" #include "rcutils/find.h" #include "rcutils/format_string.h" @@ -41,8 +43,9 @@ extern "C" #include "rcutils/repl_str.h" #include "rcutils/snprintf.h" #include "rcutils/strdup.h" + #include "rmw/error_handling.h" -#include "rmw/node_security_options.h" +#include "rmw/security_options.h" #include "rmw/rmw.h" #include "rmw/validate_namespace.h" #include "rmw/validate_node_name.h" @@ -50,11 +53,6 @@ extern "C" #include "./context_impl.h" -#define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY" -#define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE" -#define ROS_DOMAIN_ID_VAR_NAME "ROS_DOMAIN_ID" - - typedef struct rcl_node_impl_t { rcl_node_options_t options; @@ -123,8 +121,7 @@ rcl_node_init( const rcl_node_options_t * options) { size_t domain_id = 0; - const char * ros_domain_id; - const char * get_env_error_str; + rmw_localhost_only_t localhost_only = RMW_LOCALHOST_ONLY_DEFAULT; const rmw_guard_condition_t * rmw_graph_guard_condition = NULL; rcl_guard_condition_options_t graph_guard_condition_options = rcl_guard_condition_get_default_options(); @@ -258,61 +255,31 @@ rcl_node_init( RCL_CHECK_FOR_NULL_WITH_MSG( node->impl->logger_name, "creating logger name failed", goto fail); - // node rmw_node_handle - if (node->impl->options.domain_id == RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID) { - // Find the domain ID set by the environment. - get_env_error_str = rcutils_get_env(ROS_DOMAIN_ID_VAR_NAME, &ros_domain_id); - if (NULL != get_env_error_str) { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Error getting env var '" RCUTILS_STRINGIFY(ROS_DOMAIN_ID_VAR_NAME) "': %s\n", - get_env_error_str); - goto fail; - } - if (ros_domain_id) { - unsigned long number = strtoul(ros_domain_id, NULL, 0); // NOLINT(runtime/int) - if (number == ULONG_MAX) { - RCL_SET_ERROR_MSG("failed to interpret ROS_DOMAIN_ID as integral number"); - goto fail; - } - domain_id = (size_t)number; - } - } else { - domain_id = node->impl->options.domain_id; + domain_id = node->impl->options.domain_id; + if (RMW_RET_OK != rcl_domain_id(&domain_id)) { + goto fail; } - // actual domain id - node->impl->actual_domain_id = domain_id; RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id); + node->impl->actual_domain_id = domain_id; - const char * ros_security_enable = NULL; - const char * ros_enforce_security = NULL; + rmw_security_options_t node_security_options = + rmw_get_zero_initialized_security_options(); - get_env_error_str = rcutils_get_env(ROS_SECURITY_ENABLE_VAR_NAME, &ros_security_enable); - if (NULL != get_env_error_str) { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Error getting env var '" RCUTILS_STRINGIFY(ROS_SECURITY_ENABLE_VAR_NAME) "': %s\n", - get_env_error_str); - ret = RCL_RET_ERROR; + bool use_security; + ret = rcl_use_security(&use_security); + if (RCL_RET_OK != ret) { + fail_ret = ret; goto fail; } - - bool use_security = (0 == strcmp(ros_security_enable, "true")); RCUTILS_LOG_DEBUG_NAMED( ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); - get_env_error_str = rcutils_get_env(ROS_SECURITY_STRATEGY_VAR_NAME, &ros_enforce_security); - if (NULL != get_env_error_str) { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Error getting env var '" RCUTILS_STRINGIFY(ROS_SECURITY_STRATEGY_VAR_NAME) "': %s\n", - get_env_error_str); - ret = RCL_RET_ERROR; + ret = rcl_get_enforcement_policy(&node_security_options.enforce_security); + if (RCL_RET_OK != ret) { + fail_ret = ret; goto fail; } - rmw_node_security_options_t node_security_options = - rmw_get_zero_initialized_node_security_options(); - node_security_options.enforce_security = (0 == strcmp(ros_enforce_security, "Enforce")) ? - RMW_SECURITY_ENFORCEMENT_ENFORCE : RMW_SECURITY_ENFORCEMENT_PERMISSIVE; - if (!use_security) { node_security_options.enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE; } else { // if use_security @@ -328,9 +295,14 @@ rcl_node_init( } } } + + if (RMW_RET_OK != rcl_localhost_only(&localhost_only)) { + goto fail; + } + node->impl->rmw_node_handle = rmw_create_node( &(node->context->impl->rmw_context), - name, local_namespace_, domain_id, &node_security_options, rcl_localhost_only()); + name, local_namespace_, domain_id, &node_security_options, localhost_only); RCL_CHECK_FOR_NULL_WITH_MSG( node->impl->rmw_node_handle, rmw_get_error_string().str, goto fail); diff --git a/rcl/src/rcl/node_options.c b/rcl/src/rcl/node_options.c index fa7880811..024a76dc6 100644 --- a/rcl/src/rcl/node_options.c +++ b/rcl/src/rcl/node_options.c @@ -20,6 +20,7 @@ extern "C" #include "rcl/node_options.h" #include "rcl/arguments.h" +#include "rcl/domain_id.h" #include "rcl/error_handling.h" #include "rcl/logging_rosout.h" diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index 7450bdc25..292f16db1 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -12,12 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "rcl/security_directory.h" +#include "rcl/security.h" + +#include #include "rcl/error_handling.h" #include "rcutils/filesystem.h" #include "rcutils/get_env.h" #include "rcutils/format_string.h" +#include "rmw/security_options.h" #ifdef __clang__ # pragma clang diagnostic push @@ -28,6 +31,51 @@ # pragma clang diagnostic pop #endif +rcl_ret_t +rcl_use_security(bool * use_security) +{ + const char * ros_security_enable = NULL; + const char * get_env_error_str = NULL; + + if (!use_security) { + return RCL_RET_INVALID_ARGUMENT; + } + + get_env_error_str = rcutils_get_env(ROS_SECURITY_ENABLE_VAR_NAME, &ros_security_enable); + if (NULL != get_env_error_str) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Error getting env var '" RCUTILS_STRINGIFY(ROS_SECURITY_ENABLE_VAR_NAME) "': %s\n", + get_env_error_str); + return RCL_RET_ERROR; + } + + *use_security = (0 == strcmp(ros_security_enable, "true")); + return RCL_RET_OK; +} + +rcl_ret_t +rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy) +{ + const char * ros_enforce_security = NULL; + const char * get_env_error_str = NULL; + + if (!policy) { + return RCL_RET_INVALID_ARGUMENT; + } + + get_env_error_str = rcutils_get_env(ROS_SECURITY_STRATEGY_VAR_NAME, &ros_enforce_security); + if (NULL != get_env_error_str) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Error getting env var '" RCUTILS_STRINGIFY(ROS_SECURITY_STRATEGY_VAR_NAME) "': %s\n", + get_env_error_str); + return RCL_RET_ERROR; + } + + *policy = (0 == strcmp(ros_enforce_security, "Enforce")) ? + RMW_SECURITY_ENFORCEMENT_ENFORCE : RMW_SECURITY_ENFORCEMENT_PERMISSIVE; + return RCL_RET_OK; +} + /** * A security lookup function takes in the node's name, namespace, a security root directory and an allocator; * It returns the relevant information required to load the security credentials, diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index f534ee5cf..2bcbbb2a0 100644 --- a/rcl/test/CMakeLists.txt +++ b/rcl/test/CMakeLists.txt @@ -98,6 +98,7 @@ function(test_target_function) APPEND_LIBRARY_DIRS ${extra_lib_dirs} LIBRARIES ${PROJECT_NAME} AMENT_DEPENDENCIES ${rmw_implementation} "osrf_testing_tools_cpp" "test_msgs" + TIMEOUT 120 ${AMENT_GTEST_ARGS} ) diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index f75a2376b..cb7b50354 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -120,14 +120,11 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors) { osrf_testing_tools_cpp::memory_tools::disable_monitoring_in_all_threads(); rcl_ret_t ret = rcl_node_fini(&invalid_node); + EXPECT_EQ(RCL_RET_OK, rcl_context_fini(&invalid_context)) << rcl_get_error_string().str; EXPECT_EQ(RCL_RET_OK, ret); }); ret = rcl_shutdown(&invalid_context); // Shutdown to invalidate the node. ASSERT_EQ(RCL_RET_OK, ret); - OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( - { - EXPECT_EQ(RCL_RET_OK, rcl_context_fini(&invalid_context)) << rcl_get_error_string().str; - }); rcl_context_t context = rcl_get_zero_initialized_context(); ret = rcl_init(0, nullptr, &init_options, &context); ASSERT_EQ(RCL_RET_OK, ret); diff --git a/rcl/test/rcl/test_security_directory.cpp b/rcl/test/rcl/test_security_directory.cpp index 39a5b7ce9..71f0ebf16 100644 --- a/rcl/test/rcl/test_security_directory.cpp +++ b/rcl/test/rcl/test_security_directory.cpp @@ -16,7 +16,7 @@ #include #include -#include "rcl/security_directory.h" +#include "rcl/security.h" #include "rcutils/filesystem.h" #include "osrf_testing_tools_cpp/scope_exit.hpp" #include "rcl/error_handling.h" From 7009480b97b0f39898041424a6183a305e22d00b Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 24 Jan 2020 10:48:18 -0300 Subject: [PATCH 03/30] Add context name and namespace, allow configuring security directory with context name Signed-off-by: Ivan Santiago Paunovic Address peer review comments Signed-off-by: Ivan Santiago Paunovic Address peer review comments Signed-off-by: Ivan Santiago Paunovic Correct wrong RMW_RET_OK to RCL_RET_OK Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/localhost.h | 2 +- rcl/include/rcl/node_options.h | 2 +- rcl/include/rcl/security.h | 72 ++++++++++++++++----- rcl/src/rcl/init.c | 33 ++++------ rcl/src/rcl/node.c | 47 ++++---------- rcl/src/rcl/security.c | 81 ++++++++++++++++++------ rcl/test/rcl/test_security_directory.cpp | 53 ++++++++++------ 7 files changed, 179 insertions(+), 111 deletions(-) diff --git a/rcl/include/rcl/localhost.h b/rcl/include/rcl/localhost.h index 6d5c04541..d40ea6eb1 100644 --- a/rcl/include/rcl/localhost.h +++ b/rcl/include/rcl/localhost.h @@ -29,7 +29,7 @@ extern const char * const RCL_LOCALHOST_ENV_VAR; /// Determine if the user wants to communicate using loopback only. /** * Checks if localhost should be used for network communication. - * If `localhost_only` is RCL_LOCALHOST_ONLY_DEFAULT, ROS_LOCALHOST_ONLY env variable is used. + * If `localhost_only` is RMW_LOCALHOST_ONLY_DEFAULT, ROS_LOCALHOST_ONLY env variable is used. * If not, `localhost_only` is not modified. * * \param[inout] localhost_only Must not be NULL. diff --git a/rcl/include/rcl/node_options.h b/rcl/include/rcl/node_options.h index 839666f05..15dfeed6e 100644 --- a/rcl/include/rcl/node_options.h +++ b/rcl/include/rcl/node_options.h @@ -68,7 +68,7 @@ typedef struct rcl_node_options_t /** * The default values are: * - * - domain_id = RMW_DEFAULT_DOMAIN_ID + * - domain_id = RCL_NODE_OPTIONS_DEFAULT_DOMAIN_ID * - allocator = rcl_get_default_allocator() * - use_global_arguments = true * - arguments = rcl_get_zero_initialized_arguments() diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index 56328bd5f..097c1dad9 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -47,8 +47,33 @@ extern "C" #define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE" #endif +/// Init the security options from the values of the environment variables and passed names. +/** + * Inits the passed security options based on the environment. + * For more details: + * \sa rcl_security_enabled + * \sa rcl_get_enforcement_policy + * \sa rcl_get_secure_root + * + * \param name[in] name used to find the securiy root path. + * \param namespace_[in] namespace_ used to find the security root path. + * \param allocator[in] used to do allocations. + * \param security_options[out] security options that will be configured according to + * the environment. + */ +RCL_PUBLIC +rcl_ret_t +rcl_get_security_options_from_environment( + const char * name, + const char * namespace_, + const rcutils_allocator_t * allocator, + rmw_security_options_t * security_options); + /// Check if security has to be used, according to the environment. /** + * If `ROS_SECURITY_ENABLE` environment variable is set to "true", `use_security` will be set to + * true. + * * \param use_security[out] Must not be NULL. * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or * \returns RCL_RET_ERROR if an unexpected error happened, or @@ -56,10 +81,14 @@ extern "C" */ RCL_PUBLIC rcl_ret_t -rcl_use_security(bool * use_security); +rcl_security_enabled(bool * use_security); /// Get security enforcement policy from the environment. /** + * Sets `policy` based on the value of `ROS_SECURITY_STRATEGY` environment variable. + * If `ROS_SECURITY_STRATEGY` is "Enforce", `policy` will be `RMW_SECURITY_ENFORCEMENT_ENFORCE`. + * If not, `policy` will be `RMW_SECURITY_ENFORCEMENT_PERMISSIVE`. + * * \param policy[out] Must not be NULL. * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or * \returns RCL_RET_ERROR if an unexpected error happened, or @@ -69,30 +98,39 @@ RCL_PUBLIC rcl_ret_t rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); -/// Return the secure root directory associated with a node given its validated name and namespace. +/// Return the secure root given a name and namespace. /** - * E.g. for a node named "c" in namespace "/a/b", the secure root path will be - * "a/b/c", where the delimiter "/" is native for target file system (e.g. "\\" for _WIN32). - * If no exact match is found for the node name, a best match would be used instead + * The returned security directory is associated with the node or context depending on the + * rmw implementation. + * + * The value of the environment variable `ROS_SECURITY_ROOT_DIRECTORY` is used as a root. + * The specific directory to be used, is found from that root using the `name` and `namespace_` + * passed. + * E.g. for a node/context named "c" in namespace "/a/b" 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). + * + * If `ROS_SECURITY_LOOKUP_TYPE_VAR_NAME` is set to `MATCH_PREFIX`, when no exact match is found for + * the node/context name, a best match would be used instead * (by performing longest-prefix matching). * - * However, this expansion can be overridden by setting the secure node directory environment - * variable, allowing users to explicitly specify the exact secure root directory to be utilized. - * Such an override is useful for where the FQN of a node is non-deterministic before runtime, - * or when testing and using additional tools that may not otherwise be easily provisioned. + * Only for rmw implementations that associate a security directory with a node: + * However, this expansion can be overridden by setting the secure node directory environment + * (`ROS_SECURITY_NODE_DIRECTORY`) variable, allowing users to explicitly specify the exact secure + * root directory to be utilized. + * Such an override is useful for where the FQN of a node is non-deterministic before runtime, + * or when testing and using additional tools that may not otherwise be easily provisioned. * - * \param[in] node_name validated node name (a single token) - * \param[in] node_namespace validated, absolute namespace (starting with "/") + * \param[in] name validated name (a single token) + * \param[in] namespace_ validated, absolute namespace (starting with "/") * \param[in] allocator the allocator to use for allocation - * \returns machine specific (absolute) node secure root path or NULL on failure - * returned pointer must be deallocated by the caller of this function + * \returns Machine specific (absolute) secure root path or NULL on failure. + * Returned pointer must be deallocated by the caller of this function */ RCL_PUBLIC char * rcl_get_secure_root( - const char * node_name, - const char * node_namespace, - const rcl_allocator_t * allocator -); + const char * name, + const char * namespace_, + const rcl_allocator_t * allocator); #ifdef __cplusplus } diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 2cd929b8a..17b72739a 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -32,6 +32,7 @@ extern "C" #include "rcutils/logging_macros.h" #include "rcutils/stdatomic_helper.h" #include "rmw/error_handling.h" +#include "rmw/security.h" #include "tracetools/tracetools.h" static atomic_uint_least64_t __rcl_next_unique_id = ATOMIC_VAR_INIT(1); @@ -161,26 +162,18 @@ rcl_init( goto fail; } - // TODO(ivanpauno): Uncomment this when new path discovery magic is ready. - // bool use_security = false; - // ret = rcl_use_security(&use_security); - // if (RCL_RET_OK != ret) { - // fail_ret = ret; - // goto fail; - // } - // RCUTILS_LOG_DEBUG_NAMED( - // ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); - - // ret = rcl_get_enforcement_policy( - // &context->impl->init_options.impl->rmw_init_options.security_options.enforce_security); - // if (RCL_RET_OK != ret) { - // fail_ret = ret; - // goto fail; - // } - - // if (use_security) { - // ... - // } + if (!rmw_use_node_name_in_security_directory_lookup()) { + rmw_security_options_t * security_options = + &context->impl->init_options.impl->rmw_init_options.security_options; + ret = rcl_get_security_options_from_environment( + context->impl->rmw_context.options.name, + context->impl->rmw_context.options.namespace_, + &context->impl->allocator, + security_options); + if (RMW_RET_OK != ret) { + goto fail; + } + } // Initialize rmw_init. rmw_ret_t rmw_ret = rmw_init( diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index a00ed574c..5b21d483e 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -47,6 +47,7 @@ extern "C" #include "rmw/error_handling.h" #include "rmw/security_options.h" #include "rmw/rmw.h" +#include "rmw/security.h" #include "rmw/validate_namespace.h" #include "rmw/validate_node_name.h" #include "tracetools/tracetools.h" @@ -128,7 +129,6 @@ rcl_node_init( rcl_ret_t ret; rcl_ret_t fail_ret = RCL_RET_ERROR; char * remapped_node_name = NULL; - char * node_secure_root = NULL; // Check options and allocator first, so allocator can be used for errors. RCL_CHECK_ARGUMENT_FOR_NULL(options, RCL_RET_INVALID_ARGUMENT); @@ -256,7 +256,7 @@ rcl_node_init( node->impl->logger_name, "creating logger name failed", goto fail); domain_id = node->impl->options.domain_id; - if (RMW_RET_OK != rcl_domain_id(&domain_id)) { + if (RCL_RET_OK != rcl_domain_id(&domain_id)) { goto fail; } RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id); @@ -264,35 +264,16 @@ rcl_node_init( rmw_security_options_t node_security_options = rmw_get_zero_initialized_security_options(); - - bool use_security; - ret = rcl_use_security(&use_security); - if (RCL_RET_OK != ret) { - fail_ret = ret; - goto fail; - } - RCUTILS_LOG_DEBUG_NAMED( - ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); - - ret = rcl_get_enforcement_policy(&node_security_options.enforce_security); - if (RCL_RET_OK != ret) { - fail_ret = ret; - goto fail; - } - - if (!use_security) { - node_security_options.enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE; - } else { // if use_security - // File discovery magic here - node_secure_root = rcl_get_secure_root(name, local_namespace_, allocator); - if (node_secure_root) { - RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", node_secure_root); - node_security_options.security_root_path = node_secure_root; - } else { - if (RMW_SECURITY_ENFORCEMENT_ENFORCE == node_security_options.enforce_security) { - ret = RCL_RET_ERROR; - goto cleanup; - } + rmw_security_options_t * node_security_options_ptr = NULL; + if (rmw_use_node_name_in_security_directory_lookup()) { + node_security_options_ptr = &node_security_options; + ret = rcl_get_security_options_from_environment( + name, + local_namespace_, + allocator, + node_security_options_ptr); + if (RMW_RET_OK != ret) { + goto fail; } } @@ -302,7 +283,7 @@ rcl_node_init( node->impl->rmw_node_handle = rmw_create_node( &(node->context->impl->rmw_context), - name, local_namespace_, domain_id, &node_security_options, localhost_only); + name, local_namespace_, domain_id, node_security_options_ptr, localhost_only); RCL_CHECK_FOR_NULL_WITH_MSG( node->impl->rmw_node_handle, rmw_get_error_string().str, goto fail); @@ -403,7 +384,7 @@ rcl_node_init( if (NULL != remapped_node_name) { allocator->deallocate(remapped_node_name, allocator->state); } - allocator->deallocate(node_secure_root, allocator->state); + rmw_security_options_fini(node_security_options_ptr, allocator); return ret; } diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index 292f16db1..df581f220 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -17,9 +17,14 @@ #include #include "rcl/error_handling.h" + +#include "rcutils/logging_macros.h" #include "rcutils/filesystem.h" -#include "rcutils/get_env.h" #include "rcutils/format_string.h" +#include "rcutils/get_env.h" +#include "rcutils/strdup.h" + +#include "rmw/security.h" #include "rmw/security_options.h" #ifdef __clang__ @@ -32,7 +37,49 @@ #endif rcl_ret_t -rcl_use_security(bool * use_security) +rcl_get_security_options_from_environment( + const char * name, + const char * namespace_, + const rcutils_allocator_t * allocator, + rmw_security_options_t * security_options) +{ + bool use_security = false; + rcl_ret_t ret = rcl_security_enabled(&use_security); + if (RCL_RET_OK != ret) { + return ret; + } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); + + if (!rmw_use_node_name_in_security_directory_lookup()) { + ret = rcl_get_enforcement_policy(&security_options->enforce_security); + if (RCL_RET_OK != ret) { + return ret; + } + + if (!use_security) { + security_options->enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE; + } else { // if use_security + // File discovery magic here + char * secure_root = rcl_get_secure_root( + name, + namespace_, + allocator); + if (secure_root) { + RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root); + security_options->security_root_path = secure_root; + } else { + if (RMW_SECURITY_ENFORCEMENT_ENFORCE == security_options->enforce_security) { + return RCL_RET_ERROR; + } + } + } + } + return RCL_RET_OK; +} + +rcl_ret_t +rcl_security_enabled(bool * use_security) { const char * ros_security_enable = NULL; const char * get_env_error_str = NULL; @@ -227,20 +274,22 @@ char * rcl_get_secure_root( const rcl_allocator_t * allocator) { bool ros_secure_node_override = true; + bool use_node_name_in_lookup = rmw_use_node_name_in_security_directory_lookup(); // find out if either of the configuration environment variables are set const char * env_buf = NULL; if (NULL == node_name) { return NULL; } - if (rcutils_get_env(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME, &env_buf)) { - return NULL; - } - if (!env_buf) { - return NULL; + if (use_node_name_in_lookup) { + if (rcutils_get_env(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME, &env_buf)) { + return NULL; + } + if (!env_buf) { + return NULL; + } } - size_t ros_secure_root_size = strlen(env_buf); - if (!ros_secure_root_size) { + if (!use_node_name_in_lookup || 0 == strcmp("", env_buf)) { // check root directory if node directory environment variable is empty if (rcutils_get_env(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME, &env_buf)) { return NULL; @@ -248,8 +297,7 @@ char * rcl_get_secure_root( if (!env_buf) { return NULL; } - ros_secure_root_size = strlen(env_buf); - if (!ros_secure_root_size) { + if (0 == strcmp("", env_buf)) { return NULL; // environment variable was empty } else { ros_secure_node_override = false; @@ -257,20 +305,13 @@ char * rcl_get_secure_root( } // found a usable environment variable, copy into our memory before overwriting with next lookup - char * ros_secure_root_env = - (char *)allocator->allocate(ros_secure_root_size + 1, allocator->state); - memcpy(ros_secure_root_env, env_buf, ros_secure_root_size + 1); - // TODO(ros2team): This make an assumption on the value and length of the root namespace. - // This should likely come from another (rcl/rmw?) function for reuse. - // If the namespace is the root namespace ("/"), the secure root is just the node name. + char * ros_secure_root_env = rcutils_strdup(env_buf, *allocator); char * lookup_strategy = NULL; char * node_secure_root = NULL; if (ros_secure_node_override) { - node_secure_root = (char *)allocator->allocate(ros_secure_root_size + 1, allocator->state); - memcpy(node_secure_root, ros_secure_root_env, ros_secure_root_size + 1); + node_secure_root = rcutils_strdup(ros_secure_root_env, *allocator); lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_NODE_OVERRIDE]; - } else { // Check which lookup method to use and invoke the relevant function. const char * ros_security_lookup_type = NULL; diff --git a/rcl/test/rcl/test_security_directory.cpp b/rcl/test/rcl/test_security_directory.cpp index 71f0ebf16..24a8bba24 100644 --- a/rcl/test/rcl/test_security_directory.cpp +++ b/rcl/test/rcl/test_security_directory.cpp @@ -14,12 +14,18 @@ #include -#include #include +#include + #include "rcl/security.h" +#include "rcl/error_handling.h" + #include "rcutils/filesystem.h" + +#include "rmw/security.h" + #include "osrf_testing_tools_cpp/scope_exit.hpp" -#include "rcl/error_handling.h" + #define ROOT_NAMESPACE "/" #define TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME "test_security_directory" @@ -106,6 +112,7 @@ TEST_F(TestGetSecureRoot, failureScenarios) { ASSERT_EQ( rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), (char *) NULL); + rcl_reset_error(); putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); @@ -114,10 +121,12 @@ TEST_F(TestGetSecureRoot, failureScenarios) { ASSERT_EQ( rcl_get_secure_root(TEST_NODE_NAME, "/some_other_namespace", &allocator), (char *) NULL); + rcl_reset_error(); /// Wrong node name ASSERT_EQ( rcl_get_secure_root("not_" TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), (char *) NULL); + rcl_reset_error(); } TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) { @@ -196,31 +205,37 @@ TEST_F(TestGetSecureRoot, successScenarios_root_prefixMatch) { } TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { - /* Specify a valid directory */ - putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - root_path = rcl_get_secure_root( - "name shouldn't matter", "namespace shouldn't matter", &allocator); - ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); + if (rmw_use_node_name_in_security_directory_lookup()) { + /* Specify a valid directory */ + putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + root_path = rcl_get_secure_root( + "name shouldn't matter", "namespace shouldn't matter", &allocator); + ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); + } } TEST_F( TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory_overrideRootDirectoryAttempt) { - /* Setting root dir has no effect */ - putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - root_path = rcl_get_secure_root( - "name shouldn't matter", "namespace shouldn't matter", &allocator); - putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); + if (rmw_use_node_name_in_security_directory_lookup()) { + /* Setting root dir has no effect */ + putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + root_path = rcl_get_secure_root( + "name shouldn't matter", "namespace 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, 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_NODE_DIRECTORY_VAR_NAME - "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); - ASSERT_EQ( - rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), - (char *) NULL); + if (rmw_use_node_name_in_security_directory_lookup()) { + putenv_wrapper( + ROS_SECURITY_NODE_DIRECTORY_VAR_NAME + "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); + ASSERT_EQ( + rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), + (char *) NULL); + } } From d160c8de500ea17178d3b4d9ce0fc6af902153df Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 31 Jan 2020 14:11:14 -0300 Subject: [PATCH 04/30] Only destroy security options when they were created Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/node.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 5b21d483e..aeee29eac 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -129,6 +129,9 @@ rcl_node_init( rcl_ret_t ret; rcl_ret_t fail_ret = RCL_RET_ERROR; char * remapped_node_name = NULL; + rmw_security_options_t node_security_options = + rmw_get_zero_initialized_security_options(); + rmw_security_options_t * node_security_options_ptr = NULL; // Check options and allocator first, so allocator can be used for errors. RCL_CHECK_ARGUMENT_FOR_NULL(options, RCL_RET_INVALID_ARGUMENT); @@ -262,9 +265,6 @@ rcl_node_init( RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id); node->impl->actual_domain_id = domain_id; - rmw_security_options_t node_security_options = - rmw_get_zero_initialized_security_options(); - rmw_security_options_t * node_security_options_ptr = NULL; if (rmw_use_node_name_in_security_directory_lookup()) { node_security_options_ptr = &node_security_options; ret = rcl_get_security_options_from_environment( @@ -384,7 +384,9 @@ rcl_node_init( if (NULL != remapped_node_name) { allocator->deallocate(remapped_node_name, allocator->state); } - rmw_security_options_fini(node_security_options_ptr, allocator); + if (node_security_options_ptr) { + rmw_security_options_fini(node_security_options_ptr, allocator); + } return ret; } From d8cd5d5b7706ff340fcee779de3daffd226ea022 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 31 Jan 2020 16:38:47 -0300 Subject: [PATCH 05/30] Avoid mentioning node in security.* Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/security.c | 115 +++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 57 deletions(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index df581f220..dac769b1f 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -124,27 +124,27 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy) } /** - * A security lookup function takes in the node's name, namespace, a security root directory and an allocator; + * A security lookup function takes in the node/context's name, namespace, a security root directory and an allocator; * It returns the relevant information required to load the security credentials, * which is currently a path to a directory on the filesystem containing DDS Security permission files. */ typedef char * (* security_lookup_fn_t) ( - const char * node_name, - const char * node_namespace, + const char * name, + const char * namespace_, const char * ros_secure_root_env, const rcl_allocator_t * allocator ); char * exact_match_lookup( - const char * node_name, - const char * node_namespace, + const char * name, + const char * namespace_, const char * ros_secure_root_env, const rcl_allocator_t * allocator ); char * prefix_match_lookup( - const char * node_name, - const char * node_namespace, + const char * name, + const char * namespace_, const char * ros_secure_root_env, const rcl_allocator_t * allocator ); @@ -168,25 +168,25 @@ char * g_security_lookup_type_strings[] = { "MATCH_PREFIX" }; -/// Return the directory whose name most closely matches node_name (longest-prefix match), +/// Return the directory whose name most closely matches name (longest-prefix match), /// scanning under base_dir. /** - * By using a prefix match, a node named e.g. "my_node_123" will be able to load and use the - * directory "my_node" if no better match exists. + * By using a prefix match, a name "my_name_123" will be able to load and use the + * directory "my_name" if no better match exists. * \param[in] base_dir - * \param[in] node_name + * \param[in] name * \param[out] matched_name must be a valid memory address allocated with at least * _TINYDIR_FILENAME_MAX characters. * \return true if a match was found */ static bool get_best_matching_directory( const char * base_dir, - const char * node_name, + const char * name, char * matched_name) { size_t max_match_length = 0; tinydir_dir dir; - if (NULL == base_dir || NULL == node_name || NULL == matched_name) { + if (NULL == base_dir || NULL == name || NULL == matched_name) { return false; } if (-1 == tinydir_open(&dir, base_dir)) { @@ -199,9 +199,10 @@ static bool get_best_matching_directory( } if (file.is_dir) { size_t matched_name_length = strnlen(file.name, sizeof(file.name) - 1); - if ( - 0 == strncmp(file.name, node_name, matched_name_length) && - matched_name_length > max_match_length) + if (0 == + strncmp( + file.name, name, matched_name_length) && + matched_name_length > max_match_length) { max_match_length = matched_name_length; memcpy(matched_name, file.name, max_match_length); @@ -217,60 +218,60 @@ static bool get_best_matching_directory( } char * exact_match_lookup( - const char * node_name, - const char * node_namespace, + const char * name, + const char * namespace_, const char * ros_secure_root_env, const rcl_allocator_t * allocator) { - // Perform an exact match for the node's name in directory /. - char * node_secure_root = NULL; + // Perform an exact match for the node/context's name in directory /. + char * secure_root = NULL; // "/" case when root namespace is explicitly passed in - if (1 == strlen(node_namespace)) { - node_secure_root = rcutils_join_path(ros_secure_root_env, node_name, *allocator); + if (1 == strlen(namespace_)) { + secure_root = rcutils_join_path(ros_secure_root_env, name, *allocator); } else { - char * node_fqn = NULL; - char * node_root_path = NULL; - // Combine node namespace with node name + char * fqn = NULL; + char * root_path = NULL; + // Combine namespace with name // TODO(ros2team): remove the hard-coded value of the root namespace - node_fqn = rcutils_format_string(*allocator, "%s%s%s", node_namespace, "/", node_name); + fqn = rcutils_format_string(*allocator, "%s%s%s", namespace_, "/", name); // Get native path, ignore the leading forward slash // TODO(ros2team): remove the hard-coded length, use the length of the root namespace instead - node_root_path = rcutils_to_native_path(node_fqn + 1, *allocator); - node_secure_root = rcutils_join_path(ros_secure_root_env, node_root_path, *allocator); - allocator->deallocate(node_fqn, allocator->state); - allocator->deallocate(node_root_path, allocator->state); + root_path = rcutils_to_native_path(fqn + 1, *allocator); + secure_root = rcutils_join_path(ros_secure_root_env, root_path, *allocator); + allocator->deallocate(fqn, allocator->state); + allocator->deallocate(root_path, allocator->state); } - return node_secure_root; + return secure_root; } char * prefix_match_lookup( - const char * node_name, - const char * node_namespace, + const char * name, + const char * namespace_, const char * ros_secure_root_env, const rcl_allocator_t * allocator) { - // Perform longest prefix match for the node's name in directory /. - char * node_secure_root = NULL; + // Perform longest prefix match for the node/context's name in directory /. + char * secure_root = NULL; char matched_dir[_TINYDIR_FILENAME_MAX] = {0}; char * base_lookup_dir = NULL; - if (strlen(node_namespace) == 1) { + if (strlen(namespace_) == 1) { base_lookup_dir = (char *) ros_secure_root_env; } else { // TODO(ros2team): remove the hard-coded length, use the length of the root namespace instead. - base_lookup_dir = rcutils_join_path(ros_secure_root_env, node_namespace + 1, *allocator); + base_lookup_dir = rcutils_join_path(ros_secure_root_env, namespace_ + 1, *allocator); } - if (get_best_matching_directory(base_lookup_dir, node_name, matched_dir)) { - node_secure_root = rcutils_join_path(base_lookup_dir, matched_dir, *allocator); + if (get_best_matching_directory(base_lookup_dir, name, matched_dir)) { + secure_root = rcutils_join_path(base_lookup_dir, matched_dir, *allocator); } if (base_lookup_dir != ros_secure_root_env && NULL != base_lookup_dir) { allocator->deallocate(base_lookup_dir, allocator->state); } - return node_secure_root; + return secure_root; } char * rcl_get_secure_root( - const char * node_name, - const char * node_namespace, + const char * name, + const char * namespace_, const rcl_allocator_t * allocator) { bool ros_secure_node_override = true; @@ -278,7 +279,7 @@ char * rcl_get_secure_root( // find out if either of the configuration environment variables are set const char * env_buf = NULL; - if (NULL == node_name) { + if (NULL == name) { return NULL; } if (use_node_name_in_lookup) { @@ -308,9 +309,9 @@ char * rcl_get_secure_root( char * ros_secure_root_env = rcutils_strdup(env_buf, *allocator); char * lookup_strategy = NULL; - char * node_secure_root = NULL; + char * secure_root = NULL; if (ros_secure_node_override) { - node_secure_root = rcutils_strdup(ros_secure_root_env, *allocator); + secure_root = rcutils_strdup(ros_secure_root_env, *allocator); lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_NODE_OVERRIDE]; } else { // Check which lookup method to use and invoke the relevant function. @@ -324,32 +325,32 @@ char * rcl_get_secure_root( ros_security_lookup_type, g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_PREFIX])) { - node_secure_root = g_security_lookup_fns[ROS_SECURITY_LOOKUP_MATCH_PREFIX] - (node_name, node_namespace, ros_secure_root_env, allocator); + secure_root = g_security_lookup_fns[ROS_SECURITY_LOOKUP_MATCH_PREFIX] + (name, namespace_, ros_secure_root_env, allocator); lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_PREFIX]; } else { /* Default is MATCH_EXACT */ - node_secure_root = g_security_lookup_fns[ROS_SECURITY_LOOKUP_MATCH_EXACT] - (node_name, node_namespace, ros_secure_root_env, allocator); + secure_root = g_security_lookup_fns[ROS_SECURITY_LOOKUP_MATCH_EXACT] + (name, namespace_, ros_secure_root_env, allocator); lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_EXACT]; } } - if (NULL == node_secure_root || !rcutils_is_directory(node_secure_root)) { - // Check node_secure_root is not NULL before checking directory - if (NULL == node_secure_root) { + 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 node name '%s' in '%s%s'. " + "SECURITY ERROR: unable to find a folder matching the name '%s' in '%s%s'. " "Lookup strategy: %s", - node_name, ros_secure_root_env, node_namespace, lookup_strategy); + name, ros_secure_root_env, namespace_, lookup_strategy); } else { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( "SECURITY ERROR: directory '%s' does not exist. Lookup strategy: %s", - node_secure_root, lookup_strategy); + secure_root, lookup_strategy); } allocator->deallocate(ros_secure_root_env, allocator->state); - allocator->deallocate(node_secure_root, allocator->state); + allocator->deallocate(secure_root, allocator->state); return NULL; } allocator->deallocate(ros_secure_root_env, allocator->state); - return node_secure_root; + return secure_root; } From 9c5d5941790e60689e38d1a6ee04df7c032db275 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 4 Feb 2020 16:18:19 -0300 Subject: [PATCH 06/30] Fix test_info_by_topic Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_info_by_topic.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcl/test/rcl/test_info_by_topic.cpp b/rcl/test/rcl/test_info_by_topic.cpp index 7e7b67724..fa1680a86 100644 --- a/rcl/test/rcl/test_info_by_topic.cpp +++ b/rcl/test/rcl/test_info_by_topic.cpp @@ -364,7 +364,7 @@ TEST_F( &this->node, &allocator, fqdn.c_str(), false, &topic_endpoint_info_array_pub); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; - EXPECT_EQ(topic_endpoint_info_array_pub.size, 1u) << "Expected one publisher"; + ASSERT_EQ(topic_endpoint_info_array_pub.size, 1u) << "Expected one publisher"; rmw_topic_endpoint_info_t topic_endpoint_info_pub = topic_endpoint_info_array_pub.info_array[0]; EXPECT_STREQ(topic_endpoint_info_pub.node_name, this->test_graph_node_name); EXPECT_STREQ(topic_endpoint_info_pub.node_namespace, "/"); @@ -377,7 +377,7 @@ TEST_F( &this->node, &allocator, fqdn.c_str(), false, &topic_endpoint_info_array_sub); EXPECT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; - EXPECT_EQ(topic_endpoint_info_array_sub.size, 1u) << "Expected one subscription"; + ASSERT_EQ(topic_endpoint_info_array_sub.size, 1u) << "Expected one subscription"; rmw_topic_endpoint_info_t topic_endpoint_info_sub = topic_endpoint_info_array_sub.info_array[0]; EXPECT_STREQ(topic_endpoint_info_sub.node_name, this->test_graph_node_name); EXPECT_STREQ(topic_endpoint_info_sub.node_namespace, "/"); From 61acc14d9116794b2c3f83e757d267676796c7ae Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 4 Feb 2020 17:39:27 -0300 Subject: [PATCH 07/30] Add const where possible Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/security.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index dac769b1f..a52052cca 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -149,7 +149,7 @@ char * prefix_match_lookup( const rcl_allocator_t * allocator ); -security_lookup_fn_t g_security_lookup_fns[] = { +const security_lookup_fn_t g_security_lookup_fns[] = { NULL, exact_match_lookup, prefix_match_lookup, @@ -162,7 +162,7 @@ typedef enum ros_security_lookup_type_e ROS_SECURITY_LOOKUP_MATCH_PREFIX = 2, } ros_security_lookup_type_t; -char * g_security_lookup_type_strings[] = { +const char * const g_security_lookup_type_strings[] = { "NODE_OVERRIDE", "MATCH_EXACT", "MATCH_PREFIX" @@ -308,7 +308,7 @@ char * rcl_get_secure_root( // found a usable environment variable, copy into our memory before overwriting with next lookup char * ros_secure_root_env = rcutils_strdup(env_buf, *allocator); - char * lookup_strategy = NULL; + const char * lookup_strategy = NULL; char * secure_root = NULL; if (ros_secure_node_override) { secure_root = rcutils_strdup(ros_secure_root_env, *allocator); From 859929ce83ae523cb30c89cc584da2b324188b4d Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 5 Feb 2020 18:25:03 -0300 Subject: [PATCH 08/30] Please linters Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/security.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index a52052cca..d79f05272 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -202,7 +202,7 @@ static bool get_best_matching_directory( if (0 == strncmp( file.name, name, matched_name_length) && - matched_name_length > max_match_length) + matched_name_length > max_match_length) { max_match_length = matched_name_length; memcpy(matched_name, file.name, max_match_length); From 48864b8bf6a9337104c41ad57ce0c65349cb48ee Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 17 Feb 2020 16:45:08 -0300 Subject: [PATCH 09/30] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/domain_id.h | 10 +++---- rcl/include/rcl/localhost.h | 8 +++--- rcl/include/rcl/security.h | 22 +++++++-------- rcl/src/rcl/domain_id.c | 6 +---- rcl/src/rcl/init.c | 28 +++++++++++-------- rcl/src/rcl/localhost.c | 6 +---- rcl/src/rcl/node.c | 12 ++++++--- rcl/src/rcl/security.c | 54 +++++++++++++++++-------------------- 8 files changed, 69 insertions(+), 77 deletions(-) diff --git a/rcl/include/rcl/domain_id.h b/rcl/include/rcl/domain_id.h index 0259197c4..5cb696aec 100644 --- a/rcl/include/rcl/domain_id.h +++ b/rcl/include/rcl/domain_id.h @@ -30,20 +30,16 @@ extern "C" extern const char * const RCL_DOMAIN_ID_ENV_VAR; -/// Determine the default domain id, based on the environment. +/// Determine the default domain ID, based on the environment. /** - * Checks the default domain id based on ROS_DOMAIN_ID environment variable, if - * the original `domain_id` was RCL_DEFAULT_DOMAIN_ID. - * If not, the input `domain_id` is not modified. - * - * \param[inout] domain_id Must not be NULL. + * \param[out] domain_id Must not be NULL. * \returns RCL_RET_INVALID_ARGUMENT if an argument is invalid, or, * \returns RCL_RET_ERROR in case of an unexpected error, or, * \returns RCL_RET_OK. */ RCL_PUBLIC rcl_ret_t -rcl_domain_id(size_t * domain_id); +rcl_get_default_domain_id(size_t * domain_id); #ifdef __cplusplus } diff --git a/rcl/include/rcl/localhost.h b/rcl/include/rcl/localhost.h index d40ea6eb1..551b36702 100644 --- a/rcl/include/rcl/localhost.h +++ b/rcl/include/rcl/localhost.h @@ -28,18 +28,16 @@ extern const char * const RCL_LOCALHOST_ENV_VAR; /// Determine if the user wants to communicate using loopback only. /** - * Checks if localhost should be used for network communication. - * If `localhost_only` is RMW_LOCALHOST_ONLY_DEFAULT, ROS_LOCALHOST_ONLY env variable is used. - * If not, `localhost_only` is not modified. + * Checks if localhost should be used for network communication based on environment. * - * \param[inout] localhost_only Must not be NULL. + * \param[out] localhost_only Must not be NULL. * \returns RCL_RET_INVALID_ARGUMENT if an argument is invalid, or * \returns RCL_RET_ERROR if an unexpected error happened, or * \returns RCL_RET_OK. */ RCL_PUBLIC rcl_ret_t -rcl_localhost_only(rmw_localhost_only_t * localhost_only); +rcl_get_localhost_only(rmw_localhost_only_t * localhost_only); #ifdef __cplusplus } diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index 097c1dad9..51bdfdd57 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -28,23 +28,23 @@ extern "C" #include "rmw/security_options.h" #ifndef ROS_SECURITY_NODE_DIRECTORY_VAR_NAME - #define ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "ROS_SECURITY_NODE_DIRECTORY" +# define ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "ROS_SECURITY_NODE_DIRECTORY" #endif #ifndef ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME - #define ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "ROS_SECURITY_ROOT_DIRECTORY" +# define ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "ROS_SECURITY_ROOT_DIRECTORY" #endif #ifndef ROS_SECURITY_LOOKUP_TYPE_VAR_NAME - #define ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "ROS_SECURITY_LOOKUP_TYPE" +# define ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "ROS_SECURITY_LOOKUP_TYPE" #endif #ifndef ROS_SECURITY_STRATEGY_VAR_NAME -#define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY" +# define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY" #endif #ifndef ROS_SECURITY_ENABLE_VAR_NAME -#define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE" +# define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE" #endif /// Init the security options from the values of the environment variables and passed names. @@ -55,10 +55,10 @@ extern "C" * \sa rcl_get_enforcement_policy * \sa rcl_get_secure_root * - * \param name[in] name used to find the securiy root path. - * \param namespace_[in] namespace_ used to find the security root path. - * \param allocator[in] used to do allocations. - * \param security_options[out] security options that will be configured according to + * \param[in] name name used to find the securiy root path. + * \param[in] namespace_ namespace_ used to find the security root path. + * \param[in] allocator used to do allocations. + * \param[out] security_options security options that will be configured according to * the environment. */ RCL_PUBLIC @@ -74,7 +74,7 @@ rcl_get_security_options_from_environment( * If `ROS_SECURITY_ENABLE` environment variable is set to "true", `use_security` will be set to * true. * - * \param use_security[out] Must not be NULL. + * \param[out] use_security Must not be NULL. * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or * \returns RCL_RET_ERROR if an unexpected error happened, or * \returns RCL_RET_OK. @@ -89,7 +89,7 @@ rcl_security_enabled(bool * use_security); * If `ROS_SECURITY_STRATEGY` is "Enforce", `policy` will be `RMW_SECURITY_ENFORCEMENT_ENFORCE`. * If not, `policy` will be `RMW_SECURITY_ENFORCEMENT_PERMISSIVE`. * - * \param policy[out] Must not be NULL. + * \param[out] policy Must not be NULL. * \returns RCL_RET_INVALID_ARGUMENT if an argument is not valid, or * \returns RCL_RET_ERROR if an unexpected error happened, or * \returns RCL_RET_OK. diff --git a/rcl/src/rcl/domain_id.c b/rcl/src/rcl/domain_id.c index aa9592749..c6c4d6392 100644 --- a/rcl/src/rcl/domain_id.c +++ b/rcl/src/rcl/domain_id.c @@ -24,7 +24,7 @@ const char * const RCL_DOMAIN_ID_ENV_VAR = "ROS_DOMAIN_ID"; rcl_ret_t -rcl_domain_id(size_t * domain_id) +rcl_get_default_domain_id(size_t * domain_id) { const char * ros_domain_id; const char * get_env_error_str; @@ -33,10 +33,6 @@ rcl_domain_id(size_t * domain_id) return RCL_RET_INVALID_ARGUMENT; } - if (RCL_DEFAULT_DOMAIN_ID != *domain_id) { - return RCL_RET_OK; - } - get_env_error_str = rcutils_get_env(RCL_DOMAIN_ID_ENV_VAR, &ros_domain_id); if (NULL != get_env_error_str) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 17b72739a..20cc1fa09 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -148,18 +148,25 @@ rcl_init( rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id); context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id; - // Get actual domain id based on environment variable, if needed. - ret = rcl_domain_id(&context->impl->init_options.impl->rmw_init_options.domain_id); - if (RCL_RET_OK != ret) { - fail_ret = ret; - goto fail; + size_t * domain_id = &context->impl->init_options.impl->rmw_init_options.domain_id; + if (RCL_DEFAULT_DOMAIN_ID == *domain_id) { + // Get actual domain id based on environment variable. + ret = rcl_get_default_domain_id(domain_id); + if (RCL_RET_OK != ret) { + fail_ret = ret; + goto fail; + } } - // Get actual localhost_only value based on environment variable, if needed. - ret = rcl_localhost_only(&context->impl->init_options.impl->rmw_init_options.localhost_only); - if (RCL_RET_OK != ret) { - fail_ret = ret; - goto fail; + rmw_localhost_only_t * localhost_only = + &context->impl->init_options.impl->rmw_init_options.localhost_only; + if (RMW_LOCALHOST_ONLY_DEFAULT == *localhost_only) { + // Get actual localhost_only value based on environment variable, if needed. + ret = rcl_get_localhost_only(localhost_only); + if (RCL_RET_OK != ret) { + fail_ret = ret; + goto fail; + } } if (!rmw_use_node_name_in_security_directory_lookup()) { @@ -186,7 +193,6 @@ rcl_init( } TRACEPOINT(rcl_init, (const void *)context); - return RCL_RET_OK; fail: __cleanup_context(context); diff --git a/rcl/src/rcl/localhost.c b/rcl/src/rcl/localhost.c index 2165c6cc1..a798c4783 100644 --- a/rcl/src/rcl/localhost.c +++ b/rcl/src/rcl/localhost.c @@ -25,7 +25,7 @@ const char * const RCL_LOCALHOST_ENV_VAR = "ROS_LOCALHOST_ONLY"; rcl_ret_t -rcl_localhost_only(rmw_localhost_only_t * localhost_only) +rcl_get_localhost_only(rmw_localhost_only_t * localhost_only) { const char * ros_local_host_env_val = NULL; const char * get_env_error_str = NULL; @@ -34,10 +34,6 @@ rcl_localhost_only(rmw_localhost_only_t * localhost_only) return RCL_RET_INVALID_ARGUMENT; } - if (RMW_LOCALHOST_ONLY_DEFAULT == *localhost_only) { - return RCL_RET_OK; - } - get_env_error_str = rcutils_get_env(RCL_LOCALHOST_ENV_VAR, &ros_local_host_env_val); if (NULL != get_env_error_str) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index aeee29eac..4b733f4ab 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -259,8 +259,10 @@ rcl_node_init( node->impl->logger_name, "creating logger name failed", goto fail); domain_id = node->impl->options.domain_id; - if (RCL_RET_OK != rcl_domain_id(&domain_id)) { - goto fail; + if (RCL_DEFAULT_DOMAIN_ID == domain_id) { + if (RCL_RET_OK != rcl_get_default_domain_id(&domain_id)) { + goto fail; + } } RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id); node->impl->actual_domain_id = domain_id; @@ -277,8 +279,10 @@ rcl_node_init( } } - if (RMW_RET_OK != rcl_localhost_only(&localhost_only)) { - goto fail; + if (RMW_LOCALHOST_ONLY_DEFAULT == localhost_only) { + if (RMW_RET_OK != rcl_get_localhost_only(&localhost_only)) { + goto fail; + } } node->impl->rmw_node_handle = rmw_create_node( diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index d79f05272..abaab057b 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -48,31 +48,31 @@ rcl_get_security_options_from_environment( if (RCL_RET_OK != ret) { return ret; } + + if (!use_security) { + security_options->enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE; + return RMW_RET_OK; + } + RCUTILS_LOG_DEBUG_NAMED( ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); - if (!rmw_use_node_name_in_security_directory_lookup()) { - ret = rcl_get_enforcement_policy(&security_options->enforce_security); - if (RCL_RET_OK != ret) { - return ret; - } + ret = rcl_get_enforcement_policy(&security_options->enforce_security); + if (RCL_RET_OK != ret) { + return ret; + } - if (!use_security) { - security_options->enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE; - } else { // if use_security - // File discovery magic here - char * secure_root = rcl_get_secure_root( - name, - namespace_, - allocator); - if (secure_root) { - RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root); - security_options->security_root_path = secure_root; - } else { - if (RMW_SECURITY_ENFORCEMENT_ENFORCE == security_options->enforce_security) { - return RCL_RET_ERROR; - } - } + // File discovery magic here + char * secure_root = rcl_get_secure_root( + name, + namespace_, + allocator); + if (secure_root) { + RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root); + security_options->security_root_path = secure_root; + } else { + if (RMW_SECURITY_ENFORCEMENT_ENFORCE == security_options->enforce_security) { + return RCL_RET_ERROR; } } return RCL_RET_OK; @@ -84,9 +84,7 @@ rcl_security_enabled(bool * use_security) const char * ros_security_enable = NULL; const char * get_env_error_str = NULL; - if (!use_security) { - return RCL_RET_INVALID_ARGUMENT; - } + RCL_CHECK_ARGUMENT_FOR_NULL(use_security, RCL_RET_INVALID_ARGUMENT); get_env_error_str = rcutils_get_env(ROS_SECURITY_ENABLE_VAR_NAME, &ros_security_enable); if (NULL != get_env_error_str) { @@ -106,9 +104,7 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy) const char * ros_enforce_security = NULL; const char * get_env_error_str = NULL; - if (!policy) { - return RCL_RET_INVALID_ARGUMENT; - } + RCL_CHECK_ARGUMENT_FOR_NULL(policy, RCL_RET_INVALID_ARGUMENT); get_env_error_str = rcutils_get_env(ROS_SECURITY_STRATEGY_VAR_NAME, &ros_enforce_security); if (NULL != get_env_error_str) { @@ -126,7 +122,7 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy) /** * A security lookup function takes in the node/context's name, namespace, a security root directory and an allocator; * It returns the relevant information required to load the security credentials, - * which is currently a path to a directory on the filesystem containing DDS Security permission files. + * which is currently a path to a directory on the filesystem containing DDS Security permission files. */ typedef char * (* security_lookup_fn_t) ( const char * name, @@ -168,7 +164,7 @@ const char * const g_security_lookup_type_strings[] = { "MATCH_PREFIX" }; -/// Return the directory whose name most closely matches name (longest-prefix match), +/// Return the directory that most closely matches a given name (longest-prefix match), /// scanning under base_dir. /** * By using a prefix match, a name "my_name_123" will be able to load and use the From deb4f944d92c9e40b60f8fae4ee45a2cff0318f1 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 18 Feb 2020 16:58:33 -0300 Subject: [PATCH 10/30] Support overriding the security directory Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/security.h | 15 ++++++------ rcl/src/rcl/security.c | 29 ++++++++++++------------ rcl/test/rcl/test_security_directory.cpp | 8 +++---- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index 51bdfdd57..60e4734e4 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -27,8 +27,8 @@ extern "C" #include "rcl/visibility_control.h" #include "rmw/security_options.h" -#ifndef ROS_SECURITY_NODE_DIRECTORY_VAR_NAME -# define ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "ROS_SECURITY_NODE_DIRECTORY" +#ifndef ROS_SECURITY_DIRECTORY_OVERRIDE +# define ROS_SECURITY_DIRECTORY_OVERRIDE "ROS_SECURITY_DIRECTORY_OVERRIDE" #endif #ifndef ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME @@ -113,12 +113,11 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); * the node/context name, a best match would be used instead * (by performing longest-prefix matching). * - * Only for rmw implementations that associate a security directory with a node: - * However, this expansion can be overridden by setting the secure node directory environment - * (`ROS_SECURITY_NODE_DIRECTORY`) variable, allowing users to explicitly specify the exact secure - * root directory to be utilized. - * Such an override is useful for where the FQN of a node is non-deterministic before runtime, - * or when testing and using additional tools that may not otherwise be easily provisioned. + * 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. + * Such an override is useful for where the FQN of a node/context 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] namespace_ validated, absolute namespace (starting with "/") diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index abaab057b..d34b80d72 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -153,13 +153,13 @@ const security_lookup_fn_t g_security_lookup_fns[] = { typedef enum ros_security_lookup_type_e { - ROS_SECURITY_LOOKUP_NODE_OVERRIDE = 0, + ROS_SECURITY_LOOKUP_OVERRIDE = 0, ROS_SECURITY_LOOKUP_MATCH_EXACT = 1, ROS_SECURITY_LOOKUP_MATCH_PREFIX = 2, } ros_security_lookup_type_t; const char * const g_security_lookup_type_strings[] = { - "NODE_OVERRIDE", + "DIRECTORY_OVERRIDE", "MATCH_EXACT", "MATCH_PREFIX" }; @@ -270,7 +270,7 @@ char * rcl_get_secure_root( const char * namespace_, const rcl_allocator_t * allocator) { - bool ros_secure_node_override = true; + bool ros_secure_directory_override = true; bool use_node_name_in_lookup = rmw_use_node_name_in_security_directory_lookup(); // find out if either of the configuration environment variables are set @@ -278,16 +278,15 @@ char * rcl_get_secure_root( if (NULL == name) { return NULL; } - if (use_node_name_in_lookup) { - if (rcutils_get_env(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME, &env_buf)) { - return NULL; - } - if (!env_buf) { - return NULL; - } + + if (rcutils_get_env(ROS_SECURITY_DIRECTORY_OVERRIDE, &env_buf)) { + return NULL; + } + if (!env_buf) { + return NULL; } - if (!use_node_name_in_lookup || 0 == strcmp("", env_buf)) { - // check root directory if node directory environment variable is empty + 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; } @@ -297,7 +296,7 @@ char * rcl_get_secure_root( if (0 == strcmp("", env_buf)) { return NULL; // environment variable was empty } else { - ros_secure_node_override = false; + ros_secure_directory_override = false; } } @@ -306,9 +305,9 @@ char * rcl_get_secure_root( const char * lookup_strategy = NULL; char * secure_root = NULL; - if (ros_secure_node_override) { + if (ros_secure_directory_override) { secure_root = rcutils_strdup(ros_secure_root_env, *allocator); - lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_NODE_OVERRIDE]; + lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_OVERRIDE]; } else { // Check which lookup method to use and invoke the relevant function. const char * ros_security_lookup_type = NULL; diff --git a/rcl/test/rcl/test_security_directory.cpp b/rcl/test/rcl/test_security_directory.cpp index 24a8bba24..b9facb3d8 100644 --- a/rcl/test/rcl/test_security_directory.cpp +++ b/rcl/test/rcl/test_security_directory.cpp @@ -66,7 +66,7 @@ class TestGetSecureRoot : public ::testing::Test // 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_NODE_DIRECTORY_VAR_NAME); + unsetenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE); unsetenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME); allocator = rcl_get_default_allocator(); root_path = nullptr; @@ -207,7 +207,7 @@ TEST_F(TestGetSecureRoot, successScenarios_root_prefixMatch) { TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { if (rmw_use_node_name_in_security_directory_lookup()) { /* Specify a valid directory */ - putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); root_path = rcl_get_secure_root( "name shouldn't matter", "namespace shouldn't matter", &allocator); ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); @@ -219,7 +219,7 @@ TEST_F( nodeSecurityDirectoryOverride_validDirectory_overrideRootDirectoryAttempt) { if (rmw_use_node_name_in_security_directory_lookup()) { /* Setting root dir has no effect */ - putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); root_path = rcl_get_secure_root( "name shouldn't matter", "namespace shouldn't matter", &allocator); putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); @@ -232,7 +232,7 @@ TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) { * if the node override is invalid. */ if (rmw_use_node_name_in_security_directory_lookup()) { putenv_wrapper( - ROS_SECURITY_NODE_DIRECTORY_VAR_NAME + ROS_SECURITY_DIRECTORY_OVERRIDE "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); ASSERT_EQ( rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), From 08653d06428b03afb334b8f52de2bd02a0c99ed6 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 18 Feb 2020 16:59:48 -0300 Subject: [PATCH 11/30] Rename test_security_directory to test_security Signed-off-by: Ivan Santiago Paunovic --- rcl/test/CMakeLists.txt | 4 ++-- .../rcl/{test_security_directory.cpp => test_security.cpp} | 0 2 files changed, 2 insertions(+), 2 deletions(-) rename rcl/test/rcl/{test_security_directory.cpp => test_security.cpp} (100%) diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index 2bcbbb2a0..77505b150 100644 --- a/rcl/test/CMakeLists.txt +++ b/rcl/test/CMakeLists.txt @@ -317,8 +317,8 @@ rcl_add_custom_gtest(test_timer${target_suffix} AMENT_DEPENDENCIES "osrf_testing_tools_cpp" ) -rcl_add_custom_gtest(test_security_directory - SRCS rcl/test_security_directory.cpp +rcl_add_custom_gtest(test_security + SRCS rcl/test_security.cpp APPEND_LIBRARY_DIRS ${extra_lib_dirs} LIBRARIES ${PROJECT_NAME} AMENT_DEPENDENCIES "osrf_testing_tools_cpp" diff --git a/rcl/test/rcl/test_security_directory.cpp b/rcl/test/rcl/test_security.cpp similarity index 100% rename from rcl/test/rcl/test_security_directory.cpp rename to rcl/test/rcl/test_security.cpp From 9134305c7ce8381264b375b9532ac3c7fffc20d1 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 18 Feb 2020 17:27:51 -0300 Subject: [PATCH 12/30] Reenable tests for security directory override Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_security.cpp | 40 +++++++++++++++------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp index b9facb3d8..7af9bc24e 100644 --- a/rcl/test/rcl/test_security.cpp +++ b/rcl/test/rcl/test_security.cpp @@ -205,37 +205,31 @@ TEST_F(TestGetSecureRoot, successScenarios_root_prefixMatch) { } TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { - if (rmw_use_node_name_in_security_directory_lookup()) { - /* Specify a valid directory */ - putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); - root_path = rcl_get_secure_root( - "name shouldn't matter", "namespace shouldn't matter", &allocator); - ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); - } + /* Specify a valid directory */ + putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); + root_path = rcl_get_secure_root( + "name shouldn't matter", "namespace shouldn't matter", &allocator); + ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); } TEST_F( TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory_overrideRootDirectoryAttempt) { - if (rmw_use_node_name_in_security_directory_lookup()) { - /* 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", "namespace shouldn't matter", &allocator); - putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); - } + /* 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", "namespace 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, nodeSecurityDirectoryOverride_invalidDirectory) { /* The override provided should exist. Providing correct node/namespace/root dir won't help * if the node override is invalid. */ - if (rmw_use_node_name_in_security_directory_lookup()) { - putenv_wrapper( - ROS_SECURITY_DIRECTORY_OVERRIDE - "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); - ASSERT_EQ( - rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), - (char *) NULL); - } + putenv_wrapper( + ROS_SECURITY_DIRECTORY_OVERRIDE + "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); + ASSERT_EQ( + rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), + (char *) NULL); } From b4fe23ac1041eb3666c3ffe8c0a66341e54f2767 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 18 Feb 2020 18:29:57 -0300 Subject: [PATCH 13/30] Add test case for 'rcl_get_security_options_from_environment' Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_security.cpp | 44 ++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp index 7af9bc24e..9fc708305 100644 --- a/rcl/test/rcl/test_security.cpp +++ b/rcl/test/rcl/test_security.cpp @@ -22,6 +22,7 @@ #include "rcutils/filesystem.h" +#include "rmw/error_handling.h" #include "rmw/security.h" #include "osrf_testing_tools_cpp/scope_exit.hpp" @@ -32,6 +33,12 @@ #define TEST_NODE_NAME "dummy_node" #define TEST_NODE_NAMESPACE ROOT_NAMESPACE TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME +#ifndef _WIN32 +# define PATH_SEPARATOR "/" +#else +# define PATH_SEPARATOR "\\" +#endif + char g_envstring[512] = {0}; static int putenv_wrapper(const char * env_var) @@ -68,6 +75,8 @@ class TestGetSecureRoot : public ::testing::Test unsetenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME); unsetenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE); unsetenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME); + unsetenv_wrapper(ROS_SECURITY_STRATEGY_VAR_NAME); + unsetenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME); allocator = rcl_get_default_allocator(); root_path = nullptr; secure_root = nullptr; @@ -233,3 +242,38 @@ TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) { rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), (char *) NULL); } + +TEST_F(TestGetSecureRoot, test_get_security_options) { + /* The override provided should exist. Providing correct node/namespace/root dir won't help + * if the node override is invalid. */ + rmw_security_options_t options = rmw_get_zero_initialized_security_options(); + putenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME "=false"); + rcl_ret_t ret = rcl_get_security_options_from_environment( + "doesn't matter at all", "doesn't matter at all", &allocator, &options); + ASSERT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str; + EXPECT_EQ(RMW_SECURITY_ENFORCEMENT_PERMISSIVE, options.enforce_security); + EXPECT_EQ(NULL, options.security_root_path); + + putenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME "=true"); + putenv_wrapper(ROS_SECURITY_STRATEGY_VAR_NAME "=Enforce"); + + putenv_wrapper( + ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); + ret = rcl_get_security_options_from_environment( + "doesn't matter at all", "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_EQ(RMW_RET_OK, rmw_security_options_fini(&options, &allocator)); + + options = rmw_get_zero_initialized_security_options(); + unsetenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE); + putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + ret = rcl_get_security_options_from_environment( + TEST_NODE_NAME, TEST_NODE_NAMESPACE, &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 TEST_NODE_NAMESPACE PATH_SEPARATOR TEST_NODE_NAME, + options.security_root_path); +} From f7d94061870de363617f4e6ce8aad3ec8596e6ba Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 27 Feb 2020 18:11:53 -0300 Subject: [PATCH 14/30] Delete unused local variable Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/security.c | 1 - 1 file changed, 1 deletion(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index d34b80d72..df318a27c 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -271,7 +271,6 @@ char * rcl_get_secure_root( const rcl_allocator_t * allocator) { bool ros_secure_directory_override = true; - bool use_node_name_in_lookup = rmw_use_node_name_in_security_directory_lookup(); // find out if either of the configuration environment variables are set const char * env_buf = NULL; From 717c444f7ac0d7ba59ac1d526e39f02f814c9c2a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 28 Feb 2020 16:27:40 -0300 Subject: [PATCH 15/30] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/security.h | 4 ++-- rcl/src/rcl/domain_id.c | 4 ++-- rcl/src/rcl/init.c | 1 + rcl/src/rcl/security.c | 8 ++++---- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index 60e4734e4..14b6c8f80 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -47,9 +47,9 @@ extern "C" # define ROS_SECURITY_ENABLE_VAR_NAME "ROS_SECURITY_ENABLE" #endif -/// Init the security options from the values of the environment variables and passed names. +/// Initialize security options from values in the environment variables and given names. /** - * Inits the passed security options based on the environment. + * Initialize the given security options based on the environment. * For more details: * \sa rcl_security_enabled * \sa rcl_get_enforcement_policy diff --git a/rcl/src/rcl/domain_id.c b/rcl/src/rcl/domain_id.c index c6c4d6392..84b705281 100644 --- a/rcl/src/rcl/domain_id.c +++ b/rcl/src/rcl/domain_id.c @@ -26,8 +26,8 @@ const char * const RCL_DOMAIN_ID_ENV_VAR = "ROS_DOMAIN_ID"; rcl_ret_t rcl_get_default_domain_id(size_t * domain_id) { - const char * ros_domain_id; - const char * get_env_error_str; + const char * ros_domain_id = NULL; + const char * get_env_error_str = NULL; if (!domain_id) { return RCL_RET_INVALID_ARGUMENT; diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 20cc1fa09..506760d86 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -193,6 +193,7 @@ rcl_init( } TRACEPOINT(rcl_init, (const void *)context); + return RCL_RET_OK; fail: __cleanup_context(context); diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index df318a27c..c60f1c65a 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -49,14 +49,14 @@ rcl_get_security_options_from_environment( return ret; } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); + if (!use_security) { security_options->enforce_security = RMW_SECURITY_ENFORCEMENT_PERMISSIVE; return RMW_RET_OK; } - RCUTILS_LOG_DEBUG_NAMED( - ROS_PACKAGE_NAME, "Using security: %s", use_security ? "true" : "false"); - ret = rcl_get_enforcement_policy(&security_options->enforce_security); if (RCL_RET_OK != ret) { return ret; @@ -201,7 +201,7 @@ static bool get_best_matching_directory( matched_name_length > max_match_length) { max_match_length = matched_name_length; - memcpy(matched_name, file.name, max_match_length); + strncpy(matched_name, file.name, max_match_length + 1); } } if (-1 == tinydir_next(&dir)) { From ea4575fff8d749f6172d713e3e1118d5e0feba84 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 12 Mar 2020 11:36:14 -0300 Subject: [PATCH 16/30] Latest update after discussion about supporting sros2 Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/arguments.h | 1 + rcl/src/rcl/arguments.c | 68 ++++++++++++++++++++++++++++++++++++ rcl/src/rcl/arguments_impl.h | 3 ++ rcl/src/rcl/context.c | 6 ++-- rcl/src/rcl/init.c | 58 +++++++++++++++++++----------- rcl/src/rcl/node.c | 20 +---------- 6 files changed, 114 insertions(+), 42 deletions(-) diff --git a/rcl/include/rcl/arguments.h b/rcl/include/rcl/arguments.h index 0e301a207..e3b068f0e 100644 --- a/rcl/include/rcl/arguments.h +++ b/rcl/include/rcl/arguments.h @@ -42,6 +42,7 @@ typedef struct rcl_arguments_t #define RCL_PARAM_FILE_FLAG "--params-file" #define RCL_REMAP_FLAG "--remap" #define RCL_SHORT_REMAP_FLAG "-r" +#define RCL_SECURITY_DIRECTORY_FLAG "--security-directory" #define RCL_LOG_LEVEL_FLAG "--log-level" #define RCL_EXTERNAL_LOG_CONFIG_FLAG "--log-config-file" // To be prefixed with --enable- or --disable- diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 15fd3d796..f92852b03 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -245,6 +245,22 @@ _rcl_parse_param_file_rule( rcl_params_t * params, char ** param_file); +/// Parse a security directory argument. +/** + * \param[in] arg the argument to parse + * \param[in] allocator an allocator to use + * \param[in,out] security_directory parsed security directory + * \return RCL_RET_OK if a valid security directory was parsed, or + * \return RCL_RET_BAD_ALLOC if an allocation failed, or + * \return RLC_RET_ERROR if an unspecified error occurred. + */ +RCL_LOCAL +rcl_ret_t +_rcl_parse_security_directory( + const char * arg, + rcl_allocator_t allocator, + char ** security_directory); + #define RCL_ENABLE_FLAG_PREFIX "--enable-" #define RCL_DISABLE_FLAG_PREFIX "--disable-" @@ -533,6 +549,39 @@ rcl_parse_arguments( ret = RCL_RET_INVALID_ROS_ARGS; goto fail; } + + // Attempt to parse argument as a security directory + if (strcmp(RCL_SECURITY_DIRECTORY_FLAG, argv[i]) == 0) { + if (i + 1 < argc) { + if (NULL != args_impl->security_directory) { + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Overriding security directory path : %s\n", + args_impl->security_directory); + allocator.deallocate(args_impl->security_directory, allocator.state); + args_impl->security_directory = NULL; + } + if (RCL_RET_OK == _rcl_parse_security_directory( + argv[i + 1], allocator, &args_impl->security_directory)) + { + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "Got security directory : %s\n", + args_impl->security_directory); + ++i; // Skip flag here, for loop will skip value. + continue; + } + rcl_error_string_t prev_error_string = rcl_get_error_string(); + rcl_reset_error(); + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Couldn't parse security directory path: '%s %s'. Error: %s", argv[i], argv[i + 1], + prev_error_string.str); + } else { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Couldn't parse trailing %s flag. No security directory path provided.", argv[i]); + } + ret = RCL_RET_INVALID_ROS_ARGS; + goto fail; + } + RCUTILS_LOG_DEBUG_NAMED( ROS_PACKAGE_NAME, "Arg %d (%s) is not a %s flag.", i, argv[i], RCL_EXTERNAL_LOG_CONFIG_FLAG); @@ -1098,6 +1147,7 @@ rcl_arguments_fini( args->impl->num_param_files_args = 0; args->impl->parameter_files = NULL; } + args->impl->allocator.deallocate(args->impl->security_directory, args->impl->allocator.state); if (NULL != args->impl->external_log_config_file) { args->impl->allocator.deallocate( @@ -2003,6 +2053,23 @@ _rcl_parse_external_log_config_file_rule( return RCL_RET_INVALID_PARAM_RULE; } +rcl_ret_t +_rcl_parse_security_directory( + const char * arg, + rcl_allocator_t allocator, + char ** security_directory) +{ + RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(security_directory, RCL_RET_INVALID_ARGUMENT); + + *security_directory = rcutils_strdup(arg, allocator); + if (NULL == *security_directory) { + RCL_SET_ERROR_MSG("Failed to allocate memory for security directory path"); + return RCL_RET_BAD_ALLOC; + } + return RCL_RET_OK; +} + rcl_ret_t _rcl_parse_disabling_flag( const char * arg, @@ -2105,6 +2172,7 @@ _rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t args_impl->log_stdout_disabled = false; args_impl->log_rosout_disabled = false; args_impl->log_ext_lib_disabled = false; + args_out->impl->security_directory = NULL; args_impl->allocator = *allocator; return RCL_RET_OK; diff --git a/rcl/src/rcl/arguments_impl.h b/rcl/src/rcl/arguments_impl.h index 9065164c9..da708b0c3 100644 --- a/rcl/src/rcl/arguments_impl.h +++ b/rcl/src/rcl/arguments_impl.h @@ -61,6 +61,9 @@ typedef struct rcl_arguments_impl_t /// A boolean value indicating if the external lib handler should be used for log output bool log_ext_lib_disabled; + /// Security directory to be used. + char * security_directory; + /// Allocator used to allocate objects in this struct rcl_allocator_t allocator; } rcl_arguments_impl_t; diff --git a/rcl/src/rcl/context.c b/rcl/src/rcl/context.c index e9d5695a9..0f6df4da3 100644 --- a/rcl/src/rcl/context.c +++ b/rcl/src/rcl/context.c @@ -108,7 +108,7 @@ __cleanup_context(rcl_context_t * context) rcl_ret_t ret = rcl_arguments_fini(&(context->global_arguments)); if (RCL_RET_OK != ret) { RCUTILS_SAFE_FWRITE_TO_STDERR( - "[rcl|init.c:" RCUTILS_STRINGIFY(__LINE__) + "[rcl|context.c:" RCUTILS_STRINGIFY(__LINE__) "] failed to finalize global arguments while cleaning up context, memory may be leaked: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); @@ -126,7 +126,7 @@ __cleanup_context(rcl_context_t * context) rcl_ret_t ret = rcl_init_options_fini(&(context->impl->init_options)); if (RCL_RET_OK != ret) { RCUTILS_SAFE_FWRITE_TO_STDERR( - "[rcl|init.c:" RCUTILS_STRINGIFY(__LINE__) + "[rcl|context.c:" RCUTILS_STRINGIFY(__LINE__) "] failed to finalize init options while cleaning up context, memory may be leaked: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); @@ -139,7 +139,7 @@ __cleanup_context(rcl_context_t * context) rmw_ret_t rmw_ret = rmw_context_fini(&(context->impl->rmw_context)); if (RMW_RET_OK != rmw_ret) { RCUTILS_SAFE_FWRITE_TO_STDERR( - "[rcl|init.c:" RCUTILS_STRINGIFY(__LINE__) + "[rcl|context.c:" RCUTILS_STRINGIFY(__LINE__) "] failed to finalize rmw context while cleaning up context, memory may be leaked: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcutils_get_error_string().str); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 506760d86..2a9a99926 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -19,21 +19,26 @@ extern "C" #include "rcl/init.h" -#include "./arguments_impl.h" -#include "./common.h" -#include "./context_impl.h" -#include "./init_options_impl.h" +#include "rcutils/logging_macros.h" +#include "rcutils/stdatomic_helper.h" +#include "rcutils/strdup.h" + +#include "rmw/error_handling.h" +#include "rmw/security.h" + +#include "tracetools/tracetools.h" + #include "rcl/arguments.h" #include "rcl/domain_id.h" #include "rcl/error_handling.h" #include "rcl/localhost.h" #include "rcl/logging.h" #include "rcl/security.h" -#include "rcutils/logging_macros.h" -#include "rcutils/stdatomic_helper.h" -#include "rmw/error_handling.h" -#include "rmw/security.h" -#include "tracetools/tracetools.h" + +#include "./arguments_impl.h" +#include "./common.h" +#include "./context_impl.h" +#include "./init_options_impl.h" static atomic_uint_least64_t __rcl_next_unique_id = ATOMIC_VAR_INIT(1); @@ -169,17 +174,30 @@ rcl_init( } } - if (!rmw_use_node_name_in_security_directory_lookup()) { - rmw_security_options_t * security_options = - &context->impl->init_options.impl->rmw_init_options.security_options; - ret = rcl_get_security_options_from_environment( - context->impl->rmw_context.options.name, - context->impl->rmw_context.options.namespace_, - &context->impl->allocator, - security_options); - if (RMW_RET_OK != ret) { - goto fail; - } + if (context->global_arguments.impl->security_directory) { + context->impl->init_options.impl->rmw_init_options.name = rcutils_strdup( + context->global_arguments.impl->security_directory, + context->impl->allocator); + } else { + context->impl->init_options.impl->rmw_init_options.name = rcutils_strdup( + "/", context->impl->allocator); + } + if (!context->impl->init_options.impl->rmw_init_options.name) { + RCL_SET_ERROR_MSG("failed to set context name"); + fail_ret = RMW_RET_BAD_ALLOC; + goto fail; + } + + rmw_security_options_t * security_options = + &context->impl->init_options.impl->rmw_init_options.security_options; + ret = rcl_get_security_options_from_environment( + "", + context->impl->init_options.impl->rmw_init_options.name, + &context->impl->allocator, + security_options); + if (RMW_RET_OK != ret) { + fail_ret = ret; + goto fail; } // Initialize rmw_init. diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 4b733f4ab..68a92f87f 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -129,9 +129,6 @@ rcl_node_init( rcl_ret_t ret; rcl_ret_t fail_ret = RCL_RET_ERROR; char * remapped_node_name = NULL; - rmw_security_options_t node_security_options = - rmw_get_zero_initialized_security_options(); - rmw_security_options_t * node_security_options_ptr = NULL; // Check options and allocator first, so allocator can be used for errors. RCL_CHECK_ARGUMENT_FOR_NULL(options, RCL_RET_INVALID_ARGUMENT); @@ -267,18 +264,6 @@ rcl_node_init( RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Using domain ID of '%zu'", domain_id); node->impl->actual_domain_id = domain_id; - if (rmw_use_node_name_in_security_directory_lookup()) { - node_security_options_ptr = &node_security_options; - ret = rcl_get_security_options_from_environment( - name, - local_namespace_, - allocator, - node_security_options_ptr); - if (RMW_RET_OK != ret) { - goto fail; - } - } - if (RMW_LOCALHOST_ONLY_DEFAULT == localhost_only) { if (RMW_RET_OK != rcl_get_localhost_only(&localhost_only)) { goto fail; @@ -287,7 +272,7 @@ rcl_node_init( node->impl->rmw_node_handle = rmw_create_node( &(node->context->impl->rmw_context), - name, local_namespace_, domain_id, node_security_options_ptr, localhost_only); + name, local_namespace_, domain_id, localhost_only); RCL_CHECK_FOR_NULL_WITH_MSG( node->impl->rmw_node_handle, rmw_get_error_string().str, goto fail); @@ -388,9 +373,6 @@ rcl_node_init( if (NULL != remapped_node_name) { allocator->deallocate(remapped_node_name, allocator->state); } - if (node_security_options_ptr) { - rmw_security_options_fini(node_security_options_ptr, allocator); - } return ret; } From 4e48a4dadecf6d8a1d0db8b91c19e9e9f341dd8e Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 13 Mar 2020 13:40:47 -0300 Subject: [PATCH 17/30] Delete namespace from security root path functions. Delete security directory prefix lookup strategy. Add context name validation Signed-off-by: Ivan Santiago Paunovic --- rcl/CMakeLists.txt | 1 + rcl/include/rcl/security.h | 31 +-- rcl/include/rcl/validate_context_name.h | 91 +++++++++ rcl/src/rcl/init.c | 22 ++- rcl/src/rcl/security.c | 176 +----------------- rcl/src/rcl/validate_context_name.c | 146 +++++++++++++++ rcl/test/rcl/test_security.cpp | 121 ++++-------- .../{dummy_node => dummy_context}/.gitkeep | 0 8 files changed, 307 insertions(+), 281 deletions(-) create mode 100644 rcl/include/rcl/validate_context_name.h create mode 100644 rcl/src/rcl/validate_context_name.c rename rcl/test/resources/test_security_directory/{dummy_node => dummy_context}/.gitkeep (100%) diff --git a/rcl/CMakeLists.txt b/rcl/CMakeLists.txt index e04afd468..bbaf391f3 100644 --- a/rcl/CMakeLists.txt +++ b/rcl/CMakeLists.txt @@ -59,6 +59,7 @@ set(${PROJECT_NAME}_sources src/rcl/subscription.c src/rcl/time.c src/rcl/timer.c + src/rcl/validate_context_name.c src/rcl/validate_topic_name.c src/rcl/wait.c ) diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index 14b6c8f80..b3eb90e92 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -1,4 +1,4 @@ -// Copyright 2018 Open Source Robotics Foundation, Inc. +// Copyright 2018-2020 Open Source Robotics Foundation, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -35,10 +35,6 @@ extern "C" # define ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "ROS_SECURITY_ROOT_DIRECTORY" #endif -#ifndef ROS_SECURITY_LOOKUP_TYPE_VAR_NAME -# define ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "ROS_SECURITY_LOOKUP_TYPE" -#endif - #ifndef ROS_SECURITY_STRATEGY_VAR_NAME # define ROS_SECURITY_STRATEGY_VAR_NAME "ROS_SECURITY_STRATEGY" #endif @@ -56,7 +52,6 @@ extern "C" * \sa rcl_get_secure_root * * \param[in] name name used to find the securiy root path. - * \param[in] namespace_ namespace_ used to find the security root path. * \param[in] allocator used to do allocations. * \param[out] security_options security options that will be configured according to * the environment. @@ -65,7 +60,6 @@ RCL_PUBLIC rcl_ret_t rcl_get_security_options_from_environment( const char * name, - const char * namespace_, const rcutils_allocator_t * allocator, rmw_security_options_t * security_options); @@ -98,38 +92,29 @@ RCL_PUBLIC rcl_ret_t rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); -/// Return the secure root given a name and namespace. +/// Return the secure root given a context name. /** - * The returned security directory is associated with the node or context depending on the - * rmw implementation. + * Return the security directory associated with the context name. * * The value of the environment variable `ROS_SECURITY_ROOT_DIRECTORY` is used as a root. - * The specific directory to be used, is found from that root using the `name` and `namespace_` - * passed. - * E.g. for a node/context named "c" in namespace "/a/b" root "/r", the secure root path will be + * 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). * - * If `ROS_SECURITY_LOOKUP_TYPE_VAR_NAME` is set to `MATCH_PREFIX`, when no exact match is found for - * the node/context name, a best match would be used instead - * (by performing longest-prefix matching). - * * 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. - * Such an override is useful for where the FQN of a node/context is non-deterministic before runtime, + * Such an override is useful for where the context name 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] namespace_ validated, absolute namespace (starting with "/") * \param[in] allocator the allocator to use for allocation * \returns Machine specific (absolute) secure root path or NULL on failure. * Returned pointer must be deallocated by the caller of this function */ RCL_PUBLIC -char * rcl_get_secure_root( - const char * name, - const char * namespace_, - const rcl_allocator_t * allocator); +char * +rcl_get_secure_root(const char * name, const rcl_allocator_t * allocator); #ifdef __cplusplus } diff --git a/rcl/include/rcl/validate_context_name.h b/rcl/include/rcl/validate_context_name.h new file mode 100644 index 000000000..ab7957fd3 --- /dev/null +++ b/rcl/include/rcl/validate_context_name.h @@ -0,0 +1,91 @@ +// Copyright 2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef RCL__VALIDATE_CONTEXT_NAME_H_ +#define RCL__VALIDATE_CONTEXT_NAME_H_ + +#ifdef __cplusplus +extern "C" +{ +#endif + +#include "rmw/validate_namespace.h" +#include "rmw/validate_node_name.h" + +#include "rcl/macros.h" +#include "rcl/types.h" +#include "rcl/visibility_control.h" + +#define RCL_CONTEXT_NAME_VALID RMW_NAMESPACE_VALID +#define RCL_CONTEXT_NAME_INVALID_IS_EMPTY_STRING RMW_NAMESPACE_INVALID_IS_EMPTY_STRING +#define RCL_CONTEXT_NAME_INVALID_NOT_ABSOLUTE RMW_NAMESPACE_INVALID_NOT_ABSOLUTE +#define RCL_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH \ + RMW_NAMESPACE_INVALID_ENDS_WITH_FORWARD_SLASH +#define RCL_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS \ + RMW_NAMESPACE_INVALID_CONTAINS_UNALLOWED_CHARACTERS +#define RCL_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH \ + RMW_NAMESPACE_INVALID_CONTAINS_REPEATED_FORWARD_SLASH +#define RCL_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER \ + RMW_NAMESPACE_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER +#define RCL_CONTEXT_NAME_INVALID_TOO_LONG RMW_NAMESPACE_INVALID_TOO_LONG + +#define RCL_CONTEXT_NAME_MAX_LENGTH RMW_NODE_NAME_MAX_NAME_LENGTH + +/// Determine if a given context name is valid. +/** + * /sa The same rules as rmw_validate_namespace are used. + * The only difference is the maximum length, which can be 255 characters. + * + * \param[in] context_name context_name to be validated + * \param[out] validation_result int in which the result of the check is stored + * \param[out] invalid_index index of the input string where an error occurred + * \returns `RMW_RET_OK` on successfully running the check, or + * \returns `RMW_RET_INVALID_ARGUMENT` on invalid parameters, or + * \returns `RMW_RET_ERROR` when an unspecified error occurs. + */ +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_validate_context_name( + const char * context_name, + int * validation_result, + size_t * invalid_index); + +/// Deterimine if a given context name is valid. +/** + * This is an overload with an extra parameter for the length of context_name. + * \param[in] context_name The number of characters in context_name. + * + * \sa rcl_validate_context_name(const char *, int *, size_t *) + */ +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_validate_context_name_with_size( + const char * context_name, + size_t context_name_length, + int * validation_result, + size_t * invalid_index); + +/// Return a validation result description, or NULL if unknown or RCL_CONTEXT_NAME_VALID. +RCL_PUBLIC +RCL_WARN_UNUSED +const char * +rcl_context_name_validation_result_string(int validation_result); + +#ifdef __cplusplus +} +#endif + +#endif // RCL__VALIDATE_CONTEXT_NAME_H_ diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 2a9a99926..c27447c1c 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -34,6 +34,7 @@ extern "C" #include "rcl/localhost.h" #include "rcl/logging.h" #include "rcl/security.h" +#include "rcl/validate_context_name.h" #include "./arguments_impl.h" #include "./common.h" @@ -182,6 +183,26 @@ rcl_init( context->impl->init_options.impl->rmw_init_options.name = rcutils_strdup( "/", context->impl->allocator); } + int validation_result; + size_t invalid_index; + ret = rcl_validate_context_name( + context->impl->init_options.impl->rmw_init_options.name, + &validation_result, + &invalid_index); + if (RCL_RET_OK != ret) { + RCL_SET_ERROR_MSG("rcl_validate_context_name() failed"); + fail_ret = ret; + goto fail; + } + if (RCL_CONTEXT_NAME_VALID != validation_result) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "rcl_validate_context_name result is not valid: '%s'. Invalid index: %zu", + rcl_context_name_validation_result_string(validation_result), + invalid_index); + fail_ret = RMW_RET_ERROR; + goto fail; + } + if (!context->impl->init_options.impl->rmw_init_options.name) { RCL_SET_ERROR_MSG("failed to set context name"); fail_ret = RMW_RET_BAD_ALLOC; @@ -191,7 +212,6 @@ rcl_init( rmw_security_options_t * security_options = &context->impl->init_options.impl->rmw_init_options.security_options; ret = rcl_get_security_options_from_environment( - "", context->impl->init_options.impl->rmw_init_options.name, &context->impl->allocator, security_options); diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index c60f1c65a..6e73b7c86 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -1,4 +1,4 @@ -// Copyright 2018 Open Source Robotics Foundation, Inc. +// Copyright 2018-2020 Open Source Robotics Foundation, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -20,26 +20,15 @@ #include "rcutils/logging_macros.h" #include "rcutils/filesystem.h" -#include "rcutils/format_string.h" #include "rcutils/get_env.h" #include "rcutils/strdup.h" #include "rmw/security.h" #include "rmw/security_options.h" -#ifdef __clang__ -# pragma clang diagnostic push -# pragma clang diagnostic ignored "-Wembedded-directive" -#endif -#include "tinydir/tinydir.h" -#ifdef __clang__ -# pragma clang diagnostic pop -#endif - rcl_ret_t rcl_get_security_options_from_environment( const char * name, - const char * namespace_, const rcutils_allocator_t * allocator, rmw_security_options_t * security_options) { @@ -65,7 +54,6 @@ rcl_get_security_options_from_environment( // File discovery magic here char * secure_root = rcl_get_secure_root( name, - namespace_, allocator); if (secure_root) { RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root); @@ -119,155 +107,29 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy) return RCL_RET_OK; } -/** - * A security lookup function takes in the node/context's name, namespace, a security root directory and an allocator; - * It returns the relevant information required to load the security credentials, - * which is currently a path to a directory on the filesystem containing DDS Security permission files. - */ -typedef char * (* security_lookup_fn_t) ( - const char * name, - const char * namespace_, - const char * ros_secure_root_env, - const rcl_allocator_t * allocator -); - char * exact_match_lookup( const char * name, - const char * namespace_, - const char * ros_secure_root_env, - const rcl_allocator_t * allocator -); - -char * prefix_match_lookup( - const char * name, - const char * namespace_, - const char * ros_secure_root_env, - const rcl_allocator_t * allocator -); - -const security_lookup_fn_t g_security_lookup_fns[] = { - NULL, - exact_match_lookup, - prefix_match_lookup, -}; - -typedef enum ros_security_lookup_type_e -{ - ROS_SECURITY_LOOKUP_OVERRIDE = 0, - ROS_SECURITY_LOOKUP_MATCH_EXACT = 1, - ROS_SECURITY_LOOKUP_MATCH_PREFIX = 2, -} ros_security_lookup_type_t; - -const char * const g_security_lookup_type_strings[] = { - "DIRECTORY_OVERRIDE", - "MATCH_EXACT", - "MATCH_PREFIX" -}; - -/// Return the directory that most closely matches a given name (longest-prefix match), -/// scanning under base_dir. -/** - * By using a prefix match, a name "my_name_123" will be able to load and use the - * directory "my_name" if no better match exists. - * \param[in] base_dir - * \param[in] name - * \param[out] matched_name must be a valid memory address allocated with at least - * _TINYDIR_FILENAME_MAX characters. - * \return true if a match was found - */ -static bool get_best_matching_directory( - const char * base_dir, - const char * name, - char * matched_name) -{ - size_t max_match_length = 0; - tinydir_dir dir; - if (NULL == base_dir || NULL == name || NULL == matched_name) { - return false; - } - if (-1 == tinydir_open(&dir, base_dir)) { - return false; - } - while (dir.has_next) { - tinydir_file file; - if (-1 == tinydir_readfile(&dir, &file)) { - goto cleanup; - } - if (file.is_dir) { - size_t matched_name_length = strnlen(file.name, sizeof(file.name) - 1); - if (0 == - strncmp( - file.name, name, matched_name_length) && - matched_name_length > max_match_length) - { - max_match_length = matched_name_length; - strncpy(matched_name, file.name, max_match_length + 1); - } - } - if (-1 == tinydir_next(&dir)) { - goto cleanup; - } - } -cleanup: - tinydir_close(&dir); - return max_match_length > 0; -} - -char * exact_match_lookup( - const char * name, - const char * namespace_, const char * ros_secure_root_env, const rcl_allocator_t * allocator) { // Perform an exact match for the node/context's name in directory /. char * secure_root = NULL; // "/" case when root namespace is explicitly passed in - if (1 == strlen(namespace_)) { - secure_root = rcutils_join_path(ros_secure_root_env, name, *allocator); + if (0 == strcmp(name, "/")) { + secure_root = rcutils_strdup(ros_secure_root_env, *allocator); } else { - char * fqn = NULL; char * root_path = NULL; - // Combine namespace with name - // TODO(ros2team): remove the hard-coded value of the root namespace - fqn = rcutils_format_string(*allocator, "%s%s%s", namespace_, "/", name); // Get native path, ignore the leading forward slash // TODO(ros2team): remove the hard-coded length, use the length of the root namespace instead - root_path = rcutils_to_native_path(fqn + 1, *allocator); + root_path = rcutils_to_native_path(name + 1, *allocator); secure_root = rcutils_join_path(ros_secure_root_env, root_path, *allocator); - allocator->deallocate(fqn, allocator->state); allocator->deallocate(root_path, allocator->state); } return secure_root; } -char * prefix_match_lookup( - const char * name, - const char * namespace_, - const char * ros_secure_root_env, - const rcl_allocator_t * allocator) -{ - // Perform longest prefix match for the node/context's name in directory /. - char * secure_root = NULL; - char matched_dir[_TINYDIR_FILENAME_MAX] = {0}; - char * base_lookup_dir = NULL; - if (strlen(namespace_) == 1) { - base_lookup_dir = (char *) ros_secure_root_env; - } else { - // TODO(ros2team): remove the hard-coded length, use the length of the root namespace instead. - base_lookup_dir = rcutils_join_path(ros_secure_root_env, namespace_ + 1, *allocator); - } - if (get_best_matching_directory(base_lookup_dir, name, matched_dir)) { - secure_root = rcutils_join_path(base_lookup_dir, matched_dir, *allocator); - } - if (base_lookup_dir != ros_secure_root_env && NULL != base_lookup_dir) { - allocator->deallocate(base_lookup_dir, allocator->state); - } - return secure_root; -} - char * rcl_get_secure_root( const char * name, - const char * namespace_, const rcl_allocator_t * allocator) { bool ros_secure_directory_override = true; @@ -302,44 +164,22 @@ char * rcl_get_secure_root( // found a usable environment variable, copy into our memory before overwriting with next lookup char * ros_secure_root_env = rcutils_strdup(env_buf, *allocator); - const char * lookup_strategy = NULL; char * secure_root = NULL; if (ros_secure_directory_override) { secure_root = rcutils_strdup(ros_secure_root_env, *allocator); - lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_OVERRIDE]; } else { - // Check which lookup method to use and invoke the relevant function. - const char * ros_security_lookup_type = NULL; - if (rcutils_get_env(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME, &ros_security_lookup_type)) { - allocator->deallocate(ros_secure_root_env, allocator->state); - return NULL; - } - if ( - 0 == strcmp( - ros_security_lookup_type, - g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_PREFIX])) - { - secure_root = g_security_lookup_fns[ROS_SECURITY_LOOKUP_MATCH_PREFIX] - (name, namespace_, ros_secure_root_env, allocator); - lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_PREFIX]; - } else { /* Default is MATCH_EXACT */ - secure_root = g_security_lookup_fns[ROS_SECURITY_LOOKUP_MATCH_EXACT] - (name, namespace_, ros_secure_root_env, allocator); - lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_EXACT]; - } + secure_root = exact_match_lookup(name, ros_secure_root_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%s'. " - "Lookup strategy: %s", - name, ros_secure_root_env, namespace_, lookup_strategy); + "SECURITY ERROR: unable to find a folder matching the name '%s' in '%s'. ", + name, ros_secure_root_env); } else { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "SECURITY ERROR: directory '%s' does not exist. Lookup strategy: %s", - secure_root, lookup_strategy); + "SECURITY ERROR: directory '%s' does not exist.", secure_root); } allocator->deallocate(ros_secure_root_env, allocator->state); allocator->deallocate(secure_root, allocator->state); diff --git a/rcl/src/rcl/validate_context_name.c b/rcl/src/rcl/validate_context_name.c new file mode 100644 index 000000000..06b111f1d --- /dev/null +++ b/rcl/src/rcl/validate_context_name.c @@ -0,0 +1,146 @@ +// Copyright 2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "rcl/validate_context_name.h" + +#include +#include +#include + +#include + +#include "rmw/validate_namespace.h" + +#include "rcl/error_handling.h" + +#include "./common.h" + +rcl_ret_t +rcl_validate_context_name( + const char * context_name, + int * validation_result, + size_t * invalid_index) +{ + if (!context_name) { + return RCL_RET_INVALID_ARGUMENT; + } + return rmw_validate_namespace_with_size( + context_name, strlen(context_name), validation_result, invalid_index); +} + +rcl_ret_t +rcl_validate_context_name_with_size( + const char * context_name, + size_t context_name_length, + int * validation_result, + size_t * invalid_index) +{ + if (!context_name) { + return RCL_RET_INVALID_ARGUMENT; + } + if (!validation_result) { + return RCL_RET_INVALID_ARGUMENT; + } + + int t_validation_result; + size_t t_invalid_index; + rmw_ret_t ret = rmw_validate_namespace_with_size( + context_name, context_name_length, &t_validation_result, &t_invalid_index); + if (ret != RMW_RET_OK) { + return rcl_convert_rmw_ret_to_rcl_ret(ret); + } + + if (t_validation_result != RMW_NAMESPACE_VALID && + t_validation_result != RMW_NAMESPACE_INVALID_TOO_LONG) + { + switch (t_validation_result) { + case RMW_NAMESPACE_INVALID_IS_EMPTY_STRING: + *validation_result = RCL_CONTEXT_NAME_INVALID_IS_EMPTY_STRING; + break; + case RMW_NAMESPACE_INVALID_NOT_ABSOLUTE: + *validation_result = RCL_CONTEXT_NAME_INVALID_NOT_ABSOLUTE; + break; + case RMW_NAMESPACE_INVALID_ENDS_WITH_FORWARD_SLASH: + *validation_result = RCL_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH; + break; + case RMW_NAMESPACE_INVALID_CONTAINS_UNALLOWED_CHARACTERS: + *validation_result = RCL_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS; + break; + case RMW_NAMESPACE_INVALID_CONTAINS_REPEATED_FORWARD_SLASH: + *validation_result = RCL_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH; + break; + case RMW_NAMESPACE_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER: + *validation_result = RCL_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER; + break; + default: + { + char default_err_msg[256]; + // explicitly not taking return value which is number of bytes written + int ret = rcutils_snprintf( + default_err_msg, sizeof(default_err_msg), + "rcl_validate_context_name_with_size(): unknown rmw_validate_namespace_with_size()" + " result '%d'", + t_validation_result); + if (ret < 0) { + RCL_SET_ERROR_MSG("rcl_validate_context_name_with_size(): rcutils_snprintf() failed"); + } else { + RCL_SET_ERROR_MSG(default_err_msg); + } + } + return RCL_RET_ERROR; + } + if (invalid_index) { + *invalid_index = t_invalid_index; + } + return RCL_RET_OK; + } + + // context_name might be longer that namespace length, check false positives and correct + if (t_validation_result == RMW_NAMESPACE_INVALID_TOO_LONG && + context_name_length <= RCL_CONTEXT_NAME_MAX_LENGTH) + { + *validation_result = RCL_CONTEXT_NAME_VALID; + return RCL_RET_OK; + } + + // everything was ok, set result to valid namespace, avoid setting invalid_index, and return + *validation_result = RCL_CONTEXT_NAME_VALID; + return RCL_RET_OK; +} + +const char * +rcl_context_name_validation_result_string(int validation_result) +{ + switch (validation_result) { + case RCL_CONTEXT_NAME_VALID: + return NULL; + case RCL_CONTEXT_NAME_INVALID_IS_EMPTY_STRING: + return "context name must not be empty"; + case RCL_CONTEXT_NAME_INVALID_NOT_ABSOLUTE: + return "context name must be absolute, it must lead with a '/'"; + case RCL_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH: + return "context name must not end with a '/', unless only a '/'"; + case RCL_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS: + return "context name must not contain characters other than alphanumerics, '_', or '/'"; + case RCL_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH: + return "context name must not contain repeated '/'"; + case RCL_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER: + return "context name must not have a token that starts with a number"; + case RCL_CONTEXT_NAME_INVALID_TOO_LONG: + return "context name should not exceed '" + RCUTILS_STRINGIFY(RCL_CONTEXT_NAME_MAX_NAME_LENGTH) "'"; + default: + return "unknown result code for rcl context name validation"; + } +} diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp index 9fc708305..2878c5445 100644 --- a/rcl/test/rcl/test_security.cpp +++ b/rcl/test/rcl/test_security.cpp @@ -1,4 +1,4 @@ -// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -28,10 +28,9 @@ #include "osrf_testing_tools_cpp/scope_exit.hpp" -#define ROOT_NAMESPACE "/" -#define TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME "test_security_directory" -#define TEST_NODE_NAME "dummy_node" -#define TEST_NODE_NAMESPACE ROOT_NAMESPACE TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME +#define TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME "/test_security_directory" +#define TEST_CONTEXT_NAME "dummy_context" +#define TEST_CONTEXT_NAME_ABSOLUTE "/" TEST_CONTEXT_NAME #ifndef _WIN32 # define PATH_SEPARATOR "/" @@ -74,7 +73,6 @@ class TestGetSecureRoot : public ::testing::Test // 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_LOOKUP_TYPE_VAR_NAME); unsetenv_wrapper(ROS_SECURITY_STRATEGY_VAR_NAME); unsetenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME); allocator = rcl_get_default_allocator(); @@ -119,105 +117,48 @@ class TestGetSecureRoot : public ::testing::Test TEST_F(TestGetSecureRoot, failureScenarios) { ASSERT_EQ( - rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), + rcl_get_secure_root(TEST_CONTEXT_NAME_ABSOLUTE, &allocator), (char *) NULL); rcl_reset_error(); putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); /* Security directory is set, but there's no matching directory */ - /// Wrong namespace + /// Wrong context name ASSERT_EQ( - rcl_get_secure_root(TEST_NODE_NAME, "/some_other_namespace", &allocator), - (char *) NULL); - rcl_reset_error(); - /// Wrong node name - ASSERT_EQ( - rcl_get_secure_root("not_" TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), + rcl_get_secure_root("some_other_context_name", &allocator), (char *) NULL); rcl_reset_error(); } TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) { - putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + putenv_wrapper( + ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" + TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); - /* -------------------------- - * Namespace : Custom (local) - * Match type : Exact - * -------------------------- - * Root: ${CMAKE_BINARY_DIR}/tests/resources - * Namespace: /test_security_directory - * Node: dummy_node - */ - secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator); + secure_root = rcl_get_secure_root(TEST_CONTEXT_NAME_ABSOLUTE, &allocator); std::string secure_root_str(secure_root); ASSERT_STREQ( - TEST_NODE_NAME, - secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_NODE_NAME) - 1)).c_str()); -} - -TEST_F(TestGetSecureRoot, successScenarios_local_prefixMatch) { - putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator); - - /* -------------------------- - * Namespace : Custom (local) - * Match type : Prefix - * -------------------------- - * Root: ${CMAKE_BINARY_DIR}/tests/resources - * Namespace: /test_security_directory - * Node: dummy_node_and_some_suffix_added */ - root_path = rcl_get_secure_root( - TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE, &allocator); - ASSERT_STRNE(root_path, secure_root); - putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX"); - root_path = rcl_get_secure_root( - TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE, &allocator); - ASSERT_STREQ(root_path, secure_root); + TEST_CONTEXT_NAME, + secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_CONTEXT_NAME) - 1)).c_str()); } -TEST_F(TestGetSecureRoot, successScenarios_root_exactMatch) { +TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch_multipleTokensName) { putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX"); - secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator); - /* Include the namespace as part of the root security directory and test root namespace */ - set_base_lookup_dir_fqn(TEST_RESOURCES_DIRECTORY, TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); - /* -------------------------- - * Namespace : Root - * Match type : Exact - * -------------------------- - * Root: ${CMAKE_BINARY_DIR}/tests/resources/test_security_directory - * Namespace: / - * Node: dummy_node */ - root_path = rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE, &allocator); - ASSERT_STREQ(root_path, secure_root); -} - -TEST_F(TestGetSecureRoot, successScenarios_root_prefixMatch) { - putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); - putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX"); - secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator); - - /* Include the namespace as part of the root security directory and test root namespace */ - set_base_lookup_dir_fqn(TEST_RESOURCES_DIRECTORY, TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); - /* -------------------------- - * Namespace : Root - * Match type : Prefix - * -------------------------- - * Root dir: ${CMAKE_BINARY_DIR}/tests/resources/test_security_directory - * Namespace: / - * Node: dummy_node_and_some_suffix_added */ - root_path = rcl_get_secure_root( - TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE, &allocator); - ASSERT_STREQ(root_path, secure_root); + secure_root = rcl_get_secure_root( + TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME PATH_SEPARATOR TEST_CONTEXT_NAME, &allocator); + std::string secure_root_str(secure_root); + ASSERT_STREQ( + TEST_CONTEXT_NAME, + secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_CONTEXT_NAME) - 1)).c_str()); } TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { /* Specify a valid directory */ putenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); root_path = rcl_get_secure_root( - "name shouldn't matter", "namespace shouldn't matter", &allocator); + "name shouldn't matter", &allocator); ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); } @@ -227,7 +168,7 @@ TEST_F( /* 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", "namespace shouldn't matter", &allocator); + "name shouldn't matter", &allocator); putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); } @@ -239,17 +180,17 @@ TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) { ROS_SECURITY_DIRECTORY_OVERRIDE "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); ASSERT_EQ( - rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator), + rcl_get_secure_root(TEST_CONTEXT_NAME_ABSOLUTE, &allocator), (char *) NULL); } TEST_F(TestGetSecureRoot, test_get_security_options) { - /* The override provided should exist. Providing correct node/namespace/root dir won't help + /* The override provided should exist. Providing correct context name/root dir won't help * if the node override is invalid. */ rmw_security_options_t options = rmw_get_zero_initialized_security_options(); putenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME "=false"); rcl_ret_t ret = rcl_get_security_options_from_environment( - "doesn't matter at all", "doesn't matter at all", &allocator, &options); + "doesn't matter at all", &allocator, &options); ASSERT_EQ(RMW_RET_OK, ret) << rmw_get_error_string().str; EXPECT_EQ(RMW_SECURITY_ENFORCEMENT_PERMISSIVE, options.enforce_security); EXPECT_EQ(NULL, options.security_root_path); @@ -260,7 +201,7 @@ TEST_F(TestGetSecureRoot, test_get_security_options) { putenv_wrapper( ROS_SECURITY_DIRECTORY_OVERRIDE "=" TEST_RESOURCES_DIRECTORY); ret = rcl_get_security_options_from_environment( - "doesn't matter at all", "doesn't matter at all", &allocator, &options); + "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); @@ -268,12 +209,14 @@ TEST_F(TestGetSecureRoot, test_get_security_options) { options = rmw_get_zero_initialized_security_options(); unsetenv_wrapper(ROS_SECURITY_DIRECTORY_OVERRIDE); - putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); + putenv_wrapper( + ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" + TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); ret = rcl_get_security_options_from_environment( - TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator, &options); + TEST_CONTEXT_NAME_ABSOLUTE, &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 TEST_NODE_NAMESPACE PATH_SEPARATOR TEST_NODE_NAME, - options.security_root_path); + TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME + PATH_SEPARATOR TEST_CONTEXT_NAME, options.security_root_path); } diff --git a/rcl/test/resources/test_security_directory/dummy_node/.gitkeep b/rcl/test/resources/test_security_directory/dummy_context/.gitkeep similarity index 100% rename from rcl/test/resources/test_security_directory/dummy_node/.gitkeep rename to rcl/test/resources/test_security_directory/dummy_context/.gitkeep From 8680864def5be3cd1d5084a7742acf874a2c4608 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 13 Mar 2020 15:12:24 -0300 Subject: [PATCH 18/30] Delete unused headers Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/init.c | 1 - rcl/src/rcl/node.c | 1 - rcl/src/rcl/security.c | 1 - rcl/test/rcl/test_security.cpp | 1 - 4 files changed, 4 deletions(-) diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index c27447c1c..64705de36 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -24,7 +24,6 @@ extern "C" #include "rcutils/strdup.h" #include "rmw/error_handling.h" -#include "rmw/security.h" #include "tracetools/tracetools.h" diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 68a92f87f..5fa895c38 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -47,7 +47,6 @@ extern "C" #include "rmw/error_handling.h" #include "rmw/security_options.h" #include "rmw/rmw.h" -#include "rmw/security.h" #include "rmw/validate_namespace.h" #include "rmw/validate_node_name.h" #include "tracetools/tracetools.h" diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index 6e73b7c86..4fb578204 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -23,7 +23,6 @@ #include "rcutils/get_env.h" #include "rcutils/strdup.h" -#include "rmw/security.h" #include "rmw/security_options.h" rcl_ret_t diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp index 2878c5445..58b960783 100644 --- a/rcl/test/rcl/test_security.cpp +++ b/rcl/test/rcl/test_security.cpp @@ -23,7 +23,6 @@ #include "rcutils/filesystem.h" #include "rmw/error_handling.h" -#include "rmw/security.h" #include "osrf_testing_tools_cpp/scope_exit.hpp" From 0fe8a0b1c0916a05904e2679025a08beebba8dc3 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 13 Mar 2020 17:14:20 -0300 Subject: [PATCH 19/30] Correct rebasing error Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/arguments.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index f92852b03..47477f152 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -2172,7 +2172,7 @@ _rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t args_impl->log_stdout_disabled = false; args_impl->log_rosout_disabled = false; args_impl->log_ext_lib_disabled = false; - args_out->impl->security_directory = NULL; + args_impl->security_directory = NULL; args_impl->allocator = *allocator; return RCL_RET_OK; From c4807baa443386ce3563b421768e974240fb793a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 16 Mar 2020 17:34:24 -0300 Subject: [PATCH 20/30] Correct error in comment Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/security.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index b3eb90e92..f8b0712cf 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -98,7 +98,7 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); * * The value of the environment variable `ROS_SECURITY_ROOT_DIRECTORY` 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 + * 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 From 7f1b51ead04ab9bdc4f5e0c8f5da4ac6274ad5df Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 17 Mar 2020 10:53:35 -0300 Subject: [PATCH 21/30] Naming: replace context_name with security_context Signed-off-by: Ivan Santiago Paunovic --- rcl/CMakeLists.txt | 2 +- rcl/include/rcl/arguments.h | 2 +- rcl/include/rcl/security.h | 6 ++-- ...ame.h => validate_security_context_name.h} | 30 ++++++++-------- rcl/src/rcl/init.c | 20 +++++------ ...ame.c => validate_security_context_name.c} | 34 ++++++++++--------- rcl/test/rcl/test_security.cpp | 32 ++++++++--------- .../.gitkeep | 0 8 files changed, 64 insertions(+), 62 deletions(-) rename rcl/include/rcl/{validate_context_name.h => validate_security_context_name.h} (77%) rename rcl/src/rcl/{validate_context_name.c => validate_security_context_name.c} (81%) rename rcl/test/resources/test_security_directory/{dummy_context => dummy_security_context}/.gitkeep (100%) diff --git a/rcl/CMakeLists.txt b/rcl/CMakeLists.txt index bbaf391f3..0f072469e 100644 --- a/rcl/CMakeLists.txt +++ b/rcl/CMakeLists.txt @@ -59,7 +59,7 @@ set(${PROJECT_NAME}_sources src/rcl/subscription.c src/rcl/time.c src/rcl/timer.c - src/rcl/validate_context_name.c + src/rcl/validate_security_context_name.c src/rcl/validate_topic_name.c src/rcl/wait.c ) diff --git a/rcl/include/rcl/arguments.h b/rcl/include/rcl/arguments.h index e3b068f0e..185db5d9d 100644 --- a/rcl/include/rcl/arguments.h +++ b/rcl/include/rcl/arguments.h @@ -42,7 +42,7 @@ typedef struct rcl_arguments_t #define RCL_PARAM_FILE_FLAG "--params-file" #define RCL_REMAP_FLAG "--remap" #define RCL_SHORT_REMAP_FLAG "-r" -#define RCL_SECURITY_DIRECTORY_FLAG "--security-directory" +#define RCL_SECURITY_DIRECTORY_FLAG "--security-context" #define RCL_LOG_LEVEL_FLAG "--log-level" #define RCL_EXTERNAL_LOG_CONFIG_FLAG "--log-config-file" // To be prefixed with --enable- or --disable- diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index f8b0712cf..f729c1e25 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -92,9 +92,9 @@ RCL_PUBLIC rcl_ret_t rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); -/// Return the secure root given a context name. +/// Return the secure root given a security context name. /** - * Return the security directory associated with the context name. + * Return the security directory associated with the security context name. * * The value of the environment variable `ROS_SECURITY_ROOT_DIRECTORY` is used as a root. * The specific directory to be used, is found from that root using the `name` passed. @@ -104,7 +104,7 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); * 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. - * Such an override is useful for where the context name is non-deterministic before runtime, + * Such an override is useful for where the security context 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) diff --git a/rcl/include/rcl/validate_context_name.h b/rcl/include/rcl/validate_security_context_name.h similarity index 77% rename from rcl/include/rcl/validate_context_name.h rename to rcl/include/rcl/validate_security_context_name.h index ab7957fd3..e80f2da92 100644 --- a/rcl/include/rcl/validate_context_name.h +++ b/rcl/include/rcl/validate_security_context_name.h @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef RCL__VALIDATE_CONTEXT_NAME_H_ -#define RCL__VALIDATE_CONTEXT_NAME_H_ +#ifndef RCL__VALIDATE_SECURITY_CONTEXT_NAME_H_ +#define RCL__VALIDATE_SECURITY_CONTEXT_NAME_H_ #ifdef __cplusplus extern "C" @@ -42,12 +42,12 @@ extern "C" #define RCL_CONTEXT_NAME_MAX_LENGTH RMW_NODE_NAME_MAX_NAME_LENGTH -/// Determine if a given context name is valid. +/// Determine if a given security context name is valid. /** * /sa The same rules as rmw_validate_namespace are used. * The only difference is the maximum length, which can be 255 characters. * - * \param[in] context_name context_name to be validated + * \param[in] security_context security_context to be validated * \param[out] validation_result int in which the result of the check is stored * \param[out] invalid_index index of the input string where an error occurred * \returns `RMW_RET_OK` on successfully running the check, or @@ -57,24 +57,24 @@ extern "C" RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t -rcl_validate_context_name( - const char * context_name, +rcl_validate_security_context_name( + const char * security_context, int * validation_result, size_t * invalid_index); -/// Deterimine if a given context name is valid. +/// Deterimine if a given security context name is valid. /** - * This is an overload with an extra parameter for the length of context_name. - * \param[in] context_name The number of characters in context_name. + * This is an overload with an extra parameter for the length of security_context. + * \param[in] security_context The number of characters in security_context. * - * \sa rcl_validate_context_name(const char *, int *, size_t *) + * \sa rcl_validate_security_context(const char *, int *, size_t *) */ RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t -rcl_validate_context_name_with_size( - const char * context_name, - size_t context_name_length, +rcl_validate_security_context_name_with_size( + const char * security_context, + size_t security_context_length, int * validation_result, size_t * invalid_index); @@ -82,10 +82,10 @@ rcl_validate_context_name_with_size( RCL_PUBLIC RCL_WARN_UNUSED const char * -rcl_context_name_validation_result_string(int validation_result); +rcl_security_context_name_validation_result_string(int validation_result); #ifdef __cplusplus } #endif -#endif // RCL__VALIDATE_CONTEXT_NAME_H_ +#endif // RCL__VALIDATE_SECURITY_CONTEXT_NAME_H_ diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 64705de36..d8aa4d728 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -33,7 +33,7 @@ extern "C" #include "rcl/localhost.h" #include "rcl/logging.h" #include "rcl/security.h" -#include "rcl/validate_context_name.h" +#include "rcl/validate_security_context_name.h" #include "./arguments_impl.h" #include "./common.h" @@ -175,34 +175,34 @@ rcl_init( } if (context->global_arguments.impl->security_directory) { - context->impl->init_options.impl->rmw_init_options.name = rcutils_strdup( + context->impl->init_options.impl->rmw_init_options.security_context = rcutils_strdup( context->global_arguments.impl->security_directory, context->impl->allocator); } else { - context->impl->init_options.impl->rmw_init_options.name = rcutils_strdup( + context->impl->init_options.impl->rmw_init_options.security_context = rcutils_strdup( "/", context->impl->allocator); } int validation_result; size_t invalid_index; - ret = rcl_validate_context_name( - context->impl->init_options.impl->rmw_init_options.name, + ret = rcl_validate_security_context_name( + context->impl->init_options.impl->rmw_init_options.security_context, &validation_result, &invalid_index); if (RCL_RET_OK != ret) { - RCL_SET_ERROR_MSG("rcl_validate_context_name() failed"); + RCL_SET_ERROR_MSG("rcl_validate_security_context_name() failed"); fail_ret = ret; goto fail; } if (RCL_CONTEXT_NAME_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "rcl_validate_context_name result is not valid: '%s'. Invalid index: %zu", - rcl_context_name_validation_result_string(validation_result), + "rcl_validate_security_context_name result is not valid: '%s'. Invalid index: %zu", + rcl_security_context_name_validation_result_string(validation_result), invalid_index); fail_ret = RMW_RET_ERROR; goto fail; } - if (!context->impl->init_options.impl->rmw_init_options.name) { + if (!context->impl->init_options.impl->rmw_init_options.security_context) { RCL_SET_ERROR_MSG("failed to set context name"); fail_ret = RMW_RET_BAD_ALLOC; goto fail; @@ -211,7 +211,7 @@ rcl_init( rmw_security_options_t * security_options = &context->impl->init_options.impl->rmw_init_options.security_options; ret = rcl_get_security_options_from_environment( - context->impl->init_options.impl->rmw_init_options.name, + context->impl->init_options.impl->rmw_init_options.security_context, &context->impl->allocator, security_options); if (RMW_RET_OK != ret) { diff --git a/rcl/src/rcl/validate_context_name.c b/rcl/src/rcl/validate_security_context_name.c similarity index 81% rename from rcl/src/rcl/validate_context_name.c rename to rcl/src/rcl/validate_security_context_name.c index 06b111f1d..242bd70d5 100644 --- a/rcl/src/rcl/validate_context_name.c +++ b/rcl/src/rcl/validate_security_context_name.c @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "rcl/validate_context_name.h" +#include "rcl/validate_security_context_name.h" #include #include @@ -27,26 +27,26 @@ #include "./common.h" rcl_ret_t -rcl_validate_context_name( - const char * context_name, +rcl_validate_security_context_name( + const char * security_context, int * validation_result, size_t * invalid_index) { - if (!context_name) { + if (!security_context) { return RCL_RET_INVALID_ARGUMENT; } return rmw_validate_namespace_with_size( - context_name, strlen(context_name), validation_result, invalid_index); + security_context, strlen(security_context), validation_result, invalid_index); } rcl_ret_t -rcl_validate_context_name_with_size( - const char * context_name, - size_t context_name_length, +rcl_validate_security_context_name_with_size( + const char * security_context, + size_t security_context_length, int * validation_result, size_t * invalid_index) { - if (!context_name) { + if (!security_context) { return RCL_RET_INVALID_ARGUMENT; } if (!validation_result) { @@ -56,7 +56,7 @@ rcl_validate_context_name_with_size( int t_validation_result; size_t t_invalid_index; rmw_ret_t ret = rmw_validate_namespace_with_size( - context_name, context_name_length, &t_validation_result, &t_invalid_index); + security_context, security_context_length, &t_validation_result, &t_invalid_index); if (ret != RMW_RET_OK) { return rcl_convert_rmw_ret_to_rcl_ret(ret); } @@ -89,11 +89,13 @@ rcl_validate_context_name_with_size( // explicitly not taking return value which is number of bytes written int ret = rcutils_snprintf( default_err_msg, sizeof(default_err_msg), - "rcl_validate_context_name_with_size(): unknown rmw_validate_namespace_with_size()" - " result '%d'", + "rcl_validate_security_context_name_with_size(): " + "unknown rmw_validate_namespace_with_size() result '%d'", t_validation_result); if (ret < 0) { - RCL_SET_ERROR_MSG("rcl_validate_context_name_with_size(): rcutils_snprintf() failed"); + RCL_SET_ERROR_MSG( + "rcl_validate_security_context_name_with_size(): " + "rcutils_snprintf() failed"); } else { RCL_SET_ERROR_MSG(default_err_msg); } @@ -106,9 +108,9 @@ rcl_validate_context_name_with_size( return RCL_RET_OK; } - // context_name might be longer that namespace length, check false positives and correct + // security_context might be longer that namespace length, check false positives and correct if (t_validation_result == RMW_NAMESPACE_INVALID_TOO_LONG && - context_name_length <= RCL_CONTEXT_NAME_MAX_LENGTH) + security_context_length <= RCL_CONTEXT_NAME_MAX_LENGTH) { *validation_result = RCL_CONTEXT_NAME_VALID; return RCL_RET_OK; @@ -120,7 +122,7 @@ rcl_validate_context_name_with_size( } const char * -rcl_context_name_validation_result_string(int validation_result) +rcl_security_context_name_validation_result_string(int validation_result) { switch (validation_result) { case RCL_CONTEXT_NAME_VALID: diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp index 58b960783..179b27308 100644 --- a/rcl/test/rcl/test_security.cpp +++ b/rcl/test/rcl/test_security.cpp @@ -28,8 +28,8 @@ #define TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME "/test_security_directory" -#define TEST_CONTEXT_NAME "dummy_context" -#define TEST_CONTEXT_NAME_ABSOLUTE "/" TEST_CONTEXT_NAME +#define TEST_SECURITY_CONTEXT "dummy_security_context" +#define TEST_SECURITY_CONTEXT_ABSOLUTE "/" TEST_SECURITY_CONTEXT #ifndef _WIN32 # define PATH_SEPARATOR "/" @@ -116,16 +116,16 @@ class TestGetSecureRoot : public ::testing::Test TEST_F(TestGetSecureRoot, failureScenarios) { ASSERT_EQ( - rcl_get_secure_root(TEST_CONTEXT_NAME_ABSOLUTE, &allocator), + rcl_get_secure_root(TEST_SECURITY_CONTEXT_ABSOLUTE, &allocator), (char *) NULL); rcl_reset_error(); putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); /* Security directory is set, but there's no matching directory */ - /// Wrong context name + /// Wrong security context ASSERT_EQ( - rcl_get_secure_root("some_other_context_name", &allocator), + rcl_get_secure_root("some_other_security_context", &allocator), (char *) NULL); rcl_reset_error(); } @@ -135,22 +135,22 @@ TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) { ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); - secure_root = rcl_get_secure_root(TEST_CONTEXT_NAME_ABSOLUTE, &allocator); + secure_root = rcl_get_secure_root(TEST_SECURITY_CONTEXT_ABSOLUTE, &allocator); std::string secure_root_str(secure_root); ASSERT_STREQ( - TEST_CONTEXT_NAME, - secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_CONTEXT_NAME) - 1)).c_str()); + TEST_SECURITY_CONTEXT, + secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_SECURITY_CONTEXT) - 1)).c_str()); } TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch_multipleTokensName) { putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); secure_root = rcl_get_secure_root( - TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME PATH_SEPARATOR TEST_CONTEXT_NAME, &allocator); + TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME PATH_SEPARATOR TEST_SECURITY_CONTEXT, &allocator); std::string secure_root_str(secure_root); ASSERT_STREQ( - TEST_CONTEXT_NAME, - secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_CONTEXT_NAME) - 1)).c_str()); + TEST_SECURITY_CONTEXT, + secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_SECURITY_CONTEXT) - 1)).c_str()); } TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { @@ -179,13 +179,13 @@ TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) { ROS_SECURITY_DIRECTORY_OVERRIDE "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); ASSERT_EQ( - rcl_get_secure_root(TEST_CONTEXT_NAME_ABSOLUTE, &allocator), + rcl_get_secure_root(TEST_SECURITY_CONTEXT_ABSOLUTE, &allocator), (char *) NULL); } TEST_F(TestGetSecureRoot, test_get_security_options) { - /* The override provided should exist. Providing correct context name/root dir won't help - * if the node override is invalid. */ + /* The override provided should exist. Providing correct security context name/root dir + * won't help if the node override is invalid. */ rmw_security_options_t options = rmw_get_zero_initialized_security_options(); putenv_wrapper(ROS_SECURITY_ENABLE_VAR_NAME "=false"); rcl_ret_t ret = rcl_get_security_options_from_environment( @@ -212,10 +212,10 @@ TEST_F(TestGetSecureRoot, test_get_security_options) { ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); ret = rcl_get_security_options_from_environment( - TEST_CONTEXT_NAME_ABSOLUTE, &allocator, &options); + TEST_SECURITY_CONTEXT_ABSOLUTE, &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 TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME - PATH_SEPARATOR TEST_CONTEXT_NAME, options.security_root_path); + PATH_SEPARATOR TEST_SECURITY_CONTEXT, options.security_root_path); } diff --git a/rcl/test/resources/test_security_directory/dummy_context/.gitkeep b/rcl/test/resources/test_security_directory/dummy_security_context/.gitkeep similarity index 100% rename from rcl/test/resources/test_security_directory/dummy_context/.gitkeep rename to rcl/test/resources/test_security_directory/dummy_security_context/.gitkeep From eb2e187dc157b9b3f1281541b9edd2763607ded0 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 20 Mar 2020 16:56:23 -0300 Subject: [PATCH 22/30] Addrees peer review comments Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/init.c | 2 +- rcl/src/rcl/security.c | 7 ++----- rcl/src/rcl/validate_security_context_name.c | 4 ++-- rcl/test/rcl/test_security.cpp | 18 +++++++++--------- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index d8aa4d728..14cf3c678 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -195,7 +195,7 @@ rcl_init( } if (RCL_CONTEXT_NAME_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "rcl_validate_security_context_name result is not valid: '%s'. Invalid index: %zu", + "security context name is not valid: '%s'. Invalid index: %zu", rcl_security_context_name_validation_result_string(validation_result), invalid_index); fail_ret = RMW_RET_ERROR; diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index 4fb578204..09fd69f8c 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -51,9 +51,7 @@ rcl_get_security_options_from_environment( } // File discovery magic here - char * secure_root = rcl_get_secure_root( - name, - allocator); + char * secure_root = rcl_get_secure_root(name,allocator); if (secure_root) { RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root); security_options->security_root_path = secure_root; @@ -155,9 +153,8 @@ char * rcl_get_secure_root( } if (0 == strcmp("", env_buf)) { return NULL; // environment variable was empty - } else { - ros_secure_directory_override = false; } + ros_secure_directory_override = false; } // found a usable environment variable, copy into our memory before overwriting with next lookup diff --git a/rcl/src/rcl/validate_security_context_name.c b/rcl/src/rcl/validate_security_context_name.c index 242bd70d5..c552c8aaf 100644 --- a/rcl/src/rcl/validate_security_context_name.c +++ b/rcl/src/rcl/validate_security_context_name.c @@ -35,7 +35,7 @@ rcl_validate_security_context_name( if (!security_context) { return RCL_RET_INVALID_ARGUMENT; } - return rmw_validate_namespace_with_size( + return rcl_validate_security_context_name_with_size( security_context, strlen(security_context), validation_result, invalid_index); } @@ -95,7 +95,7 @@ rcl_validate_security_context_name_with_size( if (ret < 0) { RCL_SET_ERROR_MSG( "rcl_validate_security_context_name_with_size(): " - "rcutils_snprintf() failed"); + "rcutils_snprintf() failed while reporting an unknown validation result"); } else { RCL_SET_ERROR_MSG(default_err_msg); } diff --git a/rcl/test/rcl/test_security.cpp b/rcl/test/rcl/test_security.cpp index 179b27308..71c55b140 100644 --- a/rcl/test/rcl/test_security.cpp +++ b/rcl/test/rcl/test_security.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include "rcl/security.h" @@ -44,7 +45,7 @@ static int putenv_wrapper(const char * env_var) #ifdef _WIN32 return _putenv(env_var); #else - return putenv(reinterpret_cast(const_cast(env_var))); + return putenv(const_cast(env_var)); #endif } @@ -115,7 +116,7 @@ class TestGetSecureRoot : public ::testing::Test }; TEST_F(TestGetSecureRoot, failureScenarios) { - ASSERT_EQ( + EXPECT_EQ( rcl_get_secure_root(TEST_SECURITY_CONTEXT_ABSOLUTE, &allocator), (char *) NULL); rcl_reset_error(); @@ -124,7 +125,7 @@ TEST_F(TestGetSecureRoot, failureScenarios) { /* Security directory is set, but there's no matching directory */ /// Wrong security context - ASSERT_EQ( + EXPECT_EQ( rcl_get_secure_root("some_other_security_context", &allocator), (char *) NULL); rcl_reset_error(); @@ -139,7 +140,7 @@ TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) { std::string secure_root_str(secure_root); ASSERT_STREQ( TEST_SECURITY_CONTEXT, - secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_SECURITY_CONTEXT) - 1)).c_str()); + secure_root_str.substr(secure_root_str.size() - strlen(TEST_SECURITY_CONTEXT)).c_str()); } TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch_multipleTokensName) { @@ -150,7 +151,7 @@ TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch_multipleTokensName) std::string secure_root_str(secure_root); ASSERT_STREQ( TEST_SECURITY_CONTEXT, - secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_SECURITY_CONTEXT) - 1)).c_str()); + secure_root_str.substr(secure_root_str.size() - strlen(TEST_SECURITY_CONTEXT)).c_str()); } TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { @@ -166,8 +167,7 @@ TEST_F( 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); + 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); } @@ -177,8 +177,8 @@ TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) { * if the node override is invalid. */ putenv_wrapper( ROS_SECURITY_DIRECTORY_OVERRIDE - "=TheresN_oWayThi_sDirectory_Exists_hence_this_would_fail"); - ASSERT_EQ( + "=TheresN_oWayThi_sDirectory_Exists_hence_this_should_fail"); + EXPECT_EQ( rcl_get_secure_root(TEST_SECURITY_CONTEXT_ABSOLUTE, &allocator), (char *) NULL); } From daad16878f2755a2c0703bcc4fccff818f1e6a5e Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 20 Mar 2020 17:41:17 -0300 Subject: [PATCH 23/30] Please linters Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/security.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/security.c b/rcl/src/rcl/security.c index 09fd69f8c..515515d1c 100644 --- a/rcl/src/rcl/security.c +++ b/rcl/src/rcl/security.c @@ -51,7 +51,7 @@ rcl_get_security_options_from_environment( } // File discovery magic here - char * secure_root = rcl_get_secure_root(name,allocator); + char * secure_root = rcl_get_secure_root(name, allocator); if (secure_root) { RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Found security directory: %s", secure_root); security_options->security_root_path = secure_root; From 50035e34eaf78395e1e84e40b3007a02ecf20d20 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 20 Mar 2020 17:41:39 -0300 Subject: [PATCH 24/30] Replace RCL_CONTEXT_NAME_* with RCL_SECURITY_CONTEXT_NAME_* Signed-off-by: Ivan Santiago Paunovic --- .../rcl/validate_security_context_name.h | 20 +++++------ rcl/src/rcl/init.c | 2 +- rcl/src/rcl/validate_security_context_name.c | 36 +++++++++---------- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/rcl/include/rcl/validate_security_context_name.h b/rcl/include/rcl/validate_security_context_name.h index e80f2da92..ccc494ed5 100644 --- a/rcl/include/rcl/validate_security_context_name.h +++ b/rcl/include/rcl/validate_security_context_name.h @@ -27,20 +27,20 @@ extern "C" #include "rcl/types.h" #include "rcl/visibility_control.h" -#define RCL_CONTEXT_NAME_VALID RMW_NAMESPACE_VALID -#define RCL_CONTEXT_NAME_INVALID_IS_EMPTY_STRING RMW_NAMESPACE_INVALID_IS_EMPTY_STRING -#define RCL_CONTEXT_NAME_INVALID_NOT_ABSOLUTE RMW_NAMESPACE_INVALID_NOT_ABSOLUTE -#define RCL_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH \ +#define RCL_SECURITY_CONTEXT_NAME_VALID RMW_NAMESPACE_VALID +#define RCL_SECURITY_CONTEXT_NAME_INVALID_IS_EMPTY_STRING RMW_NAMESPACE_INVALID_IS_EMPTY_STRING +#define RCL_SECURITY_CONTEXT_NAME_INVALID_NOT_ABSOLUTE RMW_NAMESPACE_INVALID_NOT_ABSOLUTE +#define RCL_SECURITY_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH \ RMW_NAMESPACE_INVALID_ENDS_WITH_FORWARD_SLASH -#define RCL_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS \ +#define RCL_SECURITY_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS \ RMW_NAMESPACE_INVALID_CONTAINS_UNALLOWED_CHARACTERS -#define RCL_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH \ +#define RCL_SECURITY_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH \ RMW_NAMESPACE_INVALID_CONTAINS_REPEATED_FORWARD_SLASH -#define RCL_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER \ +#define RCL_SECURITY_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER \ RMW_NAMESPACE_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER -#define RCL_CONTEXT_NAME_INVALID_TOO_LONG RMW_NAMESPACE_INVALID_TOO_LONG +#define RCL_SECURITY_CONTEXT_NAME_INVALID_TOO_LONG RMW_NAMESPACE_INVALID_TOO_LONG -#define RCL_CONTEXT_NAME_MAX_LENGTH RMW_NODE_NAME_MAX_NAME_LENGTH +#define RCL_SECURITY_CONTEXT_NAME_MAX_LENGTH RMW_NODE_NAME_MAX_NAME_LENGTH /// Determine if a given security context name is valid. /** @@ -78,7 +78,7 @@ rcl_validate_security_context_name_with_size( int * validation_result, size_t * invalid_index); -/// Return a validation result description, or NULL if unknown or RCL_CONTEXT_NAME_VALID. +/// Return a validation result description, or NULL if unknown or RCL_SECURITY_CONTEXT_NAME_VALID. RCL_PUBLIC RCL_WARN_UNUSED const char * diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 14cf3c678..0a3f964f8 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -193,7 +193,7 @@ rcl_init( fail_ret = ret; goto fail; } - if (RCL_CONTEXT_NAME_VALID != validation_result) { + if (RCL_SECURITY_CONTEXT_NAME_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( "security context name is not valid: '%s'. Invalid index: %zu", rcl_security_context_name_validation_result_string(validation_result), diff --git a/rcl/src/rcl/validate_security_context_name.c b/rcl/src/rcl/validate_security_context_name.c index c552c8aaf..71344e54a 100644 --- a/rcl/src/rcl/validate_security_context_name.c +++ b/rcl/src/rcl/validate_security_context_name.c @@ -66,22 +66,22 @@ rcl_validate_security_context_name_with_size( { switch (t_validation_result) { case RMW_NAMESPACE_INVALID_IS_EMPTY_STRING: - *validation_result = RCL_CONTEXT_NAME_INVALID_IS_EMPTY_STRING; + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_IS_EMPTY_STRING; break; case RMW_NAMESPACE_INVALID_NOT_ABSOLUTE: - *validation_result = RCL_CONTEXT_NAME_INVALID_NOT_ABSOLUTE; + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_NOT_ABSOLUTE; break; case RMW_NAMESPACE_INVALID_ENDS_WITH_FORWARD_SLASH: - *validation_result = RCL_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH; + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH; break; case RMW_NAMESPACE_INVALID_CONTAINS_UNALLOWED_CHARACTERS: - *validation_result = RCL_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS; + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS; break; case RMW_NAMESPACE_INVALID_CONTAINS_REPEATED_FORWARD_SLASH: - *validation_result = RCL_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH; + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH; break; case RMW_NAMESPACE_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER: - *validation_result = RCL_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER; + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER; break; default: { @@ -110,14 +110,14 @@ rcl_validate_security_context_name_with_size( // security_context might be longer that namespace length, check false positives and correct if (t_validation_result == RMW_NAMESPACE_INVALID_TOO_LONG && - security_context_length <= RCL_CONTEXT_NAME_MAX_LENGTH) + security_context_length <= RCL_SECURITY_CONTEXT_NAME_MAX_LENGTH) { - *validation_result = RCL_CONTEXT_NAME_VALID; + *validation_result = RCL_SECURITY_CONTEXT_NAME_VALID; return RCL_RET_OK; } // everything was ok, set result to valid namespace, avoid setting invalid_index, and return - *validation_result = RCL_CONTEXT_NAME_VALID; + *validation_result = RCL_SECURITY_CONTEXT_NAME_VALID; return RCL_RET_OK; } @@ -125,23 +125,23 @@ const char * rcl_security_context_name_validation_result_string(int validation_result) { switch (validation_result) { - case RCL_CONTEXT_NAME_VALID: + case RCL_SECURITY_CONTEXT_NAME_VALID: return NULL; - case RCL_CONTEXT_NAME_INVALID_IS_EMPTY_STRING: + case RCL_SECURITY_CONTEXT_NAME_INVALID_IS_EMPTY_STRING: return "context name must not be empty"; - case RCL_CONTEXT_NAME_INVALID_NOT_ABSOLUTE: + case RCL_SECURITY_CONTEXT_NAME_INVALID_NOT_ABSOLUTE: return "context name must be absolute, it must lead with a '/'"; - case RCL_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH: + case RCL_SECURITY_CONTEXT_NAME_INVALID_ENDS_WITH_FORWARD_SLASH: return "context name must not end with a '/', unless only a '/'"; - case RCL_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS: + case RCL_SECURITY_CONTEXT_NAME_INVALID_CONTAINS_UNALLOWED_CHARACTERS: return "context name must not contain characters other than alphanumerics, '_', or '/'"; - case RCL_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH: + case RCL_SECURITY_CONTEXT_NAME_INVALID_CONTAINS_REPEATED_FORWARD_SLASH: return "context name must not contain repeated '/'"; - case RCL_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER: + case RCL_SECURITY_CONTEXT_NAME_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER: return "context name must not have a token that starts with a number"; - case RCL_CONTEXT_NAME_INVALID_TOO_LONG: + case RCL_SECURITY_CONTEXT_NAME_INVALID_TOO_LONG: return "context name should not exceed '" - RCUTILS_STRINGIFY(RCL_CONTEXT_NAME_MAX_NAME_LENGTH) "'"; + RCUTILS_STRINGIFY(RCL_SECURITY_CONTEXT_NAME_MAX_NAME_LENGTH) "'"; default: return "unknown result code for rcl context name validation"; } From b4d47387e865dfb8b1f0c68e92e9c26671b255af Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 23 Mar 2020 16:09:28 -0300 Subject: [PATCH 25/30] * Use security_context instead of context_name everywhere * Add rcl_get_nodes_names_with_security_contexts Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/arguments.h | 2 +- rcl/include/rcl/graph.h | 34 +++++++ rcl/src/rcl/arguments.c | 52 +++++------ rcl/src/rcl/arguments_impl.h | 4 +- rcl/src/rcl/graph.c | 47 ++++++++++ rcl/src/rcl/init.c | 4 +- rcl/test/rcl/test_get_node_names.cpp | 127 +++++++++++++++++++++++++++ 7 files changed, 239 insertions(+), 31 deletions(-) diff --git a/rcl/include/rcl/arguments.h b/rcl/include/rcl/arguments.h index 185db5d9d..a4e542205 100644 --- a/rcl/include/rcl/arguments.h +++ b/rcl/include/rcl/arguments.h @@ -42,7 +42,7 @@ typedef struct rcl_arguments_t #define RCL_PARAM_FILE_FLAG "--params-file" #define RCL_REMAP_FLAG "--remap" #define RCL_SHORT_REMAP_FLAG "-r" -#define RCL_SECURITY_DIRECTORY_FLAG "--security-context" +#define RCL_SECURITY_CONTEXT_FLAG "--security-context" #define RCL_LOG_LEVEL_FLAG "--log-level" #define RCL_EXTERNAL_LOG_CONFIG_FLAG "--log-config-file" // To be prefixed with --enable- or --disable- diff --git a/rcl/include/rcl/graph.h b/rcl/include/rcl/graph.h index 4e1f7aba4..8ae92104d 100644 --- a/rcl/include/rcl/graph.h +++ b/rcl/include/rcl/graph.h @@ -429,6 +429,7 @@ rcl_names_and_types_fini(rcl_names_and_types_t * names_and_types); * \param[out] node_names struct storing discovered node names * \param[out] node_namesspaces struct storing discovered node namespaces * \return `RCL_RET_OK` if the query was successful, or + * \return `RCL_RET_BAD_ALLOC` if an error occurred while allocating memory, or * \return `RCL_RET_ERROR` if an unspecified error occurs. */ RCL_PUBLIC @@ -440,6 +441,39 @@ rcl_get_node_names( rcutils_string_array_t * node_names, rcutils_string_array_t * node_namespaces); +/// Return a list of available nodes in the ROS graph, including their security context names. +/** + * \sa Works like rcl_get_node_names, but it also include the security context name the node is + * using as an output. + * + *
+ * Attribute | Adherence + * ------------------ | ------------- + * Allocates Memory | Yes + * Thread-Safe | No + * Uses Atomics | No + * Lock-Free | Maybe [1] + * [1] implementation may need to protect the data structure with a lock + * + * \param[in] node the handle to the node being used to query the ROS graph + * \param[in] allocator used to control allocation and deallocation of names + * \param[out] node_names struct storing discovered node names + * \param[out] node_namesspaces struct storing discovered node namespaces + * \param[out] security_contexts struct storing discovered node security contexts + * \return `RCL_RET_OK` if the query was successful, or + * \return `RCL_RET_BAD_ALLOC` if an error occurred while allocating memory, or + * \return `RCL_RET_ERROR` if an unspecified error occurs. + */ +RCL_PUBLIC +RCL_WARN_UNUSED +rcl_ret_t +rcl_get_node_names_with_security_contexts( + const rcl_node_t * node, + rcl_allocator_t allocator, + rcutils_string_array_t * node_names, + rcutils_string_array_t * node_namespaces, + rcutils_string_array_t * security_contexts); + /// Return the number of publishers on a given topic. /** * The `node` parameter must point to a valid node. diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 47477f152..6112fd351 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -245,21 +245,21 @@ _rcl_parse_param_file_rule( rcl_params_t * params, char ** param_file); -/// Parse a security directory argument. +/// Parse a security context argument. /** * \param[in] arg the argument to parse * \param[in] allocator an allocator to use - * \param[in,out] security_directory parsed security directory - * \return RCL_RET_OK if a valid security directory was parsed, or + * \param[in,out] security_context parsed security context + * \return RCL_RET_OK if a valid security context was parsed, or * \return RCL_RET_BAD_ALLOC if an allocation failed, or * \return RLC_RET_ERROR if an unspecified error occurred. */ RCL_LOCAL rcl_ret_t -_rcl_parse_security_directory( +_rcl_parse_security_context( const char * arg, rcl_allocator_t allocator, - char ** security_directory); + char ** security_context); #define RCL_ENABLE_FLAG_PREFIX "--enable-" #define RCL_DISABLE_FLAG_PREFIX "--disable-" @@ -550,33 +550,33 @@ rcl_parse_arguments( goto fail; } - // Attempt to parse argument as a security directory - if (strcmp(RCL_SECURITY_DIRECTORY_FLAG, argv[i]) == 0) { + // Attempt to parse argument as a security context + if (strcmp(RCL_SECURITY_CONTEXT_FLAG, argv[i]) == 0) { if (i + 1 < argc) { - if (NULL != args_impl->security_directory) { + if (NULL != args_impl->security_context) { RCUTILS_LOG_DEBUG_NAMED( - ROS_PACKAGE_NAME, "Overriding security directory path : %s\n", - args_impl->security_directory); - allocator.deallocate(args_impl->security_directory, allocator.state); - args_impl->security_directory = NULL; + ROS_PACKAGE_NAME, "Overriding security context name : %s\n", + args_impl->security_context); + allocator.deallocate(args_impl->security_context, allocator.state); + args_impl->security_context = NULL; } - if (RCL_RET_OK == _rcl_parse_security_directory( - argv[i + 1], allocator, &args_impl->security_directory)) + if (RCL_RET_OK == _rcl_parse_security_context( + argv[i + 1], allocator, &args_impl->security_context)) { RCUTILS_LOG_DEBUG_NAMED( - ROS_PACKAGE_NAME, "Got security directory : %s\n", - args_impl->security_directory); + ROS_PACKAGE_NAME, "Got security context : %s\n", + args_impl->security_context); ++i; // Skip flag here, for loop will skip value. continue; } rcl_error_string_t prev_error_string = rcl_get_error_string(); rcl_reset_error(); RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Couldn't parse security directory path: '%s %s'. Error: %s", argv[i], argv[i + 1], + "Couldn't parse security context name: '%s %s'. Error: %s", argv[i], argv[i + 1], prev_error_string.str); } else { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "Couldn't parse trailing %s flag. No security directory path provided.", argv[i]); + "Couldn't parse trailing %s flag. No security context path provided.", argv[i]); } ret = RCL_RET_INVALID_ROS_ARGS; goto fail; @@ -1147,7 +1147,7 @@ rcl_arguments_fini( args->impl->num_param_files_args = 0; args->impl->parameter_files = NULL; } - args->impl->allocator.deallocate(args->impl->security_directory, args->impl->allocator.state); + args->impl->allocator.deallocate(args->impl->security_context, args->impl->allocator.state); if (NULL != args->impl->external_log_config_file) { args->impl->allocator.deallocate( @@ -2054,17 +2054,17 @@ _rcl_parse_external_log_config_file_rule( } rcl_ret_t -_rcl_parse_security_directory( +_rcl_parse_security_context( const char * arg, rcl_allocator_t allocator, - char ** security_directory) + char ** security_context) { RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(security_directory, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(security_context, RCL_RET_INVALID_ARGUMENT); - *security_directory = rcutils_strdup(arg, allocator); - if (NULL == *security_directory) { - RCL_SET_ERROR_MSG("Failed to allocate memory for security directory path"); + *security_context = rcutils_strdup(arg, allocator); + if (NULL == *security_context) { + RCL_SET_ERROR_MSG("Failed to allocate memory for security context name"); return RCL_RET_BAD_ALLOC; } return RCL_RET_OK; @@ -2172,7 +2172,7 @@ _rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t args_impl->log_stdout_disabled = false; args_impl->log_rosout_disabled = false; args_impl->log_ext_lib_disabled = false; - args_impl->security_directory = NULL; + args_impl->security_context = NULL; args_impl->allocator = *allocator; return RCL_RET_OK; diff --git a/rcl/src/rcl/arguments_impl.h b/rcl/src/rcl/arguments_impl.h index da708b0c3..b4f3eff04 100644 --- a/rcl/src/rcl/arguments_impl.h +++ b/rcl/src/rcl/arguments_impl.h @@ -61,8 +61,8 @@ typedef struct rcl_arguments_impl_t /// A boolean value indicating if the external lib handler should be used for log output bool log_ext_lib_disabled; - /// Security directory to be used. - char * security_directory; + /// Security context to be used. + char * security_context; /// Allocator used to allocate objects in this struct rcl_allocator_t allocator; diff --git a/rcl/src/rcl/graph.c b/rcl/src/rcl/graph.c index 43ab9dcc2..767e00083 100644 --- a/rcl/src/rcl/graph.c +++ b/rcl/src/rcl/graph.c @@ -339,6 +339,53 @@ rcl_get_node_names( return rcl_convert_rmw_ret_to_rcl_ret(rmw_ret); } +rcl_ret_t +rcl_get_node_names_with_security_contexts( + const rcl_node_t * node, + rcl_allocator_t allocator, + rcutils_string_array_t * node_names, + rcutils_string_array_t * node_namespaces, + rcutils_string_array_t * security_contexts) +{ + if (!rcl_node_is_valid(node)) { + return RCL_RET_NODE_INVALID; // error already set + } + RCL_CHECK_ARGUMENT_FOR_NULL(node_names, RCL_RET_INVALID_ARGUMENT); + if (node_names->size != 0) { + RCL_SET_ERROR_MSG("node_names size is not zero"); + return RCL_RET_INVALID_ARGUMENT; + } + if (node_names->data) { + RCL_SET_ERROR_MSG("node_names is not null"); + return RCL_RET_INVALID_ARGUMENT; + } + RCL_CHECK_ARGUMENT_FOR_NULL(node_namespaces, RCL_RET_INVALID_ARGUMENT); + if (node_namespaces->size != 0) { + RCL_SET_ERROR_MSG("node_namespaces size is not zero"); + return RCL_RET_INVALID_ARGUMENT; + } + if (node_namespaces->data) { + RCL_SET_ERROR_MSG("node_namespaces is not null"); + return RCL_RET_INVALID_ARGUMENT; + } + RCL_CHECK_ARGUMENT_FOR_NULL(security_contexts, RCL_RET_INVALID_ARGUMENT); + if (security_contexts->size != 0) { + RCL_SET_ERROR_MSG("security_contexts size is not zero"); + return RCL_RET_INVALID_ARGUMENT; + } + if (security_contexts->data) { + RCL_SET_ERROR_MSG("security_contexts is not null"); + return RCL_RET_INVALID_ARGUMENT; + } + (void)allocator; // to be used in rmw_get_node_names in the future + rmw_ret_t rmw_ret = rmw_get_node_names_with_security_contexts( + rcl_node_get_rmw_handle(node), + node_names, + node_namespaces, + security_contexts); + return rcl_convert_rmw_ret_to_rcl_ret(rmw_ret); +} + rcl_ret_t rcl_count_publishers( const rcl_node_t * node, diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 0a3f964f8..af9930711 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -174,9 +174,9 @@ rcl_init( } } - if (context->global_arguments.impl->security_directory) { + if (context->global_arguments.impl->security_context) { context->impl->init_options.impl->rmw_init_options.security_context = rcutils_strdup( - context->global_arguments.impl->security_directory, + context->global_arguments.impl->security_context, context->impl->allocator); } else { context->impl->init_options.impl->rmw_init_options.security_context = rcutils_strdup( diff --git a/rcl/test/rcl/test_get_node_names.cpp b/rcl/test/rcl/test_get_node_names.cpp index 0cee1d99c..227e60b63 100644 --- a/rcl/test/rcl/test_get_node_names.cpp +++ b/rcl/test/rcl/test_get_node_names.cpp @@ -162,3 +162,130 @@ TEST_F(CLASSNAME(TestGetNodeNames, RMW_IMPLEMENTATION), test_rcl_get_node_names) delete node5_ptr; EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } + +TEST_F( + CLASSNAME(TestGetNodeNames, RMW_IMPLEMENTATION), test_rcl_get_node_names_with_security_contexts) +{ + rcl_ret_t ret; + rcl_init_options_t init_options = rcl_get_zero_initialized_init_options(); + ret = rcl_init_options_init(&init_options, rcl_get_default_allocator()); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + EXPECT_EQ(RCL_RET_OK, rcl_init_options_fini(&init_options)) << rcl_get_error_string().str; + }); + rcl_context_t context = rcl_get_zero_initialized_context(); + const char * security_context_name = "/default"; + const char * argv[] = {"--ros-args", "--security-context", security_context_name}; + ret = rcl_init(3, argv, &init_options, &context); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( + { + EXPECT_EQ(RCL_RET_OK, rcl_shutdown(&context)) << rcl_get_error_string().str; + EXPECT_EQ(RCL_RET_OK, rcl_context_fini(&context)) << rcl_get_error_string().str; + }); + std::multiset> + expected_nodes, discovered_nodes; + + rcl_node_t node1 = rcl_get_zero_initialized_node(); + const char * node1_name = "node1"; + const char * node1_namespace = "/"; + rcl_node_options_t node1_options = rcl_node_get_default_options(); + ret = rcl_node_init(&node1, node1_name, node1_namespace, &context, &node1_options); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + expected_nodes.insert( + std::make_tuple( + std::string(node1_name), + std::string(node1_namespace), + std::string(security_context_name))); + + rcl_node_t node2 = rcl_get_zero_initialized_node(); + const char * node2_name = "node2"; + const char * node2_namespace = "/"; + rcl_node_options_t node2_options = rcl_node_get_default_options(); + ret = rcl_node_init(&node2, node2_name, node2_namespace, &context, &node2_options); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + expected_nodes.insert(std::make_tuple( + std::string(node2_name), + std::string(node2_namespace), + std::string(security_context_name))); + + rcl_node_t node3 = rcl_get_zero_initialized_node(); + const char * node3_name = "node3"; + const char * node3_namespace = "/ns"; + rcl_node_options_t node3_options = rcl_node_get_default_options(); + ret = rcl_node_init(&node3, node3_name, node3_namespace, &context, &node3_options); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + expected_nodes.insert(std::make_tuple( + std::string(node3_name), + std::string(node3_namespace), + std::string(security_context_name))); + + rcl_node_t node4 = rcl_get_zero_initialized_node(); + const char * node4_name = "node2"; + const char * node4_namespace = "/ns/ns"; + rcl_node_options_t node4_options = rcl_node_get_default_options(); + ret = rcl_node_init(&node4, node4_name, node4_namespace, &context, &node4_options); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + expected_nodes.insert(std::make_tuple( + std::string(node4_name), + std::string(node4_namespace), + std::string(security_context_name))); + + rcl_node_t node5 = rcl_get_zero_initialized_node(); + const char * node5_name = "node1"; + const char * node5_namespace = "/"; + rcl_node_options_t node5_options = rcl_node_get_default_options(); + ret = rcl_node_init(&node5, node5_name, node5_namespace, &context, &node5_options); + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + expected_nodes.insert(std::make_tuple( + std::string(node5_name), + std::string(node5_namespace), + std::string(security_context_name))); + + std::this_thread::sleep_for(1s); + + rcutils_string_array_t node_names = rcutils_get_zero_initialized_string_array(); + rcutils_string_array_t node_namespaces = rcutils_get_zero_initialized_string_array(); + rcutils_string_array_t security_contexts = rcutils_get_zero_initialized_string_array(); + ret = rcl_get_node_names_with_security_contexts( + &node1, node1_options.allocator, &node_names, &node_namespaces, &security_contexts); + ASSERT_EQ(RCUTILS_RET_OK, ret) << rcl_get_error_string().str; + + std::stringstream ss; + ss << "[test_rcl_get_node_names]: Found node names:" << std::endl; + for (size_t i = 0; i < node_names.size; ++i) { + ss << node_names.data[i] << std::endl; + } + EXPECT_EQ(node_names.size, node_namespaces.size) << ss.str(); + + for (size_t i = 0; i < node_names.size; ++i) { + discovered_nodes.insert( + std::make_tuple( + std::string(node_names.data[i]), + std::string(node_namespaces.data[i]), + std::string(security_contexts.data[i]))); + } + EXPECT_EQ(discovered_nodes, expected_nodes); + + ret = rcutils_string_array_fini(&node_names); + ASSERT_EQ(RCUTILS_RET_OK, ret); + + ret = rcutils_string_array_fini(&node_namespaces); + ASSERT_EQ(RCUTILS_RET_OK, ret); + + ret = rcl_node_fini(&node1); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_node_fini(&node2); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_node_fini(&node3); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_node_fini(&node4); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_node_fini(&node5); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; +} From 4c239f64472fe932a5163405718f92b6d4235ad7 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 23 Mar 2020 16:29:27 -0300 Subject: [PATCH 26/30] Please linters Signed-off-by: Ivan Santiago Paunovic --- rcl/test/rcl/test_get_node_names.cpp | 37 ++++++++++++++++------------ 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/rcl/test/rcl/test_get_node_names.cpp b/rcl/test/rcl/test_get_node_names.cpp index 227e60b63..f57af5d18 100644 --- a/rcl/test/rcl/test_get_node_names.cpp +++ b/rcl/test/rcl/test_get_node_names.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -205,10 +206,11 @@ TEST_F( rcl_node_options_t node2_options = rcl_node_get_default_options(); ret = rcl_node_init(&node2, node2_name, node2_namespace, &context, &node2_options); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - expected_nodes.insert(std::make_tuple( - std::string(node2_name), - std::string(node2_namespace), - std::string(security_context_name))); + expected_nodes.insert( + std::make_tuple( + std::string(node2_name), + std::string(node2_namespace), + std::string(security_context_name))); rcl_node_t node3 = rcl_get_zero_initialized_node(); const char * node3_name = "node3"; @@ -216,10 +218,11 @@ TEST_F( rcl_node_options_t node3_options = rcl_node_get_default_options(); ret = rcl_node_init(&node3, node3_name, node3_namespace, &context, &node3_options); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - expected_nodes.insert(std::make_tuple( - std::string(node3_name), - std::string(node3_namespace), - std::string(security_context_name))); + expected_nodes.insert( + std::make_tuple( + std::string(node3_name), + std::string(node3_namespace), + std::string(security_context_name))); rcl_node_t node4 = rcl_get_zero_initialized_node(); const char * node4_name = "node2"; @@ -227,10 +230,11 @@ TEST_F( rcl_node_options_t node4_options = rcl_node_get_default_options(); ret = rcl_node_init(&node4, node4_name, node4_namespace, &context, &node4_options); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - expected_nodes.insert(std::make_tuple( - std::string(node4_name), - std::string(node4_namespace), - std::string(security_context_name))); + expected_nodes.insert( + std::make_tuple( + std::string(node4_name), + std::string(node4_namespace), + std::string(security_context_name))); rcl_node_t node5 = rcl_get_zero_initialized_node(); const char * node5_name = "node1"; @@ -238,10 +242,11 @@ TEST_F( rcl_node_options_t node5_options = rcl_node_get_default_options(); ret = rcl_node_init(&node5, node5_name, node5_namespace, &context, &node5_options); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - expected_nodes.insert(std::make_tuple( - std::string(node5_name), - std::string(node5_namespace), - std::string(security_context_name))); + expected_nodes.insert( + std::make_tuple( + std::string(node5_name), + std::string(node5_namespace), + std::string(security_context_name))); std::this_thread::sleep_for(1s); From 86da265cbe4ce34f47a1900b9e6425b56c299f67 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 26 Mar 2020 16:07:33 -0300 Subject: [PATCH 27/30] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/graph.h | 4 ++-- .../rcl/validate_security_context_name.h | 4 ++-- rcl/src/rcl/arguments.c | 9 +++++++++ rcl/src/rcl/init.c | 2 +- rcl/src/rcl/validate_security_context_name.c | 18 +++++++++--------- 5 files changed, 23 insertions(+), 14 deletions(-) diff --git a/rcl/include/rcl/graph.h b/rcl/include/rcl/graph.h index 8ae92104d..a99af360d 100644 --- a/rcl/include/rcl/graph.h +++ b/rcl/include/rcl/graph.h @@ -443,8 +443,8 @@ rcl_get_node_names( /// Return a list of available nodes in the ROS graph, including their security context names. /** - * \sa Works like rcl_get_node_names, but it also include the security context name the node is - * using as an output. + * \sa An \ref rcl_get_node_names equivalent, but including in its output the security context + * name the node is using. * *
* Attribute | Adherence diff --git a/rcl/include/rcl/validate_security_context_name.h b/rcl/include/rcl/validate_security_context_name.h index ccc494ed5..7b877b6fc 100644 --- a/rcl/include/rcl/validate_security_context_name.h +++ b/rcl/include/rcl/validate_security_context_name.h @@ -44,8 +44,8 @@ extern "C" /// Determine if a given security context name is valid. /** - * /sa The same rules as rmw_validate_namespace are used. - * The only difference is the maximum length, which can be 255 characters. + * \sa The same rules as \ref rmw_validate_namespace are used. + * The only difference is in the maximum allowed length, which can be up to 255 characters. * * \param[in] security_context security_context to be validated * \param[out] validation_result int in which the result of the check is stored diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 6112fd351..2544f86a8 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -1100,6 +1100,15 @@ rcl_arguments_copy( } } } + char * security_context_copy = rcutils_strdup(args->impl->security_context, allocator); + if (args->impl->security_context && !security_context_copy) { + if (RCL_RET_OK != rcl_arguments_fini(args_out)) { + RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error"); + } + RCL_SET_ERROR_MSG("Error while copying security context argument"); + return RCL_RET_BAD_ALLOC; + } + args_out->impl->security_context = security_context_copy; return RCL_RET_OK; } diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index af9930711..a5151b275 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -195,7 +195,7 @@ rcl_init( } if (RCL_SECURITY_CONTEXT_NAME_VALID != validation_result) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "security context name is not valid: '%s'. Invalid index: %zu", + "Security context name is not valid: '%s'. Invalid index: %zu", rcl_security_context_name_validation_result_string(validation_result), invalid_index); fail_ret = RMW_RET_ERROR; diff --git a/rcl/src/rcl/validate_security_context_name.c b/rcl/src/rcl/validate_security_context_name.c index 71344e54a..cca876e6f 100644 --- a/rcl/src/rcl/validate_security_context_name.c +++ b/rcl/src/rcl/validate_security_context_name.c @@ -53,18 +53,18 @@ rcl_validate_security_context_name_with_size( return RCL_RET_INVALID_ARGUMENT; } - int t_validation_result; - size_t t_invalid_index; + int tmp_validation_result; + size_t tmp_invalid_index; rmw_ret_t ret = rmw_validate_namespace_with_size( - security_context, security_context_length, &t_validation_result, &t_invalid_index); + security_context, security_context_length, &tmp_validation_result, &tmp_invalid_index); if (ret != RMW_RET_OK) { return rcl_convert_rmw_ret_to_rcl_ret(ret); } - if (t_validation_result != RMW_NAMESPACE_VALID && - t_validation_result != RMW_NAMESPACE_INVALID_TOO_LONG) + if (tmp_validation_result != RMW_NAMESPACE_VALID && + tmp_validation_result != RMW_NAMESPACE_INVALID_TOO_LONG) { - switch (t_validation_result) { + switch (tmp_validation_result) { case RMW_NAMESPACE_INVALID_IS_EMPTY_STRING: *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_IS_EMPTY_STRING; break; @@ -91,7 +91,7 @@ rcl_validate_security_context_name_with_size( default_err_msg, sizeof(default_err_msg), "rcl_validate_security_context_name_with_size(): " "unknown rmw_validate_namespace_with_size() result '%d'", - t_validation_result); + tmp_validation_result); if (ret < 0) { RCL_SET_ERROR_MSG( "rcl_validate_security_context_name_with_size(): " @@ -103,13 +103,13 @@ rcl_validate_security_context_name_with_size( return RCL_RET_ERROR; } if (invalid_index) { - *invalid_index = t_invalid_index; + *invalid_index = tmp_invalid_index; } return RCL_RET_OK; } // security_context might be longer that namespace length, check false positives and correct - if (t_validation_result == RMW_NAMESPACE_INVALID_TOO_LONG && + if (tmp_validation_result == RMW_NAMESPACE_INVALID_TOO_LONG && security_context_length <= RCL_SECURITY_CONTEXT_NAME_MAX_LENGTH) { *validation_result = RCL_SECURITY_CONTEXT_NAME_VALID; From 46322dc84e802b5c345ef4e082041c1d5d49b1f8 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 26 Mar 2020 16:39:23 -0300 Subject: [PATCH 28/30] Address more reviewers' comments Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/graph.h | 2 +- rcl/include/rcl/security.h | 5 +++-- rcl/include/rcl/validate_security_context_name.h | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/rcl/include/rcl/graph.h b/rcl/include/rcl/graph.h index a99af360d..b0d65c4e0 100644 --- a/rcl/include/rcl/graph.h +++ b/rcl/include/rcl/graph.h @@ -453,7 +453,7 @@ rcl_get_node_names( * Thread-Safe | No * Uses Atomics | No * Lock-Free | Maybe [1] - * [1] implementation may need to protect the data structure with a lock + * [1] RMW implementation in use may need to protect the data structure with a lock * * \param[in] node the handle to the node being used to query the ROS graph * \param[in] allocator used to control allocation and deallocation of names diff --git a/rcl/include/rcl/security.h b/rcl/include/rcl/security.h index f729c1e25..ac570d2c0 100644 --- a/rcl/include/rcl/security.h +++ b/rcl/include/rcl/security.h @@ -104,8 +104,9 @@ rcl_get_enforcement_policy(rmw_security_enforcement_policy_t * policy); * 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. - * Such an override is useful for where the security context is non-deterministic before runtime, - * or when testing and using additional tools that may not otherwise be easily provisioned. + * Such an override is useful for applications where the security context 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 diff --git a/rcl/include/rcl/validate_security_context_name.h b/rcl/include/rcl/validate_security_context_name.h index 7b877b6fc..935baba97 100644 --- a/rcl/include/rcl/validate_security_context_name.h +++ b/rcl/include/rcl/validate_security_context_name.h @@ -64,10 +64,10 @@ rcl_validate_security_context_name( /// Deterimine if a given security context name is valid. /** - * This is an overload with an extra parameter for the length of security_context. - * \param[in] security_context The number of characters in security_context. + * \sa This is an overload of \ref rcl_validate_security_context_name with an extra parameter + * for the length of security_context. * - * \sa rcl_validate_security_context(const char *, int *, size_t *) + * \param[in] security_context The number of characters in security_context. */ RCL_PUBLIC RCL_WARN_UNUSED From 000e2e3122249a5881663f83efd34d26613eaa92 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 26 Mar 2020 17:54:43 -0300 Subject: [PATCH 29/30] Address reviewer comment Signed-off-by: Ivan Santiago Paunovic --- rcl/src/rcl/validate_security_context_name.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/rcl/src/rcl/validate_security_context_name.c b/rcl/src/rcl/validate_security_context_name.c index cca876e6f..655253af3 100644 --- a/rcl/src/rcl/validate_security_context_name.c +++ b/rcl/src/rcl/validate_security_context_name.c @@ -109,10 +109,15 @@ rcl_validate_security_context_name_with_size( } // security_context might be longer that namespace length, check false positives and correct - if (tmp_validation_result == RMW_NAMESPACE_INVALID_TOO_LONG && - security_context_length <= RCL_SECURITY_CONTEXT_NAME_MAX_LENGTH) - { - *validation_result = RCL_SECURITY_CONTEXT_NAME_VALID; + if (RMW_NAMESPACE_INVALID_TOO_LONG == tmp_validation_result) { + if (RCL_SECURITY_CONTEXT_NAME_MAX_LENGTH >= security_context_length) { + *validation_result = RCL_SECURITY_CONTEXT_NAME_VALID; + } else { + *validation_result = RCL_SECURITY_CONTEXT_NAME_INVALID_TOO_LONG; + if (invalid_index) { + *invalid_index = RCL_SECURITY_CONTEXT_NAME_MAX_LENGTH - 1; + } + } return RCL_RET_OK; } From 77aad14964b484c6091bebe0cec28169546303a9 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 30 Mar 2020 09:25:21 -0300 Subject: [PATCH 30/30] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rcl/include/rcl/graph.h | 2 +- rcl/include/rcl/validate_security_context_name.h | 4 ++-- rcl/src/rcl/arguments.c | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/rcl/include/rcl/graph.h b/rcl/include/rcl/graph.h index b0d65c4e0..6614d9fb7 100644 --- a/rcl/include/rcl/graph.h +++ b/rcl/include/rcl/graph.h @@ -443,7 +443,7 @@ rcl_get_node_names( /// Return a list of available nodes in the ROS graph, including their security context names. /** - * \sa An \ref rcl_get_node_names equivalent, but including in its output the security context + * An \ref rcl_get_node_names equivalent, but including in its output the security context * name the node is using. * *
diff --git a/rcl/include/rcl/validate_security_context_name.h b/rcl/include/rcl/validate_security_context_name.h index 935baba97..ceb5c3756 100644 --- a/rcl/include/rcl/validate_security_context_name.h +++ b/rcl/include/rcl/validate_security_context_name.h @@ -44,7 +44,7 @@ extern "C" /// Determine if a given security context name is valid. /** - * \sa The same rules as \ref rmw_validate_namespace are used. + * The same rules as \ref rmw_validate_namespace are used. * The only difference is in the maximum allowed length, which can be up to 255 characters. * * \param[in] security_context security_context to be validated @@ -64,7 +64,7 @@ rcl_validate_security_context_name( /// Deterimine if a given security context name is valid. /** - * \sa This is an overload of \ref rcl_validate_security_context_name with an extra parameter + * This is an overload of \ref rcl_validate_security_context_name with an extra parameter * for the length of security_context. * * \param[in] security_context The number of characters in security_context. diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 2544f86a8..08befe156 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -1104,8 +1104,9 @@ rcl_arguments_copy( if (args->impl->security_context && !security_context_copy) { if (RCL_RET_OK != rcl_arguments_fini(args_out)) { RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error"); + } else { + RCL_SET_ERROR_MSG("Error while copying security context argument"); } - RCL_SET_ERROR_MSG("Error while copying security context argument"); return RCL_RET_BAD_ALLOC; } args_out->impl->security_context = security_context_copy;