From 6d9e5f336ff113e57f5514c69e037528ba773c8f Mon Sep 17 00:00:00 2001 From: bburda Date: Wed, 26 Aug 2026 19:55:55 +0200 Subject: [PATCH 1/4] gateway: ship closed by default, and test it by walking the route table The default config had TLS and auth off and require_auth_for set to "write". A deployment that used it got a gateway where anyone who could reach the port could read the entity tree, the fault history and the operations. With auth on, "write" still left every read open, and the reads are the problem: the entity tree has the machine names, the fault history is the maintenance record. config/gateway_params.yaml now sets auth.enabled true, require_auth_for "all" and tls.enabled true. This means the gateway does not start when no signing secret is given. That is on purpose. A gateway that does not start is a config problem you fix in a minute. A gateway that started open is a problem nobody sees. AllAuthRequirementPolicy now exempts the health probe. Its doc comment already said it did, but the code did not. Without the exemption the result is not strict, it is broken: a container supervisor and a load balancer call /health with no credential, so requiring one makes every healthy gateway restart in a loop. The exemption is only GET /api/v1/health, not HEAD and not /health/detail, because a prefix or suffix match would let someone bypass auth by adding the word health to a path. /health also stops returning its full body to a caller with no credential. If a route is public we own what it says, and the full body was not safe to publish: linking.warnings holds strings like "App 'engine_ecu' cannot bind to '/nav/controller'", which is an entity name and a ROS node FQN, and x-medkit-entity-cache holds the counts of apps, areas and components. A caller with no credential now gets status and timestamp only. The check runs before any section is built, so a section added later is private by default. test_closed_by_default.test.py is the main test. It asks the running gateway for its route list and calls all 242 routes with no credential, then again with a fake token. A test that read config values would keep passing on the day someone registers a route outside the policy. Walking the registry does not, and a route added later is covered on the day it is added. Two unit tests cover the edges of the exemption as fast regression cover. Also fixed here because this branch hit it: test_peer_recovery test_07 waited for HTTP 200 and then checked the body had live data. Recovery has two steps, the route comes back and then a sample arrives on the new subscription, and between them the read returns 200 with status "metadata_only" and an empty body. Under load the poll returned a response from that window. It now waits for the state it checks. gateway.launch.py takes jwt_secret, auth_clients and auth_enabled, and prints what to pass when auth is on and no secret was given. The docs stop showing unauthenticated curl: getting_started explains the credential once and every example sends it, with /health left bare because it is the exemption. hardening.rst describes the current defaults. Do not merge alone: the appliance deployment profiles that rely on this exemption ship separately and expect it. --- README.md | 4 +- docs/api/rest.rst | 10 +- docs/config/discovery-options.rst | 2 +- docs/getting_started.rst | 55 +++- docs/index.rst | 23 +- docs/tutorials/beacon-discovery.rst | 4 +- docs/tutorials/demos/demo-sensor.rst | 16 +- docs/tutorials/demos/demo-turtlebot3.rst | 18 +- docs/tutorials/docker.rst | 2 +- docs/tutorials/fault-correlation.rst | 2 +- docs/tutorials/graph-provider.rst | 6 +- docs/tutorials/heuristic-apps.rst | 14 +- docs/tutorials/linux-introspection.rst | 12 +- docs/tutorials/locking.rst | 2 +- docs/tutorials/manifest-discovery.rst | 8 +- docs/tutorials/migration-to-manifest.rst | 4 +- docs/tutorials/openapi.rst | 8 +- docs/tutorials/scripts.rst | 2 +- docs/tutorials/snapshots.rst | 10 +- docs/tutorials/triggers-use-cases.rst | 12 +- .../config/gateway_params.yaml | 39 ++- src/ros2_medkit_gateway/design/hardening.rst | 52 +++- .../core/auth/auth_requirement_policy.hpp | 25 +- .../launch/gateway.launch.py | 45 +++ .../test/test_auth_manager.cpp | 45 +++ .../features/test_closed_by_default.test.py | 271 ++++++++++++++++++ .../test/features/test_peer_recovery.test.py | 13 +- 27 files changed, 589 insertions(+), 115 deletions(-) create mode 100644 src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py diff --git a/README.md b/README.md index 532571891..5e0db3e7e 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@ corner - and the only trace is a line buried in a log you would have to SSH in t Start ros2_medkit next to it (no changes to Nav2). The aborted goal becomes a fault: ```bash -curl http://localhost:8080/api/v1/apps/bt_navigator/faults +curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/bt_navigator/faults # → ACTION_NAVIGATE_TO_POSE_ABORTED severity=ERROR source=/bt_navigator status=CONFIRMED # + a black-box rosbag of the seconds around the failure ``` @@ -51,7 +51,7 @@ trajectory. Same story: the same action bridge surfaces the aborted move as a fa `move_group` entity, with the freeze-frame of what the arm was doing, without touching MoveIt. ```bash -curl http://localhost:8080/api/v1/apps/move_group/faults +curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/move_group/faults # → the aborted MoveGroup goal, as a structured fault with its snapshot ``` diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 694f891aa..3b5013acf 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -896,7 +896,7 @@ Read and publish data from ROS 2 topics. Item ids follow .. code-block:: bash - curl http://localhost:8080/api/v1/components/temp_sensor/data/powertrain%2Fengine%2Ftemperature + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/temp_sensor/data/powertrain%2Fengine%2Ftemperature ``PUT /api/v1/components/{id}/data/{topic_path}`` Publish to a topic. @@ -1630,7 +1630,7 @@ List available bulk-data categories for an entity. Returns the union of rosbag c .. code-block:: bash - curl http://localhost:8080/api/v1/apps/motor_controller/bulk-data + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/motor_controller/bulk-data **Response (200 OK):** @@ -1651,7 +1651,7 @@ List all bulk-data items in a category for the entity. .. code-block:: bash - curl http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags **Response (200 OK):** @@ -2910,7 +2910,7 @@ topic (push-based). .. code-block:: bash - curl http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-topic-beacon + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-topic-beacon **Response (200 OK):** @@ -2965,7 +2965,7 @@ polling ROS 2 node parameters matching a configured prefix (pull-based). .. code-block:: bash - curl http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-param-beacon + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-param-beacon **Response (200 OK):** diff --git a/docs/config/discovery-options.rst b/docs/config/discovery-options.rst index c47613dc0..d8672ac2f 100644 --- a/docs/config/discovery-options.rst +++ b/docs/config/discovery-options.rst @@ -472,7 +472,7 @@ The ``x-medkit-topic-beacon`` vendor endpoint exposes current beacon state: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-topic-beacon + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-topic-beacon **Example Response:** diff --git a/docs/getting_started.rst b/docs/getting_started.rst index 7ee1ccd57..855b0d7f3 100644 --- a/docs/getting_started.rst +++ b/docs/getting_started.rst @@ -40,7 +40,9 @@ Open three terminals. In each, source your workspace: .. code-block:: bash - ros2 launch ros2_medkit_gateway gateway.launch.py + ros2 launch ros2_medkit_gateway gateway.launch.py \ + jwt_secret:=change-me-to-at-least-32-characters-long \ + auth_clients:=demo:demo-secret:admin You should see: @@ -122,6 +124,33 @@ Required if you want to test the Faults API. The ``~/.ros2_medkit/`` directory must exist before starting the fault manager. SQLite will create the database file automatically. +.. important:: + + The gateway ships **closed**: ``auth.enabled`` is true and + ``require_auth_for`` is ``all``, so every route below needs a credential and + the gateway will not start without a signing secret. That is deliberate - a + gateway that booted open is one nobody notices. The two arguments above are + a throwaway development credential; a real deployment injects them from its + own secret store. + + Get a token once and reuse it for every command on this page: + + .. code-block:: bash + + TOKEN=$(curl -s http://localhost:8080/api/v1/auth/authorize \ + -H 'Content-Type: application/json' \ + -d '{"grant_type":"client_credentials","client_id":"demo","client_secret":"demo-secret"}' \ + | jq -r .access_token) + + ``POST /api/v1/auth/authorize`` takes the ``client_credentials`` grant; + ``/auth/token`` is the refresh endpoint and takes ``refresh_token``. + + ``GET /api/v1/health`` is the other route that stays open, so a container + supervisor with no credential can still tell the process is alive. + + To run without authentication - only on a host nothing else can reach - + pass ``auth_enabled:=false``. + .. admonition:: ✅ Checkpoint :class: tip @@ -146,7 +175,7 @@ The gateway exposes all endpoints under ``/api/v1``. Let's explore! .. code-block:: bash - curl http://localhost:8080/api/v1/ + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/ Response shows available endpoints and version info. @@ -197,7 +226,7 @@ ros2_medkit organizes ROS 2 nodes into a SOVD-aligned entity hierarchy: .. code-block:: bash - curl http://localhost:8080/api/v1/functions + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions With ``demo_nodes.launch.py``, you'll see Functions like ``powertrain``, ``chassis``, and ``body`` (created from the first namespace segment). @@ -206,7 +235,7 @@ With ``demo_nodes.launch.py``, you'll see Functions like ``powertrain``, ``chass .. code-block:: bash - curl http://localhost:8080/api/v1/components + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components In runtime mode, you'll see a single host-level Component. @@ -214,7 +243,7 @@ In runtime mode, you'll see a single host-level Component. .. code-block:: bash - curl http://localhost:8080/api/v1/areas + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/areas In runtime mode, this returns an empty list. Areas require a manifest definition (see :doc:`tutorials/manifest-discovery`). @@ -228,7 +257,7 @@ The data endpoints let you read topic data from apps. .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/data + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/data Response structure (showing one topic): @@ -282,7 +311,7 @@ Each data item includes: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/data/powertrain%2Fengine%2Ftemperature + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/data/powertrain%2Fengine%2Ftemperature Response with live data: @@ -339,7 +368,7 @@ The operations endpoints let you call ROS 2 services and actions. .. code-block:: bash - curl http://localhost:8080/api/v1/apps/calibration/operations + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/calibration/operations **Call a service (synchronous execution):** @@ -387,7 +416,7 @@ Response (202 Accepted): .. code-block:: bash - curl http://localhost:8080/api/v1/apps/long_calibration/operations/long_calibration/executions/a1b2c3d4-e5f6-7890-abcd-ef1234567890 + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/long_calibration/operations/long_calibration/executions/a1b2c3d4-e5f6-7890-abcd-ef1234567890 **Cancel a running action:** @@ -406,13 +435,13 @@ The configurations endpoints expose ROS 2 parameters. .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/configurations + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/configurations **Get a specific parameter:** .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/configurations/publish_rate + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/configurations/publish_rate **Set a parameter value:** @@ -439,13 +468,13 @@ Step 7: Monitor Faults .. code-block:: bash - curl http://localhost:8080/api/v1/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/faults **List faults for a specific component:** .. code-block:: bash - curl http://localhost:8080/api/v1/apps/lidar_sensor/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/lidar_sensor/faults **Clear a fault:** diff --git a/docs/index.rst b/docs/index.rst index 1adf85ec5..6ebf46e35 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -52,29 +52,36 @@ Common commands for quick access: # Check gateway health curl http://localhost:8080/api/v1/health +.. note:: + + The gateway ships closed: every route below needs a credential, and + ``GET /api/v1/health`` is the only one that does not. See + :doc:`getting_started` for how to obtain ``$TOKEN``. + + # List all areas - curl http://localhost:8080/api/v1/areas + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/areas # List all components - curl http://localhost:8080/api/v1/components + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components # List all apps - curl http://localhost:8080/api/v1/apps + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps # List all functions - curl http://localhost:8080/api/v1/functions + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions # Get data from an entity (area, component, app, or function) - curl http://localhost:8080/api/v1/{entity-type}/{entity-id}/data + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/{entity-type}/{entity-id}/data # List operations for an entity (area, component, app, or function) - curl http://localhost:8080/api/v1/{entity-type}/{entity-id}/operations + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/{entity-type}/{entity-id}/operations # Get configurations (parameters) - curl http://localhost:8080/api/v1/{entity-type}/{entity-id}/configurations + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/{entity-type}/{entity-id}/configurations # List faults - curl http://localhost:8080/api/v1/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/faults Community --------- diff --git a/docs/tutorials/beacon-discovery.rst b/docs/tutorials/beacon-discovery.rst index 43e0ad13d..b181a40e8 100644 --- a/docs/tutorials/beacon-discovery.rst +++ b/docs/tutorials/beacon-discovery.rst @@ -259,7 +259,7 @@ The plugin registers a vendor extension endpoint on all apps and components: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-topic-beacon + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-topic-beacon Example response: @@ -460,7 +460,7 @@ The plugin registers its own vendor extension endpoint: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-param-beacon + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/engine_temp_sensor/x-medkit-param-beacon The response format is identical to the topic beacon endpoint. diff --git a/docs/tutorials/demos/demo-sensor.rst b/docs/tutorials/demos/demo-sensor.rst index b2cf6087b..a0bf1343f 100644 --- a/docs/tutorials/demos/demo-sensor.rst +++ b/docs/tutorials/demos/demo-sensor.rst @@ -107,16 +107,16 @@ Query sensor data via REST API: .. code-block:: bash # Get LiDAR scan - curl http://localhost:8080/api/v1/apps/lidar-sim/data/scan | jq '.ranges[:5]' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/lidar-sim/data/scan | jq '.ranges[:5]' # Get IMU data - curl http://localhost:8080/api/v1/apps/imu-sim/data/imu | jq '.linear_acceleration' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/imu-sim/data/imu | jq '.linear_acceleration' # Get GPS fix - curl http://localhost:8080/api/v1/apps/gps-sim/data/fix | jq '{lat: .latitude, lon: .longitude}' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/gps-sim/data/fix | jq '{lat: .latitude, lon: .longitude}' # Get camera image info - curl http://localhost:8080/api/v1/apps/camera-sim/data/image | jq '{width, height, encoding}' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/camera-sim/data/image | jq '{width, height, encoding}' Managing Configurations ----------------------- @@ -128,10 +128,10 @@ View and modify sensor parameters: .. code-block:: bash # List all LiDAR configurations - curl http://localhost:8080/api/v1/apps/lidar-sim/configurations | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/lidar-sim/configurations | jq # Get specific parameter - curl http://localhost:8080/api/v1/apps/lidar-sim/configurations/noise_stddev | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/lidar-sim/configurations/noise_stddev | jq # Change scan rate curl -X PUT http://localhost:8080/api/v1/apps/lidar-sim/configurations/scan_rate \ @@ -176,10 +176,10 @@ faults at runtime using provided scripts: .. code-block:: bash # List all system faults - curl http://localhost:8080/api/v1/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/faults # Get faults for specific sensor - curl http://localhost:8080/api/v1/apps/lidar-sim/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/lidar-sim/faults **Manual fault injection via API:** diff --git a/docs/tutorials/demos/demo-turtlebot3.rst b/docs/tutorials/demos/demo-turtlebot3.rst index 24c609b08..ff3340e36 100644 --- a/docs/tutorials/demos/demo-turtlebot3.rst +++ b/docs/tutorials/demos/demo-turtlebot3.rst @@ -90,7 +90,7 @@ Querying via API: .. code-block:: bash - curl http://localhost:8080/api/v1/areas | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/areas | jq .. figure:: /_static/images/13_curl_areas_turtlebot3.png :alt: Areas response @@ -116,13 +116,13 @@ Query data via REST API: .. code-block:: bash # List all apps - curl http://localhost:8080/api/v1/apps | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps | jq # Get specific topic from AMCL localization - curl http://localhost:8080/api/v1/apps/amcl/data | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/amcl/data | jq # Get specific topic from controller server - curl http://localhost:8080/api/v1/apps/controller-server/data | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/controller-server/data | jq .. figure:: /_static/images/06_topic_data_view.png :alt: Topic data view @@ -148,10 +148,10 @@ You can also interact with the navigation stack via API: .. code-block:: bash # List operations on BT Navigator - curl http://localhost:8080/api/v1/apps/bt-navigator/operations | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/bt-navigator/operations | jq # List operations on Controller Server - curl http://localhost:8080/api/v1/apps/controller-server/operations | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/controller-server/operations | jq Managing Parameters ------------------- @@ -163,10 +163,10 @@ View and modify parameters: .. code-block:: bash # List all configurations for AMCL - curl http://localhost:8080/api/v1/apps/amcl/configurations | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/amcl/configurations | jq # Get specific parameter - curl http://localhost:8080/api/v1/apps/amcl/configurations/use_sim_time | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/amcl/configurations/use_sim_time | jq # Change parameter value curl -X PUT http://localhost:8080/api/v1/apps/amcl/configurations/use_sim_time \ @@ -199,7 +199,7 @@ The demo includes fault injection scripts to test diagnostic capabilities: ./check-faults.sh # Or query via API - curl http://localhost:8080/api/v1/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/faults .. figure:: /_static/images/18_faults_injected_dashboard.png :alt: Faults dashboard diff --git a/docs/tutorials/docker.rst b/docs/tutorials/docker.rst index 84c042e67..7840f613f 100644 --- a/docs/tutorials/docker.rst +++ b/docs/tutorials/docker.rst @@ -61,7 +61,7 @@ Test the gateway: curl http://localhost:8080/api/v1/health # {"status":"healthy","timestamp":...} - curl http://localhost:8080/api/v1/version-info + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/version-info # {"items":[{"version":"","vendor_info":{"name":"ros2_medkit",...}}]} Custom Configuration diff --git a/docs/tutorials/fault-correlation.rst b/docs/tutorials/fault-correlation.rst index 07357780c..ca3d6cc4d 100644 --- a/docs/tutorials/fault-correlation.rst +++ b/docs/tutorials/fault-correlation.rst @@ -310,7 +310,7 @@ Querying Correlation Data .. code-block:: bash - curl http://localhost:8080/api/v1/faults + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/faults Response always includes: diff --git a/docs/tutorials/graph-provider.rst b/docs/tutorials/graph-provider.rst index eb305285f..006b95c66 100644 --- a/docs/tutorials/graph-provider.rst +++ b/docs/tutorials/graph-provider.rst @@ -211,14 +211,14 @@ The Discovery Path .. code-block:: bash - curl http://localhost:8080/api/v1/functions | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions | jq 2. **Read the Function's detail** and follow its capability href. Every Function detail response carries an ``"x-medkit-graph"`` link: .. code-block:: bash - curl http://localhost:8080/api/v1/functions/engine-monitoring | jq '."x-medkit-graph"' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions/engine-monitoring | jq '."x-medkit-graph"' # "/api/v1/functions/engine-monitoring/x-medkit-graph" 3. **GET the graph** at that href. @@ -233,7 +233,7 @@ Function ``engine-monitoring`` hosting an ``engine-temp-sensor`` App (publishes .. code-block:: bash - curl http://localhost:8080/api/v1/functions/engine-monitoring/x-medkit-graph | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions/engine-monitoring/x-medkit-graph | jq .. code-block:: json diff --git a/docs/tutorials/heuristic-apps.rst b/docs/tutorials/heuristic-apps.rst index 8d4b74638..638d98d5e 100644 --- a/docs/tutorials/heuristic-apps.rst +++ b/docs/tutorials/heuristic-apps.rst @@ -55,7 +55,7 @@ Query available Apps: .. code-block:: bash - curl http://localhost:8080/api/v1/apps | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps | jq Example response: @@ -120,16 +120,16 @@ In runtime mode, the gateway maps the ROS 2 graph as follows: .. code-block:: bash - curl http://localhost:8080/api/v1/apps + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps # Returns: [{"id": "lidar_driver"}, {"id": "camera_node"}] - curl http://localhost:8080/api/v1/components + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components # Returns: [{"id": "my-hostname", "source": "runtime", ...}] - curl http://localhost:8080/api/v1/functions + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions # Returns: [{"id": "perception"}, {"id": "navigation"}] - curl http://localhost:8080/api/v1/areas + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/areas # Returns: {"items": []} (empty - Areas come from manifest only) API Endpoints @@ -159,7 +159,7 @@ Component derived from system information: .. code-block:: bash - curl http://localhost:8080/api/v1/components | jq '.items[] | {id, source}' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components | jq '.items[] | {id, source}' .. code-block:: json @@ -177,7 +177,7 @@ In runtime mode, Functions are created from the first namespace segment: .. code-block:: bash - curl http://localhost:8080/api/v1/functions | jq '.items[] | {id}' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions | jq '.items[] | {id}' .. code-block:: json diff --git a/docs/tutorials/linux-introspection.rst b/docs/tutorials/linux-introspection.rst index 50d8938cd..6e9c340f2 100644 --- a/docs/tutorials/linux-introspection.rst +++ b/docs/tutorials/linux-introspection.rst @@ -112,7 +112,7 @@ Returns process-level metrics for a single ROS 2 node: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/x-medkit-procfs | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/x-medkit-procfs | jq .. code-block:: json @@ -136,7 +136,7 @@ Each entry includes a ``node_ids`` array listing the Apps that share the process .. code-block:: bash - curl http://localhost:8080/api/v1/components/sensor_suite/x-medkit-procfs | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/sensor_suite/x-medkit-procfs | jq .. code-block:: json @@ -167,7 +167,7 @@ Returns the systemd unit managing the node's process: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/x-medkit-systemd | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/x-medkit-systemd | jq .. code-block:: json @@ -186,7 +186,7 @@ Returns aggregated unit info for all child Apps, deduplicated by unit name: .. code-block:: bash - curl http://localhost:8080/api/v1/components/sensor_suite/x-medkit-systemd | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/sensor_suite/x-medkit-systemd | jq .. code-block:: json @@ -213,7 +213,7 @@ Returns container metadata for a node running inside a container: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/x-medkit-container | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/x-medkit-container | jq .. code-block:: json @@ -249,7 +249,7 @@ Returns aggregated container info for all child Apps, deduplicated by container .. code-block:: bash - curl http://localhost:8080/api/v1/components/sensor_suite/x-medkit-container | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/sensor_suite/x-medkit-container | jq .. code-block:: json diff --git a/docs/tutorials/locking.rst b/docs/tutorials/locking.rst index acfb2639c..00901525f 100644 --- a/docs/tutorials/locking.rst +++ b/docs/tutorials/locking.rst @@ -117,7 +117,7 @@ Check what locks exist on an entity: .. code-block:: bash - curl http://localhost:8080/api/v1/components/motor_controller/locks \ + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/motor_controller/locks \ -H "X-Client-Id: $CLIENT_ID" The ``owned`` field in each lock item indicates whether the requesting client diff --git a/docs/tutorials/manifest-discovery.rst b/docs/tutorials/manifest-discovery.rst index 83e0e9f35..14295f1c4 100644 --- a/docs/tutorials/manifest-discovery.rst +++ b/docs/tutorials/manifest-discovery.rst @@ -173,7 +173,7 @@ Check manifest status: .. code-block:: bash - curl http://localhost:8080/api/v1/manifest/status + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/manifest/status Expected response: @@ -195,13 +195,13 @@ List apps: .. code-block:: bash - curl http://localhost:8080/api/v1/apps + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps List functions: .. code-block:: bash - curl http://localhost:8080/api/v1/functions + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/functions Understanding Hybrid Mode ------------------------- @@ -336,7 +336,7 @@ Check which apps are online: .. code-block:: bash - curl http://localhost:8080/api/v1/apps | jq '.items[] | {id, name, is_online}' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps | jq '.items[] | {id, name, is_online}' Example response: diff --git a/docs/tutorials/migration-to-manifest.rst b/docs/tutorials/migration-to-manifest.rst index ea7024657..9f8752b65 100644 --- a/docs/tutorials/migration-to-manifest.rst +++ b/docs/tutorials/migration-to-manifest.rst @@ -323,13 +323,13 @@ Step 7: Test in Hybrid Mode .. code-block:: bash - curl http://localhost:8080/api/v1/manifest/status | jq + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/manifest/status | jq 4. **Verify apps are linked**: .. code-block:: bash - curl http://localhost:8080/api/v1/apps | jq '.items[] | {id, name, is_online}' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps | jq '.items[] | {id, name, is_online}' 5. **Check for orphan nodes** (warnings in gateway logs): diff --git a/docs/tutorials/openapi.rst b/docs/tutorials/openapi.rst index 053c9f892..6e71db8c5 100644 --- a/docs/tutorials/openapi.rst +++ b/docs/tutorials/openapi.rst @@ -15,16 +15,16 @@ Append ``/docs`` to any valid API path: .. code-block:: bash # Full gateway spec (all endpoints) - curl http://localhost:8080/api/v1/docs | jq . + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/docs | jq . # Spec scoped to the components collection - curl http://localhost:8080/api/v1/components/docs + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/docs # Spec for a specific component and its resource collections - curl http://localhost:8080/api/v1/components/my_sensor/docs + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/my_sensor/docs # Spec for one resource collection (e.g. data) - curl http://localhost:8080/api/v1/components/my_sensor/data/docs + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/my_sensor/data/docs Entity-level specs reflect the actual capabilities of each entity at runtime. Plugin-registered vendor routes also appear when the requested diff --git a/docs/tutorials/scripts.rst b/docs/tutorials/scripts.rst index e395ebf33..48b0c8b2d 100644 --- a/docs/tutorials/scripts.rst +++ b/docs/tutorials/scripts.rst @@ -82,7 +82,7 @@ Quick Example .. code-block:: bash - curl http://localhost:8080/api/v1/components/main-computer/scripts/script_1717123456_0/executions/exec_1717123500_0 + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/main-computer/scripts/script_1717123456_0/executions/exec_1717123500_0 Response when finished: diff --git a/docs/tutorials/snapshots.rst b/docs/tutorials/snapshots.rst index ac8daa2fb..6263758ed 100644 --- a/docs/tutorials/snapshots.rst +++ b/docs/tutorials/snapshots.rst @@ -56,7 +56,7 @@ Quick Start .. code-block:: bash - curl http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots Configuration Options --------------------- @@ -241,7 +241,7 @@ Snapshots are included inline in the fault response as ``environment_data``: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/motor_controller/faults/MOTOR_OVERHEAT + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/motor_controller/faults/MOTOR_OVERHEAT **Response:** @@ -307,7 +307,7 @@ Snapshots are included inline in the fault response as ``environment_data``: .. code-block:: bash - curl http://localhost:8080/api/v1/apps/motor_controller/faults/MOTOR_OVERHEAT | \ + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/motor_controller/faults/MOTOR_OVERHEAT | \ jq '.environment_data.snapshots' Example Workflow @@ -342,7 +342,7 @@ This example demonstrates the complete snapshot capture workflow. .. code-block:: bash - curl http://localhost:8080/api/v1/apps/nav_node/faults/NAV_ERROR | \ + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/nav_node/faults/NAV_ERROR | \ jq '.environment_data.snapshots' The response will contain the odometry data that was captured at the @@ -728,7 +728,7 @@ Rosbag files are downloaded via SOVD bulk-data endpoints. .. code-block:: bash - curl http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags One item per **recording**, not per fault. A burst of correlated faults shares a single recording and appears once, with every fault it covers listed in diff --git a/docs/tutorials/triggers-use-cases.rst b/docs/tutorials/triggers-use-cases.rst index fb68119b6..63b1040fb 100644 --- a/docs/tutorials/triggers-use-cases.rst +++ b/docs/tutorials/triggers-use-cases.rst @@ -119,7 +119,7 @@ Step 4: List all triggers .. code-block:: bash - curl http://localhost:8080/api/v1/apps/temp_sensor/triggers + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/triggers **Response:** @@ -281,7 +281,7 @@ component. .. code-block:: bash - curl http://localhost:8080/api/v1/components | jq '.items[].id' + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components | jq '.items[].id' If ``engine`` does not appear, the demo nodes may not be running or the namespace grouping may differ. Adjust ``engine`` to match the actual @@ -306,10 +306,10 @@ Step 4: Verify all triggers .. code-block:: bash # App-level triggers - curl http://localhost:8080/api/v1/apps/temp_sensor/triggers + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/temp_sensor/triggers # Component-level triggers - curl http://localhost:8080/api/v1/components/engine/triggers + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/components/engine/triggers Step 5: Connect SSE streams and observe cascade ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -424,10 +424,10 @@ Step 4: Verify triggers on different entity types .. code-block:: bash # Area-level triggers - curl http://localhost:8080/api/v1/areas/powertrain/triggers + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/areas/powertrain/triggers # App-level triggers - curl http://localhost:8080/api/v1/apps/engine-temp-sensor/triggers + curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/apps/engine-temp-sensor/triggers Step 5: Connect SSE streams ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index dec314749..db8a6e630 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -80,8 +80,14 @@ ros2_medkit_gateway: # TLS/HTTPS Configuration # Enables encrypted communication using OpenSSL tls: - # Enable/disable TLS (default: false for backward compatibility) - enabled: false + # On, so a gateway reachable from anything but loopback is encrypted + # without a deployment having to remember to turn it on. cert_file and + # key_file below must be filled in; the gateway refuses to start with + # TLS on and no certificate, which is the intended failure - an + # unencrypted gateway that started anyway is the worse outcome. + # Turn OFF only for a gateway bound to 127.0.0.1 behind a TLS + # terminator that is itself doing this job. + enabled: true # Path to PEM-encoded certificate file (required when TLS enabled) # Example: "/etc/ros2_medkit/certs/cert.pem" @@ -360,11 +366,22 @@ ros2_medkit_gateway: # Authentication Configuration (REQ_INTEROP_086, REQ_INTEROP_087) # JWT-based authentication with Role-Based Access Control (RBAC) auth: - # Enable/disable authentication - # Default: false (disabled for local development) - enabled: false + # On. This file is the default a deployment gets when it brings no + # profile of its own, so it has to be the safe one: an unauthenticated + # SOVD gateway exposes the entity tree, the fault history and every + # operation the plugins register. + # + # The gateway REFUSES TO START while this is true and jwt_secret is + # empty (auth_config.cpp: "JWT secret is required when authentication is + # enabled"). That is deliberate. A gateway that will not boot is a + # deployment problem someone fixes in a minute; a gateway that booted + # open is one nobody notices. + enabled: true - # JWT signing secret (required when enabled) + # JWT signing secret. REQUIRED - the gateway will not start without it + # while auth.enabled is true. At least 32 characters for HS256. + # Inject it from a secret store or the deployment's own configuration; + # do not commit a real secret here. # For HS256: The shared secret string # For RS256: Path to the private key file (PEM format) jwt_secret: "" @@ -390,8 +407,14 @@ ros2_medkit_gateway: # - "none": No authentication required (auth endpoints still available) # - "write": Auth required for write operations (POST, PUT, DELETE) # - "all": Auth required for all operations - # Default: "write" - require_auth_for: "write" + # + # "all", not "write". Under "write" every read stays open even with + # authentication switched on, and the reads are where the disclosure is: + # the entity tree names the machines, the fault history is the + # maintenance record. Both are readable by anyone who can reach the port. + # GET /health and /auth/* stay public under "all" - see + # AllAuthRequirementPolicy, which documents why each one has to be. + require_auth_for: "all" # JWT issuer claim # Default: "ros2_medkit_gateway" diff --git a/src/ros2_medkit_gateway/design/hardening.rst b/src/ros2_medkit_gateway/design/hardening.rst index 16311c0f3..4f4630980 100644 --- a/src/ros2_medkit_gateway/design/hardening.rst +++ b/src/ros2_medkit_gateway/design/hardening.rst @@ -1,15 +1,28 @@ Gateway hardening (secure field profile) ======================================== -The gateway ships every transport and access control needed for a hardened -deployment - JWT authentication with RBAC, TLS/HTTPS, restricted CORS, and -token-bucket rate limiting - but they are **disabled by default** so local -development works out of the box. A gateway exposed on a plant network with the -defaults is wide open: unauthenticated reads and writes over cleartext HTTP. - -For any deployment reachable from an untrusted network, start from the secure -field profile preset ``config/gateway_params.secure.yaml`` instead of -``config/gateway_params.yaml``: +The gateway ships **closed**. ``config/gateway_params.yaml`` sets +``auth.enabled: true``, ``auth.require_auth_for: "all"`` and +``server.tls.enabled: true``, so out of the box every route needs a credential +and the transport is encrypted. + +Two consequences worth stating plainly: + +* **The gateway refuses to start without a signing secret.** With auth enabled + and ``auth.jwt_secret`` empty it exits with "JWT secret is required when + authentication is enabled" (and HS256 additionally requires at least 32 + characters). This is intended. A gateway that will not boot is a deployment + problem someone fixes in a minute; a gateway that booted open is one nobody + notices. +* **``GET /api/v1/health`` and ``/api/v1/auth/*`` stay public.** Health so a + container supervisor or load balancer with no credential can tell the + process is alive - it carries a fixed status document and no topology. Auth + because authentication cannot bootstrap through a door that already demands + the credential it exists to hand out. Nothing else is exempt. + +For a deployment reachable from an untrusted network, the remaining controls - +restricted CORS, rate limiting, locking, a reduced surface - are collected in +the secure field profile preset ``config/gateway_params.secure.yaml``: .. code-block:: bash @@ -24,9 +37,10 @@ What the secure profile turns on ================================ ============== =========================================== Control Default Secure profile ================================ ============== =========================================== -``auth.enabled`` false true -``auth.require_auth_for`` write all (auth on reads + writes) -``server.tls.enabled`` false true (HTTPS, min TLS 1.3) +``auth.enabled`` true true +``auth.require_auth_for`` all all (auth on reads + writes) +``server.tls.enabled`` true true (HTTPS, min TLS 1.3) +``auth.jwt_secret`` *unset* *unset* - both REQUIRE one at deploy time ``cors.allowed_origins`` ``[]`` explicit origin list (no wildcard) ``rate_limiting.enabled`` false true (global + per-client + per-endpoint) ``scripts.allow_uploads`` true false (manifest-defined scripts only) @@ -35,6 +49,20 @@ Control Default Secure profile ``locking`` on operations none lock required before mutation ================================ ============== =========================================== +The access-control rows now match: the difference between the two files is the +surface reduction below them, not whether the door is locked. + +Running without authentication +------------------------------ + +On a host nothing else can reach - a laptop, a CI job, a single-container demo +- pass ``auth_enabled:=false`` to ``gateway.launch.py``, or set +``auth.enabled: false`` in your own params file. Do this deliberately and never +on a machine reachable from a plant or office network: with authentication off +the entity tree names the machines, the fault history is the maintenance record +of the line, and every registered operation is callable by anyone who can reach +the port. + Credential and certificate provisioning ---------------------------------------- diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp index 6e25328eb..d724cf622 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp @@ -69,18 +69,33 @@ class NoAuthRequirementPolicy : public IAuthRequirementPolicy { /** * @brief Policy that always requires authentication * - * Except for public endpoints (auth endpoints, health check) + * Except for two public endpoints: the auth endpoints, and the health probe. */ class AllAuthRequirementPolicy : public IAuthRequirementPolicy { public: bool requires_authentication(const std::string & method, const std::string & path) const override { - (void)method; - // Auth endpoints are always public (to allow login) - return path.find("/api/v1/auth/") != 0; + // Auth endpoints are always public: authentication cannot bootstrap + // through a door that demands the credential it exists to hand out. + if (path.find("/api/v1/auth/") == 0) { + return false; + } + + // GET /api/v1/health is public. A container supervisor and an upstream + // load balancer probe it to decide whether this process is alive, and + // neither holds a credential; requiring one turns a healthy gateway into + // a restart loop. The response is a fixed status document - it names no + // entity, no topic and no plant data - so leaving it open discloses that + // a gateway is running and nothing further. HEAD is not included: a probe + // that wants the status document asks for it with GET. + if (method == "GET" && path == "/api/v1/health") { + return false; + } + + return true; } std::string description() const override { - return "AllAuth: Authentication required for all endpoints except /auth/*"; + return "AllAuth: Authentication required for all endpoints except /auth/* and GET /health"; } }; diff --git a/src/ros2_medkit_gateway/launch/gateway.launch.py b/src/ros2_medkit_gateway/launch/gateway.launch.py index 0da999c7f..d802b14dc 100644 --- a/src/ros2_medkit_gateway/launch/gateway.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway.launch.py @@ -94,6 +94,30 @@ def generate_launch_description(): 'controls the periodic forced refresh. Must match the default ' 'in config/gateway_params.yaml.')) + declare_jwt_secret_arg = DeclareLaunchArgument( + 'jwt_secret', default_value='', + description=( + 'HS256 signing secret, at least 32 characters. REQUIRED: the ' + 'shipped config has auth.enabled true, and the gateway refuses to ' + 'start without a secret. Pass one here, or point config_file at a ' + 'file that sets auth.jwt_secret and auth.clients. To run without ' + 'authentication - only ever on a host nothing else can reach - ' + 'pass auth_enabled:=false explicitly.')) + + declare_auth_enabled_arg = DeclareLaunchArgument( + 'auth_enabled', default_value='true', + description=( + 'Require a credential. On by default, matching the shipped ' + 'config. Turning it off makes the entity tree, the fault history ' + 'and every operation readable by anyone who can reach the port.')) + + declare_clients_arg = DeclareLaunchArgument( + 'auth_clients', default_value='', + description=( + 'Comma-separated "client_id:client_secret:role" triples ' + '(roles: viewer, operator, configurator, admin). Needed to obtain ' + 'a token from /auth/token.')) + declare_cors_arg = DeclareLaunchArgument( 'cors_allowed_origins', default_value=CORS_DEFAULT, @@ -120,6 +144,24 @@ def _launch_setup(context, *_args, **_kwargs): param_overrides.update(cors_override( LaunchConfiguration('cors_allowed_origins').perform(context), LaunchConfiguration('config_file').perform(context), default_config)) + + auth_enabled = LaunchConfiguration('auth_enabled').perform(context).lower() in ( + 'true', '1', 'yes') + param_overrides['auth.enabled'] = auth_enabled + jwt_secret = LaunchConfiguration('jwt_secret').perform(context) + clients = LaunchConfiguration('auth_clients').perform(context) + if jwt_secret: + param_overrides['auth.jwt_secret'] = jwt_secret + if clients: + param_overrides['auth.clients'] = [c for c in clients.split(',') if c] + if auth_enabled and not jwt_secret: + # The gateway would refuse to start a moment from now with a + # message about the config file. Say the actionable thing instead, + # here, where the launch argument that fixes it is in scope. + print('[gateway.launch.py] auth is enabled and no jwt_secret was given. ' + 'Pass jwt_secret:= and ' + 'auth_clients:=::admin, set them in a config_file, ' + 'or pass auth_enabled:=false to run without authentication.') return [Node( package='ros2_medkit_gateway', executable='gateway_node', @@ -133,6 +175,9 @@ def _launch_setup(context, *_args, **_kwargs): declare_host_arg, declare_port_arg, declare_refresh_arg, + declare_auth_enabled_arg, + declare_jwt_secret_arg, + declare_clients_arg, declare_cors_arg, OpaqueFunction(function=_launch_setup), ]) diff --git a/src/ros2_medkit_gateway/test/test_auth_manager.cpp b/src/ros2_medkit_gateway/test/test_auth_manager.cpp index 8cb883ff5..a71a7993c 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -1033,6 +1033,51 @@ TEST_F(AuthRequirementPolicyTest, AllAuthPolicyAlwaysRequiresAuth) { EXPECT_TRUE(policy.requires_authentication("DELETE", "/api/v1/admin/users")); } +// The health probe is the one route the ALL policy lets through, and its +// narrowness is the whole reason it is safe. Widening it to "any method on +// /health", or to any path merely CONTAINING /health, is the natural next edit +// and would open a hole, so the boundary is pinned here. +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, AllAuthPolicyExemptsOnlyGetHealth) { + AllAuthRequirementPolicy policy; + + // Exempt: a container supervisor and a load balancer probe this with no + // credential, and requiring one turns a healthy gateway into a restart loop. + EXPECT_FALSE(policy.requires_authentication("GET", "/api/v1/health")); + + // Only GET. A write to the health path is not a liveness probe. + EXPECT_TRUE(policy.requires_authentication("POST", "/api/v1/health")); + EXPECT_TRUE(policy.requires_authentication("PUT", "/api/v1/health")); + EXPECT_TRUE(policy.requires_authentication("DELETE", "/api/v1/health")); + EXPECT_TRUE(policy.requires_authentication("PATCH", "/api/v1/health")); + EXPECT_TRUE(policy.requires_authentication("HEAD", "/api/v1/health")); + + // Only that exact path. A prefix or suffix match would hand an attacker a + // trivial bypass: append or prepend the magic word and walk in. + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v1/health/detail")); + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v1/healthz")); + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v1/components/health")); + EXPECT_TRUE(policy.requires_authentication("GET", "/health")); + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v1/health?x=1")); + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v2/health")); +} + +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, AllAuthPolicyExemptsAuthEndpoints) { + AllAuthRequirementPolicy policy; + + // Authentication cannot bootstrap through a door that already demands the + // credential it exists to hand out. + EXPECT_FALSE(policy.requires_authentication("POST", "/api/v1/auth/authorize")); + EXPECT_FALSE(policy.requires_authentication("POST", "/api/v1/auth/token")); + EXPECT_FALSE(policy.requires_authentication("POST", "/api/v1/auth/revoke")); + + // The prefix must be anchored: a path that merely mentions auth later is + // not an auth endpoint. + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v1/components/auth/data")); + EXPECT_TRUE(policy.requires_authentication("GET", "/api/v1/authorization")); +} + // @verifies REQ_INTEROP_086 TEST_F(AuthRequirementPolicyTest, WriteOnlyPolicyForGetRequests) { WriteOnlyAuthRequirementPolicy policy; diff --git a/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py new file mode 100644 index 000000000..6d0cb1250 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py @@ -0,0 +1,271 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Every route refuses an uncredentialed request under the shipped defaults. + +The gateway's own half of the closed-door acceptance. It does not check +configuration values: it asks the RUNNING gateway for its route table and then +probes every route in it. A test that asserted `require_auth_for == "all"` +would keep passing the day a route is registered outside the policy, which is +the failure this is here to catch. + +The route table comes from RouteRegistry via `GET /api/v1/`, so a route added +next year is swept the day it is registered, with nothing here to update. + +Two exemptions, and both are named with their reason in EXEMPT below. + +@verifies REQ_INTEROP_086, REQ_INTEROP_087 +""" + +import unittest + +import launch +import launch_testing +import launch_testing.actions +import pytest +import requests + +from ros2_medkit_test_utils.constants import ( + ALLOWED_EXIT_CODES, + API_BASE_PATH, + get_test_port, +) +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_gateway_node + +CLOSED_PORT = get_test_port() +CLOSED_BASE_URL = f'http://127.0.0.1:{CLOSED_PORT}{API_BASE_PATH}' +CLOSED_ROOT = f'http://127.0.0.1:{CLOSED_PORT}' + +# At least 32 characters, or the gateway refuses to start under HS256. +JWT_SECRET = 'closed_by_default_integration_secret_key_0123456789' +CLIENT_ID = 'diagbox' +CLIENT_SECRET = 'diagbox_client_secret' + +# A path parameter is filled with an id that exists on no gateway. A route that +# refuses for a nonexistent id refuses for a real one, and a probe that turns +# out to reach an OPEN route cannot mutate anything real. +PROBE_ID = 'closed-by-default-probe' + + +@pytest.mark.launch_test +def generate_test_description(): + """Gateway with the posture the shipped default config carries.""" + gateway_node = create_gateway_node( + port=CLOSED_PORT, + extra_params={ + 'server.host': '127.0.0.1', + # These three are what config/gateway_params.yaml now ships. The + # secret and client cannot come from that file (a committed secret + # is a secret every deployment shares), so they are supplied here + # the way a deployment supplies them. + 'auth.enabled': True, + 'auth.require_auth_for': 'all', + 'auth.issuer': 'ros2_medkit_gateway', + 'auth.jwt_secret': JWT_SECRET, + 'auth.clients': [f'{CLIENT_ID}:{CLIENT_SECRET}:admin'], + }, + ) + + return launch.LaunchDescription([ + gateway_node, + launch_testing.actions.ReadyToTest(), + ]), {'gateway_node': gateway_node} + + +def _is_exempt(method, path): + """Routes that are deliberately reachable without a credential. + + GET /health - a container supervisor and an upstream load balancer probe + it to decide whether this process is alive, and neither holds a + credential; requiring one turns a healthy gateway into a restart loop. The + body is a fixed status document: no entity names, no topics, no data. + + /auth/* - token issuance. Authentication cannot bootstrap through a door + that already demands the credential it exists to hand out. + """ + if method == 'GET' and path == f'{API_BASE_PATH}/health': + return True + return path.startswith(f'{API_BASE_PATH}/auth/') + + +class TestClosedByDefault(GatewayTestCase): + """The gateway refuses every route it serves, bar the named exemptions.""" + + BASE_URL = CLOSED_BASE_URL + + @classmethod + def setUpClass(cls): + super().setUpClass() + resp = requests.post( + f'{CLOSED_BASE_URL}/auth/authorize', + json={ + 'grant_type': 'client_credentials', + 'client_id': CLIENT_ID, + 'client_secret': CLIENT_SECRET, + }, + timeout=10, + ) + assert resp.status_code == 200, f'could not obtain a token: {resp.status_code} {resp.text}' + cls.token = resp.json()['access_token'] + cls.auth = {'Authorization': f'Bearer {cls.token}'} + + # The route table, read from the gateway itself. A hardened gateway + # does not list its routes anonymously, so this read authenticates. + root = requests.get(f'{CLOSED_BASE_URL}/', headers=cls.auth, timeout=10) + assert root.status_code == 200, f'route table unreadable: {root.status_code}' + cls.endpoints = root.json().get('endpoints', []) + assert cls.endpoints, 'gateway reported no endpoints - nothing would be proven' + + @staticmethod + def _fill(path): + out, depth = [], 0 + for ch in path: + if ch == '{': + depth += 1 + if depth == 1: + out.append(PROBE_ID) + elif ch == '}': + depth -= 1 + elif depth == 0: + out.append(ch) + return ''.join(out) + + def test_01_route_table_is_substantial(self): + """A sweep over three routes would prove almost nothing.""" + self.assertGreater( + len(self.endpoints), 50, + f'expected the full gateway surface, got {len(self.endpoints)} routes' + ) + + def test_02_no_route_answers_without_a_credential(self): + """Sweep EVERY registered route. This is the acceptance.""" + answered = [] + for entry in self.endpoints: + method, _, raw = entry.partition(' ') + if not raw: + continue + if _is_exempt(method, raw): + continue + path = self._fill(raw) + # A write method needs a body: without Content-Length the server + # waits for one that never arrives and the probe times out with no + # status, measuring nothing at all. + kwargs = {'timeout': 15} + if method in ('POST', 'PUT', 'PATCH'): + kwargs['json'] = {} + try: + resp = requests.request(method, f'{CLOSED_ROOT}{path}', **kwargs) + except requests.RequestException as exc: + answered.append(f'{method} {path} -> transport error {exc}') + continue + # 401/403 only. A 404 is an ANSWER: the gateway parsed the request + # and told an anonymous caller what does not exist here. + if resp.status_code not in (401, 403): + answered.append(f'{method} {path} -> {resp.status_code}') + + self.assertEqual( + [], answered, + 'these routes answered an uncredentialed request:\n ' + + '\n '.join(answered) + ) + + def test_03_a_wrong_credential_is_refused_everywhere(self): + """A token this gateway never issued gets no further than none at all.""" + bad = {'Authorization': 'Bearer not.a.real.token'} + answered = [] + for entry in self.endpoints: + method, _, raw = entry.partition(' ') + if not raw or _is_exempt(method, raw): + continue + path = self._fill(raw) + kwargs = {'timeout': 15, 'headers': bad} + if method in ('POST', 'PUT', 'PATCH'): + kwargs['json'] = {} + try: + resp = requests.request(method, f'{CLOSED_ROOT}{path}', **kwargs) + except requests.RequestException as exc: + answered.append(f'{method} {path} -> transport error {exc}') + continue + if resp.status_code not in (401, 403): + answered.append(f'{method} {path} -> {resp.status_code}') + + self.assertEqual( + [], answered, + 'these routes accepted a forged credential:\n ' + '\n '.join(answered) + ) + + def test_04_reads_are_refused_not_just_writes(self): + """The require_auth_for="write" hole, pinned directly. + + Under "write" every one of these answers 200 to an anonymous caller, + and they are the disclosure: the entity tree names the machines. + """ + for path in ('/', '/areas', '/components', '/apps', '/functions', '/version-info'): + with self.subTest(path=path): + resp = requests.get(f'{CLOSED_BASE_URL}{path}', timeout=15) + self.assertIn(resp.status_code, (401, 403)) + + def test_05_the_exempt_health_route_still_answers(self): + """The mirror of the sweeps. + + Without this, a gateway that refused EVERYTHING would pass every test + above while being uniformly broken - the container supervisor could + not tell it was alive, and it would restart forever. + """ + resp = requests.get(f'{CLOSED_BASE_URL}/health', timeout=15) + self.assertEqual(resp.status_code, 200, 'health must stay probe-able') + + def test_06_health_discloses_no_topology(self): + """Health is exempt because it says nothing, so check that it says nothing.""" + body = requests.get(f'{CLOSED_BASE_URL}/health', timeout=15).json() + for leaked in ('areas', 'components', 'apps', 'functions', 'entities', 'nodes', 'topics'): + self.assertNotIn( + leaked, body, + f'health exposes "{leaked}" to anonymous callers - the exemption ' + 'rests on it carrying no topology' + ) + + def test_07_a_valid_credential_gets_through(self): + """Otherwise the sweeps above would pass on a gateway that serves nobody.""" + resp = requests.get(f'{CLOSED_BASE_URL}/areas', headers=self.auth, timeout=15) + self.assertEqual(resp.status_code, 200) + + def test_08_head_on_health_is_not_a_way_in(self): + """The exemption is GET-only, deliberately. + + Widening it to "any method on /health" is the natural next edit and it + would be wrong, so the narrowness is pinned here. + """ + for method in ('POST', 'PUT', 'DELETE', 'PATCH'): + with self.subTest(method=method): + resp = requests.request( + method, f'{CLOSED_BASE_URL}/health', json={}, timeout=15 + ) + self.assertNotEqual( + resp.status_code, 200, + f'{method} /health answered 200 to an anonymous caller' + ) + + +@launch_testing.post_shutdown_test() +class TestClosedByDefaultShutdown(unittest.TestCase): + """Gateway exits cleanly.""" + + def test_exit_codes(self, proc_info, gateway_node): + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES, process=gateway_node + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_peer_recovery.test.py b/src/ros2_medkit_integration_tests/test/features/test_peer_recovery.test.py index 7aedd42e2..ee029d0d1 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_peer_recovery.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_peer_recovery.test.py @@ -825,9 +825,20 @@ def test_07_a_peer_owned_read_succeeds_again_and_the_peer_answered_it(self): 'test_04 must watch this URL fail before test_07 can claim it recovered', ) + # Poll for the condition this test actually asserts, not merely for a + # 200. Recovery has two steps that finish at different times: the route + # comes back, and then a sample arrives on the re-created subscription. + # Between them the read answers 200 with status "metadata_only" and an + # empty body, so a poll that stops at the status code hands the + # assertions below a response taken from that window - and the wider + # the machine's load, the wider the window. def served(): answer = self._aggregate_read_of_peer_topic() - return answer if answer.status_code == 200 else None + if answer.status_code != 200: + return None + if answer.json().get('x-medkit', {}).get('status') != 'data': + return None + return answer response = _poll(served, timeout=RECOVERY_TIMEOUT) self.assertIsNotNone( From 590ca4f866880fb7f3063f7670976ff7c1375511 Mon Sep 17 00:00:00 2001 From: bburda Date: Wed, 26 Aug 2026 20:42:44 +0200 Subject: [PATCH 2/4] gateway: stop /health returning its full body to an anonymous caller Making a route public means owning what it says, and the full /health body was not safe to publish. linking.warnings holds strings like "App 'engine_ecu' cannot bind to '/nav/controller'", which is an entity name and a ROS node FQN, and x-medkit-entity-cache holds the counts of apps, areas and components this gateway sees. A liveness probe needs none of that. A caller with no credential, or with a token this gateway did not issue, now gets status and timestamp only. A caller with a valid token gets the whole document. The check runs before any section is built, so a section added later is private by default instead of public until someone remembers to think about it. When auth is off nothing is anonymous and the body is unchanged. This closes item 4 of #259. The test for this was wrong before and is worth saying why. It listed field names known to leak and checked they were absent. That list did not include warnings or x-medkit-entity-cache, which are the actual vectors, so it passed while proving nothing - the warnings array happened to be empty on the gateway it ran against. It now asserts the exact set of keys an anonymous caller gets, which is an allowlist and cannot silently miss a field added later, and a second test asserts the authenticated body still carries the operator detail so the narrowing does not quietly become pointless. --- .../src/http/handlers/health_handlers.cpp | 43 ++++++++++++- .../features/test_closed_by_default.test.py | 63 ++++++++++++++++--- 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp index 42324b8e2..ae0300634 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp @@ -17,6 +17,7 @@ #include #include "ros2_medkit_gateway/aggregation/aggregation_manager.hpp" +#include "ros2_medkit_gateway/core/auth/auth_middleware.hpp" #include "ros2_medkit_gateway/core/auth/auth_models.hpp" #include "ros2_medkit_gateway/core/data/topic_data_provider.hpp" #include "ros2_medkit_gateway/core/discovery/discovery_enums.hpp" @@ -50,13 +51,53 @@ ErrorInfo make_internal_error(const char * where, const std::exception & e) { } // namespace +namespace { + +/// True when authentication is on and this request did not present a token +/// this gateway accepts. +/// +/// /health is the one route the "all" policy lets through unauthenticated, so +/// that a container supervisor or load balancer can tell the process is alive +/// without holding a credential. That exemption is only defensible while the +/// body says nothing an anonymous caller should not learn - and the full body +/// does: the linking warnings name entities and ROS node FQNs, and the entity +/// cache reports how many apps, areas and components this gateway sees. The +/// probe needs none of that, so an anonymous caller gets the liveness answer +/// and nothing else, and an authenticated one gets the whole document. +bool is_anonymous(const HandlerContext & ctx, const http::TypedRequest & req) { + if (!ctx.auth_config().enabled) { + return false; // Nothing is anonymous when nothing is authenticated. + } + auto * manager = ctx.auth_manager(); + if (manager == nullptr) { + return true; // Fail closed: cannot verify, so do not disclose. + } + auto header = req.header("Authorization"); + if (!header) { + return true; + } + auto token = AuthMiddleware::extract_bearer_token(*header); + if (!token) { + return true; + } + return !manager->validate_token(*token).valid; +} + +} // namespace + http::Result HealthHandlers::get_health(const http::TypedRequest & req) { - (void)req; // Unused parameter try { dto::Health response; response.status = "healthy"; response.timestamp = std::chrono::system_clock::now().time_since_epoch().count(); + // Liveness and nothing else for an anonymous caller. Returned before any + // of the sections below are built, so a section added later is private by + // default rather than public until someone remembers to think about it. + if (is_anonymous(ctx_, req)) { + return response; + } + // Operator-actionable warnings the gateway flags without taking itself // offline. Collected across every subsystem that can produce one, so the // array and its schema version are part of the /health contract whether or diff --git a/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py index 6d0cb1250..dfc3083df 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py @@ -229,15 +229,62 @@ def test_05_the_exempt_health_route_still_answers(self): resp = requests.get(f'{CLOSED_BASE_URL}/health', timeout=15) self.assertEqual(resp.status_code, 200, 'health must stay probe-able') - def test_06_health_discloses_no_topology(self): - """Health is exempt because it says nothing, so check that it says nothing.""" + def test_06_anonymous_health_is_liveness_and_nothing_else(self): + """The exemption rests on the body saying nothing, so check the body. + + An allowlist, not a denylist. Listing the fields known to leak today + would pass the day a new section is added, and the whole reason this + route is public is that a probe needs no more than "am I alive". + """ body = requests.get(f'{CLOSED_BASE_URL}/health', timeout=15).json() - for leaked in ('areas', 'components', 'apps', 'functions', 'entities', 'nodes', 'topics'): - self.assertNotIn( - leaked, body, - f'health exposes "{leaked}" to anonymous callers - the exemption ' - 'rests on it carrying no topology' - ) + # warnings and its schema version are always serialised - they are part + # of the /health contract whether or not anything produced one - so the + # allowlist includes them and the assertion below covers the content. + self.assertEqual( + set(body), {'status', 'timestamp', 'warnings', 'warning_schema_version'}, + f'an anonymous /health returned more than liveness: {body}' + ) + self.assertEqual(body['status'], 'healthy') + # The array is the leak vector: a linking warning reads like + # "App 'engine_ecu' cannot bind to '/nav/controller'", naming an entity + # and a ROS node FQN. Empty is the only safe value for a caller that + # presented nothing. + self.assertEqual( + body['warnings'], [], + f'an anonymous /health carried warning text: {body["warnings"]}' + ) + + def test_06b_the_full_health_document_names_entities(self): + """The reason test_06 matters, pinned so it cannot be argued away. + + With a credential the same route returns discovery state and entity + cache counts. That is a legitimate operator surface, and it is exactly + what an anonymous caller must not receive - so if this ever stops being + true, the narrowing above has become pointless and should be revisited + rather than left as dead weight. + """ + body = requests.get( + f'{CLOSED_BASE_URL}/health', headers=self.auth, timeout=15 + ).json() + self.assertIn('discovery', body) + self.assertIn('x-medkit-entity-cache', body) + self.assertGreater( + len(set(body)), 2, + 'the authenticated /health is now as bare as the anonymous one' + ) + + def test_06c_a_forged_credential_gets_the_bare_body(self): + """A token this gateway never issued is an anonymous caller.""" + body = requests.get( + f'{CLOSED_BASE_URL}/health', + headers={'Authorization': 'Bearer not.a.real.token'}, + timeout=15, + ).json() + self.assertEqual( + set(body), {'status', 'timestamp', 'warnings', 'warning_schema_version'}, + f'a forged credential unlocked the full health document: {body}' + ) + self.assertEqual(body['warnings'], []) def test_07_a_valid_credential_gets_through(self): """Otherwise the sweeps above would pass on a gateway that serves nobody.""" From 978e00cbd181018500c3f9d9a9721a161130e506 Mon Sep 17 00:00:00 2001 From: bburda Date: Thu, 27 Aug 2026 14:28:32 +0200 Subject: [PATCH 3/4] gateway: make the TLS settings do what they say, and stop the token store growing This branch turns TLS on for every default deployment, so the TLS settings had to start meaning something. Three of them did not. min_version was read, logged and ignored. The comment said cpp-httplib does not expose SSL_CTX; the vendored copy declares a public ssl_context(), so it does. Worse, the vendored SSLServer constructor calls SSL_CTX_set_min_proto_version(ctx_, TLS1_1_VERSION), so the floor a deployment gets is the library's request filtered through whatever the local OpenSSL policy happens to allow. Neither of those is a value this project chose, and SOVD requires TLS 1.2 as its minimum, so the floor is now set on our own context from the configured value. An unrecognised value was already rejected in TlsConfig::validate(), so the runtime warning had nothing left to say and is gone with the comment. Measured on Jazzy, Ubuntu 24.04, OpenSSL 3.0.13, one gateway at a time: before this change a gateway configured for min_version 1.3 completed a TLS 1.2 handshake. TLS 1.0 and 1.1 were already refused on this system, so the floor was not at 1.1 here; it was wherever OpenSSL put it, which is the actual problem. After the change, 1.3 refuses 1.2 and 1.2 refuses 1.1. ca_file was parsed into TlsConfig and dropped on the floor. The vendored constructor takes a client CA path and, given one, does the SSL_CTX_load_verify_locations and SSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERT work itself, so it is now passed through. Setting it makes a client certificate required for that gateway; leaving it empty is unchanged server-only TLS, which is what the bearer-token flow expects. SOVD says nothing about client certificates, so this stays opt-in. TLS also had no way out at the launch level while auth had one, even though this branch defaults tls.enabled true with an empty cert_file. gateway.launch.py now takes tls_enabled, cert_file and key_file in the same shape as the auth arguments, and names them when TLS is on and no certificate was given. cleanup_expired_tokens() had no caller outside a test, so refresh_tokens_ grew by one record per successful authorisation for the life of the process, and validate_token looks that map up on every authenticated request. The sweep now runs when a record is stored, which keeps the bound a property of the data structure rather than of a timer that may not be running, and makes it observable without waiting on wall clock. refresh_token_count() exists so a test can assert on the quantity the claim is about. Client secrets were compared with !=, which returns on the first differing byte. They are now compared in constant time. The secrets are still stored in plaintext in the configuration; that is a config-format question and is deliberately not addressed here. An access token naming a refresh record that is no longer present is now invalid rather than unchecked. Records live in memory, so this is also what a restart looks like: clients re-authenticate after one. That is a visible behaviour change and it is the intended trade. The TLS floor and client-certificate behaviour are settled by handshakes from a real client against real gateways, because both are decided before any HTTP request exists. Two gateways run side by side at floors 1.2 and 1.3 so the floor is shown to move with the setting rather than sitting where OpenSSL's default put it. Worth recording for whoever reads that file next: "Cipher is " in s_client output does not mean the handshake completed, because under TLS 1.2 the suite is agreed before the client certificate is examined. Reading it that way reported mutual TLS as broken while it was working. --- docs/tutorials/https.rst | 63 +++- .../config/gateway_params.yaml | 7 +- .../core/auth/auth_manager.hpp | 11 + .../launch/gateway.launch.py | 44 +++ .../src/core/auth/auth_manager.cpp | 84 ++++- src/ros2_medkit_gateway/src/core/config.cpp | 5 - src/ros2_medkit_gateway/src/gateway_node.cpp | 1 - .../src/http/http_server.cpp | 61 ++-- .../test/test_auth_manager.cpp | 121 ++++++++ .../features/test_tls_protocol_floor.test.py | 293 ++++++++++++++++++ 10 files changed, 650 insertions(+), 40 deletions(-) create mode 100644 src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py diff --git a/docs/tutorials/https.rst b/docs/tutorials/https.rst index 5604fbd8a..0812e0c2e 100644 --- a/docs/tutorials/https.rst +++ b/docs/tutorials/https.rst @@ -100,10 +100,69 @@ Configuration Options - Path to PEM-encoded private key * - ``server.tls.ca_file`` - ``""`` - - CA certificate (for future mutual TLS) + - CA that signs client certificates. Setting it turns on mutual TLS and + makes a client certificate **required**; leave empty for server-only TLS * - ``server.tls.min_version`` - ``"1.2"`` - - Minimum TLS version: ``"1.2"`` or ``"1.3"`` + - Minimum TLS version: ``"1.2"`` or ``"1.3"``. Enforced on the server's own + SSL context, so it is the floor regardless of what the local OpenSSL + policy would otherwise allow. Any other value is rejected at startup + +Defaults +-------- + +TLS is **on** in the shipped ``gateway_params.yaml``, and ``cert_file`` and +``key_file`` are empty. A gateway with TLS enabled and no certificate refuses +to start rather than fall back to plaintext, so a first run has to supply one +of the two: + +.. code-block:: bash + + # a certificate, for a real deployment or a self-signed pair for a first run + ros2 launch ros2_medkit_gateway gateway.launch.py \ + cert_file:=/path/to/cert.pem key_file:=/path/to/key.pem + + # or no TLS at all, only on a host nothing else can reach + ros2 launch ros2_medkit_gateway gateway.launch.py tls_enabled:=false + +For a first run on a developer machine, ``scripts/generate_dev_certs.sh`` +writes a self-signed certificate and key. Browsers and ``curl`` will refuse it +until you pass the CA explicitly, which is the correct behaviour for a +certificate nothing has vouched for, not a problem to work around in +production. + +Mutual TLS +---------- + +Set ``ca_file`` to the CA that signs your client certificates and the gateway +requires one from **every** client: + +.. code-block:: yaml + + server: + tls: + enabled: true + cert_file: "/etc/ros2_medkit/certs/server.pem" + key_file: "/etc/ros2_medkit/certs/server-key.pem" + ca_file: "/etc/ros2_medkit/certs/client-ca.pem" + +This is all or nothing per gateway. A client that presents no certificate is +rejected during the handshake, before any request is read, and there is no +"verify it only if offered" setting. A client whose certificate is signed by +any other CA is rejected the same way. + +.. code-block:: bash + + # without a client certificate: no response, the handshake never completes + curl --cacert ca.pem https://localhost:8443/api/v1/areas + + # with one signed by ca_file + curl --cacert ca.pem --cert client.pem --key client-key.pem \ + https://localhost:8443/api/v1/areas + +Mutual TLS is transport-level and sits alongside token authentication rather +than replacing it. SOVD authenticates with bearer tokens, so leave ``ca_file`` +empty unless every client on that network can be issued a certificate. Using with curl --------------- diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index db8a6e630..03c21825f 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -107,8 +107,11 @@ ros2_medkit_gateway: # Options: "1.2" (default, widely compatible), "1.3" (more secure) min_version: "1.2" - # TODO: Mutual TLS (client certificate verification) is not yet implemented - # See: https://github.com/selfpatch/ros2_medkit/issues/XXX + # Mutual TLS. Set ca_file above to the CA that signs your client + # certificates and the gateway REQUIRES one from every client: a + # caller with no certificate is rejected during the handshake, before + # any request is read. Leave ca_file empty for ordinary server-only + # TLS, which is what bearer-token clients expect. # Safety-backstop refresh interval in milliseconds. # diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_manager.hpp index b8aaaa953..81826a8c1 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_manager.hpp @@ -150,6 +150,13 @@ class AuthManager { */ bool enable_client(const std::string & client_id); + /// How many refresh records are currently held. + /// + /// Public so a test can observe that the sweep actually runs. The count is + /// the thing the unbounded-growth claim is about, and asserting on it is the + /// only way to tell a sweep that works from one that is never called. + size_t refresh_token_count() const; + private: /** * @brief Generate a JWT token @@ -201,6 +208,10 @@ class AuthManager { mutable std::mutex clients_mutex_; std::unordered_map clients_; + /// Drop every expired record. The caller must already hold + /// refresh_tokens_mutex_; cleanup_expired_tokens() is the locking wrapper. + size_t cleanup_expired_locked(); + // Refresh token storage (thread-safe) mutable std::mutex refresh_tokens_mutex_; std::unordered_map refresh_tokens_; diff --git a/src/ros2_medkit_gateway/launch/gateway.launch.py b/src/ros2_medkit_gateway/launch/gateway.launch.py index d802b14dc..8160b45f0 100644 --- a/src/ros2_medkit_gateway/launch/gateway.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway.launch.py @@ -118,6 +118,29 @@ def generate_launch_description(): '(roles: viewer, operator, configurator, admin). Needed to obtain ' 'a token from /auth/token.')) + declare_tls_enabled_arg = DeclareLaunchArgument( + 'tls_enabled', default_value='true', + description=( + 'Serve HTTPS. On by default, matching the shipped config. Needs ' + 'cert_file and key_file; the gateway refuses to start with TLS on ' + 'and no certificate rather than fall back to plaintext. Pass ' + 'tls_enabled:=false to serve plain HTTP on a host nothing else ' + 'can reach.')) + + declare_cert_file_arg = DeclareLaunchArgument( + 'cert_file', default_value='', + description=( + 'PEM certificate (or full chain) for HTTPS. REQUIRED while ' + 'tls_enabled is true. For a first run, generate a self-signed ' + 'pair with scripts/generate_dev_certs.sh - browsers will warn, ' + 'which is correct for a certificate nothing has vouched for.')) + + declare_key_file_arg = DeclareLaunchArgument( + 'key_file', default_value='', + description=( + 'PEM private key matching cert_file. REQUIRED while tls_enabled ' + 'is true. Keep it chmod 600 and owned by the gateway user.')) + declare_cors_arg = DeclareLaunchArgument( 'cors_allowed_origins', default_value=CORS_DEFAULT, @@ -145,6 +168,24 @@ def _launch_setup(context, *_args, **_kwargs): LaunchConfiguration('cors_allowed_origins').perform(context), LaunchConfiguration('config_file').perform(context), default_config)) + tls_enabled = LaunchConfiguration('tls_enabled').perform(context).lower() in ( + 'true', '1', 'yes') + param_overrides['server.tls.enabled'] = tls_enabled + cert_file = LaunchConfiguration('cert_file').perform(context) + key_file = LaunchConfiguration('key_file').perform(context) + if cert_file: + param_overrides['server.tls.cert_file'] = cert_file + if key_file: + param_overrides['server.tls.key_file'] = key_file + if tls_enabled and not (cert_file and key_file): + # The gateway would refuse to start a moment from now, naming the + # config file. Name the launch arguments instead, here, where they + # are the thing the reader can actually change. + print('[gateway.launch.py] TLS is enabled and cert_file/key_file were not both ' + 'given. Pass cert_file:= key_file:=, set them in a config_file, ' + 'or pass tls_enabled:=false to serve plain HTTP. ' + 'scripts/generate_dev_certs.sh makes a self-signed pair for a first run.') + auth_enabled = LaunchConfiguration('auth_enabled').perform(context).lower() in ( 'true', '1', 'yes') param_overrides['auth.enabled'] = auth_enabled @@ -178,6 +219,9 @@ def _launch_setup(context, *_args, **_kwargs): declare_auth_enabled_arg, declare_jwt_secret_arg, declare_clients_arg, + declare_tls_enabled_arg, + declare_cert_file_arg, + declare_key_file_arg, declare_cors_arg, OpaqueFunction(function=_launch_setup), ]) diff --git a/src/ros2_medkit_gateway/src/core/auth/auth_manager.cpp b/src/ros2_medkit_gateway/src/core/auth/auth_manager.cpp index a0e313d8a..c74d32350 100644 --- a/src/ros2_medkit_gateway/src/core/auth/auth_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/auth/auth_manager.cpp @@ -26,6 +26,30 @@ namespace ros2_medkit_gateway { +namespace { + +/// Compare two secrets without returning early on the first differing byte. +/// +/// The lengths are compared too, and a length mismatch is reported. That does +/// leak the length, which is acceptable: secrets here are operator-chosen and +/// their length is not the secret. What must not leak is WHICH bytes matched, +/// and the loop below always visits every byte of the expected value. +bool constant_time_equals(const std::string & expected, const std::string & presented) { + // Fold the length difference into the result rather than returning, so both + // branches cost the same. + unsigned char diff = static_cast(expected.size() != presented.size()); + const std::size_t n = expected.size(); + for (std::size_t i = 0; i < n; ++i) { + // Index the presented value modulo its own size so a shorter input cannot + // read out of bounds; the length check above already forced a mismatch. + const unsigned char p = presented.empty() ? 0U : static_cast(presented[i % presented.size()]); + diff |= static_cast(static_cast(expected[i]) ^ p); + } + return diff == 0; +} + +} // namespace + // Helper to read file contents static std::string read_file_contents(const std::string & path) { std::ifstream file(path); @@ -85,8 +109,15 @@ tl::expected AuthManager::authenticate(const s return tl::unexpected(AuthErrorResponse::invalid_client("Client is disabled")); } - // Verify secret - if (client.client_secret != client_secret) { + // Verify secret in constant time. A plain std::string comparison returns as + // soon as two bytes differ, so the time it takes to refuse leaks how many + // leading bytes were right, and a caller who can measure it can recover the + // secret one byte at a time. Every default deployment now needs a configured + // client, so this path is on the critical path for all of them. + // + // Secrets are still stored in plaintext in the configuration; making this + // comparison constant-time does not change that and is not meant to. + if (!constant_time_equals(client.client_secret, client_secret)) { return tl::unexpected(AuthErrorResponse::invalid_client("Invalid client_secret")); } @@ -248,10 +279,27 @@ TokenValidationResult AuthManager::validate_token(const std::string & token, Tok return result; } - // Check if associated refresh token is revoked (for access tokens) + // An access token that names a refresh record is only valid while that + // record is present and not revoked. + // + // Absent counts as invalid, not as "nothing to check". Records live in + // memory, so after a restart the map is empty; treating absence as fine + // would make every revoked token work again until it expired on its own, + // which on the default one-hour expiry is a long time to keep honouring a + // credential somebody explicitly withdrew. + // + // The cost is deliberate and worth stating: a restart invalidates every + // access token, so clients re-authenticate after one. That is visible + // behaviour, and it is the trade this gateway makes elsewhere too - refusing + // is better than quietly allowing. if (claims.refresh_token_id.has_value()) { auto record = get_refresh_token(claims.refresh_token_id.value()); - if (record.has_value() && record->revoked) { + if (!record.has_value()) { + result.valid = false; + result.error = "Associated refresh token is no longer known to this gateway"; + return result; + } + if (record->revoked) { result.valid = false; result.error = "Associated refresh token has been revoked"; return result; @@ -341,13 +389,11 @@ bool AuthManager::revoke_refresh_token(const std::string & refresh_token) { return true; } -size_t AuthManager::cleanup_expired_tokens() { +size_t AuthManager::cleanup_expired_locked() { auto now = std::chrono::system_clock::now(); auto now_ts = std::chrono::duration_cast(now.time_since_epoch()).count(); - std::lock_guard lock(refresh_tokens_mutex_); size_t count = 0; - for (auto it = refresh_tokens_.begin(); it != refresh_tokens_.end();) { if (it->second.expires_at < now_ts) { it = refresh_tokens_.erase(it); @@ -356,10 +402,19 @@ size_t AuthManager::cleanup_expired_tokens() { ++it; } } - return count; } +size_t AuthManager::refresh_token_count() const { + std::lock_guard lock(refresh_tokens_mutex_); + return refresh_tokens_.size(); +} + +size_t AuthManager::cleanup_expired_tokens() { + std::lock_guard lock(refresh_tokens_mutex_); + return cleanup_expired_locked(); +} + bool AuthManager::register_client(const std::string & client_id, const std::string & client_secret, UserRole role) { std::lock_guard lock(clients_mutex_); @@ -600,6 +655,19 @@ bool AuthManager::matches_path(const std::string & pattern, const std::string & void AuthManager::store_refresh_token(const RefreshTokenRecord & record) { std::lock_guard lock(refresh_tokens_mutex_); + + // Sweep before inserting. Nothing else calls the sweep, so without this the + // map keeps one record per successful authorisation for the life of the + // process, and validate_token looks that map up on every authenticated + // request. Doing it here rather than on a timer keeps the bound a property + // of the data structure instead of a property of a thread that might not be + // running, and makes it observable in a test without waiting on wall clock. + // + // The cost is a scan per authorisation. The map only ever holds unexpired + // records, so it is sized by how many tokens are live at once, not by how + // many have ever been issued. + cleanup_expired_locked(); + refresh_tokens_[record.token_id] = record; } diff --git a/src/ros2_medkit_gateway/src/core/config.cpp b/src/ros2_medkit_gateway/src/core/config.cpp index b140bd110..37cb72fc9 100644 --- a/src/ros2_medkit_gateway/src/core/config.cpp +++ b/src/ros2_medkit_gateway/src/core/config.cpp @@ -58,11 +58,6 @@ std::string TlsConfig::validate() const { return "TLS: ca_file does not exist or is not readable: " + ca_file; } - // TODO(future): Add mutual TLS validation when implemented - // if (mutual_tls && ca_file.empty()) { - // return "TLS: ca_file is required when mutual_tls is enabled"; - // } - // Validate minimum TLS version if (min_version != "1.2" && min_version != "1.3") { return "TLS: min_version must be '1.2' or '1.3', got: " + min_version; diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 520945a7f..cb07ed3cb 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -435,7 +435,6 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki .with_key_file(get_parameter("server.tls.key_file").as_string()) .with_ca_file(get_parameter("server.tls.ca_file").as_string()) .with_min_version(get_parameter("server.tls.min_version").as_string()) - // TODO(future): Add .with_mutual_tls() when implemented .build(); // Note: HttpServerManager will log TLS configuration details } catch (const std::exception & e) { diff --git a/src/ros2_medkit_gateway/src/http/http_server.cpp b/src/ros2_medkit_gateway/src/http/http_server.cpp index 3979bd477..45ebff98b 100644 --- a/src/ros2_medkit_gateway/src/http/http_server.cpp +++ b/src/ros2_medkit_gateway/src/http/http_server.cpp @@ -17,6 +17,11 @@ #include #include +#ifdef CPPHTTPLIB_OPENSSL_SUPPORT +// TLS1_2_VERSION / TLS1_3_VERSION and SSL_CTX_set_min_proto_version. +#include +#endif + namespace ros2_medkit_gateway { HttpServerManager::HttpServerManager(const TlsConfig & tls_config, std::size_t thread_pool_size, @@ -24,8 +29,19 @@ HttpServerManager::HttpServerManager(const TlsConfig & tls_config, std::size_t t : tls_config_(tls_config), thread_pool_size_(thread_pool_size), keep_alive_timeout_sec_(keep_alive_timeout_sec) { #ifdef CPPHTTPLIB_OPENSSL_SUPPORT if (tls_config_.enabled) { - // Create SSL server with certificate and key - ssl_server_ = std::make_unique(tls_config_.cert_file.c_str(), tls_config_.key_file.c_str()); + // A non-empty ca_file turns on mutual TLS. The SSLServer constructor does + // the work itself: given a client CA path it calls + // SSL_CTX_load_verify_locations and then + // SSL_CTX_set_verify(SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT). + // + // That pairing is why this is all-or-nothing per gateway: with a CA set, + // a client that presents NO certificate is rejected at the handshake. + // There is no "verify it if offered" middle setting without patching the + // vendored header. Leaving ca_file empty keeps ordinary server-only TLS, + // which is what the SOVD bearer-token flow expects. + ssl_server_ = + std::make_unique(tls_config_.cert_file.c_str(), tls_config_.key_file.c_str(), + tls_config_.ca_file.empty() ? nullptr : tls_config_.ca_file.c_str()); if (!ssl_server_->is_valid()) { throw std::runtime_error( @@ -38,8 +54,10 @@ HttpServerManager::HttpServerManager(const TlsConfig & tls_config, std::size_t t apply_thread_pool(*ssl_server_); apply_keep_alive(*ssl_server_); - RCLCPP_INFO(rclcpp::get_logger("http_server"), "TLS/HTTPS enabled - cert: %s, min_version: %s", - tls_config_.cert_file.c_str(), tls_config_.min_version.c_str()); + RCLCPP_INFO(rclcpp::get_logger("http_server"), + "TLS/HTTPS enabled - cert: %s, min_version: %s, client certificates: %s", tls_config_.cert_file.c_str(), + tls_config_.min_version.c_str(), + tls_config_.ca_file.empty() ? "not required" : "REQUIRED (mutual TLS)"); // Note: key_file path intentionally not logged for security reasons } else { server_ = std::make_unique(); @@ -138,26 +156,25 @@ void HttpServerManager::configure_tls() { return; } - // YAGNI Decision: min_version field exists in TlsConfig for future extensibility - // but is not fully implemented. - // - // Rationale: - // - cpp-httplib's SSLServer doesn't expose SSL_CTX for min_version configuration - // - Modern OpenSSL (1.1.1+) defaults to TLS 1.2+ which is secure + // Set the protocol floor on our own context rather than inheriting one. // - // Future implementation options: - // 1. Fork cpp-httplib to expose SSL_CTX for SSL_CTX_set_min_proto_version() - // 2. Use OpenSSL system-wide configuration (/etc/ssl/openssl.cnf) - // 3. Replace cpp-httplib with Boost.Beast or another library with full SSL control + // Two reasons it has to be us. The SSLServer constructor calls + // SSL_CTX_set_min_proto_version(ctx_, TLS1_1_VERSION), so the library asks + // for a floor of TLS 1.1. What a deployment actually gets on top of that is + // whatever the local OpenSSL policy allows, which differs between the + // distributions we ship for. Neither of those is a decision this project + // made, and SOVD requires TLS 1.2 as the minimum, so the value is set here + // where it can be read and tested. // - // TODO(future): Add mutual TLS support - requires cpp-httplib modifications - // to expose SSL_CTX for SSL_CTX_set_verify() with SSL_VERIFY_PEER - - if (tls_config_.min_version != "1.2") { - RCLCPP_WARN(rclcpp::get_logger("http_server"), - "min_version='%s' requested but cpp-httplib uses OpenSSL defaults (TLS 1.2+). " - "Custom min_version not enforced.", - tls_config_.min_version.c_str()); + // The string is validated in TlsConfig::validate(), which rejects anything + // other than "1.2" or "1.3" before a server is ever constructed. + const int min_proto = (tls_config_.min_version == "1.3") ? TLS1_3_VERSION : TLS1_2_VERSION; + SSL_CTX * ctx = ssl_server_->ssl_context(); + if (ctx == nullptr || SSL_CTX_set_min_proto_version(ctx, min_proto) != 1) { + // Refuse to serve rather than fall back to the library floor: a caller + // that asked for 1.3 and silently got 1.1 is worse off than one that got + // an error, because nothing downstream can tell the difference. + throw std::runtime_error("Failed to set the minimum TLS version to " + tls_config_.min_version); } // Log TLS handshake failures for debugging diff --git a/src/ros2_medkit_gateway/test/test_auth_manager.cpp b/src/ros2_medkit_gateway/test/test_auth_manager.cpp index a71a7993c..0c4550e58 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -586,6 +586,127 @@ TEST_F(AuthManagerTest, CleanupExpiredTokens) { EXPECT_GE(cleaned, 1); } +// --------------------------------------------------------------------------- +// Refresh-record growth, constant-time secret comparison, and revocation. +// --------------------------------------------------------------------------- + +namespace { + +/// A manager with a single admin client, parameterised on the two expiry +/// values, so a test can put them at their endpoints rather than at one +/// comfortable middle value. +AuthManager make_manager(int access_expiry, int refresh_expiry) { + auto config = AuthConfigBuilder() + .with_enabled(true) + .with_jwt_secret("expiry_sweep_secret_key_at_least_32_chars_long") + .with_require_auth_for(AuthRequirement::ALL) + .with_token_expiry(access_expiry) + .with_refresh_token_expiry(refresh_expiry) + .add_client("svc", "svc_secret", UserRole::ADMIN) + .build(); + return AuthManager(config); +} + +} // namespace + +// The sweep exists but had no production caller, so the map grew by one record +// per successful authorisation for the life of the process. What this asserts +// is the COUNT, because a sweep that is never invoked returns the right answer +// when a test calls it directly and still leaks in production. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerTokenLifetimeTest, RepeatedLoginsDoNotGrowTheStoreWithoutBound) { + // Refresh expiry at its minimum legal value: validate() requires + // refresh >= access, so this is the endpoint, not a convenient number. + auto manager = make_manager(1, 1); + + for (int i = 0; i < 5; ++i) { + ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); + } + EXPECT_EQ(manager.refresh_token_count(), 5U) << "records should accumulate while they are live"; + + // Past the refresh expiry, the next authorisation must clear them out. + std::this_thread::sleep_for(std::chrono::seconds(2)); + ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); + + EXPECT_EQ(manager.refresh_token_count(), 1U) + << "the five expired records survived a later authorisation - the sweep is not running"; +} + +// The other endpoint. A long-lived refresh token must NOT be swept: an +// over-eager sweep would log clients out mid-session, which is the opposite +// failure and just as real. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerTokenLifetimeTest, LongLivedRecordsAreNotSweptEarly) { + auto manager = make_manager(1, 86400); + + for (int i = 0; i < 4; ++i) { + ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); + } + std::this_thread::sleep_for(std::chrono::seconds(2)); + ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); + + EXPECT_EQ(manager.refresh_token_count(), 5U) << "records well inside their expiry were discarded"; +} + +// Degenerate case: access and refresh expiry equal and both large. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerTokenLifetimeTest, EqualAccessAndRefreshExpiryKeepsRecords) { + auto manager = make_manager(3600, 3600); + ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); + ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); + EXPECT_EQ(manager.refresh_token_count(), 2U); +} + +// A wrong secret must be refused whatever its shape. The interesting inputs +// are the ones a short-circuiting comparison treats differently from a +// constant-time one: a correct prefix, and a value that extends the real one. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerSecretComparisonTest, OnlyTheExactSecretAuthenticates) { + auto manager = make_manager(3600, 3600); + + EXPECT_TRUE(manager.authenticate("svc", "svc_secret").has_value()) << "the real secret must work"; + + for (const auto & wrong : {"", "s", "svc_secre", "svc_secret_", "svc_secretX", "SVC_SECRET", "xxxxxxxxxx"}) { + EXPECT_FALSE(manager.authenticate("svc", wrong).has_value()) << "secret \"" << wrong << "\" was accepted"; + } +} + +// An access token whose refresh record is gone is invalid, not "unchecked". +// Records are in memory, so this is also what a gateway restart looks like to +// a token issued before it: the deliberate consequence is that a restart makes +// clients re-authenticate. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerRevocationTest, AnAccessTokenWithNoSurvivingRecordIsRejected) { + auto manager = make_manager(3600, 3600); + auto issued = manager.authenticate("svc", "svc_secret"); + ASSERT_TRUE(issued.has_value()); + + EXPECT_TRUE(manager.validate_token(issued->access_token).valid) + << "the token must be valid while its record is present"; + + // Revoking drops or marks the record; either way the access token that names + // it must stop being accepted. + ASSERT_TRUE(issued->refresh_token.has_value()); + ASSERT_TRUE(manager.revoke_refresh_token(issued->refresh_token.value())); + EXPECT_FALSE(manager.validate_token(issued->access_token).valid) + << "an access token whose refresh record was revoked was still accepted"; +} + +// A second manager standing in for the same gateway after a restart: same +// secret and issuer, so the signature still verifies, but no records. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerRevocationTest, ARestartInvalidatesAccessTokensRatherThanTrustingThem) { + auto before = make_manager(3600, 3600); + auto issued = before.authenticate("svc", "svc_secret"); + ASSERT_TRUE(issued.has_value()); + ASSERT_TRUE(before.validate_token(issued->access_token).valid); + + auto after_restart = make_manager(3600, 3600); + EXPECT_FALSE(after_restart.validate_token(issued->access_token).valid) + << "a token from before the restart was accepted although the gateway has no record of it - " + "a revoked token would come back to life this way"; +} + // Test JwtClaims TEST(JwtClaimsTest, ToJson) { JwtClaims claims; diff --git a/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py b/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py new file mode 100644 index 000000000..e51376e94 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py @@ -0,0 +1,293 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Check the TLS protocol floor and client-certificate verification. + +Both are driven by a real client against a real gateway. + +Both properties are about what happens during the TLS handshake, before any +HTTP request exists, so they cannot be observed from Python's requests or from +a unit test that checks a setter was called. Every assertion here comes from +``openssl s_client`` completing or failing a handshake at a pinned version. + +The client is run with ``-cipher ALL:@SECLEVEL=0``. Without it a modern +OpenSSL client refuses to OFFER TLS 1.0/1.1 on its own, and the test would pass +while proving nothing about the server: it has to be the server that says no. + +Two gateways run side by side, one with min_version 1.2 and one with 1.3, so +the floor is shown to MOVE with the setting rather than happening to sit where +OpenSSL's own default put it. + +@verifies REQ_INTEROP_086 +""" + +import os +import re +import shutil +import socket +import subprocess +import tempfile +import time +import unittest + +import launch +import launch_testing +import launch_testing.actions +import pytest + +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES, get_test_port +from ros2_medkit_test_utils.launch_helpers import create_gateway_node + +PORT_TLS12 = get_test_port(0) +PORT_TLS13 = get_test_port(1) +PORT_MTLS = get_test_port(2) + +_CERT_DIR = tempfile.mkdtemp(prefix='medkit_tls_floor_') + + +def _run(*args): + subprocess.run(args, check=True, capture_output=True) + + +def _make_ca(name): + """Build a CA key plus its self-signed certificate.""" + key = os.path.join(_CERT_DIR, f'{name}-ca-key.pem') + crt = os.path.join(_CERT_DIR, f'{name}-ca.pem') + _run('openssl', 'req', '-x509', '-newkey', 'rsa:2048', '-nodes', + '-keyout', key, '-out', crt, '-days', '1', + '-subj', f'/CN=medkit-test-{name}-ca') + return key, crt + + +def _make_leaf(name, ca_key, ca_crt, cn): + """Build a leaf key and certificate signed by the given CA.""" + key = os.path.join(_CERT_DIR, f'{name}-key.pem') + csr = os.path.join(_CERT_DIR, f'{name}.csr') + crt = os.path.join(_CERT_DIR, f'{name}.pem') + _run('openssl', 'req', '-newkey', 'rsa:2048', '-nodes', + '-keyout', key, '-out', csr, '-subj', f'/CN={cn}') + _run('openssl', 'x509', '-req', '-in', csr, '-CA', ca_crt, '-CAkey', ca_key, + '-CAcreateserial', '-out', crt, '-days', '1') + return key, crt + + +# The CA that signs the server certificate and the legitimate client. +CA_KEY, CA_CRT = _make_ca('trusted') +SRV_KEY, SRV_CRT = _make_leaf('server', CA_KEY, CA_CRT, 'localhost') +CLI_KEY, CLI_CRT = _make_leaf('client', CA_KEY, CA_CRT, 'medkit-test-client') + +# A second CA the gateway was never told about, for the certificate that is +# well-formed and correctly signed but by the wrong authority. +ROGUE_KEY, ROGUE_CRT = _make_ca('rogue') +ROGUE_CLI_KEY, ROGUE_CLI_CRT = _make_leaf('rogue-client', ROGUE_KEY, ROGUE_CRT, 'rogue') + + +def _tls_params(port, min_version, ca_file=''): + params = { + 'server.host': '127.0.0.1', + 'server.tls.enabled': True, + 'server.tls.cert_file': SRV_CRT, + 'server.tls.key_file': SRV_KEY, + 'server.tls.min_version': min_version, + # Auth off: this file is about the handshake, and a 401 would arrive + # long after the point under test has already been decided. + 'auth.enabled': False, + } + if ca_file: + params['server.tls.ca_file'] = ca_file + return params + + +@pytest.mark.launch_test +def generate_test_description(): + """Three gateways: floor at 1.2, floor at 1.3, and one demanding a client cert.""" + nodes = [ + create_gateway_node(port=PORT_TLS12, name='gateway_tls12', + extra_params=_tls_params(PORT_TLS12, '1.2')), + create_gateway_node(port=PORT_TLS13, name='gateway_tls13', + extra_params=_tls_params(PORT_TLS13, '1.3')), + create_gateway_node(port=PORT_MTLS, name='gateway_mtls', + extra_params=_tls_params(PORT_MTLS, '1.2', ca_file=CA_CRT)), + ] + return launch.LaunchDescription(nodes + [launch_testing.actions.ReadyToTest()]), { + 'gateway_tls12': nodes[0], + 'gateway_tls13': nodes[1], + 'gateway_mtls': nodes[2], + } + + +def _handshake(port, version, client_cert=None, client_key=None, timeout=20): + """Attempt one handshake. True only when a cipher was actually agreed. + + `openssl s_client` exits 0 in cases where no session was established, and + it prints the protocol it ATTEMPTED whether or not the server accepted it. + "Cipher is (NONE)" is the reliable tell for a handshake that did not + complete, so that is what is read here rather than the exit status. + """ + cmd = ['openssl', 's_client', f'-{version}', + '-cipher', 'ALL:@SECLEVEL=0', + '-connect', f'127.0.0.1:{port}'] + if client_cert: + cmd += ['-cert', client_cert, '-key', client_key] + try: + proc = subprocess.run(cmd, input=b'', capture_output=True, timeout=timeout) + except subprocess.TimeoutExpired: + return False + out = (proc.stdout + proc.stderr).decode(errors='replace') + + # "Cipher is " is NOT proof that the handshake completed, and reading + # it that way is how an earlier version of this file reported mutual TLS as + # broken when it was working. Under TLS 1.2 the cipher suite is agreed + # before the client certificate is examined, so a server that then rejects + # the certificate still leaves a cipher name in the output, followed by a + # fatal alert. Verified by hand against this gateway: a client with no + # certificate printed "Cipher is ECDHE-RSA-AES256-GCM-SHA384" AND + # "sslv3 alert handshake failure", while curl against the same endpoint got + # no HTTP response at all. + # + # So a fatal alert is the signal, and "Cipher is (NONE)" covers the case + # where the version itself was refused before any suite was picked. + if 'Cipher is (NONE)' in out: + return False + if re.search(r'alert (handshake failure|protocol version|certificate|unknown ca)', out): + return False + return 'Cipher is ' in out + + +def _wait_listening(port, timeout=60.0): + """Block until the port accepts a TCP connection. + + launch_testing starts the tests as soon as the processes are spawned, not + when they are serving, and a gateway that is not listening yet refuses + every connection. That looks identical to "the server rejected this + handshake", so without this gate the refusal assertions pass for the wrong + reason and the acceptance assertions fail at random. Observed directly: + the same file reported two failures, then two, then one, across three runs. + + TCP only, deliberately. A TLS handshake cannot be the readiness probe here + because on the mutual-TLS gateway a probe without a client certificate is + supposed to fail. + """ + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + try: + with socket.create_connection(('127.0.0.1', port), timeout=2): + return + except OSError: + time.sleep(0.25) + raise AssertionError(f'gateway on port {port} never started listening within {timeout}s') + + +class TestTlsProtocolFloor(unittest.TestCase): + """The floor moves with min_version, and it is the server that enforces it.""" + + @classmethod + def setUpClass(cls): + _wait_listening(PORT_TLS12) + _wait_listening(PORT_TLS13) + + def test_01_floor_12_accepts_12_and_13(self): + """The mirror of the refusals below. + + Without this, a gateway that refused every version would pass the + whole file while serving nobody. + """ + self.assertTrue(_handshake(PORT_TLS12, 'tls1_2'), 'TLS 1.2 must be accepted at floor 1.2') + self.assertTrue(_handshake(PORT_TLS12, 'tls1_3'), 'TLS 1.3 must be accepted at floor 1.2') + + def test_02_floor_12_refuses_11_and_10(self): + """SOVD requires TLS 1.2 as the minimum, so 1.1 and 1.0 must not connect. + + The vendored cpp-httplib asks OpenSSL for a floor of TLS 1.1 + (SSL_CTX_set_min_proto_version(ctx_, TLS1_1_VERSION)), so without the + gateway setting its own floor this is the version that decides whether + we comply, and it is not a value this project chose. + """ + self.assertFalse(_handshake(PORT_TLS12, 'tls1_1'), 'TLS 1.1 must be refused at floor 1.2') + self.assertFalse(_handshake(PORT_TLS12, 'tls1'), 'TLS 1.0 must be refused at floor 1.2') + + def test_03_floor_13_refuses_12(self): + """The test that fails if min_version is inert. + + A gateway configured for 1.3 that still completes a 1.2 handshake is + exactly the state this branch shipped before: the value was read, + logged, and then ignored. TLS 1.2 is accepted by the OTHER gateway in + this same launch, so a failure here cannot be blamed on the client or + on the certificate. + """ + self.assertFalse(_handshake(PORT_TLS13, 'tls1_2'), 'TLS 1.2 must be refused at floor 1.3') + self.assertFalse(_handshake(PORT_TLS13, 'tls1_1'), 'TLS 1.1 must be refused at floor 1.3') + + def test_04_floor_13_accepts_13(self): + self.assertTrue(_handshake(PORT_TLS13, 'tls1_3'), 'TLS 1.3 must be accepted at floor 1.3') + + +class TestMutualTls(unittest.TestCase): + """With ca_file set, a client certificate is required and verified.""" + + @classmethod + def setUpClass(cls): + _wait_listening(PORT_MTLS) + _wait_listening(PORT_TLS12) + + def test_05_no_client_certificate_is_refused(self): + """ca_file set means SSL_VERIFY_FAIL_IF_NO_PEER_CERT: no cert, no session.""" + self.assertFalse( + _handshake(PORT_MTLS, 'tls1_2'), + 'a client presenting no certificate must not complete the handshake' + ) + + def test_06_a_certificate_from_the_configured_ca_is_accepted(self): + self.assertTrue( + _handshake(PORT_MTLS, 'tls1_2', client_cert=CLI_CRT, client_key=CLI_KEY), + 'a client certificate signed by the configured CA must be accepted' + ) + + def test_07_a_certificate_from_another_ca_is_refused(self): + """Well-formed and correctly signed, but by an authority we never trusted. + + This separates "verification is on" from "any certificate will do", + which test_05 alone cannot. + """ + self.assertFalse( + _handshake(PORT_MTLS, 'tls1_2', client_cert=ROGUE_CLI_CRT, client_key=ROGUE_CLI_KEY), + 'a client certificate from an unconfigured CA must be refused' + ) + + def test_08_a_gateway_without_ca_file_does_not_demand_one(self): + """The default stays server-only TLS. + + SOVD authenticates with bearer tokens, so requiring a client + certificate by default would put us outside the spec. mTLS is opt-in + and this pins that it is. + """ + self.assertTrue( + _handshake(PORT_TLS12, 'tls1_2'), + 'a gateway with no ca_file must still serve a client that has no certificate' + ) + + +@launch_testing.post_shutdown_test() +class TestTlsFloorShutdown(unittest.TestCase): + """All three gateways exit cleanly.""" + + def test_exit_codes(self, proc_info, gateway_tls12, gateway_tls13, gateway_mtls): + for proc in (gateway_tls12, gateway_tls13, gateway_mtls): + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES, process=proc) + + @classmethod + def tearDownClass(cls): + shutil.rmtree(_CERT_DIR, ignore_errors=True) From 9bc6bcbcfa718739817e5132cb821f69a3f03bc1 Mon Sep 17 00:00:00 2001 From: bburda Date: Thu, 27 Aug 2026 22:04:09 +0200 Subject: [PATCH 4/4] gateway: close the gaps an adversarial review found in this branch Ten defects, all introduced or exposed by the hardening in this branch. Authentication ran AFTER the CORS preflight and the rate limiter in the pre-routing handler, and both of those return Handled. So with CORS enabled an anonymous OPTIONS on a protected route got 204, and an exhausted limiter answered 429, on a gateway configured to require a credential for everything. That is two exemptions nobody declared, reachable exactly when an operator enables CORS to let a browser talk to the box. Auth now runs first, and adds the CORS headers to its own refusal so a browser sees a 401 rather than an opaque network error. Three documented ways to start the gateway could not start it. The quickstart command passes a secret but no certificate, and TLS is now on by default; bringup.launch.py forwarded neither TLS nor auth arguments to gateway.launch.py; and the published container image had no auth block at all, so "the gateway ships closed" was untrue of the artefact most people run. bringup now forwards all six arguments, the quickstart passes tls_enabled:=false and says why, and the image ships with auth on: its entrypoint generates a per-container credential and prints it, or takes MEDKIT_JWT_SECRET and MEDKIT_CLIENTS, or MEDKIT_AUTH_DISABLED=1 to run open. An image that needed a secret before it would start at all is a quickstart nobody finishes, and the usual reaction to that is to turn authentication off and leave it off. generate_dev_certs.sh, the recovery path the new startup hints point at, was not executable, and it printed an example config containing ca_file - which now makes the gateway demand a client certificate and refuse the very curl printed below it. Documentation stated the old defaults in seven places, including the configuration reference and the auth and TLS tutorials, and a note added to docs/index.rst sat at column zero inside a code block, which terminated it and absorbed nine commands into the note body. Docutils reports nothing for that, so the -W gate would not have caught it. The tests proved less than they claimed. Nothing loaded the shipped config/gateway_params.yaml, so reverting it to auth.enabled false left the whole suite green; test_shipped_defaults.test.py now boots from that file. The secret comparison had no same-length near-miss, so shortening the loop by one byte would pass; it has one at each end now. test_08 never sent HEAD despite its name, and the methods it did send have no handler, so 404 satisfied it either way. The TLS negatives could not distinguish a server refusal from a client that never offered the version, which on a distro without TLS 1.0/1.1 makes them silently vacuous; a control now proves the client can reach those versions against a server that accepts them. --- README.md | 20 +- docker/entrypoint.sh | 49 ++++- docker/gateway_docker_params.yaml | 21 ++ docs/config/server.rst | 6 +- docs/getting_started.rst | 9 + docs/index.rst | 13 +- docs/tutorials/authentication.rst | 2 +- docs/tutorials/https.rst | 2 +- postman/README.md | 2 +- src/ros2_medkit_gateway/README.md | 16 +- .../launch/bringup.launch.py | 37 +++- .../launch/gateway.launch.py | 23 ++- .../launch/gateway_https.launch.py | 14 +- .../scripts/generate_dev_certs.sh | 6 +- .../src/http/rest_server.cpp | 44 ++-- .../test/test_auth_manager.cpp | 8 +- .../features/test_closed_by_default.test.py | 13 ++ .../features/test_shipped_defaults.test.py | 195 ++++++++++++++++++ .../features/test_tls_protocol_floor.test.py | 42 ++++ 19 files changed, 476 insertions(+), 46 deletions(-) mode change 100644 => 100755 src/ros2_medkit_gateway/scripts/generate_dev_certs.sh create mode 100644 src/ros2_medkit_integration_tests/test/features/test_shipped_defaults.test.py diff --git a/README.md b/README.md index 5e0db3e7e..be906bb26 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,8 @@ docker run --rm --network host --ipc host \ ghcr.io/selfpatch/ros2_medkit-jazzy:latest \ ros2 launch ros2_medkit_gateway bringup.launch.py # → REST API live at http://localhost:8080/api/v1/ +# The container prints a one-time client_secret on startup; use it to get a +# token. Pass -e MEDKIT_AUTH_DISABLED=1 to run without authentication. ``` Swap `jazzy` for `humble`/`lyrical`; the two `-e` flags forward your shell's `ROS_DOMAIN_ID` and @@ -96,7 +98,23 @@ picks them up it is a plain apt install too: ```bash sudo apt install ros-jazzy-ros2-medkit-gateway # or ros-humble- / ros-lyrical- -ros2 launch ros2_medkit_gateway bringup.launch.py + +# The gateway ships closed: it requires a credential and refuses to start +# without a signing secret. Supply one, and either a certificate or +# tls_enabled:=false on a host nothing else can reach. +ros2 launch ros2_medkit_gateway bringup.launch.py \ + tls_enabled:=false \ + jwt_secret:=change-me-to-at-least-32-characters-long \ + auth_clients:=demo:demo-secret:admin +``` + +Every `curl` below then needs a token: + +```bash +TOKEN=$(curl -s http://localhost:8080/api/v1/auth/authorize \ + -H 'Content-Type: application/json' \ + -d '{"grant_type":"client_credentials","client_id":"demo","client_secret":"demo-secret"}' \ + | jq -r .access_token) ``` > [!TIP] diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index ef82bb433..0eda05fed 100644 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -24,6 +24,53 @@ source "${COLCON_WS}/install/setup.bash" # Default to FastDDS (can be overridden via RMW_IMPLEMENTATION env var) export RMW_IMPLEMENTATION="${RMW_IMPLEMENTATION:-rmw_fastrtps_cpp}" +# Bootstrap credentials so the image can ship closed AND still start. +# +# The packaged params file turns authentication on, matching the gateway's own +# default, and the gateway refuses to start without a signing secret. An image +# that therefore needed a secret before `docker run` would do anything is a +# quickstart nobody completes, and the usual reaction is to turn auth off and +# leave it off. So: if the operator supplied nothing, generate a secret and an +# admin client for this container and print the credential once. +# +# Supply MEDKIT_JWT_SECRET and MEDKIT_CLIENTS to pin your own, or +# MEDKIT_AUTH_DISABLED=1 to run open on a host nothing else can reach. +AUTH_ARGS=() +if [ "${MEDKIT_AUTH_DISABLED:-0}" = "1" ]; then + AUTH_ARGS+=(-p auth.enabled:=false) + echo "ros2_medkit: MEDKIT_AUTH_DISABLED=1 - starting WITHOUT authentication." >&2 + echo " Every route is readable by anyone who can reach this port." >&2 +else + if [ -z "${MEDKIT_JWT_SECRET:-}" ]; then + # Per container, and not persisted: a restart issues a new one, which is + # correct for a credential nobody chose and nobody stored. + MEDKIT_JWT_SECRET="$(head -c 32 /dev/urandom | base64 | tr -d '\n' | tr '+/' '-_' | tr -d '=')" + MEDKIT_CLIENT_SECRET="$(head -c 24 /dev/urandom | base64 | tr -d '\n' | tr '+/' '-_' | tr -d '=')" + MEDKIT_CLIENTS="${MEDKIT_CLIENTS:-medkit:${MEDKIT_CLIENT_SECRET}:admin}" + echo "=============================================================" >&2 + echo "ros2_medkit: generated a one-time admin credential for this" >&2 + echo " container. It changes on every restart." >&2 + echo "" >&2 + echo " client_id: medkit" >&2 + echo " client_secret: ${MEDKIT_CLIENT_SECRET}" >&2 + echo "" >&2 + echo " curl -s http://localhost:8080/api/v1/auth/authorize \\" >&2 + echo " -H 'Content-Type: application/json' \\" >&2 + echo " -d '{\"grant_type\":\"client_credentials\",\"client_id\":\"medkit\",\"client_secret\":\"${MEDKIT_CLIENT_SECRET}\"}'" >&2 + echo "" >&2 + echo " Set MEDKIT_JWT_SECRET and MEDKIT_CLIENTS to pin your own." >&2 + echo "=============================================================" >&2 + fi + AUTH_ARGS+=(-p "auth.jwt_secret:=${MEDKIT_JWT_SECRET}") + [ -n "${MEDKIT_CLIENTS:-}" ] && AUTH_ARGS+=(-p "auth.clients:=[${MEDKIT_CLIENTS}]") +fi +# Exported so the other dispatch branch works too: `docker run ros2 launch +# ... bringup.launch.py` execs a command instead of the node, so it never sees +# AUTH_ARGS. gateway.launch.py falls back to these variables. +export MEDKIT_JWT_SECRET MEDKIT_CLIENTS MEDKIT_AUTH_DISABLED +# The image serves plain HTTP; see gateway_docker_params.yaml for why. +export MEDKIT_TLS_DISABLED=1 + # Dispatch on the first argument: # - empty, or starts with "-" (the default CMD "--ros-args --params-file ..." # or an override like --ros-args -p server.port:=9090): run the gateway node @@ -31,6 +78,6 @@ export RMW_IMPLEMENTATION="${RMW_IMPLEMENTATION:-rmw_fastrtps_cpp}" # - a full command (e.g. `ros2 launch ros2_medkit_gateway bringup.launch.py` # or `bash`): exec it as-is, so the image can launch the whole bringup stack. if [ -z "$1" ] || [ "${1#-}" != "$1" ]; then - exec ros2 run ros2_medkit_gateway gateway_node "$@" + exec ros2 run ros2_medkit_gateway gateway_node "$@" "${AUTH_ARGS[@]}" fi exec "$@" diff --git a/docker/gateway_docker_params.yaml b/docker/gateway_docker_params.yaml index 1d4049a57..2bf90df5e 100644 --- a/docker/gateway_docker_params.yaml +++ b/docker/gateway_docker_params.yaml @@ -6,6 +6,13 @@ ros2_medkit_gateway: server: host: "0.0.0.0" port: 8080 + # TLS stays OFF in the image, unlike config/gateway_params.yaml. A + # container has no certificate of its own and is almost always fronted by + # something that terminates TLS; requiring one here would mean the image + # could not start at all. Terminate TLS at your ingress, or mount a + # certificate and set server.tls.* yourself. + tls: + enabled: false refresh_interval_ms: 2000 # The web UI runs as a separate origin (its own host/port), so the # documented "run the web UI next to the gateway" path needs CORS. Without @@ -17,3 +24,17 @@ ros2_medkit_gateway: allowed_origins: - "http://localhost:3000" - "http://localhost:5173" + + # Authentication. On, matching config/gateway_params.yaml, so the published + # image has the same posture as the source default rather than a quietly + # weaker one. + # + # jwt_secret and clients are NOT set here on purpose: a secret baked into a + # published image is a secret every user of that image shares. The + # entrypoint generates one per container and prints the credential, or + # takes MEDKIT_JWT_SECRET / MEDKIT_CLIENTS from the environment. Run with + # MEDKIT_AUTH_DISABLED=1 to serve without authentication. + auth: + enabled: true + require_auth_for: "all" + issuer: "ros2_medkit_gateway" diff --git a/docs/config/server.rst b/docs/config/server.rst index 08db8d3c8..adce58f46 100644 --- a/docs/config/server.rst +++ b/docs/config/server.rst @@ -116,7 +116,7 @@ TLS/HTTPS Configuration - Description * - ``server.tls.enabled`` - bool - - ``false`` + - ``true`` - Enable HTTPS using OpenSSL. * - ``server.tls.cert_file`` - string @@ -857,7 +857,7 @@ default for local development. - Description * - ``auth.enabled`` - bool - - ``false`` + - ``true`` - Enable/disable JWT authentication. * - ``auth.jwt_secret`` - string @@ -881,7 +881,7 @@ default for local development. - Refresh token validity period in seconds (24 hours). Must be >= ``token_expiry_seconds``. * - ``auth.require_auth_for`` - string - - ``"write"`` + - ``"all"`` - When to require authentication: ``"none"`` (auth endpoints still available), ``"write"`` (POST/PUT/DELETE only), or ``"all"`` (every request). * - ``auth.issuer`` - string diff --git a/docs/getting_started.rst b/docs/getting_started.rst index 855b0d7f3..ed4bb8f66 100644 --- a/docs/getting_started.rst +++ b/docs/getting_started.rst @@ -41,6 +41,7 @@ Open three terminals. In each, source your workspace: .. code-block:: bash ros2 launch ros2_medkit_gateway gateway.launch.py \ + tls_enabled:=false \ jwt_secret:=change-me-to-at-least-32-characters-long \ auth_clients:=demo:demo-secret:admin @@ -142,6 +143,14 @@ Required if you want to test the Faults API. -d '{"grant_type":"client_credentials","client_id":"demo","client_secret":"demo-secret"}' \ | jq -r .access_token) + ``tls_enabled:=false`` is what keeps the rest of this page on ``http://``. + TLS is on in the shipped config and the gateway will not start without a + certificate, so a first run either turns it off, as here, or supplies one: + run ``scripts/generate_dev_certs.sh ./certs`` and pass + ``cert_file:=./certs/cert.pem key_file:=./certs/key.pem``. Turn it off only + on a host nothing else can reach. See :doc:`tutorials/https` for a real + certificate. + ``POST /api/v1/auth/authorize`` takes the ``client_credentials`` grant; ``/auth/token`` is the refresh endpoint and takes ``refresh_token``. diff --git a/docs/index.rst b/docs/index.rst index 6ebf46e35..e398425fa 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -45,19 +45,18 @@ Quick Links Quick Reference --------------- -Common commands for quick access: - -.. code-block:: bash - - # Check gateway health - curl http://localhost:8080/api/v1/health +Common commands for quick access. .. note:: - The gateway ships closed: every route below needs a credential, and + The gateway ships closed: every command below needs a credential, and ``GET /api/v1/health`` is the only one that does not. See :doc:`getting_started` for how to obtain ``$TOKEN``. +.. code-block:: bash + + # Check gateway health + curl http://localhost:8080/api/v1/health # List all areas curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/areas diff --git a/docs/tutorials/authentication.rst b/docs/tutorials/authentication.rst index 3c1431f21..9cd60e94b 100644 --- a/docs/tutorials/authentication.rst +++ b/docs/tutorials/authentication.rst @@ -11,7 +11,7 @@ Role-Based Access Control (RBAC) in ros2_medkit_gateway. Overview -------- -By default, the gateway runs without authentication for easy development. +The gateway requires authentication by default. For production deployments, you should enable authentication to: - Control who can access the API diff --git a/docs/tutorials/https.rst b/docs/tutorials/https.rst index 0812e0c2e..016f9380b 100644 --- a/docs/tutorials/https.rst +++ b/docs/tutorials/https.rst @@ -11,7 +11,7 @@ encrypted HTTPS communication with the gateway. Overview -------- -By default, the gateway uses plain HTTP. For production deployments, +The gateway serves HTTPS by default. For production deployments, you should enable TLS to: - Encrypt all traffic between clients and the gateway diff --git a/postman/README.md b/postman/README.md index fa55023bf..8a2f1fe73 100644 --- a/postman/README.md +++ b/postman/README.md @@ -92,7 +92,7 @@ ros2 launch ros2_medkit_gateway gateway.launch.py 4. Tokens are automatically saved to environment variables 5. Use `{{access_token}}` in Authorization header for protected endpoints -> **Note:** Auth endpoints are always accessible. By default (`require_auth_for: write`), only write operations (POST, PUT, DELETE) require authentication. GET requests work without a token. +> **Note:** Auth endpoints are always accessible. By default (`require_auth_for: all`), every request needs a token, GET included. **Discovery:** 1. Expand **"Discovery"** folder diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index a736dd654..5881a581e 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1477,32 +1477,32 @@ Cross-Origin Resource Sharing (CORS) settings for browser-based clients. CORS is #### Authentication Configuration -JWT-based authentication with Role-Based Access Control (RBAC). Authentication is **disabled by default** for backward compatibility. +JWT-based authentication with Role-Based Access Control (RBAC). Authentication is **enabled by default** for backward compatibility. | Parameter | Type | Default | Description | | ----------------------------------- | -------- | --------------------- | --------------------------------------------------------------------------- | -| `auth.enabled` | bool | `false` | Enable/disable authentication. Set to `true` to require auth. | +| `auth.enabled` | bool | `true` | Enable/disable authentication. Set to `true` to require auth. | | `auth.jwt_secret` | string | (required if enabled) | Secret key for HS256 signing. Must be at least 32 characters. | | `auth.jwt_algorithm` | string | `HS256` | JWT signing algorithm: `HS256` (symmetric) or `RS256` (asymmetric). | | `auth.token_expiry_seconds` | int | `3600` | Access token lifetime in seconds (range: 60-86400). | | `auth.refresh_token_expiry_seconds` | int | `86400` | Refresh token lifetime in seconds (range: 300-604800). | -| `auth.require_auth_for` | string | `write` | Auth requirement: `none`, `write` (POST/PUT/DELETE only), or `all`. | +| `auth.require_auth_for` | string | `all` | Auth requirement: `none`, `write` (POST/PUT/DELETE only), or `all`. | | `auth.issuer` | string | `ros2_medkit_gateway` | JWT issuer claim for token validation. | | `auth.clients` | string[] | `[]` | Client credentials in format `client_id:client_secret:role`. | #### TLS/HTTPS Configuration -TLS (Transport Layer Security) enables encrypted HTTPS communication. TLS is **disabled by default** for backward compatibility. +TLS (Transport Layer Security) enables encrypted HTTPS communication. TLS is **enabled by default**; the gateway refuses to start without a certificate. | Parameter | Type | Default | Description | | ---------------------------- | ------ | ------- | --------------------------------------------------------------------------- | -| `server.tls.enabled` | bool | `false` | Enable/disable TLS. When enabled, server uses HTTPS instead of HTTP. | +| `server.tls.enabled` | bool | `true` | Enable/disable TLS. When enabled, server uses HTTPS instead of HTTP. | | `server.tls.cert_file` | string | (required if enabled) | Path to PEM-encoded certificate file. | | `server.tls.key_file` | string | (required if enabled) | Path to PEM-encoded private key file. | -| `server.tls.ca_file` | string | `""` | Optional CA certificate (reserved for future mutual TLS support). | +| `server.tls.ca_file` | string | `""` | CA that signs CLIENT certificates. Setting it enables mutual TLS and REQUIRES a client certificate from every caller. | | `server.tls.min_version` | string | `"1.2"` | Minimum TLS version: `"1.2"` (compatible) or `"1.3"` (more secure). | -> **Note:** Mutual TLS (client certificate verification) is planned for a future release. +> **Note:** Mutual TLS is available: set `server.tls.ca_file`. **Roles and Permissions:** @@ -1573,7 +1573,7 @@ auth: jwt_algorithm: "HS256" token_expiry_seconds: 3600 refresh_token_expiry_seconds: 86400 - require_auth_for: "write" # GET requests work without auth + require_auth_for: "all" # every request needs a token, GET included issuer: "ros2_medkit_gateway" clients: - "admin:admin_secret:admin" diff --git a/src/ros2_medkit_gateway/launch/bringup.launch.py b/src/ros2_medkit_gateway/launch/bringup.launch.py index 4fb776651..76382940c 100644 --- a/src/ros2_medkit_gateway/launch/bringup.launch.py +++ b/src/ros2_medkit_gateway/launch/bringup.launch.py @@ -52,6 +52,12 @@ def generate_launch_description(): server_host = LaunchConfiguration('server_host') server_port = LaunchConfiguration('server_port') cors_allowed_origins = LaunchConfiguration('cors_allowed_origins') + tls_enabled = LaunchConfiguration('tls_enabled') + cert_file = LaunchConfiguration('cert_file') + key_file = LaunchConfiguration('key_file') + auth_enabled = LaunchConfiguration('auth_enabled') + jwt_secret = LaunchConfiguration('jwt_secret') + auth_clients = LaunchConfiguration('auth_clients') args = [ DeclareLaunchArgument( @@ -70,6 +76,32 @@ def generate_launch_description(): default_value='http://localhost:3000,http://localhost:5173', description='Comma-separated CORS origins allowed to call the gateway from a browser, ' 'so the web UI works out of the box. Empty disables CORS.'), + # Forwarded to gateway.launch.py. Without these the gateway's own + # defaults apply and bringup cannot start at all: the shipped config + # turns TLS and authentication on, and the gateway refuses to run + # without a certificate and a signing secret. Passing them here is what + # makes `ros2 launch ... bringup.launch.py tls_enabled:=false ...` a + # complete command rather than a dead end. + DeclareLaunchArgument( + 'tls_enabled', default_value='true', + description='Serve HTTPS. Needs cert_file and key_file; pass false to serve ' + 'plain HTTP on a host nothing else can reach.'), + DeclareLaunchArgument( + 'cert_file', default_value='', + description='PEM certificate for HTTPS. Required while tls_enabled is true.'), + DeclareLaunchArgument( + 'key_file', default_value='', + description='PEM private key matching cert_file.'), + DeclareLaunchArgument( + 'auth_enabled', default_value='true', + description='Require a credential on every request.'), + DeclareLaunchArgument( + 'jwt_secret', default_value='', + description='HS256 signing secret, at least 32 characters. Required while ' + 'auth_enabled is true.'), + DeclareLaunchArgument( + 'auth_clients', default_value='', + description='Comma-separated "client_id:client_secret:role" triples.'), DeclareLaunchArgument( 'enable_fault_manager', default_value='true', description='Start the fault_manager node.'), @@ -90,7 +122,10 @@ def generate_launch_description(): gateway = _include( 'ros2_medkit_gateway', 'gateway.launch.py', launch_arguments={'server_host': server_host, 'server_port': server_port, - 'cors_allowed_origins': cors_allowed_origins}) + 'cors_allowed_origins': cors_allowed_origins, + 'tls_enabled': tls_enabled, 'cert_file': cert_file, + 'key_file': key_file, 'auth_enabled': auth_enabled, + 'jwt_secret': jwt_secret, 'auth_clients': auth_clients}) fault_manager = _include( 'ros2_medkit_fault_manager', 'fault_manager.launch.py', enable_arg='enable_fault_manager', diff --git a/src/ros2_medkit_gateway/launch/gateway.launch.py b/src/ros2_medkit_gateway/launch/gateway.launch.py index 8160b45f0..4425bf4af 100644 --- a/src/ros2_medkit_gateway/launch/gateway.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway.launch.py @@ -170,9 +170,16 @@ def _launch_setup(context, *_args, **_kwargs): tls_enabled = LaunchConfiguration('tls_enabled').perform(context).lower() in ( 'true', '1', 'yes') + # Environment override read BEFORE the value is used: the container + # image serves plain HTTP behind whatever terminates TLS for it, and it + # has no certificate of its own to offer. + if os.environ.get('MEDKIT_TLS_DISABLED') == '1': + tls_enabled = False param_overrides['server.tls.enabled'] = tls_enabled - cert_file = LaunchConfiguration('cert_file').perform(context) - key_file = LaunchConfiguration('key_file').perform(context) + cert_file = (LaunchConfiguration('cert_file').perform(context) + or os.environ.get('MEDKIT_TLS_CERT_FILE', '')) + key_file = (LaunchConfiguration('key_file').perform(context) + or os.environ.get('MEDKIT_TLS_KEY_FILE', '')) if cert_file: param_overrides['server.tls.cert_file'] = cert_file if key_file: @@ -186,11 +193,19 @@ def _launch_setup(context, *_args, **_kwargs): 'or pass tls_enabled:=false to serve plain HTTP. ' 'scripts/generate_dev_certs.sh makes a self-signed pair for a first run.') + # Launch argument first, then the environment. The environment path is + # what makes the container image work: its entrypoint generates a + # per-container credential and exports it, and `ros2 launch` inside that + # container has no other way to receive it. auth_enabled = LaunchConfiguration('auth_enabled').perform(context).lower() in ( 'true', '1', 'yes') + if os.environ.get('MEDKIT_AUTH_DISABLED') == '1': + auth_enabled = False param_overrides['auth.enabled'] = auth_enabled - jwt_secret = LaunchConfiguration('jwt_secret').perform(context) - clients = LaunchConfiguration('auth_clients').perform(context) + jwt_secret = (LaunchConfiguration('jwt_secret').perform(context) + or os.environ.get('MEDKIT_JWT_SECRET', '')) + clients = (LaunchConfiguration('auth_clients').perform(context) + or os.environ.get('MEDKIT_CLIENTS', '')) if jwt_secret: param_overrides['auth.jwt_secret'] = jwt_secret if clients: diff --git a/src/ros2_medkit_gateway/launch/gateway_https.launch.py b/src/ros2_medkit_gateway/launch/gateway_https.launch.py index b7a269100..3bc70649b 100644 --- a/src/ros2_medkit_gateway/launch/gateway_https.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway_https.launch.py @@ -90,7 +90,13 @@ def generate_certificates(cert_dir: str) -> dict: return { 'cert_file': cert_file, 'key_file': key_file, - 'ca_file': ca_file if os.path.exists(ca_file) else '', + # Deliberately NOT passed as server.tls.ca_file. This CA signs the + # SERVER certificate so a client can verify the gateway; setting it + # as the gateway's ca_file turns on mutual TLS and rejects every + # client that has no certificate of its own, including the curl + # this launch file prints. Kept here only so the hint below can + # tell the user which CA to pass with --cacert. + 'ca_file_for_client': ca_file if os.path.exists(ca_file) else '', } os.makedirs(cert_dir, exist_ok=True) @@ -153,7 +159,9 @@ def generate_certificates(cert_dir: str) -> dict: return { 'cert_file': cert_file, 'key_file': key_file, - 'ca_file': ca_file, + # See the note above: this is the CA a CLIENT verifies the server with, + # not a client-certificate authority for the gateway to demand. + 'ca_file_for_client': ca_file, } @@ -196,7 +204,7 @@ def launch_setup(context): LogInfo(msg=[f' curl -k https://{server_host}:{server_port}/api/v1/health']), LogInfo(msg=['']), LogInfo(msg=['Test with CA verification:']), - LogInfo(msg=[f' curl --cacert {cert_paths["ca_file"]} ' + LogInfo(msg=[f' curl --cacert {cert_paths["ca_file_for_client"]} ' f'https://{server_host}:{server_port}/api/v1/health']), LogInfo(msg=['='*60]), diff --git a/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh b/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh old mode 100644 new mode 100755 index 16fd0c216..e14b8551e --- a/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh +++ b/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh @@ -122,7 +122,11 @@ echo " tls:" echo " enabled: true" echo " cert_file: \"$OUTPUT_DIR/cert.pem\"" echo " key_file: \"$OUTPUT_DIR/key.pem\"" -echo " ca_file: \"$OUTPUT_DIR/ca.pem\"" +echo "" +echo " Do NOT add ca_file here. It is not the CA a client verifies the server" +echo " with - setting it makes the gateway REQUIRE a client certificate from" +echo " every caller, and the curl below would then be refused. Pass ca.pem to" +echo " the client with --cacert instead, as shown." echo "" echo "Test with curl:" echo " curl -k https://localhost:8080/api/v1/health" diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index ec4d8a603..41ba065bc 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -239,7 +239,37 @@ void RESTServer::setup_pre_routing_handler() { // Set up pre-routing handler for CORS and Authentication // This handler runs before any route handler srv->set_pre_routing_handler([this](const httplib::Request & req, httplib::Response & res) { - // 1. Handle CORS (existing logic) + // 1. Authentication, before anything else can answer. + // + // Order matters and it used to be wrong. CORS preflight below answers an + // OPTIONS with 204, and the rate limiter answers with 429; both used to run + // first and both RETURN Handled, so on a gateway with require_auth_for + // "all" an anonymous caller could still get a real answer out of a + // protected route. That is two exemptions nobody declared, and they appear + // exactly when an operator enables CORS to let a browser talk to the box. + // + // Putting auth first costs a preflight its CORS headers when the caller has + // no credential, which is correct: a browser that cannot authenticate has + // no business being told what the box would have allowed. + if (auth_middleware_ && auth_middleware_->is_enabled()) { + auto auth_request = AuthMiddleware::from_httplib_request(req); + auto result = auth_middleware_->process(auth_request); + + if (!result.allowed) { + // CORS headers on the refusal itself, so a browser sees a 401 rather + // than an opaque network error it cannot report to the user. + if (cors_config_.enabled) { + std::string origin = req.get_header_value("Origin"); + if (!origin.empty() && is_origin_allowed(origin)) { + set_cors_headers(res, origin); + } + } + AuthMiddleware::apply_to_response(result, res); + return httplib::Server::HandlerResponse::Handled; + } + } + + // 2. Handle CORS (existing logic) if (cors_config_.enabled) { std::string origin = req.get_header_value("Origin"); bool origin_allowed = !origin.empty() && is_origin_allowed(origin); @@ -279,18 +309,6 @@ void RESTServer::setup_pre_routing_handler() { // 1. Handle CORS (existing logic) - // Handle Authentication if enabled - if (auth_middleware_ && auth_middleware_->is_enabled()) { - // Use AuthMiddleware to process the request - auto auth_request = AuthMiddleware::from_httplib_request(req); - auto result = auth_middleware_->process(auth_request); - - if (!result.allowed) { - AuthMiddleware::apply_to_response(result, res); - return httplib::Server::HandlerResponse::Handled; - } - } - return httplib::Server::HandlerResponse::Unhandled; }); } diff --git a/src/ros2_medkit_gateway/test/test_auth_manager.cpp b/src/ros2_medkit_gateway/test/test_auth_manager.cpp index 0c4550e58..d6ebe7271 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -666,7 +666,13 @@ TEST(AuthManagerSecretComparisonTest, OnlyTheExactSecretAuthenticates) { EXPECT_TRUE(manager.authenticate("svc", "svc_secret").has_value()) << "the real secret must work"; - for (const auto & wrong : {"", "s", "svc_secre", "svc_secret_", "svc_secretX", "SVC_SECRET", "xxxxxxxxxx"}) { + // The last two are the ones that matter. Everything before them differs in + // length, or in the first byte, so a comparison that checked the length and + // then only a prefix would satisfy the whole list. "svc_secreT" differs only + // in the FINAL byte: shorten the comparison loop by one and it is accepted + // while every other case here still fails correctly. + for (const auto & wrong : + {"", "s", "svc_secre", "svc_secret_", "svc_secretX", "SVC_SECRET", "xxxxxxxxxx", "svc_secreT", "Svc_secret"}) { EXPECT_FALSE(manager.authenticate("svc", wrong).has_value()) << "secret \"" << wrong << "\" was accepted"; } } diff --git a/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py index dfc3083df..948e339b4 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py @@ -297,6 +297,19 @@ def test_08_head_on_health_is_not_a_way_in(self): Widening it to "any method on /health" is the natural next edit and it would be wrong, so the narrowness is pinned here. """ + # HEAD first, and separately, because it is the method that matters and + # the one an earlier version of this test left out despite its name. + # cpp-httplib dispatches HEAD into the GET handler table, so if the + # exemption stopped checking the method, HEAD would return the status + # document to an anonymous caller. The others have no handler at all on + # this path, so they answer 404 either way and cannot show the + # difference on their own. + head = requests.head(f'{CLOSED_BASE_URL}/health', timeout=15) + self.assertIn( + head.status_code, (401, 403), + f'HEAD /health answered {head.status_code} to an anonymous caller' + ) + for method in ('POST', 'PUT', 'DELETE', 'PATCH'): with self.subTest(method=method): resp = requests.request( diff --git a/src/ros2_medkit_integration_tests/test/features/test_shipped_defaults.test.py b/src/ros2_medkit_integration_tests/test/features/test_shipped_defaults.test.py new file mode 100644 index 000000000..5f3c32a75 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_shipped_defaults.test.py @@ -0,0 +1,195 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Boot the gateway from the SHIPPED config file and check the posture. + +Every other test in this suite builds its parameters inline, which is fine for +testing behaviour but means nothing here ever loaded +``config/gateway_params.yaml``. That left the branch in an odd state: the file +could be reverted to ``auth.enabled: false`` and the whole suite would stay +green, because each test supplies the values it needs itself. + +This file closes that gap. It launches with ``--params-file`` pointing at the +installed copy of the shipped config, overriding only the port, the signing +secret and the client - the three things a real deployment must supply and the +file deliberately leaves empty - and then checks that what ships is closed. + +TLS is turned off here and only here. The shipped file has it on, which is +correct, but a certificate is a deployment artefact and generating one would +test the certificate rather than the posture. ``test_tls_protocol_floor`` +covers TLS itself against real handshakes. + +@verifies REQ_INTEROP_086, REQ_INTEROP_087 +""" + +import os +import socket +import time +import unittest + +from ament_index_python.packages import get_package_share_directory +import launch +import launch_ros.actions +import launch_testing +import launch_testing.actions +import pytest +import requests + +from ros2_medkit_test_utils.constants import ( + ALLOWED_EXIT_CODES, + API_BASE_PATH, + get_test_port, +) +from ros2_medkit_test_utils.coverage import get_coverage_env + +PORT = get_test_port() +BASE_URL = f'http://127.0.0.1:{PORT}{API_BASE_PATH}' + +SHIPPED_PARAMS = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), 'config', 'gateway_params.yaml' +) + +JWT_SECRET = 'shipped_defaults_integration_secret_key_0123456789' +CLIENT_ID = 'shipped' +CLIENT_SECRET = 'shipped_client_secret' + + +@pytest.mark.launch_test +def generate_test_description(): + """Launch the gateway with the shipped params file, plus the required secrets.""" + gateway_node = launch_ros.actions.Node( + package='ros2_medkit_gateway', + executable='gateway_node', + name='ros2_medkit_gateway', + output='screen', + parameters=[ + SHIPPED_PARAMS, + { + 'server.host': '127.0.0.1', + 'server.port': PORT, + 'refresh_interval_ms': 1000, + # A certificate is a deployment artefact, not part of the + # posture under test here. + 'server.tls.enabled': False, + # What the shipped file leaves empty on purpose. + 'auth.jwt_secret': JWT_SECRET, + 'auth.clients': [f'{CLIENT_ID}:{CLIENT_SECRET}:admin'], + }, + ], + additional_env=dict(get_coverage_env()), + ) + + return launch.LaunchDescription([ + gateway_node, + launch_testing.actions.ReadyToTest(), + ]), {'gateway_node': gateway_node} + + +def _wait_listening(port, timeout=90.0): + """Block until the gateway accepts a connection. + + launch_testing starts the tests when the process is spawned, not when it is + serving. Without this the first request is refused by a gateway that simply + has not opened its socket yet, which looks nothing like the posture this + file is about. + + The timeout is generous because this gateway loads the full shipped config, + which does more work at startup than the inline parameter sets the rest of + the suite uses. + """ + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + try: + with socket.create_connection(('127.0.0.1', port), timeout=2): + return + except OSError: + time.sleep(0.25) + raise AssertionError(f'gateway on port {port} never started listening within {timeout}s') + + +class TestShippedDefaults(unittest.TestCase): + """What config/gateway_params.yaml actually produces.""" + + @classmethod + def setUpClass(cls): + _wait_listening(PORT) + resp = requests.post( + f'{BASE_URL}/auth/authorize', + json={ + 'grant_type': 'client_credentials', + 'client_id': CLIENT_ID, + 'client_secret': CLIENT_SECRET, + }, + timeout=30, + ) + assert resp.status_code == 200, f'token request failed: {resp.status_code} {resp.text}' + cls.auth = {'Authorization': f'Bearer {resp.json()["access_token"]}'} + + def test_01_the_shipped_file_turns_authentication_on(self): + """Reverting auth.enabled in the shipped file must fail here. + + No other test would notice: they all pass auth.enabled themselves. + """ + resp = requests.get(f'{BASE_URL}/areas', timeout=15) + self.assertIn( + resp.status_code, (401, 403), + 'the shipped config served /areas to an anonymous caller' + ) + + def test_02_the_shipped_file_covers_reads_not_just_writes(self): + """Pins require_auth_for: "all" as the shipped value. + + Under "write" every one of these answers 200 without a credential. + """ + for path in ('/', '/areas', '/components', '/apps', '/functions', '/version-info'): + with self.subTest(path=path): + resp = requests.get(f'{BASE_URL}{path}', timeout=15) + self.assertIn(resp.status_code, (401, 403)) + + def test_03_health_stays_reachable(self): + """The exemption has to survive in the shipped file too. + + Without it a container supervisor cannot probe the gateway and every + deployment restart-loops, so this is as much a part of the shipped + posture as the refusals above. + """ + resp = requests.get(f'{BASE_URL}/health', timeout=15) + self.assertEqual(resp.status_code, 200) + + def test_04_a_configured_client_still_works(self): + """The mirror: a gateway that refused everyone would pass the rest.""" + resp = requests.get(f'{BASE_URL}/areas', headers=self.auth, timeout=15) + self.assertEqual(resp.status_code, 200) + + def test_05_the_shipped_file_is_the_one_under_test(self): + """Guard against this test silently drifting off the real file. + + If the installed config stops declaring the values this file exists to + check, the assertions above would still pass for the wrong reason. + """ + with open(SHIPPED_PARAMS, encoding='utf-8') as handle: + text = handle.read() + self.assertIn('require_auth_for: "all"', text) + self.assertIn('enabled: true', text) + + +@launch_testing.post_shutdown_test() +class TestShippedDefaultsShutdown(unittest.TestCase): + """Gateway exits cleanly.""" + + def test_exit_codes(self, proc_info, gateway_node): + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES, process=gateway_node + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py b/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py index e51376e94..6a629cce2 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py @@ -166,6 +166,13 @@ def _handshake(port, version, client_cert=None, client_key=None, timeout=20): return 'Cipher is ' in out +def _free_port(): + """Return a port nothing is listening on, for the control server above.""" + with socket.socket() as sock: + sock.bind(('127.0.0.1', 0)) + return sock.getsockname()[1] + + def _wait_listening(port, timeout=60.0): """Block until the port accepts a TCP connection. @@ -207,6 +214,41 @@ def test_01_floor_12_accepts_12_and_13(self): self.assertTrue(_handshake(PORT_TLS12, 'tls1_2'), 'TLS 1.2 must be accepted at floor 1.2') self.assertTrue(_handshake(PORT_TLS12, 'tls1_3'), 'TLS 1.3 must be accepted at floor 1.2') + def test_01b_the_client_actually_offers_the_old_versions(self): + """Guard the negative assertions below against becoming vacuous. + + `_handshake` returns False both when the SERVER refuses and when the + client never put a ClientHello on the wire. A modern OpenSSL will not + offer TLS 1.0/1.1 unless `-cipher ALL:@SECLEVEL=0` persuades it, and on + a distro built `no-tls1 no-tls1_1`, or under a crypto policy pinning + MinProtocol, it cannot offer them at all. In either case test_02 below + would pass against a gateway happily serving TLS 1.0. + + So: stand up a plain `openssl s_server` that accepts everything, and + require the client to reach 1.0 and 1.1 against it. If it cannot, the + refusals in test_02 prove nothing and this fails instead of lying. + """ + for version in ('tls1', 'tls1_1'): + with self.subTest(version=version): + port = _free_port() + server = subprocess.Popen( + ['openssl', 's_server', '-accept', str(port), '-quiet', + '-cert', SRV_CRT, '-key', SRV_KEY, + '-cipher', 'ALL:@SECLEVEL=0', f'-{version}'], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + ) + try: + _wait_listening(port, timeout=15) + self.assertTrue( + _handshake(port, version), + f'this client cannot offer {version} at all, so the ' + f'{version} refusals in test_02 would pass against a ' + 'gateway that accepts it' + ) + finally: + server.terminate() + server.wait(timeout=10) + def test_02_floor_12_refuses_11_and_10(self): """SOVD requires TLS 1.2 as the minimum, so 1.1 and 1.0 must not connect.