From 8a8ebc7d71d635075891cca67796444357d392b8 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Thu, 27 Jan 2022 18:06:02 +0100 Subject: [PATCH 1/5] Always use JDK default URL handler --- .../apm/agent/report/ApmServerClient.java | 11 +++-- .../elastic/apm/agent/report/HttpUtils.java | 34 ++++++++++++++++ .../apm/agent/report/HttpUtilsTest.java | 40 +++++++++++++++++++ .../apm/agent/report/fake/http/Handler.java | 14 +++++++ .../apm/agent/report/fake/https/Handler.java | 5 +++ 5 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java create mode 100644 apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java index 2679d269d1..268b72fad3 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java @@ -20,11 +20,11 @@ import co.elastic.apm.agent.configuration.CoreConfiguration; import co.elastic.apm.agent.report.ssl.SslUtils; +import co.elastic.apm.agent.sdk.logging.Logger; +import co.elastic.apm.agent.sdk.logging.LoggerFactory; import co.elastic.apm.agent.util.UrlConnectionUtils; import co.elastic.apm.agent.util.Version; import co.elastic.apm.agent.util.VersionUtils; -import co.elastic.apm.agent.sdk.logging.Logger; -import co.elastic.apm.agent.sdk.logging.LoggerFactory; import org.stagemonitor.configuration.ConfigurationOption; import javax.annotation.Nonnull; @@ -112,7 +112,11 @@ public void onChange(ConfigurationOption configurationOption, List oldVa } private void setServerUrls(List serverUrls) { - this.serverUrls = serverUrls; + List newServersUrls = new ArrayList<>(); + for (URL url : serverUrls) { + newServersUrls.add(HttpUtils.withDefaultHandler(url)); + } + this.serverUrls = newServersUrls; this.apmServerVersion = healthChecker.checkHealthAndGetMinVersion(); this.errorCount.set(0); } @@ -410,6 +414,7 @@ private static String getUserAgent(CoreConfiguration coreConfiguration) { /** * Escapes the provided string from characters that are disallowed within HTTP header comments. * See spec- https://httpwg.org/specs/rfc7230.html#field.components + * * @param headerFieldComment HTTP header comment value to be escaped * @return the escaped header comment */ diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java index b34aeb24c3..bdbce8e350 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java @@ -25,7 +25,11 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.lang.reflect.InvocationTargetException; import java.net.HttpURLConnection; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLStreamHandler; public class HttpUtils { @@ -65,4 +69,34 @@ public static void consumeAndClose(@Nullable HttpURLConnection connection) { } } } + + /** + * Rebuilds the provided URL with the default handler, as it may have been overriden by an application server at + * runtime and might prevent the agent from properly communicate through HTTP/HTTPS. + * + * @param url URL to rewrite + * @return equivalent URL with the default JDK handler + * @throws IllegalArgumentException if protocol is not supported or unable to access default handler + */ + public static URL withDefaultHandler(URL url) { + // the default handler for URLs might be overridden by another implementation than the one shipped with the JDK + // for example, this happens on Weblogic application server and triggers classloading issues as the agent + // is unable to properly use the Weblogic classes as they aren't visible to the agent classloaders. + + String protocol = url.getProtocol(); + try { + return new URL(protocol, url.getHost(), url.getPort(), url.getFile(), handlerForProtocol(protocol)); + } catch (MalformedURLException e) { + throw new IllegalArgumentException(e); + } + } + + private static URLStreamHandler handlerForProtocol(String protocol) { + try { + Class handlerClass = Class.forName(String.format("sun.net.www.protocol.%s.Handler", protocol)); + return (URLStreamHandler) handlerClass.getDeclaredConstructor().newInstance(); + } catch (ClassNotFoundException | NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) { + throw new IllegalArgumentException("unable to retrieve handler for protocol : " + protocol); + } + } } diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java index d5fcb3c0aa..69bd3e131c 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java @@ -19,11 +19,17 @@ package co.elastic.apm.agent.report; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; +import java.net.URL; +import java.net.UnknownHostException; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -82,4 +88,38 @@ private static InputStream mockEmptyInputStream() throws IOException { return stream; } + @ParameterizedTest + @ValueSource(strings = {"http", "https"}) + void nonDefaultUrlHandler(String protocol) throws IOException { + String sysProperty = "java.protocol.handler.pkgs"; + + assertThat(System.getProperty(sysProperty)).isNull(); + try { + // implementation classes are provided in this package with known structure hardcoded in the JDK + System.setProperty(sysProperty, "co.elastic.apm.agent.report.fake"); + + String url = protocol + "://not.found:9999"; + + URL originalUrl = new URL(url); + + // overridden default handler does not allow opening a connection + assertThatThrownBy(originalUrl::openConnection).hasMessageStartingWith("fake handler"); + + URL modifiedUrl = HttpUtils.withDefaultHandler(originalUrl); + + // unknown host exception is expected here + assertThatThrownBy(()-> modifiedUrl.openConnection().getInputStream()) + .isInstanceOf(UnknownHostException.class); + + + } finally { + System.clearProperty(sysProperty); + + assertThat(System.getProperty(sysProperty)) + .isNull(); + } + } + + + } diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java new file mode 100644 index 0000000000..dd92d78c55 --- /dev/null +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java @@ -0,0 +1,14 @@ +package co.elastic.apm.agent.report.fake.http; + +import java.io.IOException; +import java.net.URL; +import java.net.URLConnection; +import java.net.URLStreamHandler; + +public class Handler extends URLStreamHandler { + + @Override + protected URLConnection openConnection(URL u) throws IOException { + throw new IllegalStateException("fake handler does not allow to create connections"); + } +} diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java new file mode 100644 index 0000000000..cd575dba17 --- /dev/null +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java @@ -0,0 +1,5 @@ +package co.elastic.apm.agent.report.fake.https; + + +public class Handler extends co.elastic.apm.agent.report.fake.http.Handler { +} From 37d58901967cf7090dd792d10e56fb328f4c68bf Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 28 Jan 2022 14:29:43 +0100 Subject: [PATCH 2/5] avoid test side-effects through instrumentation --- .../apm/agent/report/HttpUtilsTest.java | 129 +++++++++++++++--- .../apm/agent/report/fake/http/Handler.java | 14 -- .../apm/agent/report/fake/https/Handler.java | 5 - .../apm/agent/util/CustomEnvVariables.java | 3 - 4 files changed, 111 insertions(+), 40 deletions(-) delete mode 100644 apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java delete mode 100644 apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java index 69bd3e131c..8dda5b3874 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/HttpUtilsTest.java @@ -18,24 +18,40 @@ */ package co.elastic.apm.agent.report; +import co.elastic.apm.agent.sdk.DynamicTransformer; +import co.elastic.apm.agent.sdk.ElasticApmInstrumentation; +import co.elastic.apm.agent.util.CustomEnvVariables; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import javax.annotation.Nullable; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; import java.net.URL; +import java.net.URLConnection; +import java.net.URLStreamHandler; +import java.net.URLStreamHandlerFactory; import java.net.UnknownHostException; +import java.util.Collection; +import java.util.Collections; +import java.util.Hashtable; +import java.util.concurrent.atomic.AtomicBoolean; -import static org.assertj.core.api.Assertions.assertThat; +import static net.bytebuddy.matcher.ElementMatchers.isStatic; +import static net.bytebuddy.matcher.ElementMatchers.named; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -class HttpUtilsTest { +class HttpUtilsTest extends CustomEnvVariables { @Test void consumeAndCloseIgnoresNullConnection() { @@ -88,38 +104,115 @@ private static InputStream mockEmptyInputStream() throws IOException { return stream; } + private static final String FACTORY_MSG = "fake handler can't open connections"; + private static final AtomicBoolean factoryActive = new AtomicBoolean(false); + + static { + // This method to set the global factory is designed to be called once. It is not possible to + // un-register the factory after registration + URL.setURLStreamHandlerFactory(new URLStreamHandlerFactory() { + + @Nullable + @Override + public URLStreamHandler createURLStreamHandler(String p) { + if (!factoryActive.get()) { + // The factory will return null when it does not support a given protocol + return null; + } + + // any non-null value returned here will be registered globally + // In order to reset the default behavior calling URL.setURLStreamHandlerFactory(null) is required. + return new URLStreamHandler() { + @Override + protected URLConnection openConnection(URL u) throws IOException { + throw new IllegalStateException(FACTORY_MSG); + } + }; + } + }); + } + @ParameterizedTest @ValueSource(strings = {"http", "https"}) void nonDefaultUrlHandler(String protocol) throws IOException { - String sysProperty = "java.protocol.handler.pkgs"; - assertThat(System.getProperty(sysProperty)).isNull(); + // the handler is set in the URL constructor, thus we have to rebuild it + // every time we expect a different handler. + String url = protocol + "://not.found:9999"; + + DynamicTransformer.ensureInstrumented(URL.class, Collections.singleton(TestUrlInstrumentation.class)); + try { - // implementation classes are provided in this package with known structure hardcoded in the JDK - System.setProperty(sysProperty, "co.elastic.apm.agent.report.fake"); + // test default behavior + checkDefaultHandler(new URL(url)); - String url = protocol + "://not.found:9999"; + factoryActive.set(true); - URL originalUrl = new URL(url); + // required to remove registered handlers + URL.setURLStreamHandlerFactory(null); // overridden default handler does not allow opening a connection - assertThatThrownBy(originalUrl::openConnection).hasMessageStartingWith("fake handler"); + assertThatThrownBy(() -> new URL(url).openConnection()) + .hasMessage(FACTORY_MSG); - URL modifiedUrl = HttpUtils.withDefaultHandler(originalUrl); + checkDefaultHandler(HttpUtils.withDefaultHandler(new URL(url))); + } finally { + factoryActive.set(false); - // unknown host exception is expected here - assertThatThrownBy(()-> modifiedUrl.openConnection().getInputStream()) - .isInstanceOf(UnknownHostException.class); + // required to remove registered handlers + URL.setURLStreamHandlerFactory(null); + // should not impact non-fake URLs + checkDefaultHandler(new URL(url)); + } + } - } finally { - System.clearProperty(sysProperty); - assertThat(System.getProperty(sysProperty)) - .isNull(); - } + private void checkDefaultHandler(URL url) { + // unknown host exception is expected here + assertThatThrownBy(() -> url.openConnection().getInputStream()) + .isInstanceOf(UnknownHostException.class); } + /** + * Instrumentation of {@link URL#setURLStreamHandlerFactory(URLStreamHandlerFactory)} to allow resetting handlers + * by calling {@code URL.setURLStreamHandlerFactory(null);}. + */ + public static final class TestUrlInstrumentation extends ElasticApmInstrumentation { + + @Override + public ElementMatcher getTypeMatcher() { + return named("java.net.URL"); + } + @Override + public ElementMatcher getMethodMatcher() { + return isStatic().and(named("setURLStreamHandlerFactory")); + } + + @Override + public Collection getInstrumentationGroupNames() { + return Collections.singleton("test-url"); + } + + @Override + public String getAdviceClassName() { + return "co.elastic.apm.agent.report.HttpUtilsTest$TestUrlInstrumentation$AdviceClass"; + } + + public static final class AdviceClass { + + @Advice.OnMethodEnter(suppress = Throwable.class, inline = false, skipOn = Advice.OnNonDefaultValue.class) + public static boolean onEnter(@Advice.Argument(0) @Nullable URLStreamHandlerFactory factory, + @Advice.FieldValue("handlers") Hashtable handlers) { + + if (factory == null) { + handlers.clear(); + return true; + } + return false; + } + } + } } diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java deleted file mode 100644 index dd92d78c55..0000000000 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/http/Handler.java +++ /dev/null @@ -1,14 +0,0 @@ -package co.elastic.apm.agent.report.fake.http; - -import java.io.IOException; -import java.net.URL; -import java.net.URLConnection; -import java.net.URLStreamHandler; - -public class Handler extends URLStreamHandler { - - @Override - protected URLConnection openConnection(URL u) throws IOException { - throw new IllegalStateException("fake handler does not allow to create connections"); - } -} diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java deleted file mode 100644 index cd575dba17..0000000000 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/fake/https/Handler.java +++ /dev/null @@ -1,5 +0,0 @@ -package co.elastic.apm.agent.report.fake.https; - - -public class Handler extends co.elastic.apm.agent.report.fake.http.Handler { -} diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/util/CustomEnvVariables.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/util/CustomEnvVariables.java index 9930badee6..c25c41e06d 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/util/CustomEnvVariables.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/util/CustomEnvVariables.java @@ -20,13 +20,10 @@ import co.elastic.apm.agent.AbstractInstrumentationTest; import co.elastic.apm.agent.testinstr.SystemSingleEnvVariablesInstrumentation; -import org.junit.jupiter.api.Test; import java.util.Map; import java.util.concurrent.Callable; -import static org.assertj.core.api.Assertions.assertThat; - public abstract class CustomEnvVariables extends AbstractInstrumentationTest { protected void runWithCustomEnvVariables(Map customEnvVariables, Runnable runnable) { From d3460ac00254b7fb695749575c98073e94acadae Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 28 Jan 2022 17:20:30 +0100 Subject: [PATCH 3/5] ensure cloud provider is safe to use --- .../elastic/apm/agent/impl/metadata/CloudMetadataProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/metadata/CloudMetadataProvider.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/metadata/CloudMetadataProvider.java index 4d462cd935..21c9155c56 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/metadata/CloudMetadataProvider.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/metadata/CloudMetadataProvider.java @@ -20,6 +20,7 @@ import co.elastic.apm.agent.configuration.CoreConfiguration; import co.elastic.apm.agent.configuration.ServerlessConfiguration; +import co.elastic.apm.agent.report.HttpUtils; import co.elastic.apm.agent.util.ExecutorUtils; import co.elastic.apm.agent.util.UrlConnectionUtils; import com.dslplatform.json.DslJson; @@ -415,7 +416,7 @@ private static Map deserialize(String input) throws IOException } private static String executeRequest(String url, String method, @Nullable Map headers, int queryTimeoutMs) throws IOException { - HttpURLConnection urlConnection = (HttpURLConnection) UrlConnectionUtils.openUrlConnectionThreadSafely(new URL(url)); + HttpURLConnection urlConnection = (HttpURLConnection) UrlConnectionUtils.openUrlConnectionThreadSafely(HttpUtils.withDefaultHandler(new URL(url))); if (headers != null) { for (String header : headers.keySet()) { urlConnection.setRequestProperty(header, headers.get(header)); From fbb398d5051ed0d0f3c58e6725812ecbdfc79157 Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Fri, 28 Jan 2022 17:24:39 +0100 Subject: [PATCH 4/5] update changelog --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 12ad7b3f91..502568af2b 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -47,6 +47,7 @@ paths. The BCI warmup is on by default and may be disabled through the internal ** APM headers conversion issue within dubbo transaction * Fix External plugins automatic setting of span outcome - {pull}2376[#2376] * Avoid early initialization of JMX on Weblogic - {pull}2420[#2420] +* Ensure agent always uses JDK HTTP client - {pull}2429[#2429] [[release-notes-1.x]] === Java Agent version 1.x From 3e36ae9ddc50647b5b143209616a77a499a22c7a Mon Sep 17 00:00:00 2001 From: Sylvain Juge Date: Mon, 31 Jan 2022 09:43:30 +0100 Subject: [PATCH 5/5] fallback to original URL for safety --- .../co/elastic/apm/agent/report/HttpUtils.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java index bdbce8e350..dfd581ff4d 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/report/HttpUtils.java @@ -18,6 +18,8 @@ */ package co.elastic.apm.agent.report; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.stagemonitor.util.IOUtils; import javax.annotation.Nullable; @@ -33,6 +35,8 @@ public class HttpUtils { + private static final Logger log = LoggerFactory.getLogger(HttpUtils.class); + private HttpUtils() { } @@ -87,16 +91,22 @@ public static URL withDefaultHandler(URL url) { try { return new URL(protocol, url.getHost(), url.getPort(), url.getFile(), handlerForProtocol(protocol)); } catch (MalformedURLException e) { - throw new IllegalArgumentException(e); + return url; } } + @Nullable private static URLStreamHandler handlerForProtocol(String protocol) { try { Class handlerClass = Class.forName(String.format("sun.net.www.protocol.%s.Handler", protocol)); return (URLStreamHandler) handlerClass.getDeclaredConstructor().newInstance(); } catch (ClassNotFoundException | NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) { - throw new IllegalArgumentException("unable to retrieve handler for protocol : " + protocol); + if (log.isDebugEnabled()) { + log.debug("unable to create default HTTP stream handler for protocol '{}", protocol, e); + } else { + log.warn("unable to create default HTTP stream handler for protocol '{}", protocol); + } + return null; } } }