Update documentation

[artivis]

Bug report

Required Info:

• Operating System:
  • Ubuntu 18.04
• Installation type:
  • master
• DDS implementation:
  • rmw_fastrtps

Steps to reproduce issue

Simply follow the quick tutorial at SROS2_Linux

Expected behavior

Run a secure talker/listener.

Actual behavior

>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:

  'couldn't find all security files!

Additional information

Looks like the doc wasn't updated with #177.

Got it working with the following changes,

export ROS_SECURITY_ROOT_DIRECTORY=~/sros2_demo/demo_keys/contexts/

ros2 run demo_nodes_cpp talker --security-context /talker

ros2 run demo_nodes_cpp listener --security-context /listener

[ivanpauno]

Yes, this is one of the TODOs in https://github.com/ros2/sros2/issues/182.

@ruffsl @mikaelarguedas @kyrofa do any of you have time to contribute with this?

[kyrofa]

Yeah it wouldn't be hard, but I think things are still in flux, right? Like the root dir pointing at contexts, etc.?

[ruffsl]

Like the root dir pointing at contexts

Should be merged soon: https://github.com/ros2/system_tests/pull/410

[mikaelarguedas]

updating the doc may actually give us extra insight on the usability of the new interface. From the extract from @artivis above a couple questions come up:

ros2 run demo_nodes_cpp listener --security-context /listener

1. Do we need a --ros-args before the --security-context flag ?
2. ros2 run demo_nodes_cpp listener --ros-args --security-context /listener Thats pretty verbose.. could we make this more type-able ? maybe by providing a short flag for --security-context

[ruffsl]

maybe by providing a short flag for --security-context

Perhaps we could change it to --context given it's just the context path, and the context path might be used for other things other than security. The short flag could then just be intuitively -c, as I don't think it's been reserved for anything else under ros args.

[kyrofa]

Perhaps we could change it to --context given it's just the context path, and the context path might be used for other things other than security.

+1 to that, my thinking as well.

[mikaelarguedas]

Perhaps we could change it to --context given it's just the context path, and the context path might be used for other things other than security

That's how it was originally in the design doc. I'm still ok with it but would prefer to review why it was decided to change it from --context to --security-context in the first place. I think it was to avoid the ambiguity as people may think that multiple process can be launched using the same "context" (which is not possible). I'd like to find the discussion on the rcl PR but github makes it hard these days with all the hidden discussions and comments. maybe @ivanpauno will remember

I'm also fine with keeping --security-context as long as we provide a short flag for it.

[ivanpauno]

Do we need a --ros-args before the --security-context flag ?

Yes, the decision was that every ros specific argument goes after a --ros-args.

Thats pretty verbose.. could we make this more type-able ? maybe by providing a short flag for --security-context

Adding a short flag sounds good to me.

I think it was to avoid the ambiguity as people may think that multiple process can be launched using the same "context" (which is not possible). I'd like to find the discussion on the rcl PR but github makes it hard these days with all the hidden discussions and comments. maybe @ivanpauno will remember

My main reason to use --security-context and not --context was that there may be reasons to use a different name for contexts in the same process, and security context flag applies to all contexts in a process.

Maybe @wjwwood or @hidmic have a different opinion.

[ruffsl]

I'd like to find the discussion on the rcl PR but github makes it hard these days with all the hidden discussions and comments. maybe @ivanpauno will remember

@ivanpauno and I iterated over a comment on https://github.com/ros2/rcl/pull/515 , but apparently if you make a single comment on a PR, github blows the lime comment away after rebasing. As you mentioned, the design doc had originally specified --context, but the rcl PR was using something like --security-directory that wasn't entirely accurate, and so we just met in the middle with --security-context.

Going forward, I don't think something alternatively starting with security- would be as nice given the short flag of -s is not as descriptive.

Also, I think -c could implement a handy path autocompleter where it could expand prefix matches based on the available context paths in the current keystore at runtime to help the user.

[ruffsl]

My main reason to use --security-context and not --context was that there may be reasons to use a different name for contexts in the same process, and security context flag applies to all contexts in a process.

We resolved the security issue with bridges, such that all contexts in a bridge process should use the same context path. Was there a lingering reason for allowing multiple context path per process?

[ivanpauno]

We resolved the security issue with bridges, such that all contexts in a bridge process should use the same context path. Was there a lingering reason for allowing multiple context path per process?

We're using it now just for security, but for other use cases, I'm not sure if allowing just one context name per process is desired.

e.g.: for https://github.com/ros2/rmw_fastrtps/pull/335 it might not be ideal.

Also, I think -c could implement a handy path autocompleter where it could expand prefix matches based on the available context paths in the current keystore at runtime to help the user.

The short version sounds good. We currently don't have any autocompleter for ros arguments, so we would first need to solve that ...

[kyrofa]

Yes, the decision was that every ros specific argument goes after a --ros-args.

See https://github.com/ros2/design/pull/242 for context around that.

[wjwwood]

We're using it now just for security, but for other use cases, I'm not sure if allowing just one context name per process is desired.

If the context actually have names (wasn't clear to me if that even happened in the end), then having one context name per process is not a good idea.

It seems like this security option is completely divested from (rcl) contexts now, so having context in the name might be a bit confusing?

I dunno, you guys will have to take ownership on that decision, I probably won't have time to comment on it again until the API freeze settles down.

[hidmic]

My main reason to use --security-context and not --context was that there may be reasons to use a different name for contexts in the same process, and security context flag applies to all contexts in a process.

I agree we have to be careful with terminology overloads and/or qualification.

[ivanpauno]

I agree we have to be careful with terminology overloads and/or qualification.

enclaves has been proposed over security-contexts, to avoid the confusion.

[ivanpauno]

@kyrofa @ruffsl @mikaelarguedas Let me know if you can update the tutorials. We will start Foxy testing soon, and it covers the security tutorials.

Thanks!

[ruffsl]

Is it just the readme markdown files in this repo, or are there additional sros2 tutorial docs elsewhere as well?

[mikaelarguedas]

See https://github.com/ros2/sros2/pull/201 for a tutorial update

[ivanpauno]

Closing after #201!