fix(rbac): close four anonymous surfaces from the GA review (G1, G2, G3) - #768
Merged
Conversation
Four findings, each a different class, all reachable without credentials. enqueue-test-job wrote to the REAL job queue with no authentication (handlers.go, queue.Enqueue with no check above it). Anyone who could reach the port could enqueue without limit: a denial-of-service primitive and a source of unbounded table growth. Now requires system:config_write. /admin/license:verify answered anonymously on an /admin/ path. Submit any JWT and learn whether it verifies, its tier, its features, its expiry, and whether it was signed with the PREVIOUS key. That last one leaks key-rotation state. Now requires system:read. connectivity:check gated on host:read, but it is not a read: it makes the server open an SSH or ICMP connection to a managed host on demand. A dedicated host:connectivity_check permission already existed in the registry and was wired to nothing. Wired now. This is a deliberate behaviour change: viewer and auditor hold host:read but not host:connectivity_check, so they lose it; ops_lead, security_admin and admin keep it. GET /license disclosed customer_id, the paying organisation's identity, to any anonymous caller. Tier, status and features stay anonymous because a pre-login UI needs them; the identity does not qualify. While scoping this I measured the wider contract gap: 44 mutating routes carry no x-required-permission declaration. 30 of them DO enforce, so that is a contract accuracy problem rather than a hole, and 11 of the remaining 14 are correctly ungated (pre-session auth, or self-service on the caller's own identity). The genuinely exposed ones were the diagnostics routes above. Worth recording that the AC-18 coverage test cannot see any of this: it verifies declared-implies-enforced, and says nothing about routes that never declare. Closing that is a follow-up, and it needs the 30 declarations first. Also worth recording: my ad-hoc scan initially reported license:verify as enforcing, because it followed a helper and matched any EnforcePermission. AC-18 does not have that weakness; it requires the SPECIFIC constant the route declares, so following a helper that enforces something else does not match. Four existing tests asserted the old ungated behaviour and now pass a credentialled caller. Caught locally against the test database from #767 rather than in CI, which is the first time this session that has been true. Spec: system-rbac 1.2.0 -> 1.3.0, AC-21 (negative path). 116 specs, 116 passing. Full go test ./internal/... clean against the local test database.
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
1 participant
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.
Four findings from the GA security review, each a different class, all reachable without credentials.
1. An unauthenticated write to the job queue
POST /diagnostics:enqueue-test-jobcalledqueue.Enqueueagainst the real queue with no check above it. Anyone who could reach the port could enqueue without limit: a denial-of-service primitive and a source of unbounded table growth.Now requires
system:config_write.2. A license oracle on an /admin/ path
POST /admin/license:verifyanswered anonymously. Submit any JWT and learn whether it verifies, its tier, its features, its expiry, and whether it was signed with the previous key — which leaks key-rotation state.Now requires
system:read.3. A side effect gated as a read
POST /hosts/{id}/connectivity:checkgated onhost:read, but it is not a read: it makes the server open an SSH or ICMP connection to a managed host on demand. A dedicatedhost:connectivity_checkpermission already existed in the registry and was wired to nothing.Deliberate behaviour change: viewer and auditor hold
host:readbut nothost:connectivity_check, so they lose this. ops_lead, security_admin and admin keep it.4. Customer identity disclosed anonymously
GET /licensereturnedcustomer_id, the paying organisation's identity, to any anonymous caller. Tier, status and features stay anonymous because a pre-login UI needs them. The identity does not qualify.What I measured while scoping this
44 mutating routes carry no
x-required-permissiondeclaration. Before treating that as alarming: 30 of them do enforce, so it is a contract-accuracy problem rather than a hole, and 11 of the remaining 14 are correctly ungated (pre-session auth, or self-service on the caller's own identity). The genuinely exposed ones were the diagnostics routes above.Worth recording explicitly: the AC-18 coverage test cannot see any of this. It verifies declared-implies-enforced and says nothing about routes that never declare. Closing that blind spot is a follow-up, and it needs the 30 declarations landed first or the test just fails on all of them.
Also: my ad-hoc scan initially reported
license:verifyas enforcing, because it followed a helper and matched anyEnforcePermission. AC-18 does not have that weakness — it requires the specific constant the route declares, so following a helper that enforces something else does not match.Tests
Four existing tests asserted the old ungated behaviour and now pass a credentialled caller. Caught locally against the test database from #767 rather than in CI, which is the first time this session that has been true.
system-rbac1.2.0 -> 1.3.0, AC-21 (negative path). 116 specs, 116 passing. Fullgo test ./internal/...clean locally.