[FIXED] TLS: Possible hang during handshake by kozlovic · Pull Request #907 · nats-io/nats.c

kozlovic · 2025-09-10T15:56:20Z

The reason was that we switched the connection to blocking mode for the SSL handshake duration. In some cases, this could lead to the connection being blocked in the handshake for too long (longer than the provided connection timeout).

We now don't switch to blocking mode and check if SSL_do_handshake wants a read or write and wait for such event to be available.

Resolves #906

Signed-off-by: Ivan Kozlovic ivan@synadia.com

The reason was that we switched the connection to blocking mode for the SSL handshake duration. In some cases, this could lead to the connection being blocked in the handshake for too long (longer than the provided connection timeout). We now don't switch to blocking mode and check if `SSL_do_handshake` wants a read or write and wait for such event to be available. Resolves #906 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

codecov · 2025-09-10T17:37:57Z

Codecov Report

❌ Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.20%. Comparing base (a4f0e0d) to head (9ec900b).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/conn.c	58.82%	1 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #907      +/-   ##
==========================================
- Coverage   70.23%   70.20%   -0.04%     
==========================================
  Files          48       48              
  Lines       17236    17242       +6     
  Branches     3538     3540       +2     
==========================================
- Hits        12106    12105       -1     
- Misses       1726     1734       +8     
+ Partials     3404     3403       -1

☔ View full report in Codecov by Sentry.
📢 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.

mtmk

LGTM (with one minor comment for future)

mtmk · 2025-09-10T19:11:51Z

+                int waitMode = (sslErr == SSL_ERROR_WANT_READ ? WAIT_FOR_READ : WAIT_FOR_WRITE);
+
+                if ((s = natsSock_WaitReady(waitMode, &(nc->sockCtx))) == NATS_OK)
+                    goto DO_HANDSHAKE;


(optional) although infinite loop seems unlikely, a loop counter would be a nice to have, perhaps as a follow up pr later on if you agree as well.

Is it needed though? I mean, we should fail on the deadline being reached (that is, natsSock_WaitReady() would ultimately return NATS_TIMEOUT) and so we break out of the loop. (note: there will be always a connection timeout - which will set the deadline - either the default 2 seconds or whatever user specifies through option).

i was thinking odd cases like deadline being null or something but it is fairly tight. it's fine as it is. no need for a follow up. thanks for the explanation.

…fix_906

Strengthen the test added in PR #907. Also improved the error returned during a SSL handshake failure. Relates to #907 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

@mtmk

