ACL: three different string-identity rules in one authorization path #533

New Issue

2026-08-01T01:10:31-04:00

dohertj2 commented

2026-08-01 01:10:31 -04:00

Raised in review of #520 stage 3 (3a6b2a1d). Not a live bug — filed before the enforcement seams in stages
5/6 are written against the current shape.

One code path uses three comparison rules:

Where	Rule
`AclScopeRef` (record-struct default equality on `ScopeId`)	ordinal, case-sensitive
`PermissionTrieBuilder.Build` filtering `row.ClusterId`	`OrdinalIgnoreCase`
`PermissionTrie.CollectMatches` hashing principals	`OrdinalIgnoreCase`

It works today only because AclScopePathResolver.Resolve and PermissionTrieBuilder.Build are handed the
same NodeAcl instances, so the ScopeId strings are literally equal. The moment the two sides are fed
from different snapshots — the deploy gate reads EF entities, the node reads artifact JSON, which is exactly
the split the resolver was designed for — any case drift in a ScopeId drops every grant on that scope.

Fail-closed and diagnosed (UnresolvedScope), so not a security hole, but a silent authorization loss.

Fix: either give AclScopeRef an explicit OrdinalIgnoreCase comparison, or state the ordinal contract in
its doc and have the deploy gate enforce it. Note PermissionTrieNode.Children is deliberately Ordinal and
must stay that way — it matches RawPaths.Comparer and closed a real widening; this issue is about the
id comparisons, not the path-segment ones.

Smaller item from the same review: PermissionTrieBuildDiagnosticReason.UnresolvedScope conflates a
dangling/deleted id, the deliberately-unresolvable retired FolderSegment kind, and a producer returning an
empty list. An operator with a legacy FolderSegment row gets the same message as one with a typo; a
separate RetiredScopeKind reason would be actionable.

Raised in review of #520 stage 3 (`3a6b2a1d`). Not a live bug — filed before the enforcement seams in stages 5/6 are written against the current shape. One code path uses three comparison rules: | Where | Rule | |---|---| | `AclScopeRef` (record-struct default equality on `ScopeId`) | ordinal, case-**sensitive** | | `PermissionTrieBuilder.Build` filtering `row.ClusterId` | `OrdinalIgnoreCase` | | `PermissionTrie.CollectMatches` hashing principals | `OrdinalIgnoreCase` | It works today only because `AclScopePathResolver.Resolve` and `PermissionTrieBuilder.Build` are handed the *same* `NodeAcl` instances, so the `ScopeId` strings are literally equal. The moment the two sides are fed from different snapshots — the deploy gate reads EF entities, the node reads artifact JSON, which is exactly the split the resolver was designed for — any case drift in a `ScopeId` drops **every** grant on that scope. Fail-closed and diagnosed (`UnresolvedScope`), so not a security hole, but a silent authorization loss. Fix: either give `AclScopeRef` an explicit `OrdinalIgnoreCase` comparison, or state the ordinal contract in its doc and have the deploy gate enforce it. Note `PermissionTrieNode.Children` is deliberately `Ordinal` and must stay that way — it matches `RawPaths.Comparer` and closed a real widening; this issue is about the *id* comparisons, not the path-segment ones. Smaller item from the same review: `PermissionTrieBuildDiagnosticReason.UnresolvedScope` conflates a dangling/deleted id, the deliberately-unresolvable retired `FolderSegment` kind, and a producer returning an empty list. An operator with a legacy `FolderSegment` row gets the same message as one with a typo; a separate `RetiredScopeKind` reason would be actionable.

dohertj2 referenced this issue

2026-08-01 01:10:46 -04:00

Node ACLs are authored and deployed but never enforced — wire IPermissionEvaluator or retire the surface #520

dohertj2 referenced this issue

2026-08-01 01:51:56 -04:00

Cluster:Roles has no OTOPCUA_ROLES fallback, despite docs claiming one — ClusterId resolves null #532

dohertj2 commented

2026-08-01 01:51:56 -04:00

Fixed in 06c56d77 (branch fix/gitea-532-533-roles-and-string-identity).

The rule, now stated once on AclScopeRef and applied throughout: identifiers — ScopeId, ClusterId, Principal — compare OrdinalIgnoreCase; path segments compare Ordinal.

Chose case-insensitive ids over enforcing the ordinal contract at the gate, for two reasons the issue didn't have. First, the gate cannot enforce it: the drift the issue describes is a serialization/snapshot difference, not something an author types. Second, there is a live source of drift that is authored — ScopeId is free-text in the ACL editor until the stage-7 picker lands, so an operator typing eq-… for EQ-… currently loses the grant to a UnresolvedScope that reads exactly like a typo, which it also is.

