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

Should services servers require both publish & subscribe access to own Request/Reply? #63

Closed
ruffsl opened this issue Aug 15, 2018 · 9 comments
Closed

Should services servers require both publish & subscribe access to own Request/Reply? #63

ruffsl opened this issue Aug 15, 2018 · 9 comments
Labels
more-information-needed Further information is required question Further information is requested

Comments

@ruffsl
Copy link
Member

ruffsl commented Aug 15, 2018 •
edited

While developing on keymint, I attempted to use the same semantic access control profile that I had successfully used in ROS2 Ardent, with the addition of some new subsystem services such as set_parameters_atomically. However, this proved insufficient, and through closer inspection using RTI Connext Administration Console, I realised a new change in where ROS2 Bouncy nodes appear to be both publishing and subscribing to all of their own service level DDS topics:

image

IMO, this seem unwarranted given that a service server should not also require client level access, doing so would unecessarly widen the scope of the subject's (service host) access control policy and make it furtherly difficult to enforce asymmetric API access. If there is a good reason why this elevated access requirement for ROS service host has been introduced into Bouncy but was absent from Ardent, I'd be curious to know.

Required Info:

  • Operating System:
    • Ubuntu 18.04
  • Installation type:
    • binaries
  • Version or commit hash:
    • ros-bouncy-sros2/bionic,now 0.5.0-0bionic.20180720.000732 amd64 [installed]
    • rti-connext-dds-5.3.1/bionic,bionic,now 5.3.1-nc.x64Linux3gcc5.4.0+2 amd64 [installed,automatic]
    • ros-bouncy-fastrtps/bionic,now 1.6.0-5bionic.20180719.223049 amd64 [installed]
  • DDS implementation:
    • rmw_connext_cpp
    • rmw_fastrtps_cpp
  • Client library (if applicable):
    • rclcpp

Steps to reproduce issue

Using the following symantec policy profile, I'd expect the granting the /talker node serving the service /talker/describe_parameters with execute permission would sufficient, i.e. permission to subscribe to Request and publish to Reply; as opposed to call permission, i.e. permission to publish to Request and subscribe to Reply.

Expected comarmor profile.xml 
<?xml version="1.0" encoding="UTF-8"?>

<profiles xmlns:xi="http://www.w3.org/2001/XInclude">
    <profile name="My Talker Profile">
        <attachment>/talker</attachment>

        <ros_topic qualifier="ALLOW">
            <attachments>
                <attachment>/chatter</attachment>
            </attachments>
            <permissions>
                <ros_publish/>
            </permissions>
        </ros_topic>

        <ros_topic qualifier="ALLOW">
            <attachments>
                <attachment>/parameter_events</attachment>
            </attachments>
            <permissions>
                <ros_publish/>
                <ros_subscribe/>
            </permissions>
        </ros_topic>

        <ros_topic qualifier="ALLOW">
            <attachments>
                <attachment>/clock</attachment>
            </attachments>
            <permissions>
                <ros_subscribe/>
            </permissions>
        </ros_topic>

        <ros_service qualifier="ALLOW">
            <attachments>
                <attachment>/talker/describe_parameters</attachment>
                <attachment>/talker/get_parameter_types</attachment>
                <attachment>/talker/get_parameters</attachment>
                <attachment>/talker/list_parameters</attachment>
                <attachment>/talker/set_parameters</attachment>
                <attachment>/talker/set_parameters_atomically</attachment>
            </attachments>
            <permissions>
                <ros_execute/>
            </permissions>
        </ros_service>
    </profile>

    <profile name="My Listener Profile">
        <attachment>/listener</attachment>

        <ros_topic qualifier="ALLOW">
            <attachments>
                <attachment>/chatter</attachment>
            </attachments>
            <permissions>
                <ros_subscribe/>
            </permissions>
        </ros_topic>

        <ros_topic qualifier="ALLOW">
            <attachments>
                <attachment>/parameter_events</attachment>
            </attachments>
            <permissions>
                <ros_publish/>
                <ros_subscribe/>
            </permissions>
        </ros_topic>

        <ros_topic qualifier="ALLOW">
            <attachments>
                <attachment>/clock</attachment>
            </attachments>
            <permissions>
                <ros_subscribe/>
            </permissions>
        </ros_topic>

        <ros_service qualifier="ALLOW">
            <attachments>
                <attachment>/listener/describe_parameters</attachment>
                <attachment>/listener/get_parameter_types</attachment>
                <attachment>/listener/get_parameters</attachment>
                <attachment>/listener/list_parameters</attachment>
                <attachment>/listener/set_parameters</attachment>
                <attachment>/listener/set_parameters_atomically</attachment>
            </attachments>
            <permissions>
                <ros_execute/>
            </permissions>
        </ros_service>
    </profile>
