Add SSL cert validation callback by ckasabula · Pull Request #826 · nats-io/nats.c

ckasabula · 2024-12-12T22:55:57Z

No description provided.

ckasabula · 2024-12-12T23:44:22Z

@levb @mtmk Different approach this time based on your comments in my previous PR. This time exposing native, openssl verification callback. Smaller PR and no abstractions for cert, chain.

ckasabula · 2024-12-16T13:35:47Z

@levb @mtmk Thoughts on this PR?

kozlovic

@levb @mtmk We may reconsider the abstraction to avoid all the requested changes, but if not, then the mentioned changes here would be required to compile and generate proper documentation.

kozlovic · 2024-12-19T17:49:14Z

@@ -2594,6 +2594,8 @@ natsOptions_SetExpectedHostname(natsOptions *opts, const char *hostname);
 * By default, the server certificate is verified. You can disable the verification


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

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.

kozlovic · 2024-12-19T17:49:53Z

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


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

kozlovic · 2024-12-19T17:50:58Z


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


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

kozlovic · 2024-12-19T17:58:00Z

+ */
+NATS_EXTERN natsStatus
+natsOptions_SetSSLVerificationCallback(natsOptions *opts, SSL_verify_cb callback);
+


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

ckasabula · 2024-12-19T18:03:37Z

@kozlovic

@levb @mtmk We may reconsider the abstraction to avoid all the requested changes, but if not, then the mentioned changes here would be required to compile and generate proper documentation.

This was the original PR w/ the abstraction for cert and cert chain:
#825

ckasabula · 2024-12-19T18:11:03Z

@kozlovic I went with the abstraction originally so the client wouldn't need to include openssl headers and link w/ openssl.

kozlovic · 2024-12-19T18:17:04Z

@kozlovic I went with the abstraction originally so the client wouldn't need to include openssl headers and link w/ openssl.

Not sure what you mean by the client but the library does include and link with openssl, and the application would too, but it would be indirectly indeed.

kozlovic · 2024-12-19T18:17:43Z

I will let @levb and @mtmk have a look and decide what is the next step.

ckasabula · 2024-12-19T18:36:24Z

@kozlovic I mean the code/application that uses the library.

@kozlovic I went with the abstraction originally so the client wouldn't need to include openssl headers and link w/ openssl.

Not sure what you mean by the client but the library does include and link with openssl, and the application would too, but it would be indirectly indeed.

ckasabula · 2024-12-23T17:17:36Z

@kozlovic I believe I have addressed your comments. I also updated the docs, which resulted in a lot of diffs. Let me know what you think.

kozlovic

Please review my 2 comments and remove the last commit that updated the "doc/html". This will be done automatically if/when the PR is merged.

kozlovic

If we are going with this approach, then LGTM, but will let @levb and @mtmk decide if this should be merged or not.

mtmk · 2025-01-30T13:00:27Z

LGTM thanks @ckasabula (sorry for getting back to you late)

... just a couple of general comments mostly to share what I learned about using the OpenSSL apis. will leave the last word to @levb

Usage

API wise it's pretty simple:

#include <nats.h>

int verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
{
  // good example in test.c
  return 1;
}

natsOptions_SetSSLVerificationCallback(opts, verify_callback);

Callback definition comes from OpenSSL but I think that's practical enough since we have to expose X509_STORE_CTX which is part of OpenSSL anyway. nice and simple.

Comparison to Other Libraries

For comparison I had a quick look at curl api which looks somewhat similar except they provide a callback to setup SSL_CTX where then you can assign the callback hooking into SSL_CTX_set_verify() rather than SSL_set_verify() which is what we're using internally per connection.

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

int verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
{
  // same as above
  return 1;
}

CURLcode sslctx_function(CURL *curl, void *sslctx, void *parm) {
    SSL_CTX *ctx = (SSL_CTX *)sslctx;

    SSL_CTX_set_verify((SSL_CTX *)sslctx, SSL_VERIFY_PEER, verify_callback);

    return CURLE_OK;
}

