Add basic support for security logging plugin

[kyrofa]

Support setting the security logging properties in Connext by exposing three environment variables:

• ROS_SECURITY_LOG_FILE: Log security events to the provided file path
• ROS_SECURITY_LOG_PUBLISH: Publish security events to DDS topic
• ROS_SECURITY_LOG_VERBOSITY: Control verbosity of logged events

cc @mikaelarguedas and @artivis

[artivis]

lgtm

[ruffsl]

Questions:

• Is there a particular reason for exposing these logging options using environment variables rather than through the vendor QoS XML settings file?
  • Is it to harmonize with the other ros security envs?
  • Is setting the vendor QoS XML too out of band?
• Are we only defining these environment variables in this rmw because connext is the only DDS vendor with a security logging plugin?
  • They seem like they should be declared in rcl.
• How does this effect the behavior of stdout/stderr with respect to security log events?
  • I suppose this will be vendor specific.

[kyrofa]

Is there a particular reason for exposing these logging options using environment variables rather than through the vendor QoS XML settings file?

• Is it to harmonize with the other ros security envs?
• Is setting the vendor QoS XML too out of band?

Yeah, you got it. We want the user to be able to enable/disable security logging on the ROS end without having to care about the DDS implementation they're using.

Are we only defining these environment variables in this rmw because connext is the only DDS vendor with a security logging plugin?

• They seem like they should be declared in rcl.

No, we propose doing the same in rmw_fastrtps. Long-term this should indeed be in RCL, but we felt it needed more design before we got to that point. Security logging is actually something configured on a per-participant basis, so we have an XML file format designed for this that we were thinking would be generated by the SROS2 utilities, but it needs an actual design doc written, and the context work and introduction of enclaves has changed that a bit and we need to revisit. In the short-term, some environment variables to allow people to start playing with this functionality while we properly design the real deal seemed the best path forward.

Would you prefer to see this added to RCL and shoved into rmw_security_options_t, even knowing that this isn't the final story?

How does this effect the behavior of stdout/stderr with respect to security log events?

• I suppose this will be vendor specific.

It is indeed vendor-specific. Regarding Connext, if the security log is enabled, it will no longer show security events on stdout/stderr. Not something we recommend enabling by default as a result.

[ruffsl]

Would you prefer to see this added to RCL and shoved into rmw_security_options_t, even knowing that this isn't the final story?

It might be nice to introduce it into rcl, even its beta, as even then other rmw vendors could be added to the beta feature mid release. I'd like to rework rmw_security_options_t in the next release anyway to see if it could handle PEM files and passphrases as well, so that something like rcl_security could load from more exotic keystore sources.

@ivanpauno or @mikaelarguedas might have more of an option on pushing these log env options up into rcl instead.

[kyrofa]

I'd like to rework rmw_security_options_t in the next release anyway

Same here, but note that I'm trying to get this stuff into Foxy. That's why I'm trying to keep it as small as possible and not change API at this late date.

even then other rmw vendors could be added to the beta feature mid release

Nothing prevents that, even without changes to RCL, other than the fact that we have a feature freeze. At this point I'm happy to do what will make this land, but we don't have time to change it more than once unless the feature freeze somehow doesn't apply to this?

@ivanpauno, @jacobperron please share your thoughts on what we should do here. I can add this to RCL and add a few new items to rmw_security_options_t that will be ignored (not require code changes) in other RMWs if it's not too late to do something like that. Just note that rmw_security_options_t will in all likelihood need to change again once we have a proper abstraction in G turtle.

[jacobperron]

If you think any new additions to rmw_security_options_t are likely to change significantly in G-turtle, then I don't think it's worth moving this to rcl. My reasoning is it avoid breaks for any users who might end up using the new API introduced in Foxy. The env variables can be reused (or easily deprecated) while moving implementation to rcl in G-turtle.

[kyrofa]

Thank you @jacobperron, you outlined my position in far less words than I did 😉 . Okay, this is ready for a more thorough review, then. Once we're happy with it I can update ros2/rmw_fastrtps#362 to match.

[jacobperron]

