Fix OkHttp client mTLS when using the platform default trust store

[Debashismitra01]

Summary

This PR fixes an inconsistency between the OkHttp and JDK OTLP HTTP senders when client mTLS is configured without custom trusted certificates.

Previously, when only setClientTls(...) (or the equivalent autoconfigure properties) was configured, TlsConfigHelper correctly created an SSLContext containing the client KeyManager, but the OkHttp sender skipped installing the custom SSLSocketFactory because no explicit X509TrustManager was provided. As a result, OkHttp fell back to the platform default SSLSocketFactory, causing the configured client certificate to be silently omitted during the TLS handshake.

This change resolves the platform default X509TrustManager when no custom trust manager is configured and uses it when installing the custom SSLSocketFactory, making the OkHttp sender behave consistently with the JDK sender.

Changes

• Add a helper for resolving the platform default X509TrustManager.
• Use the platform default trust manager when configuring the OkHttp HTTP sender if no custom trust manager is supplied.
• Add a regression test covering client mTLS without custom trusted certificates against an mTLS-required endpoint.

Fixes #8562

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Debashismitra01 / name: Debashismitra01 (64d015e, c0a6159, d6866b5, ea96f60, ec843fa)

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.62%. Comparing base (2724fc6) to head (539d1a9).

Files with missing lines	Patch %	Lines
...va/io/opentelemetry/exporter/internal/TlsUtil.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8565   +/-   ##
=========================================
  Coverage     91.61%   91.62%           
- Complexity    10311    10315    +4     
=========================================
  Files          1013     1013           
  Lines         27260    27278   +18     
  Branches       3201     3202    +1     
=========================================
+ Hits          24975    24994   +19     
+ Misses         1560     1558    -2     
- Partials        725      726    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Debashismitra01]

Hi! I ran into an issue with the patch coverage check. The remaining uncovered lines are primarily defensive exception paths introduced by the new default trust manager resolution (e.g. TlsUtil.defaultTrustManager() failure handling). These paths are difficult to exercise without introducing static mocking of JDK/Mockito internals.

I considered refactoring the implementation to reduce the uncovered branches, but that would either reintroduce the original bug or require resolving the default trust manager multiple times, which I'd prefer to avoid. Before I proceed further, could you advise on the preferred direction?

Should I add tests using static mocking for these exception paths?
Or would you prefer a different implementation that keeps the fix simpler while preserving the intended behavior?

I'd appreciate your guidance on which approach best fits the project's expectations.

[abdessattar23]

Hey Mitra, thanks for taking care of this just couple things. The default trust manager gets resolved twice (once in getSslContext(), once in the sender), any way to do it once? And is the getSslContext() change needed? init(..., null, ...) already falls back to the platform default TMs.
Otherwise LGTM

[opentelemetry-pr-dashboard]

This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome.

For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question.

Automation flags a PR for human review once every review thread has a reply or is marked as resolved.

Status across open PRs is visible on the pull request dashboard.

[psx95]

Should I add tests using static mocking for these exception paths? Or would you prefer a different implementation that keeps the fix simpler while preserving the intended behavior?

I could not find any instances where static mocking is used in the repo, so I think we want to avoid that even though the mocking section did not explicitly mention it.

I think you could follow a pattern similar to how keyManager is being tested using a method added explicitly to facilitate testing.

 // Visible for testing
  static X509TrustManager defaultTrustManager(TrustManagerFactory tmf) throws SSLException {
   ...
  }

You could then refactor your existing method:

 public static X509TrustManager defaultTrustManager() throws SSLException {
    try {
      TrustManagerFactory tmf =
          TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
      return defaultTrustManager(tmf); //  <-- the visible for testing method 
    } catch (NoSuchAlgorithmException e) {
      throw new SSLException("Could not build default TrustManager.", e);
    }
  }

