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.

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

Optionally enable TLS connection #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MAHashmi
Copy link

Adds the possibility to use a tls socket instead of net socket instance,
if a tls connection is desired.

Adds the possibility to use a tls socket instead of net socket instance,
if a tls connection is desired.
@MAHashmi
Copy link
Author

Reference: #40

I have tested SSL connection between client and server with this change. I can add further documentation on the usage, if this pull request gets any attention. It would be really helpful to have it merged in the main master as opposed to my fork.

@alexguan
Copy link
Owner

There are more work than just enabling TLS connections. In Zookeeper Java Client, the TLS connection always comes with the ability to add the client certs to the Auth data so the server can authenticated using mTLS.

@MAHashmi
Copy link
Author

Thanks for the response. I am not sure I understand your point. In case mTLS is desired, the client cert and CA can be provided as an argument to tls.connect(). If rejectUnauthorized flag is enabled it would ensure mTLS. For such a case the opts for createClient would look something like this:

opts = {
    tls: {
        ca: // ca file
        key: // client key
        cert: // client cert
        rejectUnauthorized: true
    }
};

@MAHashmi
Copy link
Author

@alexguan Any thoughts on this? I can implement any additional functionality that you think might be missing. Would love to hear your feedback.

@MAHashmi
Copy link
Author

MAHashmi commented Oct 8, 2019

@alexguan ???

@sirmonin
Copy link

sirmonin commented Nov 17, 2020

@alexguan any news on this?

@EddieBenji
Copy link

Hi @alexguan
Is this change planned to be merged one day?

Thank you

@nokcm
Copy link

nokcm commented Jun 25, 2024

Hi @alexguan
Can we look into merging the PR? In my use-case I neeD the mTLS conneciton

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.

6 participants