Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion src/conn.c
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ _makeTLSConn(natsConnection *nc)
s = nats_setError(NATS_SSL_ERROR, "unable to set expected hostname '%s'", nc->tlsName);
}
if (s == NATS_OK)
SSL_set_verify(ssl, SSL_VERIFY_PEER, _collectSSLErr);
SSL_set_verify(ssl, SSL_VERIFY_PEER, nc->opts->sslCtx->callback != NULL ? nc->opts->sslCtx->callback : _collectSSLErr);
}
}
#if defined(NATS_USE_OPENSSL_1_1)
Expand Down
17 changes: 17 additions & 0 deletions src/nats.h
Original file line number Diff line number Diff line change
Expand Up @@ -2594,6 +2594,8 @@ natsOptions_SetExpectedHostname(natsOptions *opts, const char *hostname);
* By default, the server certificate is verified. You can disable the verification

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 am not sure what the discussion you had with @levb and @mtmk was, but I think your first approach may have been a bit better with the abstraction.

This PR would not compile as-is. You would need to add at the top of this file something like:

#if defined(NATS_HAS_TLS)
#include <openssl/ssl.h>
#include <openssl/x509v3.h>
#endif

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.

Also, in CMakeLists.txt of examples, examples/getstarted, examples/stan and test/dylib directories, you would need to add:

if(NATS_BUILD_WITH_TLS)
  include_directories(${OPENSSL_INCLUDE_DIR})
endif(NATS_BUILD_WITH_TLS)

so that those can be built with the openssl include directory.

* by passing <c>true</c> to this function.
*
* \note Setting this to true will clear SSL verfication callback set via natsOptions_SetSSLVerificationCallback().
*
* \warning This is fine for tests but use with caution since this is not secure.
*
* @param opts the pointer to the #natsOptions object.
Expand All @@ -2602,6 +2604,21 @@ natsOptions_SetExpectedHostname(natsOptions *opts, const char *hostname);
NATS_EXTERN natsStatus
natsOptions_SkipServerVerification(natsOptions *opts, bool skip);

typedef struct x509_store_ctx_st X509_STORE_CTX;
typedef int (*SSL_verify_cb)(int preverify_ok, X509_STORE_CTX *x509_ctx);

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.

Remove those 2 lines. This would cause errors otherwise saying that you are redefining them.


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.

This whole function would need to be protected by the #if defined(NATS_HAS_TLS) / #endif because of SSL_verify_cb symbol.

/** \brief Sets the certificate validation callback.
*
* Sets a callback used to verify the SSL certificate.
*
* \note Setting a callback will enable SSL verification if disabled via natsOptions_SkipServerVerification().
*
* @param opts the pointer to the #natsOptions object.
* @param callback the custom SSL verification handler to invoke. see https://docs.openssl.org/master/man3/SSL_CTX_set_verify/
*/
NATS_EXTERN natsStatus
natsOptions_SetSSLVerificationCallback(natsOptions *opts, SSL_verify_cb callback);

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.

This option will not show-up in the generated documentation as it stands. We would need to add to DoxyFile.NATS.Client.in:

diff --git a/doc/DoxyFile.NATS.Client.in b/doc/DoxyFile.NATS.Client.in
index be4b3485..8d70f132 100644
--- a/doc/DoxyFile.NATS.Client.in
+++ b/doc/DoxyFile.NATS.Client.in
@@ -2279,7 +2279,8 @@ INCLUDE_FILE_PATTERNS  =
 # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
 
 PREDEFINED             = BUILD_IN_DOXYGEN \
-                         @NATS_DOC_INCLUDE_STREAMING@
+                         @NATS_DOC_INCLUDE_STREAMING@ \
+                         @NATS_DOC_INCLUDE_TLS@
 
 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
 # tag can be used to specify a list of macro names that should be expanded. The

and in the main CMakeLists.txt:

iMacSynadia:build ivan$ git diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 413e0523..534de81e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -246,6 +246,7 @@ if(NATS_BUILD_WITH_TLS)
   if(NATS_BUILD_TLS_FORCE_HOST_VERIFY)
     add_definitions(-DNATS_FORCE_HOST_VERIFICATION)
   endif(NATS_BUILD_TLS_FORCE_HOST_VERIFY)
+  set(NATS_DOC_INCLUDE_TLS "NATS_HAS_TLS")
 endif(NATS_BUILD_WITH_TLS)

When generating docs, it will update the file DoxyFile.NATS.Client:

diff --git a/doc/DoxyFile.NATS.Client b/doc/DoxyFile.NATS.Client
index 2157d431..acd3a0c6 100644
--- a/doc/DoxyFile.NATS.Client
+++ b/doc/DoxyFile.NATS.Client
@@ -2279,7 +2279,8 @@ INCLUDE_FILE_PATTERNS  =
 # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
 
 PREDEFINED             = BUILD_IN_DOXYGEN \
-                         NATS_HAS_STREAMING
+                         NATS_HAS_STREAMING \
+                         NATS_HAS_TLS
 
 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
 # tag can be used to specify a list of macro names that should be expanded. The

