From f8078e2098dbd803ddf5da1dd35b79a54715f009 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 1 Apr 2022 11:39:47 +0200 Subject: [PATCH 1/5] wrap in doPrivileged for logger init --- .../apm/agent/sdk/logging/LoggerFactory.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java index c61257d21e..42d8ac6864 100644 --- a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java +++ b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java @@ -18,6 +18,9 @@ */ package co.elastic.apm.agent.sdk.logging; +import java.security.AccessController; +import java.security.PrivilegedAction; + public class LoggerFactory { private static volatile ILoggerFactory iLoggerFactory; @@ -32,11 +35,19 @@ public static void initialize(ILoggerFactory iLoggerFactory) { * @param name The name of the logger. * @return logger */ - public static Logger getLogger(String name) { + public static Logger getLogger(final String name) { if (iLoggerFactory == null) { return NoopLogger.INSTANCE; } - return iLoggerFactory.getLogger(name); + + // we have to wrap into a doPrivileged call because the logging initialization often requirest + // elevated privileges, for example to access the classloader when running with a security manager. + return AccessController.doPrivileged(new PrivilegedAction() { + @Override + public Logger run() { + return iLoggerFactory.getLogger(name); + } + }); } /** From 75b5671291d26c22110de6f829079d83096e3a1d Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 1 Apr 2022 11:53:55 +0200 Subject: [PATCH 2/5] try wrapping at lower level --- .../agent/logging/Log4jLoggerFactoryBridge.java | 15 +++++++++++++++ .../apm/agent/sdk/logging/LoggerFactory.java | 12 +----------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java index fe338a33a0..c0372ac743 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java @@ -27,6 +27,9 @@ import org.apache.logging.log4j.spi.LoggerContextFactory; import org.apache.logging.log4j.util.StackLocatorUtil; +import java.security.AccessController; +import java.security.PrivilegedAction; + /** * Based on {@code org.apache.logging.slf4j.Log4jLoggerFactory} */ @@ -35,6 +38,18 @@ public class Log4jLoggerFactoryBridge extends AbstractLoggerAdapter impl private static final String FQCN = Log4jLoggerFactoryBridge.class.getName(); private static final String PACKAGE = "co.elastic.apm.agent.sdk.logging"; + @Override + public Logger getLogger(final String name) { + // we have to wrap into a doPrivileged call because the logging initialization often requires + // elevated privileges, for example to access the classloader when running with a security manager. + return AccessController.doPrivileged(new PrivilegedAction() { + @Override + public Logger run() { + return Log4jLoggerFactoryBridge.super.getLogger(name); + } + }); + } + public static void shutdown() { LoggerContextFactory factory = LogManager.getFactory(); // the Spring tests use log4j-to-slf4j which uses SLF4JLoggerContextFactory which does not need cleanup diff --git a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java index 42d8ac6864..20df339a15 100644 --- a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java +++ b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java @@ -18,9 +18,6 @@ */ package co.elastic.apm.agent.sdk.logging; -import java.security.AccessController; -import java.security.PrivilegedAction; - public class LoggerFactory { private static volatile ILoggerFactory iLoggerFactory; @@ -40,14 +37,7 @@ public static Logger getLogger(final String name) { return NoopLogger.INSTANCE; } - // we have to wrap into a doPrivileged call because the logging initialization often requirest - // elevated privileges, for example to access the classloader when running with a security manager. - return AccessController.doPrivileged(new PrivilegedAction() { - @Override - public Logger run() { - return iLoggerFactory.getLogger(name); - } - }); + return iLoggerFactory.getLogger(name); } /** From 506892ab407c0a537012e9108f8b714ffae9cc86 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 1 Apr 2022 12:14:23 +0200 Subject: [PATCH 3/5] moving back to where it was --- .../agent/logging/Log4jLoggerFactoryBridge.java | 16 ++++------------ .../apm/agent/sdk/logging/LoggerFactory.java | 11 +++++++++-- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java index c0372ac743..61066d6f86 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java @@ -27,9 +27,6 @@ import org.apache.logging.log4j.spi.LoggerContextFactory; import org.apache.logging.log4j.util.StackLocatorUtil; -import java.security.AccessController; -import java.security.PrivilegedAction; - /** * Based on {@code org.apache.logging.slf4j.Log4jLoggerFactory} */ @@ -39,15 +36,10 @@ public class Log4jLoggerFactoryBridge extends AbstractLoggerAdapter impl private static final String PACKAGE = "co.elastic.apm.agent.sdk.logging"; @Override - public Logger getLogger(final String name) { - // we have to wrap into a doPrivileged call because the logging initialization often requires - // elevated privileges, for example to access the classloader when running with a security manager. - return AccessController.doPrivileged(new PrivilegedAction() { - @Override - public Logger run() { - return Log4jLoggerFactoryBridge.super.getLogger(name); - } - }); + public Logger getLogger(String name) { + // parent class AbstractLoggerAdapter already 'implements' the method because it has the same signature + // this provides an actual implementation of ILoggerFactory while delegating explicitly to the parent impl. + return super.getLogger(name); } public static void shutdown() { diff --git a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java index 20df339a15..493e78f3e9 100644 --- a/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java +++ b/apm-agent-plugin-sdk/src/main/java/co/elastic/apm/agent/sdk/logging/LoggerFactory.java @@ -18,6 +18,9 @@ */ package co.elastic.apm.agent.sdk.logging; +import java.security.AccessController; +import java.security.PrivilegedAction; + public class LoggerFactory { private static volatile ILoggerFactory iLoggerFactory; @@ -36,8 +39,12 @@ public static Logger getLogger(final String name) { if (iLoggerFactory == null) { return NoopLogger.INSTANCE; } - - return iLoggerFactory.getLogger(name); + return AccessController.doPrivileged(new PrivilegedAction() { + @Override + public Logger run() { + return iLoggerFactory.getLogger(name); + } + }); } /** From beff3b4222b1d3d0785e85e2a68147ee87f3bebd Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 1 Apr 2022 16:38:05 +0200 Subject: [PATCH 4/5] fix the heuristic to get caller class for log init --- .../logging/Log4jLoggerFactoryBridge.java | 25 ++++++++++--------- .../logging/LoggingConfigurationTest.java | 11 +++++--- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java index 61066d6f86..b569d75901 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/logging/Log4jLoggerFactoryBridge.java @@ -28,19 +28,15 @@ import org.apache.logging.log4j.util.StackLocatorUtil; /** - * Based on {@code org.apache.logging.slf4j.Log4jLoggerFactory} + * Based on {@code org.apache.logging.slf4j.Log4jLoggerFactory}. + *

+ * This class does not implement {@link ILoggerFactory} directly but through the super class implementation that has + * a method matching {@link ILoggerFactory#getLogger(String)}. Given this method is caller-sensitive then we can't + * implement it directly without side effects. */ public class Log4jLoggerFactoryBridge extends AbstractLoggerAdapter implements ILoggerFactory { - private static final String FQCN = Log4jLoggerFactoryBridge.class.getName(); - private static final String PACKAGE = "co.elastic.apm.agent.sdk.logging"; - - @Override - public Logger getLogger(String name) { - // parent class AbstractLoggerAdapter already 'implements' the method because it has the same signature - // this provides an actual implementation of ILoggerFactory while delegating explicitly to the parent impl. - return super.getLogger(name); - } + private static final String LOGGER_FACTORY = "co.elastic.apm.agent.sdk.logging.LoggerFactory"; public static void shutdown() { LoggerContextFactory factory = LogManager.getFactory(); @@ -60,7 +56,12 @@ protected Logger newLogger(final String name, final LoggerContext context) { @Override protected LoggerContext getContext() { - final Class anchor = StackLocatorUtil.getCallerClass(FQCN, PACKAGE); - return anchor == null ? LogManager.getContext() : getContext(StackLocatorUtil.getCallerClass(anchor)); + // the logger context is defined by the class that calls LoggerFactory + final Class factoryCaller = StackLocatorUtil.getCallerClass(LOGGER_FACTORY); + if (factoryCaller == null) { + return LogManager.getContext(); + } + return getContext(factoryCaller); } + } diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/logging/LoggingConfigurationTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/logging/LoggingConfigurationTest.java index 4ccb208435..0811579f42 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/logging/LoggingConfigurationTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/logging/LoggingConfigurationTest.java @@ -136,11 +136,16 @@ void loggingLevelChangeTest() throws IOException { // stagemonitor relies on slf4j, which should be bridged to the same log4j registries and get the same configuration assertThat(configOptionLogger.isTraceEnabled()).isFalse(); - assertThat(testLog4jContextFactory.getContext(configOptionLogger.getName())).isEqualTo(agentLoggerContext); + assertThat(testLog4jContextFactory.getContext(configOptionLogger.getName())) + .describedAs("configuration logger context should be the same as the agent logger context") + .isEqualTo(agentLoggerContext); LoggerContext pluginLoggerContext = testLog4jContextFactory.getContext(pluginLogger.getName()); assertThat(pluginLoggerContext).isNotNull(); - assertThat(pluginLoggerContext).isNotEqualTo(agentLoggerContext); + assertThat(pluginLoggerContext) + .describedAs("plugin logger context should be distinct from agent logger context") + .isNotEqualTo(agentLoggerContext); + assertThat(pluginLoggerContext.getName()).startsWith(IndyPluginClassLoader.class.getName()); assertThat(pluginLogger.isTraceEnabled()).isFalse(); @@ -231,7 +236,7 @@ private static class TestContextSelector implements ContextSelector { private final Map contextMap = new HashMap<>(); @Override - public LoggerContext getContext(String fqcn, ClassLoader loader, boolean currentContext) { + public LoggerContext getContext(String fqcn, @Nullable ClassLoader loader, boolean currentContext) { if (loader == null) { loader = ClassLoader.getSystemClassLoader(); } From 713be1ffa18eae8f9226c36f52d867f1aada9ba4 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 1 Apr 2022 17:12:07 +0200 Subject: [PATCH 5/5] update changelog --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 1ce3981324..3b10cf3fcf 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -29,6 +29,7 @@ endif::[] * Fixed service name discovery based on MANIFEST.MF file through `ServletContainerInitializer#onStartup` on Jakarta Servlet containers - {pull}2546[#2546] * Fix shaded classloader package definition - {pull}2566[#2566] +* Fix logging initialization with Security Manager - {pull}2568[#2568] [[release-notes-1.x]] === Java Agent version 1.x