Security-review follow-ups: amr propagation, ChallengeId decoupling, hardening#26
Merged
Conversation
…DB rotate Refreshed access tokens previously carried a hardcoded amr=["user"], losing the original authentication method. RefreshTokenService now records the RFC 8176 amr on the refresh family and propagates it verbatim through every rotation, so a token minted from POST /auth/refresh reflects how the session was first established. - RefreshTokenRecord + RotatedClaims gain an `amr` field; new issue(UserHandle, String, Optional, List<String>) overload. The 3-arg issue(...) is deprecated and defaults amr to ["user"] (prior behavior). - Persisted as the `amr` column (JDBI, Flyway V10) and `amr` attribute (DynamoDB); rows/items predating the change default to ["user"]. - RefreshTokenScenarios asserts amr survives the rotation round-trip on all three backends. Also (B1) DynamoDbRefreshTokenRepository.rotateAtomically now marks the parent used via a conditional UpdateItem (ignoreNullsMode SCALAR_ONLY) instead of a full-item PUT from a prior read: no pre-read, and it can't clobber a concurrently-written parent attribute. The freshness condition (incl. the numeric-ttl expiry compare) is preserved; behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The challenge store key was base64url(challenge) — the id literally was the challenge bytes. It is now an opaque random handle (ChallengeId.random()), generated at startRegistration/startAuthentication and round-tripped by the client unchanged. Security is unchanged: finish-time validation still enforces the cryptographic binding by byte-comparing the stored challenge against the bytes the authenticator signed over (clientData.challenge), and WebAuthn4J independently re-checks the same challenge during signature verification. The random id no longer reveals or depends on the challenge. Defense-in-depth, no known exploit. Removes ChallengeGenerator.idOf(byte[]) and the now-meaningless ChallengeValidation.IdMismatch variant (a tampered payload now surfaces as BytesMismatch or MissingOrConsumed). No wire-format or SPI change; the browser SDK already round-trips the id rather than deriving it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChallengeStore.takeOnce is the load-bearing primitive behind WebAuthn replay defense, but single-use was only asserted single-threaded per backend. Add a shared ChallengeStoreScenarios TCK with a concurrent "exactly one of N threads wins" race test, and drive it against the in-memory, JDBI/Postgres, and DynamoDB backends so a non-atomic read-then-delete in any implementation would fail the suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the security contracts surfaced by the review (no behavior change): - JwtConfig: the access-token TTL is the effective revocation window in stateless mode (no-op AccessTokenStore) — keep it short and adopt the AccessTokenStore SPI for immediate revocation. - AccessTokenStore.exists: must fail closed (throw) on store outage; returning true would fail open and accept possibly-revoked tokens. - RevocationCheck.isRevoked: custom deny-lists must handle a null jti, or a jti-less token slips a jti-only deny-list. - Spring PkAuthJwtAuthenticationFilter: it is additive — it never clears a pre-existing SecurityContext, which matters only if a host layers a session filter ahead of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bump the development version to 1.3.0-SNAPSHOT (additive SPI methods => minor) and add the [1.3.0] CHANGELOG section covering the security-review follow-ups. Co-Authored-By: Claude Opus 4.8 (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-ups from the security review (the remaining items after the 1.2.0 fixes). All hardening — no known exploit in any item below. Targets 1.3.0.
What's in here
A1 — Propagate the original
amrthrough refresh rotationPOST /auth/refreshminted access tokens with a hardcodedamr=["user"], erasing how the session was first established (a problem for any policy that keys offamrfor step-up).RefreshTokenServicenow records the RFC 8176amron the refresh family and carries it verbatim through every rotation.issue(UserHandle, String, Optional, List<String>); 3-argissue(...)deprecated (defaults to["user"], preserving prior behavior).RefreshTokenRecord/RotatedClaimsgainamr; persisted as theamrcolumn (JDBI Flyway V10) andamrattribute (DynamoDB). Pre-existing rows/items default to["user"].RefreshTokenScenariosassertsamrsurvives the rotation round-trip on all three backends.A2 — Decouple
ChallengeIdfrom the challenge bytesThe challenge store key was
base64url(challenge). It's now an opaque random UUID handle. Finish-time validation still enforces the binding by byte-comparing the stored challenge against the bytes the authenticator signed (and WebAuthn4J re-checks it), so the key no longer reveals or depends on the secret. No wire/SPI change — the browser SDK already round-trips the id. RemovesChallengeGenerator.idOfand the internalIdMismatchvariant.B1 — DynamoDB refresh rotate uses a conditional
UpdateItemrotateAtomicallymarked the parent used via a full-item PUT from a prior read; it now uses a conditionalUpdateItem(ignoreNullsMode(SCALAR_ONLY)) — no pre-read, and it can't clobber a concurrently-written parent attribute. Freshness condition (incl. the 1.2.0 numeric-ttlexpiry compare) preserved; behavior unchanged.C — Documented security contracts (docs only)
Stateless access-TTL = revocation window (
JwtConfig);AccessTokenStore.existsmust fail closed on outage; customRevocationCheckmust handle a nulljti; the Spring JWT filter is additive (never clears a pre-existingSecurityContext).D1 — Atomic single-use TCK for
ChallengeStoreNew shared
ChallengeStoreScenarioswith a concurrent "exactly one of N wins" race test, driven against in-memory, JDBI/Postgres, and DynamoDB — single-use was previously asserted only single-threaded.Compatibility
RefreshTokenService.issue(...).ALTER TABLE refresh_tokens ADD COLUMN amr ... DEFAULT 'user'); DynamoDB adds an optionalamrattribute. Both default gracefully for pre-existing data.CURRENT_SCHEMA_VERSION→"10".Testing
Full
./gradlew buildpasses (all module unit + JDBI/Postgres + DynamoDB-Local integration tests, spotless, javadoc, jacoco). The refresh race test and the new challenge-store race test pass on all three backends.🤖 Generated with Claude Code