Security-review follow-ups: amr propagation, ChallengeId decoupling, hardening

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<String>)`
+  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

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

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}.
  *
  * <p>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).
-   *
-   * <p>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));
-  }
 }

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> {
 
     R invalidEncoding(String detail);
 
-    R idMismatch();
-
     R missingOrConsumed();
 
     R purposeMismatch();
@@ -122,7 +114,6 @@ static <R> R toResult(ChallengeValidation v, Mapper<R> 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();

pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/ChallengeValidator.java

@@ -23,10 +23,11 @@
  *   <li>parses the client-supplied {@code clientDataJSON};
  *   <li>checks the ceremony marker (registration vs authentication);
  *   <li>checks the origin against {@link OriginValidator};
- *   <li>derives the challenge id and checks it matches the explicit field on the request;
  *   <li>consumes the {@link ChallengeRecord} via {@link ChallengeStore#takeOnce} (one-shot — even
- *       on later failure the record is gone);
- *   <li>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;
+ *   <li>checks the stored record's purpose, byte equality (the cryptographic binding between the
+ *       stored challenge and the bytes the authenticator signed), and expiry.
  * </ol>
  *
  * <p>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<ChallengeRecord> stored = challengeStore.takeOnce(challengeId);
     if (stored.isEmpty()) {
       return new ChallengeValidation.MissingOrConsumed();

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(

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));
-  }
 }

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(

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);
   }

pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/AccessTokenStore.java

@@ -64,6 +64,13 @@ void record(
    *
    * <p>The {@link #noop()} implementation returns {@code true} unconditionally so stateless
    * deployments behave as if no store were involved.
+   *
+   * <p><strong>Fail closed on outage.</strong> 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 <em>open</em> 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);
 

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.
  *
+ * <p><strong>Access-token TTL is your revocation window in stateless mode.</strong> 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
+ * <em>new</em> 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 <em>immediate</em> 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()}

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.
    *
+   * <p><strong>Handle a {@code null} jti.</strong> 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