[REP-2012] Service introspection #360
Conversation
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/rep-2012-service-introspection/26079/1 |
|
👍 |
There was a problem hiding this comment.
I have some minor feedback.
The proposal looks good, though I haven't completely read it.
It would be interesting to measure the overhead of publishing the events when there are no subscribers (ideally, it should be really small in that case, as nothing is being actually sent).
If the overhead is not small, publishing the events should be disabled by default (but hopefully, this will not add much overhead).
There was a problem hiding this comment.
@jacobperron I went ahead and dropped some comments while trying to be be constructive. One minor aspect is that services and clients are used within the content while referring to service servers and service clients. I believe I understand what's meant, but would recommend sticking with the latter since it's more obvious for the new reader. Made a few suggestions below accordingly but probably missing some.
I did not perform a complete security assessment on this (which would require diving deeper into threat modeling, etc) but left some thoughts on the topic. In summary, security-wise, you guys may want to discuss among authors the following concerns:
- The document should describe the expected behavior of service introspection (and the abstractions created as part of it) in the presence of security and without it. When considering security, I'd recommend considering various combinations of the DDS Security plugins related to encryption, authentication and authorisation. Note that DDS implementations differ in how they provide these. I left some recommendations on how to extend the security section.
- Remotely monitoring ROS 2 services should be done in a security conscious manner. My gut feeling is that this feature should only be possible against ROS 2 graphs and when security is enabled, otherwise it can easily lead to various attacks including:
- replay attacks
- exfiltration of data
- Denial-of-Service
- Even if security mechanisms are enabled and work as expected, the feature in here sounds to me juicy from a side-channel attack perspective. It's probably worth discussing this further once a prototype of the feature is available and security enabled on it. At that point we could try and challenge different ideas.
| Publishing Service Events | ||
| ------------------------- | ||
|
|
||
| Whenever a request or response is sent or received, a *service event* message will be published to a topic. |
There was a problem hiding this comment.
Whenever a request or response is sent or received, a service event message will be published to a topic.
This can lead to serious security consequences if access control is not enforced (and enabled) appropriately in the graph and can easily lead to denials of service. This will affect local setups but it's specially obvious in remote monitoring interactions.
Unless restricted in-code (while implementing this capability), crafting RTPS packages that mimic such requests or responses 12 will trigger (each) automatically a topic publication. If unrestricted, an attacker can easily overflow Service clients (and servers) with responses (or requests) triggering undesired traffic in the databus. We demonstrated in the past how a similar situation led to easily crashing DDS implementations.
I believe the conflict above (again, under the assumption access control isn't present or it's violated) can be escalated further by a motivated attacker if he/she proactively creates ficticious subscribers that require networking interactions. Such an escenario can easily then lead to saturating the Linux Networking Stack queues which can cause the system to miss other relevant traffic or worse.
Footnotes
-
E.g. via scapy and the RTPS layer we contributed https://github.com/secdev/scapy/pull/3403 ↩
-
See https://github.com/vmayoral/robot_hacking_manual/tree/master/1_case_studies/2_ros2 for an example. ↩
rep-2012.rst
Outdated
| Only applies if ``publish_service_events`` is ``true``. | ||
| The default value is ``true``. | ||
|
|
||
| By default, the event publishing feautre is off so users do not pay for a feature they do not plan to use. |
There was a problem hiding this comment.
This highlights my concerns above about the need to require access control for remote interactions. Otherwise, this can easily become an entry point from wherein attackers could launch various attack vectors, some as described above.
To launch some constructive ideas, you could consider that publish_client_events and publish_service_events could be somewhat linked to the underlying security capabilities of DDS (through env. variables, configuration files, etc.) and only in selected cases allow them to be true. Implementing this may require additional modifications.
|
|
||
| The service event topics proposed in this REP shall use the default quality of service settings [9]_. | ||
|
|
||
| Security |
There was a problem hiding this comment.
I appreciate the effort in here and like the first paragraph of this section but I don't think the second one (or the security section as a whole) is enough.
First, if SROS2 is used as part of the discussion in here, at the very least, an example should be provided that walks through the process of how to use the proposed service event topics with SROS2 security tools. If there's an ongoing prototype of this capability, such prototype can be easily extended with security as many demonstrators in the past did (e.g. see the one we contributed with TB3 1.
Also, I think this section should at least describe clearly the expected security behavior of the service event topics proposed in the presence of encryption, authorization and authentication (and/or combinations of them). I highlighted before the expected security behavior because as ROS evolve over time, the assumptions in this description should be kept in mind while maintaining this. If such assumptions don't exist, I doubt security concerns will be considered down the road. I would thereby suggest in here a subsection titled security assumptions
Ideally, this security section should start with a small threat model analysis for the particular use case of service event topics considering the existing security capabilities available within DDS and exposed to ROS 2. While doing so, various limitations will be identified and those would need to be discussed. Ideally mitigated but this won't be possible in all cases (due to the inter-dependency on DDS).
Altogether, I'd say that ideally this section should be split in the following form:
- Security
- Threat model of service event topics
- Expected behavior
- Use case example
- Discussion
Footnotes
* Add content for motivation, specification, and rationale. * Feature progress is left as TODO as we've just been prototyping * Add "Other" section will examples of tooling leveraging the proposed feature Co-authored-by: Aditya Pande <[email protected]> Co-authored-by: Brian Chen <[email protected]> Co-authored-by: Deepanshu Bansal <[email protected]> Signed-off-by: Jacob Perron <[email protected]>
* Move metadata into common ServiceEventInfo message * Generate one event message per service containing an optional request or response * Minor rewording Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Trying to make clear the distinction between 'services' as the larger concept and service 'servers' as one half of a service interaction. Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
clalancette
force-pushed
the
jacob/service_introspection
branch
from
53f5083 to
336d3b6
Compare
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-03-16/30432/1 |
| Only supporting one service per name | ||
| ------------------------------------ | ||
|
|
||
| It is technically possible to create more than one service with the same name (though not recommended). | ||
| However, this is generally not recommended and may be forbidden in the future. | ||
| Therefore, as far as this REP is concerned, creating multiple services with the same name is undefined behavior. |
There was a problem hiding this comment.
Would it make sense to mention this is undefined behavior in the specification section since it deals with behavior?
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-4-20-2023/31087/1 |
|
@clalancette I am curious about at what stage this REP-2012? |
|
|
||
| $ ros2 service echo /add_two_ints | ||
| ----------------------- | ||
| request_type: REQUEST_SENT |
There was a problem hiding this comment.
the actual output in the implementation is different from this design. need to update with the following correct one? or this is design, so i guess it does not need to match with current implementation?
$ ros2 service echo --flow-style /add_two_ints
info:
event_type: REQUEST_SENT
stamp:
sec: 1709432402
nanosec: 680094264
client_gid: [1, 15, 0, 18, 86, 208, 115, 86, 0, 0, 0, 0, 0, 0, 21, 3]
sequence_number: 247
request: []
response: []
---
info:
event_type: REQUEST_RECEIVED
stamp:
sec: 1709432402
nanosec: 680459568
client_gid: [1, 15, 0, 18, 86, 208, 115, 86, 0, 0, 0, 0, 0, 0, 20, 4]
sequence_number: 247
request: [{a: 2, b: 3}]
response: []
---
info:
event_type: RESPONSE_SENT
stamp:
sec: 1709432402
nanosec: 680765280
client_gid: [1, 15, 0, 18, 86, 208, 115, 86, 0, 0, 0, 0, 0, 0, 20, 4]
sequence_number: 247
request: []
response: [{sum: 5}]
---
info:
event_type: RESPONSE_RECEIVED
stamp:
sec: 1709432402
nanosec: 681027998
client_gid: [1, 15, 0, 18, 86, 208, 115, 86, 0, 0, 0, 0, 0, 0, 21, 3]
sequence_number: 247
request: []
response: []
---
...| ``rosbag2`` integration for service introspection will come more or less for free since the request/response events are simply being echoed through ROS 2 publishers. | ||
| Syntactic sugar may be included to enable service introspection and record, e.g. ``ros2 bag record --enable-services``. | ||
|
|
||
| Replaying service and client events | ||
| ----------------------------------- | ||
|
|
||
| The design should support implementation of a tool for "replaying" service and client events. | ||
| For example, tooling may be developed to take the recorded event stream and replay requests and responses back into the ROS network. |
There was a problem hiding this comment.
same here, rosbag2 supports the following options now.
-a, --all Record all topics and services (Exclude hidden topic).
--all-services Record all services via service event topics.
-e REGEX, --regex REGEX
Record only topics and services containing provided regular expression. Overrides --all, --all-topics and
--all-services, applies on top of topics list and service list.
--exclude-regex EXCLUDE_REGEX
Exclude topics and services containing provided regular expression. Works on top of --all, --all-topics, or
--regex.
--exclude-services ServiceName [ServiceName ...]
List of services not being recorded. Works on top of --all, --all-services, or --regex.
--services ServiceName [ServiceName ...]
List of services to record.
|
@clalancette there are some unresolved comments, but overall this looks good to me. |
Welcoming feedback from the community.