Commit graph

31 commits

Author SHA1 Message Date
Kamron Batman
a7e65aab01
perf(login): run password hashing on a parked worker thread (#2566)
## Why

An Argon2 verify is **~8.9 ms of frozen world per login attempt** — more than half a 16 ms frame. Failed attempts cost exactly the same as successful ones, by design, so a credential-stuffing flood is a full-cost stall per packet without needing valid credentials. `SetPassword` derives a hash too, so `[password`, the admin gump and account creation each pay the same.

## What the measurement says

Off-loading does not delete the cost, it relocates it. Three things stay on the loop:

| Component | Measured |
|---|---:|
| Inline verify (today) | **8.92 ms** |
| Dispatch to the worker | 210 ns |
| Drain the continuation off `LoopContext` | 13 ns |
| Loop's own work slowed by shared-L3 eviction | **0.05 – 5.44 ms** |

Net gain **3.5 – 8.9 ms** of on-loop time per login. Harness in `ModernUO-Benchmarks` (`Benchmarks/Argon2OffLoop/`): it models the loop as a dependent-load pointer chase swept across working-set sizes, which is an upper bound on cache-latency sensitivity, and copies `EventLoopContext` so the hand-off cost is the real one.

Two results shaped the design:

- **The contention tax peaks in the middle of the working-set range**, not at the top — 5.44 ms at 8 MiB (a quarter of this chip's L3), but 0.76 ms at 30 MiB and 0.10 ms at 256 KiB. A tiny hot set has nothing in L3 to lose; a huge one is already DRAM-bound.
- **Per-login tax falls as concurrency rises** (5.44 → 2.56 → 1.60 ms at 1/2/4 hashers) while *total* loop damage rises. Contention is shared, not additive, so a login rush is not the disaster case — a single login is.

## Why exactly one worker

It is load-bearing three times over, which is also why it must not quietly become a pool:

- **Cost bound.** Off-loop loses to inline only if a hash steals ~82% of the loop's throughput. One hasher contending for one core leaves the loop ~50%. **A single background hasher cannot cost the loop more than the inline verify under any scheduling regime**, which is what lets the measurement hold on hardware we cannot inspect — AMD, VPS, oversubscribed VM. Four hashers drop the loop to ~20% and break it.
- **Memory.** Exactly one hashing arena is live at a time whatever the login volume.
- **Ordering.** Writes apply in dispatch order *only* because a single thread drains FIFO. A second worker would need ordering reintroduced; `WritesApplyInDispatchOrder` fails if that happens.

Throughput is ~110 verifies/sec. Only loop time matters, not login latency, so head-of-line blocking during a rush costs nothing.

## Making every protection safe off-thread

The worker was initially Argon2-only. That was the right call for the wrong reason — it was blamed on Argon2's salt RNG, which is a stateless syscall wrapper and was never a problem. The real blockers were elsewhere, and both are fixed at the source:

| Protection | Was | Now |
|---|---|---|
| MD5/SHA1/SHA2 | shared `HashAlgorithm.ComputeHash`, which carries the running digest across `HashCore`/`HashFinal` through process-wide singletons | static `HashData` into a `stackalloc` span — no state, no allocation, identical bytes |
| PBKDF2 | `Utility.RandomMinMax` → shared `System.Random`, thread-unsafe *and* game state | `RandomNumberGenerator.GetInt32`, matching the salt beside it |
| Argon2 | already safe (`Verify` is static + stackalloc) | unchanged, singleton reused |

Literal digests are pinned in a test **before** the change and still pass after it. These are compared as strings against every account database, so any casing or encoding drift would lock out every SHA and MD5 account at once.

With all three safe, the worker no longer knows which algorithm it runs and the dispatch conditions collapse to "is off-loop available".

## Correctness

- **Phrase derivation** moves to `AccountSecurity.DerivePhrase`, so verification (stored algorithm's rule) and rehash (target algorithm's rule) cannot disagree. Deriving with the wrong one is the shape of the lockout fixed in #2562.
- **Liveness** is checked at dequeue *and* at apply — a connection can drop while queued or while the result sits in the loop queue. A job with no connection attached, such as an admin password change, runs regardless.
- **Queue overflow rejects** a login rather than verifying inline; steering work back onto the loop is what a flood wants. A password change instead falls back to hashing inline, because unlike a login it must not be dropped.
- **Shutdown and crash** both just stop the thread, and pending jobs are dropped. No save is initiated once shutdown begins — saving is the operator's choice up front, via the admin gump's save/no-save variants, and `WaitForWriteCompletion` honours one already in flight — so a write applied during teardown would reach no disk. The crash path needs its own subscription because `HandleClosed` skips `InvokeShutdown` when crashed.

## Bounding

`MaxPending` is 4096 — a backstop, not a flood defense. `SentFirstPacket` holds a connection to one pending verify and the engine caps connections at 4096, so the queue is already bounded by construction and this can only trip if that invariant breaks. A cap low enough to blunt an attack would reject real players first; during a mass reconnect they *are* the queue. Flood defense belongs at the connection layer.

The real DoS improvement is elsewhere: today every attempt stalls the world, and after this a flood occupies one core while the loop keeps ticking.

## Gate

Release builds on 4+ cores. Below that there is no spare core to move work to, so off-loading buys nothing by construction; `DEBUG` is excluded because dev boxes and test shards have few logins. Both modes call the same code — the gate only chooses where it runs.

## Engine change

One property, `AccountLoginEventArgs.Deferred`, so a subscriber can say "no verdict yet". `EventSink.AccountLogin` is `Action<...>` with no continuation, and the packet handler replies in the same call. Approved separately since it touches `Projects/Server/`.

## Docs

`dev-docs/threading-model.md` and the threading skill gain a vetted-workers section. The forbidden-patterns table bans `new Thread`, `ConcurrentQueue<T>`, `Interlocked` and `volatile` in `UOContent`, and its exceptions covered only `Projects/Server/` — the existing Advanced Search fan-out already sat outside it. The new section leads with proving the need (measure on-loop time, not wall-clock; gate on core count; record the measurement), keeps game logic on the loop via chunking, and documents the hand-off protocol in both directions.

## Testing

698 UOContent tests, 810 Server tests, Release build clean.

Covered: verify and rehash outcomes, phrase rules for SHA1/SHA2 vs Argon2, stored-format stability for MD5/SHA1/SHA2, jobs with no connection attached, and dispatch ordering through the real queue. The liveness and ordering guards are mutation-verified.
2026-08-09 00:13:34 -07:00
Kamron Batman
f33bcd6006
fix: Bind the login auth id to its account and drop the redundant verify (#2564)
## What

- Bind the login auth id to the account **and** origin address that earned it, make it a CSPRNG draw, expire it after two minutes, and spend it only once its owner presents it.
- Skip the password verify on `GameLogin` (0x91) when the presented id vouches for the submitted username and address.

## Why

A full client login hashes the password twice — `AccountLogin` (0x80) and then `GameLogin` (0x91). At the current Argon2 parameters that is **most of a 16 ms frame each, on the single-threaded game loop**, for every login attempt.

The second verify is redundant. `GameLogin` already requires an id from `_authIDWindow`, and that window is only populated by `GenerateAuthID`, called from `PlayServer` — reachable only after 0x80 has already authenticated the account **in this same process**. ModernUO Gateway has its own auth-id passing mechanism and is out of scope here.

## Why the id needed hardening first

Skipping the verify promotes the id from a correlation token to a bearer token, and it was not one:

- drawn from `Utility.Random` → `BuiltInRng`, a non-cryptographic PRNG
- bound to nothing — `AuthIDPersistence` carried only `Age` and `Version`
- never expiring; `Age` was only read to pick an eviction victim

A guessed id got you nothing while the password was still checked. Without that check it would have been an account takeover, so the id is now a CSPRNG draw, single-use, two-minute TTL, and bound to both the account and the origin address.

What remains is observing a live id on the client's network or machine — which the server cannot defend against under any design, and which already yields the password itself, since the client transmits it in the same handshake.

Network switching mid-login is deliberately unsupported.

## Behaviour

A full verify was always required before this change, and ids never expired, so every "before" is a password check.

| Case | Before | After |
|---|---|---|
| Id absent | Disconnect | Disconnect |
| Address mismatch | Verify | **Disconnect** |
| Account mismatch | Verify | **Disconnect** |
| Expired | Verify | **Verify** |
| Id vouches | Verify | **Skip** |

No case grants access the previous code would have denied. Expiry deliberately falls back to the verify rather than disconnecting — a player can idle, and turning that into a lockout would be a regression for no gain.

## Look, then take

An id is not consumed until the presenter has shown it is theirs. Removing it first 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 over a packet they had no part in.

The **address is compared before the account**, so a guesser from anywhere else is rejected before a username is ever looked at. That is what makes it safe to leave the id in place on a mismatch: there is no username-enumeration risk to trade against, and the only presenter who could enumerate is already on the victim's own address.

## The window is not a cap

It was 128 entries with the oldest evicted to make room. That is a cap on *concurrent logins*, not a resource bound: 800 people picking a server at once would have live ids discarded and those clients would arrive to `"Unable to find auth id."` — a failed login caused by nothing except other people logging in.

Issuing now sweeps expired entries and lets the window grow if everything in it is still live. Unbounded is safe here: an entry costs a **successful** password verify to create and dies after two minutes, so its size tracks logins genuinely in flight.

Removing an id when its connection drops is not an option, and this was checked rather than assumed — `NetState.cs:787` disconnects the login connection *deliberately*, immediately after the id is issued, and that disconnect is never cancelled. Surviving it is the whole purpose of the id. Expiry is the only correct reclamation.

## Handshake hardening

Choosing a server queues a disconnect, but the queue drains on the *next* slice, so a client pipelining into the same recv buffer can reach the handshake handlers again. Two had no do-once guard:

- `LoginServerSeed` (0xEF) now rejects when `state.Seeded` is already set.
- `PlayServer` (0xA0) now rejects when `state.AuthId != 0` — otherwise a connection that had already spent its id would be handed the spent one back.

Issuing is also idempotent (`EnsureAuthId`), so a connection holds exactly one id by construction and an orphan is impossible rather than something to clean up. The login state machine itself is untouched.

Also fixes a fall-through: the "Unable to find auth id" branch disconnected without returning, then continued with a default entry and nulled `state.Version`.

## Testing

`ConsumeAuthId` is a seam with no `NetState` dependency, so the auth decision is tested directly: vouching, account mismatch, address mismatch, case-insensitive usernames, IPv4-mapped-IPv6, unknown ids, single-use by the owner, **a rejected attempt leaving the id redeemable**, expiry-into-verify, and an 800-id login rush that must evict nobody. Expiry is driven by moving `Core._now`, not by waiting. Every new clause was verified to discriminate by removing it and confirming only its own tests fail.

## Cost

Halves the per-login game-loop cost. This does not make hashing cheaper or move it off the loop — that is gated on a measurement described in `docs/handoffs/2026-08-07-off-loop-argon2-hashing.md`.
2026-08-08 09:25:42 -07:00
Kamron Batman
b2c59191bd
fix: Fixes Argon2 verify correctness and the password upgrade lockout (#2562)
> ⚠️ **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.
2026-08-08 00:24:59 -07:00
Kamron Batman
e1e1a7c640
fix: Bumps deps. Updates copyrights (#2353) 2026-03-05 19:36:54 -08:00
Kamron Batman
ebaf104935
chore: Use var everywhere (#2294) 2025-12-27 16:47:28 -08:00
Kamron Batman
3a3f5ee518
feat: Adds .NET 10 / C# 14 support. (#2258)
### Summary
* Adds .NET 10 support
* Bumps to C# 14
2025-11-11 11:26:34 -08:00
Kamron Batman
d5253c2bae
chore: Cleans up unused imports (#1608) 2023-11-21 12:32:51 -08:00
Kamron Batman
dba3ec2db5
fix: Use built-in RNG (#1599)
### Summary

.NET 8 supports Xoroshiro 256** off the shelf and added Shuffle. Switching to that implementation.

### Developer Notes

* Removed many convenience methods that weren't used.
2023-11-17 17:45:18 -08:00
Kamron Batman
8389bfacfe
chore: Updates copyright (#1448) 2023-08-09 09:09:26 -07:00
Kamron Batman
f268d5d4e2
fix: Cleans up core code (#1187)
**Only one functional change**
* Fixes a bug in LogFactory where `Warning` is being logged as `Information`

Non-functional changes:
* Updates/Fixes copyright headers
* Removes namespace scopes for core files.

View with [whitespace off](https://github.com/modernuo/ModernUO/pull/1187/files?w=1).
2022-10-10 21:47:08 -07:00
Kamron Batman
c31bf20d0e
feat: Updates to .NET 6 (#843)
* Fixes an issue with moving directories across volumes
* Removes usages of WebClient
* Removes usages of Cryptographic Providers

Note: Even though .NET 6 introduces Xoshiro RNG, there is no way to control the seed. I'll do some reconciliation of Xoshiro so it functions closer to the built in one. For the most part, it has parity though.
Benchmarks show there is nothing odd about the implementations, they are within 1ns of each other.
2021-11-13 13:38:01 -08:00
Kamron Batman
1f499b00c6
fix(core): Fixes issues with deserialization due to bad checking of dirty tracking (#618)
Fixes a major issue where serialization was not respecting the dirty tracking feature flag.
2021-05-26 13:47:04 -07:00
Kamron Batman
a0893b68c0
fix(core): Updates slice with range selectors (#583)
- [X] Replaces Span slicing with range selection
- [X] Replaces string slicing with range selection
2021-04-23 20:57:24 -07:00
Kamron Batman
5e3b42bf9c
fix(network): Adds ConnectUO (#551)
- [X] Adds ability to create new protocol extension packets
- [X] Adds freeshard protocol extension support (packet 0xF1)
- [X] Adds ConnectUO support
- [X] Adds arbitrary read for `CircularBufferReader` and `SpanReader`
2021-03-15 14:14:33 -07:00
Kamron Batman
952a3e0265
fix(core): Handles a rare NPE with LoginTimer (#508) 2021-02-11 23:47:02 -08:00
Kamron Batman
c58aad733d
fix(core): Converts BB packets (#406)
- [X] Organizes and splits up bulletin boards
- [X] Converts packets
2021-01-13 22:28:31 -08:00
Kamron Batman
211c2ba8ee
fix(core): Streamlines text encoding (#407)
- [X] Streamlines text encoding
2021-01-13 18:56:32 -08:00
Kamron Batman
77ce2e1980
fix(core): Optimizes strings / .NET 5 compatibility changes (#354)
- [X] Removes some string allocations (e.g. split)
- [X] Optimizes some collections
- [X] Converts insensitive to extension methods of built-ins.
- [X] Adds ordinal (case sensitive) string helpers
- [X] Fixes conditionals for in-game commands so they use Ordinal comparisons.
- [X] Replaces ToLower.Contains with InsensitiveContains
- [X] Adds ValueStringBuilder
- [X] Implements ValueStringBuilder in a few places where it makes sense
- [X] Removes the redundant Wrap function and replaces it with an optimized version
- [X] Fixes list conversions in Utility

Closes #351

Bumps release version
2020-12-20 23:21:55 -08:00
Kamron Batman
3551d962f7
C# 9 Cleanup (#325)
- [X] Removes EventArgs - not needed
- [X] Merges sequential checks
- [X] Removes redundant type declarations
2020-11-27 00:29:21 -08:00
Kamron Batman
525cda5413
Cleanup & Fixes for .NET 5 (#309)
- [X] Fixes several bugs
- [X] Updates more ordinal issues
- [X] Cleans up the code a bit
- [X] Turns classes static that should have been
- [X] Changes TcpServer.Instances to a HashSet

Bumps release version
2020-11-15 10:03:50 -08:00
Kamron Batman
ae436cb55f
Fix bad serials (#304)
- [X] Fixes bad serial calculations
- [X] Tightens HexStrings

Bumps release version
Updates documentation
2020-11-08 12:09:26 -08:00
Kamron Batman
c3289a20ab
Changes damage packets (#299)
- [X] Changes damage packets

Bumps release version
2020-11-01 22:44:23 -08:00
Kamron Batman
8149620b0c
Fixes brace style (#248) 2020-09-13 21:49:46 -07:00
Kamron Batman
741e8d8300
Cleanup/Housekeeping (#242) 2020-09-12 15:31:21 -07:00
Kamron Batman
dad5997c85
Fixes copyright headers and live template (#216) 2020-09-03 01:47:20 -07:00
Kamron Batman
ad3775c4d7
Formats UO Content (#201) 2020-08-27 18:30:38 -07:00
Kamron Batman
ff3a28ab84
Fixes RNG (#181) 2020-07-25 15:57:32 -07:00
Kamron Batman
561b40c3d8
Moves Argon2 to real nuget (#163) 2020-06-25 13:06:31 -07:00
Kamron Batman
f957a1dd5b
Cleans up Argon2 (#141) 2020-05-24 19:13:19 -07:00
Kamron Batman
5d51e97c52
Updates Argon2 to use Spans/Stackalloc (#140) 2020-05-24 18:45:30 -07:00
Kamron Batman
8ec166bcd0
Adds assemblies config, fixes crash bugs. (#134) 2020-05-09 12:55:56 -07:00