From 136bc253440a9092205b600275eb185ef4a57c80 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 6 Mar 2023 11:39:23 +0200 Subject: [PATCH 01/10] Improve compatibility with SecurityManager --- .../javaagent/OpenTelemetryAgent.java | 31 ++++---- .../javaagent/bootstrap/AgentClassLoader.java | 9 ++- .../javaagent/bootstrap/AgentInitializer.java | 71 +++++++++++++++++-- .../tooling/ExtensionClassLoader.java | 11 +++ .../javaagent/tooling/HelperInjector.java | 8 ++- 5 files changed, 109 insertions(+), 21 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java index 642dcb403c61..eb66597d0c61 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java @@ -11,8 +11,9 @@ import java.io.File; import java.io.IOException; import java.lang.instrument.Instrumentation; +import java.net.JarURLConnection; import java.net.URISyntaxException; -import java.security.CodeSource; +import java.net.URL; import java.util.jar.JarFile; import java.util.jar.Manifest; @@ -64,24 +65,30 @@ private static void startAgent(Instrumentation inst, boolean fromPremain) { private static synchronized File installBootstrapJar(Instrumentation inst) throws IOException, URISyntaxException { - - CodeSource codeSource = OpenTelemetryAgent.class.getProtectionDomain().getCodeSource(); - - if (codeSource == null) { - throw new IllegalStateException("could not get agent jar location"); + // we are not using OpenTelemetryAgent.class.getProtectionDomain().getCodeSource() to get agent + // location because getProtectionDomain does a permission check with security manager + URL url = + OpenTelemetryAgent.class + .getClassLoader() + .getResource(OpenTelemetryAgent.class.getName().replace('.', '/') + ".class"); + if (url == null || !"jar".equals(url.getProtocol())) { + throw new IllegalStateException("could not get agent jar location from url " + url); } - - File javaagentFile = new File(codeSource.getLocation().toURI()); + String resourcePath = url.toURI().getSchemeSpecificPart(); + int protocolSeparatorIndex = resourcePath.indexOf(":"); + int resourceSeparatorIndex = resourcePath.indexOf("!/"); + if (protocolSeparatorIndex == -1 || resourceSeparatorIndex == -1) { + throw new IllegalStateException("could not get agent location from url " + url); + } + String agentPath = resourcePath.substring(protocolSeparatorIndex + 1, resourceSeparatorIndex); + File javaagentFile = new File(agentPath); if (!javaagentFile.isFile()) { throw new IllegalStateException( "agent jar location doesn't appear to be a file: " + javaagentFile.getAbsolutePath()); } - // passing verify false for vendors who sign the agent jar, because jar file signature - // verification is very slow before the JIT compiler starts up, which on Java 8 is not until - // after premain execution completes - JarFile agentJar = new JarFile(javaagentFile, false); + JarFile agentJar = ((JarURLConnection) url.openConnection()).getJarFile(); verifyJarManifestMainClassIsThis(javaagentFile, agentJar); inst.appendToBootstrapClassLoaderSearch(agentJar); return javaagentFile; diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java index 4831e05476a3..dc2a508ad092 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java @@ -16,8 +16,11 @@ import java.net.URLClassLoader; import java.net.URLConnection; import java.net.URLStreamHandler; +import java.security.AllPermission; import java.security.CodeSource; import java.security.Permission; +import java.security.Permissions; +import java.security.ProtectionDomain; import java.security.cert.Certificate; import java.util.Enumeration; import java.util.jar.JarEntry; @@ -63,6 +66,7 @@ public class AgentClassLoader extends URLClassLoader { private final URL jarBase; private final String jarEntryPrefix; private final CodeSource codeSource; + private final ProtectionDomain protectionDomain; private final Manifest manifest; /** @@ -93,6 +97,9 @@ public AgentClassLoader(File javaagentFile, String internalJarFileName) { jarBase = new URL("x-internal-jar", null, 0, "/", new AgentClassLoaderUrlStreamHandler(jarFile)); codeSource = new CodeSource(javaagentFile.toURI().toURL(), (Certificate[]) null); + Permissions permissions = new Permissions(); + permissions.add(new AllPermission()); + protectionDomain = new ProtectionDomain(codeSource, permissions); manifest = jarFile.getManifest(); } catch (IOException e) { throw new IllegalStateException("Unable to open agent jar", e); @@ -173,7 +180,7 @@ private Class findAgentClass(String name) throws ClassNotFoundException { } public Class defineClass(String name, byte[] bytes) { - return defineClass(name, bytes, 0, bytes.length, codeSource); + return defineClass(name, bytes, 0, bytes.length, protectionDomain); } private byte[] getJarEntryBytes(JarEntry jarEntry) throws IOException { diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java index 22de783d9a0a..4a61c75fdcb0 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java @@ -7,7 +7,11 @@ import java.io.File; import java.lang.instrument.Instrumentation; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.lang.reflect.Constructor; +import java.security.PrivilegedExceptionAction; import javax.annotation.Nullable; /** @@ -29,10 +33,30 @@ public static void initialize(Instrumentation inst, File javaagentFile, boolean return; } - agentClassLoader = createAgentClassLoader("inst", javaagentFile); - agentStarter = createAgentStarter(agentClassLoader, inst, javaagentFile); - if (!fromPremain || !delayAgentStart()) { - agentStarter.start(); + // we expect that at this point agent jar has been appended to boot class path and all agent + // classes are loaded in boot loader + if (AgentInitializer.class.getClassLoader() != null) { + throw new IllegalStateException("agent initializer should be loaded in boot loader"); + } + + execute( + () -> { + agentClassLoader = createAgentClassLoader("inst", javaagentFile); + agentStarter = createAgentStarter(agentClassLoader, inst, javaagentFile); + if (!fromPremain || !delayAgentStart()) { + agentStarter.start(); + } + + return null; + }); + } + + @SuppressWarnings("removal") + private static void execute(PrivilegedExceptionAction action) throws Exception { + if (System.getSecurityManager() != null && AccessControllerInvoker.canInvoke()) { + AccessControllerInvoker.invoke(action); + } else { + action.run(); } } @@ -81,8 +105,12 @@ private static boolean delayAgentStart() { * Call to this method is inserted into {@code sun.launcher.LauncherHelper.checkAndLoadMain()}. */ @SuppressWarnings("unused") - public static void delayedStartHook() { - agentStarter.start(); + public static void delayedStartHook() throws Exception { + execute( + () -> { + agentStarter.start(); + return null; + }); } public static ClassLoader getExtensionsClassLoader() { @@ -113,4 +141,35 @@ private static AgentStarter createAgentStarter( } private AgentInitializer() {} + + // using java.security.AccessController directly causes build to fail due to warnings about + // using a terminally deprecated class + private static class AccessControllerInvoker { + private static final MethodHandle doPrivilegedMethodHandle = getDoPrivilegedMethodHandle(); + + private static MethodHandle getDoPrivilegedMethodHandle() { + try { + Class clazz = Class.forName("java.security.AccessController"); + return MethodHandles.lookup() + .findStatic( + clazz, + "doPrivileged", + MethodType.methodType(Object.class, PrivilegedExceptionAction.class)); + } catch (Exception exception) { + return null; + } + } + + static boolean canInvoke() { + return doPrivilegedMethodHandle != null; + } + + static void invoke(PrivilegedExceptionAction action) { + try { + doPrivilegedMethodHandle.invoke(action); + } catch (Throwable exception) { + throw new IllegalStateException(exception); + } + } + } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java index b14b5a41f449..0790ed17b83e 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java @@ -15,6 +15,10 @@ import java.nio.channels.Channels; import java.nio.channels.ReadableByteChannel; import java.nio.file.Files; +import java.security.AllPermission; +import java.security.CodeSource; +import java.security.PermissionCollection; +import java.security.Permissions; import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; @@ -36,12 +40,14 @@ @SuppressWarnings({"unused", "SystemOut"}) public class ExtensionClassLoader extends URLClassLoader { public static final String EXTENSIONS_CONFIG = "otel.javaagent.extensions"; + private static final Permissions ALL_PERMISSIONS = new Permissions(); // NOTE it's important not to use logging in this class, because this class is used before logging // is initialized static { ClassLoader.registerAsParallelCapable(); + ALL_PERMISSIONS.add(new AllPermission()); } public static ClassLoader getInstance(ClassLoader parent, File javaagentFile) { @@ -179,6 +185,11 @@ private static void extractFile(JarFile jarFile, JarEntry jarEntry, File outputF } } + @Override + protected PermissionCollection getPermissions(CodeSource codesource) { + return ALL_PERMISSIONS; + } + private ExtensionClassLoader(URL[] urls, ClassLoader parent) { super(urls, parent); } diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index 5f548a0d0627..e6a120d8773a 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -35,6 +35,7 @@ import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.dynamic.loading.ClassInjector; +import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; import net.bytebuddy.utility.JavaModule; /** @@ -53,6 +54,8 @@ public class HelperInjector implements Transformer { private static final TransformSafeLogger logger = TransformSafeLogger.getLogger(HelperInjector.class); + private static final ProtectionDomain PROTECTION_DOMAIN = + HelperInjector.class.getProtectionDomain(); // a hook for static instrumentation used to save additional classes created by the agent // see https://github.com/open-telemetry/opentelemetry-java-contrib/tree/main/static-instrumenter @@ -308,7 +311,8 @@ private Map> injectBootstrapClassLoader(Map inject(ClassLoader classLoader, String className) { Map> result = - new ClassInjector.UsingReflection(classLoader) + new ClassInjector.UsingReflection(classLoader, PROTECTION_DOMAIN) .injectRaw(Collections.singletonMap(className, bytes.get())); return result.get(className); } From dea02bfbda0d8507c77f31f2b9e2451e4c9ea277 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 6 Mar 2023 13:32:11 +0200 Subject: [PATCH 02/10] Fix helper class injection and context sorage init --- .../javaagent/bootstrap/AgentInitializer.java | 1 - .../javaagent/tooling/AgentStarterImpl.java | 7 +++ .../javaagent/tooling/HelperInjector.java | 50 ++++++++++++++++++- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java index 4a61c75fdcb0..91a2a0b53b3f 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java @@ -51,7 +51,6 @@ public static void initialize(Instrumentation inst, File javaagentFile, boolean }); } - @SuppressWarnings("removal") private static void execute(PrivilegedExceptionAction action) throws Exception { if (System.getSecurityManager() != null && AccessControllerInvoker.canInvoke()) { AccessControllerInvoker.invoke(action); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java index 5f329fe922dc..4545a89b8fde 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.tooling; +import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; import io.opentelemetry.instrumentation.api.internal.cache.weaklockfree.WeakConcurrentMapCleaner; import io.opentelemetry.javaagent.bootstrap.AgentInitializer; @@ -88,6 +89,12 @@ public void start() { loggingCustomizer.init(); AgentInstaller.installBytebuddyAgent(instrumentation, extensionClassLoader); WeakConcurrentMapCleaner.start(); + + // LazyStorage reads system properties. Initialize it here where we have permissions to avoid + // failing permission checks when it is initialized from user code. + if (System.getSecurityManager() != null) { + Context.current(); + } } catch (Throwable t) { // this is logged below and not rethrown to avoid logging it twice startupError = t; diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index e6a120d8773a..111bbc69f45b 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -15,8 +15,12 @@ import java.io.File; import java.io.IOException; import java.lang.instrument.Instrumentation; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.net.URL; import java.nio.file.Files; +import java.security.PrivilegedAction; import java.security.ProtectionDomain; import java.security.SecureClassLoader; import java.util.Collection; @@ -389,10 +393,52 @@ private static class HelperClassInjector { } Class inject(ClassLoader classLoader, String className) { + // if security manager is present byte buddy calls + // checkPermission(new ReflectPermission("suppressAccessChecks")) Map> result = - new ClassInjector.UsingReflection(classLoader, PROTECTION_DOMAIN) - .injectRaw(Collections.singletonMap(className, bytes.get())); + execute( + () -> + new ClassInjector.UsingReflection(classLoader, PROTECTION_DOMAIN) + .injectRaw(Collections.singletonMap(className, bytes.get()))); return result.get(className); } } + + private static T execute(PrivilegedAction action) { + if (System.getSecurityManager() != null && AccessControllerInvoker.canInvoke()) { + return AccessControllerInvoker.invoke(action); + } else { + return action.run(); + } + } + + // using java.security.AccessController directly causes build to fail due to warnings about + // using a terminally deprecated class + private static class AccessControllerInvoker { + private static final MethodHandle doPrivilegedMethodHandle = getDoPrivilegedMethodHandle(); + + private static MethodHandle getDoPrivilegedMethodHandle() { + try { + Class clazz = Class.forName("java.security.AccessController"); + return MethodHandles.lookup() + .findStatic( + clazz, "doPrivileged", MethodType.methodType(Object.class, PrivilegedAction.class)); + } catch (Exception exception) { + return null; + } + } + + static boolean canInvoke() { + return doPrivilegedMethodHandle != null; + } + + @SuppressWarnings("unchecked") + static T invoke(PrivilegedAction action) { + try { + return (T) doPrivilegedMethodHandle.invoke(action); + } catch (Throwable exception) { + throw new IllegalStateException(exception); + } + } + } } From 9e96875f42a3e760dc51885f402c5fcbda278f77 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 6 Mar 2023 18:25:13 +0200 Subject: [PATCH 03/10] Fix vm crash on early 8 jdk --- .../javaagent/bootstrap/AgentInitializer.java | 49 ++++++++++--------- .../javaagent/tooling/HelperInjector.java | 5 +- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java index 91a2a0b53b3f..9d2a198fe128 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java @@ -7,10 +7,8 @@ import java.io.File; import java.lang.instrument.Instrumentation; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; import java.lang.reflect.Constructor; +import java.lang.reflect.Method; import java.security.PrivilegedExceptionAction; import javax.annotation.Nullable; @@ -39,15 +37,19 @@ public static void initialize(Instrumentation inst, File javaagentFile, boolean throw new IllegalStateException("agent initializer should be loaded in boot loader"); } + // this call deliberately uses anonymous class instead of lambda because using lambdas too + // early on early jdk8 (see isEarlyOracle18 method) causes jvm to crash. See CrashEarlyJdk8Test. execute( - () -> { - agentClassLoader = createAgentClassLoader("inst", javaagentFile); - agentStarter = createAgentStarter(agentClassLoader, inst, javaagentFile); - if (!fromPremain || !delayAgentStart()) { - agentStarter.start(); + new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + agentClassLoader = createAgentClassLoader("inst", javaagentFile); + agentStarter = createAgentStarter(agentClassLoader, inst, javaagentFile); + if (!fromPremain || !delayAgentStart()) { + agentStarter.start(); + } + return null; } - - return null; }); } @@ -105,10 +107,15 @@ private static boolean delayAgentStart() { */ @SuppressWarnings("unused") public static void delayedStartHook() throws Exception { + // this call deliberately uses anonymous class instead of lambda because using lambdas too + // early on early jdk8 (see isEarlyOracle18 method) causes jvm to crash. See CrashEarlyJdk8Test. execute( - () -> { - agentStarter.start(); - return null; + new PrivilegedExceptionAction() { + @Override + public Void run() { + agentStarter.start(); + return null; + } }); } @@ -144,29 +151,25 @@ private AgentInitializer() {} // using java.security.AccessController directly causes build to fail due to warnings about // using a terminally deprecated class private static class AccessControllerInvoker { - private static final MethodHandle doPrivilegedMethodHandle = getDoPrivilegedMethodHandle(); + private static final Method doPrivilegedMethod = getDoPrivilegedMethod(); - private static MethodHandle getDoPrivilegedMethodHandle() { + private static Method getDoPrivilegedMethod() { try { Class clazz = Class.forName("java.security.AccessController"); - return MethodHandles.lookup() - .findStatic( - clazz, - "doPrivileged", - MethodType.methodType(Object.class, PrivilegedExceptionAction.class)); + return clazz.getMethod("doPrivileged", PrivilegedExceptionAction.class); } catch (Exception exception) { return null; } } static boolean canInvoke() { - return doPrivilegedMethodHandle != null; + return doPrivilegedMethod != null; } static void invoke(PrivilegedExceptionAction action) { try { - doPrivilegedMethodHandle.invoke(action); - } catch (Throwable exception) { + doPrivilegedMethod.invoke(null, action); + } catch (Exception exception) { throw new IllegalStateException(exception); } } diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index 111bbc69f45b..9c34bf01c283 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -39,7 +39,6 @@ import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.dynamic.loading.ClassInjector; -import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; import net.bytebuddy.utility.JavaModule; /** @@ -58,6 +57,7 @@ public class HelperInjector implements Transformer { private static final TransformSafeLogger logger = TransformSafeLogger.getLogger(HelperInjector.class); + private static final ProtectionDomain PROTECTION_DOMAIN = HelperInjector.class.getProtectionDomain(); @@ -315,8 +315,7 @@ private Map> injectBootstrapClassLoader(Map Date: Thu, 23 Mar 2023 18:24:04 +0200 Subject: [PATCH 04/10] call doPrivileged directly --- .../javaagent/bootstrap/AgentInitializer.java | 33 ++-------------- .../javaagent/tooling/HelperInjector.java | 38 ++----------------- 2 files changed, 6 insertions(+), 65 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java index 9d2a198fe128..f204572dacdd 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java @@ -8,7 +8,6 @@ import java.io.File; import java.lang.instrument.Instrumentation; import java.lang.reflect.Constructor; -import java.lang.reflect.Method; import java.security.PrivilegedExceptionAction; import javax.annotation.Nullable; @@ -53,9 +52,10 @@ public Void run() throws Exception { }); } + @SuppressWarnings({"deprecation", "removal"}) // AccessController is deprecated private static void execute(PrivilegedExceptionAction action) throws Exception { - if (System.getSecurityManager() != null && AccessControllerInvoker.canInvoke()) { - AccessControllerInvoker.invoke(action); + if (System.getSecurityManager() != null) { + java.security.AccessController.doPrivileged(action); } else { action.run(); } @@ -147,31 +147,4 @@ private static AgentStarter createAgentStarter( } private AgentInitializer() {} - - // using java.security.AccessController directly causes build to fail due to warnings about - // using a terminally deprecated class - private static class AccessControllerInvoker { - private static final Method doPrivilegedMethod = getDoPrivilegedMethod(); - - private static Method getDoPrivilegedMethod() { - try { - Class clazz = Class.forName("java.security.AccessController"); - return clazz.getMethod("doPrivileged", PrivilegedExceptionAction.class); - } catch (Exception exception) { - return null; - } - } - - static boolean canInvoke() { - return doPrivilegedMethod != null; - } - - static void invoke(PrivilegedExceptionAction action) { - try { - doPrivilegedMethod.invoke(null, action); - } catch (Exception exception) { - throw new IllegalStateException(exception); - } - } - } } diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index 9c34bf01c283..a8c5cd1a624f 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -15,9 +15,6 @@ import java.io.File; import java.io.IOException; import java.lang.instrument.Instrumentation; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; import java.net.URL; import java.nio.file.Files; import java.security.PrivilegedAction; @@ -403,41 +400,12 @@ Class inject(ClassLoader classLoader, String className) { } } + @SuppressWarnings({"deprecation", "removal"}) // AccessController is deprecated private static T execute(PrivilegedAction action) { - if (System.getSecurityManager() != null && AccessControllerInvoker.canInvoke()) { - return AccessControllerInvoker.invoke(action); + if (System.getSecurityManager() != null) { + return java.security.AccessController.doPrivileged(action); } else { return action.run(); } } - - // using java.security.AccessController directly causes build to fail due to warnings about - // using a terminally deprecated class - private static class AccessControllerInvoker { - private static final MethodHandle doPrivilegedMethodHandle = getDoPrivilegedMethodHandle(); - - private static MethodHandle getDoPrivilegedMethodHandle() { - try { - Class clazz = Class.forName("java.security.AccessController"); - return MethodHandles.lookup() - .findStatic( - clazz, "doPrivileged", MethodType.methodType(Object.class, PrivilegedAction.class)); - } catch (Exception exception) { - return null; - } - } - - static boolean canInvoke() { - return doPrivilegedMethodHandle != null; - } - - @SuppressWarnings("unchecked") - static T invoke(PrivilegedAction action) { - try { - return (T) doPrivilegedMethodHandle.invoke(action); - } catch (Throwable exception) { - throw new IllegalStateException(exception); - } - } - } } From 03dc6796a70c78d2a13e6a8a878ae8006f653623 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 23 Mar 2023 21:38:26 +0200 Subject: [PATCH 05/10] add test --- .../smoketest/SecurityManagerSmokeTest.groovy | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy diff --git a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy new file mode 100644 index 000000000000..e1bbbe2d3cb6 --- /dev/null +++ b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy @@ -0,0 +1,36 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.smoketest + +import io.opentelemetry.proto.collector.trace.v1.ExportTraceServiceRequest +import spock.lang.IgnoreIf +import spock.lang.Unroll + +import static io.opentelemetry.smoketest.TestContainerManager.useWindowsContainers + +@IgnoreIf({ useWindowsContainers() }) +class SecurityManagerSmokeTest extends SmokeTest { + + protected String getTargetImage(String jdk) { + "ghcr.io/open-telemetry/opentelemetry-java-instrumentation/smoke-test-security-manager:jdk$jdk-20230323.4502979551" + } + + @Unroll + def "security manager smoke test on JDK #jdk"(int jdk) { + setup: + startTarget(jdk) + + expect: + Collection traces = waitForTraces() + countSpansByName(traces, 'test') == 1 + + cleanup: + stopTarget() + + where: + jdk << [8, 11, 17, 19] + } +} From febd2ce79dac87fd469f6341a21592c69c8519b5 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 24 Mar 2023 11:15:55 +0200 Subject: [PATCH 06/10] Add test and a flag to enable security manager support --- .../javaagent/bootstrap/AgentClassLoader.java | 24 +++++++++--- .../javaagent/bootstrap/AgentInitializer.java | 39 +++++++++++++++++-- .../tooling/ExtensionClassLoader.java | 17 ++++++-- .../smoketest/SecurityManagerSmokeTest.groovy | 6 +++ 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java index dc2a508ad092..d656a609655a 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java @@ -19,8 +19,8 @@ import java.security.AllPermission; import java.security.CodeSource; import java.security.Permission; +import java.security.PermissionCollection; import java.security.Permissions; -import java.security.ProtectionDomain; import java.security.cert.Certificate; import java.util.Enumeration; import java.util.jar.JarEntry; @@ -66,7 +66,7 @@ public class AgentClassLoader extends URLClassLoader { private final URL jarBase; private final String jarEntryPrefix; private final CodeSource codeSource; - private final ProtectionDomain protectionDomain; + private final boolean isSecurityManagerSupportEnabled; private final Manifest manifest; /** @@ -74,8 +74,11 @@ public class AgentClassLoader extends URLClassLoader { * * @param javaagentFile Used for resource lookups. * @param internalJarFileName File name of the internal jar + * @param isSecurityManagerSupportEnabled Whether this class loader should define classes with all + * permissions */ - public AgentClassLoader(File javaagentFile, String internalJarFileName) { + public AgentClassLoader( + File javaagentFile, String internalJarFileName, boolean isSecurityManagerSupportEnabled) { super(new URL[] {}, getParentClassLoader()); if (javaagentFile == null) { throw new IllegalArgumentException("Agent jar location should be set"); @@ -84,6 +87,7 @@ public AgentClassLoader(File javaagentFile, String internalJarFileName) { throw new IllegalArgumentException("Internal jar file name should be set"); } + this.isSecurityManagerSupportEnabled = isSecurityManagerSupportEnabled; bootstrapProxy = new BootstrapClassLoaderProxy(this); jarEntryPrefix = @@ -99,7 +103,6 @@ public AgentClassLoader(File javaagentFile, String internalJarFileName) { codeSource = new CodeSource(javaagentFile.toURI().toURL(), (Certificate[]) null); Permissions permissions = new Permissions(); permissions.add(new AllPermission()); - protectionDomain = new ProtectionDomain(codeSource, permissions); manifest = jarFile.getManifest(); } catch (IOException e) { throw new IllegalStateException("Unable to open agent jar", e); @@ -180,7 +183,18 @@ private Class findAgentClass(String name) throws ClassNotFoundException { } public Class defineClass(String name, byte[] bytes) { - return defineClass(name, bytes, 0, bytes.length, protectionDomain); + return defineClass(name, bytes, 0, bytes.length, codeSource); + } + + @Override + protected PermissionCollection getPermissions(CodeSource codeSource) { + if (isSecurityManagerSupportEnabled) { + Permissions permissions = new Permissions(); + permissions.add(new AllPermission()); + return permissions; + } + + return super.getPermissions(codeSource); } private byte[] getJarEntryBytes(JarEntry jarEntry) throws IOException { diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java index f204572dacdd..25a7ed584dd1 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java @@ -5,9 +5,11 @@ package io.opentelemetry.javaagent.bootstrap; +import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil; import java.io.File; import java.lang.instrument.Instrumentation; import java.lang.reflect.Constructor; +import java.security.PrivilegedAction; import java.security.PrivilegedExceptionAction; import javax.annotation.Nullable; @@ -23,6 +25,7 @@ public final class AgentInitializer { @Nullable private static ClassLoader agentClassLoader = null; @Nullable private static AgentStarter agentStarter = null; + private static boolean isSecurityManagerSupportEnabled = false; public static void initialize(Instrumentation inst, File javaagentFile, boolean fromPremain) throws Exception { @@ -36,6 +39,8 @@ public static void initialize(Instrumentation inst, File javaagentFile, boolean throw new IllegalStateException("agent initializer should be loaded in boot loader"); } + isSecurityManagerSupportEnabled = isSecurityManagerSupportEnabled(); + // this call deliberately uses anonymous class instead of lambda because using lambdas too // early on early jdk8 (see isEarlyOracle18 method) causes jvm to crash. See CrashEarlyJdk8Test. execute( @@ -52,15 +57,41 @@ public Void run() throws Exception { }); } - @SuppressWarnings({"deprecation", "removal"}) // AccessController is deprecated private static void execute(PrivilegedExceptionAction action) throws Exception { - if (System.getSecurityManager() != null) { - java.security.AccessController.doPrivileged(action); + if (isSecurityManagerSupportEnabled && System.getSecurityManager() != null) { + doPrivilegedExceptionAction(action); } else { action.run(); } } + private static boolean isSecurityManagerSupportEnabled() { + return getBoolean("otel.javaagent.experimental.security-manager.enabled", false); + } + + private static boolean getBoolean(String property, boolean defaultValue) { + // this call deliberately uses anonymous class instead of lambda because using lambdas too + // early on early jdk8 (see isEarlyOracle18 method) causes jvm to crash. See CrashEarlyJdk8Test. + return doPrivileged( + new PrivilegedAction() { + @Override + public Boolean run() { + return ConfigPropertiesUtil.getBoolean(property, defaultValue); + } + }); + } + + @SuppressWarnings({"deprecation", "removal"}) // AccessController is deprecated + private static T doPrivilegedExceptionAction(PrivilegedExceptionAction action) + throws Exception { + return java.security.AccessController.doPrivileged(action); + } + + @SuppressWarnings({"deprecation", "removal"}) // AccessController is deprecated + private static T doPrivileged(PrivilegedAction action) { + return java.security.AccessController.doPrivileged(action); + } + /** * Test whether we are running on oracle 1.8 before 1.8.0_40. * @@ -133,7 +164,7 @@ public static ClassLoader getExtensionsClassLoader() { * @return Agent Classloader */ private static ClassLoader createAgentClassLoader(String innerJarFilename, File javaagentFile) { - return new AgentClassLoader(javaagentFile, innerJarFilename); + return new AgentClassLoader(javaagentFile, innerJarFilename, isSecurityManagerSupportEnabled); } private static AgentStarter createAgentStarter( diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java index 0790ed17b83e..181a7a9ef86a 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java @@ -40,14 +40,20 @@ @SuppressWarnings({"unused", "SystemOut"}) public class ExtensionClassLoader extends URLClassLoader { public static final String EXTENSIONS_CONFIG = "otel.javaagent.extensions"; - private static final Permissions ALL_PERMISSIONS = new Permissions(); + // if this class was defined with all permissions then also define classes in this class loader + // with all permissions + // this class is defined with all permissions when security manager support is enabled + private static final boolean addAllPermissions = + ExtensionClassLoader.class + .getProtectionDomain() + .getPermissions() + .implies(new AllPermission()); // NOTE it's important not to use logging in this class, because this class is used before logging // is initialized static { ClassLoader.registerAsParallelCapable(); - ALL_PERMISSIONS.add(new AllPermission()); } public static ClassLoader getInstance(ClassLoader parent, File javaagentFile) { @@ -187,7 +193,12 @@ private static void extractFile(JarFile jarFile, JarEntry jarEntry, File outputF @Override protected PermissionCollection getPermissions(CodeSource codesource) { - return ALL_PERMISSIONS; + if (addAllPermissions) { + Permissions permissions = new Permissions(); + permissions.add(new AllPermission()); + return permissions; + } + return super.getPermissions(codesource); } private ExtensionClassLoader(URL[] urls, ClassLoader parent) { diff --git a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy index e1bbbe2d3cb6..746da3567a06 100644 --- a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy +++ b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy @@ -14,10 +14,16 @@ import static io.opentelemetry.smoketest.TestContainerManager.useWindowsContaine @IgnoreIf({ useWindowsContainers() }) class SecurityManagerSmokeTest extends SmokeTest { + @Override protected String getTargetImage(String jdk) { "ghcr.io/open-telemetry/opentelemetry-java-instrumentation/smoke-test-security-manager:jdk$jdk-20230323.4502979551" } + @Override + protected Map getExtraEnv() { + return Collections.singletonMap("OTEL_JAVAAGENT_EXPERIMENTAL_SECURITY_MANAGER_ENABLED", "true") + } + @Unroll def "security manager smoke test on JDK #jdk"(int jdk) { setup: From e361665742800b88dab41972d860872bcceff6fa Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 24 Mar 2023 12:22:34 +0200 Subject: [PATCH 07/10] Fix tests --- .../opentelemetry/javaagent/bootstrap/AgentClassLoader.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java index d656a609655a..b03e6b31f219 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java @@ -69,6 +69,10 @@ public class AgentClassLoader extends URLClassLoader { private final boolean isSecurityManagerSupportEnabled; private final Manifest manifest; + public AgentClassLoader(File javaagentFile, String internalJarFileName) { + this(javaagentFile, internalJarFileName, false); + } + /** * Construct a new AgentClassLoader. * From 13555af6ccba1f32ca3caf4fd7eef41961a29e80 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 3 Apr 2023 18:01:27 +0300 Subject: [PATCH 08/10] addess review comments --- .../javaagent/OpenTelemetryAgent.java | 13 +++++---- .../javaagent/bootstrap/AgentClassLoader.java | 2 -- .../javaagent/bootstrap/AgentInitializer.java | 7 +++-- .../javaagent/tooling/AgentStarterImpl.java | 15 ++++++---- .../tooling/ExtensionClassLoader.java | 28 +++++++++---------- .../javaagent/tooling/HelperInjector.java | 3 +- .../smoketest/SecurityManagerSmokeTest.groovy | 2 +- 7 files changed, 38 insertions(+), 32 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java index eb66597d0c61..eb146b2d09f4 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/OpenTelemetryAgent.java @@ -11,7 +11,6 @@ import java.io.File; import java.io.IOException; import java.lang.instrument.Instrumentation; -import java.net.JarURLConnection; import java.net.URISyntaxException; import java.net.URL; import java.util.jar.JarFile; @@ -67,10 +66,12 @@ private static synchronized File installBootstrapJar(Instrumentation inst) throws IOException, URISyntaxException { // we are not using OpenTelemetryAgent.class.getProtectionDomain().getCodeSource() to get agent // location because getProtectionDomain does a permission check with security manager + ClassLoader classLoader = OpenTelemetryAgent.class.getClassLoader(); + if (classLoader == null) { + classLoader = ClassLoader.getSystemClassLoader(); + } URL url = - OpenTelemetryAgent.class - .getClassLoader() - .getResource(OpenTelemetryAgent.class.getName().replace('.', '/') + ".class"); + classLoader.getResource(OpenTelemetryAgent.class.getName().replace('.', '/') + ".class"); if (url == null || !"jar".equals(url.getProtocol())) { throw new IllegalStateException("could not get agent jar location from url " + url); } @@ -88,7 +89,9 @@ private static synchronized File installBootstrapJar(Instrumentation inst) "agent jar location doesn't appear to be a file: " + javaagentFile.getAbsolutePath()); } - JarFile agentJar = ((JarURLConnection) url.openConnection()).getJarFile(); + // verification is very slow before the JIT compiler starts up, which on Java 8 is not until + // after premain execution completes + JarFile agentJar = new JarFile(javaagentFile, false); verifyJarManifestMainClassIsThis(javaagentFile, agentJar); inst.appendToBootstrapClassLoaderSearch(agentJar); return javaagentFile; diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java index b03e6b31f219..d936071667ff 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java @@ -105,8 +105,6 @@ public AgentClassLoader( jarBase = new URL("x-internal-jar", null, 0, "/", new AgentClassLoaderUrlStreamHandler(jarFile)); codeSource = new CodeSource(javaagentFile.toURI().toURL(), (Certificate[]) null); - Permissions permissions = new Permissions(); - permissions.add(new AllPermission()); manifest = jarFile.getManifest(); } catch (IOException e) { throw new IllegalStateException("Unable to open agent jar", e); diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java index 25a7ed584dd1..4c7321415fbf 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java @@ -66,7 +66,7 @@ private static void execute(PrivilegedExceptionAction action) throws Excep } private static boolean isSecurityManagerSupportEnabled() { - return getBoolean("otel.javaagent.experimental.security-manager.enabled", false); + return getBoolean("otel.javaagent.experimental.security-manager-support.enabled", false); } private static boolean getBoolean(String property, boolean defaultValue) { @@ -173,8 +173,9 @@ private static AgentStarter createAgentStarter( Class starterClass = agentClassLoader.loadClass("io.opentelemetry.javaagent.tooling.AgentStarterImpl"); Constructor constructor = - starterClass.getDeclaredConstructor(Instrumentation.class, File.class); - return (AgentStarter) constructor.newInstance(instrumentation, javaagentFile); + starterClass.getDeclaredConstructor(Instrumentation.class, File.class, boolean.class); + return (AgentStarter) + constructor.newInstance(instrumentation, javaagentFile, isSecurityManagerSupportEnabled); } private AgentInitializer() {} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java index 4545a89b8fde..ed64b5bcf003 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java @@ -30,11 +30,16 @@ public class AgentStarterImpl implements AgentStarter { private final Instrumentation instrumentation; private final File javaagentFile; + private final boolean isSecurityManagerSupportEnabled; private ClassLoader extensionClassLoader; - public AgentStarterImpl(Instrumentation instrumentation, File javaagentFile) { + public AgentStarterImpl( + Instrumentation instrumentation, + File javaagentFile, + boolean isSecurityManagerSupportEnabled) { this.instrumentation = instrumentation; this.javaagentFile = javaagentFile; + this.isSecurityManagerSupportEnabled = isSecurityManagerSupportEnabled; } @Override @@ -62,7 +67,7 @@ public boolean delayStart() { @Override public void start() { - extensionClassLoader = createExtensionClassLoader(getClass().getClassLoader(), javaagentFile); + extensionClassLoader = createExtensionClassLoader(getClass().getClassLoader()); String loggerImplementationName = ConfigPropertiesUtil.getString("otel.javaagent.logging"); // default to the built-in stderr slf4j-simple logger @@ -119,9 +124,9 @@ public ClassLoader getExtensionClassLoader() { return extensionClassLoader; } - private static ClassLoader createExtensionClassLoader( - ClassLoader agentClassLoader, File javaagentFile) { - return ExtensionClassLoader.getInstance(agentClassLoader, javaagentFile); + private ClassLoader createExtensionClassLoader(ClassLoader agentClassLoader) { + return ExtensionClassLoader.getInstance( + agentClassLoader, javaagentFile, isSecurityManagerSupportEnabled); } private static class LaunchHelperClassFileTransformer implements ClassFileTransformer { diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java index 181a7a9ef86a..5cbdef8313f3 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java @@ -40,14 +40,8 @@ @SuppressWarnings({"unused", "SystemOut"}) public class ExtensionClassLoader extends URLClassLoader { public static final String EXTENSIONS_CONFIG = "otel.javaagent.extensions"; - // if this class was defined with all permissions then also define classes in this class loader - // with all permissions - // this class is defined with all permissions when security manager support is enabled - private static final boolean addAllPermissions = - ExtensionClassLoader.class - .getProtectionDomain() - .getPermissions() - .implies(new AllPermission()); + + private final boolean isSecurityManagerSupportEnabled; // NOTE it's important not to use logging in this class, because this class is used before logging // is initialized @@ -56,7 +50,8 @@ public class ExtensionClassLoader extends URLClassLoader { ClassLoader.registerAsParallelCapable(); } - public static ClassLoader getInstance(ClassLoader parent, File javaagentFile) { + public static ClassLoader getInstance( + ClassLoader parent, File javaagentFile, boolean isSecurityManagerSupportEnabled) { List extensions = new ArrayList<>(); includeEmbeddedExtensionsIfFound(parent, extensions, javaagentFile); @@ -81,7 +76,7 @@ public static ClassLoader getInstance(ClassLoader parent, File javaagentFile) { List delegates = new ArrayList<>(extensions.size()); for (URL url : extensions) { - delegates.add(getDelegate(parent, url)); + delegates.add(getDelegate(parent, url, isSecurityManagerSupportEnabled)); } return new MultipleParentClassLoader(parent, delegates); } @@ -131,8 +126,9 @@ private static File ensureTempDirectoryExists(File tempDirectory) throws IOExcep return tempDirectory; } - private static URLClassLoader getDelegate(ClassLoader parent, URL extensionUrl) { - return new ExtensionClassLoader(new URL[] {extensionUrl}, parent); + private static URLClassLoader getDelegate( + ClassLoader parent, URL extensionUrl, boolean isSecurityManagerSupportEnabled) { + return new ExtensionClassLoader(extensionUrl, parent, isSecurityManagerSupportEnabled); } // visible for testing @@ -193,7 +189,7 @@ private static void extractFile(JarFile jarFile, JarEntry jarEntry, File outputF @Override protected PermissionCollection getPermissions(CodeSource codesource) { - if (addAllPermissions) { + if (isSecurityManagerSupportEnabled) { Permissions permissions = new Permissions(); permissions.add(new AllPermission()); return permissions; @@ -201,7 +197,9 @@ protected PermissionCollection getPermissions(CodeSource codesource) { return super.getPermissions(codesource); } - private ExtensionClassLoader(URL[] urls, ClassLoader parent) { - super(urls, parent); + private ExtensionClassLoader( + URL url, ClassLoader parent, boolean isSecurityManagerSupportEnabled) { + super(new URL[] {url}, parent); + this.isSecurityManagerSupportEnabled = isSecurityManagerSupportEnabled; } } diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index a8c5cd1a624f..b8ce7cb5c9e5 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -390,7 +390,8 @@ private static class HelperClassInjector { Class inject(ClassLoader classLoader, String className) { // if security manager is present byte buddy calls - // checkPermission(new ReflectPermission("suppressAccessChecks")) + // checkPermission(new ReflectPermission("suppressAccessChecks")) so we must call class + // injection with AccessController.doPrivileged when security manager is enabled Map> result = execute( () -> diff --git a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy index 746da3567a06..982dcdbfc057 100644 --- a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy +++ b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy @@ -21,7 +21,7 @@ class SecurityManagerSmokeTest extends SmokeTest { @Override protected Map getExtraEnv() { - return Collections.singletonMap("OTEL_JAVAAGENT_EXPERIMENTAL_SECURITY_MANAGER_ENABLED", "true") + return Collections.singletonMap("OTEL_JAVAAGENT_EXPERIMENTAL_SECURITY_MANAGER_SUPPORT_ENABLED", "true") } @Unroll From 7a9e2d9876986a0d98227941d51a382970527cc5 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 4 Apr 2023 15:37:52 +0300 Subject: [PATCH 09/10] Add comment that constructor is used by tests --- .../opentelemetry/javaagent/bootstrap/AgentClassLoader.java | 5 +++-- .../javaagent/bootstrap/AgentClassLoaderTest.groovy | 4 ++-- .../src/test/groovy/UnsafeTest.groovy | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java index d936071667ff..35f9079ab6d2 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentClassLoader.java @@ -69,8 +69,9 @@ public class AgentClassLoader extends URLClassLoader { private final boolean isSecurityManagerSupportEnabled; private final Manifest manifest; - public AgentClassLoader(File javaagentFile, String internalJarFileName) { - this(javaagentFile, internalJarFileName, false); + // Used by tests + public AgentClassLoader(File javaagentFile) { + this(javaagentFile, "", false); } /** diff --git a/javaagent-bootstrap/src/test/groovy/io/opentelemetry/javaagent/bootstrap/AgentClassLoaderTest.groovy b/javaagent-bootstrap/src/test/groovy/io/opentelemetry/javaagent/bootstrap/AgentClassLoaderTest.groovy index c837f7445d2a..da7193edc3b0 100644 --- a/javaagent-bootstrap/src/test/groovy/io/opentelemetry/javaagent/bootstrap/AgentClassLoaderTest.groovy +++ b/javaagent-bootstrap/src/test/groovy/io/opentelemetry/javaagent/bootstrap/AgentClassLoaderTest.groovy @@ -19,7 +19,7 @@ class AgentClassLoaderTest extends Specification { def className2 = 'some/class/Name2' // any jar would do, use opentelemety sdk URL testJarLocation = JavaVersionSpecific.getProtectionDomain().getCodeSource().getLocation() - AgentClassLoader loader = new AgentClassLoader(new File(testJarLocation.toURI()), "") + AgentClassLoader loader = new AgentClassLoader(new File(testJarLocation.toURI())) Phaser threadHoldLockPhase = new Phaser(2) Phaser acquireLockFromMainThreadPhase = new Phaser(2) @@ -58,7 +58,7 @@ class AgentClassLoaderTest extends Specification { boolean jdk8 = "1.8" == System.getProperty("java.specification.version") // sdk is a multi release jar URL multiReleaseJar = JavaVersionSpecific.getProtectionDomain().getCodeSource().getLocation() - AgentClassLoader loader = new AgentClassLoader(new File(multiReleaseJar.toURI()), "") { + AgentClassLoader loader = new AgentClassLoader(new File(multiReleaseJar.toURI())) { @Override protected String getClassSuffix() { return "" diff --git a/javaagent-tooling/javaagent-tooling-java9/src/test/groovy/UnsafeTest.groovy b/javaagent-tooling/javaagent-tooling-java9/src/test/groovy/UnsafeTest.groovy index e1d2d47cd9c8..671be3c55d5c 100644 --- a/javaagent-tooling/javaagent-tooling-java9/src/test/groovy/UnsafeTest.groovy +++ b/javaagent-tooling/javaagent-tooling-java9/src/test/groovy/UnsafeTest.groovy @@ -14,7 +14,7 @@ class UnsafeTest extends Specification { setup: ByteBuddyAgent.install() URL testJarLocation = AgentClassLoader.getProtectionDomain().getCodeSource().getLocation() - AgentClassLoader loader = new AgentClassLoader(new File(testJarLocation.toURI()), "") + AgentClassLoader loader = new AgentClassLoader(new File(testJarLocation.toURI())) UnsafeInitializer.initialize(ByteBuddyAgent.getInstrumentation(), loader, false) expect: From d4df9a9454a620be95a39548b1d6b0b81e7c7772 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 4 Apr 2023 15:38:32 +0300 Subject: [PATCH 10/10] Document security-manager-support flag --- docs/advanced-configuration-options.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/advanced-configuration-options.md b/docs/advanced-configuration-options.md index 5f7066f98cb7..959f4e87bb4d 100644 --- a/docs/advanced-configuration-options.md +++ b/docs/advanced-configuration-options.md @@ -17,3 +17,14 @@ which could have unknown side-effects. | System property | Environment variable | Purpose | |--------------------------------|--------------------------------|---------------------------------------------------------------------------------------------------| | otel.javaagent.exclude-classes | OTEL_JAVAAGENT_EXCLUDE_CLASSES | Suppresses all instrumentation for specific classes, format is "my.package.MyClass,my.package2.*" | + +## Running application with security manager + +This option can be used to let agent run with all privileges without being affected by security policy restricting some operations. + +| System property | Environment variable | Purpose | +|--------------------------------------------------------------|--------------------------------------------------------------|---------------------------------------| +| otel.javaagent.experimental.security-manager-support.enabled | OTEL_JAVAAGENT_EXPERIMENTAL_SECURITY_MANAGER_SUPPORT_ENABLED | Grant all privileges to agent code[1] | + +[1] Disclaimer: agent can provide application means for escaping security manager sandbox. Do not use +this option if your application relies on security manager to run untrusted code.