HADOOP-19212. Avoid Subject.getSubject method on newer JVMs #7081
+195
−8
Conversation
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
In Java 23, Subject.getSubject requires setting the system property java.security.manager to allow, else it will throw an exception. More detail is available in the release notes: https://jdk.java.net/23/release-notes This is in support of the eventual removal of the security manager, at which point, Subject.getSubject will be removed. Since this project supports older Java releases, and the API which one must migrate to was only introduced in Java 18, I hid the implementations behind a runtime version check. I verified against several local Java versions that the correct classes are loaded (and more importantly, that the incorrect classes are _not_ loaded). The outright removal of the security manager, and by extension the Subject.getSubject method would be a particularly large problem if we do not address this, which was my motivation here. Some manual testing on Java 11, 17, and 23: ``` » java --version openjdk 11.0.23 2024-04-16 LTS » java -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter Current subject is null » java --version openjdk 17.0.5 2022-10-18 LTS » java -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter Current subject is null » java --version openjdk 23 2024-09-17 » java -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter Current subject is null ``` As desired, the class containing the old implementation does not get loaded on a new JVM: ``` » java --version openjdk 23 2024-09-17 » java -verbose:class -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter | \grep -i subject | grep "class,load" | awk '{print $2}' org.apache.hadoop.util.subject.SubjectAdapter org.apache.hadoop.util.subject.HiddenGetSubject javax.security.auth.Subject org.apache.hadoop.util.subject.GetSubjectNg ``` And the inverse is true for an older JVM: ``` » java --version openjdk 17.0.5 2022-10-18 LTS » java -verbose:class -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter | \grep -i subject | grep "class,load" | awk '{print $2}' org.apache.hadoop.util.subject.SubjectAdapter org.apache.hadoop.util.subject.HiddenGetSubject javax.security.auth.Subject org.apache.hadoop.util.subject.ClassicGetSubject javax.security.auth.Subject$1 ```
|
🎊 +1 overall
This message was automatically generated. |
jojochuang
reviewed
Sep 30, 2024
|
I am happy to report no other issues. This Subject usage in hadoop-common impacts a great many projects, so |
|
Ok sounds like we should get it backported to 3.4.x and 3.3.x too. |
jbrinegar
changed the title
|
@steveloughran PR for HADOOP-19212. I updated the title to reflect the duplicate change. |
steveloughran
requested changes
Oct 2, 2024
|
|
||
| GetSubjectNg() { | ||
| try { | ||
| currentMethod = Subject.class.getMethod("current"); |
There was a problem hiding this comment.
This class, and all classes added in this PR, are in the hadoop-auth module because Subject.getSubject is used in both hadoop-auth (KerberosAuthenticator) and hadoop-common (UserGroupInformation). hadoop-common depends on hadoop-auth, so that seemed like a reasonable fit.
DynMethods, however, resides in hadoop-common, and thus it cannot be used in hadoop-auth.
How would you like me to proceed?
There was a problem hiding this comment.
@steveloughran ,
@jbrinegar 's point is valid.
How do you propose solving the circular dependency issue ?
Should the DynMethods code be copied to hadoop-auth ?
Is there some suiteable small external dependency that provides DynMethods ?
There was a problem hiding this comment.
moving Dyn* to hadoop-auth module? then they can be consumed by every hadoop module.
There was a problem hiding this comment.
I agree.
I've just confirmed that hadoop-common already depends on hadoop-auth, so the move should be transparent to all users.
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); |
There was a problem hiding this comment.
Use Shell.isJavaVersionAtLeast(18)
There was a problem hiding this comment.
My comment about dependencies applies to Shell as well. Let me know how you'd like to proceed with this one as well.
There was a problem hiding this comment.
IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
There was a problem hiding this comment.
IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
interesting thought.
how about
- we avoid Subject
- pull the string on L33 to a constant
There was a problem hiding this comment.
@stoty interesting thought about attempting and fallbacks
We'd try the java18+ first & fall back to java <= 17 otherwise? or do it in the opposite direction, at least for now?
...-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/SubjectAdapter.java
Outdated
Show resolved
Hide resolved
...-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/SubjectAdapter.java
Outdated
Show resolved
Hide resolved
...op-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/GetSubjectNg.java
Outdated
Show resolved
Hide resolved
|
@steveloughran thank you for the review. I have pushed a second commit that addresses feedback marked with 👍. The title of this PR was updated to include "HADOOP-19212." as a prefix. I am happy to rebase/squash once review is complete, if squashing is necessary. Due to dependencies, I cannot use |
pan3793
reviewed
Oct 3, 2024
...ommon-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/HiddenGetSubject.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
pan3793
reviewed
Oct 16, 2024
...-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/SubjectAdapter.java
Outdated
Show resolved
Hide resolved
pan3793
reviewed
Oct 16, 2024
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Show resolved
Hide resolved
pan3793
approved these changes
Oct 16, 2024
|
JEP 486: Permanently Disable the Security Manager is on the JDK's technical roadmap. As part of the changes (PR in review) will be for Subject.current to throw unconditionally, meaning the workaround to allow a SecurityManager will no longer work. So the timing of the PR here is good. |
|
💔 -1 overall
This message was automatically generated. |
|
I am not sure I have the permissions to retrigger the failing checks. Ultimate failure was on the remote side: This java code compiles just fine: |
steveloughran
requested changes
Oct 18, 2024
| // how getSubject operates depends on the JVM calling it. | ||
| // asserting that it does not throw is a valid test, especially on Java 18 and above | ||
| // prior to Java 18, this method is just a simple wrapper | ||
| assertDoesNotThrow(() -> SubjectAdapter.getSubject()); |
There was a problem hiding this comment.
Use our LambdaTestUtils.intercept() here as it fails better, can validates the type and message
There was a problem hiding this comment.
The comment in #7081 (comment) applies here as well.
| /** | ||
| * Indirectly calls Subject.current(), which exists in Java 18 and above only | ||
| */ | ||
| class SubjectAdapterJava18AndAbove implements HiddenSubjectAdapter { |
There was a problem hiding this comment.
Please use our new DynMethods classes, (which we copied from parquet and which is also found in iceberg). It fails better and as it is so common it's good to get familiar with it
There was a problem hiding this comment.
@steveloughran my comment from here still applies. #7081 (comment)
This was referenced Jan 8, 2025
Merged
This was referenced Jan 14, 2025
stoty
reviewed
Jan 20, 2025
|
|
||
| import javax.security.auth.Subject; | ||
|
|
||
| public interface HiddenSubjectAdapter { |
There was a problem hiding this comment.
I don't really like the name, maybe rename to JDKSpecificSubjectAdapter or similar ?
cnauroth
requested changes
Jan 24, 2025
There was a problem hiding this comment.
Thanks for this important patch, @jbrinegar ! I entered a few minor comments.
This is very nitpicky, but there is a comment in test code that still mentions AccessControlContext:
Can you remove mention of that in the comment?
This is a great step, but there will still be references to the deprecated APIs in other parts of the Hadoop codebase. (See below.) Are you interested in addressing all of these, or do you prefer to focus on getting in a hadoop-common patch and then we can focus on other spots piecemeal?
> grep -r --include '*.java' 'AccessController' *
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/client/KerberosAuthenticator.java:import java.security.AccessController;
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/client/KerberosAuthenticator.java: AccessControlContext context = AccessController.getContext();
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/PlatformName.java:import java.security.AccessController;
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/PlatformName.java: return AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> {
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsConfig.java:import static java.security.AccessController.*;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/FastByteComparisons.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/FastByteComparisons.java: theUnsafe = (Unsafe) AccessController.doPrivileged(
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java: AccessControlContext context = AccessController.getContext();
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynMethods.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynMethods.java: AccessController.doPrivileged(new MakeAccessible(hidden));
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynConstructors.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynConstructors.java: AccessController.doPrivileged(new MakeAccessible(hidden));
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CleanerUtil.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CleanerUtil.java: final Object hack = AccessController.doPrivileged(
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CleanerUtil.java: final Throwable error = AccessController.doPrivileged(
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/AbstractFuture.java:import java.security.AccessController;
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/AbstractFuture.java: AccessController.doPrivileged(
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapred/LocalDistributedCacheManager.java:import java.security.AccessController;
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapred/LocalDistributedCacheManager.java: return AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() {
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapred/LocalDistributedCacheManager.java: AccessController.doPrivileged(new PrivilegedAction<Void>() {
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java:import java.security.AccessController;
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java: return AccessController.doPrivileged(
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/pipes/Submitter.java:import java.security.AccessController;
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/pipes/Submitter.java: AccessController.doPrivileged(
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timeline-pluginstorage/src/main/java/org/apache/hadoop/yarn/server/timeline/EntityGroupFSTimelineStore.java:import java.security.AccessController;
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timeline-pluginstorage/src/main/java/org/apache/hadoop/yarn/server/timeline/EntityGroupFSTimelineStore.java: return AccessController.doPrivileged(
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxiliaryServiceWithCustomClassLoader.java:import java.security.AccessController;
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxiliaryServiceWithCustomClassLoader.java: return AccessController.doPrivileged(
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.util.subject; |
There was a problem hiding this comment.
Can you please add a package-info.java in this new sub-directory and annotate it as private and unstable?
|
|
||
| import javax.security.auth.Subject; | ||
|
|
||
| public interface HiddenSubjectAdapter { |
There was a problem hiding this comment.
Can this be package-private instead of public?
|
Thank you for working on this patch @jbrinegar! This will help eliminate a big blocker for us in trying to upgrade our stack to JDK 23. |
|
@ryantomlinson95 @cnauroth @steveloughran @jbrinegar @jojochuang @pan3793 I'd like to bring to your attention HADOOP-19486, and the corresponding thread I have opened on dev@hadoop.apache.org , where I track my full JDK23 support work. Please check my email, and join the discussion. |
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); |
There was a problem hiding this comment.
IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
interesting thought.
how about
- we avoid Subject
- pull the string on L33 to a constant
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); |
There was a problem hiding this comment.
@stoty interesting thought about attempting and fallbacks
We'd try the java18+ first & fall back to java <= 17 otherwise? or do it in the opposite direction, at least for now?
|
My alternate patch tries the new API first, and falls back to the old one, @steveloughran . Conversion either way has an overhead, as the Callable / PrivilegedExceptionAction conversion and exception re-wrapping has to happen, but IMO anything we do inside those doAs() blocks is heavyweight enough that the overhad is relatively negligible. |
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.
In Java 23, Subject.getSubject requires setting the system property java.security.manager to allow, else it will throw an exception. More detail is available in the release notes: https://jdk.java.net/23/release-notes
This is in support of the eventual removal of the security manager, at which point, Subject.getSubject will be removed. Since this project supports older Java releases, and the API which one must migrate to was only introduced in Java 18, I hid the implementations behind a runtime version check. I verified against several local Java versions that the correct classes are loaded (and more importantly, that the incorrect classes are not loaded). The outright removal of the security manager, and by extension the Subject.getSubject method would be a particularly large problem if we do not address this, which was my motivation here.
Some manual testing on Java 11, 17, and 23:
As desired, the class containing the old implementation does not get loaded on a new JVM:
And the inverse is true for an older JVM:
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?