Skip to content

change tlsVersion to minTlsVersion but keep the tls-version config name - #930

Draft
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:min-tls-version
Draft

change tlsVersion to minTlsVersion but keep the tls-version config name#930
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:min-tls-version

Conversation

@pjfanning

@pjfanning pjfanning commented Aug 10, 2026

Copy link
Copy Markdown
Member

Motivation

PemManagersProvider.createSslContext took a tlsVersion and passed it straight to
SSLContext.getInstance(tlsVersion). That name promises more than it delivers: the argument
selects a context provider, it does not pin the protocol actually negotiated. On JDK 17
getInstance("TLSv1.3") yields a context whose engines enable [TLSv1.3, TLSv1.2], so the
only configuration that really restricted anything was the TLSv1.2 default, and it did so by
also excluding TLSv1.3.

Renaming the parameter to minTlsVersion makes the intent explicit, and requires actually
enforcing a minimum rather than relying on a side effect of provider selection.

The enforcement point matters. An SSLContext has no mutable protocol list —
getDefaultSSLParameters() returns a fresh copy on every call and there is no
setDefaultSSLParameters — so a minimum can only be applied to the SSLSocket or SSLEngine
that carries the connection.

Modification

  • createSslContext no longer takes a version at all; it builds an unrestricted "TLS" context.
    Its overloads take String or Option[String] for the CA path, where None means the default
    JVM trust store (previously open-coded in HttpContactPointBootstrap).
  • New protocolsAtOrAbove(minTlsVersion, supportedProtocols) returns the satisfying subset and
    throws IllegalArgumentException for an unknown version or an unsatisfiable minimum. Protocols
    outside the known TLS ordering (SSLv3, the SSLv2Hello pseudo-protocol) are never selected.
  • New configureClientEngine(engine, minTlsVersion) applies that set via
    engine.setEnabledProtocols, sets client mode, and re-applies
    setEndpointIdentificationAlgorithm("https")
    ConnectionContext.httpsClient(sslContext)
    sets that itself, so the engine-factory overload must restore it or hostname verification is
    silently lost.
  • All four call sites (discovery-kubernetes-api, lease-kubernetes, rolling-update-kubernetes,
    management-cluster-bootstrap) now build their connection context from an engine factory.
  • Settings fields are renamed tlsVersion -> minTlsVersion. The tls-version config key is
    unchanged
    , so no user configuration has to change; the reference.conf comments now say
    "minimum TLS version".

Effective behaviour change: with the default tls-version = "TLSv1.2", TLSv1.3 is now enabled
as well and TLSv1.2 remains the floor. tls-version = "TLSv1.3" now genuinely excludes TLSv1.2,
which it did not before.

Result

tls-version means what its name says: a floor, enforced on the connection. Configuring
TLSv1.3 no longer silently continues to accept TLSv1.2.

Tests

  • New management-pki/.../PemManagersProviderSpec (9 tests) asserts on
    getEnabledProtocols of a real engine, plus client mode and the https endpoint
    identification algorithm.
  • Directional: reinstating the original defect (applying the filter to the throwaway
    getSSLParameters copy) fails "should actually restrict the protocols enabled on the engine"
    with List("TLSv1.3", "TLSv1.2") was not equal to List("TLSv1.3"). The rest of the suite still
    passes, which is why the previous settings-only tests did not catch this.
  • HttpContactPointBootstrapSpec gains a case asserting the configured minimum reaches the
    engine and that endpoint identification survives.
  • sbt "management-pki/test" "discovery-kubernetes-api/test" "lease-kubernetes/test" — 115
    succeeded, 0 failed, 1 pending.
  • sbt "management-cluster-bootstrap/testOnly ...HttpContactPointBootstrapSpec" — 6 succeeded.
  • Scoped mimaReportBinaryIssues on management-pki, discovery-kubernetes-api, lease-kubernetes,
    management-cluster-bootstrap, rolling-update-kubernetes — all success.
  • scalafmt --mode diff-ref=origin/main and sbt +headerCheckAll — clean.

References

None - createSslContext gained its tlsVersion parameter earlier on this branch; this corrects
it before it ships.

@pjfanning
pjfanning marked this pull request as draft August 10, 2026 00:03
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.

1 participant