One thing the issue understated. DraftValidator's cross-cluster check compared owner to acl.ClusterId with StringComparison.Ordinal, while PermissionTrieBuilder has always matched OrdinalIgnoreCase — so that was a fourth rule, and the one that fails in the expensive direction. It would reject an entire deployment as cross-cluster over a ClusterId case difference the runtime would have accepted, and the two spellings genuinely occur (see #532). The gate's ScopeId existence maps were ordinal too, which broke its stated invariant that "a row this rule passes is a row the resolver resolves."

Where the fix went. AclScopePathResolver re-keys its own maps in the constructor rather than documenting a comparer contract for callers. That was the point of the issue — the two producers (EF at the deploy gate, artifact JSON on the node) build those dictionaries independently, and a contract neither can check is how they diverge. Doing it inside makes the rule unbreakable from the outside. The reference scan for UNS projections is a linear compare rather than a lookup, so it needed the rule applied separately; missing it would have been the worst case — a Tag grant resolving its raw path but silently skipping a UNS path, leaving the tag writable through the NodeId the grant missed.

PermissionTrieNode.Children stays Ordinal, as the issue required.

Sub-item: RetiredScopeKind split out of UnresolvedScope, short-circuited ahead of the ScopeId checks so it mirrors the deploy gate's own ordering (NodeAclScopeKindRetired). Paired with a control test that a live kind with a dangling id still reports UnresolvedScope — without it, "split the reason out" could be satisfied by relabelling every drop.

Falsified by three separate reverts (ref equality → 1 test red, resolver map comparer → 6, gate comparisons → 2), plus removing the FolderSegment short-circuit → 1.

Fixed in `06c56d77` (branch `fix/gitea-532-533-roles-and-string-identity`). **The rule, now stated once on `AclScopeRef` and applied throughout:** identifiers — `ScopeId`, `ClusterId`, `Principal` — compare `OrdinalIgnoreCase`; path segments compare `Ordinal`. Chose case-insensitive ids over enforcing the ordinal contract at the gate, for two reasons the issue didn't have. First, the gate cannot enforce it: the drift the issue describes is a serialization/snapshot difference, not something an author types. Second, there is a live source of drift that is authored — `ScopeId` is free-text in the ACL editor until the stage-7 picker lands, so an operator typing `eq-…` for `EQ-…` currently loses the grant to a `UnresolvedScope` that reads exactly like a typo, which it also is. **One thing the issue understated.** `DraftValidator`'s cross-cluster check compared `owner` to `acl.ClusterId` with `StringComparison.Ordinal`, while `PermissionTrieBuilder` has always matched `OrdinalIgnoreCase` — so that was a fourth rule, and the one that fails in the expensive direction. It would reject an entire deployment as cross-cluster over a ClusterId case difference the runtime would have accepted, and the two spellings genuinely occur (see #532). The gate's `ScopeId` existence maps were ordinal too, which broke its stated invariant that "a row this rule passes is a row the resolver resolves." **Where the fix went.** `AclScopePathResolver` re-keys its own maps in the constructor rather than documenting a comparer contract for callers. That was the point of the issue — the two producers (EF at the deploy gate, artifact JSON on the node) build those dictionaries independently, and a contract neither can check is how they diverge. Doing it inside makes the rule unbreakable from the outside. The reference scan for UNS projections is a linear compare rather than a lookup, so it needed the rule applied separately; missing it would have been the worst case — a Tag grant resolving its raw path but silently skipping a UNS path, leaving the tag writable through the NodeId the grant missed. `PermissionTrieNode.Children` stays `Ordinal`, as the issue required. **Sub-item: `RetiredScopeKind`** split out of `UnresolvedScope`, short-circuited ahead of the ScopeId checks so it mirrors the deploy gate's own ordering (`NodeAclScopeKindRetired`). Paired with a control test that a live kind with a dangling id still reports `UnresolvedScope` — without it, "split the reason out" could be satisfied by relabelling every drop. Falsified by three separate reverts (ref equality → 1 test red, resolver map comparer → 6, gate comparisons → 2), plus removing the FolderSegment short-circuit → 1.

dohertj2 closed this issue

2026-08-01 01:51:57 -04:00

dohertj2 referenced this issue from a commit

2026-08-01 02:38:21 -04:00

fix(cluster,acl): OTOPCUA_ROLES fallback + one string-identity rule (#532, #533)

dohertj2 referenced this issue from a commit

2026-08-01 02:38:21 -04:00

merge: ACL scope model + role/identity fixes + four real test failures (#520 s3, #532, #533, #534)

dohertj2 referenced this issue

2026-08-01 02:39:22 -04:00

Node ACLs are authored and deployed but never enforced — wire IPermissionEvaluator or retire the surface #520

Sign in to join this conversation.