/** \brief Sets the verbose mode.
*
* Sets the verbose mode. If `true`, sends are echoed by the server with
Expand Down
11 changes: 6 additions & 5 deletions src/natsp.h
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,12 @@ typedef struct __natsServerInfo

typedef struct __natsSSLCtx
{
natsMutex *lock;
int refs;
SSL_CTX *ctx;
char *expectedHostname;
bool skipVerify;
natsMutex *lock;
int refs;
SSL_CTX *ctx;
char *expectedHostname;
bool skipVerify;
SSL_verify_cb callback;
Comment thread
kozlovic marked this conversation as resolved.

} natsSSLCtx;

Expand Down
34 changes: 34 additions & 0 deletions src/opts.c
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,35 @@ natsOptions_SkipServerVerification(natsOptions *opts, bool skip)

s = _getSSLCtx(opts);
if (s == NATS_OK)
{
opts->sslCtx->skipVerify = skip;
if (skip)
{
opts->sslCtx->callback = NULL;
}
}

UNLOCK_OPTS(opts);

return s;
}

natsStatus
natsOptions_SetSSLVerificationCallback(natsOptions *opts, SSL_verify_cb callback)
{
natsStatus s = NATS_OK;

LOCK_AND_CHECK_OPTIONS(opts, 0);

s = _getSSLCtx(opts);
if (s == NATS_OK)
{
opts->sslCtx->callback = callback;
if (callback != NULL)
{
opts->sslCtx->skipVerify = false;
}
}

UNLOCK_OPTS(opts);

Expand Down Expand Up @@ -758,6 +786,12 @@ natsOptions_SkipServerVerification(natsOptions *opts, bool skip)
return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR);
}

natsStatus
Comment thread
kozlovic marked this conversation as resolved.
natsOptions_SetSSLVerificationCallback(natsOptions *opts, SSL_verify_cb callback)
{
return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR);
}

#endif

natsStatus
Expand Down
1 change: 1 addition & 0 deletions test/list_test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ _test(SSLMultithreads)
_test(SSLReconnectWithAuthError)
_test(SSLServerNameIndication)
_test(SSLSkipServerVerification)
_test(SSLVerificationCallback)
_test(SSLSocketLeakWithEventLoop)
_test(SSLVerify)
_test(SSLVerifyHostname)
Expand Down
157 changes: 157 additions & 0 deletions test/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -21087,6 +21087,163 @@ void test_SSLSkipServerVerification(void)
#endif
}

#if defined(NATS_HAS_TLS)
static void
_logCert(X509 *cert)
{
char buf[32];
char *subjectName;
char *issuerName;
const ASN1_TIME* asn1NotBefore;
const ASN1_TIME* asn1NotAfter;
struct tm tmNotBefore;
struct tm tmNotAfter;
char *notBefore;
char *notAfter;

if (cert == NULL)
return;

subjectName = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0);
issuerName = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0);

asn1NotBefore = X509_get0_notBefore(cert);
ASN1_TIME_to_tm(asn1NotBefore, &tmNotBefore);
strftime(buf, sizeof(buf), "%F %T", &tmNotBefore);
notBefore = NATS_STRDUP(buf);

asn1NotAfter = X509_get0_notAfter(cert);
ASN1_TIME_to_tm(asn1NotAfter, &tmNotAfter);
strftime(buf, sizeof(buf), "%F %T", &tmNotAfter);
notAfter = NATS_STRDUP(buf);

testf("cert: subject: %s, issuer: %s, notBefore: %s, notAfter: %s\n", subjectName, issuerName, notBefore, notAfter);

OPENSSL_free(subjectName);
OPENSSL_free(issuerName);
NATS_FREE(notBefore);
NATS_FREE(notAfter);
}

static void
_logChain(STACK_OF(X509) *chain)
{
int numElements;
int level = 0;
X509 *cert;

if (chain == NULL)
return;

numElements = sk_X509_num(chain);
for (int i = 0; i < numElements; i++)
{
cert = sk_X509_value(chain, i);
testf("chain level: %d\n", ++level);
_logCert(cert);
}
}

static int
_sslVerifyCallback(int preverify_ok, X509_STORE_CTX *ctx)
{
X509 *cert = X509_STORE_CTX_get_current_cert(ctx);
SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
STACK_OF(X509) *chain = SSL_get_peer_cert_chain(ssl);
const ASN1_TIME *asn1NotAfter;
struct tm tmNotAfter;
char *issuerName;
bool result;
time_t notAfter;
time_t now;

testf("preverify_ok: %d\n", preverify_ok);

if (cert == NULL)
{
test("no cert\n");
return 0;
}
else
{
_logCert(cert);
}

if (chain == NULL)
{
test("no chain\n");
}
else
{
_logChain(chain);
}

asn1NotAfter = X509_get0_notAfter(cert);
ASN1_TIME_to_tm(asn1NotAfter, &tmNotAfter);
notAfter = mktime(&tmNotAfter);
time(&now);
issuerName = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0);

if (notAfter > now)
{
if (strstr(issuerName, "Synadia"))
{
result = 1;
}
else
{
result = preverify_ok;
}
}
else
{
result = 0;
}

OPENSSL_free(issuerName);
testf("verfiy result: %d\n", result);
return result;
}
#endif // NATS_HAS_TLS

void test_SSLVerificationCallback(void)
{
#if defined(NATS_HAS_TLS)
natsStatus s;
natsConnection *nc = NULL;
natsOptions *opts = NULL;
natsPid serverPid = NATS_INVALID_PID;

opts = _createReconnectOptions();
if (opts == NULL)
FAIL("Unable to create reconnect options!");

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

test("Check that connect fails due to server verification: ");
s = natsOptions_SetURL(opts, "nats://127.0.0.1:4443");
IFOK(s, natsOptions_SetSecure(opts, true));
IFOK(s, natsConnection_Connect(&nc, opts));
testCond(s == NATS_SSL_ERROR);
natsConnection_Destroy(nc);

test("Check that connect succeeds with validation callback:\n");
s = natsOptions_SetURL(opts, "nats://127.0.0.1:4443");
IFOK(s, natsOptions_SetSSLVerificationCallback(opts, _sslVerifyCallback));
IFOK(s, natsConnection_Connect(&nc, opts));
testCond(s == NATS_OK);
natsConnection_Destroy(nc);

natsOptions_Destroy(opts);

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

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