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

Changes from all commits
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
161 changes: 160 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 @@ -27,6 +27,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"
Expand Down Expand Up @@ -81,6 +83,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
Expand Down Expand Up @@ -140,6 +149,18 @@ struct builtin_readers
dds_entity_t rds[sizeof(builtin_topics) / sizeof(builtin_topics[0])];
};

#if RMW_SUPPORT_SECURITY
struct dds_security_files_t
{
char * identity_ca_cert = nullptr;
char * cert = nullptr;
char * key = nullptr;
char * permissions_ca_cert = nullptr;
char * governance_p7s = nullptr;
char * permissions_p7s = nullptr;
};
#endif

struct CddsEntity
{
dds_entity_t enth;
Expand Down Expand Up @@ -690,6 +711,131 @@ static std::string get_node_user_data(
#else
#define get_node_user_data get_node_user_data_name_ns
#endif
#if RMW_SUPPORT_SECURITY
/* Returns the full URI of a security file properly formatted for DDS */
bool get_security_file_URI(
char ** security_file, const char * security_filename, const char * node_secure_root,
const rcutils_allocator_t allocator)
{
*security_file = nullptr;
char * file_path = rcutils_join_path(node_secure_root, security_filename, allocator);
if (file_path != nullptr) {
if (rcutils_is_readable(file_path)) {
/* Cyclone also supports a "data:" URI */
*security_file = rcutils_format_string(allocator, "file:%s", file_path);
allocator.deallocate(file_path, allocator.state);
} else {
RCUTILS_LOG_INFO_NAMED(
"rmw_cyclonedds_cpp", "get_security_file_URI: %s not found", file_path);
allocator.deallocate(file_path, allocator.state);
}
}
return *security_file != nullptr;
}

bool get_security_file_URIs(
#if !RMW_VERSION_GTE(0, 8, 2)
const rmw_node_security_options_t * security_options,
#else
const rmw_security_options_t * security_options,
#endif
dds_security_files_t & dds_security_files, rcutils_allocator_t allocator)
{
bool ret = false;

if (security_options->security_root_path != nullptr) {
ret = (
get_security_file_URI(
&dds_security_files.identity_ca_cert, "identity_ca.cert.pem",
security_options->security_root_path, allocator) &&
get_security_file_URI(
&dds_security_files.cert, "cert.pem",
security_options->security_root_path, allocator) &&
get_security_file_URI(
&dds_security_files.key, "key.pem",
security_options->security_root_path, allocator) &&
get_security_file_URI(
&dds_security_files.permissions_ca_cert, "permissions_ca.cert.pem",
security_options->security_root_path, allocator) &&
get_security_file_URI(
&dds_security_files.governance_p7s, "governance.p7s",
security_options->security_root_path, allocator) &&
get_security_file_URI(
&dds_security_files.permissions_p7s, "permissions.p7s",
security_options->security_root_path, allocator));
}
return ret;
}

void finalize_security_file_URIs(
dds_security_files_t dds_security_files, const rcutils_allocator_t allocator)
{
allocator.deallocate(dds_security_files.identity_ca_cert, allocator.state);
dds_security_files.identity_ca_cert = nullptr;
allocator.deallocate(dds_security_files.cert, allocator.state);
dds_security_files.cert = nullptr;
allocator.deallocate(dds_security_files.key, allocator.state);
dds_security_files.key = nullptr;
allocator.deallocate(dds_security_files.permissions_ca_cert, allocator.state);
dds_security_files.permissions_ca_cert = nullptr;
allocator.deallocate(dds_security_files.governance_p7s, allocator.state);
dds_security_files.governance_p7s = nullptr;
allocator.deallocate(dds_security_files.permissions_p7s, allocator.state);
dds_security_files.permissions_p7s = nullptr;
}

#endif /* RMW_SUPPORT_SECURITY */

/* Attempt to set all the qos properties needed to enable DDS security */
rmw_ret_t configure_qos_for_security(
dds_qos_t * qos,
#if !RMW_VERSION_GTE(0, 8, 2)
const rmw_node_security_options_t * security_options
#else
const rmw_security_options_t * security_options
#endif
)
{
#if RMW_SUPPORT_SECURITY
rmw_ret_t ret = RMW_RET_UNSUPPORTED;
dds_security_files_t dds_security_files;
rcutils_allocator_t allocator = rcutils_get_default_allocator();

if (get_security_file_URIs(security_options, dds_security_files, allocator)) {
dds_qset_prop(qos, "dds.sec.auth.identity_ca", dds_security_files.identity_ca_cert);
dds_qset_prop(qos, "dds.sec.auth.identity_certificate", dds_security_files.cert);
dds_qset_prop(qos, "dds.sec.auth.private_key", dds_security_files.key);
dds_qset_prop(qos, "dds.sec.access.permissions_ca", dds_security_files.permissions_ca_cert);
dds_qset_prop(qos, "dds.sec.access.governance", dds_security_files.governance_p7s);
dds_qset_prop(qos, "dds.sec.access.permissions", dds_security_files.permissions_p7s);

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");

ret = RMW_RET_OK;
}
finalize_security_file_URIs(dds_security_files, allocator);
return ret;
#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");
Copy link
Member

Choose a reason for hiding this comment

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

I'm always getting this error message if I build a ros2 workspace without passing -DENABLE_SECURITY=ON and run any demo (using cyclone)
I think rmw_cyclonedds_cpp should be able to automatically detect if security is available or not at build time.

Copy link
Member

@kyrofa kyrofa Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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 if (security_options->security_root_path != nullptr) here?

Copy link
Member

Choose a reason for hiding this comment

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

See #172 for a possible fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@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!

I'm seeing this error without setting any security related environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -720,7 +866,11 @@ extern "C" rmw_node_t * rmw_create_node(
const dds_domainid_t did = DDS_DOMAIN_DEFAULT;
#endif
#if !RMW_VERSION_GTE(0, 8, 2)
(void) security_options;
RCUTILS_CHECK_ARGUMENT_FOR_NULL(security_options, nullptr);
#else
rmw_security_options_t * security_options;
RCUTILS_CHECK_ARGUMENT_FOR_NULL(context, nullptr);
security_options = &context->options.security_options;
#endif
rmw_ret_t ret;
int dummy_validation_result;
Expand Down Expand Up @@ -749,6 +899,15 @@ extern "C" rmw_node_t * rmw_create_node(
std::string user_data = get_node_user_data(name, namespace_);
#endif
dds_qset_userdata(qos, user_data.c_str(), user_data.size());

if (configure_qos_for_security(qos, security_options) != RMW_RET_OK) {
if (security_options->enforce_security == RMW_SECURITY_ENFORCEMENT_ENFORCE) {
dds_delete_qos(qos);
node_gone_from_domain_locked(did);
return nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This, too, can leak the domain reference, and moreover leaks qos. Isn't C just a lovely language, always being on the lookout for ways to get you? 😄

Copy link
Member

@kyrofa kyrofa 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.

Isn't C just a lovely language, always being on the lookout for ways to get you?

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

@kyrofa kyrofa 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.

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.

Fair enough, that's a perfectly valid reason.

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.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

In case you're unaware, shared/unique ptrs can be handed custom deleters that might help with that particular issue.

That sounds like something worth reading up on, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Member

@kyrofa kyrofa Apr 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@rotu that's not always possible:

[...] 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.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eboasson, this should all be covered with 1ca2269.

}
}

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