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

Switch to one participant per context #189

Merged
merged 23 commits into from
Apr 3, 2020

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Oct 10, 2019

API changes needed after ros2/rmw_fastrtps#312.

@ivanpauno ivanpauno changed the title Switch to one participant per context. [WIP] Switch to one participant per context. Oct 10, 2019
@mabelzhang mabelzhang added the enhancement New feature or request label Oct 17, 2019
@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 45c81c9 to de00c5d Compare November 20, 2019 15:42
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass

rmw/include/rmw/types.h Show resolved Hide resolved
{
enum rmw_security_enforcement_policy_t enforce_security;
const char * security_root_path;
} rmw_node_security_options_t;
} rmw_security_options_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanpauno meta: while I understand the need for rmw_security_options_t that can apply on a per context basis, I'm not entirely convinced we should be dropping rmw_node_security_options_t. There may be implementations, either not DDS-based or DDS-based but that do keep a 1-to-1 mapping between nodes and participants, where rmw_node_security_options_t still makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the security_options in the context and not in the node satisfies both use cases, one-to-one node participant mapping and one-to-one context-participant mapping.

A longer explanation:

  • The security options struct is just a boolean indicating if security is active or not, and a path. The path can be used in different ways depending on the implementation.
  • If the implementation enforces one-to-one context-participant mapping, that path will have the key/permissions/other files needed by that participant.
  • If the implementation enforces one-to-one node-participant mapping, that path will have subdirectories. The node will find it's security key/permissions/etc based on the base path specified in the (context) security options, and using the full node name to find the subdirectory. Basically, I'm proposing to move this logic from rcl to the specific rmw implementation that wants to do that (that logic will have not more sense in rcl).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjwwood do you have any opinion on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like it is necessary. Node specific configurations could still be in the new structure, just as a subset of it, if that's needed in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node specific configurations could still be in the new structure, just as a subset of it, if that's needed in the future.

That was my initial thought as well, but now I think @ivanpauno's point is the logical step forward considering the current rmw_node_security_options_t structure. Which brings about the question of whether the rmw API should include an abstraction that's tightly coupled to DDS-Security concepts in the first place. If that wasn't the case (does it have to be the case @ruffsl @mikaelarguedas @kyrofa?), then keeping a separate rmw_node_security_options_t structure would still make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if you had an rmw with security that was configured at the node level, you could still accommodate that with a single configuration to the context (you'd just have to preload the configurations for each node as a subset of the context's configurations). Going up in scope seems safe-ish to me (going from node to context). Conversely, going narrower seems less inclusive to alternative designs.

I'd defer to the security WG people though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's also true. I can't think of a case where only per node security options make sense and having to preload them into the context is a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR I don't think there is any strong reason for making this node specific, some questions about how the tooling needs to change to support the various use cases subsist.


If the implementation enforces one-to-one context-participant mapping, that path will have the key/permissions/other files needed by that participant.
If the implementation enforces one-to-one node-participant mapping, that path will have subdirectories.

Can you clarify what the implications of this are? It reads like users would have a different keystore layout based on the rmw implementation it is using. Is this correct? if so can you provide examples of what such keystores would look like (for each option)?

the question of whether the rmw API should include an abstraction that's tightly coupled to DDS-Security concepts in the first place.

I did not understand which abstraction is tightly coupled to DDS-Security, can you clarify which one?
Regarding the security options structure, ideally any implementation (DDS or other) relying on getting security artifacts to run could leverage that structure to retrieve them.
One limitation of this structure (maybe that's the coupling to DDS you're referring to) is the fact that it provides "just a path" and thus does not enable more advanced security material storage mechanisms such as TPM or secured execution areas (TEE, TrustZone, etc).

As of "does it have to be node specific or not" it is hard to answer as currently all the implementation using security are DDS-based. If the rmw context is a representation of the scope of a set of ROS 2 nodes running in a single process (and in the case of DDS in the same participant), there doesn't seem to be a use case for the security options to be anything else than context specific.

I like the idea to be able to define node roles and permissions on a per node basis and then compose them into a context-level policy, but it's unclear to me how this use case is planned on being supported in the current design. I assume it would be done by the tooling before runtime rather than by rmw during runtime? (I think this will be answered by the answer to my first keystore layout question)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what the implications of this are? It reads like users would have a different keystore layout based on the rmw implementation it is using. Is this correct? if so can you provide examples of what such keystores would look like (for each option)?

The current keystore layout is "node based", and now the security will be configurable only with context granularity. So I'm not really sure at what extend the current layout fits the needs.
We will probably start applying this change only to one of the rmw implementations, and not to all at the same time (the task is already big, doing it in the four officially supported rmw implementations at the same time is a little hard). If we do that, we will have some implementations where a participant is associated to a node, and others where it's associated to a context (that for example, will break some cross-vendor tests). If we find a keystore layout that fit the needs of both implementations, that would be great, and we won't have to do anything different depending on the implementation. But if we don't, this API change should satisfy both cases.

We will have to figure out how the sros2 keystore will look, and work in the needed changes, in a near future (the fastrtps refactor is close to be completed). I will have to read (and think) a little bit more about sros2 to have a detailed proposal of how it will look. Any help and ideas are welcomed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One limitation of this structure (maybe that's the coupling to DDS you're referring to) is the fact that it provides "just a path" and thus does not enable more advanced security material storage mechanisms such as TPM or secured execution areas (TEE, TrustZone, etc).

@mikaelarguedas Yes, but thinking about it again, I guess the problem isn't so much the path but the assumptions we make elsewhere about it being a reference to some security artifacts mounted in the local filesystem. It probably works for most use cases though, you're right about that.