Hopefully this should increase coverage by allowing you to pass mocked TrustManagerFactory objects so you can control getTrustManagers and init methods (I can't seem to see exactly which lines are uncovered).

[Debashismitra01]

Should I add tests using static mocking for these exception paths? Or would you prefer a different implementation that keeps the fix simpler while preserving the intended behavior?

I could not find any instances where static mocking is used in the repo, so I think we want to avoid that even though the mocking section did not explicitly mention it.

I think you could follow a pattern similar to how keyManager is being tested using a method added explicitly to facilitate testing.

 // Visible for testing
  static X509TrustManager defaultTrustManager(TrustManagerFactory tmf) throws SSLException {
   ...
  }

You could then refactor your existing method:

 public static X509TrustManager defaultTrustManager() throws SSLException {
    try {
      TrustManagerFactory tmf =
          TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
      return defaultTrustManager(tmf); //  <-- the visible for testing method 
    } catch (NoSuchAlgorithmException e) {
      throw new SSLException("Could not build default TrustManager.", e);
    }
  }

Hopefully this should increase coverage by allowing you to pass mocked TrustManagerFactory objects so you can control getTrustManagers and init methods (I can't seem to see exactly which lines are uncovered).

Thanks for the suggestion! I was able to get the patch coverage above the required threshold without introducing static mocking. I kept the current approach and avoided adding extra testing hooks since coverage is now passing.

[psx95]

Thanks for following up on the review! Some final thoughts/comments on the PR.

[Debashismitra01]

Thanks for the review! I addressed the remaining comments:

• Updated the default trust manager tests to avoid static mocking/global property changes and follow the existing test helper pattern.
• Applied the requested cleanup changes.
  All checks are passing now.

Thanks again for the feedback!

[Debashismitra01]

Thanks, updated defaultTrustManager_returnsX509TrustManager to use the real default trust manager instead of mocking since that covers the intent better. I kept the helper-based approach for the failure case to avoid suppressing CannotMockMethod and stay consistent with the existing test patterns.

[otelbot]

⚠️ API changes detected — additional maintainer review required

@jack-berg @jkwatson

This PR modifies the public API surface area of the following module(s):

• opentelemetry-api
• opentelemetry-common
• opentelemetry-context
• opentelemetry-exporter-common
• opentelemetry-exporter-logging
• opentelemetry-exporter-logging-otlp
• opentelemetry-exporter-otlp
• opentelemetry-exporter-otlp-common
• opentelemetry-exporter-sender-grpc-managed-channel
• opentelemetry-exporter-sender-jdk
• opentelemetry-exporter-sender-okhttp
• opentelemetry-exporter-zipkin
• opentelemetry-extension-kotlin
• opentelemetry-extension-trace-propagators
• opentelemetry-opentracing-shim
• opentelemetry-sdk
• opentelemetry-sdk-common
• opentelemetry-sdk-extension-autoconfigure
• opentelemetry-sdk-extension-autoconfigure-spi
• opentelemetry-sdk-extension-jaeger-remote-sampler
• opentelemetry-sdk-logs
• opentelemetry-sdk-metrics
• opentelemetry-sdk-testing
• opentelemetry-sdk-trace

Please review the changes in docs/apidiffs/current_vs_latest/ carefully before approving.

[Debashismitra01]

Note: the last commit (Update apidiffs baseline to 1.64.0) is unrelated to this PR's functional changes — it's just regenerating docs/apidiffs/current_vs_latest/*.txt because a new OpenTelemetry release (1.64.0) shifted the comparison baseline from 1.63.0. No actual API surface changes from this PR's mTLS fix; happy to confirm if useful.

[psx95]

Hi @Debashismitra01,
Perhaps you could try to rebase your commits onto the latest main once ? I think there should be no changes in the docs/apidiffs/current_vs_latest folder.

[Debashismitra01]

I have tried doing rebase but still this changes wasn't fixed. One of my PR was merged that have the same issue. Maybe I can try again if it's fixed @psx95