diff --git a/Projects/UOContent.Tests/Tests/Accounting/PasswordVerificationTests.cs b/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs similarity index 60% rename from Projects/UOContent.Tests/Tests/Accounting/PasswordVerificationTests.cs rename to Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs index efd506bb3..b42eddd8f 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/PasswordVerificationTests.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs @@ -6,26 +6,26 @@ using Xunit; namespace Server.Tests.Accounting; [Collection("Sequential UOContent Tests")] -public class PasswordVerificationTests : IDisposable +public class PasswordWorkerTests : IDisposable { private const string Password = "hunter2"; private readonly PasswordProtectionAlgorithm _originalAlgorithm = AccountSecurity.CurrentAlgorithm; - public PasswordVerificationTests() => AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + public PasswordWorkerTests() => AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; public void Dispose() => AccountSecurity.CurrentAlgorithm = _originalAlgorithm; private static Account CreateAccount(string username) => Accounts.GetAccount(username) as Account ?? new Account(username, Password); - private static PasswordVerificationJob JobFor(Account account, string submitted) => + private static PasswordJob JobFor(Account account, string submitted) => new() { Account = account, StoredHash = account.Password, VerifyPhrase = account.GetVerifyPhrase(submitted), - RehashPhrase = account.NeedsPasswordUpgrade() ? account.GetRehashPhrase(submitted) : null, + HashPhrase = account.NeedsPasswordUpgrade() ? account.GetRehashPhrase(submitted) : null, TargetAlgorithm = AccountSecurity.CurrentAlgorithm }; @@ -34,7 +34,7 @@ public class PasswordVerificationTests : IDisposable { var account = CreateAccount("offloop-correct-user"); - var outcome = PasswordVerificationWorker.ComputeInline(JobFor(account, Password)); + var outcome = PasswordWorker.ComputeInline(JobFor(account, Password)); Assert.True(outcome.Verified); } @@ -44,10 +44,10 @@ public class PasswordVerificationTests : IDisposable { var account = CreateAccount("offloop-wrong-user"); - var outcome = PasswordVerificationWorker.ComputeInline(JobFor(account, "not-the-password")); + var outcome = PasswordWorker.ComputeInline(JobFor(account, "not-the-password")); Assert.False(outcome.Verified); - Assert.Null(outcome.UpgradedPassword); + Assert.Null(outcome.Hash); } [Fact] @@ -55,10 +55,10 @@ public class PasswordVerificationTests : IDisposable { var account = CreateAccount("offloop-current-user"); - var outcome = PasswordVerificationWorker.ComputeInline(JobFor(account, Password)); + var outcome = PasswordWorker.ComputeInline(JobFor(account, Password)); Assert.True(outcome.Verified); - Assert.Null(outcome.UpgradedPassword); + Assert.Null(outcome.Hash); } [Fact] @@ -70,10 +70,10 @@ public class PasswordVerificationTests : IDisposable account.Password = "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; - var outcome = PasswordVerificationWorker.ComputeInline(JobFor(account, Password)); + var outcome = PasswordWorker.ComputeInline(JobFor(account, Password)); Assert.True(outcome.Verified); - Assert.StartsWith("$argon2id$v=19$m=16384,t=1,p=1$", outcome.UpgradedPassword); + Assert.StartsWith("$argon2id$v=19$m=16384,t=1,p=1$", outcome.Hash); } [Fact] @@ -83,50 +83,70 @@ public class PasswordVerificationTests : IDisposable account.Password = "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; - var outcome = PasswordVerificationWorker.ComputeInline(JobFor(account, "not-the-password")); + var outcome = PasswordWorker.ComputeInline(JobFor(account, "not-the-password")); Assert.False(outcome.Verified); - Assert.Null(outcome.UpgradedPassword); + Assert.Null(outcome.Hash); } [Fact] - public void AppliesAnUpgradeWhenThePasswordIsUnchanged() + public void AppliesAWriteWhenNothingNewerWasRequested() { var account = CreateAccount("offloop-apply-user"); - var stored = account.Password; + var sequence = account.BeginPasswordWrite(); var upgraded = Argon2PasswordProtection.Instance.EncryptPassword(Password); - account.ApplyPasswordUpgrade(stored, upgraded, PasswordProtectionAlgorithm.Argon2); + Assert.True(account.ApplyPasswordWrite(sequence, upgraded, PasswordProtectionAlgorithm.Argon2)); Assert.Equal(upgraded, account.Password); Assert.True(account.CheckPassword(Password)); } /// - /// The verify runs off-loop for ~9 ms. A password changed in that window was already written - /// with current parameters; applying the stale upgrade would replace it with a hash of the - /// previous password and lock the account out. + /// A rehash derived off-loop must not land on a password set while it ran, or the account is + /// locked to a hash of the credential that one superseded. /// [Fact] - public void DropsAnUpgradeWhenThePasswordChangedMeanwhile() + public void DropsAWriteSupersededByAnInlineSetPassword() { var account = CreateAccount("offloop-stale-apply-user"); - var storedAtDispatch = account.Password; - // Derived from the old password, as the worker would have. + var sequence = account.BeginPasswordWrite(); var upgraded = Argon2PasswordProtection.Instance.EncryptPassword(Password); - // ...and the password changes before the verdict lands. account.SetPassword("a-brand-new-password"); var afterChange = account.Password; - account.ApplyPasswordUpgrade(storedAtDispatch, upgraded, PasswordProtectionAlgorithm.Argon2); - + Assert.False(account.ApplyPasswordWrite(sequence, upgraded, PasswordProtectionAlgorithm.Argon2)); Assert.Equal(afterChange, account.Password); Assert.True(account.CheckPassword("a-brand-new-password")); Assert.False(account.CheckPassword(Password)); } + /// + /// Two changes dispatched before either lands: the newer must win regardless of the order the + /// results come back in. Comparing stored hashes instead of sequences would drop the second and + /// silently keep the older password. + /// + [Fact] + public void TheNewestWriteWinsWhateverOrderResultsLand() + { + var account = CreateAccount("offloop-two-writes-user"); + + var first = account.BeginPasswordWrite(); + var firstHash = Argon2PasswordProtection.Instance.EncryptPassword("first-new-password"); + + var second = account.BeginPasswordWrite(); + var secondHash = Argon2PasswordProtection.Instance.EncryptPassword("second-new-password"); + + // Results land out of order. + Assert.True(account.ApplyPasswordWrite(second, secondHash, PasswordProtectionAlgorithm.Argon2)); + Assert.False(account.ApplyPasswordWrite(first, firstHash, PasswordProtectionAlgorithm.Argon2)); + + Assert.True(account.CheckPassword("second-new-password")); + Assert.False(account.CheckPassword("first-new-password")); + } + [Theory] [InlineData(PasswordProtectionAlgorithm.SHA1)] [InlineData(PasswordProtectionAlgorithm.SHA2)] diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index a81b75d1a..3c506c701 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -376,14 +376,25 @@ public partial class Account : IAccount, IComparable return true; } + // Runtime only, never serialized. Orders password writes so one derived off the loop cannot + // land on top of a newer one. Bumped by every write, deferred or not. + private int _passwordSequence; + public void SetPassword(string plainPassword) { + _passwordSequence++; PasswordAlgorithm = AccountSecurity.CurrentAlgorithm; Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword( AccountSecurity.DerivePhrase(PasswordAlgorithm, _username, plainPassword) ); } + /// + /// Claims the next write slot. Taken on the loop at dispatch, so a hash derived off it can tell + /// whether anything newer was requested while it ran. + /// + internal int BeginPasswordWrite() => ++_passwordSequence; + /// The phrase that verifies against the currently stored hash. internal string GetVerifyPhrase(string plainPassword) => AccountSecurity.DerivePhrase(_passwordAlgorithm, _username, plainPassword); @@ -401,25 +412,27 @@ public partial class Account : IAccount, IComparable AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password); /// - /// Applies a rehash derived off the game loop. Distinct from the private - /// UpgradePassword below, which adopts a legacy hash during RunUO/ServUO import. + /// Applies a hash derived off the game loop, unless something newer was requested while it ran. + /// Distinct from the private UpgradePassword below, which adopts a legacy hash during + /// RunUO/ServUO import. + /// + /// Ordering by sequence rather than by comparing the stored hash: both a login rehash and an + /// explicit change need "newest wins", and comparing hashes would silently drop the second of + /// two changes dispatched before either landed. /// - /// - /// The hash verification started from. If the password changed while it ran, the newer value - /// was already written with current parameters, so the stale upgrade is dropped rather than - /// replacing a live credential with a hash of the password it superseded. - /// - internal void ApplyPasswordUpgrade( - string expectedCurrent, string newEncrypted, PasswordProtectionAlgorithm algorithm + /// False when a newer write superseded this one. + internal bool ApplyPasswordWrite( + int sequence, string newEncrypted, PasswordProtectionAlgorithm algorithm ) { - if (!string.Equals(Password, expectedCurrent, StringComparison.Ordinal)) + if (sequence != _passwordSequence) { - return; + return false; } PasswordAlgorithm = algorithm; Password = newEncrypted; + return true; } public bool CheckPassword(string plainPassword) diff --git a/Projects/UOContent/Accounting/AccountHandler.cs b/Projects/UOContent/Accounting/AccountHandler.cs index 62fc9e8a6..6c2f9f53a 100644 --- a/Projects/UOContent/Accounting/AccountHandler.cs +++ b/Projects/UOContent/Accounting/AccountHandler.cs @@ -140,8 +140,13 @@ public static class AccountHandler if (accessList[0].MatchClassC(ipAddress)) { - acct.SetPassword(pass); - from.SendMessage("The password to your account has changed."); + // Confirmed from the callback: off the loop the hash has not landed yet when + // this returns. + PasswordWorker.SetPassword( + acct, + pass, + _ => from.SendMessage("The password to your account has changed.") + ); } else { @@ -396,24 +401,29 @@ public static class AccountHandler /// private static PasswordCheckDispatch DispatchPasswordCheck(AccountLoginEventArgs e, Account acct, string pw) { - if (!PasswordVerificationWorker.Enabled || + if (!PasswordWorker.Enabled || AccountSecurity.CurrentAlgorithm != PasswordProtectionAlgorithm.Argon2 || acct.PasswordAlgorithm != PasswordProtectionAlgorithm.Argon2) { return PasswordCheckDispatch.Inline; } - var job = new PasswordVerificationJob + var needsUpgrade = acct.NeedsPasswordUpgrade(); + + var job = new PasswordJob { Account = acct, State = e.State, StoredHash = acct.Password, VerifyPhrase = acct.GetVerifyPhrase(pw), - RehashPhrase = acct.NeedsPasswordUpgrade() ? acct.GetRehashPhrase(pw) : null, - TargetAlgorithm = AccountSecurity.CurrentAlgorithm + HashPhrase = needsUpgrade ? acct.GetRehashPhrase(pw) : null, + TargetAlgorithm = AccountSecurity.CurrentAlgorithm, + Sequence = needsUpgrade ? acct.BeginPasswordWrite() : 0, + OnComplete = static (j, outcome) => + CompleteDeferredAccountLogin(j.State, j.Account, outcome.Verified) }; - return PasswordVerificationWorker.TryEnqueue(job) + return PasswordWorker.TryEnqueue(job) ? PasswordCheckDispatch.Deferred : PasswordCheckDispatch.Saturated; } diff --git a/Projects/UOContent/Accounting/Security/AccountSecurity.cs b/Projects/UOContent/Accounting/Security/AccountSecurity.cs index 2a4385043..deae4c8e0 100644 --- a/Projects/UOContent/Accounting/Security/AccountSecurity.cs +++ b/Projects/UOContent/Accounting/Security/AccountSecurity.cs @@ -57,10 +57,8 @@ public static class AccountSecurity /// with, and a rehash with the one it is moving to -- deriving with the wrong one produces a /// hash that verifies once and never again. /// - public static string DerivePhrase( - PasswordProtectionAlgorithm algorithm, string username, string plainPassword - ) => - algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 + public static string DerivePhrase(PasswordProtectionAlgorithm algorithm, string username, string plainPassword) + => algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 ? $"{username}{plainPassword}" : plainPassword; diff --git a/Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs b/Projects/UOContent/Accounting/Security/PasswordWorker.cs similarity index 60% rename from Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs rename to Projects/UOContent/Accounting/Security/PasswordWorker.cs index 1f52d39b6..e2f251659 100644 --- a/Projects/UOContent/Accounting/Security/PasswordVerificationWorker.cs +++ b/Projects/UOContent/Accounting/Security/PasswordWorker.cs @@ -2,7 +2,7 @@ * ModernUO * * Copyright 2019-2026 - ModernUO Development Team * * Email: hi@modernuo.com * - * File: PasswordVerificationWorker.cs * + * File: PasswordWorker.cs * * * * This program is free software: you can redistribute it and/or modify * * it under the terms of the GNU General Public License as published by * @@ -23,37 +23,51 @@ using Server.Network; namespace Server.Accounting.Security; /// -/// Work handed to the verification thread. Strings and references it only carries: the worker -/// reads no game state and writes none. +/// Work handed to the password thread. Strings and references it only carries: the worker reads no +/// game state and writes none. +/// +/// Either half is optional, which is what lets one job type serve both callers. A login verifies +/// and may rehash; an explicit password change only hashes. /// -internal sealed class PasswordVerificationJob +internal sealed class PasswordJob { public Account Account; + + /// Ties the job to a connection. Null when the work is not gated on one, such as a + /// password change by an admin. public NetState State; - /// The hash at dispatch. Also guards the upgrade against a password change landing - /// while this runs. + /// Hash to verify against, with . public string StoredHash; + /// Phrase to verify, or null to skip verification. public string VerifyPhrase; - /// Phrase to rehash from, or null when no upgrade is due. - public string RehashPhrase; + /// Phrase to hash, or null when nothing needs writing. + public string HashPhrase; public PasswordProtectionAlgorithm TargetAlgorithm; + + /// Write slot claimed at dispatch, checked by + /// . + public int Sequence; + + /// Runs on the game loop with the result. Free to touch game state. + public Action OnComplete; } -internal readonly struct PasswordVerificationOutcome +internal readonly struct PasswordOutcome { + /// True when no verification was asked for, or it succeeded. public readonly bool Verified; - /// The new hash, or null when the password did not verify or needed no upgrade. - public readonly string UpgradedPassword; + /// The derived hash, or null when nothing was hashed or verification failed. + public readonly string Hash; - public PasswordVerificationOutcome(bool verified, string upgradedPassword) + public PasswordOutcome(bool verified, string hash) { Verified = verified; - UpgradedPassword = upgradedPassword; + Hash = hash; } } @@ -67,9 +81,9 @@ internal readonly struct PasswordVerificationOutcome /// /// ~110 verifies/sec, which is ample: login latency is not a concern, only loop time. /// -internal sealed class PasswordVerificationWorker +internal sealed class PasswordWorker { - private static readonly ILogger logger = LogFactory.GetLogger(typeof(PasswordVerificationWorker)); + private static readonly ILogger logger = LogFactory.GetLogger(typeof(PasswordWorker)); /// /// Backstop, not a flood defence. SentFirstPacket holds a connection to one pending @@ -84,7 +98,7 @@ internal sealed class PasswordVerificationWorker // only while a save is in progress, never in steady state. private const int SaveGatePollMs = 50; - private static PasswordVerificationWorker _instance; + private static PasswordWorker _instance; // Needs a spare core to move work to, which a 1-2 core host does not have. Off in DEBUG: // dev boxes and test shards have few logins and are better served by the simpler path. @@ -97,7 +111,7 @@ internal sealed class PasswordVerificationWorker private readonly Thread _thread; private readonly AutoResetEvent _work = new(false); - private readonly ConcurrentQueue _queue = new(); + private readonly ConcurrentQueue _queue = new(); // Its own Argon2: verification is static-backed and safe to share, hashing draws from a // per-instance RNG and is not. @@ -106,7 +120,7 @@ internal sealed class PasswordVerificationWorker private int _pending; private volatile bool _exit; - private PasswordVerificationWorker() + private PasswordWorker() { _thread = new Thread(Execute) { @@ -118,7 +132,7 @@ internal sealed class PasswordVerificationWorker } // Created on first use, so a shard that never takes the off-loop path never allocates a thread. - private static PasswordVerificationWorker Instance => _instance ??= new PasswordVerificationWorker(); + private static PasswordWorker Instance => _instance ??= new PasswordWorker(); internal static int Pending => _instance?._pending ?? 0; @@ -126,9 +140,9 @@ internal sealed class PasswordVerificationWorker /// Queues a job. False when the queue is full, in which case the caller must reject the login /// without verifying. /// - internal static bool TryEnqueue(PasswordVerificationJob job) => Instance.TryEnqueueCore(job); + internal static bool TryEnqueue(PasswordJob job) => Instance.TryEnqueueCore(job); - private bool TryEnqueueCore(PasswordVerificationJob job) + private bool TryEnqueueCore(PasswordJob job) { if (Volatile.Read(ref _pending) >= MaxPending) { @@ -183,7 +197,7 @@ internal sealed class PasswordVerificationWorker continue; } - PasswordVerificationOutcome outcome; + PasswordOutcome outcome; try { @@ -193,46 +207,80 @@ internal sealed class PasswordVerificationWorker { // A verdict must still come back, or the connection never gets a reply. logger.Error(ex, "Password verification failed for {Username}", job.Account?.Username); - outcome = new PasswordVerificationOutcome(false, null); + outcome = new PasswordOutcome(false, null); } Core.LoopContext.Post(() => Apply(job, outcome)); } } - private PasswordVerificationOutcome Compute(PasswordVerificationJob job) + private PasswordOutcome Compute(PasswordJob job) { - if (!_argon2.ValidatePassword(job.StoredHash, job.VerifyPhrase)) + if (job.VerifyPhrase != null && !_argon2.ValidatePassword(job.StoredHash, job.VerifyPhrase)) { - return new PasswordVerificationOutcome(false, null); + return new PasswordOutcome(false, null); } - return new PasswordVerificationOutcome( + return new PasswordOutcome( true, - job.RehashPhrase == null ? null : _argon2.EncryptPassword(job.RehashPhrase) + job.HashPhrase == null ? null : _argon2.EncryptPassword(job.HashPhrase) ); } - private static void Apply(PasswordVerificationJob job, PasswordVerificationOutcome outcome) + private static void Apply(PasswordJob job, PasswordOutcome outcome) { - var state = job.State; - - // Re-checked: the connection can also drop while the verdict sits in the loop queue. - if (state?.Running != true) + // Re-checked: a connection can drop while the result sits in the loop queue. Jobs with no + // connection attached, such as an admin password change, are unaffected. + if (job.State != null && !job.State.Running) { return; } - if (outcome.Verified && outcome.UpgradedPassword != null) + if (outcome.Verified && outcome.Hash != null) { - job.Account.ApplyPasswordUpgrade(job.StoredHash, outcome.UpgradedPassword, job.TargetAlgorithm); + job.Account.ApplyPasswordWrite(job.Sequence, outcome.Hash, job.TargetAlgorithm); } - AccountHandler.CompleteDeferredAccountLogin(state, job.Account, outcome.Verified); + job.OnComplete?.Invoke(job, outcome); + } + + /// + /// Sets a password, off the loop when that is available and inline otherwise, invoking + /// on the loop either way. Both branches claim a write slot first, so + /// the newest request wins however the work was routed. + /// + /// The confirmation belongs in , not at the call site: off-loop it has + /// not happened yet when the call returns. + /// + internal static void SetPassword(Account account, string plainPassword, Action onDone) + { + if (!Enabled || AccountSecurity.CurrentAlgorithm != PasswordProtectionAlgorithm.Argon2) + { + account.SetPassword(plainPassword); + onDone?.Invoke(true); + return; + } + + var job = new PasswordJob + { + Account = account, + HashPhrase = account.GetRehashPhrase(plainPassword), + TargetAlgorithm = AccountSecurity.CurrentAlgorithm, + Sequence = account.BeginPasswordWrite(), + OnComplete = (_, outcome) => onDone?.Invoke(outcome.Hash != null) + }; + + if (!TryEnqueue(job)) + { + // Saturated. A password change is rare and must not be silently dropped, so this one + // pays the hash on the loop rather than failing. + account.SetPassword(plainPassword); + onDone?.Invoke(true); + } } /// Runs a job on the calling thread. The seam the tests drive. - internal static PasswordVerificationOutcome ComputeInline(PasswordVerificationJob job) => + internal static PasswordOutcome ComputeInline(PasswordJob job) => Instance.Compute(job); internal static void Exit() diff --git a/Projects/UOContent/Gumps/AdminGump.cs b/Projects/UOContent/Gumps/AdminGump.cs index 1fe917b55..f212a6796 100644 --- a/Projects/UOContent/Gumps/AdminGump.cs +++ b/Projects/UOContent/Gumps/AdminGump.cs @@ -2903,7 +2903,7 @@ namespace Server.Gumps else { notice = "The password has been changed."; - a.SetPassword(password); + Server.Accounting.Security.PasswordWorker.SetPassword(a, password, null); page = AdminGumpPage.AccountDetails_Information; CommandLogging.WriteLine( from, diff --git a/dev-docs/claude-skills/modernuo-threading.md b/dev-docs/claude-skills/modernuo-threading.md index b16f1a0f5..3b2f370c7 100644 --- a/dev-docs/claude-skills/modernuo-threading.md +++ b/dev-docs/claude-skills/modernuo-threading.md @@ -181,7 +181,7 @@ Timer.DelayCall(TimeSpan.Zero, TimeSpan.FromMilliseconds(50), () => | Worker | Justification | |---|---| -| `Accounting/Security/PasswordVerificationWorker.cs` | 8.9 ms/login on-loop; 3.5-8.9 ms measured saving | +| `Accounting/Security/PasswordWorker.cs` | 8.9 ms/login on-loop; 3.5-8.9 ms measured saving | | `Engines/Advanced Search/AdvancedSearchGump.cs` | Admin-triggered full-world scan, saves disabled | ### The five rules diff --git a/dev-docs/threading-model.md b/dev-docs/threading-model.md index 64ace0275..d625f3093 100644 --- a/dev-docs/threading-model.md +++ b/dev-docs/threading-model.md @@ -177,7 +177,7 @@ version of this -- it is a correctness bug. | Worker | Off-loop work | Justification | |---|---|---| -| `Accounting/Security/PasswordVerificationWorker.cs` | Argon2 password verification | `docs/handoffs/2026-08-07-off-loop-argon2-hashing.md` -- 8.9 ms/login on-loop, measured 3.5--8.9 ms saved | +| `Accounting/Security/PasswordWorker.cs` | Argon2 verification and hashing | `docs/handoffs/2026-08-07-off-loop-argon2-hashing.md` -- 8.9 ms/login on-loop, measured 3.5--8.9 ms saved | | `Engines/Advanced Search/AdvancedSearchGump.cs` | Parallel entity search | Admin-triggered full-world scan; saves disabled for its duration | Adding to this table needs the same bar: a measurement, and all five rules below.