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

Expand ROS Specific Namespace Prefix for Actions #203

Closed
wants to merge 1 commit into from

Conversation

@ruffsl
Copy link
Member

commented Nov 29, 2018

To better secure and segment access control of the different ROS subsystems, specifically to avoid the mapping of ROS2 actions to DDS topics from colliding with those of ROS2 topics and services, actions are to be allocated their own prefix to facilitate simpler policy profile permissions. Additionally, this and helps to prevent crossover of information flow in the case of more general permission prefix expressions.
Contex: #193 (comment)

Expand ROS Specific Namespace Prefix for Actions
To better secure and segment access control of the different ROS subsystems, specifically to avoid the mapping of ROS2 actions to DDS topics from colliding with those of ROS2 topics and services, actions are to be allocated their own prefix to facilitate simpler policy profile permissions. Additionally, this and helps to prevent crossover of information flow in the case of more general permission prefix expressions.  
Contex: #193 (comment)

@ruffsl ruffsl added the in progress label Nov 29, 2018

@ruffsl ruffsl requested a review from sloretz Nov 29, 2018

@ruffsl ruffsl added in review and removed in progress labels Nov 29, 2018

@ruffsl ruffsl referenced this pull request Nov 29, 2018
@sloretz
Copy link
Contributor

left a comment

Ok, would the _action token still then need to be embedded in DDS topic mapped by action namespace? We already have hardcoded postfix tokens in the DDS topics mapped by ROS2 services; e.g Request, Reply. I'd rather we drop _action in the rmw, rather than having to interlace this _action token string into the rest of the mappings, and then have to moderate that translation in the access control permission provisioning :<

If that makes it easier, I recommend adding a paragraph here saying the /_action/ token will be dropped in favor of a topic or service prefix.

@clalancette clalancette requested a review from Karsten1987 Nov 29, 2018

@wjwwood

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

@ruffsl Your comment link doesn't work for me, just loads the page and doesn't go to the comment.

Perhaps it's worth summarizing the issue here, for example what's wrong with _action being in the topic/service names?

This change (or actually enforcing the ra/ prefix) will require changes in rmw as it currently has no idea what an action is and that's intentional.

At the time, I thought ra would make sense, now I'm not sure it necessarily makes sense to treat topics and services that are part of an action differently from a normal topic/service.

@ruffsl

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@ruffsl Your comment link doesn't work for me, just loads the page and doesn't go to the comment.

I checked the link on my browser, and it scrolled down to the comment; it's a bit slow to load as the PR thread is so long. The review comment thread started with @gbiggs comment on Oct 27, 2018, 11:02 AM PDT #193 (comment)

Perhaps it's worth summarizing the issue here, for example what's wrong with _action being in the topic/service names?

The gist is that if we want to provide user the ability to create access control permissions that are beholden to ROS2 actions, then the manner in which ROS2 actions are mapped at the middleware layer greatly influences the fedelty as which permission expression are matched with the ROS2 subsystems governed.

For example, the sharing of the rt namespace prefix for both ROS2 topics and action topics, compounded with the insertion of _action token in the FQN would not prevent intersections or crossover of user generated permissions when attempting to provision the same grant with seemly unrelated ROS2 capabilities. To help illustrate an example:

Take this highlevel comarmor profile defining the minimal permissions for ROS 2 action client server pair:

<?xml version="1.0" encoding="UTF-8"?>
<profiles xmlns:xi="http://www.w3.org/2001/XInclude">
    <profile name="My Talker Profile">
        <attachments>
            <attachment>/spam/fibonacci_client</attachment>
        </attachments>
        <ros_action qualifier="ALLOW">
            <attachments>
                <attachment>/foo/bar/fibonacci</attachment>
            </attachments>
            <permissions>
                <ros_call/>
            </permissions>
        </ros_action>
    </profile>
    <profile name="My Listener Profile">
        <attachments>
            <attachment>/foo/bar/fibonacci_server</attachment>
        </attachments>
        <ros_action qualifier="ALLOW">
            <attachments>
                <attachment>/foo/bar/fibonacci</attachment>
            </attachments>
            <permissions>
                <ros_execute/>
            </permissions>
        </ros_action>
    </profile>
</profiles>

This intermediate representation would then be compiled down one-one to the middleware permissions:

