parameter_events topic is now absolute

[mikaelarguedas]

Both subcriber and publishers to parameter_events are now absolute. Will need backport in Foxy Subscriber: https://github.com/ros2/rclcpp/pull/1257 Publisher: https://github.com/ros2/rclcpp/blob/a8cd93623964a089ee132d65e8b5eda84c15740d/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp#L72

[ruffsl]

Is there a way we could rain in the growing number of global full duplex topic permissions?

[mikaelarguedas]

Note, however, that it's not yet backported to Foxy, and it's unclear if that will happen.

That's a good point. The following bit in the PR status lead me to believe it was very likely we'll need this backported: "jacobperron moved this from Proposed to Needs release in Foxy Patch Release 3 6 days ago"

Is there a way we could rain in the growing number of global full duplex topic permissions?

I'm not sure I got this bit, @ruffsl can you clarify what you have in mind ?

I agree that having this topic becoming absolute and being full duplex is pretty lose on a security and graph connectivity front (everyone will subscribe and publish to that and basically any node can extract a great deal of data about the entire system configuration via that single topic). Looks like the decision happened in https://github.com/ros2/rclcpp/pull/929. although it doesn't seem to match the design document that states:

Each node will provide a topic on which parameter events will be published.

Some prior discussion happened at https://github.com/ros2/ros2/issues/432 although the conclusion seems different to the new topic mapping.

maybe @ivanpauno can provide more info about the change to absolute and the default full duplex nature of the topic.

Regardless of the rationale, we'll still need this PR to be merged to match what the client libraries are doing.

[ivanpauno]

I agree that having this topic becoming absolute and being full duplex is pretty lose on a security and graph connectivity front (everyone will subscribe and publish to that and basically any node can extract a great deal of data about the entire system configuration via that single topic). Looks like the decision happened in ros2/rclcpp#929. although it doesn't seem to match the design document that states:

See discussion in https://github.com/ros2/rclcpp/pull/829#issuecomment-557660967. You can still remap the topic to something different if you want.

Each node will provide a topic on which parameter events will be published.

Though the document said that, all nodes with the same namespace were using the same topic.

You either want a topic per node, or one for everybody (that can be remapped). One per namespace didn't have much sense.

[mikaelarguedas]

thanks for the prompt answer.

Agreed that one per namespace didnt make sense. Indeed its possible to remap the topic for each and every node, this can help mitigate the breadth of the topic distribution.

Do you also have insight about the full duplex nature of it ? is it possible for nodes to not need subscription access to this topic (remapped or not) ?

[ruffsl]

I'm not sure I got this bit, @ruffsl can you clarify what you have in mind ?

You got the gist: bilateral communication plus fully connected computation graph, a worst case scenario for security and a massive setback to principle of least privilege. The only barrier to information flow would be IDL type checks, but that's not enforceable in Secure DDS from a distributed client/server attestation model.

[ivanpauno]

Do you also have insight about the full duplex nature of it ? is it possible for nodes to not need subscription access to this topic (remapped or not) ?

So, I think that nothing relies on being able to listen the topic. With one exception: we're listening the topic to detect on_sim_time changes. This actually doesn't matter much, as we're only listening to the events from the same node (and not from other nodes). If there would be a way to trigger a callback locally when a parameter is set (without subscribing to the topic), that wouldn't be needed (there are callbacks to reject accept parameters, but you cannot know if the parameter is actually going to be accepted or not there).

We also don't use its full duplex nature AFAIK. A node should be able to publish in its own parameter event topic, and may want to subscribe to the parameter event topic of another node (or its own topic, if we don't have a way to trigger a callback locally when a parameter is accepted).

So, I think that the topic can safely be made private. I don't mind strongly about a having an absolute or private topic as the default.

---

There's yet another issue: the parameter event client is always listening to /parameter_events, so if another node has a different parameter events topic you cannot listen to it (because remapping /parameter_events will move both the topic where you are publishing and the topic you're listening to). That could be solved by adding a topic_name argument to the class constructor.

It could also be solved by making the topic name private, and the topic name would be inferred from the passed node name https://github.com/ros2/rclcpp/blob/96cccf5fdef6401b14974595d3481f9cc2223d30/rclcpp/include/rclcpp/parameter_client.hpp#L79 (this isn't incompatible with passing a custom topic name to it, but it's maybe a nicer default).

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/security-ramifications-of-global-parameter-events/15924/10

[mikaelarguedas]

the change has been merged on master and foxy now, so I'm going to merge this and backport it for ros2 to stay in sync with the ROS 2 client libraries

[codecov[bot]]

Codecov Report

Merging #233 (ffc2828) into master (1b45571) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #233   +/-   ##
=======================================
  Coverage   77.25%   77.25%           
=======================================
  Files          25       25           
  Lines         664      664           
  Branches       55       55           
=======================================
  Hits          513      513           
  Misses        131      131           
  Partials       20       20           

Flag	Coverage Δ
unittests	77.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1b45571...ffc2828. Read the comment docs.