Skip to content

[FIXED] TLS: Possible hang during handshake #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kozlovic merged 1 commit into from
Sep 10, 2025
Merged

[FIXED] TLS: Possible hang during handshake #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions src/conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,20 +678,16 @@

natsMutex_Lock(nc->opts->sslCtx->lock);

s = natsSock_SetBlocking(nc->sockCtx.fd, true);
if (s == NATS_OK)
ssl = SSL_new(nc->opts->sslCtx->ctx);
if (ssl == NULL)
{
ssl = SSL_new(nc->opts->sslCtx->ctx);
if (ssl == NULL)
{
s = nats_setError(NATS_SSL_ERROR,
"Error creating SSL object: %s",
NATS_SSL_ERR_REASON_STRING);
}
else
{
SSL_set_ex_data(ssl, 0, (void*) nc);
}
s = nats_setError(NATS_SSL_ERROR,

Check warning on line 684 in src/conn.c

View check run for this annotation

Codecov / codecov/patch

src/conn.c#L684 

Added line #L684 was not covered by tests
"Error creating SSL object: %s",
NATS_SSL_ERR_REASON_STRING);
}
else
{
SSL_set_ex_data(ssl, 0, (void*) nc);
}
if (s == NATS_OK)
{
Expand Down Expand Up @@ -749,23 +745,40 @@
{
s = nats_setError(NATS_SSL_ERROR, "unable to set SNI extension for hostname '%s'", nc->cur->url->host);
}
if ((s == NATS_OK) && (SSL_do_handshake(ssl) != 1))
if (s == NATS_OK)
{
// check if there is already set NATS_SSL_ERROR from _sslCertCallback
nats_GetLastError(&s);
if (s != NATS_SSL_ERROR)
int hsErr = 0;

DO_HANDSHAKE:
hsErr = SSL_do_handshake(ssl);
if (hsErr != 1)
{
s = nats_setError(NATS_SSL_ERROR,
"SSL handshake error: %s",
(nc->errStr[0] != '\0' ? nc->errStr : NATS_SSL_ERR_REASON_STRING));
int sslErr = SSL_get_error(ssl, hsErr);
if ((sslErr == SSL_ERROR_WANT_READ) || (sslErr == SSL_ERROR_WANT_WRITE))
{
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;

@mtmk mtmk Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(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.

@kozlovic kozlovic Sep 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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).

@mtmk mtmk Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

}
else
{
// check if there is already set NATS_SSL_ERROR from _sslCertCallback
nats_GetLastError(&s);
if (s != NATS_SSL_ERROR)
{
s = nats_setError(NATS_SSL_ERROR,
"SSL handshake error: %s",
(nc->errStr[0] != '\0' ? nc->errStr : NATS_SSL_ERR_REASON_STRING));
}
}
}
}
// Make sure that if nc-errStr was set in _collectSSLErr but
// the overall handshake is ok, then we clear the error
if (s == NATS_OK)
{
nc->errStr[0] = '\0';
s = natsSock_SetBlocking(nc->sockCtx.fd, false);
}

natsMutex_Unlock(nc->opts->sslCtx->lock);
Expand Down
1 change: 1 addition & 0 deletions test/list_test.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ _test(SSLCertAndKeyFromMemory)
_test(SSLCiphers)
_test(SSLConnectVerboseOption)
_test(SSLHandshakeFirst)
_test(SSLHandshakeTimeout)
_test(SSLLoadCAFromMemory)
_test(SSLMultithreads)
_test(SSLReconnectWithAuthError)
Expand Down
204 changes: 204 additions & 0 deletions test/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22073,6 +22073,210 @@ void test_SSLHandshakeFirst(void)
#endif
}

struct testSP
{
natsSock sock1;
natsSock sock2;
};

static void
_sslProxyExData(void *closure)
{
natsStatus s = NATS_OK;
struct testSP *sp = (struct testSP*) closure;
natsSockCtx ctx1;
natsSockCtx ctx2;
char buffer[100];

memset(&ctx1, 0, sizeof(natsSockCtx));
memset(&ctx2, 0, sizeof(natsSockCtx));

ctx1.fd = sp->sock1;
ctx2.fd = sp->sock2;

while (s == NATS_OK)
{
int n = 0;
s = natsSock_Read(&ctx1, buffer, sizeof(buffer), &n);
IFOK(s, natsSock_WriteFully(&ctx2, buffer, n));
}
}

