fix(secrets): rate-limited denial warning on the Grpc hub (live-gate finding) — 0.4.1
The ScadaBridge live gate (check 4) found SecretsHubAuthInterceptor denying correctly but logging nothing: the only trace of a refusal was a framework Grpc.AspNetCore.Server Information line. A follower whose bearer token was mis-rotated therefore stopped converging silently, with the hub showing it only at INF. Every denial is now counted and summarised in a Warning emitted on the first denial after startup or after a 60-second quiet window, carrying the denials since the previous summary plus a breakdown by cause. A single probe is visible immediately; a sustained flood is bounded to one line per window, which is what makes warning safe at all — the endpoint is unauthenticated by construction, so a per-denial warning would be an unbounded log write driven by an unauthenticated caller. Counters are Interlocked and the emit is decided by a compare-exchange on the last-emit timestamp, so exactly one thread emits per window and a denial counted after the drain carries into the next summary rather than being lost. The cause breakdown is server-side diagnostics only. Denied() is untouched: every rejection still returns the identical uniform Unauthenticated status, the comparison is still constant-time, and no token material appears in any log line. The existing warn-once-on-unconfigured-token signal is unchanged and still emitted exactly once; denials in that state count toward the new summary's "unconfigured" category. TimeProvider is an optional default-valued parameter rather than a second overload — the type must keep exactly one public constructor or the gRPC interceptor activator silently stops invoking the auth gate. Two existing tests scoped their assertions to the configuration warning, since both kinds now share a category: the new line is deliberate behavior, not drift. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1
This commit is contained in:
+140
-5
@@ -24,6 +24,18 @@ namespace ZB.MOM.WW.Secrets.Replicator.Grpc;
|
||||
/// "wrong token" would hand an attacker a free oracle.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Every denial is counted, and a summary is warned at most once per
|
||||
/// <see cref="DenialWarningWindowSeconds"/> seconds.</b> A refusal that logs nothing is a follower
|
||||
/// that silently stops converging — a mis-rotated token showed up on the hub only as a framework
|
||||
/// Information line. The rate limit is what makes warning at all safe: this endpoint is
|
||||
/// unauthenticated by definition, so a per-denial warning would be an unbounded log write driven
|
||||
/// by an unauthenticated caller.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The summary's category breakdown is <b>server-side diagnostics only</b>. It never reaches the
|
||||
/// wire — the response stays the one uniform status regardless of category.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>No token material is ever logged</b>, presented or configured, at any level.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
@@ -42,12 +54,33 @@ public sealed class SecretsHubAuthInterceptor : Interceptor
|
||||
/// <summary>The one detail every denial carries. It says nothing about why.</summary>
|
||||
private const string DenialDetail = "Unauthenticated.";
|
||||
|
||||
/// <summary>
|
||||
/// How long one denial summary covers. Deliberately a constant and not a configuration key:
|
||||
/// there is no deployment for which a different value is right, and an operator-tunable log
|
||||
/// rate on an unauthenticated endpoint is a knob whose only interesting setting is a dangerous
|
||||
/// one.
|
||||
/// </summary>
|
||||
private const int DenialWarningWindowSeconds = 60;
|
||||
|
||||
private static readonly long DenialWarningWindowTicks =
|
||||
TimeSpan.FromSeconds(DenialWarningWindowSeconds).Ticks;
|
||||
|
||||
private readonly byte[] _expectedTokenUtf8;
|
||||
private readonly string _bearerTokenConfigurationKey;
|
||||
private readonly ILogger<SecretsHubAuthInterceptor> _logger;
|
||||
private readonly TimeProvider _timeProvider;
|
||||
|
||||
private int _unconfiguredWarningEmitted;
|
||||
|
||||
// Denials arrive concurrently from any number of callers, so every counter is touched through
|
||||
// Interlocked and the emit is decided by a single compare-exchange on the last-emit timestamp.
|
||||
// Zero means "never emitted", which is why the very first denial always wins the window.
|
||||
private long _lastDenialWarningUtcTicks;
|
||||
private long _denialsSinceLastWarning;
|
||||
private long _missingOrMalformedDenials;
|
||||
private long _wrongCredentialDenials;
|
||||
private long _unconfiguredDenials;
|
||||
|
||||
/// <summary>
|
||||
/// Creates the interceptor over the hub's configured token.
|
||||
/// </summary>
|
||||
@@ -59,11 +92,20 @@ public sealed class SecretsHubAuthInterceptor : Interceptor
|
||||
/// </remarks>
|
||||
/// <param name="options">The hub options carrying the shared bearer token.</param>
|
||||
/// <param name="logger">Diagnostics sink. Never receives token material.</param>
|
||||
/// <param name="timeProvider">
|
||||
/// Clock behind the denial-warning rate limit. Optional — a default-valued parameter rather than
|
||||
/// a second overload, because the single-public-constructor rule above admits no overload; both
|
||||
/// the container's activator and <c>ActivatorUtilities</c> honour the default when nothing is
|
||||
/// registered, so a host that registers a <see cref="TimeProvider"/> gets its own and one that
|
||||
/// does not gets <see cref="TimeProvider.System"/>.
|
||||
/// </param>
|
||||
public SecretsHubAuthInterceptor(
|
||||
IOptions<SecretsGrpcHubServerOptions> options,
|
||||
ILogger<SecretsHubAuthInterceptor> logger)
|
||||
ILogger<SecretsHubAuthInterceptor> logger,
|
||||
TimeProvider? timeProvider = null)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(options);
|
||||
_timeProvider = timeProvider ?? TimeProvider.System;
|
||||
|
||||
SecretsGrpcHubServerOptions value = options.Value;
|
||||
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
|
||||
@@ -135,7 +177,7 @@ public sealed class SecretsHubAuthInterceptor : Interceptor
|
||||
if (_expectedTokenUtf8.Length == 0)
|
||||
{
|
||||
WarnUnconfiguredOnce();
|
||||
throw Denied();
|
||||
throw DeniedAs(DenialCategory.Unconfigured);
|
||||
}
|
||||
|
||||
Metadata headers = context.RequestHeaders;
|
||||
@@ -157,12 +199,12 @@ public sealed class SecretsHubAuthInterceptor : Interceptor
|
||||
|
||||
if (authorizationHeaderCount != 1 || presented is null)
|
||||
{
|
||||
throw Denied();
|
||||
throw DeniedAs(DenialCategory.MissingOrMalformed);
|
||||
}
|
||||
|
||||
if (!presented.StartsWith(BearerScheme, StringComparison.Ordinal))
|
||||
{
|
||||
throw Denied();
|
||||
throw DeniedAs(DenialCategory.MissingOrMalformed);
|
||||
}
|
||||
|
||||
// FixedTimeEquals compares in time independent of the contents, and returns false for a
|
||||
@@ -171,7 +213,7 @@ public sealed class SecretsHubAuthInterceptor : Interceptor
|
||||
byte[] presentedTokenUtf8 = Encoding.UTF8.GetBytes(presented[BearerScheme.Length..]);
|
||||
if (!CryptographicOperations.FixedTimeEquals(presentedTokenUtf8, _expectedTokenUtf8))
|
||||
{
|
||||
throw Denied();
|
||||
throw DeniedAs(DenialCategory.WrongCredential);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -181,6 +223,99 @@ public sealed class SecretsHubAuthInterceptor : Interceptor
|
||||
/// </summary>
|
||||
private static RpcException Denied() => new(new Status(StatusCode.Unauthenticated, DenialDetail));
|
||||
|
||||
/// <summary>
|
||||
/// Records a denial in <paramref name="category"/>, emits the window's summary if this call is
|
||||
/// the one that opened a new window, and returns the same uniform denial every path throws.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The category only ever reaches the local log. <see cref="Denied"/> is called unchanged, so
|
||||
/// the status a caller receives is byte-identical whatever brought it here — bookkeeping runs
|
||||
/// strictly after the decision and cannot alter it.
|
||||
/// </remarks>
|
||||
private RpcException DeniedAs(DenialCategory category)
|
||||
{
|
||||
switch (category)
|
||||
{
|
||||
case DenialCategory.MissingOrMalformed:
|
||||
Interlocked.Increment(ref _missingOrMalformedDenials);
|
||||
break;
|
||||
case DenialCategory.WrongCredential:
|
||||
Interlocked.Increment(ref _wrongCredentialDenials);
|
||||
break;
|
||||
default:
|
||||
Interlocked.Increment(ref _unconfiguredDenials);
|
||||
break;
|
||||
}
|
||||
|
||||
Interlocked.Increment(ref _denialsSinceLastWarning);
|
||||
WarnDeniedIfWindowElapsed();
|
||||
|
||||
return Denied();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Emits one summary per window, drained of everything counted since the previous one.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The first denial after startup — and the first after any quiet period longer than the window
|
||||
/// — warns immediately, so a single probe is visible rather than being averaged away.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Exactly one thread emits per window: the compare-exchange publishes the new window start
|
||||
/// only for the caller that observed the old one, and every loser returns without logging. The
|
||||
/// counters are then drained with <see cref="Interlocked.Exchange(ref long, long)"/>, so a
|
||||
/// denial counted a moment after the drain is carried into the NEXT summary rather than lost.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private void WarnDeniedIfWindowElapsed()
|
||||
{
|
||||
long now = _timeProvider.GetUtcNow().UtcTicks;
|
||||
long lastEmitted = Volatile.Read(ref _lastDenialWarningUtcTicks);
|
||||
|
||||
if (now - lastEmitted < DenialWarningWindowTicks)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
if (Interlocked.CompareExchange(ref _lastDenialWarningUtcTicks, now, lastEmitted) != lastEmitted)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
long total = Interlocked.Exchange(ref _denialsSinceLastWarning, 0);
|
||||
long missingOrMalformed = Interlocked.Exchange(ref _missingOrMalformedDenials, 0);
|
||||
long wrongCredential = Interlocked.Exchange(ref _wrongCredentialDenials, 0);
|
||||
long unconfigured = Interlocked.Exchange(ref _unconfiguredDenials, 0);
|
||||
|
||||
_logger.LogWarning(
|
||||
"The gRPC secrets hub denied {DeniedCount} call(s) since the previous such warning " +
|
||||
"(missing or malformed credential: {MissingOrMalformedCount}; wrong credential: " +
|
||||
"{WrongCredentialCount}; hub token unconfigured: {UnconfiguredCount}). Denials are " +
|
||||
"summarised at most once every {DenialWarningWindowSeconds}s.",
|
||||
total,
|
||||
missingOrMalformed,
|
||||
wrongCredential,
|
||||
unconfigured,
|
||||
DenialWarningWindowSeconds);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Why a denial happened, for the local summary only. This never reaches a caller — telling one
|
||||
/// apart from another on the wire is exactly the oracle the uniform denial exists to withhold.
|
||||
/// </summary>
|
||||
private enum DenialCategory
|
||||
{
|
||||
/// <summary>No <c>Authorization</c> header, more than one, or one that is not a bearer.</summary>
|
||||
MissingOrMalformed,
|
||||
|
||||
/// <summary>A well-formed bearer credential that is not the configured one.</summary>
|
||||
WrongCredential,
|
||||
|
||||
/// <summary>The hub has no token configured, so nothing could have been accepted.</summary>
|
||||
Unconfigured,
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Warns once — not once per call — that the hub cannot serve, naming the exact key to set. A
|
||||
/// per-call warning would bury the one line that matters under the flood it describes.
|
||||
|
||||
Reference in New Issue
Block a user