Aias00 commented on PR #6408: URL: https://github.com/apache/shenyu/pull/6408#issuecomment-5156776589
Reviewed #6408 — decoupling the JWT signing key from the password hash is the right direction and the two call sites (`DashboardUserServiceImpl` sign, `ShiroRealm` verify) are updated consistently; the rewritten `ShiroRealmTest` token signatures check out. But I think this needs another pass before merge. **Blocker** 1. The default-key fallback is not production-safe. `JwtProperties.@PostConstruct` generates a random 32-byte key *in memory* when the configured key equals the sentinel `"defaultSecretKey"`, and the shipped `application.yml` sets exactly that sentinel — so out-of-the-box every restart mints a new key and all issued JWTs stop validating (mass forced re-login on every deploy/restart). Worse, ShenYu supports clustered admin (`shenyu.cluster`), and each instance generates a *different* random key, so a token issued by instance A fails verification on instance B — cluster auth is broken. The e2e only passes because it runs in a single boot. Either require an explicit key and fail-fast in production, or persist the generated key (DB / shared store) so it survives restart and is shared across instances; an ephemeral random fallback, if kept, should be dev-profile only and not shipped as the default. **Should fix** 2. Upgrade/migration: pre-PR tokens are signed with the user's password hash; post-PR they're verified against `secretKey`, so every existing session is invalidated on upgrade (and again on rollback). This is acceptable for a security fix but isn't documented — please add a release/migration note and a startup WARN so operators aren't surprised. 3. `JwtProperties.init()` only randomizes when `secretKey` equals the literal `"defaultSecretKey"`. A blank/empty/null `shenyu.jwt.secret-key:` is not caught — `Algorithm.HMAC256(null)` returns `""` (verified by `JwtUtilsTest.testGenerateTokenWithNullKey`), breaking all login; an empty string yields an insecure empty HMAC key with no warning. Detect `null/blank` (not just the magic string) and randomize-or-fail-fast. The sentinel-string approach also silently replaces a key a user might legitimately set to `"defaultSecretKey"`. 4. Coverage gap: in `DashboardUserServiceTest`, `jwtProperties` is a `@Mock` and `getSecretKey()` is never stubbed, so it returns `null` and `login()` produces an empty-string token; `assertLoginSuccessful` only checks id/userName/password, so the test passes despite the issued token being invalid. The PR's whole point is changing the signing key — please add a regression test stubbing `jwtProperties.getSecretKey()` with a real key and asserting `JwtUtils.verifyToken(loginResult.getToken(), key)` is true (and false for a wrong key). 5. `JwtPropertiesTest` isn't updated for the new `secretKey` field/getter/setter or the `@PostConstruct` randomization (the security-critical part). Note `new JwtProperties()` won't trigger `@PostConstruct`, so the randomization path needs a Spring-context test or a direct `init()` call. **Nits** 6. The `LOG.warn(...)` says the default is "not secure" immediately before making it secure (random). Reword to state the real consequence: ephemeral key → tokens don't survive restart and multi-instance breaks; configure `shenyu.jwt.secretKey` (or `SHENYU_JWT_SECRETKEY`) for production. 7. Consider not committing the literal sentinel `"defaultSecretKey"` to `application.yml`, and document the env-var / external-secret approach for real deployments. CI is green, but the e2e doesn't exercise restart or multi-instance, so it can't catch (1). -- 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]