static void
_sslProxy(void *closure)
{
natsStatus s = NATS_OK;
natsSock sock = NATS_SOCK_INVALID;
struct threadArg *arg = (struct threadArg*) closure;
int mode;
natsSockCtx ctx;
natsSockCtx srv;

memset(&ctx, 0, sizeof(natsSockCtx));
memset(&srv, 0, sizeof(natsSockCtx));

_startMockupServer(&sock, "127.0.0.1", "4444");

for (mode = 1; (s == NATS_OK) && (mode <=2); mode++)
{
if (((ctx.fd = accept(sock, NULL, NULL)) == NATS_SOCK_INVALID)
|| (natsSock_SetCommonTcpOptions(ctx.fd) != NATS_OK))
{
s = NATS_SYS_ERROR;
break;
}
natsMutex_Lock(arg->m);
if (mode == 1)
{
while ((s != NATS_TIMEOUT) && !arg->done)
s = natsCondition_TimedWait(arg->c, arg->m, 5000);

if (s != NATS_OK)
arg->status = s;

arg->done = false;
natsMutex_Unlock(arg->m);
natsSock_Close(ctx.fd);
ctx.fd = NATS_SOCK_INVALID;
continue;
}
natsMutex_Unlock(arg->m);
// In this mode, we will just read and write in small chunks.
s = natsSock_ConnectTcp(&srv, "127.0.0.1", 4443);
if (s == NATS_OK)
{
natsThread *t1 = NULL;
natsThread *t2 = NULL;
struct testSP sp1;
struct testSP sp2;

natsSock_SetBlocking(ctx.fd, true);
natsSock_SetBlocking(srv.fd, true);

sp1.sock1 = ctx.fd;
sp1.sock2 = srv.fd;

sp2.sock1 = srv.fd;
sp2.sock2 = ctx.fd;

s = natsThread_Create(&t1, _sslProxyExData, &sp1);
IFOK(s, natsThread_Create(&t2, _sslProxyExData, &sp2));

natsMutex_Lock(arg->m);
while ((s != NATS_TIMEOUT) && !arg->done)
s = natsCondition_TimedWait(arg->c, arg->m, 5000);
natsMutex_Unlock(arg->m);

natsSock_Shutdown(ctx.fd);
natsSock_Shutdown(srv.fd);

if (t1 != NULL)
{
natsThread_Join(t1);
natsThread_Destroy(t1);
}
if (t2 != NULL)
{
natsThread_Join(t2);
natsThread_Destroy(t2);
}

natsSock_Close(ctx.fd);
natsSock_Close(srv.fd);
}
}
natsSock_Close(sock);

if (s != NATS_OK)
{
natsMutex_Lock(arg->m);
arg->status = s;
natsMutex_Unlock(arg->m);
}
}

void test_SSLHandshakeTimeout(void)
{
#if defined(NATS_HAS_TLS)
natsStatus s;
natsConnection *nc = NULL;
natsOptions *opts = NULL;
natsPid serverPid = NATS_INVALID_PID;
natsThread *t = NULL;
struct threadArg arg;

s = _createDefaultThreadArgsForCbTests(&arg);
if (s != NATS_OK)
FAIL("Unable to setup test");

serverPid = _startServer("nats://127.0.0.1:4443", "-config tlsfirst.conf", true);
CHECK_SERVER_STARTED(serverPid);

test("Start proxy: ");
s = natsThread_Create(&t, _sslProxy, &arg);
testCond(s == NATS_OK);

test("Set options: ");
s = natsOptions_Create(&opts);
// Point to the proxy.
IFOK(s, natsOptions_SetURL(opts, "nats://127.0.0.1:4444"));
IFOK(s, natsOptions_SetSecure(opts, true));
IFOK(s, natsOptions_SkipServerVerification(opts, true));
IFOK(s, natsOptions_TLSHandshakeFirst(opts));
IFOK(s, natsOptions_SetTimeout(opts, 500));
testCond(s == NATS_OK);

test("SSL handshake should timeout: ");
s = natsConnection_Connect(&nc, opts);
// We expect a failure, but make sure it fails because of the
// SSLHandshake timing out, not because the proxy timed out.
if (s != NATS_OK)
{
nats_clearLastError();
s = NATS_OK;
natsMutex_Lock(arg.m);
s = arg.status;
natsMutex_Unlock(arg.m);
}
testCond((s == NATS_OK) && (nc == NULL));

test("Release proxy: ");
natsMutex_Lock(arg.m);
arg.done = true;
natsCondition_Broadcast(arg.c);
natsMutex_Unlock(arg.m);
testCond(s == NATS_OK);

test("SSL handshake should work: ");
s = natsConnection_Connect(&nc, opts);
testCond((s == NATS_OK) && (nc != NULL));

natsConnection_Destroy(nc);
natsOptions_Destroy(opts);

test("Stop proxy: ");
natsMutex_Lock(arg.m);
arg.done = true;
natsCondition_Broadcast(arg.c);
natsMutex_Unlock(arg.m);
if (t != NULL)
{
natsThread_Join(t);
natsThread_Destroy(t);
}
natsMutex_Lock(arg.m);
s = arg.status;
natsMutex_Unlock(arg.m);
testCond(s == NATS_OK);

_destroyDefaultThreadArgs(&arg);
_stopServer(serverPid);
#else
test("Skipped when built with no SSL support: ");
testCond(true);
#endif
}

void test_SSLServerNameIndication(void)
{
#if defined(NATS_HAS_TLS)
Expand Down