b2c59191bd
> ⚠️ **Rollback hazard — one-way door once logins are taken.** Serialization is unchanged, so a save > written by this build still *loads* on the previous one. Its contents do not survive the trip: on > its first successful login each account is rehashed to `$argon2id$`, and the previous build ships > Argon2.Bindings 1.19.0, whose `Verify` is gated by the verifier's own configured type and answers > `false` for an `$argon2id$` hash. **After a shard running this build has accepted logins, do not > roll back past this commit** — every account that logged in is locked out on the older binary, and > the only recovery is rolling forward again or resetting passwords by hand. Roll back only from a > save taken before the first post-deploy login. Requires [Argon2.Bindings 1.20.0](https://github.com/modernuo/Argon2.Bindings/pull/14), now published. ## What - Consume `Argon2.Bindings` 1.20.0, which resolves the Argon2 type from the stored PHC string rather than from the verifier's own configuration. - Default to **Argon2id, m=16384, t=1, p=1** — 8.51 ms against the old Argon2i 8 MiB t=3 at 10.11 ms. Cheaper *and* stronger. - Rehash on a successful login whenever the stored parameters are stale, not only when the algorithm changes. - Fix `SetPassword`, which derived the password phrase from the outgoing algorithm while storing it under the incoming one. ## Why **Verification was gated by the verifier's configured type.** `Verify` passed the instance's own `ArgonType` to native `argon2_verify`, whose `decode_string` rejects a disagreeing `$argon2i$`/`$argon2id$` prefix and returns `DECODING_FAIL` — folded into `false`, the same answer as a wrong password. Switching the default type would have locked out every existing account, and `VerifyAndUpdate` could not have migrated them either: it delegates to the same type-fixed `Verify` and never compared `ArgonType`. Fixed upstream in 1.20.0. The pinned legacy-`$argon2i$` test here fails on 1.19.0 for exactly that reason, which is what makes the package bump load-bearing rather than incidental. **Changing the defaults would otherwise have reached nobody.** Argon2's PHC string embeds `m`, `t` and `p`, so verification uses the parameters stored with each account, not the configured ones — and verification is the hot path. `CheckPassword` only rehashed when the *algorithm* changed, never when its cost parameters did, so on an established shard the new defaults would have applied to new accounts only. `IPasswordProtection.NeedsRehash` closes that: it defaults to `false`, so PBKDF2 and the `HashAlgorithm` protections are untouched — only Argon2 carries its cost inside the stored value. **`SetPassword` picked the phrase rule from the wrong algorithm.** SHA1 and SHA2 salt the phrase with the username; Argon2 and PBKDF2 do not. It chose the rule from the *outgoing* algorithm while storing under the *incoming* one, so any algorithm change wrote a credential its own next verify could not reproduce. It now assigns `PasswordAlgorithm` first and derives the phrase from that. Note this ordering is load-bearing and invisible — `UpgradingAlgorithm_DoesNotLockTheAccountOut` is what pins it. ## Cost Verification is re-derivation, so these are login numbers. A full login calls `CheckPassword` twice — `AccountLogin` (0x80) then `GameLogin` (0x91): **~20 ms before, ~17 ms after**, plus a one-time ~8.5 ms rehash on each account's migrating login. That cost is still paid on the game loop. Moving hashing off-loop is deliberately **not** in this PR — it needs a pending-auth state in the login handlers, bounding of in-flight hashes, and login rate limiting.
74 lines
3 KiB
C#
74 lines
3 KiB
C#
using System;
|
|
using Server.Accounting;
|
|
using Server.Accounting.Security;
|
|
using Xunit;
|
|
|
|
namespace Server.Tests.Accounting;
|
|
|
|
[Collection("Sequential UOContent Tests")]
|
|
public class AccountPasswordTests : IDisposable
|
|
{
|
|
private const string Password = "hunter2";
|
|
|
|
// CurrentAlgorithm is process-wide state shared with the rest of the collection.
|
|
private readonly PasswordProtectionAlgorithm _originalAlgorithm = AccountSecurity.CurrentAlgorithm;
|
|
|
|
public void Dispose() => AccountSecurity.CurrentAlgorithm = _originalAlgorithm;
|
|
|
|
[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"));
|
|
}
|
|
|
|
// SetPassword assigns PasswordAlgorithm before deriving the phrase from it. Reversing those two
|
|
// lines salts the hash by the outgoing algorithm's rule and stores it under the incoming one,
|
|
// which verifies once and then never again.
|
|
[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;
|
|
|
|
Assert.True(account.CheckPassword(Password));
|
|
Assert.Equal(PasswordProtectionAlgorithm.Argon2, account.PasswordAlgorithm);
|
|
|
|
// Must verify against what the rehash wrote.
|
|
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);
|
|
|
|
// The shipping default before this change: 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(Password));
|
|
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(Password));
|
|
Assert.Equal(afterFirst, account.Password);
|
|
}
|
|
}
|