Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add initial draft for ROS 2 Security Contexts #1
Add initial draft for ROS 2 Security Contexts #1
Changes from 1 commit
a299714
85f1e96
5f4a64b
b43e4d3
39c5aac
f5e07e7
77a21f4
4c13826
ffe489f
95f75b6
5995840
a461e4c
97ae7e1
70b59f7
19e6004
b3f50fb
b17032a
226bc24
24ef462
0c04706
d50c3d8
20ccb00
2ac697c
a972685
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's the phrasing but this seems wrong to me.
Today the keystore layout is not flat and using each level of namespacing.
What I got from offline discussion was that this redesign was about the final identity to be the context FQN instead of the node FQN. so
/context_namespace/context_name
instead of/node_namespace/node_name
The default context_namespace being
/
and the default context_name being.
so that a participant using a default context would be finding artifacts directly at the keystore's root.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the node's name would not be used to resolve the context, allowing for nodes within the same namespace to easily be merged into a common context, or the default root context.
The change I added was that root context points to the root of the contexts folder, not the root of the keystore folder, just to keep the root of the keystore file system hygienic and organized. Feel free suggest a rephrasing to clarify this this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this is the approach we settled for offline because ROS namespaces have a very lose correlation to the process/context they will be running in. Could you clarify why this PR is now using node namespaces and not context namespaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the offline notes, we visited the idea of forcing users to manually specify context namespaces, as to be comely orthogonal to node namespaces, but concluded that tracking two orthogonal namespaces would be a large mental overhead for users, as well as require much more boilerplate.
Thus we iterated further on the idea of allowing the existing namespace elements in lauchfiles to hint/push the relative context path. However, the user could still override the relative context path using a absolute context path for the node element's context attribute, just like with topics/services/actions:
Perhaps we could also add these additional observations to the alternative section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph seems unrelated to the number of context per process. It is true and a drwaback in the case of single context per process as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've extended this section with a summarizing clarification in a461e4c
Feel free to suggest additional wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I got this part. Why would the security guaranties be lost if there are multiple contexts per process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment on a461e4c .