-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable use of Cyclone DDS security features #123
Changes from 3 commits
99d2738
bca0852
5e93420
1ca2269
581ba38
68d8b52
16134dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,6 +26,8 @@ | |||||||||||||||||||||||||
#include <utility> | ||||||||||||||||||||||||||
#include <regex> | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
#include "rcutils/filesystem.h" | ||||||||||||||||||||||||||
#include "rcutils/format_string.h" | ||||||||||||||||||||||||||
#include "rcutils/get_env.h" | ||||||||||||||||||||||||||
#include "rcutils/logging_macros.h" | ||||||||||||||||||||||||||
#include "rcutils/strdup.h" | ||||||||||||||||||||||||||
|
@@ -79,6 +81,13 @@ | |||||||||||||||||||||||||
#define SUPPORT_LOCALHOST 0 | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/* Security must be enabled when compiling and requires cyclone to support QOS property lists */ | ||||||||||||||||||||||||||
#if DDS_HAS_SECURITY && DDS_HAS_PROPERTY_LIST_QOS | ||||||||||||||||||||||||||
#define RMW_SUPPORT_SECURITY 1 | ||||||||||||||||||||||||||
#else | ||||||||||||||||||||||||||
#define RMW_SUPPORT_SECURITY 0 | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/* Set to > 0 for printing warnings to stderr for each messages that was taken more than this many | ||||||||||||||||||||||||||
ms after writing */ | ||||||||||||||||||||||||||
#define REPORT_LATE_MESSAGES 0 | ||||||||||||||||||||||||||
|
@@ -642,6 +651,94 @@ static std::string get_node_user_data(const char * node_name, const char * node_ | |||||||||||||||||||||||||
std::string(";"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
#if RMW_SUPPORT_SECURITY | ||||||||||||||||||||||||||
/* Returns the full URI of a security file properly formatted for DDS */ | ||||||||||||||||||||||||||
char * get_security_file_URI( | ||||||||||||||||||||||||||
const char * security_filename, const char * node_secure_root, | ||||||||||||||||||||||||||
const rcutils_allocator_t allocator) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
char * ret; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
char * file_path = rcutils_join_path(node_secure_root, security_filename, allocator); | ||||||||||||||||||||||||||
if (file_path == nullptr) { | ||||||||||||||||||||||||||
ret = nullptr; | ||||||||||||||||||||||||||
} else if (!rcutils_is_readable(file_path)) { | ||||||||||||||||||||||||||
RCUTILS_LOG_ERROR_NAMED( | ||||||||||||||||||||||||||
"rmw_cyclonedds_cpp", "get_security_file: %s not found", file_path); | ||||||||||||||||||||||||||
ret = nullptr; | ||||||||||||||||||||||||||
allocator.deallocate(file_path, allocator.state); | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
/* Cyclone also supports a "data:" URI */ | ||||||||||||||||||||||||||
ret = rcutils_format_string(allocator, "file:%s", file_path); | ||||||||||||||||||||||||||
allocator.deallocate(file_path, allocator.state); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
return ret; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
void store_security_filepath_in_qos( | ||||||||||||||||||||||||||
dds_qos_t * qos, const char * qos_property_name, const char * file_name, | ||||||||||||||||||||||||||
const rmw_node_security_options_t * security_options) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
rcutils_allocator_t allocator = rcutils_get_default_allocator(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
char * security_file_path = get_security_file_URI( | ||||||||||||||||||||||||||
file_name, security_options->security_root_path, allocator); | ||||||||||||||||||||||||||
if (security_file_path != nullptr) { | ||||||||||||||||||||||||||
dds_qset_prop(qos, qos_property_name, security_file_path); | ||||||||||||||||||||||||||
allocator.deallocate(security_file_path, allocator.state); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
#endif /* RMW_SUPPORT_SECURITY */ | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/* Set all the qos properties needed to enable DDS security */ | ||||||||||||||||||||||||||
rmw_ret_t configure_qos_for_security( | ||||||||||||||||||||||||||
dds_qos_t * qos, const rmw_node_security_options_t * security_options) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
#if RMW_SUPPORT_SECURITY | ||||||||||||||||||||||||||
/* File path is set to nullptr if file does not exist or is not readable */ | ||||||||||||||||||||||||||
store_security_filepath_in_qos( | ||||||||||||||||||||||||||
qos, "dds.sec.auth.identity_ca", "identity_ca.cert.pem", | ||||||||||||||||||||||||||
security_options); | ||||||||||||||||||||||||||
store_security_filepath_in_qos( | ||||||||||||||||||||||||||
qos, "dds.sec.auth.identity_certificate", "cert.pem", | ||||||||||||||||||||||||||
security_options); | ||||||||||||||||||||||||||
store_security_filepath_in_qos( | ||||||||||||||||||||||||||
qos, "dds.sec.auth.private_key", "key.pem", | ||||||||||||||||||||||||||
security_options); | ||||||||||||||||||||||||||
store_security_filepath_in_qos( | ||||||||||||||||||||||||||
qos, "dds.sec.access.permissions_ca", "permissions_ca.cert.pem", | ||||||||||||||||||||||||||
security_options); | ||||||||||||||||||||||||||
store_security_filepath_in_qos( | ||||||||||||||||||||||||||
qos, "dds.sec.access.governance", "governance.p7s", | ||||||||||||||||||||||||||
security_options); | ||||||||||||||||||||||||||
store_security_filepath_in_qos( | ||||||||||||||||||||||||||
qos, "dds.sec.access.permissions", "permissions.p7s", | ||||||||||||||||||||||||||
security_options); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.auth.library.path", "dds_security_auth"); | ||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.auth.library.init", "init_authentication"); | ||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.auth.library.finalize", "finalize_authentication"); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.crypto.library.path", "dds_security_crypto"); | ||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.crypto.library.init", "init_crypto"); | ||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.crypto.library.finalize", "finalize_crypto"); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.access.library.path", "dds_security_ac"); | ||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.access.library.init", "init_access_control"); | ||||||||||||||||||||||||||
dds_qset_prop(qos, "dds.sec.access.library.finalize", "finalize_access_control"); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return RMW_RET_OK; | ||||||||||||||||||||||||||
#else | ||||||||||||||||||||||||||
(void) qos; | ||||||||||||||||||||||||||
(void) security_options; | ||||||||||||||||||||||||||
RMW_SET_ERROR_MSG( | ||||||||||||||||||||||||||
"Security was requested but the Cyclone DDS being used does not have security " | ||||||||||||||||||||||||||
"support enabled. Recompile Cyclone DDS with the '-DENABLE_SECURITY=ON' " | ||||||||||||||||||||||||||
"CMake option"); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm always getting this error message if I build a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, this error message should probably only be set if security was specifically requested by the user, but that's only checked in the above branch. @SidFaber what do you think, add an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #172 for a possible fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should only happen when ROS_SECURITY_STRATEGY=Enforce and ROS_SECURITY_ENABLE=True. The full interplay of environment variables, RCL and RMW_CYCLONE is outlined here, the message should only show in lines 19, 20 and 22. @ivanpauno, can you confirm that is or is not what you're seeing? This should work with both the current and the security branches of rmw_cyclonedds_cpp. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm seeing this error without setting any security related environment variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #175 should fix this up, thanks for pointing this out. |
||||||||||||||||||||||||||
return RMW_RET_UNSUPPORTED; | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
extern "C" rmw_node_t * rmw_create_node( | ||||||||||||||||||||||||||
rmw_context_t * context, const char * name, | ||||||||||||||||||||||||||
const char * namespace_, size_t domain_id, | ||||||||||||||||||||||||||
|
@@ -666,7 +763,9 @@ extern "C" rmw_node_t * rmw_create_node( | |||||||||||||||||||||||||
static_cast<void>(domain_id); | ||||||||||||||||||||||||||
const dds_domainid_t did = DDS_DOMAIN_DEFAULT; | ||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
(void) security_options; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
RCUTILS_CHECK_ARGUMENT_FOR_NULL(security_options, nullptr); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
rmw_ret_t ret; | ||||||||||||||||||||||||||
int dummy_validation_result; | ||||||||||||||||||||||||||
size_t dummy_invalid_index; | ||||||||||||||||||||||||||
|
@@ -688,8 +787,22 @@ extern "C" rmw_node_t * rmw_create_node( | |||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
dds_qos_t * qos = dds_create_qos(); | ||||||||||||||||||||||||||
if (qos == nullptr) { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same nit here, could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikaelarguedas, found that this needs to do a bit of cleanup before so this is still here |
||||||||||||||||||||||||||
RCUTILS_LOG_ERROR_NAMED("rmw_cyclonedds_cpp", "rmw_create_node: Unable to create qos"); | ||||||||||||||||||||||||||
node_gone_from_domain_locked(did); | ||||||||||||||||||||||||||
return nullptr; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
std::string user_data = get_node_user_data(name, namespace_); | ||||||||||||||||||||||||||
dds_qset_userdata(qos, user_data.c_str(), user_data.size()); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (security_options->enforce_security) { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is incomplete, it sets the security QoS only if The expected behavior is to:
This is most likely the cause of the failing tests at ros2/system_tests#408 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikaelarguedas this is how the other middleware behaves, but help me understand if this is the behavior we truly want. The rcl sets security_options->enforce_security to either RMW_SECURITY_ENFORCEMENT_PERMISSIVE or RMW_SECURITY_ENFORCEMENT_ENFORCE. It's always set to RMW_SECURITY_ENFORCEMENT_PERMISSIVE when ROS_SECURITY_ENABLE is anything other than "true"; that is why it's used to completely bypass security qos here. With what you propose security will be enabled anytime ROS_SECURITY_ROOT_DIRECTORY exists regardless of ROS_SECURITY_ENABLE and ROS_SECURITY_STRATEGY. The rcl only checks for the directory, it (rightfully) does not ensure all six security files exist and are properly formatted. So node creation would fail any time the middleware cannot start with files in the security directory, even when ROS_SECURITY_ENABLE is set to false. Similarly ROS_SECURITY_ENABLE=false would not disable security for testing. Instead security would have to be disabled by changing ROS_SECURITY_ROOT_DIRECTORY or moving the security directory on disk. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If don't think the described behavior matches how the other middlewares behave, or the desired behavior. Putting it in a table for ease of read:
Anything not matching that behavior is a bug IMO. I agree that the line you are referring to is misleading https://github.com/ros2/rcl/blame/73948da4c50b11d5d133e068b804ce7c2b4f9cb6/rcl/src/rcl/node.c#L316-L317 and in practice serve no purpose and at first glance could be removed completely. Mimicking the logic of the other RMWs could be a good hint of how to implement it here: https://github.com/ros2/rmw_fastrtps/blob/bd5507b61c7285d6083e514ccd49f00b2945baeb/rmw_fastrtps_shared_cpp/src/rmw_node.cpp#L292 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikaelarguedas, thanks for the discussion. It was simple to switch the logic here, but you'll also see changes to This is now passing all everything in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's awesome.
Great improvement, we should consider modifying the other rmw implementations to make sure they do the same
🎉 I didnt test the new version as it's conflicting since #106 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SidFaber I gave it a go (with Dashing to verify it supports that, too) and I noticed that it warns
whenever it starts when security either isn't supported or couldn't be configured. That is, when security is:
Case (1) seems wrong (there is no expectation of security so also no expectation of getting warnings about it) and case (3) seems like a good thing to me. I am not quite sure what to make of case (2), I'd say it should be a hard fail but perhaps that's not the intent behind permissive mode (I don't quite get that mode anyway). @mikaelarguedas could you comment on that? Just for completeness: if security is enabled in enforced mode, it only starts when the security configuration is valid and the implementation supports it. Apart from the warning not being appropriate in all cases, it looks good to me! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍
👍
Originally the "Permissive" mode is to allow someone to launch in a single environment a system with some parts using security and some parts without. An example is such error message in rmw_fastrtps_cpp https://github.com/ros2/rmw_fastrtps/blob/48b403a211b269ab383e6adfcf89d1b4333b2c39/rmw_fastrtps_shared_cpp/src/participant.cpp#L238-L242 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about I just give an info message "Security is enabled" when the cyclone QOS object gets set up properly? It will normally follow the "found security directory" message that comes from rcl. I feel some messaging is important since the rcl message implies that security is enabled which may not be the case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I find the rcl message by itself pretty noisy when launching systems with many nodes, I'd rather not add an info message always. I would prefer a warning when users need to know something is not what they requested (case (3)), and nothing (or debug message) otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikaelarguedas, I just removed the info message: there are some tests where you'll get an rcl "found security" message and create a plain text node which is similar to how other middleware behaves. Although not ideal, this is something that would be better solved with logging.
SidFaber marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
if (configure_qos_for_security(qos, security_options) != RMW_RET_OK) { | ||||||||||||||||||||||||||
dds_delete_qos(qos); | ||||||||||||||||||||||||||
node_gone_from_domain_locked(did); | ||||||||||||||||||||||||||
return nullptr; | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This, too, can leak the domain reference, and moreover leaks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Regarding that: such issues can be significantly reduced by making more substantial use of C++ features here. Can you explain why this is essentially C? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, the RMW layer is intended to be in C++. Part of the reason that it is so C-like is that I am much more experienced with C than with C++ and so whenever I try to write C++ it still ends up looking like C. There is also the fact that Cyclone is in C, that this is therefore relying on a C API, and that some of the niceties of C++ (like destructors) are simply not available on these objects. There is an implementation of a standard C++11 API of DDS waiting for approval by the Eclipse Foundation's IP review process. That interface covers all the standard features but not all the additional tricks. At some point it might be worth rewriting the RMW implementation to use that API, it's only about a handful of lines of code anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fair enough, that's a perfectly valid reason.
In case you're unaware, shared/unique ptrs can be handed custom deleters that might help with that particular issue. Anyway, I don't want to digress from the PR too much, just found myself wondering about this. Thanks for the info! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That sounds like something worth reading up on, thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eboasson better yet, define a destructors for your classes. Then you don't need to pass a custom deleter to unique_ptr/shared_ptr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rotu that's not always possible:
My suggestion related particularly to that. Another common option is to wrap such C objects in a class responsible for managing it, but the trade off is more code to maintain. It also assumes you're comfortable with those C++ constructs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it's easier to maintain a wrapper class than a deleter, but YMMV. Where there's a matched initializer + deleter pair, I find it easiest to make them into a constructor + destructor. With a factory method + deleter pair, i prefer to use a class method + protected constructor + destructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
dds_entity_t pp = dds_create_participant(did, qos, nullptr); | ||||||||||||||||||||||||||
dds_delete_qos(qos); | ||||||||||||||||||||||||||
if (pp < 0) { | ||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh! The lifespan, deadline and liveliness QoS are fully supported nowadays, there aren't really any known limitations anymore! To be fixed in another PR, obviously.