Skip to content

Add SSL cert validation callback#825

Closed
ckasabula wants to merge 1 commit into
nats-io:mainfrom
ckasabula:ck/ssl_verification_callback
Closed

Add SSL cert validation callback#825
ckasabula wants to merge 1 commit into
nats-io:mainfrom
ckasabula:ck/ssl_verification_callback

Conversation

@ckasabula

Copy link
Copy Markdown
Contributor

No description provided.

@ckasabula
ckasabula force-pushed the ck/ssl_verification_callback branch 7 times, most recently from 88fa0ee to 9818f3d Compare December 11, 2024 22:15
@ckasabula
ckasabula force-pushed the ck/ssl_verification_callback branch from 9818f3d to adddcc7 Compare December 11, 2024 23:04
Comment thread src/cert.c

#if defined(NATS_HAS_TLS)
natsStatus
natsCert_create(natsCert **newCert, X509 *x509Cert)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ckasabula What is the extra value-add of the natsCert and natsCertChain abstractions, they appear to contain the data that is already present in the X509 structs? (relative to implementing the verification callback with the X509 structs instead?)

@ckasabula ckasabula Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@levb
I could change the callback from what I have currently:
typedef bool (*CertificateValidationCB)(int preverify_ok, natsCert *cert, natsCertChain *chain);
to
typedef bool (*CertificateValidationCB)(int preverify_ok, X509 *cert, STACK_OF(X509) *chain);

Problem with that is that those openssl types would need to get pulled into nats.h (wrapped with #if defined(NATS_HAS_TLS)). I added those wrappers, so clients wouldn't have to write any openssl code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think client code would also have to link with openssl too. Adding the wrappers for cert and chain, keeps clients isolated from underlying SSL impl.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ckasabula Conversely, there is already a SSL_CTX_set_verify[_callback] in openSSL; 1/5 we should target enabling the "idiomatic" functionality in this case, rather than adding our own layer. I briefly chatted with @mtmk on this, and he volunteered to look into other implementations and see what path we want to take here.

@ckasabula ckasabula Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nats.c already implements the callback via SSL_set_verify. I'm calling the client callback from the openssl callback in conn.c.

https://github.com/nats-io/nats.c/blob/main/src/conn.c#L739

https://github.com/ckasabula/nats.c/blob/ck/ssl_verification_callback/src/conn.c#L642

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@levb @mtmk
In nats.net cert callback is called with the dotnet "idiomatic" X509Certificate and X509Chain, both from System.Security.Cryptography.X509Certificates. In dotnet these types are included with the framework. In C with headers/libs it's more challenging.
I'm open to having the callback use openssl types. Would you both/either of you be open to a short call to discuss?

@ckasabula ckasabula Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could have the client provide their own callback, which would replace _collectSSLErr when specified. That would be more in line with how the dotnet client works. It would be passed directly to SSL_set_verify.

@ckasabula ckasabula closed this Dec 12, 2024
@ckasabula

Copy link
Copy Markdown
Contributor Author

Based on feedback, closing this PR and will open a new one with a different approach.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants