Skip to content

Reject mixing keyManager and sslContext in TlsConfigHelper - #8710

Open
thswlsqls wants to merge 1 commit into
open-telemetry:mainfrom
thswlsqls:fix/tls-config-helper-key-manager-conflict
Open

Reject mixing keyManager and sslContext in TlsConfigHelper#8710
thswlsqls wants to merge 1 commit into
open-telemetry:mainfrom
thswlsqls:fix/tls-config-helper-key-manager-conflict

Conversation

@thswlsqls

Copy link
Copy Markdown
Contributor

Fixes #8708

Description

  • TlsConfigHelper.setSslContext did not check keyManager and setKeyManagerFromCerts did not check sslContext, so both APIs could be used on one builder.
  • Not a new restriction: the class Javadoc says these APIs may be used "but NOT both", and OtlpHttpSpanExporterBuilder.setSslContext tells users to "call this or set raw certificate bytes, but not both".
  • Code that now throws was already broken — senders pass getSslContext() and getTrustManager() to the transport and never read getKeyManager(), so the client certificate was silently dropped.
  • trustManager is already guarded in both directions; keyManager was the only asymmetric case.
  • Existing exception messages are unchanged; each missing direction is a separate if.

Testing done

  • Extended TlsConfigHelperTest#createKeyManager_AlreadyExists_Throws and #setSslContext_AlreadyExists_Throws with opposite-direction blocks, matching the trust manager tests.
  • Both fail when the production change is reverted.
  • ./gradlew :exporters:common:check — 70 tests passed.
  • No apidiff change: io.opentelemetry.exporter.internal is excluded from japicmp and no signature changed.

TlsConfigHelper documents that the PEM-based higher level API and the
lower level setSslContext are mutually exclusive, but only the
trustManager side enforced it. Calling setClientTls followed by
setSslContext passed without error and the client certificate was
silently dropped, because production senders read getSslContext() and
getTrustManager() only.

Add the two missing cross-checks so the documented IllegalStateException
is thrown in both directions, and extend the existing tests to cover
them.
@thswlsqls
thswlsqls marked this pull request as ready for review August 10, 2026 00:39
@thswlsqls
thswlsqls requested a review from a team as a code owner August 10, 2026 00:39
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 10, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-08-21 21:40 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

  • Inline threads: 1
Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
  • Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.65%. Comparing base (d948e13) to head (7b68e41).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8710   +/-   ##
=========================================
  Coverage     91.65%   91.65%           
- Complexity    10352    10354    +2     
=========================================
  Files          1003     1003           
  Lines         27210    27214    +4     
  Branches       3199     3201    +2     
=========================================
+ Hits          24939    24943    +4     
  Misses         1566     1566           
  Partials        705      705           

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

Comment on lines 100 to +102
}
if (this.keyManager != null) {
throw new IllegalStateException("keyManager has been previously configured");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not blocking, but consider also adding an explicit sslContext guard to setTrustManagerFromCerts for full symmetry with the two new guards here and in setKeyManagerFromCerts.

Today the setSslContextsetTrustManagerFromCerts case is caught transitively, because setSslContext also assigns this.trustManager, so the existing trustManager != null check fires. Two small downsides:

  1. The resulting message is "trustManager has been previously configured", which is a little misleading when the user actually called setSslContext.
  2. It silently relies on setSslContext requiring a non-null X509TrustManager. If that ever loosens, the guard vanishes.

Proposed addition in setTrustManagerFromCerts, mirroring the pattern used elsewhere in this PR:

  public void setTrustManagerFromCerts(byte[] trustedCertsPem) {
    if (trustManager != null) {
      throw new IllegalStateException("trustManager has been previously configured");
    }
    if (sslContext != null) {
      throw new IllegalStateException("sslContext has been previously configured");
    }
    ...
  }

With this, all three setters follow the same "reject each other mode explicitly" shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TlsConfigHelper silently drops the key manager when an SSLContext is also set

2 participants