HADOOP-19212. Avoid Subject.getSubject method on newer JVMs #7081
Conversation
This file contains 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?
| 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.
...-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)
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?