Skip to content

Netty, OkHTTP, Springboot test fixes.#334

Merged
cconlon merged 5 commits intowolfSSL:masterfrom
JeremiahM37:testing-fixes
Mar 18, 2026
Merged

Netty, OkHTTP, Springboot test fixes.#334
cconlon merged 5 commits intowolfSSL:masterfrom
JeremiahM37:testing-fixes

Conversation

@JeremiahM37
Copy link
Copy Markdown
Contributor

@JeremiahM37 JeremiahM37 commented Feb 23, 2026

wolfJSSE fixes for FIPS test coverage (JSSE correctness / root-cause fixes)

Summary
This PR consolidates wolfJSSE fixes identified while running the Netty, Spring Boot,
and OkHttp test suites on the wolfSSL OpenJDK FIPS base image.

Key fixes

  1. SSLEngine buffer handling / unwrap correctness
  • Fix BUFFER_UNDERFLOW handling for partial TLS records (header-length precheck)
  • Fix BUFFER_OVERFLOW handling by stashing decrypted app data and retrying cleanly
  • Add regression tests for partial-record underflow and small-output overflow paths
  1. SSLEngine close_notify / shutdown state handling
  • Fix SSLEngine close/shutdown state transitions and handshake status behavior
  • Improve mapping of native shutdown flags to observable SSLEngine close_notify state
  • Fix outbound completion status (isOutboundDone()) when buffered close_notify data
    is still pending
  • Add TLS 1.3 close_notify handshake-status regression coverage
  1. TrustManager verification / exception cause chains
  • Preserve TrustManager CertificateException as the cause of SSLHandshakeException
    in both SSLSocket and SSLEngine paths
  • Expose/store last verification exception through the internal verify callback path
  • Add SSLSocket + SSLEngine regression tests for preserved cause chains
  1. Hostname verification / SNI behavior
  • Prefer configured/requested SNI hostname over session peer IP for hostname checks
    when endpoint identification is enabled
  • Add server-side SSLEngine SNIMatcher enforcement after native handshake success
  • Enable auto-SNI for SSLEngine(host, port) when no explicit SNI is configured
  • Support SSLEngine(host, -1) unknown-port hints (Netty compatibility)
  • Skip session cache / serverID keying when port is unknown (-1)
  • Expose requested SNI on SSLEngine handshake sessions early enough for key
    selection
  • Return non-null ExtendedSSLSession signature algorithm arrays
  • Fix stale SNI cache reuse (native SNI first, cache fallback only)
  1. Session cache / timeout correctness
  • Fix session-timeout boundary behavior (expire at >= timeout)
  • Filter invalid/expired sessions from SSLSessionContext.getIds() / getSession()
  • Handle freed native session objects safely when updating timeouts
  • Add regression coverage for timeout boundary and invalidation filtering
  1. Trust store / trust-peer-cert correctness
  • Fix WolfSSLTrustX509.getAcceptedIssuers() operator precedence bug
  • Align WolfSSLCertManager CA loading behavior with trust-peer-cert mode
    (WOLFSSL_TRUST_PEER_CERT)
  • Simplify/fix OCSP chain issuer handling to use provided chain entries directly
  • Add OCSP regression coverage for the chain/issuer path
  1. Session / principal compatibility fixes
  • Preserve session value bindings when copying sessions
  • Improve peer principal compatibility by using Java X509 certificate subject DN
  • Add equals() / hashCode() for WolfSSLX509 principal compatibility paths
  1. Fixed Netty TLS 1.3 cipher/protocol parity in wolfJSSE: reconcile enabled protocols with configured cipher suites both at engine init and when SSLEngine.setEnabledCipherSuites()/setEnabledProtocols() is called pre-handshake.

  2. Diagnostics / test coverage

  • Added unit tests for most of the issues (SSLEngine buffer handling, close_notify status, exception cause chains, session timeout filtering, and OCSP chain/issuer handling)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses wolfJSSE correctness issues identified during FIPS test coverage with Netty, Spring Boot, and OkHttp test suites. The changes focus on fixing SSLEngine buffer handling, close_notify state management, TrustManager exception propagation, hostname verification, session cache behavior, and trust store compatibility.

Changes:

  • Fixed SSLEngine buffer underflow/overflow handling for partial TLS records and small output buffers
  • Corrected close_notify shutdown state transitions and handshake status behavior for TLS 1.3
  • Preserved TrustManager CertificateException as the cause of SSLHandshakeException in both SSLSocket and SSLEngine paths
  • Improved hostname verification using SNI hostname over session peer IP and added server-side SNIMatcher enforcement
  • Fixed session timeout boundary behavior and filtering of invalid/expired sessions from session context APIs
  • Corrected trust store operator precedence bug and OCSP chain issuer handling
  • Enhanced session value binding preservation and peer principal compatibility

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
WolfSSLTrustX509Test.java Added regression test for OCSP chain/issuer handling
WolfSSLTestFactory.java Improved error diagnostics for close_notify test failures
WolfSSLSocketTest.java Added test for SSLHandshakeException cause chain preservation
WolfSSLSessionContextTest.java Added tests for session timeout boundary and invalidation filtering
WolfSSLJSSETestSuite.java Removed trailing blank line
WolfSSLEngineTest.java Added tests for exception cause chains, TLS 1.3 close_notify, buffer underflow/overflow
WolfSSLX509.java Added equals() and hashCode() methods to Principal inner class
WolfSSLTrustX509.java Fixed operator precedence bug and simplified OCSP chain/issuer handling
WolfSSLSocket.java Preserved verify exception for SSLHandshakeException cause chain
WolfSSLSessionContext.java Added session validation and timeout checking in getSession() and getIds()
WolfSSLInternalVerifyCb.java Added verify exception storage and provider-level hostname verification
WolfSSLImplementSSLSession.java Preserved session bindings on copy and improved peer principal compatibility
WolfSSLEngineHelper.java Added getLastVerifyException() method and improved SNI auto-configuration
WolfSSLEngine.java Fixed buffer overflow/underflow handling, close_notify state management, and exception cause chains
WolfSSLContext.java Added cause to IllegalStateException when engine creation fails
WolfSSLAuthStore.java Fixed session timeout boundary condition and added error handling
WolfSSLCertManager.java Fixed trust-peer-cert mode CA loading condition

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLTrustX509Test.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLSocket.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java Outdated
@JeremiahM37 JeremiahM37 force-pushed the testing-fixes branch 5 times, most recently from 4c219b1 to 82e9fa1 Compare February 24, 2026 15:33
@JeremiahM37 JeremiahM37 force-pushed the testing-fixes branch 3 times, most recently from a3d80c9 to dd6aab3 Compare February 25, 2026 18:09
@cconlon cconlon requested a review from Copilot February 27, 2026 22:05
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLInternalVerifyCb.java Outdated
Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLSessionContextTest.java Outdated
Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLSessionContextTest.java Outdated
Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLSocketTest.java
Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLSocketTest.java
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLSessionContext.java Outdated
Copy link
Copy Markdown
Member

@cconlon cconlon left a comment

Choose a reason for hiding this comment

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

I'll review this more early next week. Does this branch pass all of our existing SunJSSE tests?

Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java Outdated
JeremiahM37 added a commit to JeremiahM37/wolfssljni that referenced this pull request Mar 2, 2026
- Add native JNI wrapper for wolfSSL_SNI_GetFromBuffer(), switch
  SSLEngine SNI extraction to use it with Java fallback for
  NOT_COMPILED_IN builds
- Add clearPendingAppData() helper, clear stale pending app data on
  unwrap() exception paths, closeInbound(), and closeOutbound()
- Reset pendingNetConsumed when stashing new decrypted data
- Move getBytes() length validation before allocation
- Add TLS-only clarifying comment on record-header underflow check
- Detect peer closing connection during handshake (SSLHandshakeException)
- Store verifyException on hostname verification failure paths
- Add debug logging in setNativeTimeout catch block
- Move word32 dropCount declarations before (void) casts for C89
- Re-interrupt thread on InterruptedException in session context tests
- Add restoreClientSessionCacheProperty() test helper
JeremiahM37 added a commit to JeremiahM37/wolfssljni that referenced this pull request Mar 2, 2026
JeremiahM37 added a commit to JeremiahM37/wolfssljni that referenced this pull request Mar 2, 2026
JeremiahM37 added a commit to JeremiahM37/wolfssljni that referenced this pull request Mar 2, 2026
JeremiahM37 added a commit to JeremiahM37/wolfssljni that referenced this pull request Mar 2, 2026
JeremiahM37 added a commit to JeremiahM37/wolfssljni that referenced this pull request Mar 2, 2026
@JeremiahM37
Copy link
Copy Markdown
Contributor Author

Changed one native file to address the claude feedback related to the c89 after declaration. This has nothing to do with my changes, but I still implemented it since it seems like something that should be done.

I addressed the rest of the claude feedback related to my changes.

@JeremiahM37
Copy link
Copy Markdown
Contributor Author

JeremiahM37 commented Mar 2, 2026

Claude comments:

  1. pendingAppData can go stale on error paths: Fixed. WolfSSLEngine now uses clearPendingAppData() to clear pendingAppData, pendingAppDataLen, and pendingNetConsumed on unwrap() exception paths and in closeInbound() / closeOutbound(). That prevents stale consumed offsets from being applied on a later successful unwrap().

  2. C89 declaration-after-statement in DTLS drop count functions: Fixed. The word32 dropCount declarations in com_wolfssl_WolfSSLSession.c were moved before the (void) statements so the code remains valid for stricter C89/C90 compilers.

  3. SNI parsing allocates on every server-side unwrap() during handshake: Addressed. WolfSSLEngine now uses WolfSSL.getSNIFromBuffer() first for pending-input SNI extraction, so the old Java parser is no longer the default path on supported builds. The old parser is retained only as a NOT_COMPILED_IN fallback.

  4. verifyHostnameForExternalTM allocates a new WolfSSLCertificate from DER each time: Reviewed, no change in this pass. The concern is valid, but the proper fix changes verification flow by moving hostname verification earlier and relying more on native hostname setup/checking. That is larger than this bug-fix pass.

  5. setEnabledCipherSuites() / setEnabledProtocols() throw unchecked IllegalArgumentException: Reviewed, no change. This matches normal JSSE behavior, including SunJSSE, where invalid setter input is surfaced as IllegalArgumentException.

  6. setNativeTimeout catch block swallows exception silently: Fixed. Added WolfSSLDebug.log() in that catch path before invalidating the session entry so the failure is diagnosable.

  7. README wrong class name in three places: Intentionally not changed. This is documentation-only and was left out to keep the branch focused on code and test fixes.

  8. README trailing whitespace: Intentionally not changed. This is documentation-only and was left out to keep the branch focused on code and test fixes.

  9. getBytes() validates len < 0 after allocation: Fixed. The bounds check now runs before new byte[len], removing the NegativeArraySizeException footgun.

  10. getPeerPrincipal() now calls heavier getPeerCertificates(): Reviewed, no change. The extra cost is intentional because the change preserves standard Java certificate handling and X500Name.equals() compatibility.

  11. Session context getIds() / getSession() now call updateTimeouts() eagerly: Partially addressed. The eager timeout invalidation remains by design for JSSE correctness, but getIds() no longer does the extra N+1 session lookups. Valid-ID filtering now happens inside WolfSSLAuthStore.getAllIDs() in one store pass, and WolfSSLSessionContext.getIds() returns that directly.

  12. Trailing newline removals: No functional issue; no change needed.

Native reuse suggestions:

  1. Replace Java SNI parsing with wolfSSL_SNI_GetFromBuffer(): Addressed. WolfSSLEngine now uses WolfSSL.getSNIFromBuffer() first. The JNI helper itself comes from upstream PR #339; this branch updates the JSSE-side call site to use it.

  2. Lean more on native cipher-list handling for TLS 1.3 vs legacy partitioning: Reviewed, no change in this pass. This would require additional native API surface and broader retesting.

  3. Replace manual hostname verification with pre-handshake native hostname setup: Reviewed, no change in this pass. Valid long-term cleanup, but it changes handshake setup and verification ordering and is larger than the fixes here.

  4. TLS record completeness check should be documented as TLS-only: Partially addressed. The optimization was kept, and a clarifying comment was added that it is TLS-only and that native WANT_READ remains the correctness fallback.

  5. Session timeout expiration could delegate more to native: Reviewed, no change. The Java session cache is distinct from the native session cache, so native flush_sessions does not replace the current Java-side invalidation behavior.

Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java Outdated
Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLSessionContextTest.java Outdated
Comment thread src/test/com/wolfssl/provider/jsse/test/WolfSSLSessionContextTest.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLInternalVerifyCb.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLInternalVerifyCb.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLInternalVerifyCb.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLTrustX509.java
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLX509.java Outdated
@JeremiahM37 JeremiahM37 force-pushed the testing-fixes branch 3 times, most recently from ae5020e to a675e39 Compare March 6, 2026 17:50
@JeremiahM37 JeremiahM37 assigned cconlon and JeremiahM37 and unassigned JeremiahM37 and cconlon Mar 6, 2026
@cconlon
Copy link
Copy Markdown
Member

cconlon commented Mar 6, 2026

I have a commit in one of my wolfssljni PRs that will fix the Patched JNI CI failure. Once that PR gets in, we can rebase this on top of master and the test should pass.

Commit ref: bf911ff

@JeremiahM37
Copy link
Copy Markdown
Contributor Author

JeremiahM37 commented Mar 9, 2026

I added a few more bug fixes I found after your most recent okhttp feedback. These changes are contained in the last 2 commits. I changed the changed the setEnabledProtocols(empty) behavior in both WolfSSLServerSocket and WolfSSLSocket to accept empty arrays as this is what sunjsse does and what testsuites like netty and okhttp expect. When empty, no protocol restrictions are applied and wolfSSL uses its default protocol set. I also modified two of the unit tests accordingly. Still passes all sunJSSE tests.

@JeremiahM37 JeremiahM37 force-pushed the testing-fixes branch 4 times, most recently from 16f7c96 to 5876be8 Compare March 11, 2026 15:29
@cconlon
Copy link
Copy Markdown
Member

cconlon commented Mar 16, 2026

Looks good overall, please hold off making any more fixes in this PR until we can get it in. For test failures, please rebase on master once #343 gets merged in, thanks.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLInternalVerifyCb.java Outdated
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLInternalVerifyCb.java
Comment thread src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java
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.

3 participants