feat(accounts): rehash stale Argon2 parameters on successful login
Argon2's PHC string embeds m, t and p, so verification uses the parameters stored with each account rather than the configured ones -- and verify is the hot path. CheckPassword only rehashed when the ALGORITHM changed, never when its cost parameters did, so changing the defaults reached nobody on an established shard and the change was cosmetic. IPasswordProtection.NeedsRehash defaults to false, leaving PBKDF2 and the HashAlgorithm protections untouched; only Argon2 carries cost in its stored value. Logged at Debug because a restart migrates the whole population at once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
1f4952e40e
commit
e284227557
5 changed files with 87 additions and 2 deletions
|
|
@ -54,4 +54,23 @@ public class AccountPasswordTests : IDisposable
|
|||
Assert.True(account.CheckPassword(Password));
|
||||
Assert.False(account.CheckPassword("wrong-password"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void StaleArgon2Parameters_AreRehashedOnLogin()
|
||||
{
|
||||
AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2;
|
||||
var account = new Account("stale-params-user", Password);
|
||||
|
||||
// Simulate an account stored under the pre-1.20.0 default: Argon2i, m=8192, t=3, p=1.
|
||||
account.Password =
|
||||
"$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw";
|
||||
|
||||
Assert.True(account.CheckPassword("hunter2"));
|
||||
Assert.StartsWith("$argon2id$v=19$m=16384,t=1,p=1$", account.Password);
|
||||
|
||||
// Already current: verifying again must not rewrite the hash.
|
||||
var afterFirst = account.Password;
|
||||
Assert.True(account.CheckPassword("hunter2"));
|
||||
Assert.Equal(afterFirst, account.Password);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -86,4 +86,38 @@ public class PasswordProtectionTest
|
|||
Assert.True(Argon2PasswordProtection.Instance.ValidatePassword(LegacyArgon2iHash, "hunter2"));
|
||||
Assert.False(Argon2PasswordProtection.Instance.ValidatePassword(LegacyArgon2iHash, "wrong"));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
// type, memory, time, parallelism -> expected NeedsRehash
|
||||
[InlineData("argon2id", 16384, 1, 1, false)] // current defaults
|
||||
[InlineData("argon2i", 8192, 3, 1, true)] // the old shipping default
|
||||
[InlineData("argon2id", 8192, 1, 1, true)] // right type, stale memory
|
||||
[InlineData("argon2id", 16384, 3, 1, true)] // right type, stale iterations
|
||||
[InlineData("argon2id", 16384, 1, 2, true)] // right type, stale parallelism
|
||||
public void Argon2_NeedsRehash_ComparesTypeAndCost(
|
||||
string type, int memory, int time, int parallelism, bool expected
|
||||
)
|
||||
{
|
||||
var hash = $"${type}$v=19$m={memory},t={time},p={parallelism}$" +
|
||||
"LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw";
|
||||
|
||||
Assert.Equal(expected, Argon2PasswordProtection.Instance.NeedsRehash(hash));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("")]
|
||||
[InlineData("not-a-hash")]
|
||||
public void Argon2_NeedsRehash_IsTrueForUnparseableHashes(string hash)
|
||||
{
|
||||
Assert.True(Argon2PasswordProtection.Instance.NeedsRehash(hash));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void NonArgon2Protections_NeverNeedRehash()
|
||||
{
|
||||
Assert.False(PBKDF2PasswordProtection.Instance.NeedsRehash("anything"));
|
||||
Assert.False(HashAlgorithmPasswordProtection.SHA2Instance.NeedsRehash("anything"));
|
||||
Assert.False(HashAlgorithmPasswordProtection.SHA1Instance.NeedsRehash("anything"));
|
||||
Assert.False(HashAlgorithmPasswordProtection.MD5Instance.NeedsRehash("anything"));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ using System.Xml;
|
|||
using ModernUO.CodeGeneratedEvents;
|
||||
using ModernUO.Serialization;
|
||||
using Server.Accounting.Security;
|
||||
using Server.Logging;
|
||||
using Server.Misc;
|
||||
using Server.Mobiles;
|
||||
using Server.Multis;
|
||||
|
|
@ -18,6 +19,8 @@ namespace Server.Accounting;
|
|||
[SerializationGenerator(6)]
|
||||
public partial class Account : IAccount, IComparable<Account>
|
||||
{
|
||||
private static readonly ILogger logger = LogFactory.GetLogger(typeof(Account));
|
||||
|
||||
public static readonly TimeSpan YoungDuration = TimeSpan.FromHours(40.0);
|
||||
public static readonly TimeSpan InactiveDuration = TimeSpan.FromDays(180.0);
|
||||
public static readonly TimeSpan EmptyInactiveDuration = TimeSpan.FromDays(30.0);
|
||||
|
|
@ -408,9 +411,13 @@ public partial class Account : IAccount, IComparable<Account>
|
|||
return false;
|
||||
}
|
||||
|
||||
// Upgrade the password protection in case we change the algorithm
|
||||
if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm)
|
||||
// Rehash when either the algorithm or its cost parameters have moved on. The short-circuit
|
||||
// ordering is load-bearing: NeedsRehash must never be handed a hash produced by a different
|
||||
// algorithm, and the first clause guarantees it is not.
|
||||
if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm ||
|
||||
AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password))
|
||||
{
|
||||
logger.Debug("Rehashing the password for account '{Username}'.", _username);
|
||||
SetPassword(plainPassword);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -4,5 +4,12 @@ namespace Server.Accounting
|
|||
{
|
||||
string EncryptPassword(string plainPassword);
|
||||
bool ValidatePassword(string encryptedPassword, string plainPassword);
|
||||
|
||||
/// <summary>
|
||||
/// True when <paramref name="encryptedPassword"/> was produced with parameters that differ
|
||||
/// from the ones this protection currently uses, so a successful login should rewrite it.
|
||||
/// Algorithms whose cost is not embedded in the stored value never need this.
|
||||
/// </summary>
|
||||
bool NeedsRehash(string encryptedPassword) => false;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -39,4 +39,22 @@ public class Argon2PasswordProtection : IPasswordProtection
|
|||
|
||||
public bool ValidatePassword(string encryptedPassword, string plainPassword) =>
|
||||
_passwordHasher.Verify(encryptedPassword, plainPassword);
|
||||
|
||||
public bool NeedsRehash(string encryptedPassword)
|
||||
{
|
||||
// Argon2's PHC string embeds m, t and p, so verification uses the parameters stored with
|
||||
// each account rather than the configured ones. Without this comparison a parameter change
|
||||
// would reach nobody: CheckPassword only rehashed when the *algorithm* changed.
|
||||
if (!Argon2PasswordHasher.TryExtractMetadataValues(encryptedPassword, out var values))
|
||||
{
|
||||
// Only reached after a successful verify, so an unparseable string means a format this
|
||||
// build does not understand. Rewriting it into the current one is strictly better.
|
||||
return true;
|
||||
}
|
||||
|
||||
return values.ArgonType != _passwordHasher.ArgonType
|
||||
|| values.MemoryCost != _passwordHasher.MemoryCost
|
||||
|| values.TimeCost != _passwordHasher.TimeCost
|
||||
|| values.Parallelism != _passwordHasher.Parallelism;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue