From b177636d7ab45a8c6280cecc55eb839629d67891 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:28:54 -0400 Subject: [PATCH] fix(GWC-38): deny Galaxy browse when the ambient identity is absent under API-key auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GatewayBrowseScopeProvider resolved `identityAccessor.Current?.EffectiveConstraints ?? ApiKeyConstraints.Empty`, so an absent ambient identity produced an empty BrowseSubtrees list — which the shared GalaxyRepository library reads as "no scoping". The control's fail state was therefore open: any future path reaching the library's galaxy service without GatewayGrpcAuthorizationInterceptor having pushed the identity would silently hand a browse-scoped key the full hierarchy, with nothing failing and nothing logged. A constraint-narrowing control's absence is invisible in normal operation, which is what made the honest "safe only because the global interceptor runs first" comment insufficient on its own. The provider now distinguishes the two states the old expression collapsed: - authenticated key with no BrowseSubtrees constraint -> empty list, unscoped (unchanged, and now asserted in its own test rather than implied); - no ambient identity with Authentication:Mode=ApiKey -> log a warning naming the broken ordering invariant and throw RpcException(PermissionDenied); - no ambient identity with Authentication:Mode=Disabled -> unscoped, because nothing authenticates there so no key and no constraints exist to scope by. The registration-order comment at GatewayApplication.cs now says the ordering is still required but no longer silently trusted. Tests: GatewayBrowseScopeProviderTests gains the PermissionDenied case and the Disabled-mode case alongside the unconstrained-identity one. GalaxyRepositoryHostWiringTests phase 1 now pushes an identity carrying no BrowseSubtrees instead of pushing none — that, not an absent identity, is production's unscoped case. Docs: docs/Authorization.md's constraint section states the two-state distinction and both fail behaviours under browse_subtrees. --- docs/Authorization.md | 15 +++++ .../GatewayApplication.cs | 6 ++ .../GatewayBrowseScopeProvider.cs | 60 +++++++++++++++-- .../Grpc/GalaxyRepositoryHostWiringTests.cs | 43 +++++++++--- .../GatewayBrowseScopeProviderTests.cs | 67 +++++++++++++++++-- 5 files changed, 169 insertions(+), 22 deletions(-) diff --git a/docs/Authorization.md b/docs/Authorization.md index b1fb269..12c393d 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -180,6 +180,21 @@ Supported constraints are: | `read_historized_only` | Read/subscription commands must target objects with historized attributes. | | `dashboard_tags` | Dashboard event-visibility tags. **Not a data-access constraint** — see below. | +`browse_subtrees` is applied by `GatewayBrowseScopeProvider`, the host seam the +shared `ZB.MOM.WW.GalaxyRepository` package calls through `IGalaxyBrowseScopeProvider`; +it reads the constraint off the ambient per-call identity that +`GatewayGrpcAuthorizationInterceptor` pushed earlier in the same call. An empty +`browse_subtrees` list is the library's "no scoping" signal, so the provider has to +distinguish two states that look alike (GWC-38): an authenticated key with no browse +constraint is legitimately unscoped and returns the empty list, whereas **no ambient +identity at all** means the interceptor did not run before the Galaxy service and the +request's scope cannot be established. In that second case, with +`MxGateway:Authentication:Mode = ApiKey`, the provider logs a warning naming the broken +ordering invariant and fails the call with `PermissionDenied` rather than serving the full +hierarchy — a narrowing control's silent absence is invisible in normal operation, so it is +made loud instead. Under `Mode = Disabled` an absent identity is the expected steady state +(nothing authenticates, so no key and no constraints exist) and browse stays unscoped. + `dashboard_tags` is the one member of the blob that constrains nothing on the gRPC data path. No read, write, browse, or subscribe check consults it, and `HasReadConstraints` / `HasWriteConstraints` deliberately ignore it: adding a tag diff --git a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs index d5415c8..e075983 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs @@ -150,6 +150,12 @@ public static class GatewayApplication builder.Services.Configure(o => o.AddSecretsAuthorization()); // Register the gateway's browse-scope provider before AddZbGalaxyRepository so the // library's TryAddSingleton default (NullGalaxyBrowseScopeProvider) does not win. + // The provider depends on GatewayGrpcAuthorizationInterceptor (registered globally above) + // having pushed the ambient identity before the library's galaxy service runs. That + // ordering is still required, but it is no longer trusted silently: with + // Authentication:Mode=ApiKey the provider denies with PermissionDenied and logs the broken + // invariant when the identity is absent, instead of resolving to the full hierarchy + // (GWC-38). Under Mode=Disabled identity-absent is expected and stays unscoped. builder.Services.AddSingleton(); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayBrowseScopeProvider.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayBrowseScopeProvider.cs index a7426e0..94186bd 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayBrowseScopeProvider.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayBrowseScopeProvider.cs @@ -1,26 +1,72 @@ using Grpc.Core; +using Microsoft.Extensions.Options; using ZB.MOM.WW.GalaxyRepository.Grpc; +using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Security.Authentication; namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; /// Scopes Galaxy browse results to the calling API key's BrowseSubtrees constraint. -public sealed class GatewayBrowseScopeProvider(IGatewayRequestIdentityAccessor identityAccessor) +/// +/// Distinguishes two states the pre-GWC-38 code collapsed into one: "authenticated, no +/// BrowseSubtrees constraint" (legitimately unscoped — the common case) and "no ambient identity +/// at all" (the interceptor-ordering invariant below is broken). The second used to resolve to the +/// full hierarchy silently, which made a scoping bypass invisible; it now denies loudly. +/// +/// Accessor for the ambient per-call API-key identity. +/// Gateway options supplying MxGateway:Authentication:Mode. +/// Logger used to name the broken invariant when identity is absent. +public sealed class GatewayBrowseScopeProvider( + IGatewayRequestIdentityAccessor identityAccessor, + IOptions options, + ILogger logger) : IGalaxyBrowseScopeProvider { /// Resolves the Galaxy browse subtrees the calling API key is scoped to, from the ambient request identity. /// The gRPC server call context for the in-flight browse request. /// The caller's configured browse subtrees, or /empty when the caller is unscoped (full hierarchy). + /// + /// when API-key authentication is enabled but no + /// ambient identity is present — the invariant described below has been broken and the + /// request's scope cannot be established. + /// public IReadOnlyList? ResolveBrowseSubtrees(ServerCallContext context) { // Invariant: the caller identity is the ambient one pushed by // GatewayGrpcAuthorizationInterceptor earlier in this same call. That interceptor is // registered globally and therefore runs before the library's galaxy service, so - // Current is populated for any authenticated galaxy RPC. If it is somehow absent we - // return empty BrowseSubtrees, which the library treats as "no scoping" (full - // hierarchy) — safe only because the global interceptor has already authenticated and - // authorized the request first. Do not reorder the interceptor without revisiting this. - ApiKeyConstraints constraints = identityAccessor.Current?.EffectiveConstraints ?? ApiKeyConstraints.Empty; - return constraints.BrowseSubtrees; + // Current is populated for any authenticated galaxy RPC. Do not reorder the interceptor + // without revisiting this. + ApiKeyIdentity? identity = identityAccessor.Current; + if (identity is not null) + { + return identity.EffectiveConstraints.BrowseSubtrees; + } + + // Mode=Disabled is the one configuration in which identity-absent is expected rather than + // broken: nothing authenticates, so no key and no constraints exist to scope by, and the + // deployment has already accepted an unauthenticated gRPC surface. Empty BrowseSubtrees is + // what the library reads as "no scoping". + if (options.Value.Authentication.Mode == AuthenticationMode.Disabled) + { + return ApiKeyConstraints.Empty.BrowseSubtrees; + } + + // Authentication is on, so an identity MUST have been pushed before the galaxy service ran. + // Failing open here would hand a browse-scoped key the full hierarchy with nothing logged + // and nothing failing — a constraint-narrowing control's absence is invisible in normal + // operation. Deny instead, and name the invariant so the misconfiguration is diagnosable + // from one log line rather than from a scoping audit nobody thought to run. + logger.LogWarning( + "Galaxy browse scope could not be resolved for {Method}: no ambient API-key identity " + + "while Authentication:Mode is {Mode}. GatewayGrpcAuthorizationInterceptor must run " + + "before the Galaxy service on every call; the request is denied rather than served " + + "unscoped.", + context.Method, + options.Value.Authentication.Mode); + + throw new RpcException(new Status( + StatusCode.PermissionDenied, + "Browse scope could not be established for this request.")); } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/GalaxyRepositoryHostWiringTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/GalaxyRepositoryHostWiringTests.cs index 2be85ae..0178369 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/GalaxyRepositoryHostWiringTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/GalaxyRepositoryHostWiringTests.cs @@ -1,5 +1,8 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; using ZB.MOM.WW.GalaxyRepository; using ZB.MOM.WW.GalaxyRepository.Grpc; +using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Security.Authentication; using ZB.MOM.WW.MxGateway.Server.Security.Authorization; using ZB.MOM.WW.MxGateway.Tests.TestSupport; @@ -23,8 +26,11 @@ public sealed class GalaxyRepositoryHostWiringTests { /// /// Verifies the full production authz-filtering chain end-to-end. - /// Phase 1: no identity pushed → BrowseChildren returns both children - /// (unconstrained). Phase 2: identity with BrowseSubtrees = ["NonExistent"] + /// Phase 1: an identity carrying no BrowseSubtrees pushed → BrowseChildren + /// returns both children (unconstrained). That, not an absent identity, is production's + /// unscoped case since GWC-38 — with Authentication:Mode=ApiKey an absent identity + /// now denies, which the isolated provider test pins. + /// Phase 2: identity with BrowseSubtrees = ["NonExistent"] /// pushed via → the real /// threads the constraint into the lib /// service → BrowseChildren returns empty children. @@ -35,7 +41,16 @@ public sealed class GalaxyRepositoryHostWiringTests { // Arrange: construct the REAL host seam — not stubs. GatewayRequestIdentityAccessor identityAccessor = new(); - GatewayBrowseScopeProvider scopeProvider = new(identityAccessor); + GatewayBrowseScopeProvider scopeProvider = new( + identityAccessor, + Options.Create(new GatewayOptions + { + Authentication = new Server.Configuration.AuthenticationOptions + { + Mode = AuthenticationMode.ApiKey, + }, + }), + NullLogger.Instance); GalaxyObject[] objects = CreateFilterObjects(); GalaxyHierarchyCacheEntry entry = GalaxyHierarchyCacheEntry.Empty with @@ -55,13 +70,23 @@ public sealed class GalaxyRepositoryHostWiringTests new GalaxyDeployNotifier(), scopeProvider); - // Phase 1: no identity on the accessor → browse is unconstrained. - // Pump_001 and Valve_001 are both children of Line3 (GobjectId = 2). - BrowseChildrenReply unconstrained = await service.BrowseChildren( - new BrowseChildrenRequest { ParentGobjectId = 2 }, - new TestServerCallContext()); + // Phase 1: an authenticated identity with no BrowseSubtrees constraint → browse is + // unconstrained. Pump_001 and Valve_001 are both children of Line3 (GobjectId = 2). + ApiKeyIdentity unscopedIdentity = new( + KeyId: "test-key", + KeyPrefix: "mxgw_test", + DisplayName: "unscoped", + Scopes: new HashSet(StringComparer.Ordinal) { GatewayScopes.MetadataRead }, + Constraints: ApiKeyConstraints.Empty); - Assert.Equal(2, unconstrained.Children.Count); + using (identityAccessor.Push(unscopedIdentity)) + { + BrowseChildrenReply unconstrained = await service.BrowseChildren( + new BrowseChildrenRequest { ParentGobjectId = 2 }, + new TestServerCallContext()); + + Assert.Equal(2, unconstrained.Children.Count); + } // Phase 2: push a constraint whose BrowseSubtrees do not match any seeded object. // GatewayBrowseScopeProvider.ResolveBrowseSubtrees must return ["NonExistent"], diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayBrowseScopeProviderTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayBrowseScopeProviderTests.cs index 9972670..1e0eba2 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayBrowseScopeProviderTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayBrowseScopeProviderTests.cs @@ -1,3 +1,7 @@ +using Grpc.Core; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Security.Authentication; using ZB.MOM.WW.MxGateway.Server.Security.Authorization; using ZB.MOM.WW.MxGateway.Tests.TestSupport; @@ -19,7 +23,7 @@ public sealed class GatewayBrowseScopeProviderTests public void ResolveBrowseSubtrees_WithCurrentIdentity_ReturnsConstraintBrowseSubtrees() { StubIdentityAccessor accessor = new(CreateIdentity(["AreaA"])); - GatewayBrowseScopeProvider provider = new(accessor); + GatewayBrowseScopeProvider provider = CreateProvider(accessor, AuthenticationMode.ApiKey); IReadOnlyList? result = provider.ResolveBrowseSubtrees(new TestServerCallContext()); @@ -27,14 +31,16 @@ public sealed class GatewayBrowseScopeProviderTests } /// - /// With no ambient identity, the provider falls back to the empty browse-subtree - /// list, which the library treats as "no scoping" rather than denying everything. + /// An authenticated key with no BrowseSubtrees constraint is legitimately unscoped: the + /// empty list is what the library reads as "no scoping". This is the state GWC-38 required + /// to stay distinguishable from an absent identity, so it is asserted alongside its + /// denying twin below rather than left implicit. /// [Fact] - public void ResolveBrowseSubtrees_WithNoCurrentIdentity_ReturnsEmptyList() + public void ResolveBrowseSubtrees_WithUnconstrainedIdentity_ReturnsEmptyList() { - StubIdentityAccessor accessor = new(current: null); - GatewayBrowseScopeProvider provider = new(accessor); + StubIdentityAccessor accessor = new(CreateIdentity([])); + GatewayBrowseScopeProvider provider = CreateProvider(accessor, AuthenticationMode.ApiKey); IReadOnlyList? result = provider.ResolveBrowseSubtrees(new TestServerCallContext()); @@ -42,6 +48,55 @@ public sealed class GatewayBrowseScopeProviderTests Assert.Empty(result); } + /// + /// GWC-38: with API-key authentication enabled, an absent ambient identity means the + /// interceptor-ordering invariant is broken — the request's browse scope cannot be + /// established. The provider must DENY rather than resolve to the full hierarchy, because + /// a silently-unscoped browse is an invisible authorization bypass. The status code is + /// pinned: PermissionDenied is what the gRPC surface returns for an authorization failure + /// the caller cannot fix by re-authenticating. + /// + [Fact] + public void ResolveBrowseSubtrees_WithNoCurrentIdentityAndApiKeyAuth_ThrowsPermissionDenied() + { + StubIdentityAccessor accessor = new(current: null); + GatewayBrowseScopeProvider provider = CreateProvider(accessor, AuthenticationMode.ApiKey); + + RpcException exception = Assert.Throws( + () => provider.ResolveBrowseSubtrees(new TestServerCallContext())); + + Assert.Equal(StatusCode.PermissionDenied, exception.StatusCode); + } + + /// + /// Under Authentication:Mode=Disabled nothing authenticates, so an absent identity + /// is the expected steady state rather than a broken invariant: there is no key and no + /// constraint to scope by. The provider keeps returning unscoped, so disabling + /// authentication does not turn every Galaxy browse into a PermissionDenied. + /// + [Fact] + public void ResolveBrowseSubtrees_WithNoCurrentIdentityAndAuthDisabled_ReturnsEmptyList() + { + StubIdentityAccessor accessor = new(current: null); + GatewayBrowseScopeProvider provider = CreateProvider(accessor, AuthenticationMode.Disabled); + + IReadOnlyList? result = provider.ResolveBrowseSubtrees(new TestServerCallContext()); + + Assert.NotNull(result); + Assert.Empty(result); + } + + private static GatewayBrowseScopeProvider CreateProvider( + StubIdentityAccessor accessor, + AuthenticationMode mode) => + new( + accessor, + Options.Create(new GatewayOptions + { + Authentication = new Server.Configuration.AuthenticationOptions { Mode = mode }, + }), + NullLogger.Instance); + private static ApiKeyIdentity CreateIdentity(IReadOnlyList browseSubtrees) => new( KeyId: "operator01",