Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 52 additions & 12 deletions ros2cli/ros2cli/daemon/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import argparse
import os
import time
from urllib.parse import urlparse
from urllib.parse import urlunparse
import uuid

import rclpy
Expand All @@ -31,32 +33,66 @@
from ros2cli.xmlrpc.local_server import SimpleXMLRPCRequestHandler


SERVER_URL_VARIABLE_NAME = 'ROS2_DAEMON_SERVER_URL'
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK, this is not officially documented anywhere, but currently we can only spawn the daemon process with 127.0.0.1 which is localhost loopback physical network IF (with only offset of port number that comes from ROS_DOMAIN_ID). and this fix can expose the daemon XMLRPC API to any IP address that is owned by this host.

although I believe this is more flexible for user to manage the ros2 daemon process in the LAN (for example, we can set SERVER_URL_VARIABLE_NAME to all ROS 2 machines and use the single ros2 daemon server in the entire ROS 2 network. this is useful for resource constrained devices to avoid starting ros2 daemon process in local), this could generate the security concern when we use the security enclaves.
when this ros2 daemon is bound to IP address in the LAN with security enclaves, that exposes the ROS 2 connectivity information to anyone in the LAN via XLRPC. this did not happen before, and there is a possibility that 3rd party remote machine or application can collect this info that is NOT supposed to be seen if administrator is not careful enough. maybe this is just a corner case, but looks like a minor security concern.

after all i think this enhancement makes sense, but we probably do not advertise this environmental variable for end users? if we do that, i think there needs to be some explanation how this works.



def get_xmlrpc_server_url(address=None):
url = urlparse(
os.environ.get(SERVER_URL_VARIABLE_NAME) or '/ros2cli/',
scheme='http')

if address is None:
address = (
url.hostname or '127.0.0.1',
str(
url.port
if url.port not in (None, '')
else (11511 + int(os.environ.get('ROS_DOMAIN_ID', 0)))
),
)

return urlunparse(url._replace(netloc=':'.join(address)))


def get_port():
base_port = 11511
base_port += int(os.environ.get('ROS_DOMAIN_ID', 0))
return base_port
url = get_xmlrpc_server_url()
return urlparse(url).port


def get_address():
return '127.0.0.1', get_port()
url = get_xmlrpc_server_url()
parsed_url = urlparse(url)
return parsed_url.hostname, parsed_url.port


def get_path():
url = get_xmlrpc_server_url()
return urlparse(url).path


class RequestHandler(SimpleXMLRPCRequestHandler):
rpc_paths = ('/ros2cli/',)

class _GetRpcPaths(property):
"""
Getter for the RPC paths value to use on the request handler.

def get_xmlrpc_server_url(address=None):
if not address:
address = get_address()
host, port = address
path = RequestHandler.rpc_paths[0]
return f'http://{host}:{port}{path}'
We need this property to work when accessed from the class reference,
so we can't just use ``@property`` here.
"""

def __get__(self, instance, owner):
return (get_path(),)

rpc_paths = _GetRpcPaths()
Comment on lines +83 to +86
Copy link
Member Author

Choose a reason for hiding this comment

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

A little more explanation on this snippet:

All of the other "getters" for endpoint-related information (get_address, get_port, and get_xmlrpc_server_url) will change if the environment variables change. If we just set the rpc_paths class attribute, it will be set to whatever the value is when the import occurs and won't change.



def make_xmlrpc_server() -> LocalXMLRPCServer:
"""Make local XMLRPC server listening over ros2cli daemon's default port."""
address = get_address()

assert urlparse(get_xmlrpc_server_url()).scheme == 'http', \
'Only http XMLRPC servers are supported at this time.'

return LocalXMLRPCServer(
address, logRequests=False,
requestHandler=RequestHandler,
Expand Down Expand Up @@ -143,7 +179,11 @@ def shutdown_handler():
shutdown = True
server.register_function(shutdown_handler, 'system.shutdown')

print('Serving XML-RPC on ' + get_xmlrpc_server_url(server.server_address))
server_path = server.RequestHandlerClass.rpc_paths[0]
server_hostname, server_port = server.server_address
server_url = f'http://{server_hostname}:{server_port}{server_path}'

print('Serving XML-RPC on ' + server_url)
try:
while rclpy.ok() and not shutdown:
server.handle_request()
Expand Down
32 changes: 30 additions & 2 deletions ros2cli/test/test_ros2cli_daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@
# limitations under the License.

import time
from unittest.mock import patch

import pytest

import rclpy
import rclpy.action

from ros2cli.daemon import SERVER_URL_VARIABLE_NAME
from ros2cli.node.daemon import DaemonNode
from ros2cli.node.daemon import is_daemon_running
from ros2cli.node.daemon import shutdown_daemon
Expand Down Expand Up @@ -103,8 +105,7 @@ def noop_execute_callback(goal_handle):
yield node


@pytest.fixture(scope='module')
def daemon_node():
def _daemon_node():
if is_daemon_running(args=[]):
assert shutdown_daemon(args=[], timeout=5.0)
assert spawn_daemon(args=[], timeout=5.0)
Expand All @@ -128,6 +129,11 @@ def daemon_node():
node.system.shutdown()


@pytest.fixture(scope='module')
def daemon_node():
yield from _daemon_node()


def test_get_name(daemon_node):
assert 'daemon' in daemon_node.get_name()

Expand Down Expand Up @@ -249,3 +255,25 @@ def test_count_clients(daemon_node):

def test_count_services(daemon_node):
assert 1 == daemon_node.count_services(TEST_SERVICE_NAME)


def test_url_override(daemon_node):
# Started by daemon_node
assert is_daemon_running(args=[])

with patch.dict(
'ros2cli.daemon.os.environ',
{SERVER_URL_VARIABLE_NAME: 'http://127.0.0.1:11744/test/'},
):
# No daemon running on that port
assert not is_daemon_running(args=[])

for _ in _daemon_node():
# New daemon running on our custom port
assert is_daemon_running(args=[])

# Custom port daemon is shut down
assert not is_daemon_running(args=[])

# Back to the one started by daemon_node
assert is_daemon_running(args=[])