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

Ros2 service crystal #71

Merged
merged 7 commits into from Jan 11, 2019

Conversation

Projects
None yet
7 participants
@ross-desmond
Copy link
Contributor

ross-desmond commented Nov 28, 2018

Added generate_permissions file based on node graph api. Enables security of services through sros policy yaml and definition file of how the policy yaml is constructed.

ros2_security_helper enables cmake macro to generate security files through colcon build for development and deployment. Note, this does not get built due to the structure of sros 2. OSRF please let me know where this macro should live to enable security by default when developing on ROS2
e.g:
find_package(ros2_security_helper REQUIRED)
ros2_secure_node(NODES minimal_publisher minimal_subscriber minimal_service)

ross-desmond added some commits Nov 20, 2018

Add generate permissions security command line
Generate an sros2 yaml permissions file with the permissions of every visible node
on the dds network.

Add custom service security to sros2

Example: run the minimal_publisher_lambda node
Execute: `ros2 security generate_permissions node_policies.yaml`

It will create the following file in the current directory:
```
/minimal_publisher:
  services:
    /minimal_publisher/describe_parameters:
      allows:
      - request
      - reply
      .
      .
      .
  topics:
    /parameter_events:
      allows:
      - publish
      - subscribe
    /topic:
      allows:
      - publish
```

cr https://code.amazon.com/reviews/CR-3943967
Proposed policy definition changes
Issue: services and actions are not considered in the policy yaml
definition.
Solution: Add ipc types (services and actions)

Issue: access values are strings with p, s, or ps in order. This is not
descriptive and difficult to scale should more permissions become necessary.
Solution: Either change the parsing of the string or change the yaml to
be more flexible and descriptive for users.

The proposed changes include:
* Access value is a list, not a string
* Add ipc types such as actions and services
* Access values are no longer shorthand p or s, but publish/subscribe

Amend policy definition with verbose ROS ipc types
cmake security macro
Add security macro for automagically generating public and private keys
for authentication and encryption.

custom macro use
`ros2_secure_node(NODES node_name_1 node_name_2 ...)`

cr https://code.amazon.com/reviews/CR-3517594

@tfoote tfoote added the in review label Nov 28, 2018

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Nov 28, 2018

@ross-desmond I think that we need to get the underlying pull requests in for the node graph API before we can considering getting this merged in to sros2. Did you make any progress on the other implementations outlined in this comment? ros2/rcl#333 (comment)

@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Nov 28, 2018

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Nov 28, 2018

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Nov 29, 2018

@ruffsl I'm just now getting up to speed on sros2 stuff after transitioning it from Mikael, would you mind putting a second set of eyes on this for me?

@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Dec 5, 2018

General comment: In the future, is anyone opposed to using jinja for templates?

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Dec 5, 2018

General comment: In the future, is anyone opposed to using jinja for templates?

As opposed to? Or for which task?

@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Dec 5, 2018

General comment: In the future, is anyone opposed to using jinja for templates?

As opposed to? Or for which task?

sros2 has all the xml code in the api.py, it would be nice to have template files that python inserts the correct data to. This is just a though, it will not change this PR

@clalancette

This comment has been minimized.

Copy link

clalancette commented Dec 5, 2018

sros2 has all the xml code in the api.py, it would be nice to have template files that python inserts the correct data to. This is just a though, it will not change this PR

I won't go too far down this road here, but my concerns would be whether a) jinja is supported on macOS, Windows, and Linux, and b) adding in yet another templating system. We already have empy (which is non-specific to XML), and I find templating systems difficult to master. Adding more of them doesn't seem like a recipe for maintainable code down the road.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Dec 5, 2018

sros2 has all the xml code in the api.py, it would be nice to have template files that python inserts the correct data to. This is just a though, it will not change this PR

That would be fine with me. We do already have a dependency on EmPy which is a template engine, so using that would avoid a new dependency, but I don't feel strongly about that. +1 for moving the file text out of the python files (but later though).

@ruffsl

This comment has been minimized.

Copy link
Member

ruffsl commented Dec 5, 2018

I won't go too far down this road here, but my concerns would be whether a) jinja is supported on macOS, Windows, and Linux, and b) adding in yet another templating system. We already have empy (which is non-specific to XML), and I find templating systems difficult to master. Adding more of them doesn't seem like a recipe for maintainable code down the road.

+1 for not adding yet another PL specific templating library. I'd suggest we formalize the permission transform to be agnostic of any programming implementation, to be more formally typed, and transparent to promote standardization of how ROS2 permissions are generated for DDS, or any other transport implementation in the future for that matter.

I've created a branch that gives an example using program language agnostic transform for converting ROS2 permission profile markup into transport specific permission XML file. Users could then ecopertate the eXtensible Stylesheet into their own tools to consistently translate the permissions as a known finite transformation. I just spent an hour or so on it, but its relatively simple to support, and I can extend it into this PR if there is interest. See #72 for current diff.

@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Dec 7, 2018

Is this planned for a merge at some point? Is there any help I can provide?

@jacobperron
Copy link
Member

jacobperron left a comment

@ross-desmond Since this doesn't appear to break any existing API, it is okay for this to be merged and released post Crystal launch (which is why it hasn't received much attention yet). I've left a preliminary review, but since I'm not up-to-speed with sros2, it would be better to get more eyes on.

# we have some policies to add !
for topic_name, policy in topic_dict.items():
tags = []
if policy['allow'] == 'ps':

This comment has been minimized.

@jacobperron

