papegaaij opened a new pull request, #1536: URL: https://github.com/apache/wicket/pull/1536
Wicket had three overlapping crypt abstractions — `org.apache.wicket.util.crypt.ICrypt` (URL encryption, "remember me"), `org.apache.wicket.pageStore.crypt.ICrypter` (page store), and the `ICryptFactory` hierarchy — with different guarantees. Notably the page store default, `DefaultCrypter`, was AES-256-CBC: unauthenticated, so stored bytes were confidential but malleable, and tampered ciphertext still reached the deserializer. This replaces all of it with one authenticated abstraction. > **Base branch:** this targets `document-security-model` (#1535), not `master` — the first commit edits `SECURITY.md`, which that branch introduces. It will retarget to `master` automatically once the base merges. ### What changes **One interface.** `org.apache.wicket.core.util.crypt.ICrypt` is now the single abstraction, used by the page store, `CryptoMapper` and the authentication cookie alike. It operates on `byte[]` with an optional associated-data parameter, and provides the URL-safe Base64 `String` layer as default methods. `decrypt` returns `null` on *any* failure — unknown scheme, failed authentication, malformed input — so callers uniformly treat undecryptable data as absent rather than catching exceptions. **Authenticated by default, and self-describing.** `SchemeCrypt` writes `marker(1) || scheme-payload`, where the marker is the id of the `ICryptScheme` that produced the payload. Encryption always uses the configured scheme; decryption looks the marker up in a whitelist and refuses anything not on it. The marker is authenticated as associated data, so it cannot be altered to force a weaker scheme — together that gives downgrade protection, and a documented migration path (whitelist the old scheme while the new one becomes the encryption scheme, then drop it once data is rewritten). **Two shipped schemes**, both AEAD: - `AesGcmCryptScheme` — JDK-native AES-256-GCM. The new default; no extra dependencies. - `AesGcmSivCryptScheme` — AES-256-GCM-SIV, nonce-misuse resistant. Requires Bouncy Castle. **Scheme and key source are separated.** An `ICryptScheme` decides *how* data is encrypted and generates the keys it consumes; an `ICryptFactory` decides only *where* the key lives — `KeyInSessionCryptFactory` (per session, the default) or `ApplicationKeyCryptFactory` (application-wide, for stateless deployments). **Encrypted pages are bound to their page id.** `CryptingPageStore` passes the page id as associated data, so a stored page cannot be replayed as a different one. ### Behavioural fix outside the crypt package `AbstractFileUploadResource` validated its signed upload settings by re-encrypting the expected values and comparing the resulting *ciphertext*. That only worked because the previous default was deterministic PBE. `SchemeCrypt` uses a fresh nonce per message, as an AEAD mode must, so the comparison could never succeed and every `FileUploadToResourceField` upload would have been rejected. It now decrypts the token and compares the settings it carries; the authentication tag is what makes the token unforgeable, and `FileUploadToResourceFieldSecurityTest` still rejects tampered limits. ### Compatibility — this is a Wicket 11 API break Removed with no replacement: `SunJceCrypt`, `AbstractCrypt`, `TrivialCrypt`, `CryptFactoryCachingDecorator`, `AESCrypt`, `AbstractJceCrypt`, `KeyInSessionSunJceCryptFactory`, `AbstractKeyInSessionCryptFactory`, `DefaultCrypter`, `GCMSIVCrypter`, `ICrypter`. `NoCrypt`, `NoCryptFactory` and `ICryptFactory` move to `org.apache.wicket.core.util.crypt`. OpenRewrite recipes for the moves and notes on the removals are in `wicket-migration`. Existing ciphertext does not decrypt under the new schemes. In practice that means encrypted URLs and page store entries from a previous version are treated as expired, and "remember me" cookies are invalidated. ### Testing - Full `wicket-core-tests` suite: **2331 tests, 0 failures**. - Full reactor `clean test-compile` passes. - New coverage: `SchemeCryptTest` (19), `CryptFactoryTest` (3), plus extended `CryptoMapperTest` and `CryptingPageStoreTest` — round-trips, associated-data mismatch, whitelist refusal, marker tampering. **Known red:** `japicmp` will fail on the removals above and needs a Wicket 11 baseline before CI is green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
