Ship closed by default: auth on, reads included, and a test that walks every route - #641
Draft
bburda wants to merge 3 commits into
Draft
Ship closed by default: auth on, reads included, and a test that walks every route#641bburda wants to merge 3 commits into
bburda wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the default gateway posture to be “closed by default” (TLS + auth enabled, and auth required for reads as well as writes), adds a narrowly-scoped public health exemption, and introduces an integration acceptance test that sweeps the live route registry to ensure unauthenticated access is refused across the surface area.
Changes:
- Switch shipped defaults to
auth.enabled: true,auth.require_auth_for: "all", andserver.tls.enabled: true. - Update
AllAuthRequirementPolicyto exempt onlyGET /api/v1/health(plus/api/v1/auth/*) and add unit tests pinning the exemption boundary. - Add/adjust integration tests: a full route-sweep “closed by default” acceptance test and a more precise recovery poll in
test_peer_recovery.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/test/features/test_peer_recovery.test.py | Tightens polling to wait for actual “data” readiness rather than HTTP 200 alone. |
| src/ros2_medkit_integration_tests/test/features/test_closed_by_default.test.py | New integration acceptance that sweeps all registered routes for unauthenticated refusal + authenticated pass-through. |
| src/ros2_medkit_gateway/test/test_auth_manager.cpp | Adds regression tests pinning the GET /api/v1/health exemption and /api/v1/auth/* public endpoints under the “all” policy. |
| src/ros2_medkit_gateway/launch/gateway.launch.py | Adds launch args for enabling/disabling auth and injecting JWT secret + clients for quickstart usability. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/auth/auth_requirement_policy.hpp | Implements the health/auth exemptions for the “all” auth-requirement policy and updates its description string. |
| src/ros2_medkit_gateway/design/hardening.rst | Updates hardening guidance to reflect the new shipped-default locked posture and its implications. |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Changes shipped default params to enable TLS + auth and require auth for all operations. |
| README.md | Updates example curl commands to include Authorization header. |
| docs/tutorials/triggers-use-cases.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/snapshots.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/scripts.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/openapi.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/migration-to-manifest.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/manifest-discovery.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/locking.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/linux-introspection.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/heuristic-apps.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/graph-provider.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/fault-correlation.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/docker.rst | Updates curl examples to include Authorization header (non-health endpoint). |
| docs/tutorials/demos/demo-turtlebot3.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/demos/demo-sensor.rst | Updates curl examples to include Authorization header. |
| docs/tutorials/beacon-discovery.rst | Updates curl examples to include Authorization header. |
| docs/index.rst | Adds a “ships closed” note and updates quick reference curls to include Authorization header. |
| docs/getting_started.rst | Updates quickstart launch invocation and subsequent curl examples for the closed-by-default posture. |
| docs/config/discovery-options.rst | Updates curl examples to include Authorization header. |
| docs/api/rest.rst | Updates curl examples to include Authorization header. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
835
to
+841
| def served(): | ||
| answer = self._aggregate_read_of_peer_topic() | ||
| return answer if answer.status_code == 200 else None | ||
| if answer.status_code != 200: | ||
| return None | ||
| if answer.json().get('x-medkit', {}).get('status') != 'data': | ||
| return None | ||
| return answer |
Comment on lines
+114
to
+119
| 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.')) |
Comment on lines
+151
to
+156
| jwt_secret = LaunchConfiguration('jwt_secret').perform(context) | ||
| clients = LaunchConfiguration('auth_clients').perform(context) | ||
| if jwt_secret: | ||
| param_overrides['auth.jwt_secret'] = jwt_secret | ||
| if clients: | ||
| param_overrides['auth.clients'] = [c for c in clients.split(',') if c] |
Comment on lines
97
to
99
| std::string description() const override { | ||
| return "AllAuth: Authentication required for all endpoints except /auth/*"; | ||
| return "AllAuth: Authentication required for all endpoints except /auth/* and GET /health"; | ||
| } |
Comment on lines
+57
to
+59
| The gateway ships closed: every route below needs a credential, and | ||
| ``GET /api/v1/health`` is the only one that does not. See | ||
| :doc:`getting_started` for how to obtain ``$TOKEN``. |
Comment on lines
+43
to
+45
| ros2 launch ros2_medkit_gateway gateway.launch.py \ | ||
| jwt_secret:=change-me-to-at-least-32-characters-long \ | ||
| auth_clients:=demo:demo-secret:admin |
bburda
changed the title
The default config had TLS and auth off and require_auth_for set to "write". A deployment that used it got a gateway where anyone who could reach the port could read the entity tree, the fault history and the operations. With auth on, "write" still left every read open, and the reads are the problem: the entity tree has the machine names, the fault history is the maintenance record. config/gateway_params.yaml now sets auth.enabled true, require_auth_for "all" and tls.enabled true. This means the gateway does not start when no signing secret is given. That is on purpose. A gateway that does not start is a config problem you fix in a minute. A gateway that started open is a problem nobody sees. AllAuthRequirementPolicy now exempts the health probe. Its doc comment already said it did, but the code did not. Without the exemption the result is not strict, it is broken: a container supervisor and a load balancer call /health with no credential, so requiring one makes every healthy gateway restart in a loop. The exemption is only GET /api/v1/health, not HEAD and not /health/detail, because a prefix or suffix match would let someone bypass auth by adding the word health to a path. /health also stops returning its full body to a caller with no credential. If a route is public we own what it says, and the full body was not safe to publish: linking.warnings holds strings like "App 'engine_ecu' cannot bind to '/nav/controller'", which is an entity name and a ROS node FQN, and x-medkit-entity-cache holds the counts of apps, areas and components. A caller with no credential now gets status and timestamp only. The check runs before any section is built, so a section added later is private by default. test_closed_by_default.test.py is the main test. It asks the running gateway for its route list and calls all 242 routes with no credential, then again with a fake token. A test that read config values would keep passing on the day someone registers a route outside the policy. Walking the registry does not, and a route added later is covered on the day it is added. Two unit tests cover the edges of the exemption as fast regression cover. Also fixed here because this branch hit it: test_peer_recovery test_07 waited for HTTP 200 and then checked the body had live data. Recovery has two steps, the route comes back and then a sample arrives on the new subscription, and between them the read returns 200 with status "metadata_only" and an empty body. Under load the poll returned a response from that window. It now waits for the state it checks. gateway.launch.py takes jwt_secret, auth_clients and auth_enabled, and prints what to pass when auth is on and no secret was given. The docs stop showing unauthenticated curl: getting_started explains the credential once and every example sends it, with /health left bare because it is the exemption. hardening.rst describes the current defaults. Do not merge alone: the appliance deployment profiles that rely on this exemption ship separately and expect it.
bburda
force-pushed
the
feat/ship-hardened-defaults
branch
from
August 26, 2026 18:17
b69681b to
0771718
Compare
Making a route public means owning what it says, and the full /health body was not safe to publish. linking.warnings holds strings like "App 'engine_ecu' cannot bind to '/nav/controller'", which is an entity name and a ROS node FQN, and x-medkit-entity-cache holds the counts of apps, areas and components this gateway sees. A liveness probe needs none of that. A caller with no credential, or with a token this gateway did not issue, now gets status and timestamp only. A caller with a valid token gets the whole document. The check runs before any section is built, so a section added later is private by default instead of public until someone remembers to think about it. When auth is off nothing is anonymous and the body is unchanged. This closes item 4 of #259. The test for this was wrong before and is worth saying why. It listed field names known to leak and checked they were absent. That list did not include warnings or x-medkit-entity-cache, which are the actual vectors, so it passed while proving nothing - the warnings array happened to be empty on the gateway it ran against. It now asserts the exact set of keys an anonymous caller gets, which is an allowlist and cannot silently miss a field added later, and a second test asserts the authenticated body still carries the operator detail so the narrowing does not quietly become pointless.
bburda
marked this pull request as draft
August 26, 2026 20:10
…tore growing This branch turns TLS on for every default deployment, so the TLS settings had to start meaning something. Three of them did not. min_version was read, logged and ignored. The comment said cpp-httplib does not expose SSL_CTX; the vendored copy declares a public ssl_context(), so it does. Worse, the vendored SSLServer constructor calls SSL_CTX_set_min_proto_version(ctx_, TLS1_1_VERSION), so the floor a deployment gets is the library's request filtered through whatever the local OpenSSL policy happens to allow. Neither of those is a value this project chose, and SOVD requires TLS 1.2 as its minimum, so the floor is now set on our own context from the configured value. An unrecognised value was already rejected in TlsConfig::validate(), so the runtime warning had nothing left to say and is gone with the comment. Measured on Jazzy, Ubuntu 24.04, OpenSSL 3.0.13, one gateway at a time: before this change a gateway configured for min_version 1.3 completed a TLS 1.2 handshake. TLS 1.0 and 1.1 were already refused on this system, so the floor was not at 1.1 here; it was wherever OpenSSL put it, which is the actual problem. After the change, 1.3 refuses 1.2 and 1.2 refuses 1.1. ca_file was parsed into TlsConfig and dropped on the floor. The vendored constructor takes a client CA path and, given one, does the SSL_CTX_load_verify_locations and SSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERT work itself, so it is now passed through. Setting it makes a client certificate required for that gateway; leaving it empty is unchanged server-only TLS, which is what the bearer-token flow expects. SOVD says nothing about client certificates, so this stays opt-in. TLS also had no way out at the launch level while auth had one, even though this branch defaults tls.enabled true with an empty cert_file. gateway.launch.py now takes tls_enabled, cert_file and key_file in the same shape as the auth arguments, and names them when TLS is on and no certificate was given. cleanup_expired_tokens() had no caller outside a test, so refresh_tokens_ grew by one record per successful authorisation for the life of the process, and validate_token looks that map up on every authenticated request. The sweep now runs when a record is stored, which keeps the bound a property of the data structure rather than of a timer that may not be running, and makes it observable without waiting on wall clock. refresh_token_count() exists so a test can assert on the quantity the claim is about. Client secrets were compared with !=, which returns on the first differing byte. They are now compared in constant time. The secrets are still stored in plaintext in the configuration; that is a config-format question and is deliberately not addressed here. An access token naming a refresh record that is no longer present is now invalid rather than unchecked. Records live in memory, so this is also what a restart looks like: clients re-authenticate after one. That is a visible behaviour change and it is the intended trade. The TLS floor and client-certificate behaviour are settled by handshakes from a real client against real gateways, because both are decided before any HTTP request exists. Two gateways run side by side at floors 1.2 and 1.3 so the floor is shown to move with the setting rather than sitting where OpenSSL's default put it. Worth recording for whoever reads that file next: "Cipher is <name>" in s_client output does not mean the handshake completed, because under TLS 1.2 the suite is agreed before the client certificate is examined. Reading it that way reported mutual TLS as broken while it was working.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
The default config had TLS and auth off, and
require_auth_forwas"write". So a deployment that uses the default config gets a gateway where anyone who can reach the port can read the entity tree, the fault history and the operations. Even with auth on,"write"left all reads open. The reads are the problem: the entity tree has the machine names, the fault history is the maintenance record.config/gateway_params.yamlnow hasauth.enabled: true,require_auth_for: "all"andtls.enabled: true.This means the gateway does not start if you give it no signing secret. This is on purpose. A gateway that does not start is a config problem you fix in a minute. A gateway that started open is a problem nobody sees.
gateway.launch.pynow takesjwt_secret,auth_clientsandauth_enabled, so the quickstart is one line longer instead of broken. It also prints what to pass if auth is on and no secret was given. Useauth_enabled:=falseon a host that nothing else can reach.Two changes that flags alone did not cover
1.
AllAuthRequirementPolicynow exempts the health probe. Its doc comment already said it did this, but the code did not. Without the exemption the result is not "strict", it is broken: a container supervisor or a load balancer calls/healthwith no credential, so requiring one makes every healthy gateway restart in a loop.The exemption is only
GET /api/v1/health. NotHEAD, not/health/detail, not/healthz. A prefix or suffix match would let someone bypass auth by adding the wordhealthto a path.2.
/healthno longer returns the full body to a caller with no credential. If a route is public, we own what it says, and the full body was not safe to make public:linking.warningshas strings like"App 'engine_ecu' cannot bind to '/nav/controller'". That is an entity name and a ROS node FQN.x-medkit-entity-cachehas the counts of apps, areas and components this gateway sees.A caller with no credential now gets only
statusandtimestamp. A caller with a valid token gets the whole body. The check runs before any section is built, so a section added later is private by default. This is item 4 of #259.TLS: the settings now do what they say
Turning TLS on for every default deployment meant the TLS settings had to start
meaning something. Three did not.
min_versionwas inert. It was read, logged, and ignored. The comment saidcpp-httplib does not expose
SSL_CTX; the vendored copy declares a publicssl_context(), so it does. The vendoredSSLServerconstructor also callsSSL_CTX_set_min_proto_version(ctx_, TLS1_1_VERSION), so the floor adeployment actually got was the library's request filtered through whatever the
local OpenSSL policy allowed. Neither value is one this project chose, and SOVD
requires TLS 1.2 as its minimum.
Measured before making the change, on Jazzy / Ubuntu 24.04 / OpenSSL 3.0.13,
with one gateway running and
min_versionset to"1.3":Note what that says. TLS 1.0 and 1.1 were already refused on this system, so
the effective floor here was not 1.1 - OpenSSL's own default was doing the
work. That is exactly the problem: the floor belonged to the operating system, not to
us, and it moves with the distribution (we build for Ubuntu 22.04 and 26.04
too) while the vendored library asks for 1.1. The row that matters is TLS 1.2
being accepted by a gateway configured for 1.3.
The client is run with
-cipher ALL:@SECLEVEL=0in these measurements and inthe test, because a modern OpenSSL client will not otherwise offer 1.0/1.1 at
all and the test would pass without the server ever refusing anything.
ca_filewas parsed and dropped. It is now passed to the constructor,which does the
SSL_CTX_load_verify_locationsandSSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERTwork itself. Setting it makesa client certificate required for that gateway - there is no "verify only
if offered" setting without patching the vendored header. Empty stays
server-only TLS. SOVD specifies bearer tokens and says nothing about client
certificates, so this is opt-in and the default path is unchanged.
TLS had no launch escape hatch while auth had one.
gateway.launch.pynowtakes
tls_enabled,cert_fileandkey_filein the same shape, and namesthem when TLS is on and no certificate was supplied.
docs/tutorials/https.rstcovers the new defaults, how to get a certificate for a first run, and mutual
TLS.
Auth hygiene this default makes everyone's problem
The refresh-token store had no sweeper.
cleanup_expired_tokens()existedwith no caller outside a test, so
refresh_tokens_grew by one record persuccessful authorisation for the life of the process, and
validate_tokenlooks that map up on every authenticated request. With auth off by default the
leak was opt-in; this branch makes it universal. The sweep now runs when a
record is stored.
Client secrets compared with
!=, which returns on the first differingbyte. Now compared in constant time. The secrets are still stored in
plaintext in the configuration. That is a config-format change and is
deliberately not in this PR, so please do not read the constant-time fix as the
whole answer.
Revoked tokens came back after a restart.
validate_tokenrejected a tokenonly when its refresh record existed and was marked revoked; records are
in-memory, so after a restart the map is empty and a revoked token was accepted
again until its
exp. An absent record now counts as invalid. The cost isdeliberate and visible: a restart invalidates every access token, so
clients re-authenticate after one.
Two things a release manager needs to read
Neither is fixed here, and neither is a code change in this PR.
Our own web UI stops working out of the box. Verified on
ros2_medkit_web_uiorigin/main: it builds its client ascreateMedkitClient({ baseUrl, fetch })(src/lib/store.ts:1097) and noAuthorizationheader appears anywhere insrc/. Its connect probe isGET /health(store.ts:1102), which the exemption above lets through, so theUI reports itself connected and then 401s on
GET /and on every entityload. The user sees a connected gateway with an empty tree, which is a worse
failure than a refused connection because nothing on screen says what is wrong.
CORS is off by default (
cors.allowed_origins: [""]), so no browser clientreaches the gateway on the default config regardless of tokens.
Together: "closed by default" currently also means "our own first-party client
is broken by default". Fixing the UI is a separate change in a separate
repository.
Testing
test/features/test_closed_by_default.test.pyis the main test. It asks the running gateway for its route list and then calls all 242 routes with no credential, and again with a fake token.I did it this way on purpose. A test that reads the config values keeps passing on the day someone registers a route outside the policy. A test that walks the route registry does not, and a route added later is covered on the day it is added.
The file also has the opposite checks: health still answers, and a valid token still works. Without those, a gateway that refused everything would pass a refusal-only test file while being completely broken.
/health, the near-miss paths), the token sweep at both ends of the expiry range, the secret comparison against prefixes and extensions, and revocation across a simulated restart.test_graph_provider_stalethat pass 4/4 when run alone. Every full run on this branch has failed a different file, always with a killed process rather than a wrong assertion, while other test suites were running concurrently on the same machine. No test was skipped and no timeout was widened.test_tls_protocol_floor.test.pydrivesopenssl s_clientagainst three real gateways: floors at 1.2 and 1.3 side by side, plus one requiring a client certificate. Nine tests, and it passed inside the full suite run, not only standalone.require_auth_forback to"write"fails all 8 route-sweep tests. Disabling the/healthnarrowing fails 2. Pinning the TLS floor to 1.1 failstest_03. Droppingca_filefailstest_05andtest_07. Removing the token sweep fails the growth test, and making revocation fail open fails the restart test. Every mutation was reverted and re-verified green.A note for whoever edits the TLS test next:
Cipher is <name>ins_clientoutput does not mean the handshake completed. Under TLS 1.2 the cipher
suite is agreed before the client certificate is examined, so a rejected client
still prints one, followed by a fatal alert. Reading it that way made an
earlier version of this test report mutual TLS as broken while it was working.
One more fix in this branch.
test_peer_recoverytest_07waited for HTTP 200 and then checked that the body had live data. Recovery has two steps: the route comes back, then a sample arrives on the new subscription. Between them the read returns 200 withstatus: "metadata_only"and an empty body. Under load the poll returned a response from that window and the test failed. It now waits for the state it checks.Docs
getting_startedexplains the credential once, and every example sends it./healthis left without one because it is the exemption.hardening.rstnow describes the current defaults.One note if you copy an older example:
POST /auth/authorizetakes theclient_credentialsgrant./auth/tokenis the refresh endpoint.Issue
/healthinfo leak)Type
Breaking change, in four ways:
auth.jwt_secretandauth.clients, or passauth_enabled:=false.cert_fileandkey_file, or passtls_enabled:=false.Checklist
Do not merge this alone
There is a companion change to the appliance deployment profiles. Those profiles need the health exemption above. If this is merged alone, their container healthchecks get 401 and the appliance stack restarts in a loop. The companion is tracked internally and is not in this repository.