Add tests for Security Manager #2539

SylvainJuge · 2022-03-23T16:15:33Z

What does this PR do?

add a test for security manager warning implemented in Add warning for misconfigured security manager #2510
add a test that helps to reproduce access to Unsafe when security manager is enabled.
refactor existing tests
add automatic configuration of JVM remote debug running in docker inspired by my own comment written 4 years ago here: https://bsideup.github.io/posts/debugging_containers/

Checklist

fix the agent init with Unsafe, or ignore the test until we can fix this (see comment below Add tests for Security Manager #2539 (comment) for details).
fix/remove all TODOs
~~refactor runtime attach tests to run in docker with this to make them simpler & more reliable.~~ this will be done in a separate PR if needed later.

SylvainJuge · 2022-03-23T16:58:11Z

How to reproduce the issue with Unsafe access:

run the co.elastic.apm.test.ServiceIT#testSecurityManagerWithPolicy test.

Stack trace

java.lang.RuntimeException: java.lang.UnsupportedOperationException: Could not access Unsafe class: sun.misc.Unsafe.defineClass(java.lang.String,[B,int,int,java.lang.ClassLoader,java.security.ProtectionDomain)
  at co.elastic.apm.agent.bci.IndyBootstrap.getIndyBootstrapMethod(IndyBootstrap.java:228) ~[elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.ElasticApmAgent.getTransformer(ElasticApmAgent.java:452) ~[elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.ElasticApmAgent.applyAdvice(ElasticApmAgent.java:412) ~[elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.ElasticApmAgent.initAgentBuilder(ElasticApmAgent.java:333) [elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:270) [elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:164) [elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.ElasticApmAgent.initialize(ElasticApmAgent.java:150) [elastic-apm-agent.jar:?]
  at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
  at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[?:?]
  at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
  at java.lang.reflect.Method.invoke(Method.java:568) ~[?:?]
  at co.elastic.apm.agent.premain.AgentMain.loadAndInitializeAgent(AgentMain.java:160) [elastic-apm-agent.jar:1.30.0-SNAPSHOT]
  at co.elastic.apm.agent.premain.AgentMain.init(AgentMain.java:101) [elastic-apm-agent.jar:1.30.0-SNAPSHOT]
  at co.elastic.apm.agent.premain.AgentMain.premain(AgentMain.java:50) [elastic-apm-agent.jar:1.30.0-SNAPSHOT]
  at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
  at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[?:?]
  at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
  at java.lang.reflect.Method.invoke(Method.java:568) ~[?:?]
  at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:491) [?:?]
  at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:503) [?:?]
Caused by: java.lang.UnsupportedOperationException: Could not access Unsafe class: sun.misc.Unsafe.defineClass(java.lang.String,[B,int,int,java.lang.ClassLoader,java.security.ProtectionDomain)
  at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$Unavailable.initialize(ClassInjector.java:2086) ~[elastic-apm-agent.jar:?]
  at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe.injectRaw(ClassInjector.java:1813) ~[elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.IndyBootstrap.loadClassInBootstrap(IndyBootstrap.java:275) ~[elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.IndyBootstrap.initIndyBootstrap(IndyBootstrap.java:233) ~[elastic-apm-agent.jar:?]
  at co.elastic.apm.agent.bci.IndyBootstrap.getIndyBootstrapMethod(IndyBootstrap.java:219) ~[elastic-apm-agent.jar:?]
  ... 19 more

apmmachine · 2022-03-23T17:09:42Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-03-06T14:39:28.562+0000
Duration: 54 min 28 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3558
Skipped	121
Total	3679

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark tests.
run jdk compatibility tests : Run the JDK Compatibility tests.
run integration tests : Run the Agent Integration tests.
run end-to-end tests : Run the APM-ITs.
run windows tests : Build & tests on windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

felixbarny · 2022-03-28T10:01:21Z

This is the AccessControlException that gets swallowed by Byte Buddy within ClassInjector$UsingUnsafe$Dispatcher$CreationAction:

java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessClassInPackage.jdk.internal.reflect")
	at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:485)
	at java.base/java.security.AccessController.checkPermission(AccessController.java:1068)
	at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:416)
	at java.base/java.lang.SecurityManager.checkPackageAccess(SecurityManager.java:1332)
	at java.base/java.lang.ClassLoader$1.run(ClassLoader.java:690)
	at java.base/java.lang.ClassLoader$1.run(ClassLoader.java:688)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
	at java.base/java.lang.ClassLoader.checkPackageAccess(ClassLoader.java:688)
	at java.base/java.lang.Class.getDeclaredFields0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredFields(Class.java:3297)
	at java.base/java.lang.Class.getDeclaredField(Class.java:2608)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$CreationAction.run(ClassInjector.java:1956)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$CreationAction.run(ClassInjector.java:1908)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe.doPrivileged(ClassInjector.java)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe.<clinit>(ClassInjector.java:1720)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection$Dispatcher$CreationAction.run(ClassInjector.java:522)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection$Dispatcher$CreationAction.run(ClassInjector.java:508)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection.doPrivileged(ClassInjector.java)
	at net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection.<clinit>(ClassInjector.java:135)
	at net.bytebuddy.dynamic.NexusAccessor$Dispatcher$CreationAction.run(NexusAccessor.java:237)
	at net.bytebuddy.dynamic.NexusAccessor$Dispatcher$CreationAction.run(NexusAccessor.java:220)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:318)
	at net.bytebuddy.dynamic.NexusAccessor.doPrivileged(NexusAccessor.java)
	at net.bytebuddy.dynamic.NexusAccessor.<clinit>(NexusAccessor.java:58)
	at net.bytebuddy.agent.builder.AgentBuilder$InitializationStrategy$SelfInjection$Split.<init>(AgentBuilder.java:3683)
	at net.bytebuddy.agent.builder.AgentBuilder$Default.<init>(AgentBuilder.java:9762)
	at co.elastic.apm.agent.bci.ElasticApmAgent.getAgentBuilder(ElasticApmAgent.java:666)
	at co.elastic.apm.agent.bci.ElasticApmAgent.initAgentBuilder(ElasticApmAgent.java:325)
	at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:270)
	at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:164)
	at co.elastic.apm.agent.bci.ElasticApmAgent.initialize(ElasticApmAgent.java:150)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at co.elastic.apm.agent.premain.AgentMain.loadAndInitializeAgent(AgentMain.java:160)
	at co.elastic.apm.agent.premain.AgentMain.init(AgentMain.java:101)
	at co.elastic.apm.agent.premain.AgentMain.premain(AgentMain.java:50)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:491)
	at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:503)

