Merge branch 'fix/archreview-p2' into main (P2 tier: completeness & polish)
# Conflicts: # archreview/remediation/00-tracking.md # clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayCliSecretRedactor.cs # clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs # src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs
This commit is contained in:
Joseph Doherty
@@ -35,5 +35,23 @@ public static class DashboardAuthenticationDefaults
|
||||
|
||||
public const string LdapGroupClaimType = "mxgateway:ldap_group";
|
||||
public const string KeyPrefixClaimType = "mxgateway:key_prefix";
|
||||
|
||||
/// <summary>
|
||||
/// Dashboard auth cookie name used when the cookie is not guaranteed to be Secure
|
||||
/// (<c>RequireHttpsCookie=false</c> → <see cref="Microsoft.AspNetCore.Authentication.Cookies.CookieSecurePolicy.SameAsRequest"/>)
|
||||
/// or when an explicit <c>MxGateway:Dashboard:CookieName</c> override is absent but the
|
||||
/// secure default cannot be applied. This plain name carries no browser-enforced guarantees.
|
||||
/// </summary>
|
||||
public const string CookieName = "MxGatewayDashboard";
|
||||
|
||||
/// <summary>
|
||||
/// Dashboard auth cookie name applied when the cookie is guaranteed Secure
|
||||
/// (<c>RequireHttpsCookie=true</c> → <see cref="Microsoft.AspNetCore.Authentication.Cookies.CookieSecurePolicy.Always"/>)
|
||||
/// and no explicit <c>MxGateway:Dashboard:CookieName</c> override is set. The <c>__Host-</c>
|
||||
/// prefix instructs browsers to enforce Secure, no <c>Domain</c>, and <c>Path=/</c>; those
|
||||
/// guarantees only hold for a Secure cookie, so this name must never be applied unless
|
||||
/// <see cref="Microsoft.AspNetCore.Authentication.Cookies.CookieSecurePolicy.Always"/> is in
|
||||
/// effect — a <c>__Host-</c> cookie without Secure is silently dropped by browsers.
|
||||
/// </summary>
|
||||
public const string SecureCookieName = "__Host-MxGatewayDashboard";
|
||||
}
|
||||
|
||||
@@ -123,13 +123,22 @@ public static class DashboardServiceCollectionExtensions
|
||||
? CookieSecurePolicy.Always
|
||||
: CookieSecurePolicy.SameAsRequest;
|
||||
|
||||
// Config-driven cookie name (MxGateway:Dashboard:CookieName). Null/blank keeps
|
||||
// the canonical default set above, so a misconfiguration cannot unname the cookie.
|
||||
// Config-driven cookie name (MxGateway:Dashboard:CookieName). An explicit override
|
||||
// always wins. With no override, restore the __Host- prefix when the cookie is
|
||||
// guaranteed Secure (RequireHttpsCookie true → SecurePolicy Always): the __Host-
|
||||
// browser guarantees (Secure required, no Domain, Path=/) hold only for a Secure
|
||||
// cookie, and a __Host- cookie without Secure is silently dropped — so the prefix
|
||||
// is never applied unless SecurePolicy is Always. Otherwise keep the plain
|
||||
// canonical default set by AddCookie above, so a misconfiguration cannot unname it.
|
||||
var cookieName = gatewayOptions.Value.Dashboard.CookieName;
|
||||
if (!string.IsNullOrWhiteSpace(cookieName))
|
||||
{
|
||||
cookieOptions.Cookie.Name = cookieName;
|
||||
}
|
||||
else if (cookieOptions.Cookie.SecurePolicy == CookieSecurePolicy.Always)
|
||||
{
|
||||
cookieOptions.Cookie.Name = DashboardAuthenticationDefaults.SecureCookieName;
|
||||
}
|
||||
});
|
||||
|
||||
services.AddAuthorization(authorization =>
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
using Microsoft.AspNetCore.SignalR;
|
||||
using Microsoft.Extensions.Options;
|
||||
using ZB.MOM.WW.MxGateway.Contracts.Proto;
|
||||
using ZB.MOM.WW.MxGateway.Server.Configuration;
|
||||
|
||||
namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs;
|
||||
|
||||
@@ -10,10 +12,22 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs;
|
||||
/// Errors are logged once and dropped — keeping the SignalR mirror best-effort
|
||||
/// preserves the gRPC contract that exists today.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// When <c>MxGateway:Dashboard:ShowTagValues</c> is false (the default), tag
|
||||
/// values are stripped from a redacted copy of the event before it reaches any
|
||||
/// dashboard client. The source <see cref="MxEvent"/> is shared with the gRPC
|
||||
/// event path and the reconnect replay ring, so it is never mutated in place —
|
||||
/// the redaction is applied to a deep clone. This closes the value-leak seam at
|
||||
/// the mirror independently of the still-outstanding per-session hub ACL
|
||||
/// (see <see cref="EventsHub"/>).
|
||||
/// </remarks>
|
||||
public sealed class DashboardEventBroadcaster(
|
||||
IHubContext<EventsHub> hubContext,
|
||||
IOptions<GatewayOptions> options,
|
||||
ILogger<DashboardEventBroadcaster> logger) : IDashboardEventBroadcaster
|
||||
{
|
||||
private readonly bool _showTagValues = options.Value.Dashboard.ShowTagValues;
|
||||
|
||||
/// <inheritdoc />
|
||||
public void Publish(string sessionId, MxEvent mxEvent)
|
||||
{
|
||||
@@ -22,6 +36,8 @@ public sealed class DashboardEventBroadcaster(
|
||||
return;
|
||||
}
|
||||
|
||||
MxEvent outbound = _showTagValues ? mxEvent : RedactValues(mxEvent);
|
||||
|
||||
// Wrap the Task acquisition in a try/catch so a hypothetical synchronous throw
|
||||
// from SendAsync (e.g. an implementation that throws before returning the Task)
|
||||
// cannot escape Publish. The interface contract is never-throw; fire-and-forget.
|
||||
@@ -30,7 +46,7 @@ public sealed class DashboardEventBroadcaster(
|
||||
{
|
||||
send = hubContext.Clients
|
||||
.Group(EventsHub.GroupName(sessionId))
|
||||
.SendAsync(EventsHub.EventMessage, mxEvent);
|
||||
.SendAsync(EventsHub.EventMessage, outbound);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
@@ -51,4 +67,27 @@ public sealed class DashboardEventBroadcaster(
|
||||
TaskScheduler.Default);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Produces a deep clone of <paramref name="source"/> with every tag-value
|
||||
/// field cleared, leaving tag reference, quality, status, and timestamps
|
||||
/// intact so the dashboard still renders the event without the value. The
|
||||
/// source event is left untouched because it is shared downstream with the
|
||||
/// gRPC stream and the replay ring.
|
||||
/// </summary>
|
||||
/// <param name="source">The source event to redact a copy of.</param>
|
||||
/// <returns>A redacted deep clone of the event.</returns>
|
||||
private static MxEvent RedactValues(MxEvent source)
|
||||
{
|
||||
MxEvent redacted = source.Clone();
|
||||
redacted.Value = null;
|
||||
|
||||
if (redacted.BodyCase == MxEvent.BodyOneofCase.OnAlarmTransition)
|
||||
{
|
||||
redacted.OnAlarmTransition.CurrentValue = null;
|
||||
redacted.OnAlarmTransition.LimitValue = null;
|
||||
}
|
||||
|
||||
return redacted;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -32,15 +32,19 @@ public sealed class EventsHub : Hub
|
||||
/// the dashboard roles (Admin or Viewer); both roles may subscribe to
|
||||
/// any session id they choose. This is acceptable today because (a) the
|
||||
/// dashboard's per-session views show non-secret session metadata that
|
||||
/// any authenticated dashboard user can already see, and (b) value
|
||||
/// logging in the source gRPC stream is gated by the same redaction
|
||||
/// policy that protects logs. The per-session ACL that gates the gRPC
|
||||
/// any authenticated dashboard user can already see, and (b) tag values
|
||||
/// are stripped from the mirrored events by
|
||||
/// <see cref="DashboardEventBroadcaster"/> when
|
||||
/// <c>MxGateway:Dashboard:ShowTagValues</c> is false (the default), so the
|
||||
/// most sensitive payload cannot leak through this seam regardless of the
|
||||
/// still-missing ACL. The per-session ACL that gates the gRPC
|
||||
/// <c>StreamEvents</c> RPC is intentionally not yet mirrored here.
|
||||
/// TODO(per-session-acl): once a role/scope is introduced that scopes a
|
||||
/// Viewer to a specific session or tenant, add a session-access check
|
||||
/// at this seam — either inline (consult the per-user allowed-session
|
||||
/// set on <c>Context.User</c> claims / <c>Context.Items</c>) or via a
|
||||
/// dedicated authorization policy applied to the hub method itself.
|
||||
/// TODO(per-session-acl): tracked as remediation roadmap item 12
|
||||
/// (SEC-25). Once a role/scope is introduced that scopes a Viewer to a
|
||||
/// specific session or tenant, add a session-access check at this seam —
|
||||
/// either inline (consult the per-user allowed-session set on
|
||||
/// <c>Context.User</c> claims / <c>Context.Items</c>) or via a dedicated
|
||||
/// authorization policy applied to the hub method itself.
|
||||
/// </remarks>
|
||||
/// <param name="sessionId">Session id to subscribe the caller to.</param>
|
||||
/// <returns>A task representing the subscription operation.</returns>
|
||||
|
||||
Reference in New Issue
Block a user