-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[POC] [Security Manager Replacement] Native Java Agent (dynamic code rewriting, must be low overhead) #16731
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,9 +77,9 @@ | |
| import java.util.stream.Stream; | ||
|
|
||
| public class DistroTestPlugin implements Plugin<Project> { | ||
| private static final String SYSTEM_JDK_VERSION = "21.0.6+7"; | ||
| private static final String SYSTEM_JDK_VERSION = "23.0.2+7"; | ||
| private static final String SYSTEM_JDK_VENDOR = "adoptium"; | ||
| private static final String GRADLE_JDK_VERSION = "21.0.6+7"; | ||
| private static final String GRADLE_JDK_VERSION = "23.0.2+7"; | ||
| private static final String GRADLE_JDK_VENDOR = "adoptium"; | ||
|
Comment on lines
80
to
83
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On main we have switched back to JDK21 and JDK23 has been deprecated. Thanks.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No worries, I will be testing with JDK-24 asap, thank you |
||
|
|
||
| // all distributions used by distro tests. this is temporary until tests are per distribution | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,7 @@ ${error.file} | |
| 9-:-Xlog:gc*,gc+age=trace,safepoint:file=${loggc}:utctime,pid,tags:filecount=32,filesize=64m | ||
|
|
||
| # Explicitly allow security manager (https://bugs.openjdk.java.net/browse/JDK-8270380) | ||
| 18-:-Djava.security.manager=allow | ||
| 18-23:-Djava.security.manager=allow | ||
|
|
||
| # JDK 20+ Incubating Vector Module for SIMD optimizations; | ||
| # disabling may reduce performance on vector optimized lucene | ||
|
|
@@ -89,3 +89,6 @@ ${error.file} | |
| # See please https://bugs.openjdk.org/browse/JDK-8341127 (openjdk/jdk#21283) | ||
| 23:-XX:CompileCommand=dontinline,java/lang/invoke/MethodHandle.setAsTypeCache | ||
| 23:-XX:CompileCommand=dontinline,java/lang/invoke/MethodHandle.asTypeUncached | ||
|
|
||
| # It should be JDK-24 (but we cannot bring JDK-24 since Gradle does not support it yet) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for Opensearch 3.0, JAVA_21 is indeed the right version, correct?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We bundle JDK 23 but AFAIK our CI images do not have it - they will use JDK 21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah you are correct. |
||
| 21-:-javaagent:agent/opensearch-agent.jar | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,3 +75,7 @@ tasks.test { | |
| tasks.check { | ||
| dependsOn test | ||
| } | ||
|
|
||
| tasks.named('assemble') { | ||
| dependsOn prepareAgent | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| */ | ||
|
|
||
| grant { | ||
| permission java.net.NetPermission "accessUnixDomainSocket"; | ||
| permission java.net.SocketPermission "*", "connect,resolve"; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| */ | ||
|
|
||
| grant { | ||
| permission java.net.NetPermission "accessUnixDomainSocket"; | ||
| permission java.net.SocketPermission "*", "connect,resolve"; | ||
| }; |
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.
Is
-Djava.security.manager=allowneeded anymore now that theSystem.setSecurityManagercalls have been removed?Uh oh!
There was an error while loading. Please reload this page.
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, not anymore :-) I keep this pull request as POC to proof check any other work
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.
@andrross: @cwperks started taking a look at clean-ups of the
-Djava.security.manageracross all code bases in core. We think its not a lot of changes and we can do the clean-up (in a day or two) once the final PR from this POC is merged.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 @kumargu, I should have the clean pull request to core tomorrow, doing some final tests with JDK-24