Skip to content
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

Jump to bottom

Add SSL cert validation callback #826

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add SSL cert validation callback #826

wants to merge 2 commits into from

Conversation

ckasabula
Copy link

No description provided.

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

@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
Copy link
Author

@levb @mtmk Thoughts on this PR?

kozlovic
kozlovic requested changes Dec 19, 2024
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

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

src/nats.h
@@ -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
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
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.

src/nats.h Outdated
Comment on lines 2607 to 2608
typedef struct x509_store_ctx_st X509_STORE_CTX;
typedef int (*SSL_verify_cb)(int preverify_ok, X509_STORE_CTX *x509_ctx);
Copy link
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.

src/nats.h Outdated
@@ -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
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.

src/natsp.h
SSL_CTX *ctx;
char *expectedHostname;
bool skipVerify;
SSL_verify_cb callback;
Copy link
Member

Choose a reason for hiding this comment

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

Would need to be:

#if defined(NATS_HAS_TLS)
     SSL_verify_cb   callback;
#endif

src/opts.c
@@ -758,6 +786,12 @@ natsOptions_SkipServerVerification(natsOptions *opts, bool skip)
return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR);
}

natsStatus
Copy link
Member

Choose a reason for hiding this comment

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

Would need to be remove, because this is what NATS_HAS_TLS is not defined, so SSL_verify_cb will be unknown.

src/nats.h
*/
NATS_EXTERN natsStatus
natsOptions_SetSSLVerificationCallback(natsOptions *opts, SSL_verify_cb callback);

Copy link
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

@ckasabula
Copy link
Author

@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
Copy link
Author

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

@kozlovic
Copy link
Member

@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
Copy link
Member

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

@ckasabula
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlovic kozlovic kozlovic requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ckasabula @kozlovic