I'm not sure why granting java.security.AllPermission does not fix that.

eyalkoren · 2022-03-28T10:56:33Z

I'm not sure why granting java.security.AllPermission does not fix that.

Maybe this is because it's relying on the package access permissions (java.base/java.lang.SecurityManager.checkPackageAccess), as opposed to the ProtectionDomain-based check, which relies on the code source. Our policy addition grants permission based on the code source.

felixbarny · 2022-03-28T12:34:06Z

Do you have a suggestion on how the policy can be adapted?

eyalkoren · 2022-03-29T04:06:17Z

Do you have a suggestion on how the policy can be adapted?

No, I wasn't familiar with that even, we'll need to see why that is and if it is indeed related to the different permission check route.

…curity-manager-setup

SylvainJuge · 2022-04-06T07:46:28Z

After a bit of further investigation, it the following policy allows to work around the permission issue. With it, it goes a bit further but we have another issue related to log4j loading.

When trying to load the Unsafe class, the protection domain is null, hence we have to grant the permission globally instead of only the agent jar. In this case, it means the whole application get extra permissions and it's thus likely not desirable.

grant {
    permission java.lang.RuntimePermission "accessClassInPackage.jdk.internal.reflect";
};

grant codeBase "file:///agent.jar" {
    permission java.security.AllPermission;
};

In the case where the agent is granted all permissions, I think it could be fine to have the agent self-allow itself and bypass the security manager as the user has already granted it the permissions in the policy. In other words, the issue of loading the Unsafe class would just be an implementation detail where we have to add an extra work-around because the protection domain is not as expected.

A better approach though would be to see if there is a way in byte-buddy to reuse the agent protection domain when loading the Unsafe class.

botelastic · 2022-11-08T18:51:56Z

Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as stalled to make it hit our filters and make sure we get back to it in as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

…curity-manager-setup

eyalkoren · 2022-11-20T13:23:08Z

The issue described above was resolved through raphw/byte-buddy#1359 (ported to the agent through #2874).

It occurs when BB loads a mirror of a JVM class only to get a field’s offset. While doing so, the JVM checks whether the current AccessControlContext (represented by the mirror class's ProtectionDomain) has the permission to access the jdk.internal.reflect.ReflectionFactory class. Since the mirror class is not loaded with any specific ProtectionDomain, this check fails (and swallowed quietly by BB). The fix was to load the mirror classes with the same ProtectionDomain of the mirrored class, thus making the check of this part of the stack pass and move down until it reaches the ByteBuddy doPrivileged action, which passes if the agent is granted the proper permissions through policy.

SylvainJuge · 2022-11-21T08:13:24Z

Since this issue is now solved, I think it would be a good idea to move forward with this PR to prevent further regressions:

testing with both JDK 11 and JDK 17
maybe testing with complete tomcat server + simple web app to cover what is fixed in Fix security manager issues #2871
in both cases that will be integration tests, and we have to make sure that most agent features are enabled, especially the ones that will require permissions to create files like log reformatting or sampling profiler).

eyalkoren · 2022-11-21T08:21:58Z

These tests currently pass even when the agent cannot work with security manager, thus they cannot prevent regression in the current form.
Let's discuss offline and see what we do with those and what we do with the Servlet container tests.

…curity-manager-setup

eyalkoren · 2023-02-05T14:52:26Z

@SylvainJuge this stayed far behind main by now. Much of the included additions are already merged and we also have full Servlet container tests with Security Manager since #2883 .
Please decide if and what to keep from the changes in this PR and let's merge what you want or close.

…curity-manager-setup

github-actions · 2023-03-06T14:07:45Z

/test

SylvainJuge · 2023-03-06T14:09:53Z

integration-tests/main-app-test/src/test/java/co/elastic/apm/test/AgentSetupIT.java

+    void testServiceNameAndVersionFromManifest(String image) {
+        try (GenericContainer<TestAppContainer> app = testAppWithJavaAgent(image)) {
+
+            app.waitingFor(Wait.forLogMessage(".* Starting Elastic APM .*", 1))
+                .start();
+
+            assertThat(app.getLogs()).contains(" as My Service Name (My Service Version) on ");
+        }
+    }


[for reviewer] this was actually tested previously within ServiceTestIT

SylvainJuge added 3 commits March 23, 2022 14:17

create container for simple test apps

6a19f43

refactor ServiceIT + test security manager warn

c5ab999

reproduce one issue with SecurityManager

82d298f

SylvainJuge mentioned this pull request Mar 23, 2022

NoClassDefFound with Tomcat + Security Manager #2520

Closed

wip : add remote debug + better tmp files

3990393

SylvainJuge mentioned this pull request Mar 28, 2022

Elasticsearch instrumentation #2550

Closed

7 tasks

Merge branch 'main' of github.com:elastic/apm-agent-java into test-se…

96ec236

…curity-manager-setup

botelastic bot added stalled and removed stalled labels Nov 8, 2022

SylvainJuge self-assigned this Nov 14, 2022

Merge branch 'main' of github.com:elastic/apm-agent-java into test-se…

2189ca9

…curity-manager-setup

eyalkoren assigned eyalkoren and unassigned SylvainJuge Nov 15, 2022

make agent work better with security manager

932fbcc

This was referenced Nov 17, 2022

Using explicit ProtectionDomain in dynamically loaded classes raphw/byte-buddy#1359

Merged

Bump ByteBuddy to version 1.12.19 #2874

Merged

Merge branch 'main' of github.com:elastic/apm-agent-java into test-se…

11a06fe

…curity-manager-setup

Merge branch 'main' of github.com:elastic/apm-agent-java into test-se…

ef0097e

…curity-manager-setup

SylvainJuge added 3 commits March 2, 2023 18:55

make test pass again

0dc61be

rename test

5b9037c

add convenient remote debug for JVM in containers

b82d2bc

SylvainJuge marked this pull request as ready for review March 6, 2023 14:07

SylvainJuge commented Mar 6, 2023

View reviewed changes

SylvainJuge requested a review from jackshirazi March 6, 2023 14:28

jackshirazi approved these changes Mar 6, 2023

View reviewed changes

SylvainJuge merged commit a13845d into elastic:main Mar 6, 2023

SylvainJuge deleted the test-security-manager-setup branch March 6, 2023 15:39

SylvainJuge mentioned this pull request Apr 12, 2023

[release] update 1.x branch #3107

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for Security Manager #2539

Add tests for Security Manager #2539

SylvainJuge commented Mar 23, 2022 •

edited

SylvainJuge commented Mar 23, 2022 •

edited by felixbarny

apmmachine commented Mar 23, 2022 •

edited

Build stats

Test stats 🧪

felixbarny commented Mar 28, 2022

eyalkoren commented Mar 28, 2022

felixbarny commented Mar 28, 2022

eyalkoren commented Mar 29, 2022

SylvainJuge commented Apr 6, 2022

botelastic bot commented Nov 8, 2022

eyalkoren commented Nov 20, 2022

SylvainJuge commented Nov 21, 2022

eyalkoren commented Nov 21, 2022

eyalkoren commented Feb 5, 2023

github-actions bot commented Mar 6, 2023

SylvainJuge Mar 6, 2023

Add tests for Security Manager #2539

Add tests for Security Manager #2539

Conversation

SylvainJuge commented Mar 23, 2022 • edited

What does this PR do?

Checklist

SylvainJuge commented Mar 23, 2022 • edited by felixbarny

apmmachine commented Mar 23, 2022 • edited

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

felixbarny commented Mar 28, 2022

eyalkoren commented Mar 28, 2022

felixbarny commented Mar 28, 2022

eyalkoren commented Mar 29, 2022

SylvainJuge commented Apr 6, 2022

botelastic bot commented Nov 8, 2022

eyalkoren commented Nov 20, 2022

SylvainJuge commented Nov 21, 2022

eyalkoren commented Nov 21, 2022

eyalkoren commented Feb 5, 2023

github-actions bot commented Mar 6, 2023

SylvainJuge Mar 6, 2023

Choose a reason for hiding this comment

SylvainJuge commented Mar 23, 2022 •

edited

SylvainJuge commented Mar 23, 2022 •

edited by felixbarny

apmmachine commented Mar 23, 2022 •

edited