From 7c41bb630aecabed7d80b72f7f88edde002b3fe2 Mon Sep 17 00:00:00 2001 From: bburda Date: Wed, 26 Aug 2026 19:55:55 +0200 Subject: [PATCH 1/7] 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 0321f22fc..2dc630be1 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -944,7 +944,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. @@ -1719,7 +1719,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):** @@ -1740,7 +1740,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):** @@ -3187,7 +3187,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):** @@ -3242,7 +3242,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 17b46c9b0..1082cd69b 100644 --- a/docs/tutorials/graph-provider.rst +++ b/docs/tutorials/graph-provider.rst @@ -211,7 +211,7 @@ 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. A Function detail response carries an ``"x-medkit-graph"`` link exactly while this @@ -220,7 +220,7 @@ The Discovery Path .. 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. @@ -235,7 +235,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 f17d55d7e..cd3aad85c 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 @@ -737,7 +737,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 d6a4dce4d..21c4f4b4c 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 aad73dd5d..3ca774862 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -1130,6 +1130,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 b7381f315731045dc741d5d64505ed47dff48f02 Mon Sep 17 00:00:00 2001 From: bburda Date: Wed, 26 Aug 2026 20:42:44 +0200 Subject: [PATCH 2/7] 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 fcc0948d0..197368d01 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 8117313d4283cff4c3c3b4560efbbb6ee64b3b22 Mon Sep 17 00:00:00 2001 From: bburda Date: Thu, 27 Aug 2026 14:28:32 +0200 Subject: [PATCH 3/7] 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 21c4f4b4c..4ed889810 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 578a927cb..da0570c8e 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 @@ -186,6 +186,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 @@ -242,6 +249,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 51ac0f626..51a44c162 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; @@ -350,13 +398,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); @@ -365,10 +411,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_); @@ -609,6 +664,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 a8f32986e..601a2f07c 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 3ca774862..895e59931 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -677,6 +677,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 4d001e45706beff954a919ad83d43a8111065b7c Mon Sep 17 00:00:00 2001 From: bburda Date: Thu, 27 Aug 2026 22:04:09 +0200 Subject: [PATCH 4/7] 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 +- .../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 ++++ 18 files changed, 445 insertions(+), 33 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 0171b268a..d7356a43b 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 301ab65bd..47f40619a 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 0cff2b0d3..4b4ee543d 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1485,32 +1485,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:** @@ -1581,7 +1581,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/test/test_auth_manager.cpp b/src/ros2_medkit_gateway/test/test_auth_manager.cpp index 895e59931..9d6ac1a89 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -757,7 +757,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. From 78a3aa029bf3a476283df2b000082f78ce7a3d4a Mon Sep 17 00:00:00 2001 From: bburda Date: Sun, 6 Sep 2026 18:58:45 +0200 Subject: [PATCH 5/7] gateway: keep CORS preflight working, and stop the launch file overriding a config file The previous commit put authentication ahead of the CORS preflight. That was wrong. A browser never sends Authorization on a preflight: asking permission before issuing the real request, headers included, is the whole point of the mechanism. Requiring a credential there does not harden anything, it makes every browser client impossible, and an authenticated preflight got 403 from the RBAC check because OPTIONS maps to no permission. The order is now preflight, then authentication, then the rate limiter. The preflight is a third named exemption alongside GET /health and /auth/*, and it is safe for a narrow reason: it returns no body, only the CORS policy for an origin the operator configured, and the real request that follows is authenticated normally. The exemption also now requires Access-Control-Request-Method, so it covers genuine preflights rather than the OPTIONS method in general. Authentication before the rate limiter was the half that was genuinely wrong and it stays fixed: an anonymous caller who exhausted the allowance used to get 429 from a protected route instead of 401. The launch file also stopped overriding config files. tls_enabled and auth_enabled defaulted to "true" and were applied unconditionally, so a deployment passing config_file with an explicit auth.enabled false got authentication anyway, and MEDKIT_TLS_DISABLED beat an explicit tls_enabled:=true, which would have turned an existing HTTPS deployment into a plaintext listener on its HTTPS port. Both are empty by default now, and the precedence is explicit argument, then environment, then the config file. Two consumers that the new defaults broke are fixed here: pixi-smoke-test.sh launched the gateway with only a port and could no longer start it, and the Postman collection saved an access token that no request ever sent. Tests cover the preflight boundary, including the plain OPTIONS that carries no Access-Control-Request-Method, and a gateway with a two-per-minute rate limit so the ordering between the limiter and authentication is observable at all - with the limiter disabled, as everywhere else in the suite, that ordering cannot be tested. --- ...os2-medkit-gateway.postman_collection.json | 12 +- scripts/pixi-smoke-test.sh | 7 +- .../launch/bringup.launch.py | 12 +- .../launch/gateway.launch.py | 50 +++-- .../src/http/rest_server.cpp | 52 +++-- .../features/test_closed_by_default.test.py | 202 +++++++++++++++++- 6 files changed, 290 insertions(+), 45 deletions(-) diff --git a/postman/collections/ros2-medkit-gateway.postman_collection.json b/postman/collections/ros2-medkit-gateway.postman_collection.json index 5a138a0da..db08042f6 100644 --- a/postman/collections/ros2-medkit-gateway.postman_collection.json +++ b/postman/collections/ros2-medkit-gateway.postman_collection.json @@ -2101,5 +2101,15 @@ } ] } - ] + ], + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{access_token}}", + "type": "string" + } + ] + } } diff --git a/scripts/pixi-smoke-test.sh b/scripts/pixi-smoke-test.sh index 077baddc9..6a8203a03 100755 --- a/scripts/pixi-smoke-test.sh +++ b/scripts/pixi-smoke-test.sh @@ -29,7 +29,12 @@ fi PORT="${GATEWAY_SMOKE_PORT:-8080}" TIMEOUT=30 -ros2 launch ros2_medkit_gateway gateway.launch.py server_port:="$PORT" & +# The shipped config has TLS and authentication on, so a bare launch refuses to +# start. This is a smoke test for "does the gateway come up and answer", not for +# the security posture, so it runs the way a developer on a local machine would: +# plain HTTP, no credential. test_shipped_defaults covers the shipped posture. +ros2 launch ros2_medkit_gateway gateway.launch.py \ + server_port:="$PORT" tls_enabled:=false auth_enabled:=false & GW_PID=$! # shellcheck disable=SC2317 # cleanup is invoked indirectly via trap diff --git a/src/ros2_medkit_gateway/launch/bringup.launch.py b/src/ros2_medkit_gateway/launch/bringup.launch.py index 76382940c..442b91668 100644 --- a/src/ros2_medkit_gateway/launch/bringup.launch.py +++ b/src/ros2_medkit_gateway/launch/bringup.launch.py @@ -83,9 +83,10 @@ def generate_launch_description(): # 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.'), + 'tls_enabled', default_value='', + description='Serve HTTPS. Empty means leave it to the config file, which ' + 'has it on. Pass false to serve plain HTTP on a host nothing ' + 'else can reach; needs cert_file and key_file when on.'), DeclareLaunchArgument( 'cert_file', default_value='', description='PEM certificate for HTTPS. Required while tls_enabled is true.'), @@ -93,8 +94,9 @@ def generate_launch_description(): 'key_file', default_value='', description='PEM private key matching cert_file.'), DeclareLaunchArgument( - 'auth_enabled', default_value='true', - description='Require a credential on every request.'), + 'auth_enabled', default_value='', + description='Require a credential on every request. Empty means leave it ' + 'to the config file, which has it on.'), DeclareLaunchArgument( 'jwt_secret', default_value='', description='HS256 signing secret, at least 32 characters. Required while ' diff --git a/src/ros2_medkit_gateway/launch/gateway.launch.py b/src/ros2_medkit_gateway/launch/gateway.launch.py index 4425bf4af..1ec3c716c 100644 --- a/src/ros2_medkit_gateway/launch/gateway.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway.launch.py @@ -105,7 +105,7 @@ def generate_launch_description(): 'pass auth_enabled:=false explicitly.')) declare_auth_enabled_arg = DeclareLaunchArgument( - 'auth_enabled', default_value='true', + 'auth_enabled', default_value='', description=( 'Require a credential. On by default, matching the shipped ' 'config. Turning it off makes the entity tree, the fault history ' @@ -119,7 +119,7 @@ def generate_launch_description(): 'a token from /auth/token.')) declare_tls_enabled_arg = DeclareLaunchArgument( - 'tls_enabled', default_value='true', + 'tls_enabled', default_value='', description=( 'Serve HTTPS. On by default, matching the shipped config. Needs ' 'cert_file and key_file; the gateway refuses to start with TLS on ' @@ -168,14 +168,25 @@ 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') - # 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': + # Precedence: an explicit launch argument, then the environment, then + # whatever the config file says. Unset means "do not touch it", which + # matters because this launch file is included by others and is used + # with config_file: re-asserting a default here would silently override + # a value someone put in their own file on purpose. + tls_arg = LaunchConfiguration('tls_enabled').perform(context).strip().lower() + if tls_arg: + tls_enabled = tls_arg in ('true', '1', 'yes') + param_overrides['server.tls.enabled'] = tls_enabled + elif os.environ.get('MEDKIT_TLS_DISABLED') == '1': + # The container image serves plain HTTP behind whatever terminates + # TLS for it, and has no certificate of its own. An explicit + # tls_enabled:= above still wins over this. tls_enabled = False - param_overrides['server.tls.enabled'] = tls_enabled + param_overrides['server.tls.enabled'] = False + else: + # Nothing said otherwise, so the shipped config decides. It has TLS + # on, and the warning below still applies to that case. + tls_enabled = True cert_file = (LaunchConfiguration('cert_file').perform(context) or os.environ.get('MEDKIT_TLS_CERT_FILE', '')) key_file = (LaunchConfiguration('key_file').perform(context) @@ -193,15 +204,20 @@ 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': + # Same precedence as TLS above: explicit argument, then environment, + # then the config file. 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_arg = LaunchConfiguration('auth_enabled').perform(context).strip().lower() + if auth_arg: + auth_enabled = auth_arg in ('true', '1', 'yes') + param_overrides['auth.enabled'] = auth_enabled + elif os.environ.get('MEDKIT_AUTH_DISABLED') == '1': auth_enabled = False - param_overrides['auth.enabled'] = auth_enabled + param_overrides['auth.enabled'] = False + else: + auth_enabled = True jwt_secret = (LaunchConfiguration('jwt_secret').perform(context) or os.environ.get('MEDKIT_JWT_SECRET', '')) clients = (LaunchConfiguration('auth_clients').perform(context) diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index 5b070aece..0c16bb0d8 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -317,8 +317,26 @@ void RESTServer::setup_pre_routing_handler() { } } - // 2. Handle preflight OPTIONS requests - if (req.method == "OPTIONS") { + // 2. Handle preflight OPTIONS requests. + // + // This is answered WITHOUT a credential, and it has to be. A browser + // never puts Authorization on a preflight - asking permission before + // sending the real request, headers included, is the entire purpose of + // the mechanism - so requiring one here does not harden the gateway, it + // makes every browser client impossible. + // + // It is safe because a preflight discloses nothing about the system: the + // response is the CORS policy for an origin the operator configured, with + // no body, and the real request that follows is authenticated normally. + // Treat this as a third named exemption alongside GET /health and + // /auth/*, not as an oversight. + // A real preflight carries Access-Control-Request-Method; the browser + // sends it to ask whether the method it is about to use is allowed. + // Requiring it keeps the exemption to the case that genuinely cannot + // authenticate. Without that check a plain anonymous OPTIONS with an + // allowed Origin took the same early return, which widens a narrow, + // necessary exemption into "OPTIONS is public". + if (req.method == "OPTIONS" && req.has_header("Access-Control-Request-Method")) { if (origin_allowed) { res.set_header("Access-Control-Max-Age", std::to_string(cors_config_.max_age_seconds)); res.status = 204; @@ -329,21 +347,13 @@ void RESTServer::setup_pre_routing_handler() { } } - // 3. Rate limiting check. If rejected, return Handled (CORS headers already set) - if (rate_limiter_ && rate_limiter_->is_enabled() && req.method != "OPTIONS") { - auto rl_result = rate_limiter_->check(req.remote_addr, req.path); - RateLimiter::apply_headers(rl_result, res); - if (!rl_result.allowed) { - RateLimiter::apply_rejection(rl_result, res); - return handled(req, res); - } - } - - // 1. Handle CORS (existing logic) - - // Handle Authentication if enabled + // 3. Authentication, before the rate limiter. + // + // This half of the order was wrong. The limiter also returns Handled, so an + // anonymous caller who exhausted the allowance used to get 429 from a + // protected route instead of 401 - an answer, and a small disclosure of + // limiter state, without any credential. 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); @@ -353,6 +363,16 @@ void RESTServer::setup_pre_routing_handler() { } } + // 4. Rate limiting check. If rejected, return Handled (CORS headers already set) + if (rate_limiter_ && rate_limiter_->is_enabled() && req.method != "OPTIONS") { + auto rl_result = rate_limiter_->check(req.remote_addr, req.path); + RateLimiter::apply_headers(rl_result, res); + if (!rl_result.allowed) { + RateLimiter::apply_rejection(rl_result, res); + return handled(req, res); + } + } + return httplib::Server::HandlerResponse::Unhandled; }); } 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 948e339b4..ccb13d5ba 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 @@ -47,8 +47,13 @@ from ros2_medkit_test_utils.launch_helpers import create_gateway_node CLOSED_PORT = get_test_port() +CORS_PORT = get_test_port(1) CLOSED_BASE_URL = f'http://127.0.0.1:{CLOSED_PORT}{API_BASE_PATH}' CLOSED_ROOT = f'http://127.0.0.1:{CLOSED_PORT}' +CORS_BASE_URL = f'http://127.0.0.1:{CORS_PORT}{API_BASE_PATH}' +RL_PORT = get_test_port(2) +RL_BASE_URL = f'http://127.0.0.1:{RL_PORT}{API_BASE_PATH}' +ALLOWED_ORIGIN = 'https://ui.example' # At least 32 characters, or the gateway refuses to start under HS256. JWT_SECRET = 'closed_by_default_integration_secret_key_0123456789' @@ -80,10 +85,48 @@ def generate_test_description(): }, ) + cors_gateway = create_gateway_node( + port=CORS_PORT, + name='gateway_with_cors', + extra_params={ + 'server.host': '127.0.0.1', + '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'], + # The configuration where the hole lived. + 'cors.allowed_origins': [ALLOWED_ORIGIN], + }, + ) + + # Rate limiting on, and tight, so the limiter can actually be exhausted + # inside a test. Without a gateway in this state the ordering between the + # limiter and authentication is unobservable, which is how it went + # unnoticed in the first place. + rl_gateway = create_gateway_node( + port=RL_PORT, + name='gateway_with_rate_limit', + extra_params={ + 'server.host': '127.0.0.1', + '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'], + 'rate_limiting.enabled': True, + 'rate_limiting.global_requests_per_minute': 2, + 'rate_limiting.client_requests_per_minute': 2, + }, + ) + return launch.LaunchDescription([ gateway_node, + cors_gateway, + rl_gateway, launch_testing.actions.ReadyToTest(), - ]), {'gateway_node': gateway_node} + ]), {'gateway_node': gateway_node, 'cors_gateway': cors_gateway, + 'rl_gateway': rl_gateway} def _is_exempt(method, path): @@ -321,11 +364,160 @@ def test_08_head_on_health_is_not_a_way_in(self): ) +class TestNothingAnswersBeforeAuth(GatewayTestCase): + """What the CORS preflight may and may not do without a credential. + + Preflight is answered anonymously on purpose, and it is the third named + exemption after GET /health and /auth/*. A browser never puts Authorization + on a preflight - asking permission before sending the real request is the + whole point of the mechanism - so demanding one would not harden anything, + it would make browser clients impossible. An earlier version of this branch + did exactly that, and the control below is what caught it. + + What must hold instead: the preflight discloses only CORS policy, and the + REAL request that follows is still refused without a credential. + + This gateway enables CORS for a real origin, which the rest of the file + deliberately does not, because that is the configuration in which any of + this is reachable at all. + """ + + BASE_URL = CORS_BASE_URL + + def _preflight(self, extra=None): + headers = {'Origin': ALLOWED_ORIGIN, 'Access-Control-Request-Method': 'GET'} + headers.update(extra or {}) + return requests.options(f'{CORS_BASE_URL}/apps', headers=headers, timeout=15) + + def test_01_an_anonymous_preflight_is_answered(self): + """The exemption, stated as a test rather than left implicit. + + This is the control that failed when the branch briefly required a + credential here: a browser cannot send one, so a 401 or 403 means no + browser client can reach this gateway at all. + """ + resp = self._preflight() + self.assertEqual( + resp.status_code, 204, + f'an anonymous preflight got {resp.status_code}; a browser cannot ' + 'authenticate a preflight, so this makes browser clients impossible' + ) + self.assertEqual(resp.headers.get('Access-Control-Allow-Origin'), ALLOWED_ORIGIN) + + def test_02_the_preflight_discloses_only_cors_policy(self): + """Why the exemption is safe: there is nothing in the response. + + If a preflight ever grew a body, the exemption would start leaking and + this fails rather than letting it pass unnoticed. + """ + resp = self._preflight() + self.assertEqual( + resp.content, b'', + f'the preflight returned a body: {resp.content[:200]!r}' + ) + + def test_03_a_preflight_from_an_unknown_origin_is_refused(self): + """The exemption is scoped to origins the operator configured.""" + resp = requests.options( + f'{CORS_BASE_URL}/apps', + headers={'Origin': 'https://not-configured.example', + 'Access-Control-Request-Method': 'GET'}, + timeout=15, + ) + self.assertEqual(resp.status_code, 403) + + def test_04_the_real_request_after_a_preflight_still_needs_a_credential(self): + """The property that actually matters. + + A preflight being answered must not carry any implication for the GET + that follows it, which is where the data is. + """ + resp = requests.get( + f'{CORS_BASE_URL}/apps', headers={'Origin': ALLOWED_ORIGIN}, timeout=15 + ) + self.assertIn( + resp.status_code, (401, 403), + f'a cross-origin GET got {resp.status_code} with no credential' + ) + + def test_04b_a_plain_options_without_the_preflight_header_is_refused(self): + """The exemption is for preflights, not for the OPTIONS method. + + A browser preflight always carries Access-Control-Request-Method. An + OPTIONS without it is an ordinary request that any client could send, + and it has no reason to skip the credential check. The helper above + always sends both headers, so this boundary needs its own case. + """ + resp = requests.options( + f'{CORS_BASE_URL}/apps', + headers={'Origin': ALLOWED_ORIGIN}, + timeout=15, + ) + self.assertIn( + resp.status_code, (401, 403), + f'a plain OPTIONS with no Access-Control-Request-Method got ' + f'{resp.status_code}; the preflight exemption is too wide' + ) + + def test_05_an_authenticated_cross_origin_request_works(self): + """The mirror: CORS is live and a credentialed browser call succeeds.""" + headers = {'Origin': ALLOWED_ORIGIN} + headers.update(self.cors_auth) + resp = requests.get(f'{CORS_BASE_URL}/apps', headers=headers, timeout=15) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.headers.get('Access-Control-Allow-Origin'), ALLOWED_ORIGIN) + + @classmethod + def setUpClass(cls): + super().setUpClass() + resp = requests.post( + f'{CORS_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}' + cls.cors_auth = {'Authorization': f'Bearer {resp.json()["access_token"]}'} + + +class TestRateLimiterDoesNotAnswerBeforeAuth(GatewayTestCase): + """An anonymous caller gets 401, never 429. + + The rate limiter runs in the same pre-routing handler and also returns + "handled". With it ordered before authentication, a caller with no + credential who exhausted the allowance received 429 from a protected route: + an answer, plus a small disclosure of limiter state, without ever presenting + anything. The limit here is deliberately tiny so the exhausted state is + reachable in a test at all. + """ + + BASE_URL = RL_BASE_URL + + def test_01_an_exhausted_anonymous_caller_still_gets_401(self): + seen = [] + # Comfortably past a limit of 2/minute. + for _ in range(8): + seen.append(requests.get(f'{RL_BASE_URL}/apps', timeout=15).status_code) + + self.assertNotIn( + 429, seen, + f'an anonymous caller was rate-limited instead of refused: {seen}' + ) + self.assertTrue( + all(code in (401, 403) for code in seen), + f'expected only 401/403 for an uncredentialed caller, got {seen}' + ) + + @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 - ) + def test_exit_codes(self, proc_info, gateway_node, cors_gateway, rl_gateway): + for proc in (gateway_node, cors_gateway, rl_gateway): + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES, process=proc + ) From d403bdb91c1f2599490fc0b0ae965b75d2ba0226 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 7 Sep 2026 15:47:03 +0200 Subject: [PATCH 6/7] gateway: make the public routes a setting, not a hardcoded exemption Under require_auth_for "all" the gateway refuses every route except /api/v1/auth/*, health included. A deployment that needs a route answered without a credential names it in auth.public_routes, written "METHOD /path" and matched exactly. No wildcards - they are rejected rather than matched literally - so the list is the whole public surface and reads as one. Empty as shipped, and absent from the config file rather than written as an empty sequence, which carries no type and stops the node. Every entry is logged at WARN on startup and a malformed entry stops the gateway. An anonymous caller on such a route gets liveness only, marked x-medkit-reduced so an empty warnings array is not read as "nothing is wrong". Also here, each closing a way the shipped posture could be lost: - the image asserts auth.enabled on the command line, so an argument override that replaces the params file cannot leave the gateway open - the image leaves an operator's own params file and config_file alone instead of overriding their credentials and their TLS setting - an unrecognised tls_enabled or auth_enabled value stops the launch instead of meaning false, so a typo cannot disable encryption - the rate limiter meters a request before authentication and answers after it, so a refused credential still costs its allowance, and an exhausted allowance refuses without verifying a signature - a refresh record outlives its own expiry by one access-token lifetime, so the last token minted from it keeps the life it was promised - an unreadable client CA bundle stops startup instead of leaving mutual TLS advertised and every handshake failing The integration harness treats any HTTP answer as proof the gateway is up. Requiring 200 from /health made it wait for an auth configuration rather than for a process. --- docker/entrypoint.sh | 43 +++- docs/config/server.rst | 33 +++ docs/tutorials/docker.rst | 45 +++- .../config/gateway_params.yaml | 30 ++- .../core/auth/auth_config.hpp | 10 + .../core/auth/auth_requirement_policy.hpp | 96 +++++-- .../ros2_medkit_gateway/dto/health.hpp | 21 +- .../launch/gateway.launch.py | 29 ++- .../src/core/auth/auth_config.cpp | 5 + .../src/core/auth/auth_manager.cpp | 21 +- .../src/core/auth/auth_requirement_policy.cpp | 102 +++++++- src/ros2_medkit_gateway/src/gateway_node.cpp | 22 ++ .../src/http/handlers/health_handlers.cpp | 18 +- .../src/http/http_server.cpp | 14 ++ .../src/http/rest_server.cpp | 53 ++-- .../test/test_auth_manager.cpp | 177 +++++++++++-- .../gateway_test_case.py | 12 +- .../features/test_closed_by_default.test.py | 238 +++++++++++------- .../features/test_shipped_defaults.test.py | 32 ++- 19 files changed, 821 insertions(+), 180 deletions(-) diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index 0eda05fed..936fbf0ff 100644 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -35,11 +35,32 @@ export RMW_IMPLEMENTATION="${RMW_IMPLEMENTATION:-rmw_fastrtps_cpp}" # # 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. + +# The params file the node will actually read, if any. `docker run` can replace +# the default CMD, so this cannot be assumed: an operator who mounts their own +# file has already decided the auth configuration, and generating a credential +# on top of it would override the one their clients hold. +PARAMS_FILE="" +prev="" +for arg in "$@"; do + if [ "$prev" = "--params-file" ]; then PARAMS_FILE="$arg"; fi + prev="$arg" +done + +# True when that file names its own signing secret, which is the marker that +# the operator brought their own credentials rather than wanting generated ones. +operator_supplied_auth() { + [ -n "$PARAMS_FILE" ] && [ -r "$PARAMS_FILE" ] && grep -qE '^[[:space:]]*jwt_secret[[:space:]]*:' "$PARAMS_FILE" +} + 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 +elif operator_supplied_auth; then + echo "ros2_medkit: ${PARAMS_FILE} carries its own auth.jwt_secret - using it as-is," >&2 + echo " generating nothing and overriding nothing." >&2 else if [ -z "${MEDKIT_JWT_SECRET:-}" ]; then # Per container, and not persisted: a restart issues a new one, which is @@ -61,6 +82,19 @@ else echo " Set MEDKIT_JWT_SECRET and MEDKIT_CLIENTS to pin your own." >&2 echo "=============================================================" >&2 fi + # Asserted here, not left to the params file. `docker run --ros-args -p + # server.port:=9090` REPLACES the default CMD, so the params file is never + # loaded, and the node's compiled-in default for auth.enabled is false - the + # image would print an admin credential and then serve every route to anyone. + # Saying it on the command line means the image is closed however it is + # started, and it is a no-op when the params file is loaded and agrees. + AUTH_ARGS+=(-p auth.enabled:=true) + if [ -z "$PARAMS_FILE" ]; then + # Same reasoning one level down: with no params file the compiled-in + # require_auth_for is "write", which leaves every read open. A file, even a + # mounted one, is the operator's statement about this and is left alone. + AUTH_ARGS+=(-p auth.require_auth_for:=all) + fi AUTH_ARGS+=(-p "auth.jwt_secret:=${MEDKIT_JWT_SECRET}") [ -n "${MEDKIT_CLIENTS:-}" ] && AUTH_ARGS+=(-p "auth.clients:=[${MEDKIT_CLIENTS}]") fi @@ -69,7 +103,14 @@ fi # 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 +# +# Only when the operator has not brought a config of their own. This is the +# image's DEFAULT, not a policy: `docker run ros2 launch ... +# gateway.launch.py config_file:=/mnt/secure.yaml` with TLS on in that file must +# get TLS, and an unconditional export here silently served it plaintext. +if [ -z "$PARAMS_FILE" ] && ! printf '%s\n' "$@" | grep -q '^config_file:='; then + export MEDKIT_TLS_DISABLED=1 +fi # Dispatch on the first argument: # - empty, or starts with "-" (the default CMD "--ros-args --params-file ..." diff --git a/docs/config/server.rst b/docs/config/server.rst index d7356a43b..b0e15d6be 100644 --- a/docs/config/server.rst +++ b/docs/config/server.rst @@ -891,6 +891,10 @@ default for local development. - string[] - ``[]`` - Pre-configured clients as ``"client_id:client_secret:role"`` strings. + * - ``auth.public_routes`` + - string[] + - ``[]`` + - Routes answered with no credential, each written ``"METHOD /path"``. Layers over ``require_auth_for`` and only ever removes a requirement. Matched exactly, no wildcards. Every entry is logged at ``WARN`` on startup, and a malformed entry stops the gateway. .. note:: @@ -922,6 +926,35 @@ Example: token_expiry_seconds: 3600 clients: ["admin:REPLACE_WITH_STRONG_SECRET:admin", "viewer:REPLACE_WITH_STRONG_SECRET:viewer"] +Opening a route to uncredentialed callers +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Under ``require_auth_for: "all"`` the only routes answered without a credential +are ``/api/v1/auth/*``, because authentication cannot bootstrap through a door +that demands the credential it hands out. Health is not special and is refused +like everything else. + +When something that cannot hold a credential has to reach a route, name it: + +.. code-block:: yaml + + auth: + public_routes: ["GET /api/v1/health"] + +Matching is exact. ``GET /api/v1/health`` opens that method on that path and +nothing else - not ``HEAD``, not ``/api/v1/healthz``, not the subtree. Wildcards +are rejected rather than accepted and matched literally, and a malformed entry +stops the gateway rather than being dropped silently. + +An anonymous caller on such a route gets a reduced body: ``GET /health`` answers +with ``status`` and ``timestamp`` only, plus ``x-medkit-reduced: true`` so a +monitor can tell a withheld answer from a clean one. A credential still returns +the whole document. + +Most liveness probes need no entry at all. A ``401`` already proves the process +is up and answering HTTP, so a probe that accepts ``200``, ``401`` and ``403`` +works against any auth configuration and leaves nothing open. Prefer that. + See :doc:`/tutorials/authentication` for a complete setup tutorial. Plugin Framework diff --git a/docs/tutorials/docker.rst b/docs/tutorials/docker.rst index 7840f613f..92abb4382 100644 --- a/docs/tutorials/docker.rst +++ b/docs/tutorials/docker.rst @@ -58,12 +58,19 @@ Test the gateway: .. code-block:: bash - curl http://localhost:8080/api/v1/health - # {"status":"healthy","timestamp":...} + curl -i http://localhost:8080/api/v1/health + # HTTP/1.1 401 Unauthorized + # The image ships closed: every route needs a credential, health included. + # A 401 here is the gateway working - it received the request, routed it and + # refused it. curl -H "Authorization: Bearer $TOKEN" http://localhost:8080/api/v1/version-info # {"items":[{"version":"","vendor_info":{"name":"ros2_medkit",...}}]} +The container prints a generated ``client_id`` and ``client_secret`` on its +first line of output; exchange them for ``$TOKEN`` at ``/api/v1/auth/authorize``. +See :doc:`authentication`. + Custom Configuration -------------------- @@ -156,7 +163,15 @@ Example ``docker-compose.yml`` with the gateway and web UI: environment: - ROS_DOMAIN_ID=42 healthcheck: - test: ["CMD", "curl", "-f", "http://localhost:8080/api/v1/health"] + # Any HTTP answer proves the process is up, 401 included. `curl -f` + # would exit non-zero on the refusal an authenticated gateway gives + # an uncredentialed probe, and report a healthy container as sick. + test: + - CMD-SHELL + - >- + code=$$(curl -s -o /dev/null -w '%{http_code}' + http://localhost:8080/api/v1/health) && case "$$code" in + 200|401|403) exit 0 ;; *) exit 1 ;; esac interval: 10s timeout: 5s retries: 3 @@ -230,17 +245,37 @@ writes. Add your own UI origin(s): Health Checks ------------- -The gateway exposes a health endpoint at ``/api/v1/health``: +The gateway exposes a health endpoint at ``/api/v1/health``. It needs a +credential like every other route, so a probe should read the status code +rather than insist on success: .. code-block:: yaml healthcheck: - test: ["CMD", "curl", "-f", "http://localhost:8080/api/v1/health"] + # 401 means the gateway is up and refused an uncredentialed probe, which + # is exactly what a liveness check wants to know. + test: + - CMD-SHELL + - >- + code=$$(curl -s -o /dev/null -w '%{http_code}' + http://localhost:8080/api/v1/health) && case "$$code" in + 200|401|403) exit 0 ;; *) exit 1 ;; esac interval: 10s timeout: 5s retries: 3 start_period: 15s +If a probe cannot be changed - a load balancer that only accepts 200, say - +open the route explicitly instead: + +.. code-block:: yaml + + auth: + public_routes: ["GET /api/v1/health"] + +An anonymous caller then gets liveness only, marked ``x-medkit-reduced``. See +:doc:`/config/server` for what that setting does and does not open. + Production Considerations ------------------------- diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index 4ed889810..8c30d4674 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -415,10 +415,36 @@ ros2_medkit_gateway: # 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. + # Only /auth/* stays public under "all", because authentication cannot + # bootstrap through a door that demands the credential it hands out. require_auth_for: "all" + # Routes answered with no credential at all, on top of whatever + # require_auth_for decides. Each entry is "METHOD /path", matched + # exactly: no wildcards, so this list is the whole public surface and a + # reviewer can read it as such. + # + # Empty as shipped. Add an entry when something that cannot hold a + # credential has to reach a route - a container supervisor or a load + # balancer probing health is the case this exists for: + # + # public_routes: ["GET /api/v1/health"] + # + # A liveness probe usually needs no entry: a 401 already proves the + # process is up and answering HTTP. Prefer teaching the probe to accept + # it over opening the route. When the route is opened, the body an + # anonymous caller gets is narrowed to liveness - no entity names, no + # counts - so the probe works and the disclosure does not follow. + # + # Every entry is logged at WARN on startup, once per route. + # + # Left absent rather than written as `public_routes: []`. An empty YAML + # sequence carries no type, so rclcpp cannot tell a string array from any + # other and the node dies at startup with "No parameter value set". The + # declared default is already empty, so absence and `[]` mean the same + # thing - one of them just starts. + # public_routes: ["GET /api/v1/health"] + # JWT issuer claim # Default: "ros2_medkit_gateway" issuer: "ros2_medkit_gateway" diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_config.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_config.hpp index b77acb99a..3970a1aa0 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_config.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_config.hpp @@ -91,6 +91,15 @@ struct AuthConfig { // Pre-configured clients (for development/testing) std::vector clients; + /// Routes answered with no credential, each written "METHOD /path". + /// + /// Empty by default, so `require_auth_for` alone decides and the gateway is + /// closed as shipped. An operator adds an entry when something that cannot + /// hold a credential has to reach a route - a container supervisor probing + /// `GET /api/v1/health` is the case this exists for. Matching is exact and + /// there are no wildcards, so the list reads as the whole public surface. + std::vector public_routes; + /// Permission entries for the routes the `RouteRegistry` does not hold. /// /// The gateway's own routes derive their entries from their registration @@ -119,6 +128,7 @@ class AuthConfigBuilder { AuthConfigBuilder & with_refresh_token_expiry(int seconds); AuthConfigBuilder & with_require_auth_for(AuthRequirement requirement); AuthConfigBuilder & with_issuer(const std::string & issuer); + AuthConfigBuilder & with_public_routes(const std::vector & public_routes); AuthConfigBuilder & add_client(const std::string & client_id, const std::string & client_secret, UserRole role); AuthConfig build(); 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 d724cf622..2a6960696 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 @@ -14,6 +14,8 @@ #pragma once +#include + #include #include #include @@ -69,33 +71,22 @@ class NoAuthRequirementPolicy : public IAuthRequirementPolicy { /** * @brief Policy that always requires authentication * - * Except for two public endpoints: the auth endpoints, and the health probe. + * The only exception is anything under `/api/v1/auth/`. Authentication cannot + * bootstrap through a door that demands the credential it exists to hand out. + * + * Nothing else is public here, health probes included. An operator who needs + * a route answered without a credential names it in `auth.public_routes`, + * which layers over this policy - see PublicRouteExemptionPolicy. */ class AllAuthRequirementPolicy : public IAuthRequirementPolicy { public: bool requires_authentication(const std::string & method, const std::string & path) const override { - // 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; + (void)method; + return path.find("/api/v1/auth/") != 0; } std::string description() const override { - return "AllAuth: Authentication required for all endpoints except /auth/* and GET /health"; + return "AllAuth: Authentication required for all endpoints except /auth/*"; } }; @@ -170,6 +161,62 @@ class ConfigurableAuthRequirementPolicy : public IAuthRequirementPolicy { bool use_requirements_map_; }; +/// One entry of `auth.public_routes`: a method and a path this gateway answers +/// with no credential at all. +struct PublicRoute { + std::string method; ///< Upper-case HTTP method, e.g. "GET" + std::string path; ///< Full request path, e.g. "/api/v1/health" + + bool operator==(const PublicRoute & other) const { + return method == other.method && path == other.path; + } +}; + +/// Parses one `auth.public_routes` entry, written "METHOD /path". +/// +/// Matching is exact and there are no wildcards, so an operator cannot open a +/// subtree by accident: every route that stops requiring a credential is a +/// line somebody wrote and a reviewer can read. `GET /api/v1/health` opens the +/// health probe and nothing else, where `GET /api/v1/*` would have opened the +/// whole read surface with one character. +/// +/// @return the parsed route, or a message naming what is wrong with the entry. +tl::expected parse_public_route(const std::string & entry); + +/// Parses a whole `auth.public_routes` list, dropping entries that do not +/// parse. Dropping keeps the route protected, which is the safe reading of a +/// malformed entry; GatewayNode validates the list first and refuses to start +/// rather than let a typo silently protect a route the operator wanted open. +std::vector parse_public_routes(const std::vector & entries); + +/** + * @brief Layers an operator's `auth.public_routes` over another policy + * + * `require_auth_for` stays the primary axis; this only ever *removes* the + * credential requirement, never adds one, so wrapping cannot make a gateway + * stricter than the policy underneath and cannot be used to shadow it. + * + * The gateway ships with an empty list, which makes this a no-op: closed by + * default, and open exactly where somebody said so. + */ +class PublicRouteExemptionPolicy : public IAuthRequirementPolicy { + public: + PublicRouteExemptionPolicy(std::unique_ptr inner, std::vector public_routes); + + bool requires_authentication(const std::string & method, const std::string & path) const override; + + std::string description() const override; + + /// The routes this layer exempts, in the order they were configured. + const std::vector & public_routes() const { + return public_routes_; + } + + private: + std::unique_ptr inner_; + std::vector public_routes_; +}; + /** * @brief Factory to create auth requirement policies from configuration */ @@ -188,6 +235,15 @@ class AuthRequirementPolicyFactory { * @return Policy implementation based on config.enabled and config.auth_requirements */ static std::unique_ptr create(const AuthConfig & config); + + /** + * @brief Create policy for a requirement level, exempting configured routes + * @param requirement The auth requirement level + * @param public_routes Entries of `auth.public_routes`, already parsed + * @return The requirement policy, wrapped only when the list is non-empty + */ + static std::unique_ptr create(AuthRequirement requirement, + const std::vector & public_routes); }; } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/health.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/health.hpp index 7cbcb7cd5..32f31bf99 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/health.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/health.hpp @@ -172,10 +172,21 @@ struct Health { /// why a peer refused their stream with 503. std::optional x_medkit_sse; std::optional peers; // free-form array of peer status objects + /// Present and true only when this answer was cut down for an anonymous + /// caller. Wire key: "x-medkit-reduced". + /// + /// `warnings` below promises that an empty array means "nothing flagged". + /// That promise cannot hold on a route an operator put in + /// `auth.public_routes`, where the sections are skipped before they are + /// built, so a monitor would read a withheld body as a clean bill of health. + /// This field is what tells it apart: when it is present, `warnings` says + /// nothing about the gateway and the caller needs a credential to learn more. + std::optional x_medkit_reduced; // wire key: "x-medkit-reduced" // NOT optional: these two are emitted in every mode, aggregation or not, so // the generated schema must list them in `required` and let a typed client // read them without a presence check. An empty `warnings` array is the - // "nothing flagged" answer - absence is not a value this endpoint has. + // "nothing flagged" answer - absence is not a value this endpoint has, + // except as qualified by `x_medkit_reduced` above. int64_t warning_schema_version{kWarningSchemaVersion}; std::vector warnings; }; @@ -190,8 +201,12 @@ inline constexpr auto dto_fields = std::make_tuple( field("discovery", &Health::discovery), field("x-medkit-data-provider", &Health::x_medkit_data_provider), field("x-medkit-subscription-executor", &Health::x_medkit_subscription_executor), field("x-medkit-entity-cache", &Health::x_medkit_entity_cache), field("x-medkit-sse", &Health::x_medkit_sse), - field("peers", &Health::peers), field("warning_schema_version", &Health::warning_schema_version), - field("warnings", &Health::warnings)); + field("peers", &Health::peers), + field("x-medkit-reduced", &Health::x_medkit_reduced, + "Present and true only when the caller presented no credential and the operator opened this route in " + "auth.public_routes. The answer then carries liveness only, and `warnings` says nothing about the " + "gateway - authenticate to read the full document."), + field("warning_schema_version", &Health::warning_schema_version), field("warnings", &Health::warnings)); template <> inline constexpr std::string_view dto_name = "HealthStatus"; diff --git a/src/ros2_medkit_gateway/launch/gateway.launch.py b/src/ros2_medkit_gateway/launch/gateway.launch.py index 1ec3c716c..9b6353ea5 100644 --- a/src/ros2_medkit_gateway/launch/gateway.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway.launch.py @@ -27,6 +27,27 @@ CORS_DEFAULT = 'http://localhost:3000,http://localhost:5173' +def parse_security_flag(name, raw): + """ + Return True/False for a security launch argument, or raise on anything else. + + An allowlist for true with everything else meaning false is the wrong shape + here: ``tls_enabled:=on`` and ``tls_enabled:=ture`` would both mean "serve + plain HTTP", and the override is still written, so the typo beats a config + file that had TLS on. For a flag whose two values are "encrypted" and "not", + an unrecognised spelling has to stop the launch rather than pick one. + """ + value = raw.strip().lower() + if value in ('true', '1', 'yes', 'on'): + return True + if value in ('false', '0', 'no', 'off'): + return False + raise RuntimeError( + f'{name}:={raw!r} is not a boolean. Use true or false. ' + f'Leaving {name} unset lets the config file decide.' + ) + + def cors_override(cors_arg, config_file, default_config): """ Return the ``cors.allowed_origins`` entry for the final overrides, or {}. @@ -173,9 +194,9 @@ def _launch_setup(context, *_args, **_kwargs): # matters because this launch file is included by others and is used # with config_file: re-asserting a default here would silently override # a value someone put in their own file on purpose. - tls_arg = LaunchConfiguration('tls_enabled').perform(context).strip().lower() + tls_arg = LaunchConfiguration('tls_enabled').perform(context).strip() if tls_arg: - tls_enabled = tls_arg in ('true', '1', 'yes') + tls_enabled = parse_security_flag('tls_enabled', tls_arg) param_overrides['server.tls.enabled'] = tls_enabled elif os.environ.get('MEDKIT_TLS_DISABLED') == '1': # The container image serves plain HTTP behind whatever terminates @@ -209,9 +230,9 @@ def _launch_setup(context, *_args, **_kwargs): # 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_arg = LaunchConfiguration('auth_enabled').perform(context).strip().lower() + auth_arg = LaunchConfiguration('auth_enabled').perform(context).strip() if auth_arg: - auth_enabled = auth_arg in ('true', '1', 'yes') + auth_enabled = parse_security_flag('auth_enabled', auth_arg) param_overrides['auth.enabled'] = auth_enabled elif os.environ.get('MEDKIT_AUTH_DISABLED') == '1': auth_enabled = False diff --git a/src/ros2_medkit_gateway/src/core/auth/auth_config.cpp b/src/ros2_medkit_gateway/src/core/auth/auth_config.cpp index 93de53553..6d6daf9d9 100644 --- a/src/ros2_medkit_gateway/src/core/auth/auth_config.cpp +++ b/src/ros2_medkit_gateway/src/core/auth/auth_config.cpp @@ -87,6 +87,11 @@ AuthConfigBuilder & AuthConfigBuilder::with_issuer(const std::string & issuer) { return *this; } +AuthConfigBuilder & AuthConfigBuilder::with_public_routes(const std::vector & public_routes) { + config_.public_routes = public_routes; + return *this; +} + AuthConfigBuilder & AuthConfigBuilder::add_client(const std::string & client_id, const std::string & client_secret, UserRole role) { ClientCredentials creds; 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 51a44c162..3e92fcd85 100644 --- a/src/ros2_medkit_gateway/src/core/auth/auth_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/auth/auth_manager.cpp @@ -89,8 +89,11 @@ AuthManager::AuthManager(const AuthConfig & config) : config_(config) { clients_[client.client_id] = client; } - // Create auth requirement policy from config - auth_policy_ = AuthRequirementPolicyFactory::create(config_.require_auth_for); + // Create auth requirement policy from config. `require_auth_for` decides the + // baseline; `public_routes` then lifts the credential requirement from the + // routes an operator named, and from nothing else. + auth_policy_ = + AuthRequirementPolicyFactory::create(config_.require_auth_for, parse_public_routes(config_.public_routes)); } tl::expected AuthManager::authenticate(const std::string & client_id, @@ -402,9 +405,21 @@ 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(); + // A record outlives its own expiry by one access-token lifetime. + // + // validate_token() rejects an access token whose refresh record is gone, so + // dropping the record the instant it expires cuts short every access token + // minted from it. The last one can be issued a second before the refresh + // token expires and is then promised a full token_expiry_seconds; sweeping + // on expires_at alone would refuse it about a minute later, with most of its + // life left. Keeping the record until nothing issued from it can still be + // valid costs one extra lifetime of memory per client and removes the whole + // race. + const int64_t grace = static_cast(config_.token_expiry_seconds); + size_t count = 0; for (auto it = refresh_tokens_.begin(); it != refresh_tokens_.end();) { - if (it->second.expires_at < now_ts) { + if (it->second.expires_at + grace < now_ts) { it = refresh_tokens_.erase(it); ++count; } else { diff --git a/src/ros2_medkit_gateway/src/core/auth/auth_requirement_policy.cpp b/src/ros2_medkit_gateway/src/core/auth/auth_requirement_policy.cpp index 16393f3b8..d48210071 100644 --- a/src/ros2_medkit_gateway/src/core/auth/auth_requirement_policy.cpp +++ b/src/ros2_medkit_gateway/src/core/auth/auth_requirement_policy.cpp @@ -15,7 +15,12 @@ #include "ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp" #include +#include +#include #include +#include +#include +#include namespace ros2_medkit_gateway { @@ -168,8 +173,101 @@ std::unique_ptr AuthRequirementPolicyFactory::create(con return std::make_unique(); } - // Use the require_auth_for setting from config - return create(config.require_auth_for); + return create(config.require_auth_for, parse_public_routes(config.public_routes)); +} + +std::vector parse_public_routes(const std::vector & entries) { + std::vector routes; + routes.reserve(entries.size()); + for (const auto & entry : entries) { + auto parsed = parse_public_route(entry); + if (parsed.has_value()) { + routes.push_back(*parsed); + } + } + return routes; +} + +std::unique_ptr +AuthRequirementPolicyFactory::create(AuthRequirement requirement, const std::vector & public_routes) { + auto policy = create(requirement); + if (public_routes.empty()) { + return policy; + } + return std::make_unique(std::move(policy), public_routes); +} + +tl::expected parse_public_route(const std::string & entry) { + auto is_space = [](unsigned char c) { + return std::isspace(c) != 0; + }; + auto begin = std::find_if_not(entry.begin(), entry.end(), is_space); + auto end = std::find_if_not(entry.rbegin(), entry.rend(), is_space).base(); + const std::string trimmed = (begin < end) ? std::string(begin, end) : std::string(); + + if (trimmed.empty()) { + return tl::unexpected("entry is empty"); + } + + const size_t space = trimmed.find(' '); + if (space == std::string::npos) { + return tl::unexpected("expected \"METHOD /path\", e.g. \"GET /api/v1/health\""); + } + + PublicRoute route; + route.method = trimmed.substr(0, space); + route.path = trimmed.substr(space + 1); + + // The path is compared against what cpp-httplib hands the middleware, which + // is a single token. A second space means two paths or a stray argument, and + // either way the entry does not describe one route. + if (route.path.find(' ') != std::string::npos) { + return tl::unexpected("path contains a space: \"" + route.path + "\""); + } + + std::transform(route.method.begin(), route.method.end(), route.method.begin(), [](unsigned char c) { + return static_cast(std::toupper(c)); + }); + + static const std::vector kMethods = {"GET", "HEAD", "POST", "PUT", "DELETE", "PATCH", "OPTIONS"}; + if (std::find(kMethods.begin(), kMethods.end(), route.method) == kMethods.end()) { + return tl::unexpected("unknown HTTP method \"" + route.method + "\""); + } + + if (route.path.empty() || route.path.front() != '/') { + return tl::unexpected("path must start with \"/\": \"" + route.path + "\""); + } + + // Wildcards are refused rather than ignored. Accepting the character and + // then matching it literally would read as "this opens the subtree" and + // silently open nothing; refusing says so while the operator is watching. + if (route.path.find('*') != std::string::npos) { + return tl::unexpected("wildcards are not supported; name each route exactly: \"" + route.path + "\""); + } + + return route; +} + +PublicRouteExemptionPolicy::PublicRouteExemptionPolicy(std::unique_ptr inner, + std::vector public_routes) + : inner_(std::move(inner)), public_routes_(std::move(public_routes)) { +} + +bool PublicRouteExemptionPolicy::requires_authentication(const std::string & method, const std::string & path) const { + for (const auto & route : public_routes_) { + if (route.method == method && route.path == path) { + return false; + } + } + return inner_->requires_authentication(method, path); +} + +std::string PublicRouteExemptionPolicy::description() const { + std::string desc = inner_->description() + "; public_routes:"; + for (const auto & route : public_routes_) { + desc += " " + route.method + " " + route.path; + } + return desc; } } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 601a2f07c..b194fe404 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -30,6 +30,7 @@ #include #include "ros2_medkit_gateway/core/aggregation/network_utils.hpp" +#include "ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp" #include "ros2_medkit_gateway/core/data/topic_data_provider.hpp" #include "ros2_medkit_gateway/core/discovery/refresh_debounce.hpp" #include "ros2_medkit_gateway/core/entity_validation.hpp" @@ -173,6 +174,7 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki declare_parameter("auth.require_auth_for", "write"); declare_parameter("auth.issuer", "ros2_medkit_gateway"); declare_parameter("auth.clients", std::vector{}); + declare_parameter("auth.public_routes", std::vector{}); // OpenAPI documentation endpoints declare_parameter("docs.enabled", true); @@ -495,10 +497,30 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki } } + // Routes the operator has taken outside authentication. Validated here + // rather than where the policy is built, because a typo must stop the + // gateway while somebody is watching: silently dropping the entry would + // leave a route protected that the operator believes is reachable, and + // silently widening it would be worse. + auto public_routes = get_parameter("auth.public_routes").as_string_array(); + for (const auto & entry : public_routes) { + auto parsed = parse_public_route(entry); + if (!parsed) { + throw std::invalid_argument("auth.public_routes entry \"" + entry + "\" is invalid: " + parsed.error()); + } + } + auth_builder.with_public_routes(public_routes); + auth_config_ = auth_builder.build(); RCLCPP_INFO(get_logger(), "Authentication enabled - algorithm: %s, require_auth_for: %s", algorithm_to_string(auth_config_.jwt_algorithm).c_str(), get_parameter("auth.require_auth_for").as_string().c_str()); + for (const auto & entry : public_routes) { + // One line per route, at WARN: every entry here is a hole somebody + // opened on purpose, and an operator reading the startup log should see + // the whole public surface without going to look for the config file. + RCLCPP_WARN(get_logger(), "auth.public_routes: %s is answered WITHOUT a credential", entry.c_str()); + } } catch (const std::exception & e) { // Fail closed: authentication was explicitly requested but could not be // built (e.g. empty jwt_secret). Refuse to start rather than silently 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 197368d01..fef043f13 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp @@ -56,14 +56,13 @@ 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. +/// Reachable two ways: under `require_auth_for: write`, where every GET is +/// open, and on a route an operator listed in `auth.public_routes`. In both an +/// anonymous caller reaches this handler, and the full body is more than the +/// probe asked for: the linking warnings name entities and ROS node FQNs, and +/// the entity cache reports how many apps, areas and components this gateway +/// sees. So an anonymous caller gets liveness and nothing else, flagged as cut +/// down, 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. @@ -94,7 +93,10 @@ http::Result HealthHandlers::get_health(const http::TypedRequest & // 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. + // The flag is what keeps the empty `warnings` below from reading as + // "nothing is wrong here" to a monitor that never presented a credential. if (is_anonymous(ctx_, req)) { + response.x_medkit_reduced = true; return response; } diff --git a/src/ros2_medkit_gateway/src/http/http_server.cpp b/src/ros2_medkit_gateway/src/http/http_server.cpp index 45ebff98b..2aaefe186 100644 --- a/src/ros2_medkit_gateway/src/http/http_server.cpp +++ b/src/ros2_medkit_gateway/src/http/http_server.cpp @@ -49,6 +49,20 @@ HttpServerManager::HttpServerManager(const TlsConfig & tls_config, std::size_t t " (key configured: " + (tls_config_.key_file.empty() ? "no" : "yes") + ")"); } + // The constructor above ignores what SSL_CTX_load_verify_locations returned, + // so `is_valid()` is true for a ca_file that exists but is unreadable or is + // not PEM. The gateway would then start, log "REQUIRED (mutual TLS)", and + // reject every client at the handshake with no trust store to check them + // against - an outage that reads as a client problem. Load it again and + // look at the answer this time; loading the same file twice is a no-op + // beyond re-adding the same certificates to the store. + if (!tls_config_.ca_file.empty() && + SSL_CTX_load_verify_locations(ssl_server_->ssl_context(), tls_config_.ca_file.c_str(), nullptr) != 1) { + throw std::runtime_error( + "Mutual TLS is configured but the client CA bundle could not be loaded: " + tls_config_.ca_file + + ". The file must be readable by the gateway and contain PEM certificates."); + } + // Configure additional TLS settings configure_tls(); apply_thread_pool(*ssl_server_); diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index 0c16bb0d8..c5d3806ec 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -328,8 +328,7 @@ void RESTServer::setup_pre_routing_handler() { // It is safe because a preflight discloses nothing about the system: the // response is the CORS policy for an origin the operator configured, with // no body, and the real request that follows is authenticated normally. - // Treat this as a third named exemption alongside GET /health and - // /auth/*, not as an oversight. + // Treat this as a named exemption alongside /auth/, not as an oversight. // A real preflight carries Access-Control-Request-Method; the browser // sends it to ask whether the method it is about to use is allowed. // Requiring it keeps the exemption to the case that genuinely cannot @@ -347,13 +346,39 @@ void RESTServer::setup_pre_routing_handler() { } } - // 3. Authentication, before the rate limiter. + // 3. Rate limiting is METERED here and ANSWERED after authentication. // - // This half of the order was wrong. The limiter also returns Handled, so an - // anonymous caller who exhausted the allowance used to get 429 from a - // protected route instead of 401 - an answer, and a small disclosure of - // limiter state, without any credential. + // Both halves matter and they pull in opposite directions. Metering after + // authentication means a request refused for a bad credential never spends + // any allowance, so an anonymous caller can hammer a protected route for + // free and pay only for the signature verification each attempt costs the + // gateway - which under RS256 is not cheap. Answering before + // authentication means an anonymous caller who exhausted the allowance + // gets 429 from a protected route instead of 401: an answer, and a small + // disclosure of limiter state, without any credential. + // + // So: consume the allowance for every request, and decide what to say + // about it once we know whether the caller had a credential. + bool rate_limited = false; + RateLimitResult rl_result; + if (rate_limiter_ && rate_limiter_->is_enabled() && req.method != "OPTIONS") { + rl_result = rate_limiter_->check(req.remote_addr, req.path); + RateLimiter::apply_headers(rl_result, res); + rate_limited = !rl_result.allowed; + } + + // 4. Authentication. if (auth_middleware_ && auth_middleware_->is_enabled()) { + // With the allowance gone, a request that presents no credential at all + // on a route that needs one is refused without verifying anything. This + // is the shape an anonymous flood takes, and it is the case where the + // verification is pure waste - the answer is 401 either way. + const bool needs_credential = auth_manager_ && auth_manager_->requires_authentication(req.method, req.path); + if (rate_limited && needs_credential && !req.has_header("Authorization")) { + res.status = 401; + return handled(req, res); + } + auto auth_request = AuthMiddleware::from_httplib_request(req); auto result = auth_middleware_->process(auth_request); @@ -363,14 +388,12 @@ void RESTServer::setup_pre_routing_handler() { } } - // 4. Rate limiting check. If rejected, return Handled (CORS headers already set) - if (rate_limiter_ && rate_limiter_->is_enabled() && req.method != "OPTIONS") { - auto rl_result = rate_limiter_->check(req.remote_addr, req.path); - RateLimiter::apply_headers(rl_result, res); - if (!rl_result.allowed) { - RateLimiter::apply_rejection(rl_result, res); - return handled(req, res); - } + // 5. Now the limiter may speak. The caller reached this line with a + // credential this gateway accepts, or on a route that needs none, so 429 + // tells them something they are entitled to know. + if (rate_limited) { + RateLimiter::apply_rejection(rl_result, res); + return handled(req, res); } 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 9d6ac1a89..4ffdb8721 100644 --- a/src/ros2_medkit_gateway/test/test_auth_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_auth_manager.cpp @@ -529,6 +529,62 @@ TEST(AuthManagerRequirementTest, RequireAuthForAll) { EXPECT_FALSE(manager.requires_authentication("POST", "/api/v1/auth/authorize")); } +// An access token keeps its promised lifetime after its refresh token expires. +// +// validate_token() refuses an access token whose refresh record is gone, which +// is what makes a revocation survive. The cost is that the sweep decides how +// long an access token really lives: sweeping on the refresh token's own +// expiry would cut short the last access token minted from it, which was +// promised a full token_expiry_seconds a moment earlier. This is the test that +// fails if the grace period is removed. +// @verifies REQ_INTEROP_086 +TEST(AuthManagerRequirementTest, AccessTokenOutlivesItsExpiredRefreshRecord) { + // The two expiries are equal, which is the tightest the builder allows + // (refresh must be >= access). That is also the worst case: refresh_access_token + // reuses the refresh token's jti rather than rotating it, so the access token + // it mints is promised token_expiry_seconds from NOW while the record still + // dies at its original expiry. Every refresh therefore produces an access + // token that outlives its own record. + // Three seconds, not one. Two constraints set these numbers. + // + // Expiries are whole seconds, so the sweep's comparison only moves at second + // boundaries: with a one-second expiry and a 1.3 s wait, `expires_at < now` + // is still false through integer truncation, and the test would pass with or + // without the grace period - measuring nothing. + // + // And the waits are wall-clock on a machine running the rest of the suite, so + // each one has to sit well clear of the boundary it is about rather than just + // past it. The record expires at t+3 and is swept after t+6; the checks are + // at t+4 and t+9, leaving 2 s and 3 s of slack for a late wake-up. + AuthConfig config = AuthConfigBuilder() + .with_enabled(true) + .with_jwt_secret("test_secret_key_min_32_chars_life_") + .with_token_expiry(3) + .with_refresh_token_expiry(3) + .with_require_auth_for(AuthRequirement::ALL) + .build(); + config.clients.push_back({"c", "s", UserRole::ADMIN, true}); + + AuthManager manager(config); + ASSERT_TRUE(manager.authenticate("c", "s").has_value()); + ASSERT_EQ(manager.refresh_token_count(), 1u); + + // t+4s: past the record's own expiry (t+3), inside the one access-token + // lifetime it is held for (to t+6). Sweeping here is what cut an access token + // short. + std::this_thread::sleep_for(std::chrono::milliseconds(4000)); + manager.cleanup_expired_tokens(); + EXPECT_EQ(manager.refresh_token_count(), 1u) + << "the record was dropped at its own expiry, so any access token minted " + "from it in its last moments is refused with most of its life left"; + + // t+9s: past the grace too. It does not live forever, or a revocation would + // be honoured out of a map that only ever grows. + std::this_thread::sleep_for(std::chrono::milliseconds(5000)); + manager.cleanup_expired_tokens(); + EXPECT_EQ(manager.refresh_token_count(), 0u) << "the record outlived even its grace period"; +} + // Test none auth requirement mode TEST(AuthManagerRequirementTest, RequireAuthForNone) { AuthConfig config = AuthConfigBuilder() @@ -715,8 +771,11 @@ TEST(AuthManagerTokenLifetimeTest, RepeatedLoginsDoNotGrowTheStoreWithoutBound) } 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)); + // Past the refresh expiry AND the access-token lifetime a record is held for + // beyond it, the next authorisation must clear them out. Records expire at + // t+1 and are swept after t+2, so this waits to t+4: far enough clear of the + // boundary that a late wake-up on a loaded machine cannot land short of it. + std::this_thread::sleep_for(std::chrono::milliseconds(4000)); ASSERT_TRUE(manager.authenticate("svc", "svc_secret").has_value()); EXPECT_EQ(manager.refresh_token_count(), 1U) @@ -1257,33 +1316,107 @@ 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. +// Health is NOT special to the ALL policy. It is closed like everything else +// until an operator names it in auth.public_routes, and this is the test that +// fails if somebody hardcodes the exemption back in. // @verifies REQ_INTEROP_086 -TEST_F(AuthRequirementPolicyTest, AllAuthPolicyExemptsOnlyGetHealth) { +TEST_F(AuthRequirementPolicyTest, AllAuthPolicyDoesNotExemptHealth) { 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("GET", "/api/v1/health")); EXPECT_TRUE(policy.requires_authentication("HEAD", "/api/v1/health")); +} + +// An entry of auth.public_routes opens the route it names and nothing beside +// it. Widening the comparison to a prefix, or dropping the method, is the +// natural next edit and would open a hole, so the boundary is pinned here. +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, PublicRouteExemptionOpensOnlyWhatItNames) { + auto policy = AuthRequirementPolicyFactory::create(AuthRequirement::ALL, {{"GET", "/api/v1/health"}}); + + // The route the operator named. + EXPECT_FALSE(policy->requires_authentication("GET", "/api/v1/health")); + + // Only GET. A write to the health path is not a liveness probe, and + // cpp-httplib dispatches HEAD into the GET handler table, so dropping the + // method check would hand the status document to an anonymous HEAD. + 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")); + 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")); + + // And the rest of the surface is untouched by the entry. + EXPECT_TRUE(policy->requires_authentication("GET", "/api/v1/areas")); + EXPECT_TRUE(policy->requires_authentication("GET", "/api/v1/")); +} + +// The layer only ever removes a requirement. Wrapping must not make a gateway +// stricter than the policy underneath, or an operator who adds a probe route +// would silently close the reads that `write` leaves open. +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, PublicRouteExemptionNeverAddsARequirement) { + auto policy = AuthRequirementPolicyFactory::create(AuthRequirement::WRITE, {{"POST", "/api/v1/health"}}); + + EXPECT_FALSE(policy->requires_authentication("GET", "/api/v1/areas")); + EXPECT_FALSE(policy->requires_authentication("POST", "/api/v1/health")); + EXPECT_TRUE(policy->requires_authentication("POST", "/api/v1/areas")); +} + +// An empty list must leave the policy exactly as it was, or "closed by +// default" would depend on the wrapper behaving itself. +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, EmptyPublicRoutesChangesNothing) { + auto policy = AuthRequirementPolicyFactory::create(AuthRequirement::ALL, {}); + + EXPECT_TRUE(policy->requires_authentication("GET", "/api/v1/health")); + EXPECT_TRUE(policy->requires_authentication("GET", "/api/v1/areas")); + EXPECT_FALSE(policy->requires_authentication("POST", "/api/v1/auth/authorize")); +} + +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, PublicRouteEntryParsing) { + auto ok = parse_public_route("GET /api/v1/health"); + ASSERT_TRUE(ok.has_value()); + EXPECT_EQ(ok->method, "GET"); + EXPECT_EQ(ok->path, "/api/v1/health"); + + // Case and surrounding whitespace are the operator's typing, not a decision. + auto lower = parse_public_route(" get /api/v1/health "); + ASSERT_TRUE(lower.has_value()); + EXPECT_EQ(lower->method, "GET"); + EXPECT_EQ(lower->path, "/api/v1/health"); + + // Everything below must be refused rather than half-understood. A wildcard + // accepted and then matched literally would read as "this opens the subtree" + // and open nothing, which is the worst of both. + EXPECT_FALSE(parse_public_route("/api/v1/health").has_value()); + EXPECT_FALSE(parse_public_route("GET").has_value()); + EXPECT_FALSE(parse_public_route("").has_value()); + EXPECT_FALSE(parse_public_route(" ").has_value()); + EXPECT_FALSE(parse_public_route("FETCH /api/v1/health").has_value()); + EXPECT_FALSE(parse_public_route("GET api/v1/health").has_value()); + EXPECT_FALSE(parse_public_route("GET /api/v1/*").has_value()); + EXPECT_FALSE(parse_public_route("GET /api/v1/health extra").has_value()); +} + +// A malformed entry must not quietly open something. Dropping it keeps the +// route protected; GatewayNode refuses to start so the typo is not silent. +// @verifies REQ_INTEROP_086 +TEST_F(AuthRequirementPolicyTest, MalformedPublicRoutesAreDropped) { + auto routes = parse_public_routes({"GET /api/v1/health", "nonsense", "GET /api/v1/*"}); + + ASSERT_EQ(routes.size(), 1u); + EXPECT_EQ(routes[0].path, "/api/v1/health"); } // @verifies REQ_INTEROP_086 diff --git a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py index 22b9cb57c..33d9248b1 100644 --- a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py +++ b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/gateway_test_case.py @@ -93,7 +93,15 @@ def setUpClass(cls): @classmethod def _wait_for_gateway_health(cls): - """Poll GET /health until the gateway responds with 200. + """Poll GET /health until the gateway answers at all. + + A refusal counts as up. What this waits for is a process that is + listening and speaking HTTP, and a 401 proves both: the request was + received, routed and decided on. Requiring 200 would instead make this + wait for a specific auth configuration, and under the shipped defaults + - ``require_auth_for: all`` with an empty ``auth.public_routes`` - + that 200 never arrives, so every test class using this helper would + time out against a perfectly healthy gateway. Uses ``time.monotonic()`` for a reliable, monotonic clock. @@ -108,7 +116,7 @@ def _wait_for_gateway_health(cls): while time.monotonic() < deadline: try: response = requests.get(f'{cls.BASE_URL}/health', timeout=2) - if response.status_code == 200: + if response.status_code in (200, 401, 403): return except requests.exceptions.RequestException: pass 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 ccb13d5ba..dc2f7f48a 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 @@ -53,6 +53,8 @@ CORS_BASE_URL = f'http://127.0.0.1:{CORS_PORT}{API_BASE_PATH}' RL_PORT = get_test_port(2) RL_BASE_URL = f'http://127.0.0.1:{RL_PORT}{API_BASE_PATH}' +PUBLIC_PORT = get_test_port(3) +PUBLIC_BASE_URL = f'http://127.0.0.1:{PUBLIC_PORT}{API_BASE_PATH}' ALLOWED_ORIGIN = 'https://ui.example' # At least 32 characters, or the gateway refuses to start under HS256. @@ -120,28 +122,45 @@ def generate_test_description(): }, ) + # The opt-in half. `auth.public_routes` is empty on every gateway above, so + # without this one nothing here would exercise the knob an operator uses to + # take a route outside authentication, and "empty by default" would be + # indistinguishable from "the setting does nothing". + public_route_gateway = create_gateway_node( + port=PUBLIC_PORT, + name='gateway_with_public_route', + extra_params={ + 'server.host': '127.0.0.1', + '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'], + 'auth.public_routes': ['GET /api/v1/health'], + }, + ) + return launch.LaunchDescription([ gateway_node, cors_gateway, rl_gateway, + public_route_gateway, launch_testing.actions.ReadyToTest(), ]), {'gateway_node': gateway_node, 'cors_gateway': cors_gateway, - 'rl_gateway': rl_gateway} + 'rl_gateway': rl_gateway, 'public_route_gateway': public_route_gateway} 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/* alone, and only because authentication cannot bootstrap through a + door that already demands the credential it exists to hand out. - /auth/* - token issuance. Authentication cannot bootstrap through a door - that already demands the credential it exists to hand out. + Health is NOT here. `auth.public_routes` is empty as shipped, so a probe + that wants an uncredentialed answer is a decision an operator makes and + writes down; TestConfiguredPublicRoute below covers that path. """ - if method == 'GET' and path == f'{API_BASE_PATH}/health': - return True + del method # the one exemption is path-shaped: every method under /auth/ return path.startswith(f'{API_BASE_PATH}/auth/') @@ -262,117 +281,162 @@ def test_04_reads_are_refused_not_just_writes(self): 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. + def test_05_health_refuses_like_everything_else(self): + """Health is not special. It is closed until somebody opens it. - 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. + The route a hardening change is most tempted to leave open, pinned so + the temptation shows up as a red test. `auth.public_routes` is the way + to open it, and TestConfiguredPublicRoute holds that end. """ resp = requests.get(f'{CLOSED_BASE_URL}/health', timeout=15) - self.assertEqual(resp.status_code, 200, 'health must stay probe-able') - - 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() - # 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"]}' + self.assertIn( + resp.status_code, (401, 403), + f'GET /health answered {resp.status_code} with no credential and an ' + 'empty auth.public_routes' ) - def test_06b_the_full_health_document_names_entities(self): - """The reason test_06 matters, pinned so it cannot be argued away. + def test_06_the_full_health_document_names_entities(self): + """Why the anonymous body has to be cut down when a route is opened. 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. + true, the narrowing in TestConfiguredPublicRoute 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.assertNotIn( + 'x-medkit-reduced', body, + 'an authenticated caller was served the cut-down 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.""" 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. +class TestConfiguredPublicRoute(GatewayTestCase): + """`auth.public_routes` opens exactly what it names, and nothing near it. + + The gateway under this class runs `require_auth_for: all` with one entry, + `GET /api/v1/health`. Everything here is about the edge of that entry: a + setting that opened the route it names AND its neighbours would pass a test + that only checked the route it names. + """ + + BASE_URL = PUBLIC_BASE_URL + + @classmethod + def setUpClass(cls): + super().setUpClass() + token = requests.post( + f'{PUBLIC_BASE_URL}/auth/authorize', + json={ + 'grant_type': 'client_credentials', + 'client_id': CLIENT_ID, + 'client_secret': CLIENT_SECRET, + }, + timeout=15, + ).json()['access_token'] + cls.auth = {'Authorization': f'Bearer {token}'} + + def test_01_the_named_route_answers_without_a_credential(self): + """The knob does something. Without this the rest proves only refusal.""" + resp = requests.get(f'{PUBLIC_BASE_URL}/health', timeout=15) + self.assertEqual( + resp.status_code, 200, + 'auth.public_routes named GET /api/v1/health and it still refused' + ) + + def test_02_the_route_next_door_is_untouched(self): + """An entry opens one route, not the surface around it. + + The failure this catches is a prefix or wildcard match creeping into + the comparison: `/health` opening `/healthz`, or worse, one entry + opening every GET. """ - # 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) + for path in ('/', '/areas', '/components', '/apps', '/version-info'): + with self.subTest(path=path): + resp = requests.get(f'{PUBLIC_BASE_URL}{path}', timeout=15) + self.assertIn( + resp.status_code, (401, 403), + f'{path} answered {resp.status_code} on a gateway whose only ' + 'public route is GET /api/v1/health' + ) + + def test_03_the_method_is_part_of_the_entry(self): + """An entry names a method, and the method is part of the match. + + cpp-httplib dispatches HEAD into the GET handler table, so a comparison + that dropped the method would hand the status document to an anonymous + HEAD. The write methods have no handler here and answer the same either + way, so HEAD is the one that can show the difference. + """ + head = requests.head(f'{PUBLIC_BASE_URL}/health', timeout=15) self.assertIn( head.status_code, (401, 403), - f'HEAD /health answered {head.status_code} to an anonymous caller' + f'HEAD /health answered {head.status_code} for an entry that named GET' ) - 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' - ) + def test_04_the_anonymous_body_is_liveness_and_says_so(self): + """Opening the route must not publish the entity inventory. + + An allowlist, not a denylist: listing the fields known to leak today + would pass the day a new section is added, and a probe needs no more + than "am I alive". + """ + body = requests.get(f'{PUBLIC_BASE_URL}/health', timeout=15).json() + self.assertEqual( + set(body), + {'status', 'timestamp', 'warnings', 'warning_schema_version', + 'x-medkit-reduced'}, + 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. + self.assertEqual(body['warnings'], []) + # And the empty array must not read as "nothing is wrong". A monitor + # that cannot tell withheld from clean would clear a real warning. + self.assertIs( + body['x-medkit-reduced'], True, + 'the cut-down body did not say it was cut down, so an empty ' + 'warnings array reads as a clean bill of health' + ) + + def test_05_a_credential_still_gets_the_whole_document(self): + """Opening a route for probes must not cost the operator surface.""" + body = requests.get( + f'{PUBLIC_BASE_URL}/health', headers=self.auth, timeout=15 + ).json() + self.assertIn('discovery', body) + self.assertNotIn('x-medkit-reduced', body) + + def test_06_a_forged_credential_is_an_anonymous_caller(self): + """A token this gateway never issued must not unlock the full body.""" + body = requests.get( + f'{PUBLIC_BASE_URL}/health', + headers={'Authorization': 'Bearer not.a.real.token'}, + timeout=15, + ).json() + self.assertIs(body.get('x-medkit-reduced'), True, body) + self.assertNotIn('discovery', body) class TestNothingAnswersBeforeAuth(GatewayTestCase): """What the CORS preflight may and may not do without a credential. - Preflight is answered anonymously on purpose, and it is the third named - exemption after GET /health and /auth/*. A browser never puts Authorization - on a preflight - asking permission before sending the real request is the - whole point of the mechanism - so demanding one would not harden anything, - it would make browser clients impossible. An earlier version of this branch - did exactly that, and the control below is what caught it. + Preflight is answered anonymously on purpose, and it is the second named + exemption after /auth/*. A browser never puts Authorization on a preflight + - asking permission before sending the real request is the whole point of + the mechanism - so demanding one would not harden anything, it would make + browser clients impossible. The control below is what pins that. What must hold instead: the preflight discloses only CORS policy, and the REAL request that follows is still refused without a credential. 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 index 5f3c32a75..e0c97a191 100644 --- 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 @@ -158,15 +158,24 @@ def test_02_the_shipped_file_covers_reads_not_just_writes(self): 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. + def test_03_health_refuses_in_the_shipped_file_too(self): + """The shipped file opens nothing, health included. - 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. + `auth.public_routes` is absent from the shipped config, so the file + that goes out in the package and the image leaves no route reachable + without a credential. An operator who wants a probe route adds the + entry themselves - that path is covered in test_closed_by_default. + + Pinned here separately from the sweep above because health is the route + a hardening change is most tempted to leave open, and a shipped file + that quietly did so would still pass every other test in this class. """ resp = requests.get(f'{BASE_URL}/health', timeout=15) - self.assertEqual(resp.status_code, 200) + self.assertIn( + resp.status_code, (401, 403), + f'the shipped config answered GET /health with {resp.status_code} ' + 'to a caller holding no credential' + ) def test_04_a_configured_client_still_works(self): """The mirror: a gateway that refused everyone would pass the rest.""" @@ -183,6 +192,17 @@ def test_05_the_shipped_file_is_the_one_under_test(self): text = handle.read() self.assertIn('require_auth_for: "all"', text) self.assertIn('enabled: true', text) + # And no route is opened by the file itself. An uncommented entry here + # would be a public route shipped to every deployment, which is exactly + # what this branch exists to stop. The commented example does not count. + live_public_routes = [ + line for line in text.splitlines() + if line.strip().startswith('public_routes:') + ] + self.assertEqual( + live_public_routes, [], + f'the shipped config declares public routes: {live_public_routes}' + ) @launch_testing.post_shutdown_test() From 975193c59c496819be549322f070cd49a1208b26 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 8 Sep 2026 19:47:36 +0200 Subject: [PATCH 7/7] ci: publish a pinnable image tag from main, not just :latest Every merge to main rebuilds :latest, so a user who pulls it has no way back to the behaviour they had yesterday. Nothing else is published from this workflow - the semver tags come from the release workflow, which needs a version tag to fire - so between releases there is no immutable reference at all. sha-, in the same spelling the release workflow uses, so one registry does not carry two formats of the same idea. --- .github/workflows/docker-publish.yml | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 4a5b82ac2..e33db5ec0 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -34,13 +34,33 @@ jobs: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} + - name: Short commit sha + id: shortsha + run: echo "value=${GITHUB_SHA::7}" >> "$GITHUB_OUTPUT" + - name: Build and push Docker image uses: docker/build-push-action@v6 with: context: . push: true build-args: ROS_DISTRO=${{ matrix.ros_distro }} - tags: ${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }}:latest + # Two tags, and the second one is the point. `:latest` moves + # on every merge, so a user who pulls it has no way back to + # the behaviour they had yesterday - and the defaults this + # image ships are exactly the kind of thing that changes + # under them. `sha-` gives them something immutable to + # pin. The release workflow publishes the semver tags; this + # one runs on every push to main, where there is no version + # number to use. + # + # Short form, to match the sha- tags the release workflow + # already publishes (docker/metadata-action's + # `type=sha,format=short`). Two spellings of the same idea in + # one registry is how a user ends up unable to find the image + # they pinned. + tags: | + ${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }}:latest + ${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }}:sha-${{ steps.shortsha.outputs.value }} cache-from: type=gha,scope=${{ matrix.ros_distro }} # mode=min, not max: max exports every intermediate build # stage, which for three distros held ~3.1 GB of the repo's