smolnar82 commented on PR #1277: URL: https://github.com/apache/knox/pull/1277#issuecomment-4863391462
I'm generally against about these changes because of the use of reflection here. Let me explain why. There are other ways that avoid reflection: 1. **Multi-Release JAR (MRJAR)**: the textbook solution for exactly this. Base classes compiled at 17 keep `doAs/getSubject`; a `META-INF/versions/18/...` overlay compiled with a JDK 18+ toolchain calls `callAs/current` directly and type-safely. No runtime reflection, no deprecation suppression on the modern path. Cost: we'll need a JDK 18+ available at build (a Maven toolchain) and some pom wiring, which conflicts with the current [17,18) enforcer, so that pin would have to relax for the build JDK. 2. **Raise the baseline to JDK 21** and compile with `--release 21,` dropping JDK 17. Then just call the new methods directly. Simplest code by far; it's a project-policy decision, not a technical blocker. More important than the reflection debate, the change looks incomplete/risky. While analyzing I found things I'd weight higher than the reflection question: 1. Only the Shiro path was migrated; the getter/setter now mismatch. `SubjectUtils.getCurrentSubject()` now prefers `Subject.current()` on JDK 18+, but `Subject.current()` only sees identities established by `callAs`, not by **`doAs`!!** (unless the legacy `-Djdk.security.auth.subject.useTL=true` flag is set). This PR converts one setter (`ShiroSubjectIdentityAdapter`) to `callAs`, but ~10 other auth providers still call `Subject.doAs(...)` directly: ``` AbstractPreAuthFederationFilter, ClientCertFilter, RemoteAuthFilter, AnonymousAuthFilter, AbstractIdentityAssertionFilter, HadoopAuthFilter/PostFilter, Pac4jIdentityAdapter, AccessTokenFederationFilter, AbstractJWTFilter, SpnegoAuthInterceptor, gateway-shell/KnoxSession ``` Consequences on JDK 18+: for any topology using those providers, `getCurrentSubject()` via `current()` would return `null` even though the request was authenticated (regression risk that appears the moment someone runs on 18+, before 23 even matters), and those `doAs` sites still throw `UnsupportedOperationException` on JDK 23+, so `KNOX-3338` isn't actually fully fixed, only the Shiro path is. The migration needs to be all-or-nothing across paired call sites. 2. Exception-wrapping semantics changed. `Subject.doAs` wraps checked exceptions in `PrivilegedActionException`; `Subject.callAs` wraps them in `CompletionException`. `doSubjectAction` unwraps `InvocationTargetException` one level, so the filter chain's `IOException/ServletException` now surfaces wrapped in `CompletionException` instead of the old type. Any caller that unwraps `PrivilegedActionException` will behave differently. Worth verifying the outer filter's error handling. 3. Minor pom hygiene in `gateway-provider-security-shiro/pom.xml`: trailing whitespace after the new `</dependency>` and the removed newline at EOF (\ No newline at end of file). Conclusion: - Reflection works and is defensible, but it's not the only option: MRJAR keeps JDK 17 support with zero runtime reflection and no deprecation suppression on the modern path; bumping the baseline to 21 is even simpler if 17 can be dropped. - Regardless of reflection vs MRJAR, the bigger issue is that the callAs/current pairing is applied inconsistently: migrating only the Shiro setter while the getter switches globally risks breaking identity propagation on JDK 18+ and leaves the other doAs sites still broken on 23+. I'll start a `[DISCUSS]` thread on the DEV mailing list about this matter and closing this PR for now. -- 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]
