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.

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

Fix access control for ardent #33

Merged
merged 13 commits into from Mar 1, 2018

Conversation

Projects
None yet
4 participants
@mikaelarguedas
Copy link
Contributor

commented Feb 21, 2018

connects to #32

This is the minimal set of changes to get access control working for Connext with the default talker/listener.

This doesn't aim to add feature to support service Access Control. Just ensuring that the nodes can be crerated and use enforced access control for pub sub.

What this does:

  • add the clock topic to allowed subscription by default
  • add the parameter_events topic to allowed publications and subscriptions by default
  • add publication for all default parameter service Request topics
  • add subscription for all default parameter service Reply topics
@andrew-konecki-apexai

This comment has been minimized.

Copy link

commented on e381722 Feb 16, 2018

@mikaelarguedas some more topics being used by ROS2

Some of them I found surprising to be a reader topic and not a writer e.g. set_parametersReply. This is at least true for a subscriber in ROS2. Have tried the variations with a publisher yet. I went ahead and pushed all the ROS2 events in both the publisher and subscriber topic list for the time being. The writer and reader list below is w.r.t ROS2 subscriber. Might be the same for publisher.

Writer

  • parameter_events

Reader

  • clock
  • get_parametersReply
  • get_parametersRequest
  • get_parameter_typesReply
  • get_parameter_typesRequest
  • list_parametersReply
  • list_parametersRequest
  • describe_parametersReply
  • describe_parametersRequest
  • set_parametersReply
  • set_parametersRequest

This comment has been minimized.

Copy link
Contributor Author

replied Feb 16, 2018

Thanks for the suggested improvement.
Unfortunately these topics are services and they vary from one DDS implementation to the next as some implement services as described in the DDS-RPC spec and some just on top of pub/sub. So I'd prefer to stick with only ROS topics and leave the ROS services out for now (keeping the comment e381722#diff-164a61093789ed771b5f88b17b5e4468R311)

# DCPS* is necessary for builtin data readers
permission_str += """\
<subscribe>
<topics>
<topic>DCPS*</topic>

This comment has been minimized.

Copy link
@ruffsl

ruffsl Feb 21, 2018

Member

I forget, what was the purpose for this rule again?
ROS2 is using the DCPS API, or for what?

This comment has been minimized.

Copy link
@mikaelarguedas

mikaelarguedas Feb 21, 2018

Author Contributor

Connext creates DCPS topics for builtin endpoints (there's a comment above).
Note that this part is not modified in this PR, just reindented as it was improperly indented before

This comment has been minimized.

Copy link
@ruffsl

ruffsl Feb 21, 2018

Member

I don't have that permission in my minimal working policy for talker listener demo, and it seems to messaging fine. Is there a way I can test ROS2 to fail when this permission is revoked? E.g. do I need test my use case across two different networked machines to change the endpoints execution path to incoundter an issue, or are these topics optional or silently failing under the API?

This comment has been minimized.

Copy link
@mikaelarguedas

mikaelarguedas Feb 21, 2018

Author Contributor

Thanks for pointing it out, my guess is that this behavior has changed in Connext 5.3.0. It was definitely necessary in 5.2.4.
As this is unrelated to this PR and will likely not be backported to ardent, let's discuss is on another ticket #34

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

@ros2/team I wont run CI on this as there is no test using this generator for access control.
I'd appreciate if someone else with connext installed can give it a try.

Steps:

  • build sros2 with this branch in a workspace
  • source the workspace
  • generate artifacts
mkdir /tmp/sros2_demo && cd /tmp/sros2_demo
ros2 security create_keystore demo_keys
ros2 security create_key demo_keys talker
ros2 security create_key demo_keys listener
wget https://raw.githubusercontent.com/ros2/sros2/release-latest/examples/sample_policy.yaml
ros2 security create_permission demo_keys talker sample_policy.yaml
ros2 security create_permission demo_keys listener sample_policy.yaml
  • set up environment (do this in two terminals)
export ROS_SECURITY_ROOT_DIRECTORY=/tmp/sros2_demo/demo_keys
export ROS_SECURITY_ENABLE=true
export ROS_SECURITY_STRATEGY=Enforce
export RMW_IMPLEMENTATION=rmw_connext_cpp
  • run the demo
    Terminal1:
    ros2 run demo_nodes_cpp talker
    Terminal2:
    ros2 run demo_nodes_cpp listener

@mikaelarguedas mikaelarguedas self-assigned this Feb 22, 2018

@ruffsl

ruffsl approved these changes Feb 22, 2018

Copy link
Member

left a comment

Just tried your test case with my Connext 5.3 installation, and all works well.

@mikaelarguedas mikaelarguedas requested a review from dhood Feb 26, 2018

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

Added @dhood as a reviewer as it was the github suggestion. If you don't have the bandwidth to test this let me know

@dhood

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

yep, I will review it

@dhood dhood referenced this pull request Feb 28, 2018

Merged

updates to #33 #36

dhood added some commits Feb 28, 2018

@dhood

dhood approved these changes Feb 28, 2018

@mikaelarguedas mikaelarguedas merged commit 59a1104 into master Mar 1, 2018

@mikaelarguedas mikaelarguedas deleted the add_clock_and_paramevent_default branch Mar 1, 2018

mikaelarguedas added a commit that referenced this pull request Mar 1, 2018

Fix access control for ardent (#33)
* set permissions for clock and parameter_events topics by default

* add the rules only if a set of permissions is passed

* parameter_event need pub and sub permissions

* [hack] add special rules to handle default parameter topics

* Add comment why blank partition is needed

* Remove parameter_events from policy because will be overwritten
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.