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 85.71429% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.54%. Comparing base (4d974ba) to head (2befe38).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/io/opentelemetry/exporter/internal/TlsUtil.java	62.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8565      +/-   ##
============================================
- Coverage     91.55%   91.54%   -0.01%     
- Complexity    10262    10263       +1     
============================================
  Files          1013     1013              
  Lines         27102    27123      +21     
  Branches       3182     3185       +3     
============================================
+ Hits          24812    24831      +19     
+ Misses         1565     1564       -1     
- Partials        725      728       +3     

☔ 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.