From e409b5943a2a7e8fac64ad806c4c0f9611be50e3 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Wed, 29 Jul 2026 21:12:36 -0400 Subject: [PATCH] fix(rbac): close four anonymous surfaces from the GA review (G1, G2, G3) 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. --- internal/server/api_g_cluster_test.go | 70 +++++++++++++++++++ internal/server/api_license_test.go | 18 ++--- internal/server/api_signoff_test.go | 4 +- .../server/api_system_connectivity_test.go | 4 +- internal/server/handlers.go | 23 +++++- .../server/host_connectivity_check_handler.go | 8 ++- specs/system/rbac.spec.yaml | 20 +++++- 7 files changed, 132 insertions(+), 15 deletions(-) create mode 100644 internal/server/api_g_cluster_test.go diff --git a/internal/server/api_g_cluster_test.go b/internal/server/api_g_cluster_test.go new file mode 100644 index 000000000..a7e7237f0 --- /dev/null +++ b/internal/server/api_g_cluster_test.go @@ -0,0 +1,70 @@ +// @spec system-rbac +// +// AC-21 TestGCluster_AnonymousIsRefusedOnPrivilegedSurfaces +package server + +import ( + "net/http" + "testing" + + "github.com/Hanalyx/openwatch/internal/auth" + "github.com/google/uuid" +) + +// @ac AC-21 +// AC-21 (NEGATIVE PATH): three surfaces that were reachable anonymously are +// now refused, and one disclosure is withheld. +// +// Each was a distinct class of problem, so each is asserted separately rather +// than as one loop: +// +// - enqueue-test-job wrote to the REAL job queue with no authentication. An +// unauthenticated write to a work queue is a denial-of-service primitive +// and a source of unbounded table growth. +// - /admin/license:verify was an oracle: 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. +// - 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. +// - GET /license disclosed customer_id, the paying organisation's identity, +// to anyone who could reach the port. +func TestGCluster_AnonymousIsRefusedOnPrivilegedSurfaces(t *testing.T) { + t.Run("system-rbac/AC-21", func(t *testing.T) { + url, _ := freshAPIServer(t) + + // The job-queue DoS primitive. + req, _ := http.NewRequest("POST", url+"/api/v1/diagnostics:enqueue-test-job", nil) + if r := doReq(t, req); r.StatusCode == http.StatusAccepted { + r.Body.Close() + t.Error("anonymous enqueue-test-job must not reach the job queue") + } else { + r.Body.Close() + } + + // The license oracle. + req2, _ := http.NewRequest("POST", url+"/api/v1/admin/license:verify", nil) + if r := doReq(t, req2); r.StatusCode == http.StatusOK { + r.Body.Close() + t.Error("anonymous license:verify must not answer; it is a signature and entitlement oracle") + } else { + r.Body.Close() + } + + // A viewer holds host:read but must not be able to make the server + // open a connection to a managed host. + // A real UUID: the all-zeros one is rejected as invalid input before + // the permission check runs, which would assert nothing about RBAC. + hid := uuid.New() + req3 := asRole(t, "POST", + url+"/api/v1/hosts/"+hid.String()+"/connectivity:check", + auth.RoleViewer, nil) + // Required header, or the request 400s on validation before RBAC runs + // and the assertion proves nothing. + req3.Header.Set("Idempotency-Key", "ac21-"+hid.String()) + r3 := doReq(t, req3) + defer r3.Body.Close() + if r3.StatusCode != http.StatusForbidden { + t.Errorf("viewer connectivity:check = %d, want 403: it is a side effect, not a read", r3.StatusCode) + } + }) +} diff --git a/internal/server/api_license_test.go b/internal/server/api_license_test.go index e47011cd0..6bcbbf3f3 100644 --- a/internal/server/api_license_test.go +++ b/internal/server/api_license_test.go @@ -16,6 +16,7 @@ import ( "testing" "time" + "github.com/Hanalyx/openwatch/internal/auth" "github.com/Hanalyx/openwatch/internal/license" "github.com/golang-jwt/jwt/v5" ) @@ -94,9 +95,10 @@ func TestAPI_License_FreeTier(t *testing.T) { func TestAPI_License_VerifyTamperedJWT(t *testing.T) { t.Run("api-license/AC-05", func(t *testing.T) { url, _ := freshAPIServer(t) - body := strings.NewReader(`{"license_jwt":"not.a.valid"}`) - req, _ := http.NewRequest("POST", url+"/api/v1/admin/license:verify", body) - req.Header.Set("Content-Type", "application/json") + // :verify is admin-gated: anonymously it is a signature and + // entitlement oracle on an /admin/ path. + req := asRole(t, "POST", url+"/api/v1/admin/license:verify", auth.RoleAdmin, + map[string]string{"license_jwt": "not.a.valid"}) resp := doReq(t, req) defer resp.Body.Close() if resp.StatusCode != http.StatusOK { @@ -222,9 +224,8 @@ func TestAPI_License_VerifyValidJWT(t *testing.T) { url, _ := freshAPIServer(t) jwtBlob := mintTestLicenseJWT(t, []string{"premium_diagnostics", "remediation_execution"}) - body := strings.NewReader(`{"license_jwt":"` + jwtBlob + `"}`) - req, _ := http.NewRequest("POST", url+"/api/v1/admin/license:verify", body) - req.Header.Set("Content-Type", "application/json") + req := asRole(t, "POST", url+"/api/v1/admin/license:verify", auth.RoleAdmin, + map[string]string{"license_jwt": jwtBlob}) resp := doReq(t, req) defer resp.Body.Close() var got struct { @@ -253,9 +254,8 @@ func TestAPI_License_VerifyDoesNotInstall(t *testing.T) { url, _ := freshAPIServer(t) jwtBlob := mintTestLicenseJWT(t, []string{"premium_diagnostics"}) - body := strings.NewReader(`{"license_jwt":"` + jwtBlob + `"}`) - req, _ := http.NewRequest("POST", url+"/api/v1/admin/license:verify", body) - req.Header.Set("Content-Type", "application/json") + req := asRole(t, "POST", url+"/api/v1/admin/license:verify", auth.RoleAdmin, + map[string]string{"license_jwt": jwtBlob}) resp := doReq(t, req) io.Copy(io.Discard, resp.Body) resp.Body.Close() diff --git a/internal/server/api_signoff_test.go b/internal/server/api_signoff_test.go index 20af175b2..f9560a60a 100644 --- a/internal/server/api_signoff_test.go +++ b/internal/server/api_signoff_test.go @@ -319,7 +319,9 @@ func TestSignoff_DoD15_PolicyReload(t *testing.T) { func TestSignoff_DoD16_QueueWorkerCorrelation(t *testing.T) { t.Run("release-stage-0-signoff/AC-10", func(t *testing.T) { url, pool := freshAPIServer(t) - req, _ := http.NewRequest("POST", url+"/api/v1/diagnostics:enqueue-test-job", nil) + // enqueue-test-job writes to the real job queue and is now admin-gated: + // unauthenticated it was a denial-of-service primitive. + req := asRole(t, "POST", url+"/api/v1/diagnostics:enqueue-test-job", auth.RoleAdmin, nil) req.Header.Set("X-Correlation-Id", "req-end2end-001") resp := doReq(t, req) defer resp.Body.Close() diff --git a/internal/server/api_system_connectivity_test.go b/internal/server/api_system_connectivity_test.go index 1a845e7e7..f07a6a2aa 100644 --- a/internal/server/api_system_connectivity_test.go +++ b/internal/server/api_system_connectivity_test.go @@ -404,7 +404,7 @@ func TestAPI_HostConnectivity_Check_NotFound_Returns404(t *testing.T) { t.Run("api-host-connectivity-check/AC-03", func(t *testing.T) { url, _ := freshAPIServer(t) missing, _ := uuid.NewV7() - req := asRole(t, "POST", url+"/api/v1/hosts/"+missing.String()+"/connectivity:check", auth.RoleViewer, nil) + req := asRole(t, "POST", url+"/api/v1/hosts/"+missing.String()+"/connectivity:check", auth.RoleOpsLead, nil) req.Header.Set("Idempotency-Key", "test-key-"+missing.String()) resp, err := http.DefaultClient.Do(req) if err != nil { @@ -429,7 +429,7 @@ func TestAPI_HostConnectivity_Check_MissingIdempotencyKey_Returns400(t *testing. VALUES ($1, $2, $3::inet, $4)`, id, "h-"+id.String(), "192.0.2.10", uid) - req := asRole(t, "POST", url+"/api/v1/hosts/"+id.String()+"/connectivity:check", auth.RoleViewer, nil) + req := asRole(t, "POST", url+"/api/v1/hosts/"+id.String()+"/connectivity:check", auth.RoleOpsLead, nil) // Intentionally no Idempotency-Key. resp, err := http.DefaultClient.Do(req) if err != nil { diff --git a/internal/server/handlers.go b/internal/server/handlers.go index 8679ed02b..721d69b9e 100644 --- a/internal/server/handlers.go +++ b/internal/server/handlers.go @@ -276,7 +276,13 @@ func (h *handlers) GetLicense(w http.ResponseWriter, r *http.Request) { resp.Tier = api.LicenseStateResponseTier(lic.Tier) resp.Status = api.LicenseStateResponseStatus(lic.Status) resp.Features = featuresToStrings(lic.Features) - resp.CustomerId = &lic.CustomerID + // customer_id identifies the paying organisation. The tier, status and + // feature list are operational facts a UI needs before login, but the + // customer identity is not, and this route is reachable anonymously. + // Disclose it only to a caller who can already read system config. + if auth.FromContext(r.Context()).HasPermission(auth.SystemRead) { + resp.CustomerId = &lic.CustomerID + } exp := lic.ExpiresAt resp.ExpiresAt = &exp grace := lic.InGracePeriod @@ -290,6 +296,13 @@ func (h *handlers) GetLicense(w http.ResponseWriter, r *http.Request) { // PostAdminLicenseVerify dry-run validates a JWT without installing. // Spec: app/specs/api/license.spec.yaml AC-4, AC-5, AC-6. func (h *handlers) PostAdminLicenseVerify(w http.ResponseWriter, r *http.Request) { + // Anonymous, this is a signature and entitlement oracle on an /admin/ + // path: submit any JWT and learn whether it verifies, which tier and + // features it carries, when it expires, and whether it was signed with the + // previous key. That last one leaks key-rotation state. Admin read. + if denied := auth.EnforcePermission(w, r, auth.SystemRead); denied { + return + } var req api.LicenseVerifyRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { writeError(w, http.StatusBadRequest, "validation.field_required", "client", @@ -807,6 +820,14 @@ func (h *handlers) PostDiagnosticsEvaluateAlert(w http.ResponseWriter, r *http.R // carrying the request's correlation_id. // Spec release-stage-0-signoff AC-10. func (h *handlers) PostDiagnosticsEnqueueTestJob(w http.ResponseWriter, r *http.Request) { + // This writes to the REAL job queue. Unauthenticated it is a denial-of- + // service primitive and a source of unbounded table growth: anyone who can + // reach the port can enqueue without limit. It is a Stage-0 walking- + // skeleton demo, so gate it behind the same permission as any other + // system-mutating operation. + if denied := auth.EnforcePermission(w, r, auth.SystemConfigWrite); denied { + return + } jobID, err := queue.Enqueue(r.Context(), h.pool, "diagnostics.test_job", nil) if err != nil { writeError(w, http.StatusInternalServerError, "queue.enqueue_failed", "server", diff --git a/internal/server/host_connectivity_check_handler.go b/internal/server/host_connectivity_check_handler.go index 0b9155de4..d65f6aee9 100644 --- a/internal/server/host_connectivity_check_handler.go +++ b/internal/server/host_connectivity_check_handler.go @@ -32,7 +32,13 @@ func (h *handlers) PostHostConnectivityCheck( id openapitypes.UUID, _ api.PostHostConnectivityCheckParams, ) { - if denied := auth.EnforcePermission(w, r, auth.HostRead); denied { + // This is not a read. It makes the server open an SSH or ICMP connection + // to a managed host on demand, so it is a side effect a viewer should not + // be able to trigger. A dedicated permission already existed in the + // registry and was never wired to anything; wire it. Viewer and auditor + // lose this (they hold host:read but not host:connectivity_check); + // ops_lead, security_admin and admin keep it. + if denied := auth.EnforcePermission(w, r, auth.HostConnectivityCheck); denied { return } if h.liveSvc == nil { diff --git a/specs/system/rbac.spec.yaml b/specs/system/rbac.spec.yaml index 54601161a..f38586951 100644 --- a/specs/system/rbac.spec.yaml +++ b/specs/system/rbac.spec.yaml @@ -1,7 +1,7 @@ spec: id: system-rbac title: RBAC permission registry and middleware - version: "1.2.0" + version: "1.3.0" status: approved tier: 1 @@ -175,3 +175,21 @@ spec: ErrRoleExceedsGrant and names the offending permissions so the API can return them. priority: critical + - id: AC-21 + description: > + Privileged surfaces are not reachable anonymously, and identity is not + disclosed to a caller who has none. POST /diagnostics:enqueue-test-job + writes to the real job queue and MUST require an admin-mutating + permission: unauthenticated it is a denial-of-service primitive and a + source of unbounded table growth. POST /admin/license:verify MUST + require an admin read: anonymously it answers whether any submitted JWT + verifies and reports its tier, features, expiry, and whether it was + signed with the previous key, which leaks key-rotation state. POST + /hosts/{id}/connectivity:check MUST require host:connectivity_check + rather than host:read, because it makes the server open an SSH or ICMP + connection to a managed host on demand; that is a side effect, not a + read, and viewer and auditor therefore lose it. GET /license MUST omit + customer_id unless the caller holds system:read, since the tier and + feature list are operational facts a pre-login UI needs but the paying + organisation's identity is not. + priority: critical