From b28dda1b8451a41c4886699d44003930839bd5e3 Mon Sep 17 00:00:00 2001 From: Michael Junek Date: Sun, 15 Nov 2026 15:19:07 +1100 Subject: [PATCH 1/2] fix(orchestrator): find the client address behind trusted proxies The client address was the last X-Forwarded-For hop, which is the address the web tier saw. Behind the TLS proxy or load balancer a deployment needs, that is the proxy for every request: five failed sign-ins from anyone locked out all sign-ins, API tokens' allowed networks were compared with the proxy, and the audit log recorded only the proxy. DV_TRUSTED_PROXIES lists the proxies in front of the web tier. The client address is now the last hop that is not one of them, so hops a client sends itself are never believed. With none set the behaviour is unchanged. --- deploy/orchestrator/orchestrator.env.example | 4 ++ docs/DEPLOYMENT.md | 14 +++++- orchestrator/accounts/tests/test_auth.py | 45 ++++++++++++++++++++ orchestrator/audit/log.py | 27 +++++++++--- orchestrator/devicevault/settings.py | 10 +++++ 5 files changed, 93 insertions(+), 7 deletions(-) diff --git a/deploy/orchestrator/orchestrator.env.example b/deploy/orchestrator/orchestrator.env.example index 79c619d..2cae1f4 100644 --- a/deploy/orchestrator/orchestrator.env.example +++ b/deploy/orchestrator/orchestrator.env.example @@ -9,6 +9,10 @@ DV_DEBUG=false DV_ALLOWED_HOSTS=devicevault.example.com # Full origins, required when the UI is served over HTTPS by a front proxy DV_CSRF_TRUSTED_ORIGINS=https://devicevault.example.com +# Address (or network, comma separated) of the TLS proxy or load balancer in front of the +# web service. Without it every user appears to come from the proxy: one client's failed +# sign-ins lock everyone out, and API tokens' allowed networks check the proxy's address. +DV_TRUSTED_PROXIES= DV_TIMEZONE=Australia/Sydney DV_LOG_LEVEL=INFO diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index f5e482a..7fe09ea 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -79,6 +79,16 @@ not exist, the bootstrap admin from `DV_ADMIN_USERNAME` / `DV_ADMIN_PASSWORD` as a member of it. Put a TLS-terminating proxy or load balancer in front of `web`, and set `DV_ALLOWED_HOSTS` and `DV_CSRF_TRUSTED_ORIGINS` to match. +Set `DV_TRUSTED_PROXIES` to that proxy's address (or its network, comma separated +for several). The `web` service records the address that connected to it, which +is then the proxy rather than the user; with the proxy listed, the orchestrator +takes the address the proxy reports instead. Without it every user appears to come +from the proxy: sign-in lockout by address then locks everyone out together, an API +token's allowed networks are compared with the proxy, and the audit log shows only +the proxy. The proxy must append to (or replace) `X-Forwarded-For`, as nginx, +HAProxy (`option forwardfor`), Traefik and cloud load balancers do. Never list an +address that clients could connect from themselves. + ### Users and access Sign-in uses a server-side session: an HttpOnly cookie (`Secure` unless @@ -86,7 +96,8 @@ Sign-in uses a server-side session: an HttpOnly cookie (`Secure` unless orchestrator node shares it. Sessions end after `DV_SESSION_IDLE_MIN` without use and `DV_SESSION_MAX_HOURS` after sign-in. Repeated failed sign-ins lock out a username or client address for `DV_LOGIN_LOCKOUT_MIN`; the client address is the -last `X-Forwarded-For` hop, which the `web` service adds. +last `X-Forwarded-For` hop that is not one of `DV_TRUSTED_PROXIES` (with none set, +the hop the `web` service adds). **API tokens** let scripts and monitoring call the API without a session: send `Authorization: Bearer dvt_...`. A token acts as its owner and never beyond their @@ -722,6 +733,7 @@ secrets). The file takes precedence. | `DV_DEBUG` | `false` | Development mode; never in production | | `DV_ALLOWED_HOSTS` | none (`*` in debug) | Hostnames the UI is served on | | `DV_CSRF_TRUSTED_ORIGINS` | none | Full origins (`https://host`) behind HTTPS | +| `DV_TRUSTED_PROXIES` | none | Addresses or networks of the proxies in front of `web`; the client address is the last `X-Forwarded-For` hop that is not one of them | | `DV_TIMEZONE` | `UTC` | Display timezone (storage is always UTC) | | `DV_LOG_LEVEL` | `INFO` | | | `DV_DB_ENGINE` | `sqlite` | `sqlite` or `postgres` | diff --git a/orchestrator/accounts/tests/test_auth.py b/orchestrator/accounts/tests/test_auth.py index 756743f..5bc1388 100644 --- a/orchestrator/accounts/tests/test_auth.py +++ b/orchestrator/accounts/tests/test_auth.py @@ -137,6 +137,51 @@ def test_the_client_address_comes_from_the_proxy_hop(alice): assert AuditLog.objects.get(action='auth.login').ip == '203.0.113.5' +def _address(settings, forwarded, proxies=()): + import ipaddress + + from django.test import RequestFactory + + from audit.log import client_ip + + settings.DEVICEVAULT_TRUSTED_PROXIES = tuple(ipaddress.ip_network(proxy) for proxy in proxies) + return client_ip(RequestFactory().get('/', REMOTE_ADDR='172.18.0.5', HTTP_X_FORWARDED_FOR=forwarded)) + + +@pytest.mark.parametrize( + ('forwarded', 'proxies', 'expected'), + [ + # A TLS proxy in front of nginx: the client is the hop the proxy added, not the proxy. + ('198.51.100.7, 10.0.0.2', ['10.0.0.0/24'], '198.51.100.7'), + # What the client sent itself (left of the proxy's hop) is never believed. + ('192.0.2.99, 198.51.100.7, 10.0.0.2', ['10.0.0.0/24'], '198.51.100.7'), + # Two proxies in a row, IPv4 and IPv6. + ('198.51.100.7, 2001:db8::1, 10.0.0.2', ['10.0.0.2/32', '2001:db8::/64'], '198.51.100.7'), + ('198.51.100.7, ::ffff:10.0.0.2', ['10.0.0.0/24'], '198.51.100.7'), + # A request from the proxy itself (a health check). + ('10.0.0.2', ['10.0.0.0/24'], '10.0.0.2'), + # A client pretending to be a proxy gains nothing: its own hop is still what the proxy wrote. + ('10.0.0.9, 198.51.100.7, 10.0.0.2', ['10.0.0.0/24'], '198.51.100.7'), + ('junk, 10.0.0.2', ['10.0.0.0/24'], None), + ('198.51.100.7, 10.0.0.2', [], '10.0.0.2'), + ], +) +def test_trusted_proxies_are_skipped_to_find_the_client(settings, forwarded, proxies, expected): + assert _address(settings, forwarded, proxies) == expected + + +def test_behind_a_trusted_proxy_one_client_cannot_lock_everyone_out(alice, settings): + import ipaddress + + settings.DEVICEVAULT_LOGIN_MAX_FAILURES = 2 + settings.DEVICEVAULT_TRUSTED_PROXIES = (ipaddress.ip_network('10.0.0.2/32'),) + for n in range(3): + sign_in(APIClient(HTTP_X_FORWARDED_FOR='198.51.100.7, 10.0.0.2'), username=f'guess-{n}', password='nope') + + assert sign_in(APIClient(HTTP_X_FORWARDED_FOR='198.51.100.7, 10.0.0.2')).status_code == 429 + assert sign_in(APIClient(HTTP_X_FORWARDED_FOR='203.0.113.5, 10.0.0.2')).status_code == 200 + + def test_disabled_users_cannot_sign_in_and_lose_their_session(alice, admin_client): client = APIClient() sign_in(client) diff --git a/orchestrator/audit/log.py b/orchestrator/audit/log.py index b2be3a7..694ba98 100644 --- a/orchestrator/audit/log.py +++ b/orchestrator/audit/log.py @@ -3,19 +3,34 @@ import ipaddress import logging +from django.conf import settings + from audit.models import AuditLog logger = logging.getLogger('devicevault.audit') def client_ip(request) -> str | None: - """The client's address: the last X-Forwarded-For hop (added by our nginx), else the peer.""" + """The client's address, from X-Forwarded-For (our nginx appends the peer it saw), else the peer. + + Read from the right, skipping ``DV_TRUSTED_PROXIES`` (a TLS proxy or load balancer in front of + nginx): the first hop that is not one of them is the client, since everything to its left is + whatever the client chose to send. With no trusted proxies that is the hop nginx added. + """ forwarded = request.META.get('HTTP_X_FORWARDED_FOR', '') - address = forwarded.split(',')[-1].strip() if forwarded else request.META.get('REMOTE_ADDR', '') - try: - return str(ipaddress.ip_address(address)) - except ValueError: - return None + hops = [hop.strip() for hop in forwarded.split(',')] if forwarded else [request.META.get('REMOTE_ADDR', '')] + address = None + for hop in reversed(hops): + try: + address = ipaddress.ip_address(hop) + except ValueError: + return None # a hop a trusted proxy would never write: nothing left of here can be believed + if address.version == 6 and address.ipv4_mapped: + address = address.ipv4_mapped + if not any(address in network for network in settings.DEVICEVAULT_TRUSTED_PROXIES): + break + # Every hop a trusted proxy: the request came from the outermost one itself. + return str(address) if address is not None else None def _token_prefix(request) -> str: diff --git a/orchestrator/devicevault/settings.py b/orchestrator/devicevault/settings.py index 66be930..b06180e 100644 --- a/orchestrator/devicevault/settings.py +++ b/orchestrator/devicevault/settings.py @@ -17,6 +17,7 @@ # All configuration comes from DV_* environment variables; each also accepts # _FILE for Docker secrets. See deploy/orchestrator/orchestrator.env.example. +import ipaddress from pathlib import Path from devicevault_protocol.bus import BusSettings @@ -134,6 +135,15 @@ SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https') USE_X_FORWARDED_HOST = True +# Proxies in front of the web tier (addresses or networks). The client address is the last +# X-Forwarded-For hop that is not one of them; with none, it is the hop the web tier added. +try: + DEVICEVAULT_TRUSTED_PROXIES = tuple( + ipaddress.ip_network(entry, strict=False) for entry in env_list('DV_TRUSTED_PROXIES') + ) +except ValueError as exc: + raise ConfigError(f'DV_TRUSTED_PROXIES must list addresses or networks: {exc}') from exc + # Server-side sessions in the database (shared by every orchestrator node), used # by the UI and the API alike. See accounts.sessions for the timeouts. SESSION_ENGINE = 'django.contrib.sessions.backends.db' From 354287d29eb4989305341054f4232ffbedf227cf Mon Sep 17 00:00:00 2001 From: Michael Junek Date: Sun, 15 Nov 2026 15:19:07 +1100 Subject: [PATCH 2/2] fix(worker): pin device SSH host keys on first use Collectors accepted any SSH host key, so a host answering in a device's place received its username, password and enable secret. A device's host key is now pinned at its first collected backup: the collector reports the key it saw in its signed collected event, and the orchestrator stores it on the device if none is pinned and the event comes from a collector of the device's own collection group. Later collect tasks carry the pinned key, and the collector refuses any other key before authenticating, as a permanent failure with reason auth. The device page shows the key's type and fingerprint. An administrator of the device's group can forget it when a device is replaced or re-keyed (POST devices/{id}/forget_host_key/). Pinning and forgetting are audited. --- CLAUDE.md | 3 +- docs/ARCHITECTURE.md | 2 + docs/DEPLOYMENT.md | 17 ++++ frontend/src/api/endpoints.ts | 2 + frontend/src/api/types.ts | 5 ++ frontend/src/pages/EditDevicePage.vue | 58 ++++++++++++ orchestrator/api/serializers.py | 6 ++ orchestrator/api/tests/test_devices.py | 40 +++++++++ orchestrator/api/views.py | 22 +++++ orchestrator/backups/jobs.py | 64 ++++++++++++- orchestrator/backups/tests/test_jobs.py | 76 +++++++++++++++- .../migrations/0008_device_ssh_host_key.py | 23 +++++ orchestrator/devices/models.py | 15 ++++ protocol/src/devicevault_protocol/jobs.py | 29 +++++- worker/src/devicevault_worker/collect.py | 3 +- worker/src/devicevault_worker/drivers/base.py | 6 ++ worker/src/devicevault_worker/drivers/ssh.py | 77 +++++++++++++++- worker/tests/test_drivers.py | 90 ++++++++++++++++++- 18 files changed, 526 insertions(+), 12 deletions(-) create mode 100644 orchestrator/devices/migrations/0008_device_ssh_host_key.py diff --git a/CLAUDE.md b/CLAUDE.md index 097ba69..8d3a185 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -99,7 +99,8 @@ phased delivery plan; new work should move the code toward it, not entrench the - Collector drivers subclass `CollectorDriver` (`worker/src/devicevault_worker/drivers/base.py`) with a pydantic `Params` model, and raise `TransientError` (retried) or `PermanentError`. SSH CLI platforms subclass `drivers.ssh.NetmikoDriver` (a Netmiko `device_type`, the - commands, and regexes for volatile lines); see `drivers/cisco.py`. Built-ins are listed in + commands, and regexes for volatile lines); see `drivers/cisco.py`. A driver that connects over SSH must + require `target.host_key` when it is set and report the key it saw in `log.host_key` (pinned on first use). Built-ins are listed in `drivers/__init__.py`; third-party drivers register under the `devicevault.collectors` entry point. Storage drivers subclass `storage.drivers.base.StorageDriver` (see `storage/drivers/local.py`, `s3.py`), stream content (`store` reads a binary file, `read` writes one; backups can be hundreds of MB) and register under diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 98d95b0..0e9d790 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -101,6 +101,8 @@ Message formats are in `protocol/src/devicevault_protocol/registration.py`. **Trust on first use:** the worker pins the orchestrator key from the first *approved* assignment that verifies against it, so the trust decision coincides with the admin's approval. After that, assignments signed with any other key are rejected, and so is every task (`device.collect`, `storage.store`, `storage.read`) not signed by the pinned key. Tasks rejected this way fail immediately and are not retried. The Workers page shows the orchestrator's fingerprint and flags a worker that pinned a different one. Changing `DV_SIGNING_KEY` means resetting every worker's state. +**Device host keys** follow the same model. A device's SSH host key is pinned at its first collected backup: the collector reports the key the device presented in its signed `collected` event, and the orchestrator stores it on the device if it has none, accepting it only from a collector of the device's own collection group. Every later `job.collect` task carries the pinned key in its signed payload, and the collector refuses any other key before authenticating (a permanent failure, reason `auth`). An administrator of the device's group forgets the key when the device is replaced or re-keyed. Collectors keep no known-hosts state of their own. + **Limitations:** - Broker credentials are shared per role, so revocation is enforced cryptographically (signatures and keys), not at the broker. Per-worker broker users can be added later. - Celery remote control stays enabled because Flower needs it, so anyone holding broker credentials can still send Celery control commands (for example, to stop a worker consuming). They cannot forge assignments or tasks. diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 7fe09ea..dbfffa8 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -210,6 +210,23 @@ log line matches. To automate approval: `manage.py approve_worker - **Revoke** stops a worker consuming. **Forget** removes it; if it is still running it registers again as pending. +### Device SSH host keys + +Collectors pin each device's SSH host key on first use. A device's first backup +over SSH accepts the key the device presents; the orchestrator records it on the +device (audited as `device.host_key_pin`, and noted in that backup's log) and sends +it with every later backup. From then on a collector connects only if the device +presents exactly that key, before any username or password is sent, so a host +answering in the device's place cannot collect its credentials. A backup refused +this way fails at once with the reason `auth` and is not retried. + +The device's page shows the pinned key's type and `SHA256:` fingerprint; compare it +with the device's own (`show crypto key mypubkey` and the like) when the first +backup is not made over a network you trust. When a device is replaced or its keys +are regenerated, an administrator of its device group chooses **Forget host key** +there (audited as `device.host_key_forget`), and the next backup pins the new key. +Only a collector of the device's own collection group can pin a key. + ### Storage targets Under **Admin Settings > Storage Targets**, create a target per place backups go: diff --git a/frontend/src/api/endpoints.ts b/frontend/src/api/endpoints.ts index 7c7887f..77f3184 100644 --- a/frontend/src/api/endpoints.ts +++ b/frontend/src/api/endpoints.ts @@ -164,6 +164,8 @@ export const devicesApi = { deactivate: async (id: Id, reason: string) => (await http.post(`/devices/${id}/deactivate/`, { reason })).data, reactivate: async (id: Id) => (await http.post(`/devices/${id}/reactivate/`)).data, + /** Drop the pinned SSH host key (a replaced or re-keyed device); the next backup pins the new one. */ + forgetHostKey: async (id: Id) => (await http.post(`/devices/${id}/forget_host_key/`)).data, /** Global administrators: unpins and deletes every backup of an inactive device. `confirm` is its name. */ purge: async (id: Id, confirm: string) => (await http.post(`/devices/${id}/purge/`, { confirm })).data, diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 75d0445..ec9e9db 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -225,6 +225,11 @@ interface DeviceBase { storage_target: string | null credential: Id | null schedule: Id | null + /** The SSH host key pinned at the device's first backup, which collectors then require; '' while none is. */ + ssh_host_key_type: string + /** `SHA256:...`, as the device and `ssh-keygen -l` show it. */ + ssh_host_key_fingerprint: string + ssh_host_key_pinned_at: IsoDateTime | null /** What the signed-in user holds on this device's group. */ user_permissions: GrantFlag[] inactive_at: IsoDateTime | null diff --git a/frontend/src/pages/EditDevicePage.vue b/frontend/src/pages/EditDevicePage.vue index d82653d..c06a3c5 100644 --- a/frontend/src/pages/EditDevicePage.vue +++ b/frontend/src/pages/EditDevicePage.vue @@ -28,6 +28,33 @@ {{ device.last_backup_time ? formatDateTime(device.last_backup_time) : 'Never' }} +
+
SSH host key
+ +
+ None pinned yet. The key the device presents at its first backup over SSH is pinned and required from + then on. +
+
@@ -211,6 +238,7 @@