Skip to content

fix(internal): narrow TLS classification to certificate failures only - #206

Open
tanderson-ld wants to merge 1 commit into
mainfrom
ta/SDK-2789/narrow-tls-classification
Open

fix(internal): narrow TLS classification to certificate failures only#206
tanderson-ld wants to merge 1 commit into
mainfrom
ta/SDK-2789/narrow-tls-classification

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

FailureClass.hasTlsOrCertificateCause matched any SSLException or GeneralSecurityException anywhere in the cause chain. Since all SDK traffic is HTTPS, every transport error arrives through the TLS layer — so this swept in transient faults unrelated to certificate validity and classified them UNEXPECTED, pushing data sources into extended-regime backoff (5 min – 1 hr).

Found during review of #200 by @jsonbailey, who flagged the breadth but could not run a JVM to confirm what JSSE actually throws. Confirmed empirically below.

The problem, measured

Against a real SSLServerSocket:

Scenario JSSE exception Old classification
Peer sends FIN mid-handshake SSLHandshakeException: Remote host terminated the handshake
← caused by EOFException: SSL peer shut down incorrectly
UNEXPECTED
Peer sends RST mid-handshake SocketException: Broken pipe NORMAL ✓
Untrusted certificate chain SSLHandshakeExceptionValidatorExceptionSunCertPathBuilderException UNEXPECTED ✓

The regime therefore depended on whether an intermediary sent FIN or RST — an arbitrary implementation detail. Real triggers for FIN-mid-handshake are all transient: load balancer draining during a rolling restart, a connection-limit polite close, an idle timeout during a slow handshake.

The compounding case is worse than a single stall. A connection that flaps faster than the 60 s healthy-operation reset window never accumulates enough continuous connectivity to reset, so it ratchets 5 m → 10 m → 20 m → 40 m → 1 hr and stays there.

The fix

Match only genuinely long-lived certificate problems:

c instanceof CertificateException           // expired, not-yet-valid, hostname mismatch;
                                            //   also covers ValidatorException
  || c instanceof CertPathValidatorException  // untrusted chain
  || c instanceof CertPathBuilderException
  || c instanceof SSLPeerUnverifiedException  // hostname mismatch

Verified that a genuinely untrusted chain still classifies UNEXPECTED — JSSE's ValidatorException is a CertificateException and SunCertPathBuilderException is a CertPathBuilderException, so two links of the real chain match.

Parity with Go

This aligns Java with the Go server SDK, whose classifyTransportFailure enumerates only certificate errors and treats everything else as normal:

tls.CertificateVerificationError
x509.UnknownAuthorityError
x509.HostnameError
x509.CertificateInvalidError
// everything else -> FailureClassNormal

The previous Java behavior was a divergence from that reference implementation, not a different reading of the spec.

Tests

  • Replaces sslHandshakeIsUnexpected, which asserted the over-broad behavior, with bareSslHandshakeFailureIsNormal and peerClosedMidHandshakeIsNormal (the latter reproducing the real SSLHandshakeExceptionEOFException shape).
  • Adds certPathValidatorFailureIsUnexpected, certPathBuilderFailureIsUnexpected, certificateNotYetValidIsUnexpected, untrustedChainWrappedInHandshakeExceptionIsUnexpected, sslExceptionFromConnectionResetIsNormal.
  • Full lib/shared/internal suite green; checkstyleMain clean.

Test plan for reviewers

  • Confirm the four matched types are the right set — in particular that CertificateException is the correct catch-all for validator failures, and that nothing in Go's four cases lacks a Java counterpart here.
  • Consider whether a bare SSLHandshakeException with a cipher/protocol mismatch cause (e.g. handshake_failure alert, "No appropriate protocol") should be UNEXPECTED. It is persistent like a cert problem, but it is not a certificate error and is now classified NORMAL. I left it as NORMAL to avoid re-widening, and because a persistent mismatch keeps retrying at 1–30 s rather than stalling — but it is a judgment call.
  • Sanity-check that no other caller depends on the old broad behavior.

Downstream

#200 consumes this classifier. It needs an internal release (1.11.1) before it can pick this up, in addition to the classifyAndLogHttpFailure rename already noted in review there.


Note

Overview
Narrows when HTTPS transport failures trigger extended-regime backoff by changing FailureClass.hasTlsOrCertificateCause to walk the exception chain for certificate validation problems only (CertificateException, CertPathValidatorException, CertPathBuilderException, SSLPeerUnverifiedException), instead of any SSLException or GeneralSecurityException.

Because all SDK traffic is TLS, the old rule treated many transient handshake faults (e.g. peer FIN mid-handshake, bare SSLHandshakeException, connection-reset SSLException) as UNEXPECTED, which could push data sources into multi-minute backoff. Genuine cert issues (expired/not-yet-valid, untrusted chain wrapped in SSLHandshakeException) still classify UNEXPECTED.

Tests are updated to match: removed the expectation that every SSL handshake failure is unexpected, added cases for cert-path errors and normal transient SSL shapes, and kept wrapped-certificate-cause coverage.

Reviewed by Cursor Bugbot for commit fa662f2. Bugbot is set up for automated code reviews on this repo. Configure here.

@tanderson-ld
tanderson-ld requested a review from a team as a code owner August 26, 2026 19:41
… (SDK-2789)

hasTlsOrCertificateCause matched any SSLException or GeneralSecurityException
anywhere in the cause chain. Because SDK traffic is HTTPS, every transport
error arrives through the TLS layer, so this swept in transient faults that
have nothing to do with certificate validity.

Confirmed against a real SSLServerSocket: a peer closing the connection
mid-handshake with a FIN produces

  SSLHandshakeException: Remote host terminated the handshake
    caused by EOFException: SSL peer shut down incorrectly

which the old predicate classified UNEXPECTED, putting a data source into
extended-regime backoff (5 min - 1 hr) for what is typically a load balancer
drain, a connection-limit close, or an idle timeout during a slow handshake.
The same fault delivered as a RST instead produces SocketException and was
classified NORMAL -- so the regime depended on whether the peer sent FIN or
RST, an arbitrary detail of the intermediary.

The compounding case is worse than a single stall: a connection that flaps
faster than the 60s healthy-operation reset window never accumulates enough
continuous connectivity to reset, so it ratchets 5m -> 10m -> 20m -> 40m -> 1h
and stays there.

Now matches only genuinely long-lived certificate problems:
- CertificateException (expired, not yet valid, hostname mismatch from a
  verifier; also covers sun.security.validator.ValidatorException)
- CertPathValidatorException / CertPathBuilderException (untrusted chain)
- SSLPeerUnverifiedException (hostname mismatch)

This aligns Java with the Go server SDK's classifier, which enumerates only
certificate errors (tls.CertificateVerificationError, x509.UnknownAuthorityError,
x509.HostnameError, x509.CertificateInvalidError) and treats everything else as
normal. The previous Java behavior was a divergence from that reference, not a
different reading of the spec.

Verified empirically that a genuinely untrusted chain still classifies
UNEXPECTED: JSSE produces SSLHandshakeException -> ValidatorException ->
SunCertPathBuilderException, and the first two match the new predicate.

Tests: replaces sslHandshakeIsUnexpected (which asserted the over-broad
behavior) with bareSslHandshakeFailureIsNormal and peerClosedMidHandshakeIsNormal;
adds coverage for CertPathValidatorException, CertPathBuilderException,
CertificateNotYetValidException, and the real untrusted-chain wrapper shape.
@tanderson-ld
tanderson-ld force-pushed the ta/SDK-2789/narrow-tls-classification branch from 698cb7a to fa662f2 Compare August 26, 2026 20:03
@tanderson-ld tanderson-ld changed the title fix(internal): narrow TLS classification to certificate failures only (SDK-2789) fix(internal): narrow TLS classification to certificate failures only Aug 26, 2026
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