ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
215 stars 194 forks source link

Topic name constraints discrepancy #291

Open artivis opened 4 years ago

artivis commented 4 years ago

Hi all,

While working with sros2 and the freshly supported DDS Security logging plugin (see e.g. rmw_connext), I encountered an issue related to the topic name constraints.

For context, the DDS Security specs defines both the security logging message type and topic name in section 9.6; more specifically: The BuiltinLoggingTopic shall have the Topic name “DDS:Security:LogTopic”. As one would expect, it is this topic that e.g. RTI uses.

After reading the 'Topic and Service name mapping to DDS' design doc and more precisely the 'ROS 2 Topic and Service Name Constraints' section it appears that the security topic, as defined by the DDS Sec specs, is not a valid topic name in ROS2 realm since the 'colon' character is not valid. What's even more annoying is that according to the same design doc, section 'DDS Topic Names', the topic name is not valid for DDS either... After briefly consulting the DDSI-RTPS spec v2.2 and the DDS spec v1.4, I could not find any explicit constraint on topic naming, but the 256 length limit. The constraints ('a', ..., 'z', 'A', ..., 'Z', '0', ..., '9', '_') mentioned in the design doc only appears in 'Annex B - Syntax for Queries and Filters' of the DDS spec v1.4. I therefore wonder if those are actual topic naming constraints...

From there my questions are,

  1. Is there a mistake in the DDS Security spec or were the naming constraints misinterpreted? (The colon character is invalid vs the topic constraints do not exist). Neither and I'm missing something?
  2. Assuming the topic DDS:Security:LogTopic is not a mistake, how could I get to subscribe to it from ROS2? Can we imagine that the colon character joins the list of valid characters? Or could the constraints be disabled much like the ROS specific namespace prefix can be disabled? Maybe using the same avoid_ros_namespace_convention function?

@ros2/security_working_group

gbiggs commented 4 years ago

it appears that the security topic, as defined by the DDS Sec specs, is not a valid topic name in ROS2 realm since the 'colon' character is not valid

That's OK, because that topic lives in the DSS world. It is not a ROS 2 topic. ROS 2's topic name constraints are extra restrictions on top of the DDS topic name constraints, and only for topics created by ROS.

Is there a mistake in the DDS Security spec or were the naming constraints misinterpreted? (The colon character is invalid vs the topic constraints do not exist). Neither and I'm missing something?

The topic constraints do not exist for topics created by DDS and its facilities. They are only relevant to names used in ROS, and designed to ensure that the names that come from ROS can be mapped unambiguously to DDS topic names.

Assuming the topic DDS:Security:LogTopic is not a mistake, how could I get to subscribe to it from ROS2?

Probably you would have to use the DDS API of your DDS vendor directly. You can get access to the objects created by your DDS implementation underneath the rmw layer and use those with the DDS API directly.

artivis commented 4 years ago

That's OK, because that topic lives in the DSS world. It is not a ROS 2 topic. ROS 2's topic name constraints are extra restrictions on top of the DDS topic name constraints, and only for topics created by ROS.

Not sure what you mean by created but the aforementioned topic is not created by ROS per se. I'm only trying to subscribe to it.

ROS 2's topic name constraints are extra restrictions on top of the DDS topic name constraints [...] The topic constraints do not exist for topics created by DDS and its facilities.

That seems contradictory or am I reading it wrong? In case the constraints do not exist for topics created by DDS then maybe the design doc needs to be fixed since it states:

In DDS, topic names are restricted by these restrictions:

From DDS 1.4 specification, or the RTI documentation:

TOPICNAME - A topic name is an identifier for a topic, and is defined as any series of characters 'a', ..., 'z', 'A', ..., 'Z', '0', ..., '9', '_' but may not start with a digit.
...

Probably you would have to use the DDS API of your DDS vendor directly. You can get access to the objects created by your DDS implementation underneath the rmw layer and use those with the DDS API directly.

