Need to define NATS_HAS_TLS with latest 3.10.0

[thierryba]

Observed behavior

With the latest 3.10, I need to define NATS_HAS_TLS otherwise my program does not even build.

Expected behavior

it should be part of the config or clearly documented.

Server and client version

3.10.0 nats.c. The problem is at compile time.

Host environment

macOS 15, Xcode 16.2, cmake

Steps to reproduce

build nats with TLS support
try to compile a program that include nats/nats.h

[mtmk]

thanks for the report @thierryba. where are you building it? Trying it on ubuntu I don't get any issues:

$ wget https://github.com/nats-io/nats.c/archive/refs/tags/v3.10.0.tar.gz
$ tar zxvf v3.10.0.tar.gz
$ cd nats.c-3.10.0
$ cmake -S . -B build -DNATS_BUILD_STREAMING=NO
$ cmake --build build
[  1%] Building C object src/CMakeFiles/nats.dir/asynccb.c.o
...
[100%] Building CXX object test/check_cpp/CMakeFiles/check_cpp.dir/check_cpp.cpp.o
[100%] Linking CXX executable ../../bin/check_cpp
[100%] Built target check_cpp
$

edit: also worked for me without tls i.e. cmake -S . -B build -DNATS_BUILD_STREAMING=OFF -DNATS_BUILD_WITH_TLS=OFF (thanks @levb)

[levb]

@thierryba Can you please provide the host environment details? And to be clear, you are trying to build without the use of TLS?

[thierryba]

@levb in fact I am building with TLS support (I have openssl). But that macro is defined nowhere. So I need to define it myself tone able to build.

[levb]

Ah, gotcha, thx!

[mtmk]

nats.c/CMakeLists.txt

Line 247 in 61c051f

add_definitions(-DNATS_HAS_TLS)

[thierryba]

Yes exactly, so now I have to do the same in my code. Is this expected?

[kozlovic]

@thierryba We use CMake to build the project. If you don't, it's fine, but you are responsible to add all required build/link commands to your build process.

[thierryba]

I do use cmake too. That's my point. But nothing in the build adding the definition mentioned above. So I have added add_definitions(-DNATS_HAS_TLS) to my own cmakelists.txt in order for my source to build while including nats.h.
And that's the part that is broken if ou ask me.

[kozlovic]

@thierryba I would think that if you build/install the NATS C client library, then you would not need to add that in your own project. This definition has been there for a very long time and there was no report of such issue, so I suspect that you are doing something that others usually don't. If you could detail a bit more how you are building your project and how you reference the NATS C client library?

[thierryba]

well I have myself been using and building nats.c for quite a few years. So something must have change in nats... Will investigate.

[kozlovic]

well I have myself been using and building nats.c for quite a few years. So something must have change in nats... Will investigate.

Thanks! So you are saying that this is new to v3.10.0? I wasn't sure if it was an issue with that release only, or you happened to use that version for the first time, or had your own new project, etc...
If switching to prior release does not exhibit this issue, then indeed, it could be something in our build process that changed? @levb do you remember anything related?

And @thierryba, any chance to post the part of the build that fails to see what is not building without adding the definition?

[thierryba]

@kozlovic

commit c4565fc added this in src/nats.h:
#if defined(NATS_HAS_TLS)
#include <openssl/ssl.h>
#include <openssl/x509v3.h>
#else
#define X509_STORE_CTX void
typedef int (SSL_verify_cb)(int preverify_ok, X509_STORE_CTX x509_ctx);
#endif

and this is new in 3.10
That means that when I include nats.h in my program, it fails because NATS_HAS_TLS is not defined

[kozlovic]

Right, that's not good. I know that someone wanted to add a TLS hostname callback and added an abstraction, but the rest of the team went for using SSL directly, which led to that. That may have been a bad choice. @levb, not sure what we want to do here...

[kozlovic]

I did suggest to add those lines in nats.h because otherwise the PR would not compile (see comment here), but as I suggested at the time, the abstraction approach would likely have been better.

[mtmk]

@lev & @kozlovic how about something similar to what cURL is doing and accept a void callback?

// remove #include <openssl/ssl.h>

typedef void (*NATS_SSL_cb)(void *ssl);

natsOptions_SetSSLVerificationCallback(natsOptions *opts, NATS_SSL_cb callback);

diff --git a/src/conn.c b/src/conn.c
                     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, nc->opts->sslCtx->callback != NULL ? nc->opts->sslCtx->callback : _collectSSLErr);
+            if (s == NATS_OK) {
+                if (nc->opts->sslCtx->callback != NULL) {
+                    nc->opts->sslCtx->callback(ssl);
+                }
+                else {
+                    SSL_set_verify(ssl, SSL_VERIFY_PEER, _collectSSLErr);
+                }
+            }

... then let the user make the appropriate SSL setup calls

#include <openssl/ssl.h>

static void
_sslCtxCb(void *ssl)
{
    SSL_set_verify((SSL *)ssl, SSL_VERIFY_PEER, _sslVerifyCallback);
}

that way we won't have to depend on any openssl definitions.

[levb]

Right. As per #825 (comment). I only looked at at a cursory level, It seemed to me like too many notes, but it was not. The contributor very quickly turned it around with no abstractions, and it got LGTMed and merged.

What do we do now?

1. @mtmk's proposal (hide behind a void *?)
2. Replace the current implementation with the abstracted one, (as first suggested by the contributor?) and cut it as a 3.11.
3. Introduce a new NATS_WITH_TLS_VERIFY_CALLBACK that would imply NATS_HAS_TLS and wrap the new API under that. Make sure it throws an error if attempted to use without the ^^ defined.
4. Other?

[faizol]

I'm having error message : error: unknown type name ‘SSL_verify_cb’
2641 | natsOptions_SetSSLVerificationCallback(natsOptions *opts, SSL_verify_cb callback);

Is this the same problem? My system is Fedora 41

[mtmk]

implementation for proposal (1) is in #858 for consideration.