Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore TLS components (SSLContext, TrustManager, KeyManager) if plain HTTP protocol is used for exporting #6329

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Ignore TLS components (SSLContext, TrustManager, KeyManager) if plain HTTP protocol is used for exporting #6329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...ers/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ public GrpcExporter<T> build() {
return result;
};

boolean isPlainHttp = "http".equals(endpoint.getScheme());
GrpcSenderProvider grpcSenderProvider = resolveGrpcSenderProvider();
GrpcSender<T> grpcSender =
grpcSenderProvider.createSender(
Expand All @@ -207,8 +208,8 @@ public GrpcExporter<T> build() {
grpcChannel,
grpcStubFactory,
retryPolicy,
tlsConfigHelper.getSslContext(),
tlsConfigHelper.getTrustManager());
isPlainHttp ? null : tlsConfigHelper.getSslContext(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since artifact layout is customized for Spring's custom Launcher classloader as mentioned above, JDK level security components are initialized without custom KeyStoreSpi implementation, because custom KeyStoreSpi implementation is not visible to system classloader, but to Spring's custom Launcher classloader.

It looks like the call to SSLContext.getInstance("TLS") is what is responsible for initializing this state? Seems brittle to need to rely on the otel java agent and otel java SDK to not call ordinary java network APIs like SSLContext.getInstance("TLS").

Why is it not ok to call SSLContext.getInstance("TLS") when the scheme is http, but acceptable to call it when scheme is https? Wouldn't spring users that want to implement KeyStoreSpi and use https be subject to this problem?

Copy link
Contributor Author

@serkan-ozal serkan-ozal Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jack-berg,

Actually, in my case, extra SSLContext.getInstance("TLS") call is not a problem. But I have though that there might be similar problem for javax.net.ssl.SSLContextSpi and java.security.Provider as they are also loaded by Java ServiceLoader. So I have prevented extra SSLContext.getInstance("TLS") calls if they are not needed (scheme is http).

If you want, I can revert those changes and only keep

builder.connectionSpecs(Collections.singletonList(ConnectionSpec.CLEARTEXT));

parts as those fix the my original issue.

Regarding to your this question

Wouldn't spring users that want to implement KeyStoreSpi and use https be subject to this problem?

Yes, you're right. They are still subject to this problem. In fact, this PR doesn't fix all the possible issues (when OTEL Java agent uses secure connection while exporting to collector), but most of the cases (I believe), because as @tylerbenson mentioned, many are exporting to local collector first without secure connection, and then local collector deals with exporting to remove vendor target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be evaluated as a performance optimization where we try to avoid doing unnecessary work in obvious cases rather than creating a guarantee that it never happens.

While this has a similar appearance as java util logging which we take great pains to deal with in the javaagent, doing so adds a significant amount of complexity in the javaagent.

For logging this complexity is worth it because logging initialization is so hard to avoid, while SSL initialization is only done in very specific locations and not difficult to avoid (with the added benefit of reducing performance overhead and not even requiring an additional setting).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The perf argument is quite different than how this is being framed. Some points to consider:

  • On my machine, SSLContext.getInstance("TLS") takes ~50ms. Anyone know how long the agent typically takes to initialize?
  • Optimizing initialization for perf reasons seems reasonable, but we shouldn't be under the impression that this is a reliable fix for the reported issue. The same issue is likely to pop up again the future.
  • If we want to optimize initialization for perf reasons, can we look at a profile of CPU time for the initialization of the agent / SDK? There's likely other low hanging fruit we can optimize.

Copy link
Contributor Author

@serkan-ozal serkan-ozal Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, for the performance optimization perspective, getting rid of redundant SSLContext.getInstance("TLS") is not the only part optimized here. But also getting rid of redundant TrustManager and KeyStore initialization in the OkHttpClient. In our case, initialization of the TrustManager is the real problem.

Here is the stacktrace to see the flow:

OpenTelemetry Javaagent failed to start
java.security.KeyStoreException: problem accessing trust store
	at java.base/sun.security.ssl.TrustManagerFactoryImpl.engineInit(TrustManagerFactoryImpl.java:73)
	at java.base/javax.net.ssl.TrustManagerFactory.init(TrustManagerFactory.java:282)
	at okhttp3.internal.platform.Platform.platformTrustManager(Platform.kt:80)
	at okhttp3.OkHttpClient.<init>(OkHttpClient.kt:237)
	at okhttp3.OkHttpClient$Builder.build(OkHttpClient.kt:1069)
	at io.opentelemetry.exporter.sender.okhttp.internal.OkHttpGrpcSender.<init>(OkHttpGrpcSender.java:97)
	at io.opentelemetry.exporter.sender.okhttp.internal.OkHttpGrpcSenderProvider.createSender(OkHttpGrpcSenderProvider.java:44)
	at io.opentelemetry.exporter.internal.grpc.GrpcExporterBuilder.build(GrpcExporterBuilder.java:189)
	at io.opentelemetry.exporter.otlp.metrics.OtlpGrpcMetricExporterBuilder.build(OtlpGrpcMetricExporterBuilder.java:243)
	at io.opentelemetry.exporter.otlp.internal.OtlpMetricExporterProvider.createExporter(OtlpMetricExporterProvider.java:71)

...
...

Caused by: java.io.IOException: Keystore was tampered with, or password was incorrect
	at java.base/sun.security.provider.JavaKeyStore.engineLoad(JavaKeyStore.java:813)
	at java.base/sun.security.util.KeyStoreDelegator.engineLoad(KeyStoreDelegator.java:242)
	at java.base/java.security.KeyStore.load(KeyStore.java:1473)
	at java.base/sun.security.ssl.TrustStoreManager$TrustAnchorManager.loadKeyStore(TrustStoreManager.java:390)
	at java.base/sun.security.ssl.TrustStoreManager$TrustAnchorManager.getTrustedCerts(TrustStoreManager.java:336)
	at java.base/sun.security.ssl.TrustStoreManager.getTrustedCerts(TrustStoreManager.java:57)
	at java.base/sun.security.ssl.TrustManagerFactoryImpl.engineInit(TrustManagerFactoryImpl.java:49)
	... 43 more
Caused by: java.security.UnrecoverableKeyException: Password verification failed
	at java.base/sun.security.provider.JavaKeyStore.engineLoad(JavaKeyStore.java:811)
	... 49 more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylerbenson @jack-berg What you are thinking about this PR? Is there anything I can add/fix this PR? Or the Do changes make sense you?

isPlainHttp ? null : tlsConfigHelper.getTrustManager());
LOGGER.log(Level.FINE, "Using GrpcSender: " + grpcSender.getClass().getName());

return new GrpcExporter<>(exporterName, type, grpcSender, meterProviderSupplier);
Expand Down
5 changes: 3 additions & 2 deletions ...ers/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ public HttpExporter<T> build() {
return result;
};

boolean isPlainHttp = endpoint.startsWith("http://");
HttpSenderProvider httpSenderProvider = resolveHttpSenderProvider();
HttpSender httpSender =
httpSenderProvider.createSender(
Expand All @@ -198,8 +199,8 @@ public HttpExporter<T> build() {
proxyOptions,
authenticator,
retryPolicy,
tlsConfigHelper.getSslContext(),
tlsConfigHelper.getTrustManager());
isPlainHttp ? null : tlsConfigHelper.getSslContext(),
isPlainHttp ? null : tlsConfigHelper.getTrustManager());
LOGGER.log(Level.FINE, "Using HttpSender: " + httpSender.getClass().getName());

return new HttpExporter<>(exporterName, type, httpSender, meterProviderSupplier, exportAsJson);
Expand Down
13 changes: 9 additions & 4 deletions ...http/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import javax.net.ssl.X509TrustManager;
import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.ConnectionSpec;
import okhttp3.HttpUrl;
import okhttp3.OkHttpClient;
import okhttp3.Protocol;
Expand Down Expand Up @@ -89,14 +90,18 @@ public OkHttpGrpcSender(
clientBuilder.addInterceptor(
new RetryInterceptor(retryPolicy, OkHttpGrpcSender::isRetryable));
}
if (sslContext != null && trustManager != null) {
clientBuilder.sslSocketFactory(sslContext.getSocketFactory(), trustManager);
}
if (endpoint.startsWith("http://")) {

boolean isPlainHttp = endpoint.startsWith("http://");
if (isPlainHttp) {
clientBuilder.connectionSpecs(Collections.singletonList(ConnectionSpec.CLEARTEXT));
clientBuilder.protocols(Collections.singletonList(Protocol.H2_PRIOR_KNOWLEDGE));
} else {
clientBuilder.protocols(Arrays.asList(Protocol.HTTP_2, Protocol.HTTP_1_1));
if (sslContext != null && trustManager != null) {
clientBuilder.sslSocketFactory(sslContext.getSocketFactory(), trustManager);
}
}

this.client = clientBuilder.build();
this.headersSupplier = headersSupplier;
this.url = HttpUrl.get(endpoint);
Expand Down
9 changes: 8 additions & 1 deletion ...http/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.opentelemetry.sdk.common.export.RetryPolicy;
import java.io.IOException;
import java.time.Duration;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
Expand All @@ -25,6 +26,7 @@
import javax.net.ssl.X509TrustManager;
import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.ConnectionSpec;
import okhttp3.HttpUrl;
import okhttp3.MediaType;
import okhttp3.OkHttpClient;
Expand Down Expand Up @@ -88,9 +90,14 @@ public OkHttpHttpSender(
if (retryPolicy != null) {
builder.addInterceptor(new RetryInterceptor(retryPolicy, OkHttpHttpSender::isRetryable));
}
if (sslContext != null && trustManager != null) {

boolean isPlainHttp = endpoint.startsWith("http://");
if (isPlainHttp) {
builder.connectionSpecs(Collections.singletonList(ConnectionSpec.CLEARTEXT));
} else if (sslContext != null && trustManager != null) {
builder.sslSocketFactory(sslContext.getSocketFactory(), trustManager);
}

this.client = builder.build();
this.url = HttpUrl.get(endpoint);
this.compressor = compressor;
Expand Down
12 changes: 8 additions & 4 deletions .../java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSamplerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import javax.annotation.Nullable;
import javax.net.ssl.SSLContext;
import javax.net.ssl.X509TrustManager;
import okhttp3.ConnectionSpec;
import okhttp3.Headers;
import okhttp3.OkHttpClient;
import okhttp3.Protocol;
Expand Down Expand Up @@ -165,14 +166,17 @@ public JaegerRemoteSampler build() {

clientBuilder.callTimeout(Duration.ofNanos(TimeUnit.SECONDS.toNanos(DEFAULT_TIMEOUT_SECS)));

SSLContext sslContext = tlsConfigHelper.getSslContext();
X509TrustManager trustManager = tlsConfigHelper.getTrustManager();
String endpoint = this.endpoint.resolve(GRPC_ENDPOINT_PATH).toString();
boolean isPlainHttp = endpoint.startsWith("http://");

SSLContext sslContext = isPlainHttp ? null : tlsConfigHelper.getSslContext();
X509TrustManager trustManager = isPlainHttp ? null : tlsConfigHelper.getTrustManager();
if (sslContext != null && trustManager != null) {
clientBuilder.sslSocketFactory(sslContext.getSocketFactory(), trustManager);
}

String endpoint = this.endpoint.resolve(GRPC_ENDPOINT_PATH).toString();
if (endpoint.startsWith("http://")) {
if (isPlainHttp) {
clientBuilder.connectionSpecs(Collections.singletonList(ConnectionSpec.CLEARTEXT));
clientBuilder.protocols(Collections.singletonList(Protocol.H2_PRIOR_KNOWLEDGE));
} else {
clientBuilder.protocols(Arrays.asList(Protocol.HTTP_2, Protocol.HTTP_1_1));
Expand Down
Loading