docs: trim the auth id comments to what is not in the code
Several comments narrated the reasoning that produced the code rather than what a reader needs. Two also outlived their subject: ConsumeAuthId's summary and its call site both still claimed the id is spent whether or not it vouches, which stopped being true when ownership checks moved ahead of the removal.
This commit is contained in:
Kamron Batman
parent
e801a69554
commit
ee1531f272
8 changed files with 52 additions and 88 deletions
|
|
@ -100,8 +100,7 @@ internal static class TestServerInitializer
|
|||
}
|
||||
|
||||
World.Configure();
|
||||
// Registers the Accounts entity persistence, without which Accounts.NewAccount cannot
|
||||
// resolve and no test can construct an Account.
|
||||
// Registers the Accounts entity persistence; without it no test can construct an Account.
|
||||
Server.Accounting.Accounts.Configure();
|
||||
RaceDefinitions.Configure();
|
||||
MovementImpl.Configure();
|
||||
|
|
|
|||
|
|
@ -30,9 +30,9 @@ public class AccountPasswordTests : IDisposable
|
|||
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.
|
||||
// SetPassword assigns PasswordAlgorithm before deriving the phrase from it. Reversed, the hash
|
||||
// is salted by the outgoing algorithm's rule but stored under the incoming one, which verifies
|
||||
// once and then never again.
|
||||
[Theory]
|
||||
[InlineData(PasswordProtectionAlgorithm.SHA1)]
|
||||
[InlineData(PasswordProtectionAlgorithm.SHA2)]
|
||||
|
|
|
|||
|
|
@ -75,7 +75,7 @@ public class PasswordProtectionTest
|
|||
Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password"));
|
||||
}
|
||||
|
||||
// The shipping default before this change. A literal, so it cannot drift with the configured
|
||||
// The shipping default before this change, as a literal so it cannot drift with the configured
|
||||
// defaults. Password: "hunter2".
|
||||
private const string LegacyArgon2iHash =
|
||||
"$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw";
|
||||
|
|
@ -105,9 +105,8 @@ public class PasswordProtectionTest
|
|||
Assert.Equal(expected, Argon2PasswordProtection.Instance.NeedsRehash(hash));
|
||||
}
|
||||
|
||||
// Digest and salt lengths are the decoded sizes of the base64 segments, not parameter-list
|
||||
// entries, so they need their own literals. Current type and cost throughout; only a length
|
||||
// differs from the defaults. The theory above is the negative control at default lengths.
|
||||
// Digest and salt lengths are decoded base64 sizes rather than parameter-list entries, so they
|
||||
// need their own literals. Current type and cost throughout; only a length differs.
|
||||
[Theory]
|
||||
// 16-byte digest: 22 base64 chars instead of the 43 a 32-byte digest encodes to.
|
||||
[InlineData("$argon2id$v=19$m=16384,t=1,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4g")]
|
||||
|
|
|
|||
|
|
@ -123,10 +123,8 @@ public class AuthIdTests : IDisposable
|
|||
);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A rejected attempt must not consume the id. Anyone landing on a live id could otherwise burn
|
||||
/// it, and its owner would arrive to "unable to find auth id" and have to log in again.
|
||||
/// </summary>
|
||||
// A rejected attempt must not consume the id, or anyone landing on a live one could burn it and
|
||||
// force its owner to log in again.
|
||||
[Fact]
|
||||
public void SurvivesAnAttemptFromTheWrongAddress()
|
||||
{
|
||||
|
|
@ -174,10 +172,6 @@ public class AuthIdTests : IDisposable
|
|||
Assert.Null(entry.Account);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An expired id is still spent by its owner: they fall back to the password verify, and the id
|
||||
/// has done everything it is ever going to do.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void AnExpiredIdIsSpentByItsOwner()
|
||||
{
|
||||
|
|
@ -202,11 +196,8 @@ public class AuthIdTests : IDisposable
|
|||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An expired id is not a lockout. A player can sit on the server list, and before any of this
|
||||
/// existed the game login always verified the password anyway -- so falling back to that verify
|
||||
/// is the behaviour we started from, not a regression.
|
||||
/// </summary>
|
||||
// Expiry is not a lockout. The game login always verified the password before any of this
|
||||
// existed, so falling back to that verify is the behaviour we started from.
|
||||
[Fact]
|
||||
public void ExpiresIntoAPasswordVerifyRatherThanARejection()
|
||||
{
|
||||
|
|
@ -275,11 +266,8 @@ public class AuthIdTests : IDisposable
|
|||
Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A connection gets exactly one id, no matter how many times it re-selects. Handing the same
|
||||
/// one back rather than minting another is what makes an orphaned id impossible, instead of
|
||||
/// something to clean up afterwards.
|
||||
/// </summary>
|
||||
// Handing the same id back rather than minting another is what makes an orphan impossible,
|
||||
// instead of something to clean up afterwards.
|
||||
[Fact]
|
||||
public void ReSelectingReturnsTheSameIdAndAddsNothingToTheWindow()
|
||||
{
|
||||
|
|
@ -298,10 +286,6 @@ public class AuthIdTests : IDisposable
|
|||
);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An id is only left behind when a client picks a server and never arrives. Issuing sweeps
|
||||
/// those, so they do not accumulate.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void AbandonedIdsAreSweptWhenNewOnesAreIssued()
|
||||
{
|
||||
|
|
@ -335,11 +319,8 @@ public class AuthIdTests : IDisposable
|
|||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A login rush is not a backlog. Every one of these ids belongs to a client on its way to
|
||||
/// redeem it, so none may be discarded to keep the window at some arbitrary size -- doing so
|
||||
/// hands a real player "unable to find auth id" and a disconnect.
|
||||
/// </summary>
|
||||
// A login rush is not a backlog. Every id belongs to a client on its way to redeem it, so none
|
||||
// may be discarded to hold the window at some arbitrary size.
|
||||
[Fact]
|
||||
public void ALoginRushDoesNotEvictAnyonesAuthId()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -343,9 +343,8 @@ public static class AccountHandler
|
|||
logger.Information("Login: {NetState} Access denied for '{Username}'", e.State, un);
|
||||
e.Accepted = false;
|
||||
}
|
||||
// The auth id already vouched for this account from this address, and it was only issued
|
||||
// after the account login packet verified the password. Re-deriving the hash here costs
|
||||
// another full Argon2 verify to answer a question already answered.
|
||||
// The auth id was only issued after the account login packet verified this password, so
|
||||
// re-deriving the hash costs a second Argon2 verify to answer the same question.
|
||||
else if (!e.PreAuthenticated && !acct.CheckPassword(pw))
|
||||
{
|
||||
logger.Information("Login: {NetState} Invalid password for '{Username}'", e.State, un);
|
||||
|
|
|
|||
|
|
@ -37,9 +37,8 @@ public class Argon2PasswordProtection : IPasswordProtection
|
|||
public bool ValidatePassword(string encryptedPassword, string plainPassword) =>
|
||||
_passwordHasher.Verify(encryptedPassword, plainPassword);
|
||||
|
||||
// The PHC string carries the parameters it was hashed with, so verification uses those rather
|
||||
// than the configured ones. Comparing them is what lets a parameter change reach existing
|
||||
// accounts.
|
||||
// Verification uses the parameters embedded in the PHC string, not the configured ones, so
|
||||
// comparing them is what lets a parameter change reach existing accounts.
|
||||
public bool NeedsRehash(string encryptedPassword)
|
||||
{
|
||||
// Unparseable but verified: a format this build does not understand, so rewrite it.
|
||||
|
|
|
|||
|
|
@ -16,8 +16,8 @@ public static partial class GameServer
|
|||
|
||||
/// <summary>
|
||||
/// The auth id presented on this game login was issued to this account, from this address,
|
||||
/// after the account login packet already verified the password. Read-only: a subscriber
|
||||
/// must not be able to grant itself the skip.
|
||||
/// after the account login packet verified the password. Read-only so a subscriber cannot
|
||||
/// grant itself the skip.
|
||||
/// </summary>
|
||||
public bool PreAuthenticated { get; }
|
||||
|
||||
|
|
|
|||
|
|
@ -28,14 +28,14 @@ namespace Server.Network;
|
|||
|
||||
public static class IncomingAccountPackets
|
||||
{
|
||||
// Initial capacity and the point at which issuing sweeps for expired ids. Not a cap: a login
|
||||
// rush must not be turned into "unable to find auth id" for whoever lands past the limit.
|
||||
// Initial capacity and the point at which issuing sweeps expired ids. Not a cap; the window
|
||||
// grows rather than evicting a live id.
|
||||
private const int _authIDWindowSize = 128;
|
||||
|
||||
private static int _authIdPurgeThreshold = _authIDWindowSize;
|
||||
|
||||
// The gap between PlayServerAck and the client's game login is seconds. Two minutes is generous,
|
||||
// and bounds how long a stolen id stays usable.
|
||||
// The gap between PlayServerAck and the game login is seconds. Bounds how long a stolen id
|
||||
// stays usable.
|
||||
private static readonly TimeSpan _authIDLifetime = TimeSpan.FromMinutes(2.0);
|
||||
|
||||
private static readonly Dictionary<int, AuthIDPersistence> _authIDWindow =
|
||||
|
|
@ -46,8 +46,8 @@ public static class IncomingAccountPackets
|
|||
public DateTime Age;
|
||||
public readonly ClientVersion Version;
|
||||
|
||||
// The account and address that earned this id on the account login packet. GameLogin skips
|
||||
// its own password verify when both match, so the id is a bearer token and must be bound.
|
||||
// GameLogin skips its password verify when both match, so the id is a bearer token and has
|
||||
// to be bound to whatever earned it.
|
||||
public readonly IAccount Account;
|
||||
public readonly IPAddress Address;
|
||||
|
||||
|
|
@ -62,13 +62,11 @@ public static class IncomingAccountPackets
|
|||
|
||||
internal enum AuthIdResult
|
||||
{
|
||||
// No such id, or it was issued for a different account or address. A client that presents
|
||||
// one of those is not one of ours; nothing here is worth a password check.
|
||||
// No such id, or it was issued for a different account or address.
|
||||
Rejected,
|
||||
|
||||
// Right account, right address, but too old to stand in for the verify. A player can idle
|
||||
// on the server list, so this is a normal thing to do -- fall back to checking the password
|
||||
// rather than turning it into a lockout.
|
||||
// Right account and address, too old to stand in for the verify. Idling on the server list
|
||||
// is normal, so this falls back to the password check rather than becoming a lockout.
|
||||
Expired,
|
||||
|
||||
// Issued to this account, from this address, recently. Stands in for the password verify.
|
||||
|
|
@ -350,10 +348,9 @@ public static class IncomingAccountPackets
|
|||
EnsureAuthId(state.AuthId, state.Account, state.Address, state.Version);
|
||||
|
||||
/// <summary>
|
||||
/// One id per connection, by construction. Choosing a server queues a disconnect, but the queue
|
||||
/// is drained on the next slice, so a client that pipelines another seed/login/select into the
|
||||
/// same buffer arrives here again. Handing back the id it already holds -- rather than minting a
|
||||
/// second and orphaning the first -- means no amount of re-selecting can leave anything behind.
|
||||
/// One id per connection, by construction. Choosing a server queues a disconnect that is not
|
||||
/// drained until the next slice, so a client pipelining another select into the same buffer
|
||||
/// arrives here again; handing back the id it already holds cannot orphan one.
|
||||
/// </summary>
|
||||
internal static int EnsureAuthId(
|
||||
int existingAuthId, IAccount account, IPAddress address, ClientVersion version
|
||||
|
|
@ -361,12 +358,10 @@ public static class IncomingAccountPackets
|
|||
|
||||
internal static int RegisterAuthId(IAccount account, IPAddress address, ClientVersion version)
|
||||
{
|
||||
// An id is issued when a server is picked and consumed by the game login seconds later, so
|
||||
// the only ones left behind belong to clients that picked a server and never arrived. Sweep
|
||||
// those, but never evict a live id to make room: the client holding it is on its way to
|
||||
// redeem it, and taking it away hands a real player a failed login. If everything is still
|
||||
// live the window simply grows -- that is a login rush, not a backlog. Creating an entry
|
||||
// costs a successful password verify, so the size is self-limiting.
|
||||
// Sweep the ids left behind by clients that picked a server and never arrived, but never
|
||||
// evict a live one to make room -- the client holding it is on its way to redeem it. If all
|
||||
// are live the window grows, which is a login rush, not a backlog. Each entry costs a
|
||||
// successful password verify, so the size is self-limiting.
|
||||
if (_authIDWindow.Count >= _authIdPurgeThreshold)
|
||||
{
|
||||
PurgeExpiredAuthIds();
|
||||
|
|
@ -375,9 +370,8 @@ public static class IncomingAccountPackets
|
|||
|
||||
int authID;
|
||||
|
||||
// A cryptographic draw across the whole int range: the id stands in for a password verify,
|
||||
// so it has to be unguessable. Zero is reserved -- GameLogin reads state.AuthId == 0 as
|
||||
// "no auth id was issued".
|
||||
// The id stands in for a password verify, so it has to be unguessable. Zero is reserved:
|
||||
// GameLogin reads state.AuthId == 0 as "no auth id was issued".
|
||||
do
|
||||
{
|
||||
authID = RandomNumberGenerator.GetInt32(int.MinValue, int.MaxValue);
|
||||
|
|
@ -389,10 +383,9 @@ public static class IncomingAccountPackets
|
|||
}
|
||||
|
||||
/// <summary>
|
||||
/// Looks up and spends an auth id. The id is removed whether or not it vouches, so a guessed id
|
||||
/// cannot be reused to enumerate usernames. An address mismatch is
|
||||
/// <see cref="AuthIdResult.Rejected"/> rather than a fallback: network switching mid-login is
|
||||
/// not supported.
|
||||
/// Spends an auth id, but only for the account and address it was issued to. An address
|
||||
/// mismatch is <see cref="AuthIdResult.Rejected"/> rather than a fallback: network switching
|
||||
/// mid-login is not supported.
|
||||
/// </summary>
|
||||
internal static AuthIdResult ConsumeAuthId(
|
||||
int authId, string username, IPAddress address, out AuthIDPersistence entry
|
||||
|
|
@ -403,11 +396,9 @@ public static class IncomingAccountPackets
|
|||
return AuthIdResult.Rejected;
|
||||
}
|
||||
|
||||
// Look, then take. Removing before the presenter has shown the id is theirs would let anyone
|
||||
// who lands on a live id burn it, and its owner would arrive to "unable to find auth id" and
|
||||
// have to log in again. Address first: a different address is a different client however
|
||||
// fresh the id is, and checking it before the username means a remote guesser never learns
|
||||
// whether a username matched.
|
||||
// Look, then take: removing before ownership is proven would let anyone landing on a live id
|
||||
// burn it, leaving its owner to log in again. Address before username, so a remote guesser
|
||||
// never learns whether a username matched.
|
||||
if (!Utility.Intern(address).Equals(entry.Address)
|
||||
|| entry.Account == null
|
||||
|| !username.InsensitiveEquals(entry.Account.Username))
|
||||
|
|
@ -416,8 +407,7 @@ public static class IncomingAccountPackets
|
|||
return AuthIdResult.Rejected;
|
||||
}
|
||||
|
||||
// Theirs, so spend it. Expired still counts as spent -- they fall back to the password
|
||||
// verify and the id has done all it is ever going to do.
|
||||
// Theirs, so spend it. Expired counts as spent; it has done all it is ever going to do.
|
||||
_authIDWindow.Remove(authId);
|
||||
|
||||
return Core.Now - entry.Age > _authIDLifetime ? AuthIdResult.Expired : AuthIdResult.Vouched;
|
||||
|
|
@ -427,8 +417,7 @@ public static class IncomingAccountPackets
|
|||
{
|
||||
var now = Core.Now;
|
||||
|
||||
// Removing during enumeration is supported on Dictionary since .NET Core 3.0, so this
|
||||
// reclaims in one pass with no scratch list.
|
||||
// Dictionary supports removal during enumeration, so this reclaims in one pass.
|
||||
foreach (var (key, entry) in _authIDWindow)
|
||||
{
|
||||
if (now - entry.Age > _authIDLifetime)
|
||||
|
|
@ -468,7 +457,6 @@ public static class IncomingAccountPackets
|
|||
var username = reader.ReadLatin1Safe(30);
|
||||
var password = reader.ReadLatin1Safe(30);
|
||||
|
||||
// Spends the id either way, so a guessed one cannot be reused to probe usernames.
|
||||
var authResult = ConsumeAuthId(authId, username, state.Address, out var ap);
|
||||
|
||||
if (authResult == AuthIdResult.Rejected)
|
||||
|
|
@ -481,8 +469,7 @@ public static class IncomingAccountPackets
|
|||
state.Version = ap.Version;
|
||||
state.Seeded = true;
|
||||
|
||||
// Expired still carries a usable entry, and the client just pays the password verify it
|
||||
// would have paid before any of this existed.
|
||||
// Expired carries a usable entry; only the password verify skip is withheld.
|
||||
var e = new GameServer.GameLoginEventArgs(
|
||||
state,
|
||||
username,
|
||||
|
|
@ -510,8 +497,8 @@ public static class IncomingAccountPackets
|
|||
|
||||
public static void PlayServer(NetState state, SpanReader reader)
|
||||
{
|
||||
// A server is picked once per connection. Picking again would hand back an id this
|
||||
// connection may already have spent on a game login, so the client could never redeem it.
|
||||
// A server is picked once per connection. Picking again hands back an id this connection may
|
||||
// already have spent on a game login, which the client could never redeem.
|
||||
if (state.AuthId != 0)
|
||||
{
|
||||
state.Disconnect("Duplicate play server packet sent.");
|
||||
|
|
@ -539,8 +526,8 @@ public static class IncomingAccountPackets
|
|||
|
||||
public static void LoginServerSeed(NetState state, SpanReader reader)
|
||||
{
|
||||
// Seeding happens once per connection. A second one means the client is restarting a
|
||||
// handshake it already completed, which no real client does.
|
||||
// Seeding happens once per connection. A second one restarts a handshake this connection
|
||||
// already completed, which no real client does.
|
||||
if (state.Seeded)
|
||||
{
|
||||
state.Disconnect("Duplicate login server seed packet sent.");
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue