diff --git a/.github/workflows/docker-publish-multiarch.yml b/.github/workflows/docker-publish-multiarch.yml index 9e24c3bcc..be2922052 100644 --- a/.github/workflows/docker-publish-multiarch.yml +++ b/.github/workflows/docker-publish-multiarch.yml @@ -166,6 +166,13 @@ jobs: # `latest` belongs to docker-publish.yml, which moves it on # every push to main; without this the action's default # `latest=auto` would have a release take that tag over. + # + # The split of the rest: this workflow owns the semver tags + # and sha-, all of them naming the multi-arch manifest + # list. docker-publish.yml owns `latest` and main-, + # both naming its linux/amd64 image. No tag name is written + # by both, so a release tag cut on a commit that is also on + # main cannot leave one name pointing at two artefacts. flavor: latest=false tags: | # version tag (0.7.0 -> 0.7.0) diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index a0ec8b1a7..c74b243dd 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -94,10 +94,40 @@ jobs: python3 src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py \ "$RUNNER_TEMP/plugin-from-image.so" --expect read-only - # :latest is created from the inspected digest, so the tag resolves - # to the artifact the step above read, attestation manifest and all. + # What the image serves before it is named. The entrypoint's auth + # rule is shell and no colcon test reaches it; both halves of it + # have been wrong in a way only a running container showed. The + # digest checked here is the one the tag step below points at. + - name: Check the image's auth posture + run: | + scripts/smoke_image_auth_posture.sh \ + ${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }}@${{ steps.build.outputs.digest }} + + # Both tags are created from the inspected digest, so either one + # resolves to the artifact the step above read, attestation + # manifest and all. One invocation, so the two tags cannot come to + # name different manifests. + # + # :latest moves on every merge, which leaves a user who pulls it 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. main- is the immutable reference to pin. The + # multi-arch workflow publishes the semver tags and fires only on a + # release tag; this one runs on every push to main, where there is + # no version number to use. + # + # The `main-` prefix is what keeps the two workflows apart. This + # image is linux/amd64 only, while the multi-arch workflow pushes a + # two-architecture manifest list under sha-. A release tag is + # normally cut on a commit that is also on main, so a shared tag + # name would be written by both and resolve to whichever job + # finished last. The prefix names the branch this image came from, + # which is also what a reader of the tag wants to know. - name: Tag the inspected digest run: | + set -euo pipefail + image=${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }} docker buildx imagetools create \ - -t ${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }}:latest \ - ${{ env.REGISTRY }}/${{ github.repository_owner }}/ros2_medkit-${{ matrix.ros_distro }}@${{ steps.build.outputs.digest }} + -t "$image:latest" \ + -t "$image:main-${GITHUB_SHA::7}" \ + "$image@${{ steps.build.outputs.digest }}" diff --git a/Dockerfile b/Dockerfile index ff13846bc..79db7788b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -164,8 +164,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ # Copy built workspace from builder (builder uses /root/ws, runtime uses /home/medkit/ws) COPY --from=builder /root/ws/install/ ${COLCON_WS}/install/ -# Default config - can be overridden via volume mount -COPY docker/gateway_docker_params.yaml /etc/ros2_medkit/params.yaml +# Default config - the same file the package ships, so the image and a source +# install have one posture rather than two that drift. Override via volume +# mount, or point the container at config/gateway_params.secure.yaml inside the +# image for the closed profile. +COPY src/ros2_medkit_gateway/config/gateway_params.yaml /etc/ros2_medkit/params.yaml # When running via ros2 run (as this container does), plugin .so paths must be # configured explicitly via plugins..path parameters in the params file. @@ -188,4 +191,14 @@ USER medkit EXPOSE 8080 ENTRYPOINT ["/entrypoint.sh"] -CMD ["--ros-args", "--params-file", "/etc/ros2_medkit/params.yaml"] +# The two values a container needs that a host install does not: bind every +# interface, because the port is published rather than reached over loopback, +# and refresh faster, because a container's graph turns over as sibling +# containers come and go. Passed after the params file so they win over it. +# +# CORS names no origin here. A published image allowing development origins is +# a setting nobody chose; a deployment that runs the web UI next to the gateway +# sets cors.allowed_origins to its own origin. +CMD ["--ros-args", "--params-file", "/etc/ros2_medkit/params.yaml", \ + "-p", "server.host:=0.0.0.0", \ + "-p", "refresh_interval_ms:=2000"] diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index ef82bb433..07507118e 100644 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -24,6 +24,47 @@ 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}" +# Closing the image is opt-in, through the environment. +# +# The packaged params file is config/gateway_params.yaml, the same one a source +# install gets, and it leaves authentication off. So `docker run ` is the +# gateway a reader of the quickstart expects, and the web UI - which sends no +# Authorization header - talks to it. +# +# Setting MEDKIT_JWT_SECRET is the statement "this container runs closed". It +# turns authentication on, sets require_auth_for to "all" and hands the gateway +# that secret, whatever any params file says: these are passed after the file +# and a later -p wins. That is the point. Reading the file and deferring to it +# let a file carrying a secret but auth.enabled false run open with the variable +# set, which is the one outcome this must not produce. MEDKIT_CLIENTS carries +# the client credentials, without which nothing can obtain a token. +# +# MEDKIT_AUTH_DISABLED=1 forces authentication off and wins over both. + +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 [ -n "${MEDKIT_JWT_SECRET:-}" ]; then + AUTH_ARGS+=(-p auth.enabled:=true) + AUTH_ARGS+=(-p auth.require_auth_for:=all) + AUTH_ARGS+=(-p "auth.jwt_secret:=${MEDKIT_JWT_SECRET}") + if [ -n "${MEDKIT_CLIENTS:-}" ]; then + AUTH_ARGS+=(-p "auth.clients:=[${MEDKIT_CLIENTS}]") + else + echo "ros2_medkit: MEDKIT_JWT_SECRET is set but MEDKIT_CLIENTS is not, so no" >&2 + echo " client can obtain a token. Pass" >&2 + echo " MEDKIT_CLIENTS=::admin as well." >&2 + fi + echo "ros2_medkit: MEDKIT_JWT_SECRET is set - authentication is ON and every" >&2 + echo " route needs a credential." >&2 +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 reads these variables and applies the same rule. +export MEDKIT_JWT_SECRET MEDKIT_CLIENTS MEDKIT_AUTH_DISABLED + # 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 +72,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 deleted file mode 100644 index 1d4049a57..000000000 --- a/docker/gateway_docker_params.yaml +++ /dev/null @@ -1,19 +0,0 @@ -# Default gateway configuration for Docker deployment. -# Override by mounting your own file at /etc/ros2_medkit/params.yaml -# or passing --ros-args --params-file to the container. -ros2_medkit_gateway: - ros__parameters: - server: - host: "0.0.0.0" - port: 8080 - 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 - # it the browser gets "Failed to fetch". These are the default web UI - # origins; a wildcard is deliberately NOT used - with auth disabled and - # write methods enabled it would let any site drive cross-origin writes. - # Add your own UI origin(s) here, and enable JWT auth for production. - cors: - allowed_origins: - - "http://localhost:3000" - - "http://localhost:5173" diff --git a/docs/config/server.rst b/docs/config/server.rst index 0171b268a..54e883531 100644 --- a/docs/config/server.rst +++ b/docs/config/server.rst @@ -129,7 +129,7 @@ TLS/HTTPS Configuration * - ``server.tls.ca_file`` - string - ``""`` - - Path to CA certificate file (reserved for mutual TLS). + - CA that signs CLIENT certificates. Setting it turns on mutual TLS and makes a client certificate **required**: a caller that presents none is rejected during the handshake, before any request is read, so every bearer-token client without a certificate goes off the air. Leave empty for ordinary server-only TLS. * - ``server.tls.min_version`` - string - ``"1.2"`` @@ -844,8 +844,10 @@ See :doc:`/api/rest` for rate limiting response headers and 429 behavior. Authentication -------------- -JWT-based authentication with Role-Based Access Control (RBAC). Disabled by -default for local development. +JWT-based authentication with Role-Based Access Control (RBAC). Off in +``config/gateway_params.yaml`` and on in ``config/gateway_params.secure.yaml``, +which also sets ``require_auth_for`` to ``all``. With authentication on and no +``jwt_secret`` the gateway refuses to start rather than serve half-protected. .. list-table:: :header-rows: 1 @@ -891,6 +893,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 +928,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 secure profile's setting - 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/authentication.rst b/docs/tutorials/authentication.rst index 301ab65bd..8a3e055ef 100644 --- a/docs/tutorials/authentication.rst +++ b/docs/tutorials/authentication.rst @@ -11,8 +11,22 @@ Role-Based Access Control (RBAC) in ros2_medkit_gateway. Overview -------- -By default, the gateway runs without authentication for easy development. -For production deployments, you should enable authentication to: +By default, the gateway runs without authentication for easy development: +``config/gateway_params.yaml`` leaves ``auth.enabled`` false and +``require_auth_for`` at ``"write"``. ``config/gateway_params.secure.yaml`` is +the profile that turns it on, together with TLS and ``require_auth_for`` +``"all"``: + +.. code-block:: bash + + ros2 launch ros2_medkit_gateway gateway.launch.py \ + config_file:=$(ros2 pkg prefix --share ros2_medkit_gateway)/config/gateway_params.secure.yaml \ + jwt_secret:= \ + auth_clients:=::admin \ + cert_file:= key_file:= + +Turn authentication on for any deployment reachable beyond the machine it runs +on, to: - Control who can access the API - Limit write operations to authorized users @@ -205,6 +219,22 @@ Response: -H "Content-Type: application/json" \ -d '{"token": "dGhpcyBpcyBhIHJlZnJlc2g..."}' +A Gateway Restart Invalidates Every Token +----------------------------------------- + +Refresh records live in memory. An access token names the refresh record it was +issued alongside, and a token whose record the gateway no longer knows is +refused - so after a restart every access token and every refresh token issued +before it stops verifying, and clients re-authenticate with their client id and +secret. + +This is deliberate. The alternative - treating an unknown record as "nothing to +check" - would make a token somebody explicitly revoked work again across a +restart, for as long as ``token_expiry_seconds`` allows. A client that holds a +long-lived credential and exchanges it for tokens handles the restart by +re-authenticating on a 401; one that caches an access token across a gateway +restart does not. + Production Recommendations -------------------------- diff --git a/docs/tutorials/docker.rst b/docs/tutorials/docker.rst index 84c042e67..01eb75a5f 100644 --- a/docs/tutorials/docker.rst +++ b/docs/tutorials/docker.rst @@ -36,6 +36,12 @@ Images are available for all supported ROS 2 distributions: * - Lyrical - ``ghcr.io/selfpatch/ros2_medkit-lyrical:latest`` +Every push to ``main`` moves ``:latest`` and also publishes +``:main-`` - the same image under the short commit hash it was built +from, which is what to pin when ``:latest`` moving underneath a deployment is +not acceptable. Release tags carry the semver tags and ``:sha-``, which +name a multi-architecture manifest list rather than this amd64-only image. + Each image includes the gateway and all open-core packages: - ``ros2_medkit_gateway`` - HTTP REST server @@ -64,13 +70,38 @@ Test the gateway: curl http://localhost:8080/api/v1/version-info # {"items":[{"version":"","vendor_info":{"name":"ros2_medkit",...}}]} +The image carries ``config/gateway_params.yaml``, the same file a source +install gets, so it answers without a credential like a source install does. +Publish the port only where that is acceptable. + +Running the container closed +---------------------------- + +Set ``MEDKIT_JWT_SECRET`` and the container runs with authentication on, +``require_auth_for`` ``all``, and the secret you gave it. ``MEDKIT_CLIENTS`` +carries the credentials a client exchanges for a token: + +.. code-block:: bash + + docker run -p 8080:8080 \ + -e MEDKIT_JWT_SECRET="$(head -c 32 /dev/urandom | base64)" \ + -e MEDKIT_CLIENTS="medkit:$(head -c 24 /dev/urandom | base64):admin" \ + ghcr.io/selfpatch/ros2_medkit-jazzy:latest + +The precedence is one rule: ``MEDKIT_AUTH_DISABLED=1`` wins over everything and +forces authentication off; otherwise a set ``MEDKIT_JWT_SECRET`` closes the +container whatever any params file says, because those parameters are passed +after the file; otherwise the file decides. The image also carries +``config/gateway_params.secure.yaml`` - TLS, rate limiting and the rest - which +you can point ``--params-file`` at once the container has a certificate. + Custom Configuration -------------------- -The default configuration listens on ``0.0.0.0:8080``. CORS is enabled for the -default web UI origins (``http://localhost:3000`` and ``http://localhost:5173``) -so the web UI works out of the box; add your own UI origin(s) as needed (see -`CORS for Web UI`_ below). To use a custom configuration, mount a params file: +The container listens on ``0.0.0.0:8080`` and refreshes discovery every 2 s - +the two values the image passes on top of the packaged config. CORS is off, so +a browser UI on another origin needs its origin named (see `CORS for Web UI`_ +below). To use a custom configuration, mount a params file: .. code-block:: bash @@ -156,7 +187,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` + # exits non-zero on the refusal a closed container gives an + # uncredentialed probe, and reports 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 @@ -213,10 +252,12 @@ For containers to discover each other's ROS 2 nodes, use the same ``ROS_DOMAIN_I CORS for Web UI --------------- -The image enables CORS for the default web UI origins (``http://localhost:3000`` -and ``http://localhost:5173``). A wildcard is deliberately not used: with auth -disabled and write methods enabled it would let any site drive cross-origin -writes. Add your own UI origin(s): +The image names no CORS origin. A published image that allowed +``http://localhost:3000`` would be making a development machine's choice for +every deployment, so the origin a browser UI is served from is named by the +deployment that runs it. A wildcard is the wrong answer here: with auth off and +write methods enabled it would let any site drive cross-origin writes. Add your +own UI origin(s): .. code-block:: yaml @@ -230,17 +271,38 @@ 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``. A container left +at the image default answers it without a credential; one running closed +refuses it, so a probe that has to work in both cases reads the status code +rather than insisting 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 closed container has to answer a probe that 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/docs/tutorials/https.rst b/docs/tutorials/https.rst index 5604fbd8a..334809334 100644 --- a/docs/tutorials/https.rst +++ b/docs/tutorials/https.rst @@ -100,10 +100,72 @@ 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 **off** in the shipped ``gateway_params.yaml`` and **on** in +``gateway_params.secure.yaml``, which leaves ``cert_file`` and ``key_file`` for +the deployment to fill in. A gateway with TLS enabled and no certificate +refuses to start rather than fall back to plaintext, so turning it on means +supplying one: + +.. code-block:: bash + + # turn it on for one run, with a certificate + ros2 launch ros2_medkit_gateway gateway.launch.py tls_enabled:=true \ + cert_file:=/path/to/cert.pem key_file:=/path/to/key.pem + + # or run the secure profile, which has TLS on already + ros2 launch ros2_medkit_gateway gateway.launch.py \ + config_file:=$(ros2 pkg prefix --share ros2_medkit_gateway)/config/gateway_params.secure.yaml \ + cert_file:=/path/to/cert.pem key_file:=/path/to/key.pem + +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/scripts/smoke_image_auth_posture.sh b/scripts/smoke_image_auth_posture.sh new file mode 100755 index 000000000..6c7f41188 --- /dev/null +++ b/scripts/smoke_image_auth_posture.sh @@ -0,0 +1,165 @@ +#!/bin/bash +# 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 what an image actually serves, in the three configurations its +# documentation names. +# +# Usage: scripts/smoke_image_auth_posture.sh +# +# The entrypoint's auth rule lives in shell, so no colcon test reaches it, and +# both halves of it have been wrong in a way only a running container showed: +# once the variable was ignored entirely, once it turned authentication on and +# left require_auth_for at "write" so every read stayed open. +# +# 1. no environment -> anonymous GET /areas answers 200 +# 2. MEDKIT_JWT_SECRET set -> anonymous GET /areas answers 401 +# 3. a mounted params file with auth off, plus MEDKIT_JWT_SECRET +# -> anonymous GET /areas answers 401 +# +# Case 3 is the one that says the environment beats the file. Without it, an +# entrypoint that consulted the file and deferred to it would pass cases 1 +# and 2 while leaving a container its operator believes closed wide open. +# +# The gateway is reached at the container's bridge address, never at a +# published port on localhost: where the Docker daemon is not in this shell's +# network namespace, -p publishes somewhere this script cannot see and every +# request times out while the container is perfectly healthy. + +set -euo pipefail + +IMAGE="${1:?usage: $0 }" +SECRET="smoke_image_posture_secret_of_at_least_32_chars" +CLIENTS="smoke:smoke_client_secret:admin" +WORKDIR="$(mktemp -d)" + +# Fixed, not collected as the cases run: each case calls posture_of inside a +# command substitution to capture the status code, and a subshell cannot append +# to the parent's array - so a list built that way is empty when the trap fires +# and every container survives the run. +CONTAINERS=(medkit-smoke-open medkit-smoke-closed medkit-smoke-override) + +cleanup() { + for name in "${CONTAINERS[@]}"; do + docker rm -f "$name" >/dev/null 2>&1 || true + done + rm -rf "$WORKDIR" +} +trap cleanup EXIT + +# The status code an anonymous GET /areas gets from a container, once it is +# answering at all. Prints the code on stdout; anything else goes to stderr so +# the caller can capture the one value. +# +# Called as: posture_of -- +# The container args replace the image CMD, which is how a case points the +# gateway at its own params file. +# +# COPY_FILE, when set, is copied into the container at COPY_DEST before it +# starts. A bind mount would be simpler and is wrong here: the daemon resolves +# a -v source path on ITS filesystem, so where the daemon is not in this +# shell's namespace the container gets nothing and the gateway dies parsing an +# absent file. `docker cp` streams the bytes through the API instead, which +# works either way. +posture_of() { + local name=$1 + shift + local flags=() + while [ $# -gt 0 ] && [ "$1" != "--" ]; do + flags+=("$1") + shift + done + if [ $# -gt 0 ]; then + shift # drop the -- + fi + docker rm -f "$name" >/dev/null 2>&1 || true + docker create --name "$name" "${flags[@]}" "$IMAGE" "$@" >/dev/null + if [ -n "${COPY_FILE:-}" ]; then + docker cp "$COPY_FILE" "$name:${COPY_DEST:?COPY_DEST required with COPY_FILE}" + fi + docker start "$name" >/dev/null + + local ip + ip=$(docker inspect -f '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' "$name") + if [ -z "$ip" ]; then + echo "$name: container has no bridge address" >&2 + return 1 + fi + + # curl prints 000 itself when it cannot connect and exits non-zero; a + # fallback `|| echo 000` here appended a SECOND 000, and "000000" is not + # "000", so the wait loop finished on the first attempt with a value no + # comparison below could match. + local code="000" + for _ in $(seq 1 60); do + code=$(curl -s -o /dev/null -w '%{http_code}' "http://$ip:8080/api/v1/areas") || true + [ -n "$code" ] && [ "$code" != "000" ] && break + sleep 2 + done + if [ "$code" = "000" ]; then + echo "$name: never answered on http://$ip:8080" >&2 + docker logs "$name" >&2 2>&1 || true + return 1 + fi + echo "$code" +} + +expect() { + local label=$1 want=$2 got=$3 + if [ "$got" = "$want" ]; then + echo "ok $label: anonymous GET /areas -> $got" + else + echo "FAIL $label: anonymous GET /areas -> $got, expected $want" >&2 + return 1 + fi +} + +# A params file that turns authentication OFF and names a secret of its own, +# which is exactly the file an entrypoint must not defer to when the +# environment says closed. +cat > "$WORKDIR/auth-off.yaml" <<'YAML' +ros2_medkit_gateway: + ros__parameters: + auth: + enabled: false + require_auth_for: "write" + jwt_secret: "a_file_supplied_secret_of_at_least_32_characters" +YAML +# Readable and traversable by the image's non-root user. +chmod 755 "$WORKDIR" +chmod a+r "$WORKDIR/auth-off.yaml" + +failures=0 + +open_code=$(posture_of medkit-smoke-open --) +expect "default, no environment" 200 "$open_code" || failures=$((failures + 1)) + +closed_code=$(posture_of medkit-smoke-closed \ + -e MEDKIT_JWT_SECRET="$SECRET" -e MEDKIT_CLIENTS="$CLIENTS" --) +expect "MEDKIT_JWT_SECRET set" 401 "$closed_code" || failures=$((failures + 1)) + +# The container args replace the CMD, so server.host has to be repeated here: +# the packaged profile binds loopback and the image's own CMD is what opens it. +override_code=$(COPY_FILE="$WORKDIR/auth-off.yaml" COPY_DEST=/tmp/auth-off.yaml \ + posture_of medkit-smoke-override \ + -e MEDKIT_JWT_SECRET="$SECRET" -e MEDKIT_CLIENTS="$CLIENTS" \ + -- --ros-args --params-file /tmp/auth-off.yaml -p server.host:=0.0.0.0) +expect "environment over a params file with auth off" 401 "$override_code" \ + || failures=$((failures + 1)) + +if [ "$failures" -ne 0 ]; then + echo "$failures image posture case(s) failed" >&2 + exit 1 +fi +echo "image posture: all three cases as documented" diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index 5c0f55990..7d34b9a0b 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1507,10 +1507,10 @@ TLS (Transport Layer Security) enables encrypted HTTPS communication. TLS is **d | `server.tls.enabled` | bool | `false` | 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:** diff --git a/src/ros2_medkit_gateway/config/gateway_params.secure.yaml b/src/ros2_medkit_gateway/config/gateway_params.secure.yaml index f157ce2d9..cdbac3b06 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.secure.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.secure.yaml @@ -1,10 +1,22 @@ # ROS 2 Medkit Gateway - secure field profile # # Hardened parameter preset for on-prem / plant-network (appliance) -# deployments. It turns ON every control that the default development config -# leaves OFF: JWT auth, TLS, restricted CORS, and rate limiting. Use this file -# instead of gateway_params.yaml for any deployment reachable from an -# untrusted network. +# deployments. This is the closed profile: it turns ON every control that +# gateway_params.yaml leaves OFF - JWT auth with require_auth_for "all", TLS, +# rate limiting - and replaces the default's empty CORS origin list with an +# explicit one. Use this file instead of gateway_params.yaml for any +# deployment reachable from an untrusted network. +# +# gateway_params.yaml stays open so that an existing launch, an existing +# config and the web UI keep working unchanged. Closing a gateway is a +# deployment decision, and this file is how it is made: +# +# ros2 launch ros2_medkit_gateway gateway.launch.py \ +# config_file:=$(ros2 pkg prefix --share ros2_medkit_gateway)/config/gateway_params.secure.yaml \ +# jwt_secret:="$MEDKIT_JWT_SECRET" \ +# auth_clients:="operator:$OP_SECRET:operator" \ +# cert_file:=/etc/ros2_medkit/certs/server.pem \ +# key_file:=/etc/ros2_medkit/certs/server-key.pem # # ros2 run ros2_medkit_gateway gateway_node \ # --ros-args --params-file gateway_params.secure.yaml diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index d6a4dce4d..c3c70dcde 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -80,7 +80,10 @@ ros2_medkit_gateway: # TLS/HTTPS Configuration # Enables encrypted communication using OpenSSL tls: - # Enable/disable TLS (default: false for backward compatibility) + # Off in this profile. A gateway reachable from anything but loopback + # wants it on, and cert_file and key_file below then have to be filled + # in - the gateway refuses to start with TLS on and no certificate. + # config/gateway_params.secure.yaml is that profile. enabled: false # Path to PEM-encoded certificate file (required when TLS enabled) @@ -101,8 +104,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. # @@ -360,11 +366,24 @@ 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) + # Off in this profile, which is what every existing launch, config and + # the web UI expect. An unauthenticated SOVD gateway does expose the + # entity tree, the fault history and every operation the plugins + # register, so a deployment reachable by anything other than the machine + # it runs on should not use this file: + # config/gateway_params.secure.yaml turns this on together with TLS and + # require_auth_for "all". + # + # With this true and jwt_secret empty the gateway REFUSES TO START + # (auth_config.cpp: "JWT secret is required when authentication is + # enabled"), so turning it on here without supplying a secret does not + # produce a half-protected gateway. enabled: false - # JWT signing secret (required when enabled) + # JWT signing secret. Required whenever auth.enabled is true - the + # gateway will not start without it. 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: "" @@ -391,8 +410,41 @@ ros2_medkit_gateway: # - "write": Auth required for write operations (POST, PUT, DELETE) # - "all": Auth required for all operations # Default: "write" + # + # Note what "write" leaves open: every read, 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. + # "all" closes them, leaving only /auth/* public because authentication + # cannot bootstrap through a door that demands the credential it hands + # out. config/gateway_params.secure.yaml uses "all". require_auth_for: "write" + # 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/design/hardening.rst b/src/ros2_medkit_gateway/design/hardening.rst index 16311c0f3..168ed9f04 100644 --- a/src/ros2_medkit_gateway/design/hardening.rst +++ b/src/ros2_medkit_gateway/design/hardening.rst @@ -35,6 +35,35 @@ Control Default Secure profile ``locking`` on operations none lock required before mutation ================================ ============== =========================================== +Two things the secure profile brings with it are worth stating plainly. + +**The gateway refuses to start without a signing secret.** With +``auth.enabled`` true and ``auth.jwt_secret`` empty it exits with "JWT secret is +required when authentication is enabled" (and HS256 additionally requires at +least 32 characters). That is the intended failure. A gateway that will not boot +is a deployment problem someone fixes in a minute; a gateway that booted +half-protected is one nobody notices. + +**Under ``require_auth_for: "all"`` only ``/api/v1/auth/*`` is exempt, and +health is not.** Auth is exempt because authentication cannot bootstrap through +a door that already demands the credential it exists to hand out. Everything +else, ``GET /api/v1/health`` included, needs a credential, so a container +supervisor, load balancer or browser UI that probes health without one gets +401. A probe that accepts 200, 401 and 403 works against either profile and +leaves nothing open; a probe that cannot be changed gets the route named for +it:: + + auth: + public_routes: ["GET /api/v1/health"] + +``auth.public_routes`` is empty in both profiles, which is what makes the +exemption a deployment decision rather than a property of the artefact. The +match is on method and path exactly, so the entry above opens +``GET /api/v1/health`` and neither ``HEAD`` nor ``/api/v1/health/detail``. An +anonymous caller on a route opened that way gets liveness only - ``status``, +``timestamp`` and ``x-medkit-reduced: true`` - because the full body names +entities and ROS nodes. + Credential and certificate provisioning ---------------------------------------- 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..ef4d74f50 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 what a route needs. + /// 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_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/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..66e32b093 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,13 +71,17 @@ class NoAuthRequirementPolicy : public IAuthRequirementPolicy { /** * @brief Policy that always requires authentication * - * Except for public endpoints (auth endpoints, health check) + * 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 { (void)method; - // Auth endpoints are always public (to allow login) return path.find("/api/v1/auth/") != 0; } @@ -155,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. + * + * Both shipped profiles leave the list empty, which makes this a no-op: a + * route is open exactly where somebody said so and nowhere else. + */ +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 */ @@ -173,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/core/config.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/config.hpp index 7a2c3c33a..7288e7d79 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/config.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/config.hpp @@ -26,7 +26,8 @@ namespace ros2_medkit_gateway { * When enabled, the gateway will start an HTTPS server instead of HTTP. */ struct TlsConfig { - /// Whether TLS is enabled (default: false for backward compatibility) + /// Whether TLS is enabled. False matches config/gateway_params.yaml, the + /// default profile; config/gateway_params.secure.yaml sets it true. bool enabled{false}; /// Path to PEM-encoded certificate file 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/bringup.launch.py b/src/ros2_medkit_gateway/launch/bringup.launch.py index 4fb776651..8eb3ef4b8 100644 --- a/src/ros2_medkit_gateway/launch/bringup.launch.py +++ b/src/ros2_medkit_gateway/launch/bringup.launch.py @@ -47,11 +47,26 @@ def generate_launch_description(): 'config', 'bringup_params.yaml', ) + # The same file gateway.launch.py defaults to. Named here so the argument + # below always carries a real path: gateway.launch.py loads this value as a + # parameters entry, and an empty string is not a file. + gateway_default_config = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), + 'config', + 'gateway_params.yaml', + ) params_file = LaunchConfiguration('params_file') 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') + config_file = LaunchConfiguration('config_file') args = [ DeclareLaunchArgument( @@ -70,6 +85,43 @@ 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, along with config_file. Without them + # bringup can only run the profile gateway.launch.py defaults to and + # has nowhere to receive a certificate or a signing secret, so + # `ros2 launch ... bringup.launch.py config_file:= + # jwt_secret:=... cert_file:=...` is a complete command rather than a + # dead end. + DeclareLaunchArgument( + 'config_file', default_value=gateway_default_config, + description='Path to a gateway YAML config. Defaults to the package ' + 'config/gateway_params.yaml, the profile with auth and TLS ' + 'off. Point it at config/gateway_params.secure.yaml in the ' + 'same directory for the closed profile. An empty value is ' + 'not accepted: gateway.launch.py loads this path, so it has ' + 'to name a file.'), + DeclareLaunchArgument( + 'tls_enabled', default_value='', + description='Serve HTTPS. Empty leaves it to the config file, which has it ' + 'off in the default profile and on in the secure one. Needs ' + 'cert_file and key_file when on.'), + 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='', + description='Require a credential. Empty leaves it to the config file, ' + 'which has it off in the default profile and on in the secure ' + 'one.'), + 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 +142,11 @@ 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, + 'config_file': config_file}) 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 0da999c7f..6173cd286 100644 --- a/src/ros2_medkit_gateway/launch/gateway.launch.py +++ b/src/ros2_medkit_gateway/launch/gateway.launch.py @@ -21,12 +21,73 @@ from launch.actions import DeclareLaunchArgument, OpaqueFunction from launch.substitutions import LaunchConfiguration from launch_ros.actions import Node +import yaml # Default web UI origins enabled when the user does not override CORS, so the # bundled web UI works out of the box. A wildcard is deliberately not used. 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 config_says(config_file, *path, default=False): + """ + Return a boolean from the config file at ``path``, or ``default``. + + Which profile is in force is a property of the file, not of this launch + file: ``config/gateway_params.yaml`` leaves auth and TLS off and + ``config/gateway_params.secure.yaml`` turns both on, and either can be + named through ``config_file``. Assuming one of them here is how a warning + ends up firing on a launch that is about to work perfectly, or staying + silent on one that is about to be refused. + + Anything this cannot read - a missing file, invalid YAML, or a document + that is not a mapping of node names - reads as ``default`` and says so, + because "cannot tell" is what it is. This decides whether to PRINT a + warning; the gateway parses the same file a moment later and is the + authority on what it contains. + """ + try: + with open(config_file, encoding='utf-8') as handle: + document = yaml.safe_load(handle) or {} + except (OSError, yaml.YAMLError): + return default + if not isinstance(document, dict): + print(f'[gateway.launch.py] {config_file} is not a mapping of node names, so ' + f'the auth and TLS settings in it cannot be read here. The gateway still ' + f'reads the file itself; only the advice printed below is affected.') + return default + for node in document.values(): + section = node.get('ros__parameters') if isinstance(node, dict) else None + for key in path: + if not isinstance(section, dict): + section = None + break + section = section.get(key) + if isinstance(section, bool): + return section + return default + + def cors_override(cors_arg, config_file, default_config): """ Return the ``cors.allowed_origins`` entry for the final overrides, or {}. @@ -76,7 +137,10 @@ def generate_launch_description(): declare_override_config_arg = DeclareLaunchArgument( 'config_file', default_value=default_config, description='Path to YAML config file to override gateway parameters. Default config ' - 'is the ros2_medkit_gateway/config/gateway_params.yaml.') + 'is the ros2_medkit_gateway/config/gateway_params.yaml, which leaves auth ' + 'and TLS off. Point this at config/gateway_params.secure.yaml in the same ' + 'directory for the closed profile: auth on, require_auth_for "all", TLS on, ' + 'rate limiting on.') declare_host_arg = DeclareLaunchArgument( 'server_host', default_value='127.0.0.1', @@ -94,6 +158,54 @@ 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 whenever ' + 'auth is on - the default config leaves it off, the secure ' + 'profile turns it on, and the gateway refuses to start with auth ' + 'on and no secret. Pass one here, or point config_file at a file ' + 'that sets auth.jwt_secret and auth.clients.')) + + declare_auth_enabled_arg = DeclareLaunchArgument( + 'auth_enabled', default_value='', + description=( + 'Require a credential. Unset means the config_file decides: the ' + 'default config leaves auth off, config/gateway_params.secure.yaml ' + 'turns it on. With auth off the entity tree, the fault history ' + 'and every operation are 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_tls_enabled_arg = DeclareLaunchArgument( + 'tls_enabled', default_value='', + description=( + 'Serve HTTPS. Unset means the config_file decides: the default ' + 'config leaves TLS off, config/gateway_params.secure.yaml turns ' + 'it on. With TLS on, cert_file and key_file are required - the ' + 'gateway refuses to start without a certificate rather than fall ' + 'back to plaintext.')) + + 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, @@ -120,6 +232,91 @@ def _launch_setup(context, *_args, **_kwargs): param_overrides.update(cors_override( LaunchConfiguration('cors_allowed_origins').perform(context), LaunchConfiguration('config_file').perform(context), default_config)) + + # Precedence: an explicit launch argument, 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() + if tls_arg: + tls_enabled = parse_security_flag('tls_enabled', tls_arg) + param_overrides['server.tls.enabled'] = tls_enabled + else: + # Nothing said otherwise, so the config file decides - read it + # rather than assume a profile. + tls_enabled = config_says( + LaunchConfiguration('config_file').perform(context), + 'server', 'tls', 'enabled') + 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: + 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.') + + # Same precedence as TLS above: explicit argument, then environment, + # then the config file. + # + # MEDKIT_JWT_SECRET is not merely a value here, it is a statement: + # "this deployment runs closed". The container image is where it comes + # from - `docker run ros2 launch ... bringup.launch.py` execs a + # command instead of the node, so the entrypoint's own parameters never + # reach it and this is the only path the variable has. Handing over the + # secret and leaving auth.enabled at whatever the config file says + # produced a gateway serving the entity tree, the fault history and + # every operation to anyone who could reach the port, with the operator + # told it was closed. So the variable turns authentication on and sets + # require_auth_for to "all" - "write" would leave every read open, and + # the reads are the disclosure. + auth_arg = LaunchConfiguration('auth_enabled').perform(context).strip() + 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', '')) + env_closes = (bool(os.environ.get('MEDKIT_JWT_SECRET', '')) + and os.environ.get('MEDKIT_AUTH_DISABLED') != '1') + if auth_arg: + 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 + param_overrides['auth.enabled'] = False + elif env_closes: + auth_enabled = True + param_overrides['auth.enabled'] = True + param_overrides['auth.require_auth_for'] = 'all' + else: + auth_enabled = config_says( + LaunchConfiguration('config_file').perform(context), + 'auth', 'enabled') + 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 env_closes and not clients: + print('[gateway.launch.py] MEDKIT_JWT_SECRET is set, so authentication is on ' + 'and every route needs a credential, but no client is configured and ' + 'nothing can obtain a token. Set MEDKIT_CLIENTS=::admin, or ' + 'pass auth_clients:=::admin.') + 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 +330,12 @@ 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_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/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/package.xml b/src/ros2_medkit_gateway/package.xml index 47d9c3479..4194c80ba 100644 --- a/src/ros2_medkit_gateway/package.xml +++ b/src/ros2_medkit_gateway/package.xml @@ -35,6 +35,7 @@ ament_index_python + python3-yaml launch launch_ros ros2_medkit_fault_manager diff --git a/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh b/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh old mode 100644 new mode 100755 index 16fd0c216..e14b8551e --- a/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh +++ b/src/ros2_medkit_gateway/scripts/generate_dev_certs.sh @@ -122,7 +122,11 @@ echo " tls:" echo " enabled: true" echo " cert_file: \"$OUTPUT_DIR/cert.pem\"" echo " key_file: \"$OUTPUT_DIR/key.pem\"" -echo " ca_file: \"$OUTPUT_DIR/ca.pem\"" +echo "" +echo " Do NOT add ca_file here. It is not the CA a client verifies the server" +echo " with - setting it makes the gateway REQUIRE a client certificate from" +echo " every caller, and the curl below would then be refused. Pass ca.pem to" +echo " the client with --cacert instead, as shown." echo "" echo "Test with curl:" echo " curl -k https://localhost:8080/api/v1/health" diff --git a/src/ros2_medkit_gateway/src/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 51ac0f626..50e28a5b3 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); @@ -65,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, @@ -85,8 +112,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 deployment that turns authentication on + // authenticates a client here, so this path carries 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 +282,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,25 +401,44 @@ 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; + // 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 { ++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 +679,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/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/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..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); @@ -435,7 +437,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) { @@ -496,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 fcc0948d0..fef043f13 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,55 @@ 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. +/// +/// 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. + } + 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. + // 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; + } + // 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_gateway/src/http/http_server.cpp b/src/ros2_medkit_gateway/src/http/http_server.cpp index 3979bd477..2aaefe186 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( @@ -33,13 +49,29 @@ 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_); 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 +170,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/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index 5b070aece..192c39b58 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -317,8 +317,25 @@ 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 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 is what keeps the exemption to the case that genuinely + // cannot authenticate: matching on the method and the origin alone would + // let a plain anonymous OPTIONS take this early return, which is + // "OPTIONS is public" rather than "a preflight 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 +346,46 @@ void RESTServer::setup_pre_routing_handler() { } } - // 3. Rate limiting check. If rejected, return Handled (CORS headers already set) + // 3. Rate limiting is METERED here and ANSWERED after authentication. + // + // 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. + // + // The X-RateLimit-* headers are NOT written here. They report how much + // allowance is left and when it resets, and writing them at metering time + // put them on the anonymous 401 below - limiter state disclosed to a caller + // holding no credential, which is the thing answering before authentication + // would have done. They go on after the auth decision, at the point the + // limiter is allowed to speak. + bool rate_limited = false; + RateLimitResult rl_result; + bool rate_metered = false; 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); - } + rl_result = rate_limiter_->check(req.remote_addr, req.path); + rate_limited = !rl_result.allowed; + rate_metered = true; } - // 1. Handle CORS (existing logic) - - // Handle Authentication if enabled + // 4. Authentication. if (auth_middleware_ && auth_middleware_->is_enabled()) { - // Use AuthMiddleware to process the request + // No special case for an exhausted anonymous caller. `process` returns + // on a missing Authorization header before it extracts or verifies + // anything, so a request with no credential already costs nothing beyond + // the route lookup - which is the whole of what an anonymous flood would + // buy by being refused earlier. A second refusal path here produced a + // 401 of a different shape from every other one: no WWW-Authenticate and + // no error document, so a client could not tell a missing credential + // from an expired one, and the difference was itself a signal that the + // limiter rather than the credential had decided. auto auth_request = AuthMiddleware::from_httplib_request(req); auto result = auth_middleware_->process(auth_request); @@ -353,6 +395,18 @@ void RESTServer::setup_pre_routing_handler() { } } + // 5. Now the limiter may speak, headers included. The caller reached this + // line with a credential this gateway accepts, or on a route that needs + // none, so the allowance and the reset time tell them something they are + // entitled to know. + if (rate_metered) { + RateLimiter::apply_headers(rl_result, res); + } + 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 aad73dd5d..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() @@ -677,6 +733,136 @@ 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 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) + << "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"; + + // 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"; + } +} + +// 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; @@ -1130,6 +1316,125 @@ TEST_F(AuthRequirementPolicyTest, AllAuthPolicyAlwaysRequiresAuth) { EXPECT_TRUE(policy.requires_authentication("DELETE", "/api/v1/admin/users")); } +// 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, AllAuthPolicyDoesNotExemptHealth) { + AllAuthRequirementPolicy policy; + + 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")); + + // 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 +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/package.xml b/src/ros2_medkit_integration_tests/package.xml index 3ae4e8ae0..58a2f60fe 100644 --- a/src/ros2_medkit_integration_tests/package.xml +++ b/src/ros2_medkit_integration_tests/package.xml @@ -35,6 +35,7 @@ ament_index_python python3-requests python3-jsonschema + python3-yaml ament_cmake_flake8 ros2_medkit_gateway ros2_medkit_fault_manager 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..5b8fcce73 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 - against a gateway running + ``require_auth_for: all`` with an empty ``auth.public_routes`` that 200 + never arrives, so a 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/ros2_medkit_test_utils/launch_helpers.py b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/launch_helpers.py index f36a329de..199d9c20c 100644 --- a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/launch_helpers.py +++ b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/launch_helpers.py @@ -102,7 +102,7 @@ def create_gateway_node(*, port=DEFAULT_PORT, name='ros2_medkit_gateway', extra_params=None, coverage=True, extra_env=None, - respawn=False, respawn_delay=1.0): + respawn=False, respawn_delay=1.0, params_file=None): """Create a ``gateway_node`` launch action with standard config. Parameters @@ -132,6 +132,11 @@ def create_gateway_node(*, port=DEFAULT_PORT, name='ros2_medkit_gateway', and the DDS participant are released before the replacement binds them, which also gives a test a window in which the port is provably down - the only way to tell "restarted" from "never died". + params_file : str or None + Path to a YAML params file loaded BEFORE the inline parameters, so a + test can launch a shipped profile - ``config/gateway_params.yaml`` or + ``config/gateway_params.secure.yaml`` - and still override the port and + the credentials a committed file cannot carry. Returns ------- @@ -142,6 +147,7 @@ def create_gateway_node(*, port=DEFAULT_PORT, name='ros2_medkit_gateway', params = {'refresh_interval_ms': 1000, 'server.port': port} if extra_params: params.update(extra_params) + file_then_inline = ([params_file] if params_file else []) + [params] env = dict(get_coverage_env() if coverage else {}) if extra_env: @@ -152,7 +158,7 @@ def create_gateway_node(*, port=DEFAULT_PORT, name='ros2_medkit_gateway', executable='gateway_node', name=name, output='screen', - parameters=[params], + parameters=file_then_inline, additional_env=env, respawn=respawn, respawn_delay=respawn_delay, 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..0fd3fff94 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py @@ -0,0 +1,696 @@ +#!/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 secure profile. + +The gateway's own half of the closed-door acceptance, run against +`config/gateway_params.secure.yaml` - the profile a deployment uses to close a +gateway. 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 os +import unittest + +from ament_index_python.packages import get_package_share_directory +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() +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}' +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' + +SECURE_PARAMS = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), + 'config', 'gateway_params.secure.yaml' +) + +# 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 loaded from the secure profile, the way a deployment closes one.""" + gateway_node = create_gateway_node( + port=CLOSED_PORT, + params_file=SECURE_PARAMS, + extra_params={ + 'server.host': '127.0.0.1', + # TLS is a deployment artefact, not the posture under test; the + # route sweep below is about who may reach a route, not about how + # the bytes travel. test_tls_protocol_floor covers the transport. + 'server.tls.enabled': False, + # Two settings of that profile are turned back off here, and each + # would otherwise make the sweep prove less than it claims. + # + # Rate limiting: the profile allows 120 requests per client per + # minute and this sweep sends two per route, so most of it would + # answer 429 instead of 401 - a refusal for the wrong reason. + # rl_gateway below is the gateway that exists to test the + # limiter's interaction with authentication. + 'rate_limiting.enabled': False, + # Docs: the profile turns the /docs routes off to reduce the + # surface. They are registered routes and this sweep is about + # every registered route, so leaving them off would quietly + # shrink what it covers. + 'docs.enabled': True, + # The secret and the client cannot come from a committed file - a + # committed secret is a secret every deployment shares - so they + # are supplied here the way a deployment supplies them. + 'auth.jwt_secret': JWT_SECRET, + 'auth.clients': [f'{CLIENT_ID}:{CLIENT_SECRET}:admin'], + }, + ) + + 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'], + # CORS on, which is what puts a preflight through the pre-routing handler. + 'cors.allowed_origins': [ALLOWED_ORIGIN], + }, + ) + + # Rate limiting on, and tight, so the limiter can actually be exhausted + # inside a test. The ordering between the limiter and authentication is + # only observable against a gateway in this state. + 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'], + # Small enough that a test can exhaust it, with room for the + # credentialed half of the case below to obtain a token and make a + # request before the allowance is gone. + 'rate_limiting.enabled': True, + 'rate_limiting.global_requests_per_minute': 5, + 'rate_limiting.client_requests_per_minute': 5, + }, + ) + + # 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, 'public_route_gateway': public_route_gateway} + + +def _is_exempt(method, path): + """Routes that are deliberately reachable without a credential. + + /auth/* alone, and only because 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. + """ + del method # the one exemption is path-shaped: every method under /auth/ + 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_health_refuses_like_everything_else(self): + """Health is not special. It is closed until somebody opens it. + + 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.assertIn( + resp.status_code, (401, 403), + f'GET /health answered {resp.status_code} with no credential and an ' + 'empty auth.public_routes' + ) + + 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 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.assertNotIn( + 'x-medkit-reduced', body, + 'an authenticated caller was served the cut-down body' + ) + + 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) + + +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. + """ + 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} for an entry that named GET' + ) + + 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 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. + + 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 refusing it 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_the_limiter_state_reaches_only_a_credentialed_caller(self): + """X-RateLimit-* says how much allowance is left and when it resets. + + Reporting it to a caller holding no credential is the disclosure that + answering 429 before authentication would have made, arriving by + another door. A caller who authenticates is entitled to it, and that + half is checked first - both because the allowance is still there, and + because without it this would pass against a gateway that had simply + stopped emitting the headers. + """ + token = requests.post( + f'{RL_BASE_URL}/auth/authorize', + json={ + 'grant_type': 'client_credentials', + 'client_id': CLIENT_ID, + 'client_secret': CLIENT_SECRET, + }, + timeout=30, + ) + self.assertEqual(token.status_code, 200, token.text) + authed = requests.get( + f'{RL_BASE_URL}/areas', + headers={'Authorization': f'Bearer {token.json()["access_token"]}'}, + timeout=15, + ) + self.assertEqual(authed.status_code, 200, authed.text) + self.assertNotEqual( + [h for h in authed.headers if h.lower().startswith('x-ratelimit')], [], + f'a credentialed caller was told nothing about the limiter: ' + f'{dict(authed.headers)}' + ) + + anonymous = requests.get(f'{RL_BASE_URL}/apps', timeout=15) + self.assertIn(anonymous.status_code, (401, 403), anonymous.text) + leaked = [h for h in anonymous.headers if h.lower().startswith('x-ratelimit')] + self.assertEqual( + leaked, [], + f'an uncredentialed caller was told the limiter state: {leaked}' + ) + + def test_02_an_exhausted_anonymous_caller_still_gets_401(self): + responses = [] + # Comfortably past a limit of 5/minute. + for _ in range(12): + responses.append(requests.get(f'{RL_BASE_URL}/apps', timeout=15)) + seen = [r.status_code for r in responses] + + 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}' + ) + + def test_03_the_exhausted_refusal_is_shaped_like_every_other_401(self): + """Status alone is not the contract; the body and the challenge are. + + The refusal an exhausted anonymous caller gets is produced on a + different line from the one an unexhausted caller gets, so nothing + stops the two drifting apart. A client that reads the error document to + tell a missing credential from an expired one gets nothing to read from + a bare 401 - and a 401 that is visibly a different shape is itself the + disclosure that the limiter, not the credential, decided it. + """ + fresh = requests.get(f'{RL_BASE_URL}/areas', timeout=15) + exhausted = None + for _ in range(8): + exhausted = requests.get(f'{RL_BASE_URL}/apps', timeout=15) + + self.assertEqual( + fresh.json().keys(), exhausted.json().keys(), + f'the two refusals are different documents: {fresh.json()} vs ' + f'{exhausted.json()}' + ) + for label, resp in (('first', fresh), ('exhausted', exhausted)): + with self.subTest(response=label): + self.assertIn(resp.status_code, (401, 403), resp.text) + self.assertIn( + 'WWW-Authenticate', resp.headers, + f'the {label} refusal carries no challenge header: ' + f'{dict(resp.headers)}' + ) + body = resp.json() + # The shape AuthMiddleware produces: an OAuth-style error and + # a human-readable description, not the SOVD `error_code` + # envelope the handlers use for their own refusals. + self.assertTrue( + body.get('error'), + f'the {label} refusal carries no error: {body}' + ) + self.assertTrue( + body.get('error_description'), + f'the {label} refusal carries no error_description: {body}' + ) + + +@launch_testing.post_shutdown_test() +class TestClosedByDefaultShutdown(unittest.TestCase): + """Gateway exits cleanly.""" + + def test_exit_codes(self, proc_info, gateway_node, cors_gateway, rl_gateway, + public_route_gateway): + for proc in (gateway_node, cors_gateway, rl_gateway, public_route_gateway): + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES, process=proc + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_env_closes_the_gateway.test.py b/src/ros2_medkit_integration_tests/test/features/test_env_closes_the_gateway.test.py new file mode 100644 index 000000000..b9094eb9c --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_env_closes_the_gateway.test.py @@ -0,0 +1,156 @@ +#!/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. + +"""MEDKIT_JWT_SECRET in the environment closes a gateway started by the launch file. + +The container image is where this matters. `docker run ros2 launch +ros2_medkit_gateway bringup.launch.py` execs a command instead of the node, so +the entrypoint's own `-p` arguments never reach the gateway and the environment +is the only channel the variable has. A launch file that took the secret and +left `auth.enabled` at the open profile's `false` produced exactly the failure +worth a test: the operator is told the container is closed, and it serves the +entity tree, the fault history and every operation to anyone who reaches the +port. + +Driven through `gateway.launch.py` itself, with no launch arguments, because +the defect lived in the argument-versus-environment precedence inside that +file. Supplying the parameters directly would test the gateway, which was never +wrong here. + +@verifies REQ_INTEROP_086, REQ_INTEROP_087 +""" + +import os +import unittest + +from ament_index_python.packages import get_package_share_directory +import launch +from launch.actions import IncludeLaunchDescription +from launch.launch_description_sources import PythonLaunchDescriptionSource +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 + +PORT = get_test_port() +BASE_URL = f'http://127.0.0.1:{PORT}{API_BASE_PATH}' + +JWT_SECRET = 'an_environment_supplied_secret_of_at_least_32_chars' +CLIENT_ID = 'envclosed' +CLIENT_SECRET = 'env_closed_client_secret' + + +@pytest.mark.launch_test +def generate_test_description(): + """Include gateway.launch.py with the closing variables in its environment. + + No launch argument names auth or TLS. The config file it defaults to is the + open profile, so anything that closes this gateway came from the + environment - which is what the case is about. + """ + launch_file = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), 'launch', 'gateway.launch.py') + + # SetEnvironmentVariable, because gateway.launch.py reads os.environ when + # the launch description is evaluated and that happens in this process. + # Confining it to this file is what launch_testing already does: each test + # file runs in a process of its own, so the variable reaches this gateway + # and no other. + return launch.LaunchDescription([ + launch.actions.SetEnvironmentVariable('MEDKIT_JWT_SECRET', JWT_SECRET), + launch.actions.SetEnvironmentVariable( + 'MEDKIT_CLIENTS', f'{CLIENT_ID}:{CLIENT_SECRET}:admin'), + IncludeLaunchDescription( + PythonLaunchDescriptionSource(launch_file), + launch_arguments={ + 'server_port': str(PORT), + 'server_host': '127.0.0.1', + }.items(), + ), + launch_testing.actions.ReadyToTest(), + ]), {} + + +class TestEnvClosesTheGateway(GatewayTestCase): + """The environment variable alone is enough to close the gateway.""" + + BASE_URL = BASE_URL + + def test_01_an_anonymous_read_is_refused(self): + """The claim the image documentation makes, checked on the launch path. + + ``/areas`` and not ``/health``: a read is what "closed" has to mean + here. Under ``require_auth_for: "write"`` - the open profile's value, + and what the gateway keeps if only ``auth.enabled`` is asserted - this + request answers 200 with authentication switched on. + """ + resp = requests.get(f'{BASE_URL}/areas', timeout=15) + self.assertIn( + resp.status_code, (401, 403), + f'MEDKIT_JWT_SECRET was in the environment and an anonymous GET ' + f'/areas answered {resp.status_code}. Body: {resp.text[:300]}' + ) + + def test_02_health_is_refused_too(self): + """No route is exempt but the auth routes, health included.""" + resp = requests.get(f'{BASE_URL}/health', timeout=15) + self.assertIn( + resp.status_code, (401, 403), + f'an anonymous GET /health answered {resp.status_code}' + ) + + def test_03_the_environment_credential_issues_a_working_token(self): + """The mirror: a gateway that refused everyone would pass the two above. + + It also pins MEDKIT_CLIENTS reaching the gateway - without it the + container is closed to its operator as well as to everyone else. + """ + token = requests.post( + f'{BASE_URL}/auth/authorize', + json={ + 'grant_type': 'client_credentials', + 'client_id': CLIENT_ID, + 'client_secret': CLIENT_SECRET, + }, + timeout=30, + ) + self.assertEqual(token.status_code, 200, token.text) + access = token.json()['access_token'] + resp = requests.get( + f'{BASE_URL}/areas', + headers={'Authorization': f'Bearer {access}'}, + timeout=15, + ) + self.assertEqual(resp.status_code, 200, resp.text) + + +@launch_testing.post_shutdown_test() +class TestEnvClosesTheGatewayShutdown(unittest.TestCase): + """Every process exits cleanly. + + Swept without naming one: the gateway is created inside the included launch + file, so this file holds no handle to it. + """ + + def test_exit_codes(self, proc_info): + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES) diff --git a/src/ros2_medkit_integration_tests/test/features/test_open_default_profile.test.py b/src/ros2_medkit_integration_tests/test/features/test_open_default_profile.test.py new file mode 100644 index 000000000..4da937ad8 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_open_default_profile.test.py @@ -0,0 +1,153 @@ +#!/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. + +"""The default profile answers a caller who holds no credential. + +``config/gateway_params.yaml`` is what every existing launch, every existing +config file and the web UI get when they name no profile of their own, and none +of them sends an ``Authorization`` header. Closing that file is therefore a +breaking change for all three at once, and it is a one-word edit. + +The mirror of ``test_secure_profile``: that file pins the closed profile +closed, this one pins the open profile open, and between them a flip in either +direction has to be deliberate. Nothing else in the suite would notice - every +other test supplies the auth parameters it needs inline, so both files could +say anything and stay green. + +What this does NOT assert is that leaving it open is correct. It asserts that +the file has not changed underneath a deployment that already trusts it. + +@verifies REQ_INTEROP_086 +""" + +import os +import unittest + +from ament_index_python.packages import get_package_share_directory +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 +import yaml + +PORT = get_test_port() +BASE_URL = f'http://127.0.0.1:{PORT}{API_BASE_PATH}' + +DEFAULT_PARAMS = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), + 'config', 'gateway_params.yaml' +) + + +@pytest.mark.launch_test +def generate_test_description(): + """Launch the gateway from the default params file, overriding only the port.""" + gateway_node = create_gateway_node( + port=PORT, + params_file=DEFAULT_PARAMS, + # Nothing about auth or TLS is set here: whatever the file says is + # exactly what this test is about. The host is narrowed because a test + # has no business binding every interface on the machine it runs on. + extra_params={'server.host': '127.0.0.1'}, + ) + + return launch.LaunchDescription([ + gateway_node, + launch_testing.actions.ReadyToTest(), + ]), {'gateway_node': gateway_node} + + +class TestOpenDefaultProfile(GatewayTestCase): + """The default profile serves an uncredentialed caller.""" + + BASE_URL = BASE_URL + + def test_01_an_anonymous_read_of_areas_succeeds(self): + """A GET with no Authorization header is answered, not refused. + + ``/areas`` rather than ``/health``: health has its own anonymous + handling (a reduced body on a route opened through + ``auth.public_routes``), so it can answer 200 for a reason that has + nothing to do with the profile. An entity collection has no such path - + a 200 here means the request was authorised. + """ + resp = requests.get(f'{BASE_URL}/areas', timeout=15) + self.assertEqual( + resp.status_code, 200, + f'the default profile answered an anonymous GET /areas with ' + f'{resp.status_code}; every existing launch and the web UI send no ' + f'credential. Body: {resp.text[:300]}' + ) + + def test_02_an_anonymous_read_of_health_succeeds_in_full(self): + """Health answers, and answers whole. + + ``x-medkit-reduced`` marks the cut-down body an anonymous caller gets + when authentication is on. Its absence is what separates "auth is off" + from "auth is on and this route was opened". + """ + resp = requests.get(f'{BASE_URL}/health', timeout=15) + self.assertEqual(resp.status_code, 200, resp.text) + body = resp.json() + self.assertEqual(body.get('status'), 'healthy', body) + self.assertNotIn( + 'x-medkit-reduced', body, + 'health came back marked reduced, which means authentication is on ' + f'in the default profile: {body}' + ) + + def test_03_the_file_under_test_is_the_open_one(self): + """Guard against this test drifting off the file it names. + + Read by key path through a YAML parse. The three values are the whole + of the profile's posture, and each is one word away from its opposite. + """ + with open(DEFAULT_PARAMS, encoding='utf-8') as handle: + document = yaml.safe_load(handle) + params = document['ros2_medkit_gateway']['ros__parameters'] + auth = params['auth'] + self.assertIs( + auth['enabled'], False, + f"the default profile sets auth.enabled to {auth['enabled']!r}" + ) + self.assertEqual( + auth['require_auth_for'], 'write', + 'the default profile sets auth.require_auth_for to ' + f'{auth["require_auth_for"]!r}' + ) + self.assertIs( + params['server']['tls']['enabled'], False, + 'the default profile sets server.tls.enabled to ' + f"{params['server']['tls']['enabled']!r}" + ) + + +@launch_testing.post_shutdown_test() +class TestOpenDefaultProfileShutdown(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_openapi_contract.test.py b/src/ros2_medkit_integration_tests/test/features/test_openapi_contract.test.py index aa9755188..4a1e88659 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_openapi_contract.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_openapi_contract.test.py @@ -148,6 +148,17 @@ class TestOpenApiContract(GatewayTestCase): MIN_EXPECTED_APPS = 2 REQUIRED_APPS = {'calibration', 'temp_sensor'} + # The app entities appearing is not enough for this file. A node is listed + # in the ROS graph before its service endpoints have propagated, so a + # discovery sweep can build the App with an empty service list, and the + # cache-derived operation items in `/docs` are built from exactly that + # list. Until one service is in the cache every operations sub-document + # publishes only projections, and the comparison over `operations` in + # `test_a_scoped_item_says_what_its_templated_sibling_says` has nothing to + # compare. Waiting for the capability the assertion reads, rather than for + # the entity that carries it, is what makes the file independent of how + # fast the runner propagates a service. + REQUIRED_OPERATIONS = {'/apps/calibration': 'calibrate'} _spec = None diff --git a/src/ros2_medkit_integration_tests/test/features/test_secure_profile.test.py b/src/ros2_medkit_integration_tests/test/features/test_secure_profile.test.py new file mode 100644 index 000000000..abd1bedd1 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_secure_profile.test.py @@ -0,0 +1,237 @@ +#!/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 SECURE profile and check that it is closed. + +``config/gateway_params.yaml`` is the open profile and +``config/gateway_params.secure.yaml`` is the closed one. Every other test in +this suite builds its parameters inline, which is fine for testing behaviour +but means nothing loads either file - so the secure profile could be edited +back 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 for the secure profile. It launches with the +installed copy of that file, 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 it ships is closed. +``test_open_default_profile`` is the mirror for the other file. + +TLS is turned off here and only here. The secure 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 +import yaml + +PORT = get_test_port() +BASE_URL = f'http://127.0.0.1:{PORT}{API_BASE_PATH}' + +SECURE_PARAMS = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), + 'config', 'gateway_params.secure.yaml' +) + +JWT_SECRET = 'secure_profile_integration_secret_key_01234567890' +CLIENT_ID = 'secure' +CLIENT_SECRET = 'secure_client_secret' + + +@pytest.mark.launch_test +def generate_test_description(): + """Launch the gateway with the secure 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=[ + SECURE_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 secure 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 secure profile, + 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 TestSecureProfile(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_secure_file_turns_authentication_on(self): + """Reverting auth.enabled in the secure 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 secure profile served /areas to an anonymous caller' + ) + + def test_02_the_secure_file_covers_reads_not_just_writes(self): + """Pins require_auth_for: "all" as the secure profile's 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_refuses_in_the_secure_file_too(self): + """The secure file opens nothing, health included. + + `auth.public_routes` is absent from the secure profile, so the file + that a closed deployment starts from 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 secure profile + that quietly did so would still pass every other test in this class. + """ + resp = requests.get(f'{BASE_URL}/health', timeout=15) + self.assertIn( + resp.status_code, (401, 403), + f'the secure profile 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.""" + resp = requests.get(f'{BASE_URL}/areas', headers=self.auth, timeout=15) + self.assertEqual(resp.status_code, 200) + + def test_05_the_secure_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. + + Read through a YAML parse and addressed by key path. A substring search + cannot do this job: ``enabled: true`` occurs under several different + parents in this file, so a text guard for it stays green with + ``auth.enabled: false`` - the one drift this test exists to catch. The + parse also settles ``public_routes`` for free, because a commented + example is not a key and a key written in flow style still is one. + """ + with open(SECURE_PARAMS, encoding='utf-8') as handle: + document = yaml.safe_load(handle) + params = document['ros2_medkit_gateway']['ros__parameters'] + auth = params['auth'] + self.assertIs( + auth['enabled'], True, + f"the secure profile sets auth.enabled to {auth['enabled']!r}" + ) + self.assertEqual( + auth['require_auth_for'], 'all', + 'the secure profile sets auth.require_auth_for to ' + f'{auth["require_auth_for"]!r}' + ) + self.assertIs( + params['server']['tls']['enabled'], True, + 'the secure profile sets server.tls.enabled to ' + f"{params['server']['tls']['enabled']!r}" + ) + # No route is opened by the file itself. An entry here would be a + # public route in every deployment that uses this profile, which is + # exactly what it exists to stop. + self.assertNotIn( + 'public_routes', auth, + 'the secure profile declares public routes: ' + f'{auth.get("public_routes")!r}' + ) + + +@launch_testing.post_shutdown_test() +class TestSecureProfileShutdown(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 new file mode 100644 index 000000000..5dec68740 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_tls_protocol_floor.test.py @@ -0,0 +1,334 @@ +#!/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. 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: a client with no certificate + # against this gateway prints "Cipher is ECDHE-RSA-AES256-GCM-SHA384" AND + # "sslv3 alert handshake failure", while curl on the same endpoint gets no + # HTTP response at all. + # + # The fatal alert is therefore 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 _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. + + 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_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. + + 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)