feat(security): author ACL grants from the real tree, not free text (#520 stage 7)
The last stage of #520. Enforcement landed in stages 5-6; this closes the authoring end, where a grant could be saved, deployed and sealed green while resolving to nothing. Scope kind and scope target are now chosen TOGETHER from a picker over the cluster's raw and UNS trees (AclScopePickerModal -> AclScopeTree, backed by IAclScopeService). They were previously an unrelated dropdown and a free-text box, so "RawDevice + a TagId" was a well-formed row. Because the trees are rooted at the grant's own cluster and each node supplies its own kind, NodeAclScopeCrossCluster, NodeAclScopeIdUnexpected, NodeAclScopeIdMissing and NodeAclScopeKindRetired all become unauthorable by construction. CheckScopeAsync mirrors DraftValidator.ValidateNodeAclScopes rule for rule and blocks the save when a scope resolves to nothing - re-checked on submit, not trusted from form load, because a row whose entity was deleted since is exactly where the operator never touches the picker. The list page marks such rows unresolved with the reason. The principal field suggests known principals and reports how many live keys reach the one typed; reaching nobody warns rather than blocks, since authoring ahead of minting is legitimate. Deliberate departures from the plan line, all recorded in the design doc: - The banners are reshaped, not removed. "Not enforced" stopped being true at stage 6, but enforcement is still a per-node flag the AdminUI cannot see, so a narrow grant really does confer nothing on a default-configured node. - A Cluster row produces a NULL ScopeId. The deploy gate rejects one that carries an id, so filling in the cluster id would author undeployable rows. - AclScopeTree is a new component rather than a third mode on RawTree, which already carries the /raw editing tree and the equipment reference picker. - UNS tags are not offered: a UNS signal is a UnsTagReference, and a Tag grant already covers the raw node and every UNS projection of it. Also fixed: the ACL editor had a DataAnnotationsValidator and no ValidationSummary, so an invalid NodeAclId made Create a silent no-op. Tests: 30 in AdminUI.Tests/Acls, falsified with six production mutations - each turned exactly the intended tests red and no others. Live gate PASSED on docker-dev, 11 legs; both central containers asserted on the same image by container id. The pair that matters is legs 8 and 11: the page and the deploy gate reject the same row with the same reason. Legs 5 and 9 are the positive controls - without them the gate passes against a page that refuses everything. Not covered: UNS-realm scope selection (SITE-A has no UNS areas). Rig restored to zero ACL rows. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
@@ -108,7 +108,10 @@ mismatch).
|
||||
`Descend` like any other kind, so given a path it *would* have matched and been reported under
|
||||
whatever level it happened to land on. It was unlabellable, not unplaceable.
|
||||
- `NodePermissions.AlarmRead` has no `OpcUaOperation` mapping — grantable but unreachable.
|
||||
- `NodeAcl.ScopeId` has no FK and no existence check, so scope ids dangle silently.
|
||||
- ~~`NodeAcl.ScopeId` has no FK and no existence check, so scope ids dangle silently.~~ **Closed** —
|
||||
stage 3 gave it a deploy-time existence check, and stage 7 an authoring-time one plus a picker that
|
||||
cannot produce a dangling id. There is still no FK: `ScopeId` names a row in one of eight tables
|
||||
chosen by `ScopeKind`, which is not a foreign key any schema can express.
|
||||
- ~~`ZB.MOM.WW.OtOpcUa.OpcUaServer.csproj` does not reference `Core` at all~~ — **stale**; stage 1 added
|
||||
the reference. The line read as an open item long after it was closed.
|
||||
- `NodePermissions.HistoryUpdate` is deliberately in no bundle until HistoryUpdate exists. Leave it.
|
||||
@@ -169,8 +172,8 @@ Each stage is one commit, builds clean, and leaves the tree shippable.
|
||||
Also closed here: `NodeAcl` rows had **no deploy-time validation of any kind** and were not even
|
||||
reachable from `DraftValidator` (`DraftSnapshot` carried no `NodeAcls` collection). They are now
|
||||
gated for dangling ids, cross-cluster scopes, blank/contradictory `ScopeId`, the retired
|
||||
`FolderSegment` kind, and undefined ordinals. That gate is what substitutes for the ACL scope picker
|
||||
until stage 7 — `ScopeId` is still a free-text box.
|
||||
`FolderSegment` kind, and undefined ordinals. That gate substituted for the ACL scope picker until
|
||||
stage 7 shipped one; it remains the backstop for rows edited outside the AdminUI.
|
||||
4. **Artifact + lifecycle.** ✅ **Done.** `ConfigComposer` snapshots `ApiKeys` alongside the `NodeAcls`
|
||||
it already carried; `DeploymentArtifact.ParseDataPlaneAuthority` decodes both node-side (grants
|
||||
scoped to the node's own cluster, keys not — a key is a fleet-wide credential and the ACL rows are
|
||||
@@ -271,8 +274,37 @@ Each stage is one commit, builds clean, and leaves the tree shippable.
|
||||
the moment anything injects a test clock — at which point the evaluator denies *everything* with a
|
||||
verdict indistinguishable from "no grant matched". Stage 5's claim that these predicates "can never
|
||||
fire" was accidentally true, not true by construction; they are now disabled explicitly.
|
||||
7. **AdminUI.** Key management (create/revoke, show-once) ✅ **done early, in stage 4's predecessor**; ACL authoring against principals, and
|
||||
**remove the two "not enforced" warning banners** — which is the honest signal that this is done.
|
||||
7. **AdminUI.** ✅ **Done.** Key management (create/revoke, show-once) landed early, in stage 4's
|
||||
predecessor. This stage added the **scope picker** (`AclScopePickerModal` over the cluster's raw and
|
||||
UNS trees, backed by `IAclScopeService`), authoring-time **scope and principal resolution**, and
|
||||
reshaped the two banners.
|
||||
|
||||
Four things went differently from this line, all deliberate:
|
||||
|
||||
- **The banners were reshaped, not removed.** "Remove the two warning banners" assumed the only thing
|
||||
they said was *not enforced*, which stage 6 falsified. What remains true is that enforcement is a
|
||||
**per-node** flag the AdminUI cannot see, so an `Equipment`-scoped grant still confers nothing on a
|
||||
default-configured node — deleting the banner would have replaced one wrong impression with
|
||||
another. They are now `alert-info` describing both modes, flag-on first.
|
||||
- **The picker sets the kind AND the id, and there is no longer a hand-edit path.** The plan asked
|
||||
for "a `RawTree` picker for `ScopeId`", which would have left the kind dropdown beside it and kept
|
||||
`RawDevice` + a TagId authorable. Selecting a tree node supplies both, which makes
|
||||
`NodeAclScopeCrossCluster`, `NodeAclScopeIdUnexpected`, `NodeAclScopeIdMissing` and
|
||||
`NodeAclScopeKindRetired` unauthorable by construction rather than merely discouraged.
|
||||
- **It is a new component, not a third mode on `RawTree`.** `RawTree` already carries the `/raw`
|
||||
editing tree and the equipment reference picker. A third mode would put the scope picker's bugs on
|
||||
the device-authoring page, and with no bUnit in this repo the only thing that would catch that is
|
||||
someone noticing. `AclScopeTree` renders a unified `AclScopeNode` instead, which is also what lets
|
||||
ONE component serve both hierarchies — `RawTree` covers only the raw side.
|
||||
- **Saving is blocked on an unresolved scope, and the list page marks the rot.** Not asked for; it is
|
||||
what closes `ScopeId` dangling silently (the open item at the top of this doc) rather than moving
|
||||
the discovery from deploy time to slightly-earlier deploy time. The check re-runs on submit rather
|
||||
than trusting the verdict from when the form loaded, because a row loaded unchanged is exactly the
|
||||
case where the operator never touches the picker.
|
||||
|
||||
Also fixed en route: the ACL editor had a `DataAnnotationsValidator` and **no `ValidationSummary`**,
|
||||
so an invalid `NodeAclId` made the Create button a silent no-op — the [[project_adminui_no_bunit_live_verify]]
|
||||
failure mode, present in this page since it was written.
|
||||
8. **Rollout.** Docs, the ScadaBridge key hand-off, and flipping `Security:Acl:Enabled` on docker-dev
|
||||
as the live gate.
|
||||
|
||||
@@ -350,6 +382,41 @@ Rig left as found: key, ACL row, the second device and its tag all deleted and r
|
||||
returned to `Security:Acl:Enabled` unset, confirmed by the absence of the startup banner and an
|
||||
anonymous read succeeding again.
|
||||
|
||||
## Stage 7 live gate — PASSED (docker-dev, 2026-08-01)
|
||||
|
||||
Driven through the AdminUI at `:9200` against cluster **SITE-A**. Both central containers were asserted
|
||||
to be running the same freshly-built image *by container id*, not from one `docker compose build` — the
|
||||
stale-central-2 trap from the stage-6 gate.
|
||||
|
||||
| # | Leg | Result |
|
||||
|---|---|---|
|
||||
| 1 | Picker lazily expands the real raw tree | ✅ cluster → `sa-modbus2` → `plc` → `SiteAOnly`, one level per expand |
|
||||
| 2 | Each level offers its own scope kind | ✅ `CLUSTER` / `RAWDRIVER` / `RAWDEVICE` / `TAG` chips |
|
||||
| 3 | Selecting a node sets kind **and** id | ✅ → `RAWDEVICE` + `SITEA-dev`, "✓ Device 'plc' in cluster 'SITE-A'" |
|
||||
| 4 | Principal reaching nobody warns | ✅ `not-a-real-principal` → ⚠ "No active API key uses this principal" |
|
||||
| 5 | …and the positive control | ✅ `anonymous` → ✓ "The reserved anonymous principal" |
|
||||
| 6 | Grant saves and lists with its resolved target | ✅ |
|
||||
| 7 | A rotted row is marked on the list | ✅ id broken by SQL → `UNRESOLVED` chip + reason |
|
||||
| 8 | Saving a rotted row is **blocked** | ✅ "Scope: No device in this configuration has the id …" |
|
||||
| 9 | Re-picking a valid scope unblocks it | ✅ `NAMESPACE raw` → saved |
|
||||
| 10 | The authored grant **deploys** | ✅ `POST /api/deployments` → `Sealed` |
|
||||
| 11 | The deploy gate rejects what the page rejects | ✅ same row broken by SQL → HTTP 422 `NodeAclScopeUnresolved` |
|
||||
|
||||
Legs 8 and 11 are the pair that matters: the page and the deploy gate reject the same row for the same
|
||||
reason, which is the claim "`CheckScopeAsync` mirrors `DraftValidator`" reduces to. Leg 11 also
|
||||
confirmed the *kind* is honoured on both sides — the row was re-pointed at `RawDriver`, and the gate's
|
||||
message named `DriverInstance`.
|
||||
|
||||
Legs 5 and 9 are the positive controls. Without them the gate passes against a page that refuses
|
||||
everything, which is the same failure shape stage 6's `resolvedScopes=N` leg exists to catch.
|
||||
|
||||
**Not covered:** a UNS-realm scope (SITE-A has no UNS areas, so the picker's UNS column correctly showed
|
||||
"no UNS areas yet" and only the namespace row was selectable there). `UnsArea` / `UnsLine` / `Equipment`
|
||||
selection is covered offline only.
|
||||
|
||||
Rig restored afterwards: the gate's ACL row deleted and a further deployment sealed, leaving SITE-A at
|
||||
zero ACL rows as found.
|
||||
|
||||
## Known consequence to design around
|
||||
|
||||
**Driver-only nodes have no ConfigDb (Phase 4).** Keys and ACLs must therefore reach them inside the
|
||||
|
||||
+59
-13
@@ -228,12 +228,13 @@ Two consequences worth knowing:
|
||||
no roles. Admitting is the pre-#520 behaviour, and defaulting to it precisely when the server cannot
|
||||
determine what anonymous may do stops being harmless once reads are gated.
|
||||
|
||||
⚠️ **Node ACLs are enforced only when `Security:Acl:Enabled` is set, and only for three operations.**
|
||||
With the flag **off — the default** — a key's grants are collapsed into the two server-wide role gates
|
||||
and **only `Cluster`-scoped grants count**, so a narrower grant confers nothing rather than silently
|
||||
widening to server-wide authority. With it **on**, writes, alarm commands and HistoryRead are decided
|
||||
by scoped `NodeAcl` grants; **reads, browses and subscriptions stay ungated in either mode** (stage 6).
|
||||
See [Node ACL enforcement](#node-acl-enforcement-securityaclenabled) and
|
||||
⚠️ **Node ACLs are enforced only when `Security:Acl:Enabled` is set.** With the flag **off — the
|
||||
default** — a key's grants are collapsed into the two server-wide role gates and **only
|
||||
`Cluster`-scoped grants count**, so a narrower grant confers nothing rather than silently widening to
|
||||
server-wide authority, and read, browse, subscribe and HistoryRead are ungated for any admitted
|
||||
session. With it **on**, **every** data-plane operation is decided by scoped `NodeAcl` grants — read,
|
||||
browse, subscribe, write, HistoryRead and the alarm commands (stage 6). See
|
||||
[Node ACL enforcement](#node-acl-enforcement-securityaclenabled) and
|
||||
`docs/plans/2026-07-31-apikey-acl-enforcement-design.md`.
|
||||
|
||||
### LDAP resilience — mostly retired with the data-plane authenticator (#520)
|
||||
@@ -451,9 +452,10 @@ What is real about it:
|
||||
necessary at stage 6's read/browse/subscribe seams, which are on the publish path — and it will want
|
||||
a per-`(session, node)` decision cache, not this type.
|
||||
|
||||
`NodeAclScopeKind.FolderSegment` is the retired v2 kind with no v3 entity behind it. It is still offered
|
||||
in the authoring dropdown (labelled retired), but it can never resolve, and both ends now say so
|
||||
specifically rather than reporting it as a missing id: the deploy gate rejects it with
|
||||
`NodeAclScopeKind.FolderSegment` is the retired v2 kind with no v3 entity behind it. The stage-7 picker
|
||||
does not offer it at all, so it can only arrive on a legacy row or a hand-edited one; it can never
|
||||
resolve, and every end now says so specifically rather than reporting it as a missing id: the AdminUI
|
||||
refuses to save it, the deploy gate rejects it with
|
||||
`NodeAclScopeKindRetired`, and the trie builder drops it with a `RetiredScopeKind` diagnostic (Gitea
|
||||
#533 — it used to arrive as `UnresolvedScope`, which sent an operator holding a legacy row hunting for a
|
||||
typo in an id that was never the problem). One inconsistency remains: `NodePermissions.AlarmRead` has no
|
||||
@@ -462,13 +464,56 @@ typo in an id that was never the problem). One inconsistency remains: `NodePermi
|
||||
**String identity in this subsystem** (Gitea #533): **identifiers** — `ScopeId`, `ClusterId`,
|
||||
`Principal` — compare `OrdinalIgnoreCase`, because they are opaque handles whose spelling genuinely
|
||||
varies by source (a `ClusterId` arrives lower-cased through `OTOPCUA_ROLES` and verbatim through
|
||||
`Cluster:Roles`; a `ScopeId` is operator free-text until the stage-7 picker lands). **Path segments** —
|
||||
`Cluster:Roles`; a `ScopeId` comes from the stage-7 picker but a legacy row's may be anything).
|
||||
**Path segments** —
|
||||
every level of a scope path, and therefore `PermissionTrieNode.Children` — compare `Ordinal`, because
|
||||
they are browse names and `Speed` / `speed` are two different nodes that can coexist under one device.
|
||||
The rule is stated in full on `AclScopeRef`. It matters most where the deploy gate and the node are fed
|
||||
from *different* snapshots, which is exactly the split `AclScopePathResolver` exists to bridge: case
|
||||
drift there would silently drop every grant on the affected scope.
|
||||
|
||||
### Authoring grants (#520 stage 7)
|
||||
|
||||
Grants are authored at **`/clusters/{clusterId}/acls`**. Two things about the surface are worth knowing
|
||||
before relying on it, because both close a way a grant could previously be saved and deployed while
|
||||
authorizing nobody.
|
||||
|
||||
**The scope kind and the scope target are chosen together, from a tree.** "Choose scope…" opens a
|
||||
picker showing the cluster's **raw device tree** (lazily expanded, exactly the `/raw` hierarchy) beside
|
||||
its **UNS tree**, plus a whole-namespace row on each side. Selecting a node sets `ScopeKind` *and*
|
||||
`ScopeId` in one action. Until this landed the two were an unrelated dropdown and a free-text box, so
|
||||
`RawDevice` + a TagId was a well-formed row: it saved, it deployed, and it resolved to nothing.
|
||||
|
||||
Consequences of the trees being rooted at the grant's own cluster and of a node supplying its own kind:
|
||||
|
||||
- A **cross-cluster** scope is structurally unreachable — the picker's counterpart to the deploy gate's
|
||||
`NodeAclScopeCrossCluster` rule, rather than a second copy of it.
|
||||
- A **Cluster** row deliberately produces **no** `ScopeId`. The deploy gate rejects a Cluster grant that
|
||||
carries one (`NodeAclScopeIdUnexpected`), so a picker that helpfully filled in the cluster id would
|
||||
author rows that cannot be deployed.
|
||||
- **UNS tags are not offered.** A UNS signal is a `UnsTagReference` to a raw tag, and a `Tag` grant
|
||||
already covers the raw node *and* every UNS projection of it — so a UNS tag row would produce a grant
|
||||
that reads narrower than the one it creates. Pick the tag in the raw tree.
|
||||
- `FolderSegment` is not offered at all (see above).
|
||||
|
||||
**Rows are resolved against the live configuration, at authoring time.** `IAclScopeService.CheckScopeAsync`
|
||||
mirrors `DraftValidator.ValidateNodeAclScopes` rule for rule, so the page rejects what the deployment
|
||||
would reject:
|
||||
|
||||
- The editor **refuses to save** a grant whose scope does not resolve — re-checked on submit, not
|
||||
trusted from when the form loaded, because an existing row's entity may have been deleted since and
|
||||
the operator may never touch the picker.
|
||||
- The **list page** resolves every row and marks a broken one `unresolved` with the reason, so a grant
|
||||
that has rotted since it was authored is visible without waiting for a deploy to fail.
|
||||
- The **principal** field suggests every known principal (API keys, existing grants, `anonymous`) and
|
||||
reports how many *live* keys currently authenticate as the one typed. Reaching nobody is a **warning,
|
||||
not a block** — authoring a grant before minting its key is legitimate; a typo is not, and the two are
|
||||
indistinguishable to the server.
|
||||
|
||||
The checks are in `AclScopeService` rather than in the Razor pages because this repo has no bUnit:
|
||||
logic only a page can reach is untestable, and "the grant authorizes nobody" is not a failure mode to
|
||||
leave unpinned.
|
||||
|
||||
### Hierarchy (design)
|
||||
|
||||
ACLs are evaluated against the node's scope path. `NodeScope` (`src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/NodeScope.cs`) carries a `Kind` that selects between two hierarchy shapes:
|
||||
@@ -567,9 +612,10 @@ to those role strings via `GroupToRole`, e.g.:
|
||||
⚠️ **Only two role strings do anything.** `OpcUaDataPlaneRoles` declares exactly `WriteOperate` and
|
||||
`AlarmAck`, and those are the only values any gate compares against. `ReadOnly`, `WriteTune` and
|
||||
`WriteConfigure` appear in the permission vocabulary but **no code path reads them** — mapping a group
|
||||
to `WriteTune` grants nothing, and mapping one to `ReadOnly` restricts nothing (reads are ungated for
|
||||
everyone regardless). Earlier revisions of this document listed all five as "code-true"; that was
|
||||
wrong for three of them.
|
||||
to `WriteTune` grants nothing, and mapping one to `ReadOnly` restricts nothing (with the flag off,
|
||||
reads are ungated for everyone regardless; with it on, these role strings are not consulted at all —
|
||||
the scoped ACL decision replaces them). Earlier revisions of this document listed all five as
|
||||
"code-true"; that was wrong for three of them.
|
||||
|
||||
If this mapping is absent, **writes and alarm acknowledgement** default-deny: they return
|
||||
`BadUserAccessDenied` even for users who authenticate successfully. (The same requirement gates both
|
||||
|
||||
Reference in New Issue
Block a user