jacobperron Dec 7, 2018

Member

Is this change necessary? I think it would be better not include it to help keep the PR succinct.

This comment has been minimized.

@ross-desmond

ross-desmond Jan 3, 2019

Author Contributor

It would help keep the PR succinct, but then there would be different ways of describing the security for services(-request, -response) vs topics (ps). It makes the parsing and description of the elements more verbose and robust.

Show resolved Hide resolved sros2/api/__init__.py Outdated
Show resolved Hide resolved sros2/api/__init__.py Outdated
Show resolved Hide resolved examples/sample_policy.yaml
Show resolved Hide resolved examples/policy_definition.txt Outdated
@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Jan 3, 2019

Thank you for the review @jacobperron.

Just a reminder the security_helpers folder in sros2 may want to be split out to another package as it does not build under the current sros2 package structure. If we believe this helper macro should stay in sros2 then I'll need to restructure to build sros2 python modules and the helper macro, thoughts?

@jacobperron

This comment has been minimized.

Copy link
Member

jacobperron commented Jan 8, 2019

Just a reminder the security_helpers folder in sros2 may want to be split out to another package as it does not build under the current sros2 package structure. If we believe this helper macro should stay in sros2 then I'll need to restructure to build sros2 python modules and the helper macro, thoughts?

IMO, the cmake package can remain in this repo (but remain a separate package) as it matches the repo description. However, I would consider remaining the package to something like sros2_cmake, since its main purpose is providing cmake macros for interacting with ROS 2 security.
I would restructure this repo so that the two packages are at top along with the general docs about sros2:

sros2
  |_ sros2 (python lib)
  |_ test
  |_ package.xml
  |_ setup.py
  |_ ... etc
sros2_cmake
  |_ cmake
    |_ ros2_create_keystore.cmake
    |_ ros2_secure_node.cmake
  |_ CMakeLists.txt
  |_ package.xml
  |_ ... etc
CONTRIBUTING.md
LICENSE
README.md
SROS2_Linux.md
SROS2_MacOS.md
SROS2_Windows.md

I'd like to get some other thoughts on this proposal.

If we go this approach, I think it would be better if the cmake package is added in a separate PR as it is distinct from the CLI changes and is a larger change.

Show resolved Hide resolved examples/policy_definition.md Outdated
Show resolved Hide resolved CHANGELOG.rst Outdated
Show resolved Hide resolved ros2_security_helper/CMakeLists.txt Outdated

ross-desmond added some commits Jan 7, 2019

Addresses comments from pull-71
* Adds check to allow "/" or no "/" in permissions file
* Renames policy definition file to markdown

@ross-desmond ross-desmond force-pushed the ross-desmond:ros2_service_crystal branch from 513f0b3 to 81189cf Jan 9, 2019

@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Jan 9, 2019

Fixing flake8 errors, incoming

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Jan 9, 2019

CI up to SROS2:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Jan 9, 2019

Just a reminder the security_helpers folder in sros2 may want to be split out to another package as it does not build under the current sros2 package structure. If we believe this helper macro should stay in sros2 then I'll need to restructure to build sros2 python modules and the helper macro, thoughts?

IMO, the cmake package can remain in this repo (but remain a separate package) as it matches the repo description. However, I would consider remaining the package to something like sros2_cmake, since its main purpose is providing cmake macros for interacting with ROS 2 security.
I would restructure this repo so that the two packages are at top along with the general docs about sros2:

sros2
  |_ sros2 (python lib)
  |_ test
  |_ package.xml
  |_ setup.py
  |_ ... etc
sros2_cmake
  |_ cmake
    |_ ros2_create_keystore.cmake
    |_ ros2_secure_node.cmake
  |_ CMakeLists.txt
  |_ package.xml
  |_ ... etc
CONTRIBUTING.md
LICENSE
README.md
SROS2_Linux.md
SROS2_MacOS.md
SROS2_Windows.md

I'd like to get some other thoughts on this proposal.

If we go this approach, I think it would be better if the cmake package is added in a separate PR as it is distinct from the CLI changes and is a larger change.

I'm in for this, I'll remove it from this package, @ruffsl how does that sound?

@ruffsl

This comment has been minimized.

Copy link
Member

ruffsl commented Jan 10, 2019

I'm in for this, I'll remove it from this package, @ruffsl how does that sound?

I'd be fine with that restructuring. I'd still like to recommend the use of stylesheets to compile the xml permissions, rather than a yaml implementation specific approach. I'd welcome feedback on #72 , and could rebase if the repo is restructured as above.

@ross-desmond

This comment has been minimized.

Copy link
Contributor Author

ross-desmond commented Jan 10, 2019

I'm in for this, I'll remove it from this package, @ruffsl how does that sound?

I'd be fine with that restructuring. I'd still like to recommend the use of stylesheets to compile the xml permissions, rather than a yaml implementation specific approach. I'd welcome feedback on #72 , and could rebase if the repo is restructured as above.

Awesome, I'm happy to move to XML as well. In the meantime, using generate_permissions with create_permission will have the same result as with json and xml. Let's get the xml in for the next patch release?

@jacobperron
Copy link
Member

jacobperron left a comment

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit 9efd4e0 into ros2:master Jan 11, 2019

1 check passed

Cpr__sros2__ubuntu_bionic_amd64 Build finished.
Details

@jacobperron jacobperron removed the in review label Jan 11, 2019

@ross-desmond ross-desmond referenced this pull request Jan 11, 2019

Merged

Sros2 cmake macros #75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment