Build, lint and test hofmann-rust in CI - #83
Merged
Conversation
cargo test never ran in CI -- "Analyze (rust)" is CodeQL, not a test job -- which is how the p256 0.14 bump in #71 broke the build on main undetected. Adds a `rust` job running cargo fmt --all --check, clippy and cargo test. ubuntu-latest ships a stable toolchain with cargo, rustfmt and clippy, so no third-party toolchain action needs pinning; the cargo registry and target directory are cached on Cargo.lock. Both cargo commands pass --locked so a manifest edit committed without its lockfile update fails rather than silently resolving something else. Clippy runs with -D warnings, which required clearing the three warnings the crate already had, all in test code and all machine-applicable fixes from cargo clippy --fix: - expand_message_xmd.rs: repeat().take() -> repeat_n() - opaque_roundtrip.rs (x2): .expect(&format!(..)) -> .unwrap_or_else(|_| panic!(..)) Note that -D warnings makes a future toolchain bump able to turn new clippy lints into a red build. That is the tradeoff for having a lint gate with teeth; a rust-toolchain.toml would pin it if that becomes annoying. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
v3.0.0 is released: tagged, GitHub release published, and the Java artifacts pushed to Maven Central. Moves the working versions to 3.0.1 -- gradle.properties to 3.0.1-SNAPSHOT, hofmann-rust Cargo.toml (and Cargo.lock) and hofmann-typescript package.json (and package-lock.json) to 3.0.1. Note that gradle.properties only supplies the version for untagged builds: settings.gradle.kts overrides it from an exact-match git tag on HEAD, so the released Java version always comes from the tag rather than from this file. The Rust and TypeScript versions are literal, so those two are what `cargo publish` and `npm publish` would push. Verified at 3.0.1: Java clean build with 609 tests passing, Rust cargo fmt --all --check / clippy -D warnings / cargo test all clean under --locked, and TypeScript typecheck, build and tests passing. The --locked runs matter here because bumping the crate version rewrites Cargo.lock, and the new rust CI job from #83 would fail on a stale one. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
….0) (#85) * Open the 3.0.1 development cycle v3.0.0 is released: tagged, GitHub release published, and the Java artifacts pushed to Maven Central. Moves the working versions to 3.0.1 -- gradle.properties to 3.0.1-SNAPSHOT, hofmann-rust Cargo.toml (and Cargo.lock) and hofmann-typescript package.json (and package-lock.json) to 3.0.1. Note that gradle.properties only supplies the version for untagged builds: settings.gradle.kts overrides it from an exact-match git tag on HEAD, so the released Java version always comes from the tag rather than from this file. The Rust and TypeScript versions are literal, so those two are what `cargo publish` and `npm publish` would push. Verified at 3.0.1: Java clean build with 609 tests passing, Rust cargo fmt --all --check / clippy -D warnings / cargo test all clean under --locked, and TypeScript typecheck, build and tests passing. The --locked runs matter here because bumping the crate version rewrites Cargo.lock, and the new rust CI job from #83 would fail on a stale one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record the August 2026 security review in TODO.md Replaces the February 2026 checklist with the findings from a six-reviewer audit covering hofmann-rfc, hofmann-server, hofmann-client, both framework integrations, and the Rust/TypeScript ports. Every entry on the old DONE list was re-verified against the current tree. The claims that hold are compressed into a "Confirmed sound" section so they are not re-litigated; four that did not hold are reopened as work items and tagged [was marked DONE]: - OWASP Dependency-Check is referenced nowhere in the build. CI runs dependency-submission, which feeds Dependabot alerts but is not a gate. - The AutoCloseable zeroization on ClientAuthState/ClientRegistrationState is never invoked: no try-with-resources and no close() call exists in any src/main tree. - The point-validation entry credits OpaqueCrypto.deserializePoint and OctetStringUtils.toEcPoint, neither of which exists. The validation is real but lives elsewhere. - The constant-time work covered modInverse and scalar serialization only; ECPoint.multiply on the server's long-term key was never addressed. Items demonstrated by executing code are tagged [reproduced]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reject the identity element when decoding a ristretto255 point RFC 9497 §2.1 requires DeserializeElement to reject the group identity. The ristretto255 suite did not, because the all-zero 32-byte string is a *valid* ristretto255 encoding — it is the canonical encoding of the identity's equivalence class, so every RFC 9496 §4.3.1 decode check passes for it and none of them can catch it. The consequence: blindInv * O = O, so OprfCipherSuite.finalize() degraded to H(len||input||len||0^32||"Finalize") a function of the input alone, independent of both the blind and the server key. A malicious, breached, or MITM'd server answering with 32 zero bytes silently turned the OPRF into an unkeyed, unsalted hash, and mode 0x00 has no verifiability proof for the client to detect it with. This affected the standalone OPRF product (OprfClientManager.hashResult) as well as OPAQUE, where the envelope MAC degraded it to an authentication failure instead. P-256/384/521 already rejected the identity; ristretto255 was the only affected suite, and Java was the only affected implementation — the Rust and TypeScript ports both guard this already. The guard goes in decodeRistretto255 rather than in finalize() because decodeRistretto255 has exactly one caller, scalarMultiply(), which is the shared entry point for the OPRF unblind and all six OPAQUE 3DH operations. One check covers every caller, and it matches where the Rust port guards. Also fixed, all found while verifying the above: - finalize() accepted a blind congruent to 0 mod n, which inverts to 0 and produces the identical key-independent collapse from the caller's side rather than the server's. Affects all four suites. No in-tree path can reach it (randomScalar samples [1, n-1]) but finalize() is public API. - The new guard turned a malformed group element at POST /opaque/auth/start from HTTP 400 into HTTP 500, because that endpoint — alone among the eight, in both frameworks — did not catch SecurityException. That endpoint is unauthenticated and the unknown-credential path reaches it identically, so this is now mapped to 400 in OpaqueResource and OpaqueController. Mapped to 400 rather than 401 because nothing is authenticated at that stage. - ByteUtils.isAllZero's javadoc claimed decodeRistretto255 accepts the all-zero encoding, which this change makes false. Adds regression coverage across all four suites for identity rejection, for the key-independence property itself, and for blind validation, mirroring the existing TypeScript regression in test/oprf.test.ts. hofmann-rfc: 377 tests, 0 failures. Verified by mutation — with the guard commented out, exactly the three new ristretto255 assertions fail and no pre-existing test does, which also confirms no legitimate path ever needed to decode the identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Canonicalize the credential identifier at the DTO boundary The server carried two identity keys for the same user. CredentialStore keys on the DECODED BYTES (InMemoryCredentialStore.ByteKey), while the JWT `sub` claim, InMemorySessionStore and its credentialToJtis reverse index, and every rate-limiter bucket key on the RAW BASE64 STRING the client sent. Base64.getDecoder() is non-canonical: it ignores both the padding and the unused trailing bits of the final character. So YWxpY2U=, YWxpY2U, YWxpY2V, YWxpY2V=, YWxpY2W= and YWxpY2X= all decode to "alice", and nothing normalized them. The alias count depends on identifier length — 1 for len%3==0, 8 for len%3==2, 32 for len%3==1 — so roughly two thirds of a real user base was affected. Two consequences, both reproduced end to end against real OPAQUE crypto and the real in-memory stores: - REVOCATION BYPASS. Every account-mutating operation performs a bytes-keyed mutation beside a string-keyed revocation (:319-320 recovery, :360-361 delete, :414-417 change-password). A session opened under one spelling survived a password change or account deletion performed under another, staying valid until natural JWT expiry. This contradicted the Javadoc on both methods and the contract stated in SessionStore. - RATE-LIMIT MULTIPLIER. Each spelling drew from its own token bucket while resolving to the same account: measured 48 attempts from a capacity-6 recovery bucket (8x) for a 17-byte identifier. That is the control standing between a deployment and online guessing, and it also gates the recovery OTP. Fixed by canonicalizing once at the trust boundary: CredentialIdentifiers decodes and re-encodes, and all six request models carrying an identifier override the generated record accessor. Every downstream consumer — session index, JWT subject, rate-limit key, log line — now sees one spelling per identifier. Verified that all six models are the complete set, and that every rate-limiter call site (6), revocation site (3), and the single JWT issuance path resolve through these accessors. Note that a record's equals/hashCode/toString still bind the raw field rather than the accessor. Confirmed nothing security-relevant depends on that: these DTOs are request-scoped and are never used as map keys, compared, or placed in collections anywhere in main sources. This also removes a pre-existing failure mode rather than adding one — a client that varied its padding between recovery steps previously got "Recovery token does not match credential" and was locked out. All four spelling combinations across recoveryStart/recoveryVerify/registrationFinish now succeed. Adds a server-level regression test alongside the model-level ones, because the canonicalization lives in hofmann-rfc but the vulnerability manifests in hofmann-server: dropping the override from any single model must fail where the property actually matters. All three assertions fail without the fix. Also bounds canonicalize() by the same 4096-char cap the models use, so an oversized identifier is rejected before a full decode-and-re-encode. Upgrade note: a JWT issued before this change under a non-canonical spelling carries the raw subject and will no longer match on delete or change-password until the user re-authenticates. No client in the repo (Java, TypeScript, or Rust) ever emits a non-canonical spelling, and sessions are in-memory with a default 3600s TTL, so this should not be observable in practice. Full build green: 667 tests, 0 failures, including 104 integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Refuse server-supplied key-stretching parameters below a client floor In OPAQUE the key-stretching function runs entirely on the CLIENT, so its parameters decide how expensive an offline dictionary attack is against the record the server stores. Both clients took those parameters from the server over the wire with no floor: - Java: OpaqueClientConfig.fromServerConfig() adopted argon2MemoryKib / Iterations / Parallelism verbatim, and argon2MemoryKib == 0 selected OpaqueConfig.IdentityKsf, whose stretch() returns the input unchanged. Reached from the production @Inject path via clientFor(). - TypeScript: the same in OpaqueHttpClient.create(). A malicious, breached, or MITM'd server answering GET /opaque/config with {"argon2MemoryKib":0} at REGISTRATION time makes the client store a record derived from an unstretched password — offline attack cost falls from 64 MiB of Argon2id per guess to roughly one hash. The server keeps serving the same config afterwards, so authentication continues to work and nothing looks wrong from either side. The quieter variant (8 KiB, one iteration) is the same attack with a smaller footprint. The server already gates this behind allowIdentityKsf and refuses to start without it; the client had no counterpart. Both clients now enforce a floor of 19456 KiB / 2 iterations — the OWASP Argon2id minimum at t=2,p=1, below the server's own 65536/3 default and equal to the mobile target MIGRATION.md already nominates. Opting out is a local decision only: fromServerConfig(cfg, true) in Java, { allowWeakServerKsf } in TypeScript, or pinning a config through the client manager's overrides map, which does not consult the server at all. Nothing remote can set it. The TypeScript guard needed more than a magnitude comparison. Casting the parsed JSON to the DTO type is a compile-time assertion with no runtime force, so the fields are whatever the server sent; any non-numeric value yields NaN, every comparison with NaN is false, and control fell through to identityKsf. Omitting a single JSON key restored the entire vulnerability through the guarded entry point. The parameters are now type-checked with Number.isInteger BEFORE any magnitude check, and — more importantly — create()'s strict path no longer contains a branch to identityKsf at all: it passes the validated values straight to argon2idKsf, so there is nothing for a coercion bug to land on. That structure holds even with the type check disabled, where it fails closed at stretch time rather than silently omitting stretching. Java was never exposed to this: Jackson refuses to bind a non-integer to int. Also adds memory, iteration, and parallelism ceilings in both languages. These are DoS hardening, not security floors: Argon2id cost is linear in iterations and was unbounded, so a server could hang the client on its first registration just as effectively as with an absurd memory request. Two test harnesses now opt in explicitly, both against servers that genuinely serve sub-floor parameters: HofmannOpaqueClientManagerTest (identity KSF) and cross-client.test.ts (real Argon2id at 1024/1 for test speed, per hofmann-integration-tests/src/test/resources/application.yml). hofmann-demo and hofmann-testserver both run 65536/3, so demo.ts and integration.test.ts correctly remain strict. Deferred deliberately, recorded rather than dropped: - Pinning context locally instead of adopting the server's. Real, but a lower-severity relay concern and an API break for every caller, so it needs its own change with a migration path. - Enforcing https on the client endpoint. SECURITY.md documents TLS-terminating proxies with plaintext on a private network as a supported topology, and this fix defends against a server reached legitimately over TLS that is itself malicious, so scheme-checking would not have stopped any of it. BREAKING: fromServerConfig(cfg) and OpaqueHttpClient.create(url) now throw against a server offering the identity KSF or sub-floor parameters. Warning and proceeding was considered and rejected — a warning the attacker's own payload triggers does not stop the client writing an unstretched record. hofmann-client 27 tests, hofmann-typescript 80 tests, and no assertion failures anywhere in the Java suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Throttle registration/finish and stop it revealing whether a credential exists POST /opaque/registration/finish is unauthenticated and was, on the non-recovery path, completely unthrottled: the rate limiter was consumed only inside the bearerToken branch. It also branched observably on existence — an already-registered credential threw IllegalArgumentException (HTTP 400) while a new one returned HTTP 204. Together that is a free, unlimited, unauthenticated user-enumeration oracle, and it defeats the enumeration resistance authStart spends an entire fake-KE2 code path providing. Two changes: - The non-recovery path now consumes a registration token BEFORE the existence lookup, so a probe costs the same whether or not the credential turns out to exist. Ordering is the security-relevant part: consuming afterwards would leave probing of *registered* accounts free. - An already-registered credential now returns normally without storing, instead of throwing. The property that matters is "does not overwrite", and that is preserved and directly asserted on the stored bytes. Signalling *why* nothing was written is what leaked. Both branches now run under a 25 ms floor, mirroring the existing RECOVERY_VERIFY_MIN_NANOS. Without it the unification is only true for InMemoryCredentialStore: the not-exists branch performs a write the exists branch skips, which measures ~340 ns in memory but is an INSERT of commonly 0.2-5 ms against the database-backed CredentialStore the docs recommend — remotely measurable, and enough to reopen the oracle at the HTTP layer. Verified by execution: both adapters now return byte-identical 204 responses (status, body, and headers) for existing and non-existing credentials; the victim's record is unchanged after an attacker POST and they still authenticate with their original password; the attacker's password is rejected; base64 alias spellings of the identifier do not bypass the guard. WHAT THIS DOES NOT FIX. Rate limiting does not meaningfully mitigate identifier squatting, and an earlier draft of this message claimed otherwise. The limiter is keyed per identifier and squatting an unused identifier costs exactly one token, so the bucket is never the constraint: 2000 of 2000 distinct identifiers were squatted in 9 ms with the default limiter installed. An attacker can still bind any unregistered identifier to an unusable record with a single request and no valid OPAQUE data, after which the real owner can never register — and, because of this change, receives no error explaining why. The only in-library control that would touch this is an IP-dimension limiter, which this class structurally cannot apply because it is framework-agnostic and never sees the request context; the adapters could. Eliminating it needs proof of identifier ownership at the deployment layer. Recorded in TODO.md rather than papered over. Also corrects a mock in OpaqueResourceTest that threw "Credential already registered" — a message the manager can no longer produce. Test note: registrationFinish_isRateLimited now registers ALICE first so it exercises the already-exists branch. As originally written it only covered the not-exists path and passed even with the rate-limit check moved after the existence lookup, so it asserted that a token was consumed but not that the ordering held. It now fails against that mutation. Full Java suite 687 tests, 0 failures, integration suite included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Track P0/P1-1 as done and record three findings from verification Marks the three P0 items and P1-1 complete with their commit hashes, keeping the analysis pointers rather than deleting the entries outright, since each records a reproduction worth not losing. Adds three items surfaced while verifying e63db78 rather than by the original review: - Squatting needs an IP-dimension limiter, which only the adapters can apply. Recorded explicitly because the per-identifier limiter does NOT mitigate it, and saying otherwise would leave a false reassurance in the file. - Spring Boot flattens every error status to 401, so the new throttle is invisible to Spring clients. Pre-existing and reproducible on untouched endpoints. - A junk recovery bearer token drains the recovery limiter before validation, locking a victim out of the flow that would rescue a squatted account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Validate client-uploaded registration records before storing them RegistrationFinishRequest base64-decodes clientPublicKey, maskingKey, envelopeNonce and authTag and handed them straight to credentialStore.store(). Nothing checked the lengths against the suite's Npk/Nh/Nn/Nm, and the client public key was never checked to be a group element at all. The failure then landed at authentication time, where it is worse than a bad request. createCredentialResponse XORs serverPublicKey || envelope against a fixed-width pad, so a wrong-length envelope threw on the mismatch: /auth/start answered a poisoned identifier with an error while an unknown identifier got a fake KE2. That is an enumeration oracle, and a wrong-length public key produced a third distinguishable outcome one step later. Adds Server.validateRegistrationRecord, which checks every field against the suite's sizes and forces a group-element decode of the client public key via scalarMultiply(ONE, pk) — GroupSpec exposes no deserialize method, and scalarMultiply routes through the same on-curve, non-identity and canonical- encoding validation the AKE depends on. Called from all three write paths to the credential store, each positioned so the check cannot be turned into a weapon of its own: - registrationFinish, normal path: after the rate-limiter token is consumed, so the decode cannot be driven unthrottled. That matters most on ristretto255, where the Montgomery ladder runs a fixed 253 iterations regardless of the scalar and costs ~1.2 ms. - registrationFinish, recovery path: after its limiter but before recoveryTokenStore.remove(), so a malformed record neither burns a legitimate recovery token nor reaches the delete. - changePasswordFinish: after the JWT check but before the delete. This is the third write path and was missed on the first pass — leaving it unguarded reproduced the original defect exactly, just scoped to an attacker's own account. Failures are normalised to IllegalArgumentException (HTTP 400). Left alone the status would depend on the suite rather than the fault: BouncyCastle rejects a bad compressed point with IllegalArgumentException on the NIST curves, while ristretto255 raises SecurityException, which the adapters map to 401 — an authentication challenge on an unauthenticated endpoint the caller has no credentials to satisfy. SCOPE, stated precisely because an earlier draft of this claimed more. This closes the enumeration oracle. It does NOT prevent a caller storing a record that is well-formed but cryptographically meaningless: a valid point with random envelope bytes passes every check and still leaves the identifier unauthenticatable. Only the password holder can produce a record that verifies, so on an unauthenticated registration endpoint that is inherent, and the answer is proof of identifier ownership at the deployment layer. Tests run over all four cipher suites rather than the P-256 default, because P-256 has Nh == Nm == Nn == 32 and hides any confusion among them: swapping the nonce's constant from Nn to Nh passed the entire suite when the tests were P-256 only, and now fails on P-384, P-521 and ristretto255. P-521 additionally separates Npk (67) from Nsk (66) from Nh (64) and is covered. The off-curve case now searches for an encoding that genuinely fails to decode. Flipping bits in a compressed x-coordinate is not enough — roughly half of all x values have a square root, so the flipped encoding is a valid point about half the time. The original test failed 7 of 10 real runs; the replacement passed 5 of 5 forced reruns, and both the decode-deletion and constant-swap mutations are now caught deterministically. Full Java suite 732 tests, 0 failures, integration suite included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Validate and normalize the OPRF server key instead of trusting config The OPRF secret key was read as new BigInteger(masterKeyHex, 16) at both framework configuration sites and never checked. oprfMasterKeyHex: "00" on ristretto255 makes BlindEvaluate return the identity element for every request — and because the identity is a decodable ristretto encoding, the deployment serves traffic normally while having no effective key at all. Together with the client-side identity acceptance fixed in 71846ed, that was a silently keyless OPRF at both ends. Rejects a key congruent to zero modulo the group order, at startup and again per request. Does NOT reject a key at or above the group order, which was the first attempt and was wrong. Scalar multiplication reduces modulo the order anyway, so such a key already works and is simply a spelling of k mod n — and the recipe the docs recommend, openssl rand -hex 32, produces a value above ristretto255's order about 94% of the time (measured 188/200). Refusing them broke 26 integration tests, and would have broken live deployments whose stored OPRF outputs only that exact key reproduces. Those keys are now normalized instead. Normalization is transparent by construction — Ristretto255GroupSpec.scalarMul reduces as its first statement, and the NIST curves have cofactor 1 with subgroup-validated points — and a warning is logged, since a key above the order means the operator's generation recipe does not match their curve and nothing else would tell them. The per-request check exists because the supplier is a documented key-rotation seam and can introduce a key startup never saw. Two supporting details: - process() now snapshots supplier.get() ONCE. It called it twice, so a request straddling a rotation could compute a hash with key A and label it key B — a stored value that can never be recomputed or verified. This was filed as a separate lower-priority item but is a precondition for the check above: without the snapshot, validate and multiply could read different keys and the new check would itself be racy against the seam it defends. - The per-request failure is raised as IllegalStateException, not the IllegalArgumentException validateSecretKey throws. Both HTTP adapters catch IllegalArgumentException on this path and rewrite it to 400 "Invalid EC point data" without logging, so a key gone bad under rotation would blame every client for a server fault, discard the reason, and leave health checks green — the same "looks healthy while broken" mode this change exists to remove, relocated rather than fixed. It now surfaces as a 5xx with an ERROR log naming the cause. Cost of the per-request check is a single BigInteger mod: 57 ns against a 169 us scalar multiplication on P-256, 14 ns against 1.5 ms on ristretto255. It adds no side-channel surface — ristretto's scalarMul performs the same reduction as its first line regardless. Tests cover all four suites and pin the startup behaviour at the configuration sites themselves, which the first version did not: deleting the validation from both config sites left the entire suite green, because the per-request check still refused the key so no identity was ever emitted. What was lost was failing at startup rather than on every request, which is the whole reason those two files are touched. Full Java suite 783 tests, 0 failures, integration suite included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Use a constant-time ladder for scalar multiplication on the NIST curves scalarMultiply and scalarMultiplyGenerator called p.multiply(scalar), which BouncyCastle resolves to WNafL2RMultiplier for all three NIST prime curves — confirmed by reflection on the live curve objects, not assumed. Window-NAF leaks the scalar three ways: the add/double sequence follows its digits, the precomputed table is indexed by secret values, and the window size comes from its bit length. Measured on P-256 with two scalars of identical bit length and Hamming weight 2 vs 166: scalarMultiply +17% to +19% scalarMultiplyGenerator +12.9% Every scalar on these paths is secret: the server's long-term OPRF key, the per-credential OPRF key, the server's long-term and ephemeral AKE keys, the client's blind, and the client's recovered long-term private key. Server-side blindEvaluate is the sharpest target — the attacker supplies the point, so BouncyCastle's per-point precomputation cache misses on every request and the signal stays clean, and evaluations against a long-lived key can be requested without limit. P256_SHA256 is the default suite. BouncyCastle 1.85 no longer ships MontgomeryLadderMultiplier — it exposes WNaf, GLV, FixedPointComb and WTauNaf, none suitable for a secret scalar on a curve without an endomorphism — so the ladder is written out over BouncyCastle's point arithmetic. This is scheduling, not new field code: the underlying operations remain BouncyCastle's audited point formulas. Both paths now use it. The generator path was missed on the first pass and is not a lesser case: OpaqueEnvelope.recover recomputes the client's long-term private key from the same secret value on every authentication, which is precisely the repeated-measurement pattern that makes a 13% signal dangerous. The scalar is rescaled to a fixed width first. Adding the group order is free arithmetically — n·P = O — but it fixes the bit length, which matters because BouncyCastle short-circuits addition on the point at infinity: without the rescale, iterations above the leading set bit are cheaper, and a 3-bit scalar ran about twenty times faster than a full-width one. That channel carried only ~2 bits for a uniform key, but closing it costs nothing measurable. After, all four channels are within noise on P-256: scalarMultiply hamming weight, equal width -1.43% scalarMultiply 3-bit vs full width -1.49% scalarMultiply 128-bit vs full width -0.11% scalarMultiplyGenerator hamming weight +0.13% Cost is about 2.4x on the primitive. End to end the server pays roughly 0.5 ms extra per login on P-256 (0.66 -> 1.19 ms), and on the client the production Argon2id KSF at ~120 ms swamps it entirely — full P-256 authentication is within noise. Deliberately no configuration switch: a "fast but leaky" toggle on this path would be a footgun. Residual, documented in the code rather than glossed: the two-element accumulator array is indexed by a secret bit. Both references share a cache line and it is a far smaller target than wNAF's precomputed table, but the access pattern to the two heap objects stays observable to a co-located attacker using cache probing. Removing that needs field-level constant-time primitives BouncyCastle does not expose. Correctness is the risk that matters here, so it is checked against BouncyCastle as an oracle: ~70,000 comparisons across all four Weierstrass curves with random and boundary scalars, the ladder invariant verified at every individual step, and the RFC and cross-implementation vectors re-run rather than accepted as UP-TO-DATE. The TypeScript client, which uses entirely independent arithmetic, still round-trips against the Java server. One test deliberately asserts wall-clock behaviour. A refactor that drops the rescale leaves every functional test green — the extra iterations are provable no-ops — so nothing else pins the property the change exists to provide. The threshold is 2x against a pre-fix gap of about 20x, using an interleaved median, so the margin to noise is large; an earlier structural attempt that tested the rescale helper directly did NOT catch that mutation, because removing the call leaves the helper's own contract intact. Full Java suite 817 tests, 0 failures, integration suite included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Harden the release pipeline against supply-chain compromise Both release workflows imported the GPG signing key and wrote signing.gnupg.passphrase into ~/.gradle/gradle.properties, then ran unpinned third-party actions in the SAME job with those credentials still on disk and CENTRAL_PORTAL_USERNAME / PASSWORD / GPG_PASSPHRASE / GPG_KEY_ID reachable. Whoever controlled a mutable tag — softprops/action-gh-release@v2 or gradle/actions/setup-gradle@v3 — could have taken the code-signing key and the Central Portal credentials and published signed artifacts under com.codeheadsystems:*. Three changes: - Every action is now pinned by commit SHA rather than a mutable tag, across all five workflows. gradle.yml already did this for gradle/actions; the practice simply had not reached the release path, which is the one that holds the credentials. Each pin keeps the SAME major version already in use and carries the exact version in a trailing comment — the resolved SHAs for the latest majors were deliberately not taken, since bumping setup-java to v5 or action-gh-release to v3 would be an unrequested upgrade riding along in a security change. - Signing material is scrubbed as soon as publishing completes, before the release action runs: gradle.properties is shredded and the GPG keyring removed. Pinning bounds which third-party code runs; scrubbing bounds what any later step can reach at all. Runs under `if: always()` so a failed publish does not leave the key on disk for the rest of the job. - manual-release.yml now refuses to publish from a ref that is not an ancestor of origin/main. It is a workflow_dispatch job that accepts any ref, creates a version tag from it and publishes — so without this gate anyone able to dispatch it could ship arbitrary code to Maven Central under this project's coordinates without it ever appearing on main. All five workflow files re-parse as valid YAML. Not addressed here, and recorded in TODO.md rather than folded in: the auto-merge workflows approve and merge without a test gate, and auto-dependabot.yml runs fetch-metadata and then ignores its outputs, so major-version bumps merge unreviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reclaim on demand instead of denying, and fix origin rate-limit keying Two DoS-sensitive structures were keyed entirely on attacker-chosen values and failed closed in the worst possible way. InMemoryRateLimiter denied any key not already resident once maxEntries was reached, and the key is a credential identifier taken straight from an unauthenticated body. A flood of one-shot identifiers therefore denied every legitimate caller whose bucket was not resident. InMemoryPendingSessionStore threw at capacity, and authStart stores an entry for every request including the manufactured-KE2 path that exists so an unknown identifier is indistinguishable from a known one — so unfinished handshakes 503'd everyone. Both now reclaim before refusing: stale buckets and expired sessions are dropped on demand rather than waiting for a background reaper. Refusing when genuinely full is kept — admitting on overflow would let an attacker bypass the limit outright, and evicting live entries would let them cancel other users' handshakes. Scope, stated plainly: this converts a persistent outage into a self-healing one. It does NOT stop a live adversary. An attacker who touches each of 50,000 keys once every four minutes keeps every entry inside the five-minute stale window, so the reclaim finds nothing and the outage holds at roughly the same 167 req/s as before. Properly bounding the key space — prefix aggregation and a structure that cannot be filled — is recorded in TODO.md as the real fix. Also fixes a genuine keying bug, and it is worth being direct that the first version of this change introduced half of it. The origin-keyed limiter read the request from a @Context-injected FIELD on a singleton JAX-RS resource, which silently yields null: every caller resolved to the constant "unknown", so the "per-origin" limiter was one global bucket that any single client could drain at 2 req/s to deny all six OPAQUE endpoints for the whole deployment. That is strictly worse than no limiter. The request is now threaded in as a method parameter, which is how OprfResource.evaluate already took its ContainerRequestContext. The same null-field bug was already present in OprfResource, where it made the pre-existing OPRF limiter global: in the default untrusted configuration every client shared one bucket, capping the whole deployment's OPRF oracle at 30 req/min. Fixed the same way. All four call sites now route through a single ClientIpResolver rather than three near-copies that had already drifted — OprfResource omitted the blank remote-address guard. The resolver ignores X-Forwarded-For unless the operator declares a trusted proxy, and takes the right-most entry when it does, since appending proxies put attacker-supplied values to the left. The origin limiter itself now defaults to DISABLED rather than 120/min. As a blanket default it failed both ways: one login draws two tokens, so 120/min is 60 logins/min for an entire origin, which a corporate NAT or mobile CGNAT would hit immediately — while an attacker sidesteps it with 84 addresses, or one IPv6 /64, since the key has no prefix aggregation. It is a useful control for a deployment that knows its client-address distribution and a liability switched on blindly, so it is opt-in with the reasoning recorded on the config method. Tests: the origin-keying regression is now pinned by passing two different peers and asserting two different buckets — no existing test used more than one origin, which is exactly why the null-field bug shipped. Added the full ClientIpResolver forgeability matrix. Rewrote the pending-store test, which passed with the fix removed: its one-second TTL let the background reaper fire during its own sleep, so it never exercised the on-demand path; it now backdates entries under a long TTL and fails when evictExpired() is removed. The rate limiter now warns at construction when a config takes longer to refill than the stale window, since eviction recreates a bucket full and would otherwise hand back tokens the refill rate would not have granted — an invariant every shipped config satisfies but nothing enforced. Full Java suite 838 tests, 0 failures, integration suite included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop the Spring security chain competing with the host application's Two defects in the Spring integration, both of which the repo's own tests were structurally unable to detect. HofmannSecurityConfig published an unconditional SecurityFilterChain matching every URL, which globally disabled CSRF, forced STATELESS sessions, replaced the CORS source and redefined anyRequest() as "must present a Hofmann JWT". A consumer with a chain of their own was incompatible with it: on Spring Security 6.2+ two any-request chains fail fast with UnreachableFilterChainException and the application refuses to boot. (An earlier note in this series described that as failing silently — that was wrong on this stack, and the javadoc and USAGE.md have been corrected. The case that did pass silently was a *scoped* consumer chain, where Hofmann's covered the remainder.) Separately, the autoconfiguration registered only HofmannAutoConfiguration, which carried no @import, so the controllers, the security config and the health indicator loaded only if the consumer's component scan happened to reach com.codeheadsystems.hofmann.springboot. USAGE.md said autoconfiguration "activates automatically". HofmannTestApplication sits in exactly that package, so every existing test found them regardless. The chain is now @ConditionalOnMissingBean(SecurityFilterChain.class). It stays unscoped deliberately: scoping it to /opaque/** and /oprf/** would stop the JWT filter authenticating the consumer's own endpoints, which is what the library is for. Instead it steps aside entirely when the application has a chain, and JwtAuthenticationFilter remains a bean so they can wire it in. Taking over is all-or-nothing, and that is now documented rather than implied: the condition triggers on the presence of a chain, not on what it matches, so a consumer chain scoped with securityMatcher("/api/**") displaces this one and leaves every URL outside that matcher with no chain at all. That is a fail-open gap, and it is easy to reach by accident because the previous startup crash recommended securityMatcher as the remedy. Also fixed, all found while verifying the above: - The ERROR dispatch was rewritten to 401, so every 400, 429 and 503 the controllers raise reached the client as "unauthorized" — a throttled client would re-prompt for a password instead of backing off, silently defeating the 429s added for the registration and origin limiters. Now permitted, but bound to the configured error path: permitting the ERROR dispatch to any path would let an application that maps a custom error page at a protected controller serve that controller's body to unauthenticated callers, reachable by sending a malformed request to any permitted endpoint. - corsConfigurationSource was not conditional, and @import made it universally present for the first time. A consumer following the documented instruction to override it could not start (BeanDefinitionOverrideException), and one configuring CORS the ordinary MVC way had its policy silently overridden even when this chain had correctly backed off, because Spring Security prefers a bean of that exact name. Renamed to hofmannCorsConfigurationSource, made conditional on that name, and injected by name — by type is ambiguous, since mvcHandlerMappingIntrospector also implements CorsConfigurationSource. - The chain order was 100, which places a catch-all early; Spring's convention is that an any-request chain is published last. Now LOWEST_PRECEDENCE - 5, matching Spring Boot's own default. Tests use WebApplicationContextRunner with no component scanning at all, because the existing suite physically cannot detect this class of bug. Added wire-level assertions that 400 responses arrive as 400: the ERROR permit could previously be deleted with the entire build staying green. Replaced an @order test that asserted a constant was non-zero — it passed with the annotation removed, so it tested nothing. Full Java suite 847 tests, 0 failures, integration suite included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record P0/P1 completion and the follow-ups verification surfaced Marks every P0 and P1 item done with its commit hash, keeping the analysis rather than deleting the entries, since each records a reproduction. Adds four items that came out of verifying the fixes rather than from the original review. Two are scope limits on fixes that landed and are recorded so the partial coverage is not mistaken for completeness: the rate-limiter flood is only half-closed, and recoveryVerify still amplifies across origins. Two are deliberate deferrals from the client KSF work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add release notes for 3.1.0 and bump the version Versions the release 3.1.0 rather than 3.0.1: it carries breaking changes, so a patch version would be wrong under semver. Bumps all three artifacts, which this project keeps in lockstep — the Rust crate has no source changes this cycle and moves only to stay aligned, as it did for 3.0.0. The CHANGELOG entry leads with what an adopter needs to decide from: which findings are exploitable and by whom, what breaks, and what is deliberately still open. The known-limitations section is there on purpose — the review this release came from began by disproving a TODO list that read as complete, and a changelog that implies the flood is fixed would repeat that mistake. MIGRATION.md gains a note under Argon2id parameter selection, since that is where an operator chooses the values the new client floor checks. Its existing recommendations already sit at or above the floor, so the guidance stands; what was missing was that going below it is now a local decision rather than something a server can ask for. Version-upgrade guidance lives in the CHANGELOG rather than MIGRATION.md, which is scoped to adopting OPAQUE from traditional password auth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #82, closing the last CI gap:
cargo testhas never run in CI.Analyze (rust)is CodeQL, not a test job — which is exactly how thep2560.14 bump in #71 broke the build on main undetected.The job
ubuntu-latestships a stable Rust toolchain with cargo, rustfmt and clippy already installed, so the job uses it directly and prints the versions it got.~/.cargo/registry,~/.cargo/gitandhofmann-rust/target, keyed onCargo.lock.--lockedon both cargo commands so a manifest edit committed without its matching lockfile update fails the build instead of silently resolving something else. That is the shape of the Bump p256 from 0.13.2 to 0.14.0 in /hofmann-rust in the dev-dependencies group #71 breakage.Clippy warnings cleared
-D warningsrequired fixing the three warnings the crate already carried. All are in test code and all were machine-applicable fixes fromcargo clippy --fix:src/elliptic_curve/expand_message_xmd.rsrepeat().take()→repeat_n()tests/opaque_roundtrip.rs(×2).expect(&format!(..))→.unwrap_or_else(|_| panic!(..))No production code changed.
Verified locally
All three commands run with the exact flags in the job, against
rustc1.96.0:cargo fmt --all --check— exit 0cargo clippy --all-targets --locked -- -D warnings— exit 0cargo test --locked— exit 0, 82 tests passingOne tradeoff worth knowing
-D warningsmeans a future toolchain bump can turn newly-added clippy lints into a red build on an unrelated PR. That is the price of a lint gate that can actually fail; the alternative is a clippy step that only ever prints. If it becomes annoying, arust-toolchain.tomlwould pin the version — say the word and I will add one.🤖 Generated with Claude Code