from:"ivmaykov"

[GitHub] zookeeper issue #680: ZOOKEEPER-3174: Quorum TLS - support reloading trust/k...

2018-12-14 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/680
  
@anmolnar is there anything blocking this from being merged at this point?


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-10 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/680


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-10 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/680

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

Allow reloading SSL trust stores and key stores from disk when the files on 
disk change.

## Added support for reloading key/trust stores when the file on disk 
changes
- 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
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3174

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/680.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #680


commit cc72c083c0b70409d78da11507ca5e80e726bb69
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store




---

[GitHub] zookeeper issue #680: ZOOKEEPER-3174: Quorum TLS - support reloading trust/k...

2018-12-06 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/680
  
@anmolnar removed finalizer, use explicit close()


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-06 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239641391
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(trustStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Trust store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newTrustStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+}
+trustStoreFileWatcher = newTrustStoreFileWatcher;
+trustStoreFileWatcher.start();
+}
+}
+
+/**
+ * Disables automatic reloading of the trust store and key store files 
when they change on disk.
+ * Stops background threads and closes WatchService instances.
+ */
+public void disableCertFileReloading() {
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+keyStoreFileWatcher = null;
+}
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+trustStoreFileWatcher = null;
+}
+}
+
+// Finalizer guardian object, see Effective Java item 7
+// TODO: finalize() is deprecated starting with Java 10. This needs to 
be
+// replaced with an explicit shutdown call.
+@SuppressWarnings("unused")
+private final Object finalizerGuardian = new Object() {
--- End diff --

Done.


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-06 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239593442
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
 ---
@@ -0,0 +1,253 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import com.sun.nio.file.SensitivityWatchEventModifier;
+import org.apache.zookeeper.server.ZooKeeperThread;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.nio.file.ClosedWatchServiceException;
+import java.nio.file.FileSystem;
+import java.nio.file.Path;
+import java.nio.file.StandardWatchEventKinds;
+import java.nio.file.WatchEvent;
+import java.nio.file.WatchKey;
+import java.nio.file.WatchService;
+import java.util.function.Consumer;
+
+/**
+ * Instances of this class can be used to watch a directory for file 
changes. When a file is added to, deleted from,
+ * or is modified in the given directory, the callback provided by the 
user will be called from a background thread.
+ * Some things to keep in mind:
+ * 
+ * The callback should be thread-safe.
+ * Changes that happen around the time the thread is started may be 
missed.
+ * There is a delay between a file changing and the callback 
firing.
+ * The watch is not recursive - changes to subdirectories will not 
trigger a callback.
+ * 
+ */
+public final class FileChangeWatcher {
--- End diff --

I prefer to use composition over inheritance in cases where inheritance is 
not clearly better - it tends to avoid problems. If FileChangeWatcher extends 
Thread, then it will have a "is a" relationship with Thread, and can be used 
anywhere a Thread is used. Since it's not a generic Thread, it's not clear to 
me that this would be correct. But I don't feel too strongly about it.


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-06 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239593417
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(trustStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Trust store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newTrustStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+}
+trustStoreFileWatcher = newTrustStoreFileWatcher;
+trustStoreFileWatcher.start();
+}
+}
+
+/**
+ * Disables automatic reloading of the trust store and key store files 
when they change on disk.
+ * Stops background threads and closes WatchService instances.
+ */
+public void disableCertFileReloading() {
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+keyStoreFileWatcher = null;
+}
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+trustStoreFileWatcher = null;
+}
+}
+
+// Finalizer guardian object, see Effective Java item 7
+// TODO: finalize() is deprecated starting with Java 10. This needs to 
be
+// replaced with an explicit shutdown call.
+@SuppressWarnings("unused")
+private final Object finalizerGuardian = new Object() {
--- End diff --

I'm worried about forgetting to call `close()` and leaking the background 
threads. X509Util is created in other places besides QuorumPeer. But I'll see 
what I can do about it, we only need to call close() if we called 
`enableCertFileReloading()` so it might be ok ...


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-06 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239593458
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
--- End diff --

Will do.


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-06 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239593424
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(trustStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Trust store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newTrustStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+}
+trustStoreFileWatcher = newTrustStoreFileWatcher;
+trustStoreFileWatcher.start();
+}
+}
+
+/**
+ * Disables automatic reloading of the trust store and key store files 
when they change on disk.
+ * Stops background threads and closes WatchService instances.
+ */
+public void disableCertFileReloading() {
--- End diff --

Will do.


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-03 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/680

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

Allow reloading SSL trust stores and key stores from disk when the files on 
disk change.

## Added support for reloading key/trust stores when the file on disk 
changes
- 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
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3174

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/680.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #680


commit b373878e45151ea736055cd0a1cf51181a0f0e0d
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store




---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-12-03 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/710

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3195

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/710.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #710


commit fc7f54819fa3ec618d368f73e014614e4e39ef11
Author: Ilya Maykov 
Date:   2018-11-20T23:30:23Z

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.
Also added a unit test that verifies client-initiated renegotiation fails.




---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-03 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/680


---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-12-03 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/710


---

[GitHub] zookeeper issue #680: ZOOKEEPER-3174: Quorum TLS - support reloading trust/k...

2018-11-30 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/680
  
@anmolnar looks like FileMonitor only exists in our fork for now and is not 
upstreamed yet, so never mind about that comment! I'll work with other FB 
engineers to unify the two classes.


---

[GitHub] zookeeper issue #680: ZOOKEEPER-3174: Quorum TLS - support reloading trust/k...

2018-11-29 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/680
  
@anmolnar is there anything I can do to help move this PR and #681 along?


---

[GitHub] zookeeper issue #709: ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKT...

2018-11-27 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/709
  
@anmolnar could you merge this into master and branch-3.5?


---

[GitHub] zookeeper pull request #727: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-27 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/727


---

[GitHub] zookeeper issue #680: ZOOKEEPER-3174: Quorum TLS - support reloading trust/k...

2018-11-27 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/680
  
@anmolnar I just stumbled across the FileMonitor class, which has similar 
functionality to my FileChangeWatcher. The main difference is that it polls for 
changes and sleeps the thread in between, rather than using WatchService. It 
also ignores mtime changes if the file contents have not changed. Maybe I 
should use FileMonitor and remove the FileChangeWatcher class?


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-27 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/680


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-27 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/680

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

Allow reloading SSL trust stores and key stores from disk when the files on 
disk change.

## Added support for reloading key/trust stores when the file on disk 
changes
- 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
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3174

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/680.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #680


commit 6c492bbad6021c12f3e1bc0b57a3b35bab771696
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store




---

[GitHub] zookeeper issue #727: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-27 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/727
  
@anmolnar here is the branch-3.5 version of #679


---

[GitHub] zookeeper pull request #727: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-27 Thread ivmaykov

GitHub user ivmaykov opened a pull request:

https://github.com/apache/zookeeper/pull/727

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

### Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the accept() thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172-branch-3.5

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/727.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #727


commit 4a6a22f083930597b8ab3a5cb35048a6ab24813b
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades




---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-26 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
@anmolnar renamed the tests and cleaned them up quite a bit too (the 
formerly-DoS tests don't actually need a background thread for the bad client 
socket, which simplifies things).


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-26 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
@anmolnar put the DoS tests back in. If you decide that you're ok with 
keeping them, let me know what you'd like me to rename / comment.


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-26 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
I like the old DoS tests, I'm happy to restore them. The new unit test I 
added can stay too.


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-26 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
@anmolnar removed DoS tests from `UnifiedServerSocketTest.java`. Added a 
new unit test `UnifiedServerSocketModeDetectionTest.java` which checks that 
calling certain methods on the server side of a unified socket connection does 
not trigger mode detection. Also rebased on latest master to fix merge conflict.


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-23 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
@anmolnar I understand your point about the DoS tests, and agree that they 
are not very focused and do end up testing the structure of the test code 
itself in (i.e. that the implementation of 
`UnifiedServerSocketTest$UnifiedServerThread` plays nicely with 
`UnifiedServerSocket`). Let me explain really briefly how that test came about, 
since the history behind it is missing in this discussion.

Initially when I rewrote `UnifiedServerSocket` and added the 
`UnifiedSocket` inner class, I didn't have the `UnifiedInputStream` and 
`UnifiedOutputStream` inner classes. I was careful to avoid any reads inside 
`accept()`, and thought that was good enough. However, with this version of the 
code we still saw problems on our test cluster during a stress 
disconnect/reconnect test. Investigation showed evidence that the Leader's 
`accept()` thread was getting stuck, and then I found the code in 
`Leader$LearnerCnxAcceptor#run()` that calls `getInputStream()` on the accepted 
socket and wraps the result in a `BufferedInputStream`. The only way at the 
time to get the input stream was to resolve the type of socket (by doing a read 
of first 5 bytes and then TLS/plaintext mode detection), and get the real 
socket's input stream.

My fix involved adding the `UnifiedInputStream` and `UnifiedOutputStream` 
inner classes, which allowed me to delay the mode detection. Now, instead of 
triggering mode detection at the time of `getInputStream()`, we can trigger it 
the first time the stream is used for I/O. I wrote the DoS test as part of the 
same diff, and was careful to replicate the threading behavior of 
`Leader$LearnerCnxAcceptor.run()` in `UnifiedServerThread`. I also verified 
that the new tests failed on a version of the code that didn't have the unified 
stream wrappers.

If I remove the DoS tests, the behavior of delaying the mode detection's 
read to the point of first I/O operation would not be tested properly. I think 
that code should be tested! However, if you prefer I could probably test it in 
a more focused way (i.e. just test each method that's meant to be non-blocking 
by itself). Would that be an acceptable compromise?


---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-21 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/710#discussion_r235552846
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -60,6 +60,12 @@
 public abstract class X509Util {
 private static final Logger LOG = 
LoggerFactory.getLogger(X509Util.class);
 
+static {
+// Client-initiated renegotiation in TLS is unsafe and
+// allows MITM attacks, so we should always disable it.
+System.setProperty("jdk.tls.rejectClientInitiatedRenegotiation", 
"true");
--- End diff --

Sure, will do


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-21 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/679


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-21 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/669

ZOOKEEPER-3152: Port ZK netty stack to netty4

Summary: Ported the client connection netty stack from netty3 to netty4. 
This includes both the server side (NettyServerCnxn and friends) and the client 
side (ClientCnxnSocketNetty).

Test Plan: Modified `FourLetterWordsTest` and `NettyServerCnxnTest`, plus 
manual testing on a regional ensemble.

FB Reviewers: nixon

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3152

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/669.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #669


commit 4fb8eb6ebe69a4f9a0852624d652d0893d6ba625
Author: Ilya Maykov 
Date:   2018-08-31T23:26:55Z

port ZK netty stack from netty3 to netty4

Summary:
Ported the client connection netty stack from netty3 to netty4. This 
includes both the server side
(NettyServerCnxn and friends) and the client side (ClientCnxnSocketNetty).

Test Plan: Modified `FourLetterWordsTest` and `NettyServerCnxnTest`, plus 
manual testing on a regional ensemble.

Reviewers: nixon, nwolchko, nedelchev

Subscribers:

Differential Revision: https://phabricator.intern.facebook.com/D9646262

Tasks:

Tags:

Blame Revision:




---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-21 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/669


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-21 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/679

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

## Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the `accept()` thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/679.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #679






---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-21 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/710

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3195

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/710.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #710


commit 027d7d24912dc7a00d798a77196f83be06229755
Author: Ilya Maykov 
Date:   2018-11-20T23:30:23Z

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.




---

[GitHub] zookeeper issue #710: ZOOKEEPER-3195: TLS - disable client-initiated renegot...

2018-11-21 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/710
  
The jenkins failures seem to be due to unrelated flaky tests. I'll try 
closing/reopening the PR until it succeeds ...


---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-21 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/710


---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-21 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/710#discussion_r235489458
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -60,6 +60,12 @@
 public abstract class X509Util {
 private static final Logger LOG = 
LoggerFactory.getLogger(X509Util.class);
 
+static {
+// Client-initiated renegotiation in TLS is unsafe and
+// allows MITM attacks, so we should always disable it.
+System.setProperty("jdk.tls.rejectClientInitiatedRenegotiation", 
"true");
--- End diff --

I'm not sure. However, setting it to true before any SSLContext objects are 
created by X509Util seems to do the trick. I'm not sure if flipping it back to 
`false` after the server is running and sockets are created would lead to 
future connections allowing client-initiated renegotiation or not. I don't 
think it matters since we don't have a way to flip the option back to `false`.


---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-21 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/710

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3195

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/710.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #710


commit 027d7d24912dc7a00d798a77196f83be06229755
Author: Ilya Maykov 
Date:   2018-11-20T23:30:23Z

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.




---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-21 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/710


---

[GitHub] zookeeper pull request #709: ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug...

2018-11-21 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/709


---

[GitHub] zookeeper pull request #709: ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug...

2018-11-21 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/709

ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKTrustManager

Fix an obvious copy/paste bug. Tested by making sure ZKTrustManagerTest 
still passes.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3194

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/709.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #709


commit ad6502ddc47236c4db359e1d1fa5af3a48e2684c
Author: Ilya Maykov 
Date:   2018-11-20T23:23:28Z

ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKTrustManager




---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-20 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/710

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3195

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/710.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #710


commit 027d7d24912dc7a00d798a77196f83be06229755
Author: Ilya Maykov 
Date:   2018-11-20T23:30:23Z

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.




---

[GitHub] zookeeper pull request #709: ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug...

2018-11-20 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/709


---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-20 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/710


---

[GitHub] zookeeper pull request #709: ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug...

2018-11-20 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/709

ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKTrustManager

Fix an obvious copy/paste bug. Tested by making sure ZKTrustManagerTest 
still passes.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3194

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/709.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #709


commit ad6502ddc47236c4db359e1d1fa5af3a48e2684c
Author: Ilya Maykov 
Date:   2018-11-20T23:23:28Z

ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKTrustManager




---

[GitHub] zookeeper pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated ...

2018-11-20 Thread ivmaykov

GitHub user ivmaykov opened a pull request:

https://github.com/apache/zookeeper/pull/710

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3195

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/710.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #710


commit 027d7d24912dc7a00d798a77196f83be06229755
Author: Ilya Maykov 
Date:   2018-11-20T23:30:23Z

ZOOKEEPER-3195: TLS - disable client-initiated renegotiation

Summary: client-initiated renegotiation is insecure and is vulnerable to 
MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and

https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html

Test Plan: manually tested by running a secure ZK server and probing the 
listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 
9, 10, and 11.




---

[GitHub] zookeeper pull request #709: ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug...

2018-11-20 Thread ivmaykov

GitHub user ivmaykov opened a pull request:

https://github.com/apache/zookeeper/pull/709

ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKTrustManager

Fix an obvious copy/paste bug. Tested by making sure ZKTrustManagerTest 
still passes.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3194

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/709.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #709


commit ad6502ddc47236c4db359e1d1fa5af3a48e2684c
Author: Ilya Maykov 
Date:   2018-11-20T23:23:28Z

ZOOKEEPER-3194: Quorum TLS - fix copy/paste bug in ZKTrustManager




---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-20 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
@anmolnar the benefit of the current DOS tests is to verify that certain 
actions on a UnifiedSocket (getting input stream, setting socket options, etc) 
do not trigger the blocking read for TLS / plaintext mode detection. I think 
there is benefit to having such tests `UnifiedServerSocketTest`, since they 
test behavior that's implemented in `UnifiedServerSocket.java`. What do you 
think?


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-16 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/680


---

[GitHub] zookeeper pull request #681: ZOOKEEPER-3176: Quorum TLS - add SSL config opt...

2018-11-16 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/681

ZOOKEEPER-3176: Quorum TLS - add SSL config options

Add  SSL config options for enabled protocols and client auth mode.
Improve handling of SSL config options for protocols and cipher suites - 
previously these came from system properties, now they can come from ZKConfig 
which means they are easier to isolate in tests, and now we don't need to parse 
system properties every time we create a secure socket.

Note that this is stacked on top of #678, #679, and #680 and thus includes 
them. Please only consider the ZOOKEEPER-3176 commit when reviewing. Once the 
other PRs are merged upstream, I will rebase this so it only contains one 
commit.

## Added more options for ssl settings to X509Util and encapsulate them 
better
- 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.
- now all SSL-related settings come from the `ZKConfig` object used to 
create the SSL context
- new settings added:
- `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`.
- `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.
- `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`

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3176

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/681.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #681


commit 367bef0980193e2761c7008844c5e9fe029d8a66
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

commit fd58fa45cdd76c6b4c1bb2f529ee8f6d7fff553d
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

commit d232235519e2c6e252dcac700dcc05146cea5dbc
Author: Ilya Maykov 
Date:   2018-10-25T02:12:04Z

ZOOKEEPER-3176: Quorum TLS - add SSL config options




---

[GitHub] zookeeper pull request #681: ZOOKEEPER-3176: Quorum TLS - add SSL config opt...

2018-11-16 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/681


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-15 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
@anmolnar use Executor and InetAddress.getLoopbackAddress()


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-15 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/679

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

## Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the `accept()` thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/679.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #679


commit 367bef0980193e2761c7008844c5e9fe029d8a66
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades




---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-15 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/679


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-15 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r234053630
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
 
-String testDataPath = System.getProperty("test.data.dir", 
"build/test/data");
 
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, 
"org.apache.zookeeper.server.NettyServerCnxnFactory");
 System.setProperty(ZKClientConfig.ZOOKE

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-15 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r234053611
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
--- End diff --

Done


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-15 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r234052921
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
 
-String testDataPath = System.getProperty("test.data.dir", 
"build/test/data");
 
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, 
"org.apache.zookeeper.server.NettyServerCnxnFactory");
 System.setProperty(ZKClientConfig.ZOOKE

[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-15 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/669
  
@anmolnar does anything else need to be done with this PR before it can be 
merged?


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233668370
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
 
-String testDataPath = System.getProperty("test.data.dir", 
"build/test/data");
 
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, 
"org.apache.zookeeper.server.NettyServerCnxnFactory");
 System.setProperty(ZKClientConfig.ZOOKE

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233663529
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
 ---
@@ -116,170 +116,104 @@ public void channelConnected(ChannelHandlerContext 
ctx,
 
 NettyServerCnxn cnxn = new NettyServerCnxn(channel,
 zkServer, NettyServerCnxnFactory.this);
-ctx.setAttachment(cnxn);
+ctx.channel().attr(CONNECTION_ATTRIBUTE).set(cnxn);
 
 if (secure) {
-SslHandler sslHandler = 
ctx.getPipeline().get(SslHandler.class);
-ChannelFuture handshakeFuture = sslHandler.handshake();
+SslHandler sslHandler = 
ctx.pipeline().get(SslHandler.class);
+Future handshakeFuture = 
sslHandler.handshakeFuture();
 handshakeFuture.addListener(new 
CertificateVerifier(sslHandler, cnxn));
 } else {
-allChannels.add(ctx.getChannel());
+allChannels.add(ctx.channel());
 addCnxn(cnxn);
 }
 }
 
 @Override
-public void channelDisconnected(ChannelHandlerContext ctx,
-ChannelStateEvent e) throws Exception
-{
+public void channelInactive(ChannelHandlerContext ctx) throws 
Exception {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnected " + e);
+LOG.trace("Channel inactive {}", ctx.channel());
 }
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+allChannels.remove(ctx.channel());
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnect caused close " + e);
+LOG.trace("Channel inactive caused close {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void exceptionCaught(ChannelHandlerContext ctx, 
ExceptionEvent e)
-throws Exception
-{
-LOG.warn("Exception caught " + e, e.getCause());
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+public void exceptionCaught(ChannelHandlerContext ctx, Throwable 
cause) throws Exception {
+LOG.warn("Exception caught", cause);
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isDebugEnabled()) {
-LOG.debug("Closing " + cnxn);
+LOG.debug("Closing {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void messageReceived(ChannelHandlerContext ctx, 
MessageEvent e)
-throws Exception
-{
-if (LOG.isTraceEnabled()) {
-LOG.trace("message received called " + e.getMessage());
-}
+public void userEventTriggered(ChannelHandlerContext ctx, Object 
evt) throws Exception {
 try {
-if (LOG.isDebugEnabled()) {
-LOG.debug("New message " + e.toString()
-+ " from " + ctx.getChannel());
-}
-NettyServerCnxn cnxn = 
(NettyServerCnxn)ctx.getAttachment();
-synchronized(cnxn) {
-processMessage(e, cnxn);
+if (evt == NettyServerCnxn.AutoReadEvent.ENABLE) {
+LOG.debug("Received AutoReadEvent.ENABLE");
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).get();
+// TODO(ilyam): Not sure if cnxn can be null here. It 
becomes null if channelInactive()
--- End diff --

Ah, right. I think I misunderstood. So the code is good as-is, yes?


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233661987
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
 ---
@@ -116,170 +116,104 @@ public void channelConnected(ChannelHandlerContext 
ctx,
 
 NettyServerCnxn cnxn = new NettyServerCnxn(channel,
 zkServer, NettyServerCnxnFactory.this);
-ctx.setAttachment(cnxn);
+ctx.channel().attr(CONNECTION_ATTRIBUTE).set(cnxn);
 
 if (secure) {
-SslHandler sslHandler = 
ctx.getPipeline().get(SslHandler.class);
-ChannelFuture handshakeFuture = sslHandler.handshake();
+SslHandler sslHandler = 
ctx.pipeline().get(SslHandler.class);
+Future handshakeFuture = 
sslHandler.handshakeFuture();
 handshakeFuture.addListener(new 
CertificateVerifier(sslHandler, cnxn));
 } else {
-allChannels.add(ctx.getChannel());
+allChannels.add(ctx.channel());
 addCnxn(cnxn);
 }
 }
 
 @Override
-public void channelDisconnected(ChannelHandlerContext ctx,
-ChannelStateEvent e) throws Exception
-{
+public void channelInactive(ChannelHandlerContext ctx) throws 
Exception {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnected " + e);
+LOG.trace("Channel inactive {}", ctx.channel());
 }
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+allChannels.remove(ctx.channel());
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnect caused close " + e);
+LOG.trace("Channel inactive caused close {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void exceptionCaught(ChannelHandlerContext ctx, 
ExceptionEvent e)
-throws Exception
-{
-LOG.warn("Exception caught " + e, e.getCause());
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+public void exceptionCaught(ChannelHandlerContext ctx, Throwable 
cause) throws Exception {
+LOG.warn("Exception caught", cause);
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isDebugEnabled()) {
-LOG.debug("Closing " + cnxn);
+LOG.debug("Closing {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void messageReceived(ChannelHandlerContext ctx, 
MessageEvent e)
-throws Exception
-{
-if (LOG.isTraceEnabled()) {
-LOG.trace("message received called " + e.getMessage());
-}
+public void userEventTriggered(ChannelHandlerContext ctx, Object 
evt) throws Exception {
 try {
-if (LOG.isDebugEnabled()) {
-LOG.debug("New message " + e.toString()
-+ " from " + ctx.getChannel());
-}
-NettyServerCnxn cnxn = 
(NettyServerCnxn)ctx.getAttachment();
-synchronized(cnxn) {
-processMessage(e, cnxn);
+if (evt == NettyServerCnxn.AutoReadEvent.ENABLE) {
+LOG.debug("Received AutoReadEvent.ENABLE");
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).get();
+// TODO(ilyam): Not sure if cnxn can be null here. It 
becomes null if channelInactive()
--- End diff --

It will not be called twice, since removing the connection attribute means 
the second time we will get null and there is a null check in both places.


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233661570
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/PrependableSocket.java
 ---
@@ -18,32 +18,47 @@
 
 package org.apache.zookeeper.server.quorum;
 
-import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.SequenceInputStream;
+import java.io.PushbackInputStream;
 import java.net.Socket;
 import java.net.SocketImpl;
 
 public class PrependableSocket extends Socket {
 
-  private SequenceInputStream sequenceInputStream;
+  private PushbackInputStream pushbackInputStream;
--- End diff --

Yes, SequenceInputStream likely still works in practice, but exposes the 
discontinuity between the underlying streams to the caller. Using 
PushbackInputStream hides it and I think is preferable.


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233657773
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
 
-String testDataPath = System.getProperty("test.data.dir", 
"build/test/data");
 
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, 
"org.apache.zookeeper.server.NettyServerCnxnFactory");
 System.setProperty(ZKClientConfig.ZOOKE

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233657163
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
 
-String testDataPath = System.getProperty("test.data.dir", 
"build/test/data");
 
System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, 
"org.apache.zookeeper.server.NettyServerCnxnFactory");
 System.setProperty(ZKClientConfig.ZOOKE

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233656740
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketTest.java
 ---
@@ -17,156 +17,644 @@
  */
 package org.apache.zookeeper.server.quorum;
 
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.SSLSocket;
+
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.Time;
+import org.apache.zookeeper.common.KeyStoreFileType;
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509KeyType;
+import org.apache.zookeeper.common.X509TestContext;
 import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
-import javax.net.ssl.HandshakeCompletedEvent;
-import javax.net.ssl.HandshakeCompletedListener;
-import javax.net.ssl.SSLSocket;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.junit.Assert.assertThat;
+@RunWith(Parameterized.class)
+public class UnifiedServerSocketTest extends BaseX509ParameterizedTestCase 
{
 
-public class UnifiedServerSocketTest {
+@Parameterized.Parameters
+public static Collection params() {
+ArrayList result = new ArrayList<>();
+int paramIndex = 0;
+for (X509KeyType caKeyType : X509KeyType.values()) {
+for (X509KeyType certKeyType : X509KeyType.values()) {
+for (Boolean hostnameVerification : new Boolean[] { true, 
false  }) {
+result.add(new Object[]{
+caKeyType,
+certKeyType,
+hostnameVerification,
+paramIndex++
+});
+}
+}
+}
+return result;
+}
 
 private static final int MAX_RETRIES = 5;
 private static final int TIMEOUT = 1000;
+private static final byte[] DATA_TO_CLIENT = "hello client".getBytes();
+private static final byte[] DATA_FROM_CLIENT = "hello 
server".getBytes();
 
 private X509Util x509Util;
 private int port;
-private volatile boolean handshakeCompleted;
+private InetSocketAddress localServerAddress;
+private final Object handshakeCompletedLock = new Object();
+// access only inside synchronized(handshakeCompletedLock) { ... } 
blocks
+private boolean handshakeCompleted = false;
+
+public UnifiedServerSocketTest(
+final X509KeyType caKeyType,
+final X509KeyType certKeyType,
+final Boolean hostnameVerification,
+final Integer paramIndex) {
+super(paramIndex, () -> {
+try {
+return X509TestContext.newBuilder()
+.setTempDir(tempDir)
+.setKeyStoreKeyType(certKeyType)
+.setTrustStoreKeyType(caKeyType)
+.setHostnameVerification(hostnameVerification)
+.build();
+} catch (Exception e) {
+throw new RuntimeException(e);
+}
+});
+}
 
 @Before
 public void setUp() throws Exception {
-handshakeCompleted = false;
-
 port = PortAssignment.unique();
+localServerAddress = new InetSocketAddress("localhost", port);
--- End diff --

Sure, I could do that if you prefer.


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233656201
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -350,14 +389,22 @@ public static X509TrustManager createTrustManager(
 public SSLSocket createSSLSocket() throws X509Exception, IOException {
 SSLSocket sslSocket = (SSLSocket) 
getDefaultSSLContext().getSocketFactory().createSocket();
 configureSSLSocket(sslSocket);
-
+sslSocket.setUseClientMode(true);
 return sslSocket;
 }
 
-public SSLSocket createSSLSocket(Socket socket) throws X509Exception, 
IOException {
-SSLSocket sslSocket = (SSLSocket) 
getDefaultSSLContext().getSocketFactory().createSocket(socket, null, 
socket.getPort(), true);
+public SSLSocket createSSLSocket(Socket socket, byte[] pushbackBytes) 
throws X509Exception, IOException {
+SSLSocket sslSocket;
+if (pushbackBytes != null && pushbackBytes.length > 0) {
+sslSocket = (SSLSocket) 
getDefaultSSLContext().getSocketFactory().createSocket(
+socket, new ByteArrayInputStream(pushbackBytes), true);
+} else {
+sslSocket = (SSLSocket) 
getDefaultSSLContext().getSocketFactory().createSocket(
+socket, null, socket.getPort(), true);
+}
 configureSSLSocket(sslSocket);
-
+sslSocket.setUseClientMode(false);
--- End diff --

Yes and yes. In #681 I make the client auth setting configurable.


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/679#discussion_r233656579
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/PrependableSocket.java
 ---
@@ -18,32 +18,47 @@
 
 package org.apache.zookeeper.server.quorum;
 
-import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.SequenceInputStream;
+import java.io.PushbackInputStream;
 import java.net.Socket;
 import java.net.SocketImpl;
 
 public class PrependableSocket extends Socket {
 
-  private SequenceInputStream sequenceInputStream;
+  private PushbackInputStream pushbackInputStream;
--- End diff --

I don't know why it worked like that, but that is what I observed in tests. 
It looks like SequenceInputStream does not join returned data across boundaries 
of the underlying streams when the first stream gets to EOF. I don't think this 
is desired behavior since it would cause the stream to return 5 bytes when more 
than 5 bytes are actually available.


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233651445
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
 ---
@@ -316,16 +251,17 @@ public void operationComplete(ChannelFuture future)
 if (KeeperException.Code.OK !=
 authProvider.handleAuthentication(cnxn, null)) 
{
 LOG.error("Authentication failed for session 0x{}",
-Long.toHexString(cnxn.sessionId));
+Long.toHexString(cnxn.getSessionId()));
 cnxn.close();
 return;
 }
 
-allChannels.add(future.getChannel());
+final Channel futureChannel = future.getNow();
--- End diff --

I think they are equivalent here since we know the future is completed (we 
are inside a `if (future.isSuccess())` block). I can change it to `get()` if 
you prefer.


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233651730
  
--- Diff: 
zookeeper-server/src/test/java/org/apache/zookeeper/test/TestByteBufAllocator.java
 ---
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.CompositeByteBuf;
+import io.netty.buffer.PooledByteBufAllocator;
+import io.netty.util.ResourceLeakDetector;
+
+/**
+ * This is a custom ByteBufAllocator that tracks outstanding allocations 
and
+ * crashes the program if any of them are leaked.
+ *
+ * Never use this class in production, it will cause your server to run out
+ * of memory! This is because it holds strong references to all allocated
+ * buffers and doesn't release them until checkForLeaks() is called at the
+ * end of a unit test.
+ *
+ * Note: the original code was copied from 
https://github.com/airlift/drift,
+ * with the permission and encouragement of airlift's author (dain). 
Airlift
+ * uses the same apache 2.0 license as Zookeeper so this should be ok.
+ *
+ * However, the code was modified to take advantage of Netty's built-in
+ * leak tracking and make a best effort to print details about buffer 
leaks.
+ *
+ */
+public class TestByteBufAllocator extends PooledByteBufAllocator {
+private static AtomicReference INSTANCE =
+new AtomicReference<>(null);
+
+/**
+ * Get the singleton testing allocator.
+ * @return the singleton allocator, creating it if one does not exist.
+ */
+public static TestByteBufAllocator getInstance() {
+TestByteBufAllocator result = INSTANCE.get();
+if (result == null) {
+ResourceLeakDetector.Level oldLevel = 
ResourceLeakDetector.getLevel();
+
ResourceLeakDetector.setLevel(ResourceLeakDetector.Level.PARANOID);
+INSTANCE.compareAndSet(null, new 
TestByteBufAllocator(oldLevel));
+result = INSTANCE.get();
+}
+return result;
+}
+
+/**
+ * Destroys the singleton testing allocator and throws an error if any 
of the
+ * buffers allocated by it have been leaked. Attempts to print leak 
details to
+ * standard error before throwing, by using netty's built-in leak 
tracking.
+ * Note that this might not always work, since it only triggers when a 
buffer
+ * is garbage-collected and calling System.gc() does not guarantee 
that a buffer
+ * will actually be GC'ed.
+ *
+ * This should be called at the end of a unit test's tearDown() method.
+ */
+public static void checkForLeaks() {
+TestByteBufAllocator result = INSTANCE.getAndSet(null);
+if (result != null) {
+result.checkInstanceForLeaks();
+}
+}
+
+private final List trackedBuffers = new ArrayList<>();
+private final ResourceLeakDetector.Level oldLevel;
+
+private TestByteBufAllocator(ResourceLeakDetector.Level oldLevel)
+{
+super(false);
+this.oldLevel = oldLevel;
+}
+
+@Override
+protected ByteBuf newHeapBuffer(int initialCapacity, int maxCapacity)
+{
+return track(super.newHeapBuffer(initialCapacity, maxCapacity));
+}
+
+@Override
+protected ByteBuf newDirectBuffer(int initialCapacity, int maxCapacity)
+{
+return track(super.newDirectBuffer(initialCapacity, maxCapacity));
+}
+
+@Override
+public CompositeByteBuf compositeHeapBuffer(int maxNumComponents)
+{
+return track(super.comp

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233650367
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
 ---
@@ -116,170 +116,104 @@ public void channelConnected(ChannelHandlerContext 
ctx,
 
 NettyServerCnxn cnxn = new NettyServerCnxn(channel,
 zkServer, NettyServerCnxnFactory.this);
-ctx.setAttachment(cnxn);
+ctx.channel().attr(CONNECTION_ATTRIBUTE).set(cnxn);
 
 if (secure) {
-SslHandler sslHandler = 
ctx.getPipeline().get(SslHandler.class);
-ChannelFuture handshakeFuture = sslHandler.handshake();
+SslHandler sslHandler = 
ctx.pipeline().get(SslHandler.class);
+Future handshakeFuture = 
sslHandler.handshakeFuture();
 handshakeFuture.addListener(new 
CertificateVerifier(sslHandler, cnxn));
 } else {
-allChannels.add(ctx.getChannel());
+allChannels.add(ctx.channel());
 addCnxn(cnxn);
 }
 }
 
 @Override
-public void channelDisconnected(ChannelHandlerContext ctx,
-ChannelStateEvent e) throws Exception
-{
+public void channelInactive(ChannelHandlerContext ctx) throws 
Exception {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnected " + e);
+LOG.trace("Channel inactive {}", ctx.channel());
 }
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+allChannels.remove(ctx.channel());
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnect caused close " + e);
+LOG.trace("Channel inactive caused close {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void exceptionCaught(ChannelHandlerContext ctx, 
ExceptionEvent e)
-throws Exception
-{
-LOG.warn("Exception caught " + e, e.getCause());
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+public void exceptionCaught(ChannelHandlerContext ctx, Throwable 
cause) throws Exception {
+LOG.warn("Exception caught", cause);
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isDebugEnabled()) {
-LOG.debug("Closing " + cnxn);
+LOG.debug("Closing {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void messageReceived(ChannelHandlerContext ctx, 
MessageEvent e)
-throws Exception
-{
-if (LOG.isTraceEnabled()) {
-LOG.trace("message received called " + e.getMessage());
-}
+public void userEventTriggered(ChannelHandlerContext ctx, Object 
evt) throws Exception {
 try {
-if (LOG.isDebugEnabled()) {
-LOG.debug("New message " + e.toString()
-+ " from " + ctx.getChannel());
-}
-NettyServerCnxn cnxn = 
(NettyServerCnxn)ctx.getAttachment();
-synchronized(cnxn) {
-processMessage(e, cnxn);
+if (evt == NettyServerCnxn.AutoReadEvent.ENABLE) {
+LOG.debug("Received AutoReadEvent.ENABLE");
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).get();
+// TODO(ilyam): Not sure if cnxn can be null here. It 
becomes null if channelInactive()
--- End diff --

I do remove it in both places, by calling 
`ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null)`. The getAndSet(null) 
will atomically return the old value of the attribute and set the new value to 
null.

Now that I think about it, I'm not sure if we need to remove the attribute 
in `exceptionCaught()` ... we can probably leave it and let `channelInactive()` 
take care of removing the attribute. Let me know if you want me to make this 
change, I think it probably doesn't matter too much either way.


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233652712
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNetty.java 
---
@@ -103,71 +105,102 @@
 boolean isConnected() {
 // Assuming that isConnected() is only used to initiate connection,
 // not used by some other connection status judgement.
-return channel != null;
+connectLock.lock();
+try {
+return channel != null || connectFuture != null;
--- End diff --

As the comment above says, the `isConnected()` method is only used in the 
main loop inside `ClientCnxn$SendThread.run()` to see if a new connection 
should be initiated. So, this method should return false if a connection 
attempt is already in progress. This is the case when `connectFuture` is not 
null. Arguably the method should be called `isConnectedOrConnecting()` but I 
didn't want to go around refactoring APIs in this diff - can do it if you like.


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233653584
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNetty.java 
---
@@ -103,71 +105,102 @@
 boolean isConnected() {
 // Assuming that isConnected() is only used to initiate connection,
 // not used by some other connection status judgement.
-return channel != null;
+connectLock.lock();
+try {
+return channel != null || connectFuture != null;
+} finally {
+connectLock.unlock();
+}
+}
+
+private Bootstrap configureBootstrapAllocator(Bootstrap bootstrap) {
+ByteBufAllocator testAllocator = TEST_ALLOCATOR.get();
+if (testAllocator != null) {
+return bootstrap.option(ChannelOption.ALLOCATOR, 
testAllocator);
+} else {
+return bootstrap;
+}
 }
 
 @Override
 void connect(InetSocketAddress addr) throws IOException {
 firstConnect = new CountDownLatch(1);
 
-ClientBootstrap bootstrap = new ClientBootstrap(channelFactory);
-
-bootstrap.setPipelineFactory(new 
ZKClientPipelineFactory(addr.getHostString(), addr.getPort()));
-bootstrap.setOption("soLinger", -1);
-bootstrap.setOption("tcpNoDelay", true);
-
-connectFuture = bootstrap.connect(addr);
-connectFuture.addListener(new ChannelFutureListener() {
-@Override
-public void operationComplete(ChannelFuture channelFuture) 
throws Exception {
-// this lock guarantees that channel won't be assgined 
after cleanup().
-connectLock.lock();
-try {
-if (!channelFuture.isSuccess() || connectFuture == 
null) {
-LOG.info("future isn't success, cause: {}", 
channelFuture.getCause());
-return;
-}
-// setup channel, variables, connection, etc.
-channel = channelFuture.getChannel();
-
-disconnected.set(false);
-initialized = false;
-lenBuffer.clear();
-incomingBuffer = lenBuffer;
-
-sendThread.primeConnection();
-updateNow();
-updateLastSendAndHeard();
-
-if (sendThread.tunnelAuthInProgress()) {
-waitSasl.drainPermits();
-needSasl.set(true);
-sendPrimePacket();
-} else {
-needSasl.set(false);
-}
+Bootstrap bootstrap = new Bootstrap()
+.group(eventLoopGroup)
+.channel(NettyUtils.nioOrEpollSocketChannel())
+.option(ChannelOption.SO_LINGER, -1)
+.option(ChannelOption.TCP_NODELAY, true)
+.handler(new ZKClientPipelineFactory(addr.getHostString(), 
addr.getPort()));
+bootstrap = configureBootstrapAllocator(bootstrap);
+bootstrap.validate();
 
-// we need to wake up on first connect to avoid 
timeout.
-wakeupCnxn();
-firstConnect.countDown();
-LOG.info("channel is connected: {}", 
channelFuture.getChannel());
-} finally {
-connectLock.unlock();
+connectLock.lock();
+try {
+connectFuture = bootstrap.connect(addr);
+connectFuture.addListener(new ChannelFutureListener() {
+@Override
+public void operationComplete(ChannelFuture channelFuture) 
throws Exception {
+// this lock guarantees that channel won't be assigned 
after cleanup().
+connectLock.lock();
+try {
+if (!channelFuture.isSuccess()) {
+LOG.info("future isn't success, cause:", 
channelFuture.cause());
+return;
+} else if (connectFuture == null) {
--- End diff --

As the comment below says, there could be a race if the connect attempt was 
cancelled right around the time the listener callback fired. `cleanup()` below 
will cancel an in-progress connection attempt and clear the `connectFuture` 
variable. If this happens, `connectFuture` will be null here.


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r233649908
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
 ---
@@ -116,170 +116,104 @@ public void channelConnected(ChannelHandlerContext 
ctx,
 
 NettyServerCnxn cnxn = new NettyServerCnxn(channel,
 zkServer, NettyServerCnxnFactory.this);
-ctx.setAttachment(cnxn);
+ctx.channel().attr(CONNECTION_ATTRIBUTE).set(cnxn);
 
 if (secure) {
-SslHandler sslHandler = 
ctx.getPipeline().get(SslHandler.class);
-ChannelFuture handshakeFuture = sslHandler.handshake();
+SslHandler sslHandler = 
ctx.pipeline().get(SslHandler.class);
+Future handshakeFuture = 
sslHandler.handshakeFuture();
 handshakeFuture.addListener(new 
CertificateVerifier(sslHandler, cnxn));
 } else {
-allChannels.add(ctx.getChannel());
+allChannels.add(ctx.channel());
 addCnxn(cnxn);
 }
 }
 
 @Override
-public void channelDisconnected(ChannelHandlerContext ctx,
-ChannelStateEvent e) throws Exception
-{
+public void channelInactive(ChannelHandlerContext ctx) throws 
Exception {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnected " + e);
+LOG.trace("Channel inactive {}", ctx.channel());
 }
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+allChannels.remove(ctx.channel());
+NettyServerCnxn cnxn = 
ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null);
 if (cnxn != null) {
 if (LOG.isTraceEnabled()) {
-LOG.trace("Channel disconnect caused close " + e);
+LOG.trace("Channel inactive caused close {}", cnxn);
 }
 cnxn.close();
 }
 }
 
 @Override
-public void exceptionCaught(ChannelHandlerContext ctx, 
ExceptionEvent e)
-throws Exception
-{
-LOG.warn("Exception caught " + e, e.getCause());
-NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment();
+public void exceptionCaught(ChannelHandlerContext ctx, Throwable 
cause) throws Exception {
--- End diff --

We call `cnxn.close()` at the end of `exceptionCaught()`, which will end up 
closing the channel so `channelInactive()` will get called, so I think it would 
be redundant to remove from `allChannels` here.


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-11 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/680

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

Allow reloading SSL trust stores and key stores from disk when the files on 
disk change.

Note that this is stacked on top of #678 and #679 and thus includes them. 
Please only consider the ZOOKEEPER-3174 commit when reviewing. Once the other 
PRs are merged upstream, I will rebase this so it only contains one commit.

## Added support for reloading key/trust stores when the file on disk 
changes
- 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
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3174

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/680.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #680


commit 33f7aaab6fe16122b7e1faedbb408d739bbe8a30
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

commit 30adde0fa951d5d99b6b33370eca9736e370a952
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store




---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-11 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/680


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-11 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/679

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

## Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the `accept()` thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/679.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #679


commit 33f7aaab6fe16122b7e1faedbb408d739bbe8a30
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades




---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-11 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/679


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-11 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
Don't use UnifiedServerSocket for the non-port-unification path in this PR, 
since cert reloading is not yet available here.


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-08 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r231969309
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
 ---
@@ -0,0 +1,191 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import com.sun.nio.file.SensitivityWatchEventModifier;
+import org.apache.zookeeper.server.ZooKeeperThread;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.nio.file.ClosedWatchServiceException;
+import java.nio.file.FileSystem;
+import java.nio.file.Path;
+import java.nio.file.StandardWatchEventKinds;
+import java.nio.file.WatchEvent;
+import java.nio.file.WatchKey;
+import java.nio.file.WatchService;
+import java.util.function.Consumer;
+
+/**
+ * Instances of this class can be used to watch a directory for file 
changes. When a file is added to, deleted from,
+ * or is modified in the given directory, the callback provided by the 
user will be called from a background thread.
+ * Some things to keep in mind:
+ * 
+ * The callback should be thread-safe.
+ * Changes that happen around the time the thread is started may be 
missed.
+ * There is a delay between a file changing and the callback 
firing.
+ * The watch is not recursive - changes to subdirectories will not 
trigger a callback.
+ * 
+ */
+public final class FileChangeWatcher {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChangeWatcher.class);
+
+public enum State {
+NEW,  // object created but start() not called yet
+STARTING, // start() called but background thread has not entered 
main loop
+RUNNING,  // background thread is running
+STOPPING, // stop() called but background thread has not exited 
main loop
+STOPPED   // stop() called and background thread has exited, or 
background thread crashed
+}
+
+private final WatcherThread watcherThread;
+private State state; // protected by synchronized(this)
+
+/**
+ * Creates a watcher that watches dirPath and invokes 
callback on changes.
+ *
+ * @param dirPath the directory to watch.
+ * @param callback the callback to invoke with events. 
event.kind() will return the type of event,
+ * and event.context() will return the 
filename relative to dirPath.
+ * @throws IOException if there is an error creating the WatchService.
+ */
+public FileChangeWatcher(Path dirPath, Consumer> 
callback) throws IOException {
+FileSystem fs = dirPath.getFileSystem();
+WatchService watchService = fs.newWatchService();
+if (LOG.isDebugEnabled()) {
+LOG.debug("Registering with watch service: " + dirPath);
+}
+dirPath.register(
+watchService,
+new WatchEvent.Kind[]{
+StandardWatchEventKinds.ENTRY_CREATE,
+StandardWatchEventKinds.ENTRY_DELETE,
+StandardWatchEventKinds.ENTRY_MODIFY,
+StandardWatchEventKinds.OVERFLOW},
+SensitivityWatchEventModifier.HIGH);
+state = State.NEW;
+this.watcherThread = new WatcherThread(watchService, callback);
+this.watcherThread.setDaemon(true);
+}
+
+public synchronized State getState() {
+return state;
+}
+
+private synchronized void setState(State newState) {
+state = newState;
+this.notifyAll();
+}
+
+/**
+ * Tells the background thread to start. Does not wait for it to be 
running.
+ * Calling this method more than once has no effect.
+ */
+public synchronized void start() {

[GitHub] zookeeper pull request #681: ZOOKEEPER-3176: Quorum TLS - add SSL config opt...

2018-11-07 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/681

ZOOKEEPER-3176: Quorum TLS - add SSL config options

Add  SSL config options for enabled protocols and client auth mode.
Improve handling of SSL config options for protocols and cipher suites - 
previously these came from system properties, now they can come from ZKConfig 
which means they are easier to isolate in tests, and now we don't need to parse 
system properties every time we create a secure socket.

Note that this is stacked on top of #678, #679, and #680 and thus includes 
them. Please only consider the ZOOKEEPER-3176 commit when reviewing. Once the 
other PRs are merged upstream, I will rebase this so it only contains one 
commit.

## Added more options for ssl settings to X509Util and encapsulate them 
better
- 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.
- now all SSL-related settings come from the `ZKConfig` object used to 
create the SSL context
- new settings added:
- `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`.
- `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.
- `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`

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3176

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/681.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #681


commit b45b716a59709b933c0652f48d563b08e1091e92
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

commit 28565e3466dd35a339a3d1d71de20298072abb1a
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

commit 4bd95d4d0f69fa73342e2c6af57c38e41421e140
Author: Ilya Maykov 
Date:   2018-10-25T02:12:04Z

ZOOKEEPER-3176: Quorum TLS - add SSL config options




---

[GitHub] zookeeper pull request #681: ZOOKEEPER-3176: Quorum TLS - add SSL config opt...

2018-11-07 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/681


---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-07 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/669

ZOOKEEPER-3152: Port ZK netty stack to netty4

Summary: Ported the client connection netty stack from netty3 to netty4. 
This includes both the server side (NettyServerCnxn and friends) and the client 
side (ClientCnxnSocketNetty).

Test Plan: Modified `FourLetterWordsTest` and `NettyServerCnxnTest`, plus 
manual testing on a regional ensemble.

FB Reviewers: nixon

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3152

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/669.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #669


commit 94c4516bea9b46f5428ef29ff51490f9647eaac3
Author: Ilya Maykov 
Date:   2018-08-31T23:26:55Z

port ZK netty stack from netty3 to netty4

Summary:
Ported the client connection netty stack from netty3 to netty4. This 
includes both the server side
(NettyServerCnxn and friends) and the client side (ClientCnxnSocketNetty).

Test Plan: Modified `FourLetterWordsTest` and `NettyServerCnxnTest`, plus 
manual testing on a regional ensemble.

Reviewers: nixon, nwolchko, nedelchev

Subscribers:

Differential Revision: https://phabricator.intern.facebook.com/D9646262

Tasks:

Tags:

Blame Revision:




---

[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-07 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/669


---

[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...

2018-11-07 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/678
  
@anmolnar thanks for merging this :) I've rebased #679, #680, and #681 on 
top of master. Let's get those in soon :)


---

[GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...

2018-11-07 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/679
  
Rebase on latest master, no longer includes #678 as it has been merged 
upstream.


---

[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...

2018-11-06 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/678
  
@anmolnar the latest attempt does not have test failures, but claims a 
findbugs failure.

>[exec] -1 overall.  GitHub Pull Request  Build
>[exec]   
>[exec] 
>[exec] +1 @author.  The patch does not contain any @author tags.
>[exec] 
>[exec] +0 tests included.  The patch appears to be a documentation 
patch that doesn't require tests.
>[exec] 
>[exec] +1 javadoc.  The javadoc tool did not generate any warning 
messages.
>[exec] 
>[exec] +1 javac.  The applied patch does not increase the total 
number of javac compiler warnings.
>[exec] 
>[exec] -1 findbugs.  The patch appears to cause Findbugs (version 
3.0.1) to fail.
>[exec] 
>[exec] +1 release audit.  The applied patch does not increase the 
total number of release audit warnings.
>[exec] 
>[exec] +1 core tests.  The patch passed core unit tests.
>[exec] 
>[exec] +1 contrib tests.  The patch passed contrib unit tests.

Link to full output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2599/console


---

[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...

2018-11-06 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/678
  
Jenkins claims there is a findbugs failure, but there is no actual output 
of findbugs failures, and findbugs passes on my machine. Â¯\_(ã)_/Â¯


---

[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...

2018-11-06 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/678
  
I don't understand why this PR started failing Jenkins builds all of a 
sudden. The next 2 PRs stacked on this one (#678 and #680) also fail, but the 
last one (#681) passes.


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-06 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/679

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

Note that this is stacked on top of #678 and thus includes it. Please only 
consider the ZOOKEEPER-3172 commit when reviewing. Once the other PR is merged 
upstream, I will rebase this so it only contains one commit.

## Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the `accept()` thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/679.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #679


commit 2122c8c23a0dbb27f9b2aff55e800e48d253f943
Author: Ilya Maykov 
Date:   2018-10-25T00:41:48Z

ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
ZOOKEEPER-3175: Quorum TLS - test improvements

Add support for loading key and trust stores from PEM files.
Also added test utils for testing X509-related code, because it
was very difficult to untangle them from the PEM support code.

commit 514d48a26aeeca37290ad14ff8f0cdae69b53eb2
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades




---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-06 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/679


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-06 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/679

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

Note that this is stacked on top of #678 and thus includes it. Please only 
consider the ZOOKEEPER-3172 commit when reviewing. Once the other PR is merged 
upstream, I will rebase this so it only contains one commit.

## Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the `accept()` thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/679.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #679


commit 2122c8c23a0dbb27f9b2aff55e800e48d253f943
Author: Ilya Maykov 
Date:   2018-10-25T00:41:48Z

ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
ZOOKEEPER-3175: Quorum TLS - test improvements

Add support for loading key and trust stores from PEM files.
Also added test utils for testing X509-related code, because it
was very difficult to untangle them from the PEM support code.

commit 514d48a26aeeca37290ad14ff8f0cdae69b53eb2
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades




---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-06 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/679


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-06 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/680

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store

Allow reloading SSL trust stores and key stores from disk when the files on 
disk change.

Note that this is stacked on top of #678 and #679 and thus includes them. 
Please only consider the ZOOKEEPER-3174 commit when reviewing. Once the other 
PRs are merged upstream, I will rebase this so it only contains one commit.

## Added support for reloading key/trust stores when the file on disk 
changes
- 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
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3174

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/680.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #680


commit 2122c8c23a0dbb27f9b2aff55e800e48d253f943
Author: Ilya Maykov 
Date:   2018-10-25T00:41:48Z

ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
ZOOKEEPER-3175: Quorum TLS - test improvements

Add support for loading key and trust stores from PEM files.
Also added test utils for testing X509-related code, because it
was very difficult to untangle them from the PEM support code.

commit 69f5185c8c14720e94c81f0147ee9cbc2ae42f89
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

commit d9e09dc73a42be079a6bd28b51c09635fff32e95
Author: Ilya Maykov 
Date:   2018-10-25T01:54:06Z

ZOOKEEPER-3174: Quorum TLS - support reloading trust/key store




---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-06 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/680


---

[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...

2018-11-06 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/678
  
@anmolnar is there a way to reproduce the exact same steps that jenkins 
runs on my macbook?


---

[GitHub] zookeeper issue #678: ZOOKEEPER-3173: Quorum TLS - support PEM trust/key sto...

2018-11-06 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/678
  
@anmolnar it works for me as far as I can tell, I think the contbuild is 
flaky? The contbuild on #681, which includes the same commit, passes.


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-05 Thread ivmaykov

Github user ivmaykov closed the pull request at:

https://github.com/apache/zookeeper/pull/679


---

[GitHub] zookeeper pull request #679: ZOOKEEPER-3172: Quorum TLS - fix port unificati...

2018-11-05 Thread ivmaykov

GitHub user ivmaykov reopened a pull request:

https://github.com/apache/zookeeper/pull/679

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades

Fix numerous problems with UnifiedServerSocket, such as hanging the 
accept() thread when the client doesn't send any data or crashing if less than 
5 bytes are read from the socket in the initial read.

Re-enable the "portUnification" config option.

Note that this is stacked on top of #678 and thus includes it. Please only 
consider the ZOOKEEPER-3172 commit when reviewing. Once the other PR is merged 
upstream, I will rebase this so it only contains one commit.

## Fixed networking issues/bugs in UnifiedServerSocket

- don't crash the `accept()` thread if the client closes the connection 
without sending any data
- don't corrupt the connection if the client sends fewer than 5 bytes for 
the initial read
- 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.
- 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ivmaykov/zookeeper ZOOKEEPER-3172

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/679.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #679


commit 2122c8c23a0dbb27f9b2aff55e800e48d253f943
Author: Ilya Maykov 
Date:   2018-10-25T00:41:48Z

ZOOKEEPER-3173: Quorum TLS - support PEM trust/key stores
ZOOKEEPER-3175: Quorum TLS - test improvements

Add support for loading key and trust stores from PEM files.
Also added test utils for testing X509-related code, because it
was very difficult to untangle them from the PEM support code.

commit 69f5185c8c14720e94c81f0147ee9cbc2ae42f89
Author: Ilya Maykov 
Date:   2018-10-25T01:22:24Z

ZOOKEEPER-3172: Quorum TLS - fix port unification to allow rolling upgrades




---

[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-05 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/669
  
@maoling it will be hard to do perf comparison of netty3 vs netty4 for us 
because we are currently using the NIO transports, and we don't plan on 
switching to netty3 in production. Comparing NIO vs netty4 is kind of 
apples-and-oranges. A quick perf test with this patch showed that NIO to netty4 
did not result in any obvious performance regressions, so I think we can say 
they are at least comparable :)


---

[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-11-05 Thread ivmaykov

Github user ivmaykov commented on the issue:

https://github.com/apache/zookeeper/pull/669
  
Rebase, update localAddress after accepting connection and log it


---

[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-11-03 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r230574766
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
 ---
@@ -0,0 +1,191 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import com.sun.nio.file.SensitivityWatchEventModifier;
+import org.apache.zookeeper.server.ZooKeeperThread;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.nio.file.ClosedWatchServiceException;
+import java.nio.file.FileSystem;
+import java.nio.file.Path;
+import java.nio.file.StandardWatchEventKinds;
+import java.nio.file.WatchEvent;
+import java.nio.file.WatchKey;
+import java.nio.file.WatchService;
+import java.util.function.Consumer;
+
+/**
+ * Instances of this class can be used to watch a directory for file 
changes. When a file is added to, deleted from,
+ * or is modified in the given directory, the callback provided by the 
user will be called from a background thread.
+ * Some things to keep in mind:
+ * 
+ * The callback should be thread-safe.
+ * Changes that happen around the time the thread is started may be 
missed.
+ * There is a delay between a file changing and the callback 
firing.
+ * The watch is not recursive - changes to subdirectories will not 
trigger a callback.
+ * 
+ */
+public final class FileChangeWatcher {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChangeWatcher.class);
+
+public enum State {
+NEW,  // object created but start() not called yet
+STARTING, // start() called but background thread has not entered 
main loop
+RUNNING,  // background thread is running
+STOPPING, // stop() called but background thread has not exited 
main loop
+STOPPED   // stop() called and background thread has exited, or 
background thread crashed
+}
+
+private final WatcherThread watcherThread;
+private State state; // protected by synchronized(this)
+
+/**
+ * Creates a watcher that watches dirPath and invokes 
callback on changes.
+ *
+ * @param dirPath the directory to watch.
+ * @param callback the callback to invoke with events. 
event.kind() will return the type of event,
+ * and event.context() will return the 
filename relative to dirPath.
+ * @throws IOException if there is an error creating the WatchService.
+ */
+public FileChangeWatcher(Path dirPath, Consumer> 
callback) throws IOException {
+FileSystem fs = dirPath.getFileSystem();
+WatchService watchService = fs.newWatchService();
+if (LOG.isDebugEnabled()) {
+LOG.debug("Registering with watch service: " + dirPath);
+}
+dirPath.register(
+watchService,
+new WatchEvent.Kind[]{
+StandardWatchEventKinds.ENTRY_CREATE,
+StandardWatchEventKinds.ENTRY_DELETE,
+StandardWatchEventKinds.ENTRY_MODIFY,
+StandardWatchEventKinds.OVERFLOW},
+SensitivityWatchEventModifier.HIGH);
+state = State.NEW;
+this.watcherThread = new WatcherThread(watchService, callback);
+this.watcherThread.setDaemon(true);
+}
+
+public synchronized State getState() {
--- End diff --

I agree, there are use cases where a R/W lock is a great choice, but I 
don't think this is one of them.


---

1 2 3 >

1 - 100 of 227 matches

Mail list logo