<?xml version="1.0" encoding="utf-8"?>
<dds>
  <permissions>
    <grant name="fibonacci_client">
      <subject_name>CN=/spam/fibonacci_client</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>
        <subscribe>
          <topics>
            <!-- subscribe to action topics  -->
            <topic>rt/foo/bar/fibonacci/_action/feedback</topic>
            <topic>rt/foo/bar/fibonacci/_action/status</topic>
            <!-- subscribe to action replys  -->
            <topic>rr/foo/bar/fibonacci/_action/cancelReply</topic>
            <topic>rr/foo/bar/fibonacci/_action/get_resultReply</topic>
            <topic>rr/foo/bar/fibonacci/_action/send_goalReply</topic>
          </topics>
        </subscribe>
        <publish>
          <topics>
            <!-- publish to action requests  -->
            <topic>rq/foo/bar/fibonacci/_action/cancelRequest</topic>
            <topic>rq/foo/bar/fibonacci/_action/get_resultRequest</topic>
            <topic>rq/foo/bar/fibonacci/_action/send_goalRequest</topic>
          </topics>
        </publish>
      </allow_rule>
      <default>DENY</default>
    </grant>
    <grant name="fibonacci_server">
      <subject_name>CN=/foo/bar/fibonacci_server</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>
            <!-- publish to action topics  -->
            <topic>rt/foo/bar/fibonacci/_action/feedback</topic>
            <topic>rt/foo/bar/fibonacci/_action/status</topic>
            <!-- publish to action replys  -->
            <topic>rr/foo/bar/fibonacci/_action/cancelReply</topic>
            <topic>rr/foo/bar/fibonacci/_action/get_resultReply</topic>
            <topic>rr/foo/bar/fibonacci/_action/send_goalReply</topic>
          </topics>
        </publish>
        <subscribe>
          <topics>
            <!-- subscribe to action requests  -->
            <topic>rq/foo/bar/fibonacci/_action/cancelRequest</topic>
            <topic>rq/foo/bar/fibonacci/_action/get_resultRequest</topic>
            <topic>rq/foo/bar/fibonacci/_action/send_goalRequest</topic>
          </topics>
        </subscribe>
      </allow_rule>
      <default>DENY</default>
    </grant>
  </permissions>
</dds>

However should the user wish to use permission expression to wildcard all action under the allocated /foo/bar namespace, then the _action offers little purpose, given the generated action permission will then subsequently equate to allowing all topics under the /foo/bar prefix:

<?xml version="1.0" encoding="utf-8"?>
<dds>
  <permissions>
    <grant name="fibonacci_client">
      <subject_name>CN=/spam/fibonacci_client</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>
        <subscribe>
          <topics>
            <!-- subscribe to action topics  -->
            <!-- subscribe to action replys  -->
            <topic>rt/foo/bar/*</topic>
          </topics>
        </subscribe>
        <publish>
          <topics>
            <!-- publish to action requests  -->
            <topic>rq/foo/bar/*</topic>
          </topics>
        </publish>
      </allow_rule>
      <default>DENY</default>
    </grant>
    <grant name="fibonacci_server">
      <subject_name>CN=/foo/bar/fibonacci_server</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>
            <!-- publish to action topics  -->
            <!-- publish to action replys  -->
            <topic>rt/foo/bar/*</topic>
          </topics>
        </publish>
        <subscribe>
          <topics>
            <!-- subscribe to action requests  -->
            <topic>rq/foo/bar/*</topic>
          </topics>
        </subscribe>
      </allow_rule>
      <default>DENY</default>
    </grant>
  </permissions>
</dds>

Granted the inclusion or omission for the _action token in the middleware mapping in the case above does little in the light of using a trailing * wildcard, yet other valid POSIX fnmatch strings, the expression expansion used by default in Secure DDS, could certainly conjure up more conercases or unanticipated conflicts.

This change (or actually enforcing the ra/ prefix) will require changes in rmw as it currently has no idea what an action is and that's intentional.

I think that would be better than having to rely on a hard coded _action token in the FQN, either to help hide these meta topics/services, given it would be redundant if the role of the channel as a action could be also inferred from the ra prefix.

At the time, I thought ra would make sense, now I'm not sure it necessarily makes sense to treat topics and services that are part of an action differently from a normal topic/service.

Specifically, I'd like to ensure we are sufficiently partitioning the fundamental objects to be protected, so I would argue it is entirely justified to treat topics and services that are part of an action differently from a normal topic/service. In fact, I'd like to do the same for the services that are used by parameters (I could make another PR to address this as well). To reference JH Saltzer et al, "The Protection of Information in Computer Systems", 1975:

"2) The Essentials of Information Protection: For purposes of discussing protection, the information stored in a computer system is not a single object. When one is considering direct access, the information is divided into mutually exclusive partitions, as specified by its various creators. Each partition contains a collection of information, all of which is intended to be protected uniformly. The uniformity of protection is the same kind of uniformity that applies to all of the diamonds stored in the same vault: any person who has a copy of the combination can obtain any of the diamonds. Thus the collections of information in the partitions are the fundamental objects to be protected.
Conceptually, then, it is necessary to build an impenetrable wall around each distinct object that warrants separate protection, construct a door in the wall through which access can be obtained, and post a guard at the door to control its use."

I think keeping the action namespace unmared from tokens and colliding subsystems would help simplify policy equivalency between higher level intermediate representations of ROS2 permission; especially in regards to the access control model checker I'm working to help attest or refute validity proofs wrt the information flow control of ROS 2 computation graphs.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

I think that would be better than having to rely on a hard coded _action token in the FQN, either to help hide these meta topics/services, given it would be redundant if the role of the channel as a action could be also inferred from the ra prefix.

I understand your interest in using the ra prefix, but I strongly disagree that introducing the knowledge of actions into rmw is ok. At best we'd have the calling code (i.e. rcl) set the prefix explicitly for topics and services, so the ra* prefixes could be used.


I assume you cannot change rt/foo/bar/* to be something like rt/foo/bar.*/_action/+* (or similar)? It seems like a limitation of the governance (is that the right term?) file if you cannot. The only reason ra is preferable is that it makes for a slightly simpler glob. That's not a very compelling reason to violate encapsulation (putting action specifics into rmw).

