Switch to one participant per context

[ivanpauno]

API changes needed after ros2/rmw_fastrtps#312.

[hidmic]

First pass

[hidmic]

@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.

[ivanpauno]

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).

[ivanpauno]

@wjwwood do you have any opinion on this?

[wjwwood]

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.

[hidmic]

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.

[wjwwood]

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.

[hidmic]

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.

[mikaelarguedas]

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)

[ivanpauno]

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.

[hidmic]

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.

[hidmic]

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

[ivanpauno]

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.

[ivanpauno]

@wjwwood and on this?

[wjwwood]

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.

[hidmic]

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.

[wjwwood]

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.

[hidmic]

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.

[wjwwood]

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.

[hidmic]

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).

[hidmic]

Overall LGTM but for small comments.

[hidmic]

Overall LGTM but for some minor comments and one concern.

[hidmic]

LGTM but for a few minor comments

[hidmic]

LGTM !