Release notes will be: This release contains some breaking changes. See the "Changed" section below. * Build * Disable NATS Streaming by default by @mtmk in #770 * TLS * Require OpenSSL 1.1.1+ to compile. Removed the `NATS_BUILD_TLS_USE_OPENSSL_1_1_API` CMake variable by @kozlovic in #905 * The option `natsOptions_SetSSLVerificationCallback` signature was changed to replace the use of `SSL_verify_cb` (which required OpenSSL dependency in the `nats.h` file), to the new callback `natsSSLVerifyCb`. See documentation of `natsSSLVerifyCb` to see the cast needed to compile with this new header file by @kozlovic in #908 * Modification of a `natsOptions` object if it has TLS/SSL configuration and is actively used by connections will now return a `NATS_ILLEGAL_STATE` by @kozlovic in #912 * Options * Ability to load the trusted CA certificates from a directory using the new option `natsOptions_LoadCATrustedCertificatesPath` by @kerbert101 in #862 * Ability to connect via HTTP proxy for instance by adding a proxy connection handler using the new option `natsOptions_SetProxyConnHandler` by @wolfkor in #871 and @kozlovic in #897 * Ability to load the certificate chain and key from a file on every connection attempt using the new option `natsOptions_LoadCertificatesChainDynamic` by @Matus-p in #901 * Ability to perform concurrent TLS handshakes that may improve time it takes for concurrent connections to be established using the new option `natsOptions_AllowConcurrentTLSHandshakes ` by @kozlovic in #914. Issue was reported by @yanyongcheng in #899 * JetStream * Per-message TTL support (a NATS Server v2.11 feature) by @levb in #863 * Pull consumer priority groups (a NATS Server v2.11 feature) by @levb in #869 * ObjectStore support by @kozlovic in #902. Thanks to @jfflynn41 and @alex1891 for the feedback in #876 * JetStream * Handling of publish asynchronous timeouts by @kozlovic in #886. Issue reported by @yanyongcheng in #880 * Timer insertion by @kozlovic in #883. Issue reported by @yanyongcheng in #881 * EventLoop: * Handling of possible failure on initial attach by @kozlovic in #918 * LibEvent: `natsConnection_Close()` not closing the TCP connection by @kozlovic in #882. Issue was reported by @yanyongcheng in #879 * Libuv: Possible crash if connection is destroyed while receiving data by @kozlovic in #889. Issue was reported by @yanyongcheng in #888 * KeyValue * Keys, History or watcher's next may incorrectly return `NATS_TIMEOUT` by @kozlovic in #917/ Issue was reported by @ArashPartow in #916 * MicroServices: * Wrong marshaling of `average_processing_time` by @kozlovic in #892. Issue was reported by @Archie3d in #890 * Statistics error was always incremented by @kozlovic in #894. Issue was reported by @Archie3d in #893 * TLS * Unknown type name `SSL_verify_cb` by @kozlovic in #878. Issue was reported by @philipfoulkes in #877 * Initialization and cleanup code related to OpenSSL was removed since it was deprecated for versions post OpenSSL 1.1. A cleanup function pertinent to 1.1+ code was possibly causing a problem. By @kozlovic in #905. Issue was reported by @vdeters in #904 * Possible hang during handshake by @kozlovic in #907. Issue was reported by @etrochim in #906 * Protect calls to `SSL_read` and `SSL_write` with a mutex. Since the same `SSL` object is shared between different threads, the OpenSSL library requires a mutex to be used by @kozlovic in #913 * Memory allocation check by @wooffie in #868 * Add missing status text string by @oldnick85 in #872 and @kozlovic in #874 (the issue was not present in any published release and was introduced in #869) * Parsing of message headers with `NULL` or all-whitespace values by @habbbe in #873 * Removed some unused code related to handling of responses and added custom inbox with very long prefix test by @kozlovic in #885. Issue was reported by @yanyongcheng in #884 * Connection drain could cause missed reply and/or a 100ms delay by @kozlovic in #915. Issue was reported by @T-Maxxx in #911 * Build * Deprecated Ubuntu 20.04 in GitHub actions by @levb in #864 * Removed the older compiler jobs by @levb in #865 * Fixed Windows build to use the NATS Server main branch by @levb in #866 * @kerbert101 made their first contribution in #862 * @habbbe made their first contribution in #873 * @wolfkor made their first contribution in #871 * @Matus-p made their first contribution in #901 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

kozlovic force-pushed the fix_906 branch from 16af306 to 9ec900b Compare September 10, 2025 16:15

kozlovic requested a review from mtmk September 10, 2025 16:29

mtmk approved these changes Sep 10, 2025

View reviewed changes

kozlovic merged commit ea2b716 into main Sep 10, 2025
45 of 50 checks passed

kozlovic deleted the fix_906 branch September 10, 2025 21:25

github-actions Bot pushed a commit that referenced this pull request Sep 10, 2025

[skip ci] Update docs: ea2b716: Merge pull request #907 from nats-io/…

eeedc10

…fix_906

kozlovic mentioned this pull request Sep 12, 2025

Fixed SSLHandshakeTimeout test to prevent flapping #910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FIXED] TLS: Possible hang during handshake#907

[FIXED] TLS: Possible hang during handshake#907
kozlovic merged 1 commit into
mainfrom
fix_906

kozlovic commented Sep 10, 2025

Uh oh!

codecov Bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

mtmk left a comment

Uh oh!

mtmk Sep 10, 2025

Uh oh!

kozlovic Sep 10, 2025

Uh oh!

mtmk Sep 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kozlovic commented Sep 10, 2025

Uh oh!

codecov Bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mtmk left a comment

Choose a reason for hiding this comment

Uh oh!

mtmk Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

kozlovic Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

mtmk Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Sep 10, 2025 •

edited

Loading