Refactor: layered architecture + pluggable hashing & token format

[ssddOnTop]

Summary

Restructures the crate into a clean, layered architecture and makes the storage-hash algorithm and token format pluggable. Pre-1.0, so this intentionally includes breaking API changes.

Architecture

Strict, one-directional dependency tree so the generate and verify paths never import each other:

manager            (orchestrator: ApiKeyManager)
  ├── generate     Generator, ApiKey<Hash>, GenerateError
  └── verify       Verifier, KeyStatus, VerifyError
        shared     checksum · hasher · token_parser · secure
          config   primitives + ConfigBuilder + ValidatedConfig

• Config up front: a single ConfigBuilder validates everything in build() and, via tailcall-valid, reports all errors at once (ConfigErrors) instead of failing on the first.
• Separate error types: GenerateError vs VerifyError (plus ConfigErrors / InitError). A wrong key is Ok(KeyStatus::Invalid), not an error.
• Shared, no cross-imports: checksum logic was extracted out of the generator into shared/checksum.rs; generate appends, verify checks, via the same type.
• Errors wrap upstream causes with #[from]/transparent rather than stringifying them.

Pluggable hashing (HashAlgo)

• Sha256 — fast, default (high-entropy keys only need an approved fast hash per NIST SP 800-63B).
• HmacSha256 { pepper } — fast + keyed; recommended for production (DB-leak resistant).
• Argon2id(Argon2Params) — memory-hard, opt-in; used by high_security().

Stored hashes are self-describing (sha256$…, hmac-sha256$…, $argon2id$…), so verification dispatches on the stored value — a database can hold hashes from multiple algorithms during a migration.

Token-format ergonomics

• Separator::Underscore (Stripe/GitHub sk_live_… style).
• Environment::Custom(String) and ConfigBuilder::no_environment() (fold role/env into the prefix).
• ChecksumBits newtype — checksum length is now specified in bits (fixes the old hex-chars-vs-bits footgun).

Breaking changes

• ApiKeyManagerV0 → ApiKeyManager; construct via ApiKeyManager::new(ConfigBuilder::…​.build()?).
• HashSpec → Argon2Params; hash_params(m,t,p) → hash(HashAlgo::…).
• Default hash is now SHA-256 (was Argon2id). Old $argon2id$ hashes still verify (prefix dispatch).
• checksum(algo, usize) → checksum(algo, ChecksumBits).

Testing

Full suite green (84 lib + integration + doctests), clippy --all-targets clean.

Draft: opening for early review; commit history may be squashed before merge.

[ssddOnTop]

Review: architecture 👍, but one confirmed panic bug, one threat-model gap, and some "minimal" goals not yet met

I checked out the branch and verified locally: cargo fmt --check, cargo clippy --all-targets, and the full test suite (85 lib tests + integration + doctests) are all green. The layering is real — I verified generate and verify genuinely never import each other, and shared/checksum.rs is a clean extraction of the old verify → generator reach-around. Separate GenerateError/VerifyError, Ok(KeyStatus::Invalid) for wrong keys, #[from]/transparent error wrapping, self-describing hash tags, and ChecksumBits are all keepers. Dropping example_key() before merge was the right instinct.

That said, I found issues worth addressing before merge — details in the inline comments, summary here:

🐛 Bugs

1. Confirmed overflow panic on attacker-controlled input in verify_expiry (verify.rs:186). I reproduced it: a crafted key with expiry ≈ i64::MAX panics any build with overflow checks on (debug, or release with overflow-checks = true). Unauthenticated remote DoS. Repro in the inline comment. (Carried over from old validator.rs, but this PR rewrites the function — now is the time.)
2. PR description no longer matches the code: the summary says "Sha256 — fast, default", but commit 80b3c8d made HmacSha256 the default with a required pepper — ConfigBuilder::new().prefix("sk").build() now fails with EmptyPepper. That's a defensible security decision, but it's a breaking behavioral change and the PR body should advertise it accurately.
3. Several doc fragments still describe the Argon2-by-default era (inline comments on generate.rs, verify.rs, README).

🔐 Security decision needed

4. Tag-dispatch verification silently defeats the pepper under a DB-write threat model (shared/hasher.rs). The pepper's pitch is "a leaked key DB alone can't verify keys" — but an attacker with DB write access can insert sha256$<hex> of a self-generated key and an HMAC-configured service will verify it. The migration story is good; it just needs its threat-model fine print (or an opt-in allowlist). Inline comment has options.