If the rmw context is a representation of the scope of a set of ROS 2 nodes running in a single process (and in the case of DDS in the same participant), there doesn't seem to be a use case for the security options to be anything else than context specific.

You can still have multiple contexts (and multiple participants) in the same process, but you don't have to. So things don't change much when compared to the current state i.e. security options apply to nodes even though many of them may share process space.

The current keystore layout is "node based", and now the security will be configurable only with context granularity. So I'm not really sure at what extend the current layout fits the needs.

@ivanpauno One idea. If I understand correctly, SROS key paths mimic ROS node FQNs. That's neat and makes configuration easy. But if these keys should only ever be assigned to one process each, why not giving processes a name? To keep key lookup transparent as it is today, not to give them entity in the ROS graph. For DDS-based RMW implementations this will preclude using SROS with multiple participants that share the same domain ID[1], but that shouldn't be a problem if we agree that assigning potentially non-overlapping roles and permissions to the same process doesn't make sense.This also removes the need for rmw_*_security_options_t structs, as it becomes a process wide setting.

One good thing about this is that, if the process has a single node, process name and node name can be matched, and the SROS tooling and keystore layout doesn't have to change at all regardless of where the RMW of choice instantiates its node/participant/actor/client/device abstraction. Tooling can later be extended to transform a set of node roles and permissions to one that matches their actual process layout (e.g. merging permissions, checking if a given layout is consistent with those, before distribution or on launch).

[1] Though DDS Security spec seems to suggest (needs double check) you could have multiple participants with different domain IDs using the same governance, certificate and permission files.

@@ -172,7 +172,7 @@ rmw_create_node(
const char * name,
const char * namespace_,
size_t domain_id,
const rmw_node_security_options_t * security_options,
const rmw_security_options_t * security_options,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanpauno if security options are still an argument for rmw_create_node, why changing the struct name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically avoided to modify the rcl API further, in order to avoid propagating the changes in upstream packages and be able to test this.

domain_id and localhost_only also don't have more sense here.
My proposal is to do the same with them that with the security_options, and move them to the context. It still have sense in implementations that have a one-to-one mapping between node-participant, as modifying the domain_id or specifying localhost only with node granularity is really not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjwwood and on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they don't make sense at a node level, then move them to the context. I don't see any sense in avoiding it just to protect the API nor in order to support some imagined future rmw. I would just move them out of the arguments.

Copy link
Contributor

@hidmic hidmic Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically avoided to modify the rcl API further, in order to avoid propagating the changes in upstream packages and be able to test this.

Good call. But I can't avoid thinking we're conflating interface and implementation again. IMHO we should keep both (on a per context and per node basis), and let implementations decide when to use, when to override, when to ignore, when to assert.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep both and then implementations can decide when to use, when to override, when to assert.

Yes, but rather than pass it every where, I'd prefer to pass it at the highest scope and let it filter down if needed. Having, the domain id for instance, at the node level just makes what @ivanpauno wants to do impossible. You'd have to either ignore it or assert that the context and node's domain id match. I'd prefer to only have the context domain id, and if a new rmw that's not DDS could possible implement the ROS domain id on a node level, then we just loose that more powerful feature in favor of a more accommodating API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd have to either ignore it or assert that the context and node's domain id match

Some implementations may end up doing that, yes. I can't say I agree with the idea of overconstraining an API to spare an argument or a check for feature support. If ergonomics are an issue, you can always bundle arguments in a structure and pass NULL. But that's just my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't want the same setting in two places is all. Especially if today, ever setting only one of them (the node one) would result in misconfiguration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, fine with moving all these options to the context.

But FWIW, I was thinking of node specific configuration taking precedence over context specific configuration if the underlying rmw implementation supports it (which requires configuration settings to have std::optional-like semantics). You'd run into the same problem between process wide configuration and context specific configuration if at some point some rmw only supported a single ROS_DOMAIN_ID per process (e.g. because it cannot support multiple instances of its node/participant/actor/device/client abstraction in the same process space).

rmw/src/security_options.c Outdated Show resolved Hide resolved
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM but for small comments.

rmw/CMakeLists.txt Show resolved Hide resolved
rmw/include/rmw/security_options.h Show resolved Hide resolved
@ivanpauno ivanpauno changed the title [WIP] Switch to one participant per context. Switch to one participant per context Feb 6, 2020
@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from c25b645 to 14bf6eb Compare February 6, 2020 20:55
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM but for some minor comments and one concern.

rmw/include/rmw/security_options.h Outdated Show resolved Hide resolved
rmw/src/security_options.c Outdated Show resolved Hide resolved
@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from a7c6342 to afa5ffd Compare March 19, 2020 21:36
@ivanpauno ivanpauno requested a review from hidmic March 25, 2020 17:53
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but for a few minor comments

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/src/security_options.c Outdated Show resolved Hide resolved
rmw/src/security_options.c Outdated Show resolved Hide resolved
@ivanpauno ivanpauno requested a review from hidmic March 26, 2020 14:52
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
ivanpauno and others added 17 commits March 27, 2020 10:36
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan S Paunovic <ivanpauno@gmail.com>
Signed-off-by: Ivan S Paunovic <ivanpauno@gmail.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 2122d71 to 4747320 Compare March 27, 2020 13:36
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added in review Waiting for review (Kanban column) and removed needs review labels Apr 2, 2020
@ivanpauno ivanpauno merged commit 2f954d7 into master Apr 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/one-participant-per-context branch April 3, 2020 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants