Node ACLs are authored and deployed but never enforced — wire IPermissionEvaluator or retire the surface #520
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Split out of the
deferment.md§3.1 audit. Decision taken 2026-07-27: make the non-enforcement explicit now (docs + UI warnings — done), and track the real wire-up here.What is true today
IPermissionEvaluator,TriePermissionEvaluator,PermissionTrieCache,PermissionTrieBuilderlive insrc/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/with 30 passing unit tests.IPermissionEvaluator.Authorizehas zero production call sites. Nothing registers it in DI.ZB.MOM.WW.OtOpcUa.OpcUaServer.csprojdoes not referenceZB.MOM.WW.OtOpcUa.Coreat all — the evaluator is not even on the node manager's compile-time reference graph.NodeAclrows (/clusters/{id}/acls), andConfigComposersnapshots every row into every deployment artifact — so an ACL edit shifts the artifactRevisionHashand ships fleet-wide.DeploymentArtifactnever deserializes them. The bytes arrive and are dropped.Net: an operator can author a grant, deploy it green, and have it change nothing.
Actual enforcement is two server-wide role checks —
WriteOperate(Value writes, either realm) andAlarmAck(all five Part 9 alarm methods). Read, Browse, HistoryRead and CreateMonitoredItems have no authorization check at all, so any admitted session including Anonymous reads the whole address space.Four blockers — none of them is "call the evaluator"
NodeHierarchyKindhas exactly one member,Equipment. Raw NodeIds areFolder/Driver/Device/TagGroup/TagRawPaths, whichNodeScopecannot express — yet the write gate is attached to raw tags. Roughly half the writable address space has no scope to evaluate. This is a design decision, not wiring.NodeAcl.LdapGroup, butOpcUaUserAuthResultcarries only mapped role strings; the group list is discarded insideLdapOpcUaUserAuthenticator.PermissionTrieBuilder'sscopePathsargument has no producer anywhere in the tree. Building without it drops every sub-cluster grant into the root-child fallback — the hazard the builder's own comments warn about. Note every existing evaluator unit test runs in the no-scopePaths"deterministic test mode", so the production hierarchy path is effectively untested.UserAuthorizationStatehas the freshness/staleness predicates but nothing constructs one, stampsAuthGenerationId, or drives a refresh.Plus: the node side never parses ACLs out of the artifact, and
OtOpcUaNodeManagerhas no cluster identity (NodeScope.ClusterIdisrequiredand the evaluator fails closed on mismatch).Smaller inconsistencies inside the subsystem
NodeAclScopeKind.FolderSegmentis offered in the authoring dropdown but the trie walker has no level to match it at.NodePermissions.AlarmReadhas noOpcUaOperationmapping — grantable but unreachable.NodeAcl.ScopeIdhas no FK and no existence check, so scope ids can dangle silently.SystemPlatformhierarchy kind thatacl-design.mddescribes was retired with v3.Scope of a fix
Write has a real chokepoint already (
OnEquipmentTagWrite) — it runs under the node-managerLock, so the check must stay non-blocking; the evaluator is pure, so that fits. Read has no seam at all — it needs either aReadoverride or a per-variableOnSimpleReadValuehandler on the hot publish path.Already done
docs/security.md,docs/ReadWriteOperations.md,docs/v2/acl-design.mdanddocs/v2/v2-release-readiness.mdcorrected; warning banners added toClusterAcls.razorandAclEdit.razor.v2-release-readiness.mdhad marked this "CLOSED 2026-04-19, PR #94" with a detailed description of Browse/CreateMonitoredItems/Call gating — none of those types exist insrc/.Direction decided 2026-07-31, design written, stage 1 landed on
feat/gitea-520-apikey-acl.Decisions (owner)
LdapOpcUaUserAuthenticatorand the OPC UA half ofSecurity:Ldapgo; AdminUI login keeps its own LDAP.D1 dissolves blocker #2 rather than solving it — the principal comes from the key, so there is no group list to plumb onto the session. D3 is the tidy one: there is no "anonymous bypass" branch in the evaluator to get wrong, because an ungranted anonymous session is denied by the same fail-closed path as an ungranted key.
Re-verification first
All six claims in the issue body still hold on
masteras of today. Two corrections to the issue's own survey, in both directions:HistoryReaddoes have a seam —HistoryReadRawModified/Processed/AtTime/Eventsare already overridden, they just carry no check.NodeAclScopeKindalready carriesNamespaceandFolderSegment. AddingNodeHierarchyKind.Rawover the RawPath segments makes raw-realm nodes representable and givesFolderSegmenta level to match at for the first time — it does not need aNodeScoperedesign.Two things the issue did not record
Security:Acl:Enabled, default off — the same dark-switch discipline asMeshTransport/ConfigSource/Telemetry. Without it the first deploy carrying this code silently starts denying every client with no key, including ScadaBridge's Data Connection Layer, which consumes this address space from another repo.NodeAclrows do. Real sharp edge, documented rather than discovered.Landed so far (stages 1a + 1b, behaviour-neutral)
NodeAcl.LdapGroup→Principalthrough the whole ACL model.LdapGroupRoleMappingis a different entity that also has anLdapGroupcolumn and is untouched — it backs AdminUI login.dbo.sp_ComputeGenerationDiff, created inV3Initial.StoredProceduresand never redefined. SQL Server does not validate a procedure body when a column is renamed under it, so the proc keeps reporting healthy and fails at CALL time withInvalid column name, silently breaking config-drift detection. Verified against the live dev DB that the deployed proc really does reference it. The migration re-emits the proc from one template shared by Up and Down.ApiKeyentity +ApiKeySecret(otua_<keyId>_<secret>, peppered HMAC-SHA256, fail-closed on an unset pepper), 18 new tests.OpcUaServer.csprojnow referencesCore— it did not before, soIPermissionEvaluatorwas literally uncallable from the node manager.Core 247 / OpcUaServer 364 / Configuration 128 green. Migrations are deliberately not applied to the shared dev DB yet: the running containers still use the old model and
ConfigComposerreadsdb.NodeAclson every deploy, so applying before a rebuild would break the rig's deploy path.Remaining stages
2 identity cutover · 3 raw scope +
scopePathsproducer · 4 artifact parsing + session lifecycle · 5 enforce write/alarms/HistoryRead · 6 enforce read/browse/subscribe · 7 AdminUI key management (and removing the two "not enforced" banners, which is the honest done-signal) · 8 docs + ScadaBridge key hand-off + live gate.Design doc:
docs/plans/2026-07-31-apikey-acl-enforcement-design.md.Stage 3 done —
3a6b2a1donfeat/gitea-520-stage3-raw-scope(unmerged). Stages 1/2/4 merged earlier infbe7403c.Stage 3 makes the
/rawdevice tree addressable by an ACL rule and builds thescopePathsproducer. It was larger than planned: the design doc described a raw path as the fixed five-tupleFolder/Driver/Device/TagGroup/Tag, but folders and tag groups both self-nest, soNodeScopehad to stop being fixed-arity entirely. Separately, level 1 of the walk gated on aNamespaceIdthat v3 has no entity for — it could only ever fail to match, which silently made every sub-cluster grant unreachable while cluster-scoped grants kept working. That level is now the realm, derived from the scope's kind.Blockers 1, 3 and 4 from this issue are now closed. Blocker 2 was dissolved by the API-key model in stage 2.
⚠️ Node ACLs are still NOT enforced. Nothing constructs a
NodeScopeor calls the evaluator; onlyCluster-scoped grants confer anything, viaApiKeyInterimRoleMapper. That is stages 5 (write / alarms / HistoryRead) and 6 (read / browse / subscribe). This stage makes the model able to express what those will enforce.Four defects found by review while doing it, all fixed: #528 #529 #530 #531. Two deferred: #532 #533 — #533 is worth settling before the stage 5/6 seams are written against the current shape.
Stage 3 is merged to
master— merge94990990(stage-3 commit3a6b2a1d), pushed.Two of the four wire-up blockers this issue tracks are now closed:
Raw-realm nodes have no representable scope—NodeScopeis(ClusterId, Kind, Segments)with a variable-length path,NodeHierarchyKindcarriesUnsandRaw, and fourRaw*scope kinds were appended for authoring. The old fixed-arity shape could not address a real raw tree at all: folder chains and tag-group chains both nest without bound, so it silently capped at five levels and mislabelled every one.—PermissionTrieBuilder'sscopePathshas no producerAclScopePathResolveris it, built on both sides of the split (EF at the deploy gate, artifact JSON on the node).scopePathsis now required with no fallback; an unresolvable scope is dropped with a diagnostic rather than planted at a trie position the walk can never reach.Blocker 4 (no session lifecycle) is stage 5's, as before.
Node ACLs are still NOT enforced. The two coarse server-wide gates (
WriteOperate,AlarmAck) remain the whole of data-plane authorization; Read/Browse/HistoryRead/CreateMonitoredItems are still ungated for any admitted session.docs/security.mdis updated to say so precisely.Also landed in the same merge, all closed: #532 (
Cluster:Roleshad noOTOPCUA_ROLESfallback despite two doc sites promising one — the null ClusterId made #528's cross-cluster guard fail open), #533 (three string-identity rules in one authorization path; now one rule — identifiersOrdinalIgnoreCase, path segmentsOrdinal), and #534 (both libplctag drivers ignored the authored EtherNet/IP port).Next: stage 5 — enforcement. It is unblocked in both realms now.
Stage 5 done — write, alarms and HistoryRead are enforced behind
Security:Acl:EnabledBranch
feat/gitea-520-stage5-enforce, not yet merged. Full solution suite green.The flag defaults to
false, and with it off behaviour is byte-for-byte what it was. With it on, scopedNodeAclgrants decide writes, alarm commands and HistoryRead — the last of which had no authorization check of any kind before this. Read, Browse and CreateMonitoredItems stay ungated in both modes; that is stage 6, and the AdminUI banners stay up until it lands (they now state the conditional truth rather than a flat "not enforced").The one design decision worth arguing with
The ACL decision replaces the role check; the two are not chained. Chained, a grant would have to confer the server-wide role as well as pass the scope check — and only
Cluster-scoped grants confer a role (ApiKeyInterimRoleMapper) — so every narrow grant would have stayed inert and the flag would have looked inert too. Narrow grants working is the whole point of the stage.That has a testing consequence I want on the record: the load-bearing test is an ALLOW, not a deny. A session carrying no roles at all writes successfully on the strength of its grant. A deny-only test passes against a build where the ACL is never consulted, because the role gate denies too — confirmed by mutation.
Found en route: the principal never reached the session
The authenticator passed it as
OpcUaUserAuthResult.DisplayName, butRoleCarryingUserIdentityderivesDisplayNamefrom the presented token — which for every key-bearing client is the static usernameapikey. So the trie's matching key was being dropped at the identity boundary, and the auditClientUserIdon a rejected write recordedapikeyfor every consuming system.PrincipalandApiKeyIdare now carried explicitly.Beyond the stage as written
UserAuthorizationStateis built per call, not per session. The per-session shape is LDAP-inherited: it exists so a long-lived session can re-resolve membership that changed out of band. Nothing can change out of band under API keys, so the generation stamp is the entire invalidation signal and a per-call record picks it up for free, where a cached one would need explicit invalidation to do the same job.IsStale/NeedsRefreshtherefore cannot fire on this path and are documented as vestigial rather than left looking load-bearing. Caching earns its keep at stage 6's publish-path seams, and wants a per-(session, node)decision cache — not this type.DataPlaneAuthorityState.Apply, under the same generation as the snapshot, rather than by a second wiring at the publish actor. An uninstalled trie denies everything, which is indistinguishable from working enforcement until someone checks that an ALLOW is possible at all. Deleting the install turns 13 of the 22 new tests red.unresolvedScopescount agreed with the builder on one of its four drop reasons; a rule with a blankScopeId, a retired scope kind, or a scope path not starting at a realm vanished with the log reading zero.Store widening
Both
IApiKeyStoreimplementations now return the full grant set. The narrowing was redundant —ApiKeyInterimRoleMapperapplies the same filter itself — and became a trap once scope was enforced. Pinned by asserting the roles are unchanged, with the two grants deliberately carrying different flags so it cannot pass by accident.Falsification
Each mutation turns the intended tests red and no others: enforcement never engaging (2), the write seam ignoring the ACL (1 — the allow-side test), the realm dropped from the scope build (1), the revocation check removed (2), the trie never installed (13 of 22).
Not done here
NodePermissions.AlarmReadstill has noOpcUaOperationmapping (grantable, unreachable), andScopeIdis still free text until the stage 7 picker.Stage 5 merged to
master— merge1ccc7400(branch commitf692f6af), pushed to origin.This supersedes the "done but unmerged" note in my previous comment. Nothing changed in the tree between that comment and the merge;
masterwas still at94990990, so the merge is exactly the reviewed branch.The stage 8 live gate still has NOT run.
Security:Acl:Enableddefaults tofalse, so merging changes no deployed behaviour — but the flag is now flippable on a real node without a code change, and every failure path in the gate denies. A wiring or config mistake therefore presents as a total outage of writes, alarm commands and HistoryRead rather than as a subtle permission bug. Do not turn it on anywhere that matters until the docker-dev gate has run.Remaining: stage 6 (read/browse/subscribe, with per-
(session, node)decision caching on the hot publish path), stage 7 (authoring UI against principals + aRawTreepicker forScopeId, and removal of the AdminUI warning banners), stage 8 (docs, ScadaBridge key hand-off, the live gate).Two known inconsistencies carried forward, neither blocking:
NodePermissions.AlarmReadhas noOpcUaOperationmapping, so it is grantable but unreachable; andNodeAcl.ScopeIdstays free text until stage 7 ships the picker.Stage 6 merged to
master— merged4389f57, pushed. Stage 8 live gate PASSED on docker-dev the same day.Enforcement is now complete behind
Security:Acl:Enabled(still defaultfalse): read (every attribute, not just Value), browse,TranslateBrowsePath,CreateMonitoredItems,SubscribeToEventsandSetMonitoringModeall consult scoped grants.The two things that needed live verification
Revoking a grant mid-subscription, as a transcript:
53 -> 157across the gap: the device counter ran throughout, so delivery stopped, not the source.Browse filtering, A/B against the same artifact: gated node shows
plconly; control node (flag off) showsplcandplc2.Design decisions worth arguing with
Browsegrant on each ancestor — the additive walk would carry that onto every sibling subtree. Accepted consequence: a narrow Browse grant exposes the names of its ancestor folders.MonitoringMode.Disabled. Disabling discards the item queued notifications, so the client is silenced without being told why. The over-the-wire test caught this; the unit path reports the item as stood down either way. Detaching is also reversible, so a restored grant resumes the feed with no client action.Three defects found en route, all the same shape
An
object-typed SDK parameter that the wrong cast reads asnull, disabling a gate silently and compiling cleanly:ContinuationPoint.NodeToBrowseis aNodeHandle, not aNodeState— the browse source gate was skipped.SubscribeToEventssourceIdlikewise — an ungranted session could have subscribed to an equipment event notifier and received every alarm it raises, while every value on it was refused.UserAuthorizationStatestaleness ceiling couples the authorizer clock to the evaluator, and any divergence denies everything with a verdict indistinguishable from "no grant matched". Stage 5 claim that these predicates cannot fire was accidentally true; now disabled explicitly.Not covered by the gate
A UNS-realm grant (the rig SITE-A cluster has no UNS equipment) and alarm-command scoping (no alarms there). Both are covered offline; neither has been seen live.
Consumer-visible, and the umbrella index was wrong
With the flag on, a read-only consumer now needs a
Readgrant. The scadaproj index previously told ScadaBridge that reads and subscriptions were ungated in either mode — corrected and pushed (555d1b4).Remaining: stage 7 (ACL authoring UI against principals, a
RawTreepicker forScopeId, and removing the two AdminUI warning banners). Known inconsistencies carried forward:NodePermissions.AlarmReadhas noOpcUaOperationmapping so it is grantable but unreachable, andNodeAcl.ScopeIdstays free text until that picker.Stage 7 done — #520 is complete.
master3796c0bf(merge), work commitd542ab5b. Pushed.What landed
Grants are authored from a scope picker over the cluster's real raw + UNS trees, and it supplies the scope kind and the scope target together. Before this 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 authorized nobody. Because the trees are rooted at the grant's own cluster and each node carries its own kind, four of the deploy gate's five rules become unauthorable by construction rather than merely rejected later —NodeAclScopeCrossCluster,NodeAclScopeIdUnexpected,NodeAclScopeIdMissing,NodeAclScopeKindRetired.IAclScopeService.CheckScopeAsyncmirrorsDraftValidator.ValidateNodeAclScopesrule for rule and blocks the save when a scope resolves to nothing; the list page marks rotted rowsunresolvedwith 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 a key is legitimate and a typo is not, and the two are indistinguishable to the server.Departures from the stage line, all deliberate
Clusterrow produces a NULLScopeId. The deploy gate rejects a Cluster grant carrying one, so a picker that helpfully filled in the cluster id would author rows that cannot be deployed.AclScopeTreeis a new component, not a third mode onRawTree.RawTreealready carries the/rawediting tree and the equipment reference picker; a third mode would put this feature's bugs on the device-authoring page, and with no bUnit here the only thing that catches that is someone noticing. It also renders a unified node type, which is what lets one component serve both hierarchies.UnsTagReference, and aTaggrant already covers the raw node and every UNS projection of it — a UNS tag row would read narrower than the grant it creates.Found en route
The ACL editor had a
DataAnnotationsValidatorand noValidationSummary, so an invalidNodeAclIdmade Create a silent no-op. Present since the page was written; same failure mode as #505.Verification
30 tests, falsified with six production mutations — each turned exactly the intended tests red and no others. Full AdminUI suite 999/999.
Live gate PASSED on docker-dev, 11 legs. Both central containers asserted on the same freshly-built image by container id (the stale-central-2 trap from the stage-6 gate). The pair that matters:
NodeAclScopeUnresolved, and the gate's message namedDriverInstanceafter the row was re-pointed atRawDriver, confirming the kind is honoured on both sides.That pair is what the claim "the page mirrors the deploy gate" reduces to. Legs 5 and 9 are the positive controls — without them the gate passes against a page that refuses everything, the same shape stage 6's
resolvedScopes=Nleg exists to catch. Leg 10: a grant authored through the picker deploysSealed.Not covered: UNS-realm scope selection — SITE-A has no UNS areas, so the picker's UNS column correctly showed "no UNS areas yet".
UnsArea/UnsLine/Equipmentare covered offline only.Rig restored to zero ACL rows with a further sealed deployment.
Still open, deliberately
NodePermissions.AlarmReadhas noOpcUaOperationmapping — grantable but unreachable. Unchanged by this stage.NodeAcl.ScopeIdstill has no FK. It names a row in one of eight tables chosen byScopeKind, which is not a foreign key any schema can express; the existence check is the substitute, now at both authoring and deploy time.Docs:
docs/security.md§"Authoring grants",docs/plans/2026-07-31-apikey-acl-enforcement-design.md(stage 7 + its live gate),CLAUDE.md. Umbrella index updated onscadaproj/main(7a6f997).One stale line fixed in
docs/security.mdwhile here: it still claimed reads, browses and subscriptions "stay ungated in either mode", which stage 6 had already falsified elsewhere in the same document.The UNS gap in the stage-7 gate is closed.
mastera88cefd5.The first pass could not exercise UNS-realm scope selection — SITE-A had no UNS, so the picker's UNS column correctly showed "no UNS areas yet" and only the namespace row was selectable. Authored one on docker-dev (area → line → equipment) and re-ran that part.
UNSAREA/UNSLINE/EQUIPMENT, eagerEQUIPMENT+EQ-f3dd0e8e27a4, "✓ Equipment 'packer-1' in cluster 'SITE-A'"SealedUNRESOLVEDnamingMAIN; deploy → HTTP 422NodeAclScopeCrossClusterLeg 15 is the one worth keeping. It is the only rejection whose id is real, so a check that asked only "does a row with this id exist?" would accept it and confer authority across a cluster boundary. Both ends catch it and both name the owning cluster.
Every stage-7 claim is now covered live. Nothing outstanding on this issue.
Filed en route: #535. Authoring that UNS hit the same defect class this stage just closed, in a different page: the
/unsarea and line modals do not validate the UNS segment format, soPackagingandLine 1saved without complaint and then failed the deploy withUnsSegmentInvalid. Only the equipment form carries the^[a-z0-9-]{1,32}$rule. A deployment snapshots the whole database, so one badly-named area blocks every cluster's deploy. Out of scope here, not fixed.