</profiles>

See here for more of an detailed example: https://github.com/ruffsl/ros2_docker_demos/tree/master/sros2

Expected behavior

The following permission.xml is the expected minimally permission needed (using the comarmor profile from above), where publish access is granted to Reply topics and subscribe access is given to Request topics for a given service server.

Expected minimal permissions.xml 
<?xml version="1.0" encoding="utf-8"?>
<dds>
  <permissions>
    <grant name="talker">
      <subject_name>CN=talker</subject_name>
      <validity>
        <not_before>2013-10-26T00:00:00</not_before>
        <not_after>2023-10-26T22:45:30</not_after>
      </validity>
      <allow_rule>
        <domains>
          <id>0</id>
        </domains>
        <publish>
          <topics>
            <topic>rt/chatter</topic>
            <topic>rt/parameter_events</topic>
            <topic>rr/talker/describe_parametersReply</topic>
            <topic>rr/talker/get_parameter_typesReply</topic>
            <topic>rr/talker/get_parametersReply</topic>
            <topic>rr/talker/list_parametersReply</topic>
            <topic>rr/talker/set_parametersReply</topic>
            <topic>rr/talker/set_parameters_atomicallyReply</topic>
          </topics>
        </publish>
        <subscribe>
          <topics>
            <topic>rt/parameter_events</topic>
            <topic>rt/clock</topic>
            <topic>rq/talker/describe_parametersRequest</topic>
            <topic>rq/talker/get_parameter_typesRequest</topic>
            <topic>rq/talker/get_parametersRequest</topic>
            <topic>rq/talker/list_parametersRequest</topic>
            <topic>rq/talker/set_parametersRequest</topic>
            <topic>rq/talker/set_parameters_atomicallyRequest</topic>
          </topics>
        </subscribe>
      </allow_rule>
      <default>DENY</default>
    </grant>
  </permissions>
</dds>

Actual behavior

The following permission.xml presently works when service Request/Reply topics are all given both publish and subscribe access.

Presently needed permissions.xml 
<?xml version="1.0" encoding="utf-8"?>
<dds>
  <permissions>
    <grant name="talker">
      <subject_name>CN=talker</subject_name>
      <validity>
        <not_before>2013-10-26T00:00:00</not_before>
        <not_after>2023-10-26T22:45:30</not_after>
      </validity>
      <allow_rule>
        <domains>
          <id>0</id>
        </domains>
        <publish>
          <topics>
            <topic>rt/chatter</topic>
            <topic>rt/parameter_events</topic>
            <topic>rq/talker/describe_parametersRequest</topic>
            <topic>rq/talker/get_parameter_typesRequest</topic>
            <topic>rq/talker/get_parametersRequest</topic>
            <topic>rq/talker/list_parametersRequest</topic>
            <topic>rq/talker/set_parametersRequest</topic>
            <topic>rq/talker/set_parameters_atomicallyRequest</topic>
            <topic>rr/talker/describe_parametersReply</topic>
            <topic>rr/talker/get_parameter_typesReply</topic>
            <topic>rr/talker/get_parametersReply</topic>
            <topic>rr/talker/list_parametersReply</topic>
            <topic>rr/talker/set_parametersReply</topic>
            <topic>rr/talker/set_parameters_atomicallyReply</topic>
          </topics>
        </publish>
        <subscribe>
          <topics>
            <topic>rt/parameter_events</topic>
            <topic>rt/clock</topic>
            <topic>rr/talker/describe_parametersReply</topic>
            <topic>rr/talker/get_parameter_typesReply</topic>
            <topic>rr/talker/get_parametersReply</topic>
            <topic>rr/talker/list_parametersReply</topic>
            <topic>rr/talker/set_parametersReply</topic>
            <topic>rr/talker/set_parameters_atomicallyReply</topic>
            <topic>rq/talker/describe_parametersRequest</topic>
            <topic>rq/talker/get_parameter_typesRequest</topic>
            <topic>rq/talker/get_parametersRequest</topic>
            <topic>rq/talker/list_parametersRequest</topic>
            <topic>rq/talker/set_parametersRequest</topic>
            <topic>rq/talker/set_parameters_atomicallyRequest</topic>
          </topics>
        </subscribe>
      </allow_rule>
      <default>DENY</default>
    </grant>
  </permissions>
</dds>

Additional information

