Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable use of Cyclone DDS security features #123

Merged
merged 7 commits into from
Apr 8, 2020
Merged

Enable use of Cyclone DDS security features #123

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99d2738
Enable use of Cyclone DDS security features
SidFaber Mar 5, 2020
bca0852
Update conditional compile logic
SidFaber Mar 23, 2020
5e93420
Fix memory leaks
SidFaber Mar 27, 2020
1ca2269
Improve security logic and memory management
SidFaber Apr 2, 2020
581ba38
Merge branch 'master' into dds-security
SidFaber Apr 2, 2020
68d8b52
Removed stray newline
SidFaber Apr 2, 2020
16134dc
Merge remote-tracking branch 'origin/master' into dds-security
SidFaber Apr 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,5 @@ DDS directly instead of via the ROS2 abstraction.

## Known limitations

Cyclone DDS doesn't yet implement the DDS Security standard, nor does it fully implement
Copy link
Collaborator

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.

the Lifespan, Deadline and some of the Liveliness QoS modes. Consequently these features
of ROS2 are also not yet supported when using Cyclone DDS.
Cyclone DDS doesn't yet fully implement the Lifespan, Deadline and some of the Liveliness QoS modes.
Consequently these features of ROS2 are also not yet supported when using Cyclone DDS.
94 changes: 93 additions & 1 deletion rmw_cyclonedds_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@
#include <utility>
#include <regex>

#include "rcutils/filesystem.h"
#include "rcutils/get_env.h"
#include "rcutils/logging_macros.h"
#include "rcutils/strdup.h"
#include "rcutils/format_string.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: alphabetical order for includes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in bca0852


#include "rmw/allocators.h"
#include "rmw/convert_rcutils_ret_to_rmw_ret.h"
Expand Down Expand Up @@ -642,6 +644,80 @@ static std::string get_node_user_data(const char * node_name, const char * node_
std::string(";");
}

/* 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);
}
}

/* Set all the qos properties needed to enable DDS security */
void configure_qos_for_security(
dds_qos_t * qos, const rmw_node_security_options_t * security_options)
{
/* 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", "libdds_security_auth.so");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "lib" and ".so" affixes will cause trouble on macOS and Windows. If the specified file does not exist, Cyclone also tries the names extended in the platform's native way (so lib/.dylib for macOS and .dll for Windows). So just "dds_security_auth" seems better (and analogously for the other libraries).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in bca0852

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks platfrom specific, maybe there is support for the library basename and cyclone then modifies and look for specific names based on the platform (ala class_loader https://github.com/ros/class_loader/blob/11982ab3efc390608f10f2db1e97edf4037afec5/src/class_loader.cpp#L51-L73)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with bca0852

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", "libdds_security_crypto.so");
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", "libdds_security_ac.so");
dds_qset_prop(qos, "dds.sec.access.library.init", "init_access_control");
dds_qset_prop(qos, "dds.sec.access.library.finalize", "finalize_access_control");
}

extern "C" rmw_node_t * rmw_create_node(
rmw_context_t * context, const char * name,
const char * namespace_, size_t domain_id,
Expand All @@ -666,7 +742,13 @@ 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;

if (security_options == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative: Use RCUTILS_CHECK_ARGUMENT_FOR_NULL macro instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with 1ca2269

RCUTILS_LOG_ERROR_NAMED(
"rmw_cyclonedds_cpp", "rmw_create_node: security options null");
return nullptr;
}

rmw_ret_t ret;
int dummy_validation_result;
size_t dummy_invalid_index;
Expand All @@ -688,8 +770,18 @@ extern "C" rmw_node_t * rmw_create_node(
#endif

dds_qos_t * qos = dds_create_qos();
if (qos == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nit here, could use RCUTILS_CHECK_FOR_NULL_WITH_MSG instead

Copy link
Contributor Author

@SidFaber SidFaber Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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");
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is incomplete, it sets the security QoS only if enforce_security is set.

The expected behavior is to:
If the security files can be found: setup the plugins
If they cannot be found:

  • if enforce_security is true -> exit with error message
  • else -> create a participant without the security plugins instanciated

This is most likely the cause of the failing tests at ros2/system_tests#408

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.
Overall the expected behavior is the one described at: https://design.ros2.org/articles/ros2_dds_security.html

Putting it in a table for ease of read:

ROS_SECURITY_ENABLE ROS_SECURITY_STRATEGY valid set of security files located under ROS_SECURE_ROOT_DIRECTORY Expected outcome
false * * No security plugins instanciated, creating non-secure participant
true Enforce Yes creates secure participant
true Enforce No fails to create participant
true Permissive Yes creates secure participant
true Permissive No creates unsecure participant

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.
The one that matters in our case is the else branch of that if: https://github.com/ros2/rcl/blame/73948da4c50b11d5d133e068b804ce7c2b4f9cb6/rcl/src/rcl/node.c#L318-L323
If ROS_SECURITY_ENABLE is false: node_security_options.security_root_path is not set and this is the variable that tells you if you should try to instantiate the security plugins or not.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 configure_qos_for_security: all files are checked before any properties are added to qos to enable cyclone dds security.

This is now passing all everything in the test_security, that's been a great help and I'd like to see those test routines continue to mature!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome.

all files are checked before any properties are added to qos to enable cyclone dds security.

Great improvement, we should consider modifying the other rmw implementations to make sure they do the same

This is now passing all everything in the test_security, that's been a great help and I'd like to see those test routines continue to mature!

🎉

I didnt test the new version as it's conflicting since #106

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

[INFO] [rmw_cyclonedds_cpp]: rmw_create_node: Unable to configure security

whenever it starts when security either isn't supported or couldn't be configured. That is, when security is:

  1. disabled, regardless of whether security support is included in the build;
  2. enabled in permissive mode, when security support not included in the build;
  3. enabled in permissive mode, when security support is included in the build but something is awry with the configuration.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case (1) seems wrong

👍

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)

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.
I don't think it's been used much in practice.
In the case of 2, the rmw implementation itself doesnt support security (because not included in the build) so there's no chance of having any part of the system using security. So IMO it should hard fail and provide a clearer error message as of why.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
configure_qos_for_security(qos, security_options);
}

dds_entity_t pp = dds_create_participant(did, qos, nullptr);
dds_delete_qos(qos);
if (pp < 0) {
Expand Down