On balance, allowing for a custom prefix in rmw_create_publisher() et al., seems reasonable. Though that is not as simple as it sounds, because you still need rmw to distinguish between DDS and non-DDS topics, and it would have no knowledge of custom prefixes, so every node would need to inform rmw of the possible additional prefixes as well as passing them for specific topics and services that should use them.

Ultimately, it should be possible in the future to add a new concept like actions or parameters which builds on the primitives of rmw, without any modifications what so ever to rmw itself or its various implementations.

@ruffsl

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

I assume you cannot change rt/foo/bar/* to be something like rt/foo/bar./_action/+ (or similar)? It seems like a limitation of the governance (is that the right term) file if you cannot. The only reason ra is preferable is that it makes for a slightly simpler glob. That's not a very compelling reason to violate encapsulation (putting action specifics into rmw).

One could introduce more advance interlacing expressions, but then this would not only obscure the permission mapping across the set of all valid topic strings from the administrator (human readability), and also cripple the use of many formal verification techniques (model based checking) when auditing policy for correctness. In general, termination proofs for arbitrary expression expansion is difficult, so the more more complex you make the expression in the permission structures, the harder it may be to solve the underlying propositional satisfiability problem.

I think ensuring ROS2 connectivity behaviour remains readily auditable using computer security methods is important in ensuring the maturity and soundness of the systems that will be built upon our abstractions. I'd argue it's worthwhile we do this at the design level to cater to formal static analysis of security mechanisms used in ROS2, as many other engineering domains have: e.g. Verified iptables Firewall Analysis and Verification.

On balance, allowing for a custom prefix in rmw_create_publisher() et al., seems reasonable. Though that is not as simple as it sounds, because you still need rmw to distinguish between DDS and non-DDS topics, and it would have no knowledge of custom prefixes, so every node would need to inform rmw of the possible additional prefixes as well as passing them for specific topics and services that should use them.

Forgive my ignorance; how are topics and services distinguished presently if they aren't providing custom prefix in rmw_create_publisher() et al? Wouldn't it be reasonable to expect actions to mimic the same pattern? I would think if actions and parameters are truly meant to be a first class primitive in ROS2, then they should be given the same degree of independent access control from the other subsystems.

Ultimately, it should be possible in the future to add a new concept like actions or parameters which builds on the primitives of rmw, without any modifications what so ever to rmw itself or its various implementations.

That does sound awesome, and I agree! My only caution would be that it be made to include an orthogonal handle for the Policy Decision Point, something to strongly label the type (topic/service/action/parameter/etc) and origin (FQN/URI/robot/swarm/etc) of the data channel, and sufficiently partition the fundamental objects to be protected. Thus my stated affinity for annotating the interfaces using something like DDS data tags, as I mentioned in my prior comments linked here:#193 (comment) .

Perhaps one aspect of the problem now is that we are overloading origin of the data channel to also include aspects about its type.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

One could introduce more advance interlacing expressions, but then this would not only obscure the permission mapping across the set of all valid topic strings from the administrator (human readability), and also cripple the use of many formal verification techniques (model based checking) when auditing policy for correctness. In general, termination proofs for arbitrary expression expansion is difficult, so the more more complex you make the expression in the permission structures, the harder it may be to solve the underlying propositional satisfiability problem.

I'm not talking about anything categorically different from what is currently supported, i.e. /foo/bar/* and /foo/bar/*/_action/* are both relatively simple globs.

I think your concerns about complexity and readability are valid, but I don't think the suggestion of support more than "suffix only globing" is unreasonable or too complicated.

Also, all of this only matters if you want to "allow all topics and services that are part of an action within a namespace". That is a super complicated expression, and if readability and auditability are your main concerns then I'd argue that the specifications should not support globing at all and instead should require the user to specify exactly which actions they want to allow. In that case, there is no benefit for ra vs /_action/.

Forgive my ignorance; how are topics and services distinguished presently if they aren't providing custom prefix in rmw_create_publisher() et al?

Topics are always assigned rt and services are either assigned rs or rr/rq depending on the implementation.

Wouldn't it be reasonable to expect actions to mimic the same pattern?

Perhaps, but the reality is that they do not, and that is because actions and parameters only exist as concepts in rcl and above. They are intentionally left out of rmw to make it simpler to create new rmw implementations.

I would think if actions and parameters are truly meant to be first class primitive in ROS2, then they should be given the same degree of independent access control from the other substems.

I believe it's incorrect to equate "is a concept in rmw" with "is a first class citizen in ROS". One simply does not imply the other.

My only caution would be that it be made to include an orthogonal handle for the Policy Decision Point, something to strongly label the type (topic/service/action/etc) and origin (namespace/URI/etc) of the data channel, and sufficiently partition the fundamental objects to be protected. Thus my stated affinity for annotating the interfaces using something like DDS data tags, as I mentioned in my prior comments linked here:#193 (comment) .

This is the crux of the issue for me, your argument, as I understand it, is that putting all topics and services related to an action in /_action/ somehow less completely partitions them from normal topics and services than using the ra prefix, but I simply don't find that to be the case.

It appears to me that the only argument against only using /_action/ is that it's harder to express with wildcard matching (or whatever you'd like to call it: globing, etc.).

However, that reason by itself is not enough to convince me personally that we need to change how we discriminate actions from normal topics and services and in the process complicate the rmw API or even worse introduce additional primitives to the rmw API.


I'm sorry to disagree with you, but I'm not convinced the argument so far.

But going along with what I said before, if it is technically impossible to improve the wildcard matching and dropping wildcard matching is unreasonable, then I would be ok with changing rmw so that you can specify the prefix to be used when creating a publisher/subscription or service client/server.

But as I also said above, its more complicated that it sounds, because you need to handle things like:

  • how does rmw_get_topic_names_and_types discriminate between action topics and dds topics?
    • do you have to pass all possible prefixes to rmw_init?
    • should we change the prefix to be r/{t,s,a}/... so that everything that starts with r/ is a "ROS thing"?
  • does ros2 topic list ever include the action topics, i.e. do they show up with a --all or --include-hidden option?
  • can this pattern be reused to do the same thing with parameters?
    • what about with an as yet undescribed new concept which builds on the rmw primitives?

And that's just what I can think of off the top of my head, so from my perspective it's everything but simple.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

I would think if actions and parameters are truly meant to be first class primitive in ROS2, then they should be given the same degree of independent access control from the other substems.

I believe it's incorrect to equate "is a concept in rmw" with "is a first class citizen in ROS". One simply does not imply the other.

And just to clarify here, I am not opposed to giving actions "the same degree of independent access control from the other substems" -- I just take issue with the technical proposal for how to accomplish it. I still think either making the globing slightly more powerful or having rcl pass extra information into rmw are valid solutions, though I would personally prefer to see the former because it requires no changes to rmw and would probably be useful in other scenarios (e.g. allow /foo/bar/*/image_left/*).

@ruffsl

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

I'm not talking about anything categorically different from what is currently supported, i.e. /foo/bar/* and /foo/bar//_action/ are both relatively simple globs.
I think your concerns about complexity and readability are valid, but I don't think the suggestion of support more than "suffix only globing" is unreasonable or too complicated.

The control flow for both may be simple to forward evaluate, but they are not equivalent in complexity wrt to reverse inference. /foo/bar/* is a simple case of prefix matching or simple boolean mask, while /foo/bar/*/_action/* is more a general case of approximate string matching, best approached via dynamic programming. For g grants and p permissions per grant comprised of fixed strings or prefix patterns, asking a SAT solver to prove or refute the feasible intersection of such sets is fast and sound.

Doing the same for fuzzy string searching is much more involved and no longer tractable at the scale of ROS graphs found on a turtlebot, say 💯 topics; then think of the rqt graph I showed you on the one HSR at TRI that froze SVG viewer, like 💯e2 topics.

Also, all of this only matters if you want to "allow all topics and services that are part of an action within a namespace". That is a super complicated expression, and if readability and auditability are your main concerns ...

I think that's confounding the static logic of the Policy Enforcement Point (PEP) that is being used to intercept a subjects access request to a resource, and the configurable Policy Decision Point (PDP) which evaluates access requests against authorization policies before issuing access decisions. If the subsystems are independently addressable by the PEP, then there is no need for a 'super complicated expression' to be given to the PDP.

In the default Secure DDS plugin, the PEP is achieved by simply iterating over the structured DOM of the permissions.xml file with a static finite state machine, i.e. like an traditional ordered rule based filter. The PDP is basically a function call to the POSIX fnmatch(pattern, string). If the subsystems are not independently addressable then, yes you'd have to write a cryptic pattern to arbitrate the difference between namespaces of services vs actions as an ugly pattern.

... then I'd argue that the specifications should not support globing at all and instead should require the user to specify exactly which actions they want to allow. In that case, there is no benefit for ra vs /_action/.

Pattern expressions could be safely used as long as integrity of the input is well forme, and may even be required as fixed string matching may not always suffice in balancing flexibility with security.

I believe it's incorrect to equate "is a concept in rmw" with "is a first class citizen in ROS". One simply does not imply the other.

But when would it be appropriate to access control one subsystems but not the other? Having to tiptoe with one subsystem to guarantee security of the other seem seem like a gamble. We could reuse topic and serves functions to define heyer level interfaces, but let's not forgo separation of privilege.

This is the crux of the issue for me, your argument, as I understand it, is that putting all topics and services related to an action in /_action/ somehow less completely partitions them from normal topics and services than using the ra prefix, but I simply don't find that to be the case

How would one provision a transparency data logger to record all normal topics but no actions that may be session confidential, or permit a action server to host only it own action namespace yet be denied from using any normal topic? This would be mutually exclusive under a unioned rt prefix.

It appears to me that the only argument against only using /_action/ is that it's harder to express with wildcard matching (or whatever you'd like to call it: globing, etc.).

Nay, it impedes static analysis of permissions using expressions which is important for general security. The making it harder to write correct and sound expressions mainly ebbs usability.

However, that reason by itself is not enough to convince me personally that we need to change how we discriminate actions from normal topics and services and in the process complicate the rmw API or even worse introduce additional primitives to the rmw API.

I'll concede on the _action; perhaps we can patch and work around its insertion from security tooling, but I would insist we still need some way of allowing the PEP to arbitrate between subsystems type without relying on the rcl FQN. It doesn't have to be what was first proposed here, but it should ensure no back channels or crosstalk are possible. I'm totally open to alternatives; I just want to avoid any security holes like what happened in ROS2 Ardent, i.e exploiting the required empty partition permissions to access across DDS topics, and so ROS 2 topics.

And that's just what I can think of off the top of my head, so from my perspective it's everything but simple.

Thanks for being patient with me @wjwwood , I appreciate gaining a better grasp on the severity of changes that such an extension as this would inevitably require throughout the codebase.

@Karsten1987 Karsten1987 removed their request for review Jan 15, 2019

@sloretz

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

I'm not sure where the discussion ended.

I'll concede on the _action; perhaps we can patch and work around its insertion from security tooling, but I would insist we still need some way of allowing the PEP to arbitrate between subsystems type without relying on the rcl FQN. It doesn't have to be what was first proposed here, but it should ensure no back channels or crosstalk are possible

@ruffsl Is sounds like you might not be interested in adding a topic prefix to the rmw api anymore. Does this mean this PR can be closed, or will it be updated at some point with an alternative?

@ruffsl

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@ruffsl Is sounds like you might not be interested in adding a topic prefix to the rmw api anymore. Does this mean this PR can be closed, or will it be updated at some point with an alternative?

I'd still prefer to have a means to differentiate between ROS2 subsystems types for security policy guarantees, but atm there doesn't seem to be a reliable manner for doing that with the present namespacing. As I currently lack the in-depth rmw knowledge to recommend an attractive alternative (as well as the time to dive deeper alone, as a full-time student) I'm not sure I'd have more to offer given my expertise remains under the rmw layer and with the DDS security plugin system. However, I'd accept any mentorship from anyone who is more familiar with the present rmw architecture and API roadmap.

@sloretz

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@ruffsl It sounds like the next step is brainstorming for a different approach. I'll close this for now.

@sloretz sloretz closed this Mar 5, 2019

@sloretz sloretz removed the in review label Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.