curl_easy_setopt(curl, CURLOPT_SSL_CTX_FUNCTION, sslctx_function);

Notably, different to curl api, we are calling SSL_set_verify() every time we reconnect which might be seen as unnecessary, since we can't swap out the callback between reconnects. I don't think there is a use case to change callbacks in between reconnects, so it's not really an issue.

In my opinion with, this PR we are introducing a simple way of setting the callback up and if/when needed, in a follow up PR or in the future we might want to consider exposing a callback to setup SSL_CTX similar to curl during the connection creation phase.

levb

Thanks @ckasabula for your contribution and persistence, and everyone for the reviews/inputs!

…s-io#826)

ckasabula · 2025-01-30T21:00:19Z

Thanks everyone.

Add SSL cert validation callback

8df350f

ckasabula force-pushed the ck/ssl_verification_callback_take2 branch from ea88eca to 8df350f Compare December 12, 2024 23:30

kozlovic requested changes Dec 19, 2024

View reviewed changes

kozlovic requested changes Dec 23, 2024

View reviewed changes

Comment thread doc/DoxyFile.NATS.Client.in

Comment thread src/nats.h

Comment thread examples/getstarted/CMakeLists.txt

ckasabula force-pushed the ck/ssl_verification_callback_take2 branch 3 times, most recently from 562bfe6 to d030d1d Compare January 7, 2025 14:55

Address comments

316ea64

ckasabula force-pushed the ck/ssl_verification_callback_take2 branch from d030d1d to 316ea64 Compare January 7, 2025 16:54

kozlovic approved these changes Jan 7, 2025

View reviewed changes

mtmk approved these changes Jan 30, 2025

View reviewed changes

levb approved these changes Jan 30, 2025

View reviewed changes

levb merged commit c4565fc into nats-io:main Jan 30, 2025

github-actions Bot pushed a commit that referenced this pull request Jan 30, 2025

[skip ci] Update docs: c4565fc: Add SSL cert validation callback (#826)

96efd95

github-actions Bot pushed a commit to levb/nats.c that referenced this pull request Jan 30, 2025

[skip ci] Update docs: c4565fc: Add SSL cert validation callback (nat…

cf4ba22

…s-io#826)

levb mentioned this pull request Feb 28, 2025

Release v3.10 #853

Merged

kozlovic mentioned this pull request Mar 4, 2025

Need to define NATS_HAS_TLS with latest 3.10.0 #857

Closed

levb mentioned this pull request Mar 12, 2025

[FIXED] make natsOptions_SetSSLVerificationCallback experimental #860

Merged

		@@ -2594,6 +2594,8 @@ natsOptions_SetExpectedHostname(natsOptions opts, const char hostname);
		* By default, the server certificate is verified. You can disable the verification

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

Uh oh!

Conversation

ckasabula commented Dec 12, 2024

Uh oh!

ckasabula commented Dec 12, 2024

Uh oh!

ckasabula commented Dec 16, 2024

Uh oh!

kozlovic left a comment

Choose a reason for hiding this comment

Uh oh!

kozlovic Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

kozlovic Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

kozlovic Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

kozlovic Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kozlovic Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

ckasabula commented Dec 19, 2024

Uh oh!

ckasabula commented Dec 19, 2024

Uh oh!

kozlovic commented Dec 19, 2024

Uh oh!

kozlovic commented Dec 19, 2024

Uh oh!

ckasabula commented Dec 19, 2024

Uh oh!

ckasabula commented Dec 23, 2024

Uh oh!

kozlovic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kozlovic left a comment

Choose a reason for hiding this comment

Uh oh!

mtmk commented Jan 30, 2025

Usage

Comparison to Other Libraries

Uh oh!

levb left a comment

Choose a reason for hiding this comment

Uh oh!

ckasabula commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants