-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
ZOOKEEPER-4790: Make client hostname verification configurable #2173
ZOOKEEPER-4790: Make client hostname verification configurable #2173
Conversation
4bb9978
to
802953e
Compare
Accidentally closed the PR, sorry. Have you considered replacing the current apporach with separate config settings?
|
In a green field I think that makes sense, but I don't think it's worth breaking the old config value "just" for this. |
You can keep backward compatibility by parsing both |
@@ -1739,6 +1739,12 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp | |||
Disabling it only recommended for testing purposes. | |||
Default: true | |||
|
|||
* *ssl.clientHostnameVerification* and *ssl.quorum.clientHostnameVerification* : | |||
(Java system properties: **zookeeper.ssl.clientHostnameVerification** and **zookeeper.ssl.quorum.clientHostnameVerification**) | |||
**New in (INSERT VERSION HERE):** |
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.
3.9.3
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.
Skipped 3.9.3 since it's already released at this point
* *ssl.clientHostnameVerification* and *ssl.quorum.clientHostnameVerification* : | ||
(Java system properties: **zookeeper.ssl.clientHostnameVerification** and **zookeeper.ssl.quorum.clientHostnameVerification**) | ||
**New in (INSERT VERSION HERE):** | ||
Specifies whether the client's hostname verification is enabled in client and quorum TLS negotiation process. |
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.
Please add that this setting will require server hostnameVerification
setting to be true.
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.
@nightkr Ignore my previous comment. Since client hostname verification is bound to server hostname verification setting, it makes sense to keep the original and general |
Sorry about the delay, got distracted by other stuff. |
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.
+1
Reviewers: anmolnar Author: nightkr Closes #2173 from nightkr/feature/config-client-hostname-verification (cherry picked from commit 91ab3f5) Signed-off-by: Andor Molnar <[email protected]>
FIPS mode technically covers this, in a sort of sledgehammery way, but I think it's still worthwhile to have an explicit option for this and only this. Especially since FIPS compliance is a pretty broad thing (see ZOOKEEPER-4832) that will likely expand in the future to cover a lot of things that may or may not be desired.