-
Notifications
You must be signed in to change notification settings - Fork 7.3k
ZOOKEEPER-4929: Make c client side cert optional in connecting to tls server #2257
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
base: master
Are you sure you want to change the base?
ZOOKEEPER-4929: Make c client side cert optional in connecting to tls server #2257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Would you mind test this and document zookeeper_init_ssl
somehow ?
LOG_ERROR(LOGCALLBACK(zh), "SSL_CTX_check_private_key failed"); | ||
errno = EINVAL; | ||
return ZBADARGUMENTS; | ||
if (fd->cert->cert != NULL && fd->cert->passwd != NULL && fd->cert->key != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe need SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_NONE, NULL);
in client when skip cert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not particularly familiar with the SSL APIs, but my assumption was that we would be in client mode, and still want to validate the server certificate so the existing settings are still correct. We only want the TLS handshake to continue if ths server certificate is valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a server side option ssl.clientAuth
.
I plan to construct a test for this pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eseabrook1, I created eseabrook1#1 to add test case for this pr. Would you mind take a look ? This pr will be updated automatically once you merged that pr.
ZOOKEEPER_4929: Add test case for server only tls
In the Zookeeper C library it is possible to initiate a connection using SSL by providing a "cert" string to zookeeper_init_ssl(). However in order to call this function, it is my understanding that callers must provide four things:
This understanding is based on the implementation of init_ssl_for_socket
zookeeper/zookeeper-client/zookeeper-client-c/src/zookeeper.c
Lines 2758 to 2793 in b86ccf1
For our use case, connecting to a server that does not support mTLS, it would be useful if we could specify only the CA for the server certificate, omitting the client parameters completely. This is something this is already possible with other Zookeeper client libraries, for example Kazoo: https://github.com/python-zk/kazoo/blob/c5ab98819b3a797e12a0315e97e51851525da70f/kazoo/handlers/utils.py#L253-L260
This Pull Request proposes a change to relax the requirements for the client SSL certificates and allow just a sever certificate to be provided.