Repository navigation
Bound LDAP StartTLS shutdown with read timeout - #53279
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Successful JNDI shutdown skips restoring the underlying transport timeout because the layered TLS socket is already closed.
Review effort: Balanced
Findings: 1
What changed in this PR
Bounds LDAP StartTLS shutdown to prevent cleanup from blocking on close_notify.
Changes:
- Adds tracked TLS socket shutdown with a 3-second timeout.
- Integrates bounded cleanup into admin and authentication connections.
- Adds focused shutdown tests.
| File | Description |
|---|---|
LDAPStartTlsClose.java |
Implements tracked, bounded TLS shutdown. |
LDAPContextManager.java |
Applies bounded shutdown to admin connections. |
LDAPOperationManager.java |
Applies bounded shutdown to authentication cleanup. |
LDAPStartTlsCloseTest.java |
Tests tracking, timeout, and forced closure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Thanks for this PR but I don't think it fixes all scenarios. Here is why:
The three-second socket timeout does not bound shutdown when JNDI’s reader thread is already blocked in a read.
With TLS 1.2 and JNDI’s layered socket (autoClose=false), TLS shutdown can wait for the read lock held by that reader. The pending read retains its original infinite timeout; calling setSoTimeout(3000) does
not update it. Consequently, tlsResponse.close() can remain blocked indefinitely, and the catch (IOException) fallback that closes the underlying transport is never reached. See OpenJDK’s socket read
implementation and TLS shutdown lock acquisition.
The current tests cannot catch this. They use fake sockets and explicitly throw SocketTimeoutException, which verifies cleanup after an exception but assumes the behavior that needs proving: that real TLS
shutdown actually times out.
For these teardown paths, a simpler fix would be to capture the original TCP socket passed to SSLSocketFactory.createSocket(Socket, …) and close it before invoking TLS cleanup. Closing the underlying transport
unblocks the reader. Keep LDAP context cleanup in finally. This sacrifices graceful close_notify and potentially LDAP Unbind delivery, but both callers are disposing of the connection immediately, so plaintext reuse is unnecessary.
A regression test should:
- Establish a real TLS 1.2 session layered over a TCP socket with autoClose=false.
- Start a client reader and verify it is blocked inside the TLS read before beginning shutdown; a latch fired before read() plus a sleep is insufficient.
- Keep the peer connected without reading or responding to close_notify.
- Assert that both shutdown and the reader terminate within a bounded time.
- Run shutdown on a separate daemon thread and always close the raw sockets from the test thread in finally, so a regression fails the test without hanging cleanup.
|
Hi @martin-kanis, Thank you martin for your valuable guidance and I have updated the PR accordingly. Teardown: Close the underlying TCP transport before calling tlsResponse.close(), so a pending socket read is interrupted and TLS shutdown cannot wait indefinitely for the read lock. Context cleanup: Keep ldapContext.close() and authCtx.close() protected by finally blocks. Regression test: Add a real TLS 1.2 session layered over loopback TCP with autoClose=false. The test verifies the reader is blocked in the underlying socket read before shutdown, leaves the peer connected without reading, runs shutdown on a daemon thread, checks that both threads terminate within a bound, and closes the raw sockets in the test thread’s finally block. Validation: the 5 focused tests pass on Java 17 and 21; all 37 LDAP module tests pass on Java 17; spotless:check passes. Could you please take another look? thank you so much. |
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
73f1dd8 to
cf01473
Compare
There was a problem hiding this comment.
30 stability reruns of Distribution IT https://github.com/martin-kanis/keycloak/actions/runs/36990030052 didn't reveal any issue.
@atiqur-rahman-pro Thank you!
There was a problem hiding this comment.
Thanks, @atiqur-rahman-pro.
Fixes the StartTLS close-notify hang that causes #51872 and #52603.
Root cause
When JNDI's background reader thread is blocked in an infinite socket read, OpenJDK's
SSLSocketImpllock hierarchy preventsStartTlsResponse.close()from acquiring the lock needed for graceful TLS shutdown. CallingsetSoTimeout()on the SSLSocket does not interrupt an in-flight read. Consequently,StartTlsResponse.close()blocks indefinitely waiting for the read lock, hanging the server thread long enough to causeNoHttpResponseExceptionand test timeouts on Windows CI.Fix & Trade-off
Following maintainer guidance,
LDAPStartTlsClosecloses the captured underlying TCP transport socket first, before callingStartTlsResponse.close().Closing the raw transport socket immediately interrupts the blocked reader with
SocketException: Socket closed, releasing the read lock and guaranteeing that teardown completes without blocking.Trade-off: This prioritizes avoiding hangs over graceful delivery of
close_notifyor LDAPUnbind. Since both callers (LDAPContextManagerandLDAPOperationManager) dispose of the LDAP connection immediately, plaintext socket reuse is unnecessary. Context cleanups (ldapContext.close()andauthCtx.close()) remain protected insidefinallyblocks.Tests
LDAPStartTlsCloseTest#realTlsShutdownUnblocksBlockedReader: Establishes a real TLS 1.2 session layered over loopback TCP withautoClose=false. Verifies the client reader is blocked inside the underlying socket read before shutdown, leaves the peer connected without responding, runs shutdown on a daemon thread, verifies bounded unblocking/termination of both threads, and cleans up raw sockets in the test thread'sfinallyblock.spotless:checkpasses.Closes #51872
Closes #52603