The DDS implementation rmw_fastrtps_cpp still appears to fail at runtime startup when using the same security artifacts that otherwise work fine with rmw_connext_cpp:

Update I tested this again in a container, and seems to be working like rmw_connext_cpp, so perhaps I've messed with rmw_fastrtps_cpp on my host install.

$ export RMW_IMPLEMENTATION=rmw_fastrtps_cpp
$ ros2 run demo_nodes_cpp talker
[SECURITY Error] Error validating the local participant identity. () -> Function init
[RTPS_PARTICIPANT Error] Cannot create participant due to security initialization error -> Function createParticipant
[PARTICIPANT Error] Problem creating RTPSParticipant -> Function createParticipant
terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  failed to initialize rcl node: create_node() could not create participant, at /tmp/binarydeb/ros-bouncy-rmw-fastrtps-cpp-0.5.1/src/rmw_node.cpp:91, at /tmp/binarydeb/ros-bouncy-rcl-0.5.1/src/rcl/node.c:336
@mikaelarguedas
Copy link
Member

A service Server needs server access only, a service client needs client access only.

Since Bouncy all nodes start a service server for parameters ( in Ardent only a service client was automatically started).
What you seem to be reporting in this issue is the fact that we "still" create automatically a client in Bouncy. Is that correct?

Some links relevant to starting parameter service automatically in Bouncy ros2/rclcpp#478 and #52.

@mikaelarguedas mikaelarguedas added the more-information-needed Further information is required label Aug 16, 2018
@ruffsl
Copy link
Member Author

ruffsl commented Aug 16, 2018 •
edited

A service Server needs server access only, a service client needs client access only.

here, here!

Since Bouncy all nodes start a service server for parameters ( in Ardent only a service client was automatically started).

Ah, looking closely back at Ardent, I was a little perplexed why only client access was needed before. That explances it.

What you seem to be reporting in this issue is the fact that we "still" create automatically a client in Bouncy. Is that correct?

Correct.

@mikaelarguedas
Copy link
Member

What you seem to be reporting in this issue is the fact that we "still" create automatically a client in Bouncy. Is that correct?

Correct.

Ok, so the reason we create clients automatically since Ardent is because we introduced the notion of clock in c++ nodes.
In order to leverage different types of clocks (e.g. the simulated clock), ROS uses the use_sim_time parameter, so each node needs to interact with the use_sim_time parameter (I believe in Ardent it was only done to filter the /parameter_events topic but there is work underway to expand the notion of ROS Time throughout the stack ros2/ros2#530).

@ruffsl
Copy link
Member Author

ruffsl commented Aug 17, 2018

In order to leverage different types of clocks (e.g. the simulated clock), ROS uses the use_sim_time parameter, so each node needs to interact with the use_sim_time parameter (I believe in Ardent it was only done to filter the /parameter_events topic

Ah, I see such is the case here:

https://github.com/ros2/rclcpp/blob/ec17d68b41146f23b3715928814018eeeb5d7769/rclcpp/src/rclcpp/time_source.cpp#L215-L242

I played around in rclcpp and found that cutting out parameter_client_ from TimeSource::attachNode avoids the need for client level permissions for local parameter services.

https://github.com/ros2/rclcpp/blob/ec17d68b41146f23b3715928814018eeeb5d7769/rclcpp/src/rclcpp/time_source.cpp#L90-L95

@mikaelarguedas
Copy link
Member

Yeah I had the same feeling when I wrote the previous answer. That is what prompted me to point to the ongoing Time related changes as removing the client may impact it.

If the ongoing rclcpp time changes don't require a client, I would recommend removing the default client completely as well as leveraging the server to react to "use_sim_time" param value change, rather than subscribing to and filtering the messages published to the parameter_events topic.

@ruffsl
Copy link
Member Author

ruffsl commented Aug 18, 2018

For reference WRT security of the parameter_events topic:
Related: ros2/ros2#432 (comment)

@mikaelarguedas mikaelarguedas added the question Further information is requested label Aug 18, 2018
@mikaelarguedas
Copy link
Member

I believe the original question has been answered so I'm going to close this.

I opened ros2/rclcpp#537 to track the current use of the parameter client in rclcpp, and ros2/ros2#432 is still opened to discuss the more generic use of parameter related topics and services

@ros-discourse
Copy link

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/security-ramifications-of-global-parameter-events/15924/4

@ros-discourse
Copy link

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/security-ramifications-of-global-parameter-events/15924/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
more-information-needed Further information is required question Further information is requested
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ruffsl @mikaelarguedas @ros-discourse