feat/v0-3-dashboard-role-enforcement #293

johnride · 2026-05-25T12:23:32Z

johnride commented

2026-05-25 12:23:32 +00:00

No description provided.

johnride added 2 commits 2026-05-25 12:23:33 +00:00

feat(fleet-operator): require fleet-admin role on dashboard routes aad14cd04d

Previously the dashboard granted full access to any Zitadel user with
a valid JWT — issuer, audience and signature were the only checks.
Operators routinely give non-admin users an account in Zitadel for SSO
into adjacent apps, so a valid JWT is not by itself evidence the user
should manage a fleet. This change closes that gap.

Behaviour:
- Verified sessions now carry a `roles: Vec<String>` extracted from a
  configurable JWT claim (env `FLEET_AUTH_ROLES_CLAIM`, default the
  Zitadel role URN). The extractor accepts both OIDC array shape and
  Zitadel's object-map shape; missing/malformed claim yields empty
  vec (closed door, never wildcard).
- `require_fleet_admin` middleware layers above `require_auth` on the
  private routes. A logged-in user without the role gets a human-
  readable 403 page that names the missing role and offers a sign-out
  link.
- `/logout` is in `public_routes` so a non-admin can switch accounts
  without being trapped by the gate that rendered the 403.

Shared role extraction:
- `harmony_zitadel_auth::roles::extract_roles` is the single source
  of truth for Zitadel role parsing.
- The NATS auth callout's `ZitadelValidator::extract_roles` now
  delegates to it. Any future quirk in Zitadel's role-emission shape
  is fixed once.

Tests:
- 9 unit tests on the role extractor: both claim shapes, empty/missing
  fail closed, non-string array entries skipped, dotted paths,
  Zitadel URN paths.
- 5 axum middleware tests via `tower::ServiceExt::oneshot`: no
  session → unauthenticated redirect, role-less session → 403 with
  expected role name + logout link, fleet-admin session → handler
  reached, HTML (not JSON) content-type on 403.

Defers to follow-ups: a `fleet-viewer` read-only role; unifying the
dashboard role policy with the NATS callout's per-device scoping.

refactor(zitadel-auth): typed Role/Roles, drop string-path role lookup

Run Check Script / check (pull_request) Successful in 2m21s

Details

e9522464af

Removes the brittle pieces that landed in the original role-enforcement
commit: `extract_roles(value, path)` walking `serde_json::Value` with
a `path.contains("urn:")`-aware splitter, an env-driven
`FLEET_AUTH_ROLES_CLAIM`, and `VerifiedSession.roles: Vec<String>`
compared with `iter().any(|r| r == "fleet-admin")` in middleware.

This is a security-relevant code path; runtime string heuristics inside
it hide injection-shaped bugs (a misconfigured env or a custom Zitadel
mapper shifts the lookup to a path the attacker controls).

Replaced with end-to-end typed serde decoding:

* `Role` — closed enum (one variant today: `FleetAdmin`). Adding a
  variant is a deliberate code change to the security path. Unknown
  role names emitted by the IdP cannot be represented and therefore
  cannot grant access.
* `RoleClaims` — wire-side struct, flattened into the JWT `Claims`.
  The two well-known role-claim locations are matched verbatim by
  `#[serde(rename = "urn:zitadel:iam:org:project:roles")]` and
  `#[serde(rename = "roles")]`. No dotted-path navigation. No env
  string. If a future issuer adds a third location it is an additive
  `#[serde(rename = ...)]` field inside the security boundary.
* `Roles` — domain value on `VerifiedSession`. Construction is
  restricted to `RoleClaims::into_roles` plus a typed
  `FromIterator<Role>` for test fixtures. `Roles::has(Role::FleetAdmin)`
  is the only check the middleware needs; no string comparison exists
  anywhere downstream.
* Malformed shapes (scalar at the URN path, mixed-type array) now
  ERROR at decode rather than degrading to an empty-vec "closed door".
  Fail-loud is the security-correct default when the IdP misbehaves
  — the user re-logs in, the operator notices.

Callout side: reverted the shared-extract_roles delegation. The
callout retains its own, unchanged role-extraction logic. We do NOT
need cross-crate sharing here, and the shared extractor was the entry
point we were trying to delete — the callout's own behaviour was the
status quo and is preserved verbatim.

Dropped exports: `DEFAULT_ADMIN_ROLE`, `DEFAULT_ROLES_CLAIM`,
`extract_roles`, `ROLES_CLAIM_ENV`, `ZitadelAuthConfig.roles_claim`.
None had external consumers in the workspace.

Tests:
* 12 tests on `RoleClaims` deserialization: both shapes resolve,
  Zitadel URN wins precedence, unknown roles dropped, malformed
  scalar/mixed-type arrays error at decode, missing claim → empty,
  empty object/array → empty, extra unrelated claims are ignored,
  display matches wire spelling.
* 4 middleware tests on the typed `require_role(Role::FleetAdmin, …)`
  path. Dropped the redundant "admin-only vs admin+other" test —
  with a single-variant enum it duplicated the positive case.

reda commented

2026-05-25 14:01:35 +00:00

The current callout get the role claim location and role names from env variable but the new dashboard code has them hardcoded. Is that intentional? If deployments can customize those values dashboard auth and NATS auth can disagree if I'm not mistaken

Run Check Script / check (pull_request) Successful in 2m21s

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feat/v0-3-dashboard-role-enforcement:feat/v0-3-dashboard-role-enforcement

git checkout feat/v0-3-dashboard-role-enforcement

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#293