While I could implement a plain DDS subscriber, I wish to benefit from ROS to be vendor agnostic. Furthermore, we (the Security WG) are working on enabling the DDS Security logging in ROS2, the fourth of five DDS security plugins defined in the DDS Security specs. As mentioned previously PRs were already merged in the vendor specific rmw layer so that at the moment the logging can be enabled but not accessed from ROS. This logging will allow one to monitor the security events of a ROS2 graph (e.g. an unexpected subscription to your image (ros) topic). It would be unfortunate if this information cannot be accessible at the ROS2 level in any ways and monitored through e.g. rqt_console and such.

Of course I'm not advocating for removing the topic naming constraints but rather for opening the discussion on how to access this particular topic (DDS:Security:LogTopic) from ROS.

Friendly ping to @wjwwood as he wrote most of the design doc. I'd be interested in knowing your view on this matter.

gbiggs commented 3 years ago

One option could be to add an API in the rmw layer.

artivis commented 3 years ago

@gbiggs not sure to understand what you mean. An API in the rmw layer to (?) expand/disable the topic naming constraint?

gbiggs commented 3 years ago

An API that gives access to information about the underlying security information, such as logged events.

wjwwood commented 3 years ago

I think this is a bug in the DDS security spec or implementation, if they are knowingly using : in their topic names. It is definitely not allowed (yet) in the DDS specification. The section we quoted from our design docs is still there in the DDS spec. At this moment the ROS 2 spec is a super set of what is technically allowed in the DDS spec, in that we also allow the / character, in addition to alphanumerics and underscores.

If either the DDS spec is updated to allow : or they just continue using : in the name anyways, then you will not be able to subscribe to this topic with ROS 2. I don't think we'd add : to our allowed names either.

So you'd either need to add a way to ROS 2 to skip checking the topic name for validity (the avoid_ros_namespace_convention will not do this) or you'd need a function that subscribes to the security topic or otherwise exposes the information from it, as @gbiggs suggested.

Either way, our design document is still up-to-date right now, so I'll close this issue.

I recommend continuing the discussion with the ROS 2 security working group and/or asking someone in the DDS community and/or the OMG group about this issue.

artivis commented 3 years ago

Hi, so, we (the Security WG) had a meeting today with members of the OMG group regarding various security-related topics. During the meeting we had the opportunity to ask about DDS topic naming constraints and got the confirmation that, as of today, there are no limitations on what characters can be used for a DDS topic name. The OMG group acknowledges that this can potentially be an issue and plans on defining such valid subset of characters. One may refer to the (open) issue DDS15-197 for further information. Here is an extract:

The DDS specification states a Topic name is a string but it does not state what the legal characters are. [...] Because of this it is also possible for multiple vendors to put in place restrictions that could cause incompatibilities.

As pointed in my initial post, the constraint 'a', ..., 'z', 'A', ..., 'Z', '0', ..., '9', '_' was mistakenly understood as applying to topic names when really it concerns syntax for queries and filters.

The OMG group plans on fixing this situation by the next revision and we received an oral confirmation that the colon character will be part of the valid set (thus the security logging topic will be valid as per the specs).

Now I don't know how this update will affect the ROS 2 topic naming constraints but would it be foolish of me to hope to see the colon character supported by ROS 2 as well ? :slightly_smiling_face: Or should I start working on a workaround ? :slightly_frowning_face:

Edit: Could we see this issue re-opened? Thanks

wjwwood commented 3 years ago

I'd argue for a workaround that allows you to use any character you want under the hood, bypassing the ROS checks, rather than updating the ROS constraints to be more permissive.

simue commented 2 months ago

Hi, was there subsequent discussions on ros2's topic naming scheme?
The cited part in ros2 design document on topic naming uses the query language grammar as argument for topic naming restrictions, while according to DDS wire protocol 2.5 the topic name is just defined as string<256>.

I am a bit puzzled with the ros2 design document using the RTI documentation's query language to specify topic naming scheme. Is this an error in dds spec, or ros2 spec?

Also: the ros2 design document narrows down name tokens to alphanumeric and not having a digit as first character, while for pure eprosima fast-dds a topic like rt/sensors/0/data would be legal and is working. This breaks interoperability with dds, making ros2 dds communication a subset of what is legal in dds.