addess review comments

Author: laurit

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;

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);

javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java

@@ -66,7 +66,7 @@ private static void execute(PrivilegedExceptionAction<Void> 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() {}

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());
 
     Iterator<LoggingCustomizer> loggingCustomizers =
         ServiceLoader.load(LoggingCustomizer.class, extensionClassLoader).iterator();
@@ -100,9 +105,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 {

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<URL> extensions = new ArrayList<>();
 
     includeEmbeddedExtensionsIfFound(parent, extensions, javaagentFile);
@@ -81,7 +76,7 @@ public static ClassLoader getInstance(ClassLoader parent, File javaagentFile) {
 
     List<ClassLoader> 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);
   }
@@ -132,8 +127,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
@@ -194,15 +190,17 @@ 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;
     }
     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;
   }
 }

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<String, Class<?>> result =
           execute(
               () ->

smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SecurityManagerSmokeTest.groovy

@@ -21,7 +21,7 @@ class SecurityManagerSmokeTest extends SmokeTest {
 
   @Override
   protected Map<String, String> getExtraEnv() {
-    return Collections.singletonMap("OTEL_JAVAAGENT_EXPERIMENTAL_SECURITY_MANAGER_ENABLED", "true")
+    return Collections.singletonMap("OTEL_JAVAAGENT_EXPERIMENTAL_SECURITY_MANAGER_SUPPORT_ENABLED", "true")
   }
 
   @Unroll