Skip to content

Coordinate password login admission - #2601

Draft
niemyjski wants to merge 1 commit into
mainfrom
feature/shared-auth-service
Draft

niemyjski wants to merge 1 commit into
mainfrom
feature/shared-auth-service

Conversation

@niemyjski

@niemyjski niemyjski commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Coordinates password login admission across interactive and Basic callers, preserving token aliases and quarter-hour windows.

Validation: 122 focused tests passed locally. Login windows restart at deployment. Hosted checks pending.

@niemyjski

Copy link
Copy Markdown
Member Author

@ejsmith could you review this specific cache-counter behavior before we simplify AuthService? I would prefer ordinary counters over retaining the bounded-marker workaround.

With Foundatio 13.0.4, .NET 10 on macOS arm64, five isolated runs of 10,000 InMemoryCacheClient.IncrementAsync(key, 1) calls at concurrency 16 produced:

Run Expected Actual
1 10,000 9,405
2 10,000 9,329
3 10,000 9,663
4 10,000 9,594
5 10,000 9,539

This reproduction uses only an in-memory cache, without application services or network access. The decompiled implementation reads and mutates the same existing CacheEntry inside the ConcurrentDictionary.AddOrUpdate callback. Concurrent callbacks can read the same value and overwrite each other's increments; that appears to explain the observed results. Redis counter behavior is not implicated by this reproduction.

Exact reproduction

Create a .NET 10 console project with <PackageReference Include="Foundatio" Version="13.0.4" /> and implicit usings enabled, then run:

using Foundatio.Caching;

using var cache = new InMemoryCacheClient();
const int incrementCount = 10000;
for (int run = 0; run < 5; run++)
{
    string key = $"counter-{run}";
    await cache.SetAsync(key, 0L);
    await Parallel.ForEachAsync(Enumerable.Range(0, incrementCount),
        new ParallelOptions { MaxDegreeOfParallelism = 16 },
        async (attempt, cancellationToken) => await cache.IncrementAsync(key, 1));
    long actual = (await cache.GetAsync<long>(key)).Value;
    Console.WriteLine($"Run {run + 1}: expected {incrementCount}, actual {actual}");
    if (actual != incrementCount)
        Environment.ExitCode = 1;
}

This PR remains draft because simply replacing the markers with the current in-memory counter implementation would lose concurrent failures. The dependency should provide the atomic counter behavior so authentication can stay simple.

@ejsmith

ejsmith commented Sep 29, 2026

Copy link
Copy Markdown
Member

Reviewing 3cd7d986e: this PR is broader and more complicated than the Basic-auth password-guessing fix requires. Several changes are worthwhile, but I recommend splitting them up.

Roughly 80% of the additions are tests, and security regression coverage is justified. The concern is the production scope and the custom throttling machinery.

  • Keep: one shared password-attempt limiter used by normal login and Basic auth, consistent email normalization, account/IP limits, existing recovery integration, and compatibility with Basic API-key credentials.
  • Separate: OAuth authorization-code redemption, outbound address filtering, source-map changes, webhook validation/logging, and broader inactive-account enforcement. Those deserve their own focused security changes.
  • Simplify: the five account markers, fifteen IP markers, GUID values, captured failure snapshots, and conditional cleanup. That machinery works around the Foundatio counter bug, yet still leaves a parallel-guessing gap.

For the concurrency concern, a local reproduction using the actual Basic-auth handler with delayed repository responses admitted 100 requests before completion. It evaluated 99 incorrect passwords, and the final correct guess authenticated even after new requests were blocked. Admission needs to account for password checks already underway; the existing tests establish eventual blocking after failures complete.

The Foundatio counter bug is real and was also reproduced locally, so simply replacing the markers with today's counters would be incorrect. Fix and test that underlying primitive separately, then use a small limiter with explicit handling of in-flight verification. Atomic counters alone do not solve the admission race.

There is necessary complexity around concurrency, successful requests, and recovery. However, requiring successful authentication to perform no cache writes or distributed locking makes the design harder. That optimization should have a measured need, and successful requests should not consume failure quota.

I recommend narrowing this PR to the limiter, its two authentication callers, recovery wiring, and targeted regression tests. That should make the change substantially smaller and easier to establish as correct.

@niemyjski
niemyjski force-pushed the feature/shared-auth-service branch from 3cd7d98 to f3425cb Compare September 29, 2026 15:21
@niemyjski
niemyjski force-pushed the feature/shared-auth-service branch from f3425cb to d1d1c99 Compare October 2, 2026 03:18
@niemyjski niemyjski changed the title Normalize authentication through a shared AuthService Coordinate password login admission Oct 2, 2026
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Code Coverage

Package Line Rate Branch Rate Complexity Health
Exceptionless.AppHost 22% 23% 141 ❌
Exceptionless.Core 77% 68% 10853 ✔
Exceptionless.Insulation 51% 43% 370 ➖
Exceptionless.Web 86% 70% 9149 ✔
Summary 80% (27887 / 34961) 69% (13759 / 20074) 20513 ✔

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants