Remove jasypt 1.9.3 dependency (v41 breaking change)#568
Open
amcclain wants to merge 11 commits into
Open
Conversation
Introduces four new classes under io.xh.hoist.security to provide pure-JDK (and Spring-Security-Crypto-backed) alternatives to the jasypt 1.9.3 dependency: - HoistPasswordEncoder: user-facing BCrypt wrapper for consuming apps' User domain classes. matches() transparently verifies both new BCrypt hashes and legacy jasypt-format hashes so existing user records continue to authenticate post-upgrade. isLegacyHash() supports migrate-on-login patterns. - AesTextCipher: AES-256-GCM with PBKDF2WithHmacSHA256-derived keys. Output is Base64 wrapped with a "$hoist-aes1$" marker prefix so values are unambiguously distinguishable from legacy ciphertext. Used internally by AppConfig. - SaltedSha256Digester: per-instance random-salt SHA-256, used internally by AppConfig to produce stable opaque fingerprints of pwd-typed values for the admin UI's config-differ. - LegacyJasyptDecrypter: read-only pure-JDK reproduction of jasypt 1.9.3's default BasicTextEncryptor (PBEWithMD5AndDES, 1000 iterations, 8-byte salt prepended) and BasicPasswordEncryptor (MD5, 1000 iterations, 8-byte salt prepended) algorithms. Enables in-place decode of values written by hoist-core <= v40 without keeping jasypt on the classpath. No callers are wired up to these yet — that arrives in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the org.jasypt:jasypt:1.9.3 dependency entirely. Jasypt has been end-of-life since 2014 and breaks at runtime on JDK 21+ under Spring Boot's launcher classloader: BasicTextEncryptor.encrypt and BasicPasswordEncryptor.encryptPassword both throw "Could not perform a valid UNICODE normalization" from a reflective call inside jasypt's Normalizer wrapper. Standard --add-opens flags do not resolve this — the failure is in jasypt's reflective code path, not JDK module accessibility — so the library is effectively unusable on modern JDKs. build.gradle: - Remove "api org.jasypt:jasypt:1.9.3" - Add "api org.springframework.security:spring-security-crypto" (version managed by Spring Boot BOM; only the crypto module — no filters/controllers) AppConfig.groovy: - Static encryptor now an AesTextCipher (AES-256-GCM, PBKDF2-derived key) - Static digestEncryptor now a SaltedSha256Digester (per-process random salt, stable within instance) - New static legacyDecrypter (LegacyJasyptDecrypter) handles read-side fallback for values written by hoist-core <= v40 - decryptPassword now dispatches by the AesTextCipher.FORMAT_PREFIX marker — new ciphertext routes to the AES cipher, legacy ciphertext routes to the legacy decrypter. Re-saving any pwd config promotes it to the new format; no DB migration required. - Hardcoded encryption password preserved verbatim (it is in the open-source source already, so not a secret) — it now seeds both the new PBKDF2 derivation and the legacy decrypter, allowing the marker prefix to safely distinguish formats. Sourcing this key from instance config remains a future enhancement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First unit tests in hoist-core. Adds src/test/groovy/ with five Spock specs covering the v41 crypto rework:
- AesTextCipherSpec: round-trip across unicode/large/empty inputs, fresh-output-per-call, format marker detection, null-input rejection, wrong-password failure, cross-instance compatibility.
- SaltedSha256DigesterSpec: stable-within-instance, varies across instances, base64 output shape.
- LegacyJasyptDecrypterSpec: decrypts four BasicTextEncryptor fixtures and matches six BasicPasswordEncryptor fixtures — all captured out-of-band from jasypt 1.9.3 itself, anchoring the migration shim's correctness against real legacy data shapes.
- AppConfigEncryptionRoutingSpec: exercises the same marker-prefix dispatch logic AppConfig uses, confirming a mixed-format DB reads correctly regardless of write era.
- HoistPasswordEncoderSpec: BCrypt encode/match round trip, unique-hash-per-call, transparent legacy verification, isLegacyHash detection, malformed-input handling.
build.gradle gains testImplementation "org.spockframework:spock-core:2.3-groovy-4.0" plus tasks.named('test') { useJUnitPlatform() }.
All 45 tests pass under ./gradlew test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CHANGELOG (41.0-SNAPSHOT): breaking-change section calling out the jasypt removal with pointer to the upgrade notes, plus technical entries detailing the AesTextCipher / SaltedSha256Digester / HoistPasswordEncoder / LegacyJasyptDecrypter additions, the spring-security-crypto dependency, and the first src/test/groovy/ tree in the repo. docs/upgrade-notes/v41-upgrade-notes.md (new): step-by-step migration with explicit before/after for consuming apps' User domain class swap from BasicPasswordEncryptor to HoistPasswordEncoder, optional migrate-on-login pattern using isLegacyHash, and a "no DB migration required" section covering the AppConfig pwd-value handling. CLAUDE.md: refresh the AppConfig description and key-dependencies list to reflect the move off jasypt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related cleanups to the v41 crypto rework, both surfaced by review: * Replace `SaltedSha256Digester` with `ConfigValueDigester`: a deterministic SHA-256 of the plaintext (no salt). The previous per-process random salt broke the Admin Console's Config Diff workflow, which compares `AppConfig.formatForJSON()` payloads across two environments — identical plaintexts produced different digests in different JVMs and showed as spurious diffs on every `pwd` row. The digest is admin-only (HOIST_ADMIN_READER), same trust boundary as the plaintext read path, so dropping the salt does not change the threat model. * Rename `CONFIG_ENCRYPTION_PASSWORD` to `CONFIG_VALUE_OBFUSCATION_KEY` and add a comment block that explicitly names the threat model — this is at-rest obfuscation for low-sensitivity admin UI display, not a confidentiality boundary. Add inline `gitleaks` / `allowlist secret` suppression markers so secret scanners do not alert on a value that has been in this file's source for years and was preserved verbatim for backward compatibility.
LegacyJasyptDecrypterSpec now covers empty / single-char / 256-char / NFC / NFD / whitespace fixtures for both the text-decrypt and password-digest paths, each generated by jasypt 1.9.3 on a standalone JVM (the failure mode that motivated the jasypt removal only manifests under Spring Boot's launcher classloader). Adds an explicit NFC↔NFD cross-check confirming the password digest path NFC-normalizes input while the text path does not, mirroring jasypt 1.9.3 behavior. LegacyJasyptDecrypter's class doc now lists the exact cipher and digest parameters it implements against, so future debugging of an in-the-wild legacy value can be cross-referenced to jasypt 1.9.3 source at a glance. AesTextCipher, LegacyJasyptDecrypter, and HoistPasswordEncoder class- and method-level docs trimmed to essentials per review feedback — design rationale and migration narrative live in the v41 upgrade notes.
* Added src/test/groovy/README.md covering how to run the suite, the scope of what belongs here (pure-JDK only, no Grails context), naming conventions, and the regeneration recipe for the jasypt-1.9.3 legacy-format fixtures pinned in the new crypto specs. * Pointed CLAUDE.md at the test README and listed `./gradlew test` in the build-commands section so AI agents discover the testing surface. * Rewrote the v41 CHANGELOG entries to single-sentence bullets per docs/changelog-format.md; the previous wrapped paragraphs duplicated detail already covered by the upgrade notes. * Added a paragraph to the v41 upgrade notes naming the obfuscation-key threat model explicitly and noting that the key is unchanged from prior releases.
* v41-upgrade-notes.md: replace two stale references to the pre-rename `SaltedSha256Digester` / "salted SHA-256" with the current `ConfigValueDigester` / "deterministic SHA-256" naming. Caught during PR write-up; doc-only, no runtime impact. * AesTextCipher: class-level Groovydoc now explicitly states the intended scope (at-rest obfuscation of `pwd` AppConfig values behind a source-visible key) and warns that it is NOT a confidentiality boundary or a general-purpose secrets primitive — direct readers should reach for instance config / env vars / a real secrets manager instead. * LegacyJasyptDecrypter: class-level Groovydoc now flags that the reproduced jasypt algorithms (PBE-MD5-DES, MD5+8-byte-salt) are obsolete and that the class is a one-release migration shim, not for new use cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes a long-standing weakness in the `pwd`-typed AppConfig story. The hardcoded obfuscation key that lived in this open-source file's source was previously used to encrypt every `pwd` value at rest — adequate to mask values in the admin UI, but trivially reversible by anyone with source access. v41 fixes that without requiring a DB migration. * `AppConfig.encryptIfPwd` now requires an app-supplied key sourced from instance config (`appConfigCryptoKey` — env var `APP_<appCode>_APP_CONFIG_CRYPTO_KEY` or YAML). Without it, reads of pre-v41 ciphertexts still work via the LegacyJasyptDecrypter shim, but writes fail closed with an IllegalStateException naming the missing config. This keeps the upgrade boot-safe for apps that haven't yet wired the key, while making it impossible to write new `pwd` values under a known key. * `AppConfig.decryptPassword` dispatches by format marker as before, with a new fail-closed branch for the misconfiguration case where a `$hoist-aes1$` value exists but no active key is configured. * The old hardcoded key is renamed `LEGACY_OBFUSCATION_KEY` and now only flows into `LegacyJasyptDecrypter` — never into `AesTextCipher`. The key is a one-way migration shim, structurally analogous to the legacy jasypt algorithm code itself. Doc and class-comment changes follow the new shape: * `AesTextCipher` Groovydoc drops the "not for real secrets" disclaimer — the cipher is now an honest primitive whose strength is exactly the strength of the caller-supplied key. * `LegacyJasyptDecrypter` Groovydoc sharpens the migration-only language and explicitly notes that pre-v41 `pwd` ciphertexts must be treated as if stored in plaintext. * CHANGELOG breaking-changes section gains a second bullet for the new required setup step; difficulty rating widens to 🟡 MEDIUM for apps that write `pwd` configs. * v41-upgrade-notes.md adds a full step 4 covering key generation, instance-config wiring, the fail-closed write behavior, the in-place migration path (re-save), and a hard warning against rotating the key once `pwd` rows exist under it. * CLAUDE.md refresh to match. Tests: `AppConfigEncryptionRoutingSpec` now exercises the three-way decrypt dispatch (active cipher / legacy shim / unconfigured-key failure) plus the fail-closed write path. 60 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 4's pwd-value migration paragraph now describes what triggers re-encryption rather than prescribing a runbook the framework doesn't actually guarantee. Reflects that the upgrade-path polish here is intentionally light — pwd-typed AppConfig values aren't widely used, and the release's value is the redesigned encryption-key story, not a migration UX we don't ship. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
Member
Author
|
Downstream consumer PR (draft, blocked on this one shipping): xh/toolbox#856 — Add Playwright E2E testing setup. Demonstrates the |
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
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.
TL;DR for a busy reviewer
org.jasypt:jasypt:1.9.3dependency (EOL since 2014, no successor release). Replaces its two internal uses in hoist-core with pure-JDK code, ships a new BCrypt-based password encoder for consuming apps, and moves theAppConfigpwdencryption key out of open-source code and into operator-controlled instance config.User(orAppUser) domain class importsorg.jasypt.util.password.BasicPasswordEncryptor(the common pattern) will fail to compile until it switches toio.xh.hoist.security.HoistPasswordEncoder. Roughly 5 LoC per affected domain class.pwd-typedAppConfigvalues must now supply an encryption key via instance config (appConfigCryptoKey— env varAPP_<appCode>_APP_CONFIG_CRYPTO_KEYor YAML). Without it,pwdreads still work (legacy shim), butpwdwrites fail closed with a clear error. Apps that don't usepwdconfigs are unaffected by this branch.pwdvalues decrypt transparently via the one-releaseLegacyJasyptDecryptershim. Existing user password hashes verify transparently viaHoistPasswordEncoder.matches()— no forced password reset.Why this PR
The proximate trigger was a Toolbox bootstrap that exploded under JDK 21, with the failure surfacing inside
jasypt's reflective Unicode-normalizer wrapper. End-to-end verification (see "Verification" below) found that the surface symptom was actually caused by a Gradle build-script bug in Toolbox that fed the daemon'sjava.hometo a forked toolchain JVM — independent ofjasypt. The Toolbox fix lands separately and removes the immediate symptom.That doesn't change the case for this PR, which stands on its own merits:
jasypt-spring-boot(a community shim) still pins the same 1.9.3 artifact. Carrying an unmaintained transitiveapidep across every consuming Hoist app is a slow-burn risk; any future JDK move that breaks one of its many reflective code paths becomes our problem.pwdAppConfig encryption was effectively obfuscation, not encryption. The fixed key lived in this open-source file'sstaticblock. Anyone with read access to the repo could decrypt everypwdvalue from a DB dump. The PR replaces that with operator-supplied keys.BasicPasswordEncryptordefault. BCrypt's adaptive cost is the de-facto standard.src/test/groovy/with Spock; that's a long-term win independent of the crypto rework.What's in the change
New code (
src/main/groovy/io/xh/hoist/security/)HoistPasswordEncoder— thin BCrypt wrapper (viaspring-security-crypto).encode()writes BCrypt;matches()transparently accepts BCrypt or legacy jasypt-default hashes;isLegacyHash()lets apps opportunistically re-encode on successful login.crypto/AesTextCipher— AES-256-GCM with PBKDF2-derived key (SHA-256, 65,536 iterations), random salt + IV per encryption, output prefixed with the$hoist-aes1$format marker. Authenticated encryption — tampering is detected on decrypt. Strength = key strength; the class doc is explicit that the cipher is as good as the key the caller supplies.crypto/LegacyJasyptDecrypter— read-only, pure-JDK reproduction of jasypt 1.9.3'sBasicTextEncryptor(PBEWithMD5AndDES, 1000 iterations, 8-byte salt) andBasicPasswordEncryptor(MD5, 1000 iterations, 8-byte salt, jasypt's specific re-digest-without-resalt loop). Applies NFC normalization to match jasypt exactly. Class doc explicitly scopes this as a one-release migration shim with no security value of its own.crypto/ConfigValueDigester— deterministic SHA-256 of plaintext, used by the Admin Console's Config Diff workflow to comparepwdvalues across environments without exposing them. No salt by design (admin-only trust boundary; salt would have broken cross-environment diff).Wiring (
AppConfig)LEGACY_OBFUSCATION_KEY, but its sole consumer is nowLegacyJasyptDecrypter— a read-only path. It never reachesAesTextCipherand never encrypts new content.InstanceConfigUtils.getInstanceConfig('appConfigCryptoKey'). If unset, the cipher isnullandpwdwrites fail closed (IllegalStateExceptionnaming the missing config);pwdreads of pre-v41 values still flow through the legacy shim unaffected.decryptPassworddispatches by$hoist-aes1$format marker: present → active cipher (with the same fail-closed branch for the misconfiguration case where a$hoist-aes1$row exists but no key is configured); absent → legacy shim. Re-saving apwdconfig from the admin UI promotes the row to the new format under the app-supplied key.Build (
build.gradle)api org.jasypt:jasypt:1.9.3api org.springframework.security:spring-security-crypto(crypto module only, version managed by the Spring Boot BOM)testImplementation org.spockframework:spock-core:2.3-groovy-4.0+tasks.named('test') { useJUnitPlatform() }Tests (first
src/test/groovy/tree in the repo)60 Spock specs across 5 files —
./gradlew testgreen. Pure-JDK only (no Grails context, no DB). Notable coverage:isLegacyHashdetection, malformed-input handling.AppConfig.src/test/groovy/README.md.Docs
CHANGELOG.md— v41-SNAPSHOT breaking-change + technical entries, including the new requiredappConfigCryptoKeysetup for apps that writepwdconfigs.docs/upgrade-notes/v41-upgrade-notes.md— step-by-step migration with before/after code for theUserdomain swap, optional migrate-on-login pattern, a full step for theappConfigCryptoKeysetup with env-var / YAML wiring, fail-closed write semantics, the in-place re-save migration path, and a hard warning against rotating the key oncepwdrows exist under it.CLAUDE.md— refresh AppConfig description and key-dependencies list.Verification
End-to-end manual smoke test against this branch from Toolbox with
runHoistInline=trueon JDK 21 Temurin (a Toolbox-sidebuild.gradlefix was needed to keepbootRunfrom feeding the daemon'sjava.homeinto the forked toolchain JVM — landing separately in xh/toolbox#856)../gradlew testgreen (60/60 locally)./gradlew clean assemblegreenappConfigCryptoKeyset; admin UI reads existing legacypwdconfig; save promotes the row to$hoist-aes1$format under the app-supplied keyHO+/H4S...legacy base64) and after ($hoist-aes1$rrYO...); round-tripped readback returns the new plaintext via the new digestappConfigCryptoKeyconfigured: legacypwdreads succeed;pwdwrites raiseIllegalStateExceptionwith actionable message; row unmodifiedtest-legacy@xh.iowith the spec'splaywright-testlegacy fixture (3hrIc7ebBL/Nxnkk...); login returnedsuccess: true. Bootstrap-createdtest-admin@xh.ioandtest-user@xh.ioshow$2a$10$...BCrypt hashes in the DBSafety / release readiness
LegacyJasyptDecrypter(read-only); no new content can be written under it.$hoist-aes1$prefix uses$which cannot appear in Base64, so no legacy ciphertext can collide with the new format.length == 60 && startsWith('$2')— also disjoint from Base64-only legacy hashes.BasicPasswordEncryptorin aUserdomain class are not specifically addressed in the upgrade notes — they would also get a compile failure on the version bump. No such usage is known among XH apps but worth agrep org.jasyptpass on each consumer at upgrade time.pwdre-save (does it always sendvaluewhen the user clicks Save?) is not exercised by this PR. The expected migration path — open and save — was verified at the API level here; the UI flow should be smoke-tested before tagging the release.🤖 Generated with Claude Code