fix(accounts): derive the password phrase from the target algorithm
SetPassword salted the phrase with the username according to the OUTGOING algorithm but stored the result under the INCOMING one. SHA1 and SHA2 prepend the username; Argon2 and PBKDF2 do not. Two live lockouts followed: - A ServUO-imported SHA2 account logging in with the default Argon2 config was 'upgraded' to argon2(username + password) and tagged Argon2. The next login rebuilt the phrase as bare password and could never match. One successful login, then permanent lockout. - SetPassword also runs from the Account constructor, before _passwordAlgorithm is assigned, so it is still None. Creating an account under SHA1 or SHA2 hashed the bare password and tagged it with an algorithm that re-adds the username -- a brand-new account that could never log in. Derive the phrase from the algorithm being written. Also configures Accounts persistence in the UOContent test fixture so an Account can be constructed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
23dc6649a0
commit
970cd42699
3 changed files with 69 additions and 8 deletions
|
|
@ -100,6 +100,10 @@ internal static class TestServerInitializer
|
|||
}
|
||||
|
||||
World.Configure();
|
||||
// Registers the Accounts entity persistence. Production reaches this through
|
||||
// AssemblyHandler.Invoke("Configure"); the curated subset here must call it so that
|
||||
// Accounts.NewAccount resolves and tests can construct an Account.
|
||||
Server.Accounting.Accounts.Configure();
|
||||
RaceDefinitions.Configure();
|
||||
MovementImpl.Configure();
|
||||
PathFollower.Configure();
|
||||
|
|
|
|||
|
|
@ -0,0 +1,47 @@
|
|||
using Server.Accounting;
|
||||
using Server.Accounting.Security;
|
||||
using Xunit;
|
||||
|
||||
namespace Server.Tests.Accounting;
|
||||
|
||||
[Collection("Sequential UOContent Tests")]
|
||||
public class AccountPasswordTests
|
||||
{
|
||||
private const string Password = "hunter2";
|
||||
|
||||
[Theory]
|
||||
[InlineData(PasswordProtectionAlgorithm.SHA1)]
|
||||
[InlineData(PasswordProtectionAlgorithm.SHA2)]
|
||||
[InlineData(PasswordProtectionAlgorithm.PBKDF2)]
|
||||
[InlineData(PasswordProtectionAlgorithm.Argon2)]
|
||||
public void NewAccount_CanLogIn(PasswordProtectionAlgorithm algorithm)
|
||||
{
|
||||
AccountSecurity.CurrentAlgorithm = algorithm;
|
||||
var account = new Account($"new-{algorithm}-user", Password);
|
||||
|
||||
Assert.Equal(algorithm, account.PasswordAlgorithm);
|
||||
Assert.True(account.CheckPassword(Password));
|
||||
Assert.False(account.CheckPassword("wrong-password"));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(PasswordProtectionAlgorithm.SHA1)]
|
||||
[InlineData(PasswordProtectionAlgorithm.SHA2)]
|
||||
[InlineData(PasswordProtectionAlgorithm.PBKDF2)]
|
||||
public void UpgradingAlgorithm_DoesNotLockTheAccountOut(PasswordProtectionAlgorithm from)
|
||||
{
|
||||
AccountSecurity.CurrentAlgorithm = from;
|
||||
var account = new Account($"upgrade-{from}-user", Password);
|
||||
Assert.True(account.CheckPassword(Password));
|
||||
|
||||
AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2;
|
||||
|
||||
// First login verifies under the old algorithm and rehashes under the new one.
|
||||
Assert.True(account.CheckPassword(Password));
|
||||
Assert.Equal(PasswordProtectionAlgorithm.Argon2, account.PasswordAlgorithm);
|
||||
|
||||
// Second login must verify against what the first one wrote.
|
||||
Assert.True(account.CheckPassword(Password));
|
||||
Assert.False(account.CheckPassword("wrong-password"));
|
||||
}
|
||||
}
|
||||
|
|
@ -376,21 +376,31 @@ public partial class Account : IAccount, IComparable<Account>
|
|||
return true;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// SHA1 and SHA2 are the ServUO-compatible algorithms; they salt the password with the
|
||||
/// username. Argon2 and PBKDF2 carry their own salt and do not.
|
||||
/// </summary>
|
||||
private static bool UsesUsernamePhrase(PasswordProtectionAlgorithm algorithm) =>
|
||||
algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2;
|
||||
|
||||
public void SetPassword(string plainPassword)
|
||||
{
|
||||
var phrase = _passwordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2
|
||||
? $"{_username}{plainPassword}"
|
||||
: plainPassword;
|
||||
// The phrase must match how CheckPassword will rebuild it *after* the algorithm changes,
|
||||
// so it is derived from the target algorithm rather than the outgoing one. Deriving it
|
||||
// from _passwordAlgorithm stored a username-salted hash under an algorithm that never
|
||||
// re-adds the username, locking the account out on its next login -- and, because this
|
||||
// runs from the constructor before _passwordAlgorithm is assigned, it also produced
|
||||
// brand-new SHA1/SHA2 accounts that could never log in at all.
|
||||
var algorithm = AccountSecurity.CurrentAlgorithm;
|
||||
var phrase = UsesUsernamePhrase(algorithm) ? $"{_username}{plainPassword}" : plainPassword;
|
||||
|
||||
Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword(phrase);
|
||||
PasswordAlgorithm = AccountSecurity.CurrentAlgorithm;
|
||||
Password = AccountSecurity.GetPasswordProtection(algorithm).EncryptPassword(phrase);
|
||||
PasswordAlgorithm = algorithm;
|
||||
}
|
||||
|
||||
public bool CheckPassword(string plainPassword)
|
||||
{
|
||||
var phrase = _passwordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2
|
||||
? $"{_username}{plainPassword}"
|
||||
: plainPassword;
|
||||
var phrase = UsesUsernamePhrase(_passwordAlgorithm) ? $"{_username}{plainPassword}" : plainPassword;
|
||||
|
||||
var ok = AccountSecurity.GetPasswordProtection(_passwordAlgorithm).ValidatePassword(Password, phrase);
|
||||
if (!ok)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue