Config security using enclave ROS argument #412
Conversation
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
There was a problem hiding this comment.
Thanks for working on this! Please find my feedback below.
Also, my preference is to not merge this PR while it still depends on rmw_dds_common. We only need one function from that library and I don't think we need to add a dependency for that reason especially since rmw_sercurity_common is in the works. Semantically, it also does not make sense for this middleware to depend on a DDS package. I see two paths forward:
- Get
rmw_sercurity_commonmerged upstream, backport that merge to Jazzy atleast, and release binaries for the package on Rolling and Jazzy. Then merge Use rmw_security_common package #434 into this one. - Temporarily copy in the definition and implementation of rmw_dds_common::get_security_files into this repo and use the function without linking depending on
rmw_dds_common.
Personally I think we should do 2) first (in this PR) and then when 1) is ready, delete the internal get_security_files and switch to the one in rmw_security_common.
| find_package(tracetools REQUIRED) | ||
| find_package(zenoh_cpp_vendor REQUIRED) | ||
|
|
||
| if(SECURITY) | ||
| find_package(OpenSSL REQUIRED) |
There was a problem hiding this comment.
I don't see OpenSSL being linked anywhere? Does this happen transitively via rmw_dds_common::rmw_dds_common_library?
| find_package(tracetools REQUIRED) | ||
| find_package(zenoh_cpp_vendor REQUIRED) | ||
|
|
||
| if(SECURITY) | ||
| find_package(OpenSSL REQUIRED) | ||
| set(HAVE_SECURITY 1) |
There was a problem hiding this comment.
Let's not have any special build flags for security. We will overwrite the Zenoh config with auth params if security_options is present in rmw_context_impl_s.
| @@ -52,7 +53,8 @@ class rmw_context_impl_s::Data final : public std::enable_shared_from_this<Data> | |||
| // Constructor. | |||
| Data( | |||
| std::size_t domain_id, | |||
| const std::string & enclave) | |||
| const std::string & enclave, | |||
| const rmw_security_options_t * security_options) | |||
There was a problem hiding this comment.
| const rmw_security_options_t * security_options) | |
| const rmw_security_options_t & security_options) |
| #ifdef HAVE_SECURITY | ||
| std::unordered_map<std::string, std::string> security_files_paths; | ||
| if (rmw_dds_common::get_security_files( | ||
| true, "", security_options->security_root_path, security_files_paths)) |
There was a problem hiding this comment.
| true, "", security_options->security_root_path, security_files_paths)) | |
| true, "", security_options.security_root_path, security_files_paths)) |
| @@ -67,7 +69,37 @@ class rmw_context_impl_s::Data final : public std::enable_shared_from_this<Data> | |||
| if (!config.has_value()) { | |||
| throw std::runtime_error("Error configuring Zenoh session."); | |||
| } | |||
|
|
|||
| #ifdef HAVE_SECURITY | |||
There was a problem hiding this comment.
Why do we need this flag? My understanding is that rmw_dds_common::get_security_files will return false if the enclave ROS arg is not passed or is invalid so we can have this codeblock that modifies for all situations. Let me know if I misunderstood something.
| "\t\t\t\t\"root_ca_certificate\": \"" + security_files_paths["IDENTITY_CA"] + "\",\n" + | ||
| "\t\t\t\t\"listen_private_key\": \"" + security_files_paths["PRIVATE_KEY"] + "\",\n" + | ||
| "\t\t\t\t\"listen_certificate\": \"" + security_files_paths["CERTIFICATE"] + "\",\n" + | ||
| "\t\t\t\t\"connect_private_key\": \"" + security_files_paths["PRIVATE_KEY"] + "\",\n" + | ||
| "\t\t\t\t\"connect_certificate\": \"" + security_files_paths["CERTIFICATE"] + "\",\n" + |
There was a problem hiding this comment.
Let's check that these keys exist in security_files_paths before accessing their values. If not present, return an error.
| @@ -432,9 +464,10 @@ class rmw_context_impl_s::Data final : public std::enable_shared_from_this<Data> | |||
| ///============================================================================= | |||
| rmw_context_impl_s::rmw_context_impl_s( | |||
| const std::size_t domain_id, | |||
| const std::string & enclave) | |||
| const std::string & enclave, | |||
| const rmw_security_options_t * security_options) | |||
There was a problem hiding this comment.
| const rmw_security_options_t * security_options) | |
| const rmw_security_options_t & security_options) |
| @@ -40,7 +40,8 @@ struct rmw_context_impl_s final | |||
| // check has not succeeded. | |||
| rmw_context_impl_s( | |||
| const std::size_t domain_id, | |||
| const std::string & enclave); | |||
| const std::string & enclave, | |||
| const rmw_security_options_t * security_options); | |||
There was a problem hiding this comment.
| const rmw_security_options_t * security_options); | |
| const rmw_security_options_t & security_options); |
| @@ -110,7 +110,8 @@ rmw_init(const rmw_init_options_t * options, rmw_context_t * context) | |||
| return RMW_RET_BAD_ALLOC, | |||
| rmw_context_impl_t, | |||
| context->actual_domain_id, | |||
| std::string(options->enclave) | |||
| std::string(options->enclave), | |||
| &context->options.security_options | |||
There was a problem hiding this comment.
| &context->options.security_options | |
| context->options.security_options |
As I mentioned in this other PR #411 (comment) we should be able to configure the security features using
--ros-args --enclave <enclave name>.This PR drafted the required changes in the code to be able to set them, note that the Zenoh Config should be configure before the session is created.
What do you think about the changes?
Another question, Up to this point, we only supported one kind of rmw: DDS, we are using a package called
rmw_dds_commonto get the certificates, etc, in this package which is not DDS based. I know it's just a naming thing, but do we need to create a more generic package name and move some stuff there? @clalancette @Yadunund ?