fix(transport): enforce the documented per-IP bundle-unlock throttle (row 44)

BundleUnlockRateLimiter was registered (ServiceCollectionExtensions), injected
into BundleImporter, unit-tested, documented on
TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10 and pinned in
appsettings.Central.json — but _unlockRateLimiter was never invoked. A
brute-force passphrase attacker against a stolen bundle was bounded only by the
per-bundle 3-strike lockout, which a re-export side-steps (new ContentHash).

LoadAsync now registers every unlock attempt against the limiter. Ordering is
load-bearing: AFTER the per-bundle lockout check (hammering a locked bundle
spends no hourly budget) and BEFORE the decrypt (so a denial burns no per-bundle
strike — otherwise the throttle would be a way to lock a bundle someone is
legitimately importing). Over budget throws the new
BundleUnlockThrottledException, fail-closed with a clear message on both
surfaces; it lives in Commons because the management surface (Commons-only
references) must catch it to return a curated error rather than MapFault's
generic internal-error text.

Client key plumbing:
- Central UI: new scoped CircuitClientAddressProvider snapshots
  Connection.RemoteIpAddress at circuit establishment — the same constraint and
  the same pattern as CookieAuthenticationStateProvider, since HttpContext is
  null by the time a circuit submits a passphrase.
- CLI/management: additive ManagementEnvelope.ClientIp, stamped by
  ManagementEndpoints and threaded through DispatchCommand to the preview/import
  bundle handlers.
- No attributable address => a single SHARED "<unattributed>" bucket, not a
  bypass: unknown clients get a stricter budget, never none. Per-client
  isolation is only real behind a ForwardedHeaders-trusted proxy, the same
  caveat the login throttle carries (Component-Security.md).

Docs: Component-Transport.md + docs/components/Transport.md record the two-level
ordering, the client-key sourcing, the shared-bucket fallback and a
troubleshooting entry for the new error.

Tests: 6 importer tests (cap enforced; denial != passphrase failure, no strike
burned; per-client isolation; shared unattributed bucket; unencrypted bundles
consume no budget; a locked bundle spends no hourly budget) — 4 of the 6 fail
with the limiter call disabled, i.e. against the dead-code state. Plus
ManagementActor tests pinning the ClientIp forwarding and the curated
throttle error.
This commit is contained in:
Joseph Doherty
2026-08-15 06:01:14 -04:00
parent 708389829b
commit 5ab26a4233
16 changed files with 545 additions and 24 deletions
@@ -165,8 +165,22 @@ public sealed class BundleImporter : IBundleImporter
_logger = logger;
}
/// <summary>
/// T-004 / register row 44: the rate-limiter bucket used when a caller supplies no
/// client key. Deliberately a single SHARED key rather than a per-call bypass — an
/// unattributable caller is throttled together with every other unattributable
/// caller, so "no IP available" degrades to a stricter budget, never to none. A
/// production unlock always carries an IP (Central UI circuit, management endpoint),
/// so in practice this bucket only serves in-process callers and tests.
/// </summary>
internal const string UnattributedClientKey = "<unattributed>";
/// <inheritdoc />
public async Task<BundleSession> LoadAsync(Stream bundleStream, string? passphrase, CancellationToken ct = default)
public async Task<BundleSession> LoadAsync(
Stream bundleStream,
string? passphrase,
CancellationToken ct = default,
string? clientKey = null)
{
ArgumentNullException.ThrowIfNull(bundleStream);
@@ -264,6 +278,33 @@ public sealed class BundleImporter : IBundleImporter
throw new BundleLockedException(manifest.ContentHash, priorFailures);
}
// T-004 / register row 44: the documented per-IP-per-hour cap. The limiter
// was registered, injected and unit-tested but never invoked, so a
// brute-force attacker against a stolen bundle was bounded only by the
// per-bundle 3-strike lockout (side-steppable by re-exporting the bundle
// for a fresh ContentHash).
//
// Ordering is load-bearing:
// * AFTER the per-bundle lockout check, so hammering an already-locked
// bundle does not spend the client's hourly budget on rejected calls.
// * BEFORE the decrypt, so a THROTTLED attempt never reaches
// IncrementUnlockFailureCount — a denial must not burn a per-bundle
// strike, or the throttle would become a way to lock a bundle that an
// operator is legitimately importing.
// Registered on every unlock attempt (successful ones included) because the
// documented budget counts attempts, not failures.
var throttleKey = string.IsNullOrWhiteSpace(clientKey)
? UnattributedClientKey
: clientKey;
var perIpCap = _options.Value.MaxUnlockAttemptsPerIpPerHour;
if (!_unlockRateLimiter.TryRegisterAttempt(throttleKey, perIpCap))
{
_logger?.LogWarning(
"Bundle unlock throttled: client {ClientKey} exceeded {Cap} attempts in the trailing hour.",
throttleKey, perIpCap);
throw new BundleUnlockThrottledException(throttleKey, perIpCap);
}
var aad = Encryption.BundleManifestAad.Compute(manifest);
try
{