📦 Overdone / not minimal (the PR's own stated goal)

5. tailcall-valid pulls in serde, serde_json, http, serde_path_to_error, derive_setters, wasm-bindgen (verified via cargo tree) — and then ConfigErrors::from_causes discards the .trace(...) field context, the library's one differentiating feature over a plain Vec. A ~20-line hand-rolled accumulator produces byte-identical output with zero new deps.
6. regex + lazy_static can both be dropped: the r"v\d+" regex is duplicated by a hand-rolled is_version_like in generate.rs anyway, and Environment::variants() (the only other lazy_static user) has zero callers.
7. Smaller redundancies: verify() parses the token twice and length-checks twice; Verifier constructs a second KeyHasher the manager already built.

⚠️ API footguns (pick your battles)

8. .hash() / .pepper() silently overwrite each other — order-dependent builder in an otherwise report-everything design.
9. Environment::Custom(String) pub field bypasses the normalization that Environment::custom() applies.
10. ChecksumSpec.length is pub and denominated in hex chars — the exact unit footgun ChecksumBits was introduced to kill.
11. Error-taxonomy asymmetry in the expiry path (Err(MalformedInput) vs Ok(Invalid) for equally-hostile garbage).

🚦 CI

All PR checks are red (Lint, Build and Test, Analyze, SonarQube), each dying in 2–8s with no failed steps and no retrievable logs — smells like workflow/infra rather than code (everything passes locally). Worth diagnosing separately, but this shouldn't merge with an unexplained red pipeline.

Suggested pre-merge checklist

• Fix verify_expiry overflow (saturating_add) + regression test with near-i64::MAX expiry
• Document (or gate) the tag-dispatch/pepper-downgrade behavior
• Replace tailcall-valid with a local accumulator, or justify the dep tree
• Deduplicate version-like detection; drop regex + lazy_static; delete unused variants()
• Sync PR body + stale Argon2-era docs (README checksum diagram still says 16 hex chars; min is now 32)
• Optional: single parse_token per verify, share the KeyHasher, builder conflict detection

None of this invalidates the architecture — items 5–6 actually complete the PR's stated minimalism goal.

[ssddOnTop]

🐛 Confirmed bug: overflow panic on attacker-controlled input (unauthenticated DoS in debug / overflow-checks = true builds).

expiry_timestamp comes straight out of the presented token. With expiry = i64::MAX - 3 (a valid canonical no-pad base64url encoding) and any non-zero grace, expiry_timestamp + grace overflows. The BLAKE3 checksum is unkeyed, so an attacker can compute it themselves — the crafted key sails through the checksum stage and panics here.

I reproduced it against this branch:

let hostile: i64 = i64::MAX - 3;
let exp_b64 = URL_SAFE_NO_PAD.encode(hostile.to_be_bytes());
let mut h = blake3::Hasher::new();
h.update(body.as_bytes());          // key body without real checksum
h.update(exp_b64.as_bytes());
let sum = h.finalize().to_hex()[..32].to_string();
let crafted = SecureString::from(format!("{body}.{exp_b64}.{sum}"));
let _ = m.verify(&crafted, stored_hash);

thread '...' panicked at crates/api-keys-simplified/src/verify.rs:186:12:
attempt to add with overflow

In release (wrapping) it silently wraps negative and returns Invalid, which masks the bug. Fix:

if expiry_timestamp.saturating_add(grace) < now {

This predates the PR (same expression in old validator.rs), but this PR rewrites the function — now is the moment. Please also add a regression test with a near-i64::MAX expiry.

[ssddOnTop]

Related nit: as_secs() as i64 can itself go negative for absurd configured durations (Duration::from_secs(u64::MAX)), which would let expired keys verify (expiry + negative_grace < now flips). grace_period is the one builder field with no validation in ConfigBuilder::build() — a cheap range check there (or i64::try_from(...).unwrap_or(i64::MAX) here) would close this off.

[ssddOnTop]

🔐 Security decision needed: tag-dispatch silently defeats the pepper under a DB-write threat model.

The pepper's advertised guarantee (docs, README, PR body) is "a leaked key database alone cannot be used to verify keys." But because dispatch is on the stored string's tag rather than the configured algorithm, an attacker with DB write access can insert sha256$<hex> of a key they generated themselves, and an HMAC-configured service will verify it — no pepper required. The migration feature doubles as a permanent algorithm-downgrade path.

The tests actively celebrate this (verify_dispatches_on_stored_prefix_not_config), so it's clearly intentional — it just needs its fine print. Options, cheapest first:

1. (a) Document the caveat on HashAlgo / here: "tag dispatch means an attacker who can write to the hash column can bypass keyed hashing."
2. (b) Accept only the configured algo's tag by default, plus an explicit opt-in allowlist (e.g. .also_accept(&[HashTag::Argon2id])) for migrations.

I'd do (a) now and consider (b) later; shipping it silently is the only wrong option.

Minor related note: an unknown/corrupted tag returns false, indistinguishable from "wrong key" — deliberate for timing-oracle reasons, but ops-wise a corrupted hash column becomes a silent lockout. Worth a sentence in the docs.

[ssddOnTop]

📦 Main "keep it minimal" objection: tailcall-valid costs ~6 transitive deps and its one differentiating feature is discarded right here.

cargo tree confirms this dep pulls serde, serde_json, http, serde_path_to_error, derive_setters, and wasm-bindgen into a security-focused crate that previously had a lean tree — to validate a config builder.

And on this line, .map(|c| c.error) throws away every .trace("prefix") / .trace("checksum") field annotation — the applicative validator's only advantage over a plain Vec. The observable behavior of the whole Vc<A> + fuse + trace machinery reduces to "push errors into a Vec".

A hand-rolled accumulator produces byte-identical ConfigErrors with zero new deps:

let mut errors = Vec::new();
let prefix = validate_prefix(self.prefix, &mut errors);
let entropy = validate_entropy(self.entropy_bytes, &mut errors);
// ...
if errors.is_empty() { Ok(ValidatedConfig { .. }) } else { Err(ConfigErrors(errors)) }

The accumulate-all-errors UX is genuinely good and worth keeping — the dependency is not. (If you do keep it, at least stop discarding the trace context; render it in Display as prefix: ....)

[ssddOnTop]

📦 Duplicated rule + two droppable deps. This hand-rolled is_version_like duplicates the r"v\d+" regex in config.rs. The comment says it's "kept local to avoid a cross-layer dependency" — but generate already imports config (that's the whole architecture: arrows point down; see the imports at the top of this file).

Suggestion: move is_version_like into config as pub(crate), use it for prefix validation too, and drop both regex and lazy_static from Cargo.toml. Two fewer deps, one source of truth for the rule, and the config-layer and generate-layer definitions can never drift apart.

[ssddOnTop]

Redundant work per verify() call (not a bug, just accidental duplication in a brand-new design):

1. The token is parsed twice — once in verify_checksum(), again in verify_hash_and_expiry() — and the key is length-checked twice. A single parse_token up front whose Parts are passed to both stages would remove it.
2. Two lines up in Verifier::new (line 73), the verifier builds its own KeyHasher from config — while ApiKeyManager::new already constructed an identical one two lines earlier and passes it around separately. Passing the hasher into Verifier::new (or having the manager own a single one) removes a divergence trap: today nothing guarantees the manager's hasher and the verifier's hasher agree beyond both being derived from the same config clone.

[ssddOnTop]

Stale doc (Argon2-era): Hash is no longer necessarily "The Argon2 hash (PHC format)" — with the new default it's an hmac-sha256$… string, which is deterministic (no per-hash salt), so "changes each hash due to random salt" below is also wrong for the default. Suggest: "The stored-hash string (self-describing: sha256$…, hmac-sha256$…, or $argon2id$… PHC) plus a stable key identifier."

[ssddOnTop]

Stale doc: "checksum → Argon2 → expiry" — Argon2 is no longer the (only or default) hash. "checksum → hash verify → expiry" matches the dispatch behavior described in the module docs above.

[ssddOnTop]

Two doc issues in this README:

1. The 2900x DoS framing is stale under the new default. "2900x faster" compares BLAKE3 (~µs) to Argon2 (~300ms). With the new HMAC-SHA256 default, full hash verification is already microseconds, so the checksum's remaining value under the default config is skipping a DB lookup via verify_checksum() (and staying cheap if someone opts into Argon2id). That's still real — but a different story than the one told here and in the "📊 DoS Protection" section below. Recommend reframing rather than deleting.

2. The "Key Format" diagram further down (lines ~96–107, unchanged by this PR) is now wrong: it says "BLAKE3 (recommended, 16 hex chars)" and the examples end in 16-hex-char checksums (.a1b2c3d4e5f6g7h8). The new minimum is 128 bits = 32 hex chars — 16 is no longer even representable via ChecksumBits. Since this PR changed the checksum rules, the diagram should follow.

[ssddOnTop]

Error-taxonomy asymmetry: an unparseable token → Ok(KeyStatus::Invalid) (per the crate's own "wrong key is not an error" rule), but a token that parses and then fails canonical base64 decode of the expiry → Err(VerifyError::MalformedInput). Both inputs are equally attacker-controlled garbage that survived the checksum only if the attacker computed it deliberately.

Not a bug, but callers now have to handle hostile input on both the Ok(Invalid) and Err(_) paths. Collapsing this to Ok(Invalid) (keeping Err only for the length-limit DoS guards, which genuinely indicate caller/infra issues) would make the contract stated on VerifyError ("only oversized / genuinely malformed inputs surface here") simpler and truer — right now InputTooLong is the only variant a well-behaved caller can ever see... except this one.