{"action":"created","issue":{"url":"https://api.github.com/repos/apache/zookeeper/issues/627","repository_url":"https://api.github.com/repos/apache/zookeeper","labels_url":"https://api.github.com/repos/apache/zookeeper/issues/627/labels{/name}","comments_url":"https://api.github.com/repos/apache/zookeeper/issues/627/comments","events_url":"https://api.github.com/repos/apache/zookeeper/issues/627/events","html_url":"https://github.com/apache/zookeeper/pull/627","id":360438351,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE1Njk4MzE0","number":627,"title":" ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol [part 2]","user":{"login":"ivmaykov","id":682290,"node_id":"MDQ6VXNlcjY4MjI5MA==","avatar_url":"https://avatars0.githubusercontent.com/u/682290?v=4","gravatar_id":"","url":"https://api.github.com/users/ivmaykov","html_url":"https://github.com/ivmaykov","followers_url":"https://api.github.com/users/ivmaykov/followers","following_url":"https://api.github.com/users/ivmaykov/following{/other_user}","gists_url":"https://api.github.com/users/ivmaykov/gists{/gist_id}","starred_url":"https://api.github.com/users/ivmaykov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ivmaykov/subscriptions","organizations_url":"https://api.github.com/users/ivmaykov/orgs","repos_url":"https://api.github.com/users/ivmaykov/repos","events_url":"https://api.github.com/users/ivmaykov/events{/privacy}","received_events_url":"https://api.github.com/users/ivmaykov/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2018-09-14T20:09:07Z","updated_at":"2018-10-15T21:42:31Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/apache/zookeeper/pulls/627","html_url":"https://github.com/apache/zookeeper/pull/627","diff_url":"https://github.com/apache/zookeeper/pull/627.diff","patch_url":"https://github.com/apache/zookeeper/pull/627.patch"},"body":"# Overview\r\nThese are fixes and improvements to #184 that we've coded up at Facebook while testing that PR internally. This is meant to be in addition to #184, and should be stacked on top of it (and in fact includes the commits from that pull request). Most notable changes:\r\n\r\n## Fixed networking issues/bugs in UnifiedServerSocket\r\n- don't crash the accept() thread if the client closes the connection without sending any data\r\n- don't corrupt the connection if the client sends fewer than 5 bytes for the initial read\r\n- delay the detection of TLS vs. plaintext mode until a socket stream is read from or written to. This prevents the accept() thread from getting blocked on a `read()` operation from the newly connected socket.\r\n- prepending 5 bytes to `PrependableSocket` and then trying to read >5 bytes would only return the first 5 bytes, even if more bytes were available. This is fixed.\r\n\r\n## Added support for PEM formatted key stores and trust stores\r\n- key store and trust store files can now be in PEM format as well as JKS.\r\n- Added config properties to tell ZK what type of trust/key store to load:\r\n- `zookeeper.ssl.keyStore.type` and `zookeeper.ssl.trustStore.type` for ClientX509Util\r\n- `zookeeper.ssl.quorum.keyStore.type` and `zookeeper.ssl.quorum.trustStore.type` for QuorumX509Util\r\n- store type properties could have the values \"JKS\", \"PEM\", or not set\r\n- leaving the type properties unset will cause auto-detection of the store type based on the file extension (\".jks\" or \".pem\")\r\n\r\n## Added support for reloading key/trust stores when the file on disk changes\r\n- new property `sslQuorumReloadCertFiles` which controls the behavior for reloading the key and trust store files for `QuorumX509Util`. Reloading of key and trust store for `ClientX509Util` is not in this PR but could be added easily\r\n- this allows a ZK server to keep running on a machine that uses short-lived certs that refresh frequently without having to restart the ZK process.\r\n\r\n## Added test utilities for easily creating X509 certs and using them in unit tests\r\n- added new class `X509TestContext` and its friend, `X509TestHelpers`\r\n- rewrote some existing unit tests to use these classes, and added new tests that use them\r\n- some existing tests (i.e. `QuorumSSLTest`) should probably be ported to use this as well, haven't got around to it yet\r\n\r\n## Added more options for ssl settings to X509Util and encapsulate them better\r\n- previously, some SSL settings came from a ZKConfig and others came from global `System.getProperties()`. This made it hard to isolate certain settings in tests.\r\n- now all SSL-related settings come from the ZKConfig object used to create the context\r\n- new settings added:\r\n- `zookeeper.ssl(.quorum).enabledProtocols` - list of enabled protocols. If not set, defaults to a single-entry list with the value of `zookeeper.ssl(.quorum).protocol`.\r\n- `zookeeper.ssl(.quorum).clientAuth` - can be \"NONE\", \"WANT\", or \"NEED\". This controls whether the server doesn't want / allows / requires the client to present an X509 certificate.\r\n- `zookeeper.ssl(.quorum).handshakeDetectionTimeoutMillis` - timeout for the first read of 5 bytes to detect the transport mode (TLS or plaintext) of a client connection made to a `UnifiedServerSocket`\r\n\r\n## Fixed odd plaintext perf regression caused by new dependency on `org.apache.httpcomponents`\r\n- internal regression testing showed a >10% perf degradation in plaintext mode that was tracked down to the addition of the `org.apache.httpcomponents` dependency.\r\n- Removing the dependency and implementing the hostname verification by mostly copy-pasting the hostname verification code from `httpcomponents` fixed the regression.\r\n\r\nThere may be some other changes that I'm forgetting, but those are the main ones.\r\n\r\nWe believe that #184 should not be accepted without this PR coming close behind, mainly because of the issues with UnifiedServerSocket. We were able to put clusters into a bad state pretty easily simply by having a participant disconnect immediately after connecting to the quorum port. This crashed the accept thread in the Leader and prevented any other participants from connecting. This could happen due to network hick-ups, etc. Ideally, both PRs get +1s around the same time and can land with a small delay one after another.\r\n\r\nWe have started testing internal builds with #184 and the code in this PR. Quorums with TLS disabled have shown no perf or stability regressions, so we believe it's safe to merge this code to master as it's purely opt-in at this point, gated behind config options. We've done correctness testing of quorums with TLS enabled, but have not done perf testing to measure the impact of enabling quorum TLS yet. That should happen in the near future, and I will update this PR with our perf test results once we have them."},"comment":{"url":"https://api.github.com/repos/apache/zookeeper/issues/comments/430024886","html_url":"https://github.com/apache/zookeeper/pull/627#issuecomment-430024886","issue_url":"https://api.github.com/repos/apache/zookeeper/issues/627","id":430024886,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMDAyNDg4Ng==","user":{"login":"ivmaykov","id":682290,"node_id":"MDQ6VXNlcjY4MjI5MA==","avatar_url":"https://avatars0.githubusercontent.com/u/682290?v=4","gravatar_id":"","url":"https://api.github.com/users/ivmaykov","html_url":"https://github.com/ivmaykov","followers_url":"https://api.github.com/users/ivmaykov/followers","following_url":"https://api.github.com/users/ivmaykov/following{/other_user}","gists_url":"https://api.github.com/users/ivmaykov/gists{/gist_id}","starred_url":"https://api.github.com/users/ivmaykov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ivmaykov/subscriptions","organizations_url":"https://api.github.com/users/ivmaykov/orgs","repos_url":"https://api.github.com/users/ivmaykov/repos","events_url":"https://api.github.com/users/ivmaykov/events{/privacy}","received_events_url":"https://api.github.com/users/ivmaykov/received_events","type":"User","site_admin":false},"created_at":"2018-10-15T21:42:31Z","updated_at":"2018-10-15T21:42:31Z","author_association":"NONE","body":"@anmolnar it doesn't look like #184 is in master branch yet. Do you have any ETA on that?"}}
{"actor":{"display_login":"ivmaykov"}}