diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b4f8c0..f755e67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,61 @@ tags. ## [Unreleased] +## [1.3.0] + +Security-review follow-ups (hardening; no known exploit in the items below). + +### Added + +- **Refresh tokens carry the original `amr` through rotation.** + `RefreshTokenService.issue(UserHandle, String, Optional, List)` + records the RFC 8176 authentication method references on the refresh + family, and every rotation propagates them verbatim, so an access token + minted from `POST /auth/refresh` reflects how the session was first + established (e.g. `["pkauth","webauthn"]`) instead of a generic + `["user"]`. `RefreshTokenRecord` and `RotatedClaims` gain an `amr` + field; persisted as the `amr` column (JDBI, Flyway **V10**) and the + `amr` attribute (DynamoDB). The previous three-arg + `issue(UserHandle, String, Optional)` is **deprecated** (defaults `amr` + to `["user"]`, preserving prior behavior). +- `ChallengeStoreScenarios` in `pk-auth-testkit`: a shared SPI compliance + suite asserting `ChallengeStore.takeOnce` is single-use, including a + concurrent "exactly one winner" race test, now driven against the + in-memory, JDBI/Postgres, and DynamoDB backends. + +### Changed + +- **`ChallengeId` is now an opaque random handle, decoupled from the + challenge bytes.** Previously the store key was + `base64url(challenge)`; it is now a random UUID + (`ChallengeId.random()`). Finish-time validation still enforces the + cryptographic binding by byte-comparing the stored challenge against + the bytes the authenticator signed (and WebAuthn4J re-checks the same + challenge), so the store key no longer reveals or depends on the + challenge. No wire-format change; the browser SDK already round-trips + the id. `ChallengeGenerator.idOf(byte[])` and the internal + `ChallengeValidation.IdMismatch` variant are removed. +- **`PkAuthJwtValidator` (stateful mode) rejects `jti`-less tokens.** When + a non-noop `AccessTokenStore` is bound, a validly-signed token with no + `jti` now returns `MissingClaim("jti")` instead of bypassing the + revocation gate. (Carried over from 1.2.0's fix; the validator now + detects stateful mode up front.) +- **DynamoDB refresh rotation uses a conditional `UpdateItem`.** + `rotateAtomically` now marks the parent used via a partial conditional + update (`ignoreNullsMode(SCALAR_ONLY)`) instead of a full-item PUT from + a prior read — it no longer reads the parent first and can't clobber a + concurrently-written parent attribute. Behavior is unchanged; the + freshness condition (incl. the numeric-`ttl` expiry compare from 1.2.0) + is preserved. + +### Documentation + +- Documented that the stateless access-token TTL is the effective + revocation window (`JwtConfig`), that `AccessTokenStore.exists` must + fail closed on outage, that custom `RevocationCheck` deny-lists must + handle a `null` jti, and that the Spring JWT filter is additive (never + clears a pre-existing `SecurityContext`). + ## [1.2.0] ### Security @@ -145,7 +200,8 @@ tags. First stable release. Captures the surface produced by the 0.x development series; see `git log` for the full history. -[Unreleased]: https://github.com/codeheadsystems/pk-auth/compare/v1.2.0...HEAD +[Unreleased]: https://github.com/codeheadsystems/pk-auth/compare/v1.3.0...HEAD +[1.3.0]: https://github.com/codeheadsystems/pk-auth/compare/v1.2.0...v1.3.0 [1.2.0]: https://github.com/codeheadsystems/pk-auth/compare/v1.1.0...v1.2.0 [1.1.0]: https://github.com/codeheadsystems/pk-auth/compare/v1.0.0...v1.1.0 [1.0.0]: https://github.com/codeheadsystems/pk-auth/releases/tag/v1.0.0 diff --git a/gradle.properties b/gradle.properties index 6076d1a..b759cf4 100644 --- a/gradle.properties +++ b/gradle.properties @@ -21,4 +21,4 @@ org.gradle.jvmargs=-Xmx2g \ # Project identity group=com.codeheadsystems -version=1.2.0-SNAPSHOT +version=1.3.0-SNAPSHOT diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeGenerator.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeGenerator.java index 0317cb2..3233d4f 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeGenerator.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeGenerator.java @@ -1,14 +1,13 @@ // SPDX-License-Identifier: MIT package com.codeheadsystems.pkauth.internal; -import com.codeheadsystems.pkauth.api.ChallengeId; -import com.codeheadsystems.pkauth.json.Base64Url; import java.security.SecureRandom; /** - * Produces challenge bytes and the matching {@link ChallengeId}. The id is the base64url-encoded - * challenge so the server can rebuild it from the client's {@code clientDataJSON} at finish time - * (the client also sends it explicitly as a cross-check). + * Produces WebAuthn challenge bytes. The challenge's store handle is a separate, random {@code + * ChallengeId} (see {@link com.codeheadsystems.pkauth.api.ChallengeId#random()}) — the id is + * deliberately independent of the challenge bytes, so the store key never exposes the challenge and + * the finish-time binding rests solely on the byte comparison in {@link ChallengeValidator}. * *

32 random bytes matches the WebAuthn level 3 recommendation of "at least 16 bytes" with * comfortable headroom. @@ -34,16 +33,4 @@ public byte[] generate() { random.nextBytes(challenge); return challenge; } - - /** - * Derives a {@link ChallengeId} from challenge bytes (base64url-encodes them as the id value). - * - *

Future work: {@code ChallengeId} could wrap a random UUID instead of the challenge bytes, - * but that requires a {@code ChallengeStore.takeByBytes(byte[])} so finish-time lookup can still - * find the record by the bytes carried in {@code clientDataJSON.challenge}, plus schema / index - * changes in the JDBI and DynamoDB impls. - */ - public static ChallengeId idOf(byte[] challenge) { - return new ChallengeId(Base64Url.encode(challenge)); - } } diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidation.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidation.java index 9eac388..1f3126d 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidation.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidation.java @@ -56,12 +56,6 @@ record InvalidEncoding(String detail) implements ChallengeValidation { } } - /** - * The challenge id derived from {@code clientData.challenge} did not match the explicit - * challengeId field on the request. This catches a tampered or swapped client payload. - */ - record IdMismatch() implements ChallengeValidation {} - /** No record exists for the challenge id — unknown, expired, or already consumed. */ record MissingOrConsumed() implements ChallengeValidation {} @@ -97,8 +91,6 @@ interface Mapper { R invalidEncoding(String detail); - R idMismatch(); - R missingOrConsumed(); R purposeMismatch(); @@ -122,7 +114,6 @@ static R toResult(ChallengeValidation v, Mapper mapper) { case CeremonyTypeMismatch t -> mapper.ceremonyTypeMismatch(t.expected(), t.actual()); case OriginMismatch o -> mapper.originMismatch(o.actual()); case InvalidEncoding e -> mapper.invalidEncoding(e.detail()); - case IdMismatch ignored -> mapper.idMismatch(); case MissingOrConsumed ignored -> mapper.missingOrConsumed(); case PurposeMismatch ignored -> mapper.purposeMismatch(); case BytesMismatch ignored -> mapper.bytesMismatch(); diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidator.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidator.java index 333ecb9..1b2f408 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidator.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidator.java @@ -23,10 +23,11 @@ *

  • parses the client-supplied {@code clientDataJSON}; *
  • checks the ceremony marker (registration vs authentication); *
  • checks the origin against {@link OriginValidator}; - *
  • derives the challenge id and checks it matches the explicit field on the request; *
  • consumes the {@link ChallengeRecord} via {@link ChallengeStore#takeOnce} (one-shot — even - * on later failure the record is gone); - *
  • checks the stored record's purpose, byte equality, and expiry. + * on later failure the record is gone), keyed by the opaque {@link ChallengeId} the client + * round-trips from the start response; + *
  • checks the stored record's purpose, byte equality (the cryptographic binding between the + * stored challenge and the bytes the authenticator signed), and expiry. * * *

    The caller maps the returned {@link ChallengeValidation} variant to the result hierarchy that @@ -139,17 +140,17 @@ public ChallengeValidation validate( } byte[] challengeBytes; - ChallengeId derivedId; try { challengeBytes = clientData.challengeBytes(); - derivedId = ChallengeGenerator.idOf(challengeBytes); } catch (RuntimeException ex) { return new ChallengeValidation.InvalidEncoding("invalid challenge encoding"); } - if (!derivedId.equals(challengeId)) { - return new ChallengeValidation.IdMismatch(); - } + // The challengeId is an opaque, server-generated handle (random UUID) independent of the + // challenge bytes. Look the record up by it, then enforce the cryptographic binding below by + // byte-comparing the stored challenge against the bytes the authenticator actually signed over + // (clientData.challenge) — WebAuthn4J independently re-checks the same challenge during + // signature verification. Optional stored = challengeStore.takeOnce(challengeId); if (stored.isEmpty()) { return new ChallengeValidation.MissingOrConsumed(); diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java index 3266675..a2f463b 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java @@ -222,7 +222,7 @@ public StartRegistrationResponse startRegistration( UserHandle userHandle = userLookup.getOrCreateHandle(req.username()); byte[] challenge = challengeGenerator.generate(); - ChallengeId challengeId = ChallengeGenerator.idOf(challenge); + ChallengeId challengeId = ChallengeId.random(); challengeStore.put( challengeId, @@ -458,7 +458,7 @@ public StartAuthenticationResponse startAuthentication( } byte[] challenge = challengeGenerator.generate(); - ChallengeId challengeId = ChallengeGenerator.idOf(challenge); + ChallengeId challengeId = ChallengeId.random(); challengeStore.put( challengeId, @@ -673,12 +673,6 @@ public RegistrationResult invalidEncoding(String detail) { return new RegistrationResult.InvalidPayload(detail); } - @Override - public RegistrationResult idMismatch() { - return new RegistrationResult.InvalidChallenge( - "challengeId / clientData.challenge mismatch"); - } - @Override public RegistrationResult missingOrConsumed() { return new RegistrationResult.InvalidChallenge( @@ -731,12 +725,6 @@ public AssertionResult invalidEncoding(String detail) { return new AssertionResult.InvalidChallenge(detail); } - @Override - public AssertionResult idMismatch() { - return new AssertionResult.InvalidChallenge( - "challengeId / clientData.challenge mismatch"); - } - @Override public AssertionResult missingOrConsumed() { return new AssertionResult.InvalidChallenge( diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeGeneratorTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeGeneratorTest.java index 63b8f12..a150464 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeGeneratorTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeGeneratorTest.java @@ -3,8 +3,6 @@ import static org.assertj.core.api.Assertions.assertThat; -import com.codeheadsystems.pkauth.api.ChallengeId; -import com.codeheadsystems.pkauth.json.Base64Url; import java.security.SecureRandom; import org.junit.jupiter.api.Test; @@ -34,11 +32,4 @@ public void nextBytes(byte[] bytes) { assertThat(challenge[2]).isEqualTo((byte) 9); assertThat(challenge[3]).isEqualTo((byte) 7); } - - @Test - void idOfDerivesBase64UrlOfChallenge() { - byte[] challenge = new byte[] {1, 2, 3}; - ChallengeId id = ChallengeGenerator.idOf(challenge); - assertThat(id.value()).isEqualTo(Base64Url.encode(challenge)); - } } diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeValidatorTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeValidatorTest.java index 1102c94..43351c4 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeValidatorTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/ChallengeValidatorTest.java @@ -101,12 +101,23 @@ void disallowedOriginReturnsOriginMismatch() { } @Test - void challengeIdMismatchReturnsIdMismatch() { - byte[] otherChallenge = filled(32, (byte) 7); - byte[] cd = clientData("webauthn.get", Base64Url.encode(otherChallenge), "https://example.com"); + void opaqueChallengeIdIndependentOfBytesStillValidates() { + // The challengeId is now an opaque handle unrelated to the challenge bytes. A record stored + // under a random UUID id, looked up by that same id, with matching bytes, must still validate — + // proving the id is no longer derived from (or coupled to) the challenge bytes. + ChallengeId opaqueId = new ChallengeId("11111111-2222-3333-4444-555555555555"); + when(challengeStore.takeOnce(opaqueId)) + .thenReturn( + Optional.of( + new ChallengeRecord( + CHALLENGE, + ChallengeRecord.Purpose.AUTHENTICATION, + USER_HANDLE, + NOW.plusSeconds(60)))); + byte[] cd = clientData("webauthn.get", Base64Url.encode(CHALLENGE), "https://example.com"); ChallengeValidation out = - validator.validate(ChallengeValidator.Ceremony.AUTHENTICATION, CHALLENGE_ID, cd); - assertThat(out).isInstanceOf(ChallengeValidation.IdMismatch.class); + validator.validate(ChallengeValidator.Ceremony.AUTHENTICATION, opaqueId, cd); + assertThat(out).isInstanceOf(ChallengeValidation.Valid.class); } @Test @@ -129,9 +140,9 @@ void crossPurposeChallengeReturnsPurposeMismatch() { @Test void mismatchedStoredBytesReturnsBytesMismatch() { - // store has different bytes than what client returned. Trick: ChallengeId is derived from - // bytes, so to bypass IdMismatch we have to store a record under the same id but with - // different bytes — which only happens if the store hands back tampered data. Emulate that. + // The record stored under CHALLENGE_ID carries different bytes than the client returned in + // clientData.challenge. The finish-time byte comparison is the cryptographic binding, so this + // must be rejected as BytesMismatch. when(challengeStore.takeOnce(CHALLENGE_ID)) .thenReturn( Optional.of( diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceTest.java index 90db910..54655b8 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceTest.java @@ -152,7 +152,11 @@ void startRegistrationPersistsChallengeAndReturnsEnvelope() { StartRegistrationResponse resp = service.startRegistration(new StartRegistrationRequest("alice", "Alice", null, null)); - assertThat(resp.challengeId()).isEqualTo(CHALLENGE_ID); + // The challengeId is now an opaque random handle, independent of the challenge bytes. Assert it + // is well-formed and that the SAME id is what gets persisted (the binding to the bytes is + // enforced at finish time, not via the id). + assertThat(resp.challengeId()).isNotNull(); + assertThat(resp.challengeId().value()).isNotBlank(); assertThat(resp.publicKey().rp().id()).isEqualTo("example.com"); assertThat(resp.publicKey().user().name()).isEqualTo("alice"); assertThat(resp.publicKey().user().displayName()).isEqualTo("Alice"); @@ -163,7 +167,7 @@ void startRegistrationPersistsChallengeAndReturnsEnvelope() { assertThat(resp.publicKey().excludeCredentials()).isNotNull().isEmpty(); verify(challengeStore) - .put(eq(CHALLENGE_ID), any(ChallengeRecord.class), eq(Duration.ofMinutes(5))); + .put(eq(resp.challengeId()), any(ChallengeRecord.class), eq(Duration.ofMinutes(5))); verify(metrics).incrementCounter("pkauth.registration.start", "rp", "example.com"); } @@ -260,10 +264,19 @@ void finishRegistrationRejectsBadOrigin() { } @Test - void finishRegistrationRejectsChallengeIdMismatch() { - byte[] otherChallenge = filled(32, (byte) 7); - byte[] cd = - clientData("webauthn.create", Base64Url.encode(otherChallenge), "https://example.com"); + void finishRegistrationRejectsChallengeBytesMismatch() { + // The record stored under the opaque CHALLENGE_ID carries different challenge bytes than the + // client returned in clientData.challenge. The finish-time byte comparison is the cryptographic + // binding (the challengeId itself is just an opaque handle), so this must be rejected. + when(challengeStore.takeOnce(CHALLENGE_ID)) + .thenReturn( + Optional.of( + new ChallengeRecord( + filled(32, (byte) 7), + ChallengeRecord.Purpose.REGISTRATION, + USER_HANDLE, + NOW.plusSeconds(60)))); + byte[] cd = clientData("webauthn.create", Base64Url.encode(CHALLENGE), "https://example.com"); RegistrationResult result = service.finishRegistration(finishReg(cd)); assertThat(result).isInstanceOf(RegistrationResult.InvalidChallenge.class); } diff --git a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/AccessTokenStore.java b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/AccessTokenStore.java index d5bcc0b..055484b 100644 --- a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/AccessTokenStore.java +++ b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/AccessTokenStore.java @@ -64,6 +64,13 @@ void record( * *

    The {@link #noop()} implementation returns {@code true} unconditionally so stateless * deployments behave as if no store were involved. + * + *

    Fail closed on outage. If the backing store is unreachable, throw rather + * than returning {@code true}: a thrown exception propagates out of {@link + * PkAuthJwtValidator#validate(String)} so the host's filter rejects the request (no token + * accepted), whereas returning {@code true} would fail open and accept a + * possibly-revoked token during the outage. Host integrations must therefore treat a thrown + * {@code validate(...)} as an authentication failure, not swallow it into success. */ boolean exists(String jti); diff --git a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtConfig.java b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtConfig.java index 4720ed4..170bd55 100644 --- a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtConfig.java +++ b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtConfig.java @@ -14,6 +14,16 @@ * validator. Additional accepted audiences come from {@link TokenTtlPolicy#knownAudiences()} — see * {@link #allowedAudiences()} for the resolved set. * + *

    Access-token TTL is your revocation window in stateless mode. With the + * default {@link AccessTokenStore#noop() no-op access-token store}, pk-auth issues stateless JWTs + * that cannot be invalidated before their {@code exp} — a logout or user-disable only stops + * new tokens, while already-issued access tokens stay valid until they expire. The access + * TTL ({@link #DEFAULT_TOKEN_TTL} = 1 hour by default, or per-audience via {@link TokenTtlPolicy}) + * is therefore the worst-case window an attacker keeps access after credentials are pulled; keep it + * short (minutes-to-an-hour) and pair it with rotating refresh tokens for long sessions. Hosts that + * need immediate revocation must bind a real {@link AccessTokenStore} (stateful mode); see + * ADR 0015. + * * @param issuer the {@code iss} claim value * @param defaultAudience the audience used when {@link JwtClaims#audience()} is absent at issue * time; always part of {@link #allowedAudiences()} diff --git a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/RevocationCheck.java b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/RevocationCheck.java index c967172..76161e9 100644 --- a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/RevocationCheck.java +++ b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/RevocationCheck.java @@ -17,6 +17,15 @@ public interface RevocationCheck { * Return {@code true} if the token (identified by its {@code jti} / {@code sub}) has been revoked * and must be rejected. * + *

    Handle a {@code null} jti. A token may carry no {@code jti}, in which case + * {@code jti} is {@code null} here. An implementation that maintains a deny-list keyed solely on + * {@code jti} must decide explicitly what to do with a jti-less token — typically reject it (the + * deny-list can never prove it un-revoked) or fall back to a {@code sub}-scoped rule. A naive + * {@code denyList.contains(jti)} silently lets every jti-less token through. (In stateful mode, + * {@link PkAuthJwtValidator} already rejects jti-less tokens before this check via the {@link + * AccessTokenStore}; this guidance matters for stateless deployments that rely only on a {@code + * RevocationCheck}.) + * * @param jti the JWT ID claim ({@code jti}), may be {@code null} if absent from the token * @param subject the subject claim ({@code sub}), always non-null when called from the validator * @return {@code true} to reject the token; {@code false} to allow it diff --git a/pk-auth-micronaut/src/test/java/com/codeheadsystems/pkauth/micronaut/PkAuthRefreshControllerTest.java b/pk-auth-micronaut/src/test/java/com/codeheadsystems/pkauth/micronaut/PkAuthRefreshControllerTest.java index 4fc6fa6..ebe9112 100644 --- a/pk-auth-micronaut/src/test/java/com/codeheadsystems/pkauth/micronaut/PkAuthRefreshControllerTest.java +++ b/pk-auth-micronaut/src/test/java/com/codeheadsystems/pkauth/micronaut/PkAuthRefreshControllerTest.java @@ -42,7 +42,10 @@ class PkAuthRefreshControllerTest { void rotateMintsValidAccessJwtAndNewRefreshToken() { RefreshTokenPair root = refreshService.issue( - UserHandle.of(new byte[] {1}), "https://app.example.com", Optional.empty()); + UserHandle.of(new byte[] {1}), + "https://app.example.com", + Optional.empty(), + java.util.List.of("pkauth", "webauthn")); RefreshResponse response = client @@ -61,7 +64,10 @@ void rotateMintsValidAccessJwtAndNewRefreshToken() { void replayReturns401() { RefreshTokenPair root = refreshService.issue( - UserHandle.of(new byte[] {2}), "https://app.example.com", Optional.empty()); + UserHandle.of(new byte[] {2}), + "https://app.example.com", + Optional.empty(), + java.util.List.of("pkauth", "webauthn")); // First rotation succeeds. client diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java index f281cfe..4d55199 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java @@ -22,9 +22,11 @@ import software.amazon.awssdk.enhanced.dynamodb.Key; import software.amazon.awssdk.enhanced.dynamodb.TableSchema; import software.amazon.awssdk.enhanced.dynamodb.model.ConditionCheck; +import software.amazon.awssdk.enhanced.dynamodb.model.IgnoreNullsMode; import software.amazon.awssdk.enhanced.dynamodb.model.PutItemEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.QueryConditional; import software.amazon.awssdk.enhanced.dynamodb.model.TransactPutItemEnhancedRequest; +import software.amazon.awssdk.enhanced.dynamodb.model.TransactUpdateItemEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.TransactWriteItemsEnhancedRequest; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; @@ -98,26 +100,33 @@ public boolean rotateAtomically( return wrap( "refresh_tokens.rotateAtomically", () -> { - // We can't read userHandleB64u from the predicate, so we need to look up the parent - // first to know which user-index item to update. The lookup + transact path is - // safe because the conditional on the parent UpdateItem inside the transaction is - // the authoritative freshness check — even if state changed between lookup and - // transaction, the conditional fails and we return false. - RefreshTokenItem parent = - table.getItem( - Key.builder() - .partitionValue(PRIMARY_PK_PREFIX + parentRefreshId) - .sortValue(PRIMARY_PK_PREFIX + parentRefreshId) - .build()); - if (parent == null) { - return false; - } + // Mark the parent's primary item used via a conditional UpdateItem that touches ONLY + // used_at (ignoreNulls) — not a read-modify-write of the whole item — so a concurrent + // writer to any other parent attribute can never be clobbered, and no prior read is + // needed. The successor's user-index partition is derived from the successor's own + // userHandle, not the parent's, so the parent never has to be loaded. + // + // The condition is the authoritative freshness check: the parent must exist + // (attribute_exists(pk) — UpdateItem would otherwise upsert a brand-new row) and be + // unused, unrevoked, and unexpired. Expiry compares the numeric epoch-second `ttl` + // attribute (== expiresAt.epochSecond), NOT the ISO string: Instant.toString() is + // variable-precision (it drops the fractional-seconds field when zero), so a + // lexicographic ">" sorts "...:00Z" after "...:00.000001Z" and would treat an expired + // token as still fresh. + RefreshTokenItem parentMark = new RefreshTokenItem(); + parentMark.setPk(PRIMARY_PK_PREFIX + parentRefreshId); + parentMark.setSk(PRIMARY_PK_PREFIX + parentRefreshId); + parentMark.setUsedAtIso(now.toString()); - String nowIso = now.toString(); - // Mark the parent's primary item used. The user-index and family-index items aren't - // authoritative for used_at — only the primary is — so we don't mutate them here. - RefreshTokenItem updatedParent = copy(parent); - updatedParent.setUsedAtIso(nowIso); + Expression freshness = + Expression.builder() + .expression( + "attribute_exists(pk) AND attribute_not_exists(usedAtIso)" + + " AND attribute_not_exists(revokedAtIso) AND #ttl > :nowEpoch") + .putExpressionName("#ttl", "ttl") + .putExpressionValue( + ":nowEpoch", AttributeValue.fromN(Long.toString(now.getEpochSecond()))) + .build(); // Build the three successor items. RefreshTokenItem successorPrimary = @@ -136,29 +145,14 @@ public boolean rotateAtomically( FAMILY_PK_PREFIX + successor.familyId(), INDEX_SK_PREFIX + successor.refreshId()); - // Transact: conditional update on parent primary (still fresh) + put successor items. - // ConditionExpression — fresh iff used_at, revoked_at unset and expires_at > now. - // Expiry compares the numeric epoch-second `ttl` attribute (== expiresAt.epochSecond), - // NOT the ISO string: Instant.toString() is variable-precision (it drops the - // fractional-seconds field when zero), so a lexicographic ">" sorts "...:00Z" after - // "...:00.000001Z" and would treat an expired token as still fresh. - Expression freshness = - Expression.builder() - .expression( - "attribute_not_exists(usedAtIso) AND attribute_not_exists(revokedAtIso)" - + " AND #ttl > :nowEpoch") - .putExpressionName("#ttl", "ttl") - .putExpressionValue( - ":nowEpoch", AttributeValue.fromN(Long.toString(now.getEpochSecond()))) - .build(); - try { enhanced.transactWriteItems( TransactWriteItemsEnhancedRequest.builder() - .addPutItem( + .addUpdateItem( table, - TransactPutItemEnhancedRequest.builder(RefreshTokenItem.class) - .item(updatedParent) + TransactUpdateItemEnhancedRequest.builder(RefreshTokenItem.class) + .item(parentMark) + .ignoreNullsMode(IgnoreNullsMode.SCALAR_ONLY) .conditionExpression(freshness) .build()) .addPutItem( @@ -438,30 +432,11 @@ private static RefreshTokenItem toItem(RefreshTokenRecord r, String pk, String s item.setUsedAtIso(r.usedAt().map(Instant::toString).orElse(null)); item.setRevokedAtIso(r.revokedAt().map(Instant::toString).orElse(null)); item.setRevokedReason(r.revokedReason().map(Enum::name).orElse(null)); + item.setAmr(String.join(",", r.amr())); item.setTtl(r.expiresAt().getEpochSecond()); return item; } - private static RefreshTokenItem copy(RefreshTokenItem from) { - RefreshTokenItem item = new RefreshTokenItem(); - item.setPk(from.getPk()); - item.setSk(from.getSk()); - item.setRefreshId(from.getRefreshId()); - item.setTokenHashB64u(from.getTokenHashB64u()); - item.setUserHandleB64u(from.getUserHandleB64u()); - item.setAudience(from.getAudience()); - item.setDeviceId(from.getDeviceId()); - item.setFamilyId(from.getFamilyId()); - item.setParentRefreshId(from.getParentRefreshId()); - item.setIssuedAtIso(from.getIssuedAtIso()); - item.setExpiresAtIso(from.getExpiresAtIso()); - item.setUsedAtIso(from.getUsedAtIso()); - item.setRevokedAtIso(from.getRevokedAtIso()); - item.setRevokedReason(from.getRevokedReason()); - item.setTtl(from.getTtl()); - return item; - } - private static RefreshTokenRecord toRecord(RefreshTokenItem item) { Map ignored = new HashMap<>(); byte[] hash = Base64Url.decode(item.getTokenHashB64u()); @@ -477,7 +452,19 @@ private static RefreshTokenRecord toRecord(RefreshTokenItem item) { Instant.parse(item.getExpiresAtIso()), Optional.ofNullable(item.getUsedAtIso()).map(Instant::parse), Optional.ofNullable(item.getRevokedAtIso()).map(Instant::parse), - Optional.ofNullable(item.getRevokedReason()).map(RevokeReason::valueOf)); + Optional.ofNullable(item.getRevokedReason()).map(RevokeReason::valueOf), + splitAmr(item.getAmr())); + } + + /** + * Parses the stored comma-separated {@code amr} attribute back into a list. A null/blank value + * (items written before the attribute existed) maps to the generic {@code ["user"]}. + */ + private static List splitAmr(String stored) { + if (stored == null || stored.isBlank()) { + return List.of("user"); + } + return List.of(stored.split(",")); } private static T wrap(String op, Supplier body) { diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/RefreshTokenItem.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/RefreshTokenItem.java index bbe5bd9..b3e06c5 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/RefreshTokenItem.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/RefreshTokenItem.java @@ -41,6 +41,7 @@ public class RefreshTokenItem { private String usedAtIso; private String revokedAtIso; private String revokedReason; + private String amr; private Long ttl; public RefreshTokenItem() {} @@ -159,6 +160,15 @@ public void setRevokedReason(String revokedReason) { this.revokedReason = revokedReason; } + /** RFC 8176 {@code amr} method references, stored comma-separated (e.g. "pkauth,webauthn"). */ + public String getAmr() { + return amr; + } + + public void setAmr(String amr) { + this.amr = amr; + } + public Long getTtl() { return ttl; } diff --git a/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbCeremonyIntegrationTest.java b/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbCeremonyIntegrationTest.java index 12ed615..63ea762 100644 --- a/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbCeremonyIntegrationTest.java +++ b/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbCeremonyIntegrationTest.java @@ -102,4 +102,10 @@ void challengeTakeOnceIsSingleUse() { assertThat(challengeStore.takeOnce(id)).isPresent(); assertThat(challengeStore.takeOnce(id)).isEmpty(); } + + @Test + void challengeStoreSingleUseUnderConcurrency() throws Exception { + new com.codeheadsystems.pkauth.testkit.ChallengeStoreScenarios(challengeStore) + .concurrentTakeOnceYieldsExactlyOneWinner(); + } } diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRefreshTokenRepository.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRefreshTokenRepository.java index 3b13bb1..46e6d7a 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRefreshTokenRepository.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRefreshTokenRepository.java @@ -23,9 +23,10 @@ import org.jdbi.v3.core.statement.Update; /** - * {@link RefreshTokenRepository} backed by the {@code refresh_tokens} table (Flyway V9). The - * load-bearing {@link #rotateAtomically} uses a JDBI transaction that wraps a conditional {@code - * UPDATE} on the parent + an {@code INSERT} for the successor — see ADR 0013. + * {@link RefreshTokenRepository} backed by the {@code refresh_tokens} table (Flyway V9; the {@code + * amr} column added in V10). The load-bearing {@link #rotateAtomically} uses a JDBI transaction + * that wraps a conditional {@code UPDATE} on the parent + an {@code INSERT} for the successor — see + * ADR 0013. * * @since 1.1.0 */ @@ -180,9 +181,9 @@ private static void insert(Handle h, RefreshTokenRecord r) { "INSERT INTO refresh_tokens" + " (refresh_id, token_hash, user_handle, audience, device_id, family_id," + " parent_refresh_id, issued_at, expires_at, used_at, revoked_at," - + " revoked_reason)" + + " revoked_reason, amr)" + " VALUES (:rid, :hash, :uh, :aud, :did, :fam, :pid, :iat, :exp, :uat, :rat," - + " :reason)") + + " :reason, :amr)") .bind("rid", r.refreshId()) .bind("hash", r.tokenHash()) .bind("uh", r.userHandle().value()) @@ -192,7 +193,8 @@ private static void insert(Handle h, RefreshTokenRecord r) { .bind("pid", r.parentRefreshId().orElse(null)) .bind("iat", OffsetDateTime.ofInstant(r.issuedAt(), ZoneOffset.UTC)) .bind("exp", OffsetDateTime.ofInstant(r.expiresAt(), ZoneOffset.UTC)) - .bind("reason", r.revokedReason().map(Enum::name).orElse(null)); + .bind("reason", r.revokedReason().map(Enum::name).orElse(null)) + .bind("amr", joinAmr(r.amr())); // used_at and revoked_at are TIMESTAMPTZ; JDBI's untyped-null default (Types.VARCHAR) is // rejected by Postgres against a TIMESTAMPTZ column. Force Types.TIMESTAMP_WITH_TIMEZONE on // the null branch. @@ -247,6 +249,23 @@ private static RefreshTokenRecord readRow(ResultSet rs) throws SQLException { rs.getObject("expires_at", OffsetDateTime.class).toInstant(), Optional.ofNullable(usedAt).map(OffsetDateTime::toInstant), Optional.ofNullable(revokedAt).map(OffsetDateTime::toInstant), - Optional.ofNullable(revokedReasonStr).map(RevokeReason::valueOf)); + Optional.ofNullable(revokedReasonStr).map(RevokeReason::valueOf), + splitAmr(rs.getString("amr"))); + } + + /** Serializes the RFC 8176 {@code amr} references as a comma-separated string for storage. */ + private static String joinAmr(List amr) { + return String.join(",", amr); + } + + /** + * Parses the stored comma-separated {@code amr} string back into a list. A null/blank value (rows + * written before the V10 column existed) maps to the generic {@code ["user"]}. + */ + private static List splitAmr(String stored) { + if (stored == null || stored.isBlank()) { + return List.of("user"); + } + return List.of(stored.split(",")); } } diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java index 288088c..4c75200 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java @@ -22,7 +22,7 @@ public final class PkAuthJdbiSchema { * #migrateForDevelopment(DataSource)} pins Flyway's {@code target} to this value so that * unreleased migrations on the classpath are never applied accidentally. */ - public static final String CURRENT_SCHEMA_VERSION = "9"; + public static final String CURRENT_SCHEMA_VERSION = "10"; private PkAuthJdbiSchema() {} diff --git a/pk-auth-persistence-jdbi/src/main/resources/db/migration/V10__refresh_tokens_amr.sql b/pk-auth-persistence-jdbi/src/main/resources/db/migration/V10__refresh_tokens_amr.sql new file mode 100644 index 0000000..71cbf58 --- /dev/null +++ b/pk-auth-persistence-jdbi/src/main/resources/db/migration/V10__refresh_tokens_amr.sql @@ -0,0 +1,13 @@ +-- SPDX-License-Identifier: MIT +-- +-- 1.3.0: carry the original authentication method references (RFC 8176 "amr") through refresh +-- rotation, so an access token minted from a refresh reflects how the session was first +-- established (e.g. "pkauth,webauthn") instead of a generic marker. +-- +-- Stored as a comma-separated string of method tokens (amr entries are simple RFC 8176 references +-- and never contain a comma; see RefreshTokenRecord). Rows created before this column existed +-- predate the feature and are defaulted to the generic 'user' marker, matching the legacy behavior +-- of RefreshHandler. + +ALTER TABLE refresh_tokens + ADD COLUMN amr VARCHAR(255) NOT NULL DEFAULT 'user'; diff --git a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCeremonyIntegrationTest.java b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCeremonyIntegrationTest.java index 5a06746..fd317b5 100644 --- a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCeremonyIntegrationTest.java +++ b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCeremonyIntegrationTest.java @@ -91,4 +91,10 @@ void challengeStoreTakeOnceConsumesAtomically() { assertThat(challengeStore.takeOnce(new com.codeheadsystems.pkauth.api.ChallengeId("ch-1"))) .isEmpty(); } + + @Test + void challengeStoreSingleUseUnderConcurrency() throws Exception { + new com.codeheadsystems.pkauth.testkit.ChallengeStoreScenarios(challengeStore) + .concurrentTakeOnceYieldsExactlyOneWinner(); + } } diff --git a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiNullableBindingIntegrationTest.java b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiNullableBindingIntegrationTest.java index d2c1fc2..ced137b 100644 --- a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiNullableBindingIntegrationTest.java +++ b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiNullableBindingIntegrationTest.java @@ -90,7 +90,8 @@ void refreshTokenCreatedWithNullUsedAtAndNullRevokedAt() { now.plusSeconds(3600), Optional.empty(), Optional.empty(), - Optional.empty()); + Optional.empty(), + java.util.List.of("user")); refreshTokens.create(token); diff --git a/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenRecord.java b/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenRecord.java index a9f0aaf..2c64277 100644 --- a/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenRecord.java +++ b/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenRecord.java @@ -4,6 +4,7 @@ import com.codeheadsystems.pkauth.api.UserHandle; import java.time.Instant; import java.util.Arrays; +import java.util.List; import java.util.Objects; import java.util.Optional; @@ -32,6 +33,11 @@ * @param revokedAt set when this token (or its family) is revoked * @param revokedReason categorical reason matching {@link #revokedAt}; present iff {@code * revokedAt} is set + * @param amr RFC 8176 authentication method references from the original authentication, carried + * verbatim through every rotation so refreshed access tokens reflect how the session was first + * established (e.g. {@code ["pkauth", "webauthn"]}). Must be non-empty; entries are simple + * method tokens and must not contain a comma (the persistence layer joins them + * comma-separated). Added in 1.3.0. * @since 1.1.0 */ public record RefreshTokenRecord( @@ -46,7 +52,8 @@ public record RefreshTokenRecord( Instant expiresAt, Optional usedAt, Optional revokedAt, - Optional revokedReason) { + Optional revokedReason, + List amr) { public RefreshTokenRecord { Objects.requireNonNull(refreshId, "refreshId"); @@ -77,6 +84,20 @@ public record RefreshTokenRecord( throw new IllegalArgumentException( "revokedAt and revokedReason must both be set or both absent"); } + Objects.requireNonNull(amr, "amr"); + if (amr.isEmpty()) { + throw new IllegalArgumentException("amr must be non-empty"); + } + for (String m : amr) { + Objects.requireNonNull(m, "amr entry"); + if (m.isBlank()) { + throw new IllegalArgumentException("amr entries must be non-blank"); + } + if (m.indexOf(',') >= 0) { + throw new IllegalArgumentException("amr entries must not contain ','"); + } + } + amr = List.copyOf(amr); tokenHash = tokenHash.clone(); } @@ -100,7 +121,8 @@ public boolean equals(Object o) { && Objects.equals(expiresAt, r.expiresAt) && Objects.equals(usedAt, r.usedAt) && Objects.equals(revokedAt, r.revokedAt) - && Objects.equals(revokedReason, r.revokedReason); + && Objects.equals(revokedReason, r.revokedReason) + && Objects.equals(amr, r.amr); } @Override @@ -117,6 +139,7 @@ public int hashCode() { expiresAt, usedAt, revokedAt, - revokedReason); + revokedReason, + amr); } } diff --git a/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenService.java b/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenService.java index fe4227a..8930aca 100644 --- a/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenService.java +++ b/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/RefreshTokenService.java @@ -32,6 +32,9 @@ public final class RefreshTokenService { private static final Logger LOG = LoggerFactory.getLogger(RefreshTokenService.class); + /** Default {@code amr} applied when a caller issues a refresh token without specifying one. */ + private static final List DEFAULT_AMR = List.of("user"); + private final RefreshTokenRepository repository; private final RefreshTokenConfig config; private final ClockProvider clockProvider; @@ -58,13 +61,33 @@ public RefreshTokenService( } /** - * Issues a fresh refresh token belonging to a new family (root of the rotation chain). Returns - * the wire token plus the persisted record summary. + * Issues a fresh refresh token belonging to a new family (root of the rotation chain), defaulting + * the carried {@code amr} to {@code ["user"]}. + * + * @deprecated since 1.3.0 — prefer {@link #issue(UserHandle, String, Optional, List)} so the + * refresh family records the original authentication method references (RFC 8176) and + * refreshed access tokens reflect how the session was first established instead of a generic + * {@code ["user"]}. Retained for source compatibility. */ + @Deprecated public RefreshTokenPair issue(UserHandle userHandle, String audience, Optional deviceId) { + return issue(userHandle, audience, deviceId, DEFAULT_AMR); + } + + /** + * Issues a fresh refresh token belonging to a new family (root of the rotation chain). Returns + * the wire token plus the persisted record summary. The supplied {@code amr} — RFC 8176 + * authentication method references describing how the user just authenticated — is stored on the + * family and carried verbatim into every access token minted from a rotation of this token. + * + * @since 1.3.0 + */ + public RefreshTokenPair issue( + UserHandle userHandle, String audience, Optional deviceId, List amr) { Objects.requireNonNull(userHandle, "userHandle"); Objects.requireNonNull(audience, "audience"); Objects.requireNonNull(deviceId, "deviceId"); + Objects.requireNonNull(amr, "amr"); if (audience.isBlank()) { throw new IllegalArgumentException("audience must be non-blank"); } @@ -84,7 +107,8 @@ public RefreshTokenPair issue(UserHandle userHandle, String audience, Optional deviceId) { +public record RotatedClaims( + UserHandle userHandle, String audience, Optional deviceId, List amr) { public RotatedClaims { Objects.requireNonNull(userHandle, "userHandle"); Objects.requireNonNull(audience, "audience"); Objects.requireNonNull(deviceId, "deviceId"); + Objects.requireNonNull(amr, "amr"); + if (amr.isEmpty()) { + throw new IllegalArgumentException("amr must be non-empty"); + } + amr = List.copyOf(amr); } } diff --git a/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/web/RefreshHandler.java b/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/web/RefreshHandler.java index 7314983..4b86482 100644 --- a/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/web/RefreshHandler.java +++ b/pk-auth-refresh-tokens/src/main/java/com/codeheadsystems/pkauth/refresh/web/RefreshHandler.java @@ -5,7 +5,6 @@ import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer; import com.codeheadsystems.pkauth.refresh.RefreshTokenService; import com.codeheadsystems.pkauth.refresh.RotateResult; -import java.util.List; import java.util.Objects; /** @@ -52,7 +51,7 @@ public Outcome handle(RefreshRequest request) { JwtClaims.forRefresh( s.claimsForAccessIssue().userHandle(), s.claimsForAccessIssue().audience(), - List.of("user")); + s.claimsForAccessIssue().amr()); String accessJwt = accessIssuer.issue(claims); yield new Outcome.Success( new RefreshResponse(s.pair().wireToken(), accessJwt, s.pair().record().expiresAt())); diff --git a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/security/PkAuthJwtAuthenticationFilter.java b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/security/PkAuthJwtAuthenticationFilter.java index f178b44..2adf08f 100644 --- a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/security/PkAuthJwtAuthenticationFilter.java +++ b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/security/PkAuthJwtAuthenticationFilter.java @@ -29,6 +29,14 @@ * add a cookie fallback here; host apps that want session-cookie auth should layer their own filter * ahead of this one and accept the CSRF responsibilities that come with it. * + *

    Additive, not authoritative-clearing. The filter only ever sets the + * authentication on a successful token; it never clears a pre-existing one on an invalid + * or absent token. Under the shipped stateless chain ({@code SessionCreationPolicy.STATELESS}) the + * context starts empty every request, so this is moot. But a host that layers a + * session-establishing filter ahead of this one would let a request bearing a valid + * session cookie plus a bogus or expired bearer keep the session identity. If you combine the two, + * clear the context on a non-{@code Success} result here, or order this filter first. + * *

    Design note. This filter intentionally bypasses Spring's {@code AuthenticationManager} * for zero-overhead JWT validation. There is no companion {@code AuthenticationProvider} — host * apps that need the canonical manager pipeline can declare their own filter + provider. diff --git a/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthRefreshIntegrationTest.java b/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthRefreshIntegrationTest.java index 0d67130..9c7a145 100644 --- a/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthRefreshIntegrationTest.java +++ b/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthRefreshIntegrationTest.java @@ -51,7 +51,10 @@ void setUp() { void rotateMintsValidAccessJwtAndNewRefreshToken() throws Exception { RefreshTokenPair root = refreshService.issue( - UserHandle.of(new byte[] {1}), "pk-auth-test-clients", Optional.empty()); + UserHandle.of(new byte[] {1}), + "pk-auth-test-clients", + Optional.empty(), + java.util.List.of("pkauth", "webauthn")); MvcResult result = mockMvc @@ -80,7 +83,10 @@ void rotateMintsValidAccessJwtAndNewRefreshToken() throws Exception { void replayReturns401Replayed() throws Exception { RefreshTokenPair root = refreshService.issue( - UserHandle.of(new byte[] {2}), "pk-auth-test-clients", Optional.empty()); + UserHandle.of(new byte[] {2}), + "pk-auth-test-clients", + Optional.empty(), + java.util.List.of("pkauth", "webauthn")); // First rotation succeeds. mockMvc diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/ChallengeStoreScenarios.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/ChallengeStoreScenarios.java new file mode 100644 index 0000000..2134292 --- /dev/null +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/ChallengeStoreScenarios.java @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: MIT +package com.codeheadsystems.pkauth.testkit; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.codeheadsystems.pkauth.api.ChallengeId; +import com.codeheadsystems.pkauth.api.UserHandle; +import com.codeheadsystems.pkauth.spi.ChallengeRecord; +import com.codeheadsystems.pkauth.spi.ChallengeStore; +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Shared compliance scenarios for {@link ChallengeStore} implementations — the single-use contract + * every backend (in-memory, JDBI, DynamoDB) must honour. {@link ChallengeStore#takeOnce} is the + * load-bearing primitive behind WebAuthn replay defense: a challenge must be consumable exactly + * once, even under concurrent finish requests, or an assertion could be replayed. + * + *

    Drive these from each backend's test class (passing a fresh, empty store) so the atomic + * read-and-remove contract is verified identically everywhere, not just asserted single-threaded. + * + * @since 1.3.0 + */ +public final class ChallengeStoreScenarios { + + private static final UserHandle USER = UserHandle.of(new byte[] {4, 2}); + private static final byte[] CHALLENGE = new byte[32]; + + private final ChallengeStore store; + + public ChallengeStoreScenarios(ChallengeStore store) { + this.store = Objects.requireNonNull(store, "store"); + } + + /** takeOnce returns the record the first time and empty every time after. */ + public void takeOnceConsumesExactlyOnce() { + ChallengeId id = new ChallengeId("tck-single-use"); + store.put(id, record(), Duration.ofMinutes(5)); + assertThat(store.takeOnce(id)).as("first take wins").isPresent(); + assertThat(store.takeOnce(id)).as("second take is consumed").isEmpty(); + } + + /** + * The non-negotiable atomicity test: N threads call {@link ChallengeStore#takeOnce} on the same + * id simultaneously and exactly one must receive the record. A non-atomic read-then-delete would + * let two threads both observe and "consume" the same challenge, enabling assertion replay. + */ + public void concurrentTakeOnceYieldsExactlyOneWinner() throws Exception { + ChallengeId id = new ChallengeId("tck-concurrent-take-once"); + store.put(id, record(), Duration.ofMinutes(5)); + + int threads = 8; + CountDownLatch ready = new CountDownLatch(threads); + CountDownLatch fire = new CountDownLatch(1); + ExecutorService pool = Executors.newFixedThreadPool(threads); + AtomicInteger winners = new AtomicInteger(); + try { + List> futures = new java.util.ArrayList<>(); + for (int i = 0; i < threads; i++) { + futures.add( + pool.submit( + () -> { + ready.countDown(); + try { + fire.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } + Optional taken = store.takeOnce(id); + if (taken.isPresent()) { + winners.incrementAndGet(); + } + })); + } + assertThat(ready.await(5, TimeUnit.SECONDS)).isTrue(); + fire.countDown(); + for (Future f : futures) { + f.get(10, TimeUnit.SECONDS); + } + assertThat(winners.get()).as("exactly one thread consumes the challenge").isEqualTo(1); + assertThat(store.takeOnce(id)).as("nothing left after the race").isEmpty(); + } finally { + pool.shutdownNow(); + } + } + + private static ChallengeRecord record() { + return new ChallengeRecord( + CHALLENGE, + ChallengeRecord.Purpose.AUTHENTICATION, + USER, + Instant.now().plus(Duration.ofMinutes(5))); + } +} diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java index 53e2247..4bc7284 100644 --- a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java @@ -152,7 +152,8 @@ private static RefreshTokenRecord markUsed(RefreshTokenRecord r, Instant now) { r.expiresAt(), Optional.of(now), r.revokedAt(), - r.revokedReason()); + r.revokedReason(), + r.amr()); } private static RefreshTokenRecord markRevoked( @@ -169,6 +170,7 @@ private static RefreshTokenRecord markRevoked( r.expiresAt(), r.usedAt(), Optional.of(now), - Optional.of(reason)); + Optional.of(reason), + r.amr()); } } diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/RefreshTokenScenarios.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/RefreshTokenScenarios.java index ec2f418..fa5baee 100644 --- a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/RefreshTokenScenarios.java +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/RefreshTokenScenarios.java @@ -43,6 +43,7 @@ public final class RefreshTokenScenarios { private static final Instant NOW = Instant.parse("2026-05-16T12:00:00Z"); private static final UserHandle USER = UserHandle.of(new byte[] {1, 2, 3}); private static final String AUDIENCE = "web"; + private static final List AMR = List.of("pkauth", "webauthn"); private final RefreshTokenRepository repository; private final RefreshTokenService service; @@ -68,7 +69,7 @@ public RefreshTokenScenarios(RefreshTokenRepository repository, ClockProvider cl /** Issue → rotate → revoke happy path. */ public void issueRotateRevokeHappyPath() { - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); assertThat(root.wireToken()).contains("."); assertThat(root.record().userHandle()).isEqualTo(USER); assertThat(root.record().familyId()).isEqualTo(root.record().refreshId()); @@ -80,6 +81,11 @@ public void issueRotateRevokeHappyPath() { assertThat(success.pair().record().parentRefreshId()).hasValue(root.record().refreshId()); assertThat(success.claimsForAccessIssue().userHandle()).isEqualTo(USER); assertThat(success.claimsForAccessIssue().audience()).isEqualTo(AUDIENCE); + // amr is persisted on the family and carried verbatim through rotation (survives the backend + // round-trip), so a refreshed access token reflects the original authentication method. + assertThat(root.record().amr()).isEqualTo(AMR); + assertThat(success.pair().record().amr()).isEqualTo(AMR); + assertThat(success.claimsForAccessIssue().amr()).isEqualTo(AMR); // Revoke the family — subsequent rotates of the successor return Revoked. service.revokeFamily(root.record().familyId(), RevokeReason.LOGOUT); @@ -90,7 +96,7 @@ public void issueRotateRevokeHappyPath() { /** Successor's parent-link points back at the parent's refreshId. */ public void rotationUpdatesFamilyChainAndChildLinksToParent() { - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); RotateResult.Success rotated = (RotateResult.Success) service.rotate(root.wireToken()); List familyIds = new ArrayList<>(); @@ -103,7 +109,7 @@ public void rotationUpdatesFamilyChainAndChildLinksToParent() { /** Presenting a used token triggers a family scorch and returns Replayed. */ public void replayOfUsedTokenScorchesEntireFamily() { - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); RotateResult.Success first = (RotateResult.Success) service.rotate(root.wireToken()); // Re-present the root (already used) — replay defense. RotateResult replay = service.rotate(root.wireToken()); @@ -123,7 +129,7 @@ public void replayOfUsedTokenScorchesEntireFamily() { /** Past-due tokens return Expired without any family revocation. */ public void expiredTokenRotationReturnsExpired() { // Issue at NOW, then build a service whose clock is 60 days later (default TTL is 14d). - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); RefreshTokenService laterService = new RefreshTokenService( repository, @@ -148,7 +154,7 @@ public void unknownRefreshIdReturnsUnknown() { * mark the legitimate token used. This is the hash-before-mark-used invariant from ADR 0013. */ public void wrongSecretReturnsUnknownAndDoesNotBurnLegitToken() { - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); String forged = root.record().refreshId() + ".wrongSecretBase64Url"; assertThat(service.rotate(forged)).isInstanceOf(RotateResult.Unknown.class); // Legitimate rotation still works after the failed presentation. @@ -157,7 +163,7 @@ public void wrongSecretReturnsUnknownAndDoesNotBurnLegitToken() { /** Calling revokeFamily twice is a no-op the second time. */ public void revokeFamilyIsIdempotent() { - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); service.revokeFamily(root.record().familyId(), RevokeReason.LOGOUT); service.revokeFamily(root.record().familyId(), RevokeReason.LOGOUT); var family = repository.findByFamilyId(root.record().familyId()); @@ -169,9 +175,9 @@ public void revokeFamilyIsIdempotent() { public void revokeAllForUserRevokesEveryActiveFamily() { UserHandle alice = UserHandle.of(new byte[] {1}); UserHandle bob = UserHandle.of(new byte[] {2}); - service.issue(alice, AUDIENCE, Optional.empty()); - service.issue(alice, "cli", Optional.empty()); - service.issue(bob, AUDIENCE, Optional.empty()); + service.issue(alice, AUDIENCE, Optional.empty(), AMR); + service.issue(alice, "cli", Optional.empty(), AMR); + service.issue(bob, AUDIENCE, Optional.empty(), AMR); int revoked = service.revokeAllForUser(alice, RevokeReason.USER_DELETED); assertThat(revoked).isEqualTo(2); @@ -188,7 +194,7 @@ public void revokeAllForUserRevokesEveryActiveFamily() { * on motif's {@code concurrent_sameSecret_exactlyOneSucceeds_familyRevoked}. */ public void concurrentRotationExactlyOneSucceedsFamilyRevoked() throws Exception { - RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty()); + RefreshTokenPair root = service.issue(USER, AUDIENCE, Optional.empty(), AMR); int threads = 8; CountDownLatch ready = new CountDownLatch(threads); diff --git a/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryChallengeStoreTest.java b/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryChallengeStoreTest.java new file mode 100644 index 0000000..b850972 --- /dev/null +++ b/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryChallengeStoreTest.java @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +package com.codeheadsystems.pkauth.testkit; + +import org.junit.jupiter.api.Test; + +/** Drives the shared {@link ChallengeStoreScenarios} against the in-memory reference store. */ +class InMemoryChallengeStoreTest { + + @Test + void takeOnceConsumesExactlyOnce() { + new ChallengeStoreScenarios(new InMemoryChallengeStore()).takeOnceConsumesExactlyOnce(); + } + + @Test + void concurrentTakeOnceYieldsExactlyOneWinner() throws Exception { + new ChallengeStoreScenarios(new InMemoryChallengeStore()) + .concurrentTakeOnceYieldsExactlyOneWinner(); + } +}