-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-17078: Add SecurityManagerCompatibility shim #16522
KAFKA-17078: Add SecurityManagerCompatibility shim #16522
Conversation
Signed-off-by: Greg Harris <[email protected]>
Signed-off-by: Greg Harris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gharris1727! The shim strategy here looks excellent overall.
I've gone through everything except the new unit test for the security manager compatibility class. Most comments are trivial or requests for more documentation; I do think I found one possible bug, though. Let me know when this is ready for another round and I can take a look at the tests.
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/internals/SecurityManagerCompatibility.java
Outdated
Show resolved
Hide resolved
|
||
<T> T doPrivileged(PrivilegedAction<T> action); | ||
|
||
Subject current(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe call this currentSubject
? The name is a little ambiguous and doesn't immediately point readers towards the JDK API that it's a shim for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the JRE-18+ API is called Subject#current
and I think we should have parity with that, so that in the future SecurityManagerCompatibility.get().current()
can be replaced with Subject.current()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, it's a simple IDE find+replace to update this in the future, and Subject::current
has meaning to it that SecurityManagerCompatibility.get().current()
does not. I don't think it's worth the hit in readability for the (very long) interim period where we use this class instead of the JDK APIs that will eventually replace it. But I won't block on it.
Signed-off-by: Greg Harris <[email protected]>
Signed-off-by: Greg Harris <[email protected]>
Signed-off-by: Greg Harris <[email protected]>
Signed-off-by: Greg Harris <[email protected]>
...apache/kafka/common/security/oauthbearer/internals/OAuthBearerSaslClientCallbackHandler.java
Show resolved
Hide resolved
Signed-off-by: Greg Harris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Greg!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* AccessControlContext will be ignored while the action is performed. | ||
* | ||
* @param <T> the type of the value returned by the PrivilegedAction's | ||
* {@code run} method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent
@gharris1727 , I think we can merge this change now. Let me know if you have objection. |
Is there a plan on how/when this change will be integrated? Using kafka-clients:3.7.1 with JDK 23 we get the following exception: java.lang.UnsupportedOperationException: getSubject is supported only if a security manager is allowed
at javax.security.auth.Subject.getSubject(Subject.java:347)
at org.apache.kafka.common.security.oauthbearer.internals.OAuthBearerSaslClientCallbackHandler.handleCallback(OAuthBearerSaslClientCallbackHandler.java:99)
at org.apache.kafka.common.security.oauthbearer.internals.OAuthBearerSaslClientCallbackHandler.handle(OAuthBearerSaslClientCallbackHandler.java:83)
at org.apache.kafka.common.security.oauthbearer.internals.OAuthBearerSaslClient.evaluateChallenge(OAuthBearerSaslClient.java:92)
at org.apache.kafka.common.security.authenticator.SaslClientAuthenticator.lambda$createSaslToken$1(SaslClientAuthenticator.java:535) |
@gharris1727 can you merge in trunk to run this with the new CI? |
Hey @danishnawab thanks for reminding us. Since this may only be included in future releases (3.7.2, 3.8.1, 3.9.1, 4.0.0), existing releases you can use the workaround of setting the system property |
Thanks for the tip @gharris1727. Any idea a future release with this fix might be out? I can then evaluate if we should wait a bit longer for that. |
Hey @danishnawab I think the earliest we could release this would be 1-4 months from today. Officially Kafka only supports up to Java 21, but we can look to add Java 23 compatibility support in Kafka 4.0, in case this is not the only compatibility issue. |
@showuon I see some real test failures in the CI, this is not ready to merge just yet. I'll start working through the failures now. |
Signed-off-by: Greg Harris <[email protected]>
/** | ||
* A strategy which uses reflection to access methods without requiring them at compile-time. | ||
*/ | ||
abstract class ReflectiveStrategy implements SecurityManagerCompatibility { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to to be an abstract class. It doesn't have any fields. This can be achieve with static methods, no? And it avoids all of the complexity of inheritance and abstract classes.
Also this class doesn't really implement SecurityManagerCompatibility
. It doesn't implement a single method of that interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using this class to store common methods that were used by both the Legacy and Modern implementations, but not by Composite or Unsupported.
I converted it to a package-local util class instead.
Signed-off-by: Greg Harris <[email protected]>
Test failures are resolved, I'll merge this to trunk tomorrow. I plan to backport this to 3.8 and 3.7 immediately, and then 3.9 after the code freeze is finished. Edit: there were some concerns raised about backporting, i'll only merge this to trunk for now. |
@gharris1727 it seems these changes couldn't make it to 3.9.0 release [1]. [1] https://downloads.apache.org/kafka/3.9.0/RELEASE_NOTES.html |
@danishnawab This and the other improvements for Java 23 compatibility are coming in 4.0.0: https://issues.apache.org/jira/browse/KAFKA-17638 |
Thanks @gharris1727. |
@danishnawab You can find the tenative release schedule for 4.0 in the release plan: https://cwiki.apache.org/confluence/display/KAFKA/Release+Plan+4.0.0 |
Is this going to be backported to 3.9? Without this change, users on JDK 24 are forced to upgrade to 4.0 due to a lack of alternatives (or patch it on their end). This is something that came up during our pre-24 tests in https://github.com/trinodb/trino |
@wendigo I've raised this on the mailing list for discussion: https://lists.apache.org/thread/vl43q9wqq4xs67xx61f0t0850y2b037o Please join the discussion and advocate for this. I think it makes sense to disconnect the Java version from the major version upgrade, but this wasn't ready in time for the 3.9.0 release. |
@gharris1727 done :) |
hello. is this PR resolving KIP-1006? |
According to the discussion in the mailing list, there is no will to backport this to 3.9. This will be available in 4.0 which also drops support for Zookeeper based coordination which is a no-go for a lot of projects so I guess that Kafka won't support JDK23+ in the meantime |
will a kafka client in version 4 be able to talk to a kafka 3 broker? |
Thanks for the question @vsevel If you decide to upgrade to the Kafka 4.0 clients to gain Java 23+ support, applications will no longer be able to contact brokers running Kafka 0.x, 1.x, or 2.0.x You can see the planned phase-out of protocol versions here: https://cwiki.apache.org/confluence/display/KAFKA/KIP-896%3A+Remove+old+client+protocol+API+versions+in+Kafka+4.0 |
This is a shim to allow the use of both modern (JRE 18+) and legacy APIs (deprecated in 17, degraded in 23, removal unknown) related to JEP 411: Deprecate the Security Manager for Removal.
This shim implements the interim strategy outlined in KIP-1006: Remove SecurityManager Support where legacy APIs are used as long as they are available and functional. Once the legacy APIs are removed or degraded, the modern APIs are used instead.
The shim is structured as a general interface with four 'strategy' implementations for testability. These implementations allow for mocking out the classloading infrastructure to simulate situations which no current JRE implements, namely removal and further degradation of functionality.
Committer Checklist (excluded from commit message)