This PR needs a rebase to get the latest changes upstream so that CI will pass.

[mikaelarguedas]

A bit late to the party but just to restate publically what happened in passed discussions.

End goal is to have a single entry point for configuring logging, that would be an XML file containing only ROS concepts (so not vendor specific and even not DDS-specific).
The code for parsing that xml and populating the relevant the datastructure should be implemented only once and reused by all implementations supporting secure logging.
Removing any requirement on environment variables in RCL is desired. The added ones here are temporary and do not impact the refactoring of environment variables in rcl into another package (ala ros2/rcl#545)

These were too big of changes to be made last minute for Foxy, this PR and corresponding rmw_fastrtps PR are minimal self contained implementation that allow to configure some parts of the logging plugin to allow a minimal use. This was meant as an MVP not touching asny public API, this is why all new functions and defines are private and why the code is duplicated between this PR and the rmw_fastrtps one.

+1 for no public API change and not change to rmw_security_options this time around.
Adding a TODO(security-wg) above all the temporary hardcoded strings and constants would be good to remind any reader that this is temporary and meant to be (re)factored out.

@kyrofa I thought this change came with an sros2 change to add the relevant pure DDS logging topic to the permissions. Is this change on a branch somewhere to be tested along this ?
Is there a branch from test_security allowing to validate that this work ? or is this something we're aiming for later on ?

I haven't been able to test this but hope to do so and review the PRs by tomorrow

[kyrofa]

This PR needs a rebase to get the latest changes upstream so that CI will pass.

Done.

[kyrofa]

Adding a TODO(security-wg) above all the temporary hardcoded strings and constants would be good to remind any reader that this is temporary and meant to be (re)factored out.

Good idea, just pushed that.

@kyrofa I thought this change came with an sros2 change to add the relevant pure DDS logging topic to the permissions. Is this change on a branch somewhere to be tested along this?

I'm working on that now, so not yet. However, this doesn't depend on that so much as that depends on this.

Is there a branch from test_security allowing to validate that this work ? or is this something we're aiming for later on?

No, but I fully intend to write one, particularly to ensure RMWs that support logging work the same way. I just need to learn my way around them, first 😄 . Other than the unit tests in this PR to ensure we're turning the right knobs, actually ensuring it turns into Connext logging as expected is currently a manual affair. The test_security addition should not be subject to the feature freeze like this is, however, so we should be able to get that in no problem.

[mikaelarguedas]

Gave this a quick try.
It works! logged to file successfully (it always append to the file instead of overwriting which I guess is nice when multiple participants use the same file).
Didnt get a chance to confirm that the logging to topic wors. Surprisingly it didnt complain about missing permissions to do so.

A bit confused with the verbosity levels, a longer comment about it inline

[mikaelarguedas]

TBH this has been pretty unexpected and I had to go in the code to figure out what value to put to get this working.
I epected one of the following to work:

• what's in the spec:
  • INFO_LEVEL
  • INFORMATIONAL_LEVEL
• What I saw in most DDS examples (Connext, CoreDX): numbers between 0 and 7
• The ROS severity levels https://github.com/ros2/rcutils/blob/a357b67e7d9f00f8fbfe306491438cd1e8ee5219/rcutils/logging.py#L17

And NONE worked...

I believe we need those to be both more visible, with a more informative error message than:
failed to set security logging verbosity: 7 is not a supported verbosity
We need to either accept more values, or pick ones that are already agreed on elsewhere, either the numbers or string matching the

[kyrofa]

Both Fast RTPS and Connext follow the enum in section 9.6 of DDS-Security... mostly. Connext uses the numeric value of the enum in 5.3, but deprecated that in v6 to use the strings instead, sans the "_LEVEL", which was really the model for this. Fast RTPS follows 9.6 exactly, including the "_LEVEL." However, given that:

1. We want all RMWs to work the same way, even given that this is temporary, and
2. We want this to be more of a ROS abstraction than a DDS-level thing

We settled on the more clear and less "go read the spec" ish option. You're right though-- would it help to simply print all the supported verbosities in the error message?

[kyrofa]

Also, I'd be happy to use the ROS verbosities instead if that seems more clear. I think that's a pretty good idea. There aren't quite as many levels though so we'll need to agree on a mapping. Maybe:

ROS verbosity	DDS verbosity
FATAL	EMERGENCY
(also ERROR)	ALERT
(also ERROR)	CRITICAL
ERROR	ERROR
WARN	WARNING
(also INFO)	NOTICE
INFO	INFORMATIONAL
DEBUG	DEBUG

Thoughts?

[mikaelarguedas]

I don't feel very strongly either way as this is temporary, as long as we make it more visible.
For the longer term, we'll need to decide what to put in the XML file exposed to users.
As that XML file aim to have only ROS concepts, maybe we should use ROS verbosity levels in the XML.
So for consistency I'd say let's use that here too.
The mapping in the table above looks good to me 👍

[artivis]

I can't really make my mind on whether we should map or not. I see the benefit of sticking to ROS verbosities but can't yet foresee the cons of having less levels. And the specs does not help much here, from section 9.6:

    EMERGENCY_LEVEL,    // System is unusable. Should not continue use.
    ALERT_LEVEL,        // Should be corrected immediately
    CRITICAL_LEVEL,     // A failure in primary application.
    ERROR_LEVEL,        // General error conditions
    WARNING_LEVEL,      // May indicate future error if action not taken.
    NOTICE_LEVEL,       // Unusual, but nor erroneous event or condition.
    INFORMATIONAL_LEVEL, // Normal operational. Requires no action.
    DEBUG_LEVEL

I guess either way is fine for now so we should prefer the most user friendly solution and that'd be using ROS verbosities.

[kyrofa]

Alright, just pushed a change to use ROS verbosities, as well as a nicer error message when an unsupported one is selected. Take another look?

[kyrofa]

Surprisingly it didnt complain about missing permissions to do so.

FYI, I'm able to confirm that logging to DDS works via the RTI admin console. That doesn't seem to be in accordance with my (nor your, it appears) reading of the spec, but it means that the sros2 profile tweak isn't required at this time. This will need some more investigation to fully understand, but it shouldn't stand in the way of this PR.

[kyrofa]

(ros2/rmw_fastrtps#362 has been updated to function the same way as this PR)

[ruffsl]

I like the ROS verbosity mapping. Could use a round of CI for a final check.

[mikaelarguedas]

lgtm with green CI

[kyrofa]

CI for rmw_connext_cpp and rmw_connext_shared_cpp:

• Linux
• Linux-aarch64
• macOS
• Windows

Notes:

• aarch64 seems to have issues unrelated to this PR (it seemingly can't find rmw_connext at all)
• The Windows warnings appear to be coming from rcutils' get_env.h, not this PR

[mikaelarguedas]

The Windows warnings appear to be coming from rcutils' get_env.h, not this PR

The fact that these warnings appear in this PR that uses rcutils_get_env and they are not present on master when multiple other packages use that same function is suspicious. It looks like this PR is introducing these warnigs and we should find a fix to not do that.

aarch64 seems to have issues unrelated to this PR (it seemingly can't find rmw_connext at all)

aarch failure is expected 👍 there is no connext deb for arm

[kyrofa]

I was hoping maybe the visibility macro was the hidden magic here, but apparently not:

• Linux
• macOS
• Windows

Note that ros2/rmw_fastrtps#362, which is super similar code, does not have a warning. I'm at a loss here. @sloretz any chance you have an idea?

[sloretz]

one small fix, otherwise LGTM!

[sloretz]

Note that ros2/rmw_fastrtps#362, which is super similar code, does not have a warning. I'm at a loss here. @sloretz any chance you have an idea?

No idea. Maybe the issue can be avoided for now by using the rcpputils version? https://github.com/ros2/rcpputils/blob/master/include/rcpputils/get_env.hpp

[kyrofa]

Alright, I've changed to using rcpputils, let's see if that resolves things:

• Linux
• macOS
• Windows

[sloretz]

LGTM with green CI

[kyrofa]

Excellent, all green now! No idea what was going on, but I like this better anyway.