Conversation
63b6fcb to
0b873ad
Compare
|
Ok, I think the basics are working at least: Details |
ivanpauno
left a comment
There was a problem hiding this comment.
Some of the verbs arguments or arguments' help should be updated.
e.g.:
or
There was a problem hiding this comment.
Great, thanks @ruffsl !
not a full review, didnt test the code just some fly-by comments
|
@ruffsl To allow publishing/subscribing to
I didn't open a PR to avoid adding extra noise and just in case you want to implement this differently. The first commit can probably be taken as-is. The second one, you maybe want to add something more explicit to the verbs arguments.
I have already updated system_tests/test_security based on changes here and in
I have already added this to fastrtps, and connext rmw implementations. I have to add something similar in cyclonedds (though they don't support security yet, just for completion), and implement the function in |
|
@ruffsl Friendly ping. It would be great to wrap up this for the beginning of next week (e.g. Wednesday), considering that Foxy deadline is close. I can help with some of the remaining tasks if you don't have time to handle them (e.g.: updating the tests). Let me know if I can help with anything. |
6720468 to
a2397e6
Compare
| </topics> | ||
| </subscribe> | ||
| </allow_rule> | ||
| </xsl:if> |
There was a problem hiding this comment.
Lets hope we don't have to keep this fully connected data flow rule forever.
There was a problem hiding this comment.
Yes. I think that in a near future (two/three months), all the rmw implementation will be migrated.
There was a problem hiding this comment.
I believe what @ruffsl mean is that hopefully this will be done out of band and not require a fully connected rule on this topic.
There was a problem hiding this comment.
Oh, I see. I think it's definitely possible.
It would be great to finish discussing this alternative in the design document, before the other migrations are done.
Of course, we can first migrate all to use the ros_discovery_info topic, and then change them again to use Participant userData.
|
Hey @ivanpauno @mikaelarguedas , I had a moment to push some commits to address things.
I've cherry picked them into the PR. You're suggesting adding a verb to control this? If its already reading from the env, we may not need to clutter the CLI with a subsumption layer of settings.
Sounds fine.
Thanks! Could you wrap up this PR with the tests and any hanging review comments. I can't immediately think of a pythonic pattern to do the file/dir/symlink checks for the keystore, and I've got another deadline for my lab this week. |
sros2/sros2/verb/create_key.py
Outdated
| def add_arguments(self, parser, cli_name): | ||
| arg = parser.add_argument('ROOT', help='root path of keystore') | ||
| arg = parser.add_argument( | ||
| '-k', '--keystore-root-path', |
There was a problem hiding this comment.
To avoid a hard API break, 'ROOT' can be maintained too.
It could be also detected if it was used and print a deprecation warning if later is going to be removed.
P.S.: People will have to read about changes in sros2 for Foxy, so maybe it's not too bad to break this too.
There was a problem hiding this comment.
People will have to read about changes in sros2 for Foxy, so maybe it's not too bad to break this too.
Yeah, I wanted to use this as an opportunity to make the CLI more consistent. The mix use of positional and keyed command arguments was really annoying in terms if UI.
There was a problem hiding this comment.
👍 I was considering doing this before Foxy as well, currently only newly introduced verbs like generate_artifacts have keyed arguments. For clarity it would be better to have them in a separate PR. If we keep it in this one, can we make this in an independent commit (similar to file renaming commits) ?
There was a problem hiding this comment.
I will consolidate things in different commits before merging.
Yes, I will follow up with this tomorrow. Thanks for your prompt answer!! |
mikaelarguedas
left a comment
There was a problem hiding this comment.
thanks @ruffsl for the iteration!
added a couple inline questions to be able to review the following developments
sros2/sros2/api/__init__.py
Outdated
| @@ -198,17 +242,21 @@ def is_key_name_valid(name): | |||
| try: | |||
| return (validate_namespace(node_ns) and validate_node_name(node_name)) | |||
There was a problem hiding this comment.
is this still relevant ?
Seems like key names will now be de correlated from the concept of node name or node namespace
There was a problem hiding this comment.
I've created a function in rcl: rcl_validate_security_context_name.
It does pretty much the same that rmw_validate_namespace.
I will use the later, until I propagate the rcl function to rclpy.
sros2/sros2/api/__init__.py
Outdated
| dest_governance_path = os.path.join(key_dir, 'governance.p7s') | ||
| shutil.copyfile(keystore_governance_path, dest_governance_path) | ||
| relativepath = os.path.relpath(keystore_governance_path, key_dir) | ||
| os.symlink(src=relativepath, dst=dest_governance_path) |
There was a problem hiding this comment.
What happens in the destination file already exists?
There was a problem hiding this comment.
If the file already exists, it will raise a FileExistsError.
Should the old file be deleted and overridden?
There was a problem hiding this comment.
We could use a logic similar to the above one above:
- if it exists and is the same, do nothing
- if it exists and is different -> delete + override it (this part is not above but I believe would be nicer behavior)
There was a problem hiding this comment.
Addressed in f5080e8.
Instead of delete + override, I'm raising an error in that last case.
I'm afraid to delete something a distracted user didn't want.
They can cleanup manually and rerun if they really wanted to delete the file.
There was a problem hiding this comment.
Maybe it's my workflow but I find it convenient to change a couple rules in a policy file and just rerun generate_artifacts on my entire policy file and trust the tool to override the changed permission files accordingly. It looks like with this more conservative approach I would need to go prune by hand all the directories that may have a permission change.
I'd prefer not changing the previous behavior, but either way it would be good to provide an option to opt-in (or out) of overwriting existing files
There was a problem hiding this comment.
In my prior work with keymint, I treated auto generated security files as build artifacts, and thus subsequent build file could be modified at will by the build tool, where the user would recognize the temporal nature of these files if they instructed the build tool to rebuild changed source files. So I'd say let the tool overwrite it the generated files by default.
In keymint, I also separated the pipeline into a src. build, and install folder structure, so we may want to explore using similar workflows that could separate the policy source files, from the generated intermediate representations built, to the final signed security artifacts installed. Keymint had a --dry-run flag that the CLI could check to see if it should warn about detected file diffs and not write to disk.
|
I've addressed comments and made tests pass. |
| raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), keystore_path) | ||
| if not os.path.exists(contexts_path): | ||
| return True | ||
| for name in os.listdir(contexts_path): |
There was a problem hiding this comment.
Context paths can be nested. Is listdir recursive?
I.e. will it walk over contexts that aren't just flat in the root of the context sub folder?
There was a problem hiding this comment.
👍 to that.
To be fair it was already the case before this change so doesn't have to be addressed here. Should be ticketed though if not addressed
There was a problem hiding this comment.
To be fair it was already the case before this change so doesn't have to be addressed here. Should be ticketed though if not addressed
Agreed.
What should be the output of:
-> contexts
-> context1
-> security artifacts ...
-> nested1
-> security artifacts ...
->context2
->nested2
->security artifacts...
?
IMO, it should be:
/context1
/context1/nested1
/context2/nested2
If the contexts folder have valid security artifacts, / should be showed too.
mikaelarguedas
left a comment
There was a problem hiding this comment.
thanks @ivanpauno for the follow-up. I did another pass below
sros2/sros2/api/__init__.py
Outdated
| dest_governance_path = os.path.join(key_dir, 'governance.p7s') | ||
| shutil.copyfile(keystore_governance_path, dest_governance_path) | ||
| relativepath = os.path.relpath(keystore_governance_path, key_dir) | ||
| os.symlink(src=relativepath, dst=dest_governance_path) |
There was a problem hiding this comment.
We could use a logic similar to the above one above:
- if it exists and is the same, do nothing
- if it exists and is different -> delete + override it (this part is not above but I believe would be nicer behavior)
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-03-18/13313/1 |
|
@ruffsl @mikaelarguedas
Something else missing to get this approved? |
|
I was only wondering about that last unresolved comment you linked. If you'd like to follow up that with a septate PR, that's fine. This repo is set so authors of PR can't approve the same PR, but this all LGTM! |
There was a problem hiding this comment.
Couple high-level comments from me. This mostly looks good, although @mikaelarguedas is clearly more up-to-date on the design than I am. In the future, I'd prefer to see unrelated changes like the CLI bits in another PR. This one was big.
sros2/setup.py
Outdated
| 'generate_artifacts = sros2.verb.generate_artifacts:GenerateArtifactsVerb', | ||
| 'generate_policy = sros2.verb.generate_policy:GeneratePolicyVerb', | ||
| # TODO(ivanpauno): Reactivate this after having a way to instronspect | ||
| # security context names in rclpy |
There was a problem hiding this comment.
Do we have an issue in rclpy that we can use for tracking this?
There was a problem hiding this comment.
+1, TODOs in code usually don't get revisited if there isn't an associated ticket (same for other TODOs below)
Spelling: instronspect -> introspect
There was a problem hiding this comment.
Can we add a reference to ros2/rclpy#529 in this comment, please?
| def security_context_keys_dir(tmpdir_factory): | ||
| keystore_dir = Path(str(tmpdir_factory.mktemp('keystore'))) |
There was a problem hiding this comment.
Has pytest been updated enough we can use tmp_path_factory here now? Would save you a little work. See #145 for reference.
There was a problem hiding this comment.
Sure, but I think we can left that for a follow-up, because I'm not sure which pytest version we're using in the different platforms.
|
Alright this looks good to me now. What are we going to do about the failing tests, though? Can we ignore Focal for now in order to get this in before the freeze? @ivanpauno let us know what you learn about Windows. |
Yes, I think it's safe to ignore the tests that are failing in the nightlies. |
|
@ivanpauno if you revert/re-sign my commits here, could you move those reverted CLI commits to a new PR? I'd really prefer to remove any positional arguments in SROS from foxy before the API freeze. I think the PR only looks big because of the static xml files generated to test/track the xsd templates. |
The motivation for reverting it was not because of the size but the change but their nature. There were doubts on the relevance of making all these non-positional (discussion on matrix). Especially as as-is they were made optional. Currently So in the end a more complete discussion need to be had and evaluate what UX we are trying to accomplish here. Discussion can happen on such PR though. |
Thanks @ivanpauno for the summary. Is there a way for maintainers here want to be notified for sros2/test_security failing jobs? I see some of those have been failing for a long time but we weren't aware of it. |
I will ignore all existing failures on nightlies. Unfortunately, there's a minimal chance I introduce a regression by doing this.
https://github.com/ros2/sros2/tree/contexts-backup has the original commits of the PR.
That's the last failure I see in my jobs I that's not on the nightlies.
IDK. @ros2/team for feedback. |
I think at least the ROS 2 buildfarmer should open issues for failures (e.g. on sros2) and consider tagging @ros-security-wg if the issue is in a different repository. |
The windows failures of security tests were those, and the same test was failing with connext. Fixed them in ros2/system_tests@1d93d97. I will re-run CI today by end of day. If everything looks good tomorrow, I will merge. |
mikaelarguedas
left a comment
There was a problem hiding this comment.
lgtm, a bionic CI on aarch64 would be good to confirm this doesnt break anything on that platform
Done. We're not supporting These are the last jobs I ran ros2/rmw_fastrtps#312 (comment) (only with unrelated failures), and I have to wait to these two before merging ros2/rmw_fastrtps#312 (comment). Hopefully, I will be merging today. |
| <subject_name>CN=/add_two_ints/add_two_ints_server</subject_name> | ||
| <validity> | ||
| <not_before>2013-10-26T00:00:00</not_before> | ||
| <not_after>2023-10-26T22:45:30</not_after> |
There was a problem hiding this comment.
@ruffsl @ivanpauno meta: is this supposed to span the foreseeable lifetime of Foxy? Won't this bite us in 3 years time? Same elsewhere.
There was a problem hiding this comment.
this is something I'm planning to address separately as unrelated to this PR.
This should be using the time of generation as the start date similarly to how it's done for the certificates. I'll open an issue to make sure it doesnt slip off.
Hackier way to address this is to just change the hardcoded value to start in 2020 which will give us a lot of time to implement it more properly ^^
Edit: opened ticket at #186
There was a problem hiding this comment.
I'd rather address #186 instead of pushing the expiration date (and then forgetting about it). Thanks for the explanation @mikaelarguedas!
There was a problem hiding this comment.
Me too. Happy to review any PR addressing #186, timing before feature freeze is pretty tight so if it falls on my plate I'll most likely just push the date back
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Codecov Report
@@ Coverage Diff @@
## master #177 +/- ##
=========================================
Coverage ? 61.57%
=========================================
Files ? 18
Lines ? 838
Branches ? 64
=========================================
Hits ? 516
Misses ? 308
Partials ? 14
Continue to review full report at Codecov.
|
Related: ros2/design#274 (comment)
TODO:
Bonus items
<profiles>type attribute