Conversation
Migrate all JJWT usage to the 0.12+ API: - Replace Jwts.parser().setSigningKey(String) with Jwts.parser().verifyWith(SecretKey).build().parseSignedClaims() - Replace Jwts.builder().signWith(algorithm, String) with Jwts.builder().signWith(SecretKey) - Replace getBody() with getPayload() - Replace Jwts.claims().setX() builder with Map-based claims - Derive SecretKey via Keys.hmacShaKeyFor(Decoders.BASE64.decode()) - Remove JwtSignatureAlgorithmFactory from AbstractJwtService and IdTokenFactory (algorithm inferred from key size) Affected modules: uPortal-soffit-core, uPortal-soffit-renderer, uPortal-security-core, uPortal-spring
bjagg
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the 0.11 → 0.12+ API migration is unavoidable work and the mechanical parts look right. Before merging I rebuilt the new code paths against real jjwt 0.13.0 jars on Java 11 to check the parts I was least sure about. Three worries turned out to be non-issues, and three things I'd like to resolve.
What I verified as safe
-
The
Map<String,Object>claims rewrite round-trips correctly. I was concerned that puttingjava.util.Datevalues into a raw map underClaims.EXPIRATION/Claims.ISSUED_ATwould skip the RFC date conversion that.expiration(...)/.issuedAt(...)apply. It doesn't —Jwts.builder().claims().add(map)runs the same converters, and the payload serializes as numeric epoch seconds:{"sub":"student","iss":"Soffit","groups":["Students"],"exp":1789681303,"iat":1789677703,"class":"...Bearer","jti":"..."}So the expiry check in
parseEncryptedTokenstill behaves. -
Default deployments see no algorithm change.
DEFAULT_SIGNATURE_KEYdecodes to 1072 bits, soKeys.hmacShaKeyForyieldsHmacSHA512— identical to the oldSIGNATURE_ALGORITHM_DEFAULT = HS512. -
Cross-version interop holds. This was my main worry, since
uPortal-soffit-coreis a published artifact and a remote soffit app may still be on 0.11.5 after the portal upgrades. I signed a token with the new 0.13 path and parsed it with 0.11.5'sJwts.parser().setSigningKey(base64Key).parseClaimsJws(...)— verifies fine, subject and custom claims intact. No forced lockstep upgrade for deployers' soffit apps.
What I'd like resolved
1. JwtSignatureAlgorithmFactory becomes dead config rather than being removed.
After this PR nothing consumes it, but the @Component and its documented property org.apereo.portal.soffit.jwt.signatureAlgorithm (@since 5.6.1, default HS512) are still there. The algorithm is now inferred from key length instead. A deployer who deliberately set HS256 and holds a >=512-bit key will silently start emitting HS512 with no warning in the logs. Interop-wise that's survivable given the 0.11.5 result above, but it will bite anyone whose downstream consumer pins an expected alg.
Either direction is fine by me, but I'd like it to be deliberate: delete the class and the property with a note in the upgrade docs, or keep honoring it via signWith(key, Jwts.SIG.HS256/HS384/HS512).
2. Key-derivation failures move from request time to boot time.
Keys.hmacShaKeyFor(Decoders.BASE64.decode(signatureKey)) now runs in @PostConstruct (AbstractJwtService, IdTokenFactory) and in the SoffitApiPreAuthenticatedProcessingFilter constructor. I confirmed both sides of this:
- jjwt 0.13.0: a sub-256-bit key throws
WeakKeyExceptionat derivation, i.e. now at context startup. - jjwt 0.11.5 on
master:signWith(SignatureAlgorithm.HS512, shortKey)throwsWeakKeyExceptiontoo — but only when a token is first created.
So a deployer with an undersized org.apereo.portal.soffit.jwt.signatureKey who uses neither soffits nor OIDC id tokens goes from a dormant misconfiguration to a portal that won't start, and the stack trace won't obviously name the property. Failing fast is defensible, but could we catch it in init() and rethrow with a message naming SIGNATURE_KEY_PROPERTY?
3. No test coverage for the rewritten paths.
This changes token creation and parsing across four modules, and there's no create -> parse round-trip test in the tree today — uPortal-soffit-core has only JwtEncryptorTest, and the green CI here isn't evidence that the new code is correct, just that nothing exercises it. The harness I wrote to check the above is basically the missing test: build claims via createClaims, sign, parse, assert subject / exp / jti / a custom collection claim survive. Could we land that against BearerService or AbstractJwtService as part of this PR? It would also lock in the interop behavior so a future jjwt bump can't quietly break it.
Happy to pair on any of these if it's easier — none of it is a rewrite, and the migration itself looks sound.
Resolves the gradle.properties conflict: keep jjwtVersion=0.13.0 from this branch, take jodaTimeVersion=2.14.3 from master (uPortal-Project#3018).
Problem: the jjwt 0.13 migration dropped both uses of JwtSignatureAlgorithmFactory but left the @component and its documented property org.apereo.portal.soffit.jwt.signatureAlgorithm in the tree, so a deployer who set HS256 would silently start emitting a different alg with nothing in the logs. Separately, jjwt 0.12+ derives the HMAC key eagerly, so an undersized signatureKey now fails at context startup instead of on first token use, and jjwt's WeakKeyException does not name the property responsible. Goal: leave the documented configuration working as documented, and make the new boot-time failure diagnosable. Changes: - JwtSignatureAlgorithmFactory returns a MacAlgorithm resolved from the Jwts.SIG registry, falling back to HS512 with a warning naming the property (previously a bare exception message) - AbstractJwtService and IdTokenFactory sign with signWith(secretKey, algorithmFactory.getAlgorithm()) again - add AbstractJwtService.deriveKey(String), which wraps WeakKeyException in an IllegalStateException naming SIGNATURE_KEY_PROPERTY and the 256-bit minimum; route all three derivation sites through it - drop the now-unused Keys/Decoders imports Notes: verified against jjwt 0.13.0 that signWith only enforces a MINIMUM key length per algorithm, so the 1072-bit DEFAULT_SIGNATURE_KEY is valid for HS256, HS384 and HS512 alike -- honoring the property does not require re-deriving the key per algorithm. Refs: uPortal-Project#3010 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Problem: the jjwt 0.11 -> 0.13 migration rewrote token creation and parsing across four modules, and nothing in the tree exercised either path -- uPortal-soffit-core had only JwtEncryptorTest. A green build said nothing about whether tokens still round-tripped. Goal: pin the token contract so a future jjwt upgrade cannot quietly change it, and cover the two behaviours fixed in the previous commit. Changes: - add BearerServiceTest with six cases: create/parse round-trip asserting username, groups and both single- and multi-value attribute shapes survive; expired token rejected; token signed with a different key rejected; signatureAlgorithm property honored (HS256 appears in the JWT header); unsupported algorithm falls back to HS512; undersized signatureKey fails at init naming the property Notes: JwtEncryptor is mocked as a pass-through rather than constructed -- the real one instantiates jasypt's BasicTextEncryptor, which needs commons-logging that this module's test runtime does not have. Field injection uses plain reflection for the same reason, since Spring's ReflectionTestUtils pulls in commons-logging too. Refs: uPortal-Project#3010 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
I've pushed three commits to this branch rather than leave it parked — it's the keystone for the
Worth saying that the first four passed against your migration unchanged — the round-trip, Two notes on the test, both to avoid adding a dependency:
|
Migrate all JJWT usage to the 0.12+ API:
Affected modules: uPortal-soffit-core, uPortal-soffit-renderer, uPortal-security-core, uPortal-spring