Repository navigation
Coordinate password login admission - #2601
Conversation
|
@ejsmith could you review this specific cache-counter behavior before we simplify With Foundatio 13.0.4, .NET 10 on macOS arm64, five isolated runs of 10,000
This reproduction uses only an in-memory cache, without application services or network access. The decompiled implementation reads and mutates the same existing Exact reproductionCreate a .NET 10 console project with 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. |
|
Reviewing Roughly 80% of the additions are tests, and security regression coverage is justified. The concern is the production scope and the custom throttling machinery.
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. |
f3425cb to
d1d1c99
Compare
niemyjski
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da2f75b130
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| private Task RemoveFailuresAsync(IEnumerable<KeyValuePair<string, string>> failures) | ||
| => Task.WhenAll(failures.Select(failure => _cache.RemoveIfEqualAsync(failure.Key, failure.Value))); |
There was a problem hiding this comment.
When concurrent successful logins observed the same prior failure, both call this removal; if a third login reserves that slot between the provider's comparison and deletion, the stale remover can delete the third login's reservation. The checked-in Foundatio 13.0.4 stack still has unresolved conditional-operation concurrency (upstream #570), while the new tests exercise only InMemoryCacheClient, so the five-user/fifteen-IP bounds are not established for the deployed provider. Use a provider-level atomic compare-and-delete primitive (or an equivalent lock) before relying on these entries for login admission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 474a079a83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| string normalizedEmailAddress = emailAddress.Trim().ToLowerInvariant(); | ||
|
|
||
| return Enumerable.Range(0, UserFailureLimit).Select(index => $"user:{normalizedEmailAddress}:attempts:{expiresUtc.Ticks}:{index}").ToArray(); |
There was a problem hiding this comment.
Keep the new slots interoperable with the previous user:{email}:attempts counter during migration. The production workflow in .github/workflows/build.yaml performs a normal Helm upgrade, and k8s/exceptionless/templates/api.yaml uses the default rolling Deployment strategy, so old and new API pods coexist while writing disjoint user and IP keys; during every rollout, a client can obtain both versions' independent budgets and roughly double the intended password checks.
Useful? React with 👍 / 👎.
f258e6b to
84b34ad
Compare
f541c2e to
69d4f36
Compare
|
Share password-login admission between interactive and Basic authentication, preserving API-key authentication and recovery behavior. Use configured test services and control completion order while requests overlap at the admission boundary. Handler tests live in ApiKeyAuthenticationHandlerTests.
Validation: 71/71 focused tests passed; all eight Basic cases passed again without retries. A separate localhost Redis probe passed account/IP limits and failure accounting across two independent clients, 80 rounds, and 65 requests per round. All hosted checks passed for
1d2dddd29: Build. AuthService coverage is 100% lines and 94.2% branches.Known limitation: in-memory concurrent writes remain tracked in #2655 and are accepted temporarily; the Foundatio upgrade stays separate. The pinned Redis provider uses atomic conditional operations. Redis-backed deployments do not require that upgrade for this change.
Performance: uncontended account-plus-IP admission uses four cache calls and completion four more. Conditional writes retry up to eight times. Basic requests poll every 50 ms for up to two seconds, with at most 100 waiters per service instance; provider I/O retains its own timeout. These are source-derived bounds, not measured throughput. Shared-IP budgets aggregate clients and may deny sustained bursts.
Rollout: temporary login counters reset on first deployment; mixed versions use separate budgets until rollout completes.
Verification details
Focused validation covers AuthServiceTests, ApiKeyAuthenticationHandlerTests, and HttpExtensionsTests. The Basic fixture tests overlapping admission with controlled completions; it does not claim simultaneous in-memory writes are fixed. Local Redis 8.6 used the installed Foundatio.Redis 13.0.4 package and production serializer. Its older-Redis Lua fallback was inspected, not runtime-tested. Live production configuration was not inspected; the checked-in Kubernetes defaults select Redis.