[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[asfgit]

Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104245364
  
--- Diff: src/java/test/org/apache/zookeeper/ZKTestCase.java ---
@@ -51,6 +51,12 @@ public void starting(FrameworkMethod method) {
 // accidentally attempting to start multiple admin servers on 
the
 // same port.
 System.setProperty("zookeeper.admin.enableServer", "false");
+// ZOOKEEPER-2693 disables all 4lw by default.
+// Here we enable the 4lw which ZooKeeper tests depends.
+System.setProperty("zookeeper.4lw.commands.whitelist",
+"ruok, envi, conf, stat, srvr, cons, dump," +
--- End diff --

updated tests to address the concern of using explicit list in base test 
case.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104198451
  
--- Diff: src/java/test/org/apache/zookeeper/ZKTestCase.java ---
@@ -51,6 +51,12 @@ public void starting(FrameworkMethod method) {
 // accidentally attempting to start multiple admin servers on 
the
 // same port.
 System.setProperty("zookeeper.admin.enableServer", "false");
+// ZOOKEEPER-2693 disables all 4lw by default.
+// Here we enable the 4lw which ZooKeeper tests depends.
+System.setProperty("zookeeper.4lw.commands.whitelist",
+"ruok, envi, conf, stat, srvr, cons, dump," +
--- End diff --

ZKTestCase is base test class, covering a test scenario from this class 
should be avoided. May be you can add more test case in 
FourLetterWordsWhiteListTest to increase the coverage.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104096586
  
--- Diff: src/java/test/org/apache/zookeeper/ZKTestCase.java ---
@@ -51,6 +51,12 @@ public void starting(FrameworkMethod method) {
 // accidentally attempting to start multiple admin servers on 
the
 // same port.
 System.setProperty("zookeeper.admin.enableServer", "false");
+// ZOOKEEPER-2693 disables all 4lw by default.
+// Here we enable the 4lw which ZooKeeper tests depends.
+System.setProperty("zookeeper.4lw.commands.whitelist",
+"ruok, envi, conf, stat, srvr, cons, dump," +
--- End diff --

Yeah I thought about this but I ended up keeping the current form to get 
more test coverage. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104092982
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,151 @@
+/**
+ * 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.io.IOException;
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.apache.zookeeper.server.command.FourLetterCommands;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
--- End diff --

Now non-whitelist commands are processed in the same flow as the whitelist 
commands which are already tested and closing the connection.  it is ok to skip 
connection test


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104092091
  
--- Diff: src/java/test/org/apache/zookeeper/ZKTestCase.java ---
@@ -51,6 +51,12 @@ public void starting(FrameworkMethod method) {
 // accidentally attempting to start multiple admin servers on 
the
 // same port.
 System.setProperty("zookeeper.admin.enableServer", "false");
+// ZOOKEEPER-2693 disables all 4lw by default.
+// Here we enable the 4lw which ZooKeeper tests depends.
+System.setProperty("zookeeper.4lw.commands.whitelist",
+"ruok, envi, conf, stat, srvr, cons, dump," +
--- End diff --

In test cases it is fine to enable all the commands, use 
zookeeper.4lw.commands.whitelist=* instead of list of commands


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104016184
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -18,10 +18,16 @@
 
 package org.apache.zookeeper.server.command;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.nio.ByteBuffer;
 import java.util.Collections;
--- End diff --

The import java.util.Collections is never used


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r104015988
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,163 @@
+/**
+ * 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.io.IOException;
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.apache.zookeeper.server.command.FourLetterCommands;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
+protected static final Logger LOG =
+LoggerFactory.getLogger(FourLetterWordsWhiteListTest.class);
+
+/*
+ * ZOOKEEPER-2693: test white list of four letter words.
+ * For 3.5.x default white list is empty. Verify that is
+ * the case (except 'stat' command which is enabled in ClientBase
+ * which other tests depend on.).
+ */
+@Test(timeout=3)
+public void testFourLetterWordsAllDisabledByDefault() throws Exception 
{
+stopServer();
+FourLetterCommands.resetWhiteList();
+System.setProperty("zookeeper.4lw.commands.whitelist", "stat");
+startServer();
+
+// Default white list for 3.5.x is empty, so all command should 
fail.
+verifyAllCommandsFail();
+
+TestableZooKeeper zk = createClient();
+String sid = getHexSessionId(zk.getSessionId());
--- End diff --

sid is not used


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r103512572
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -216,6 +216,10 @@ public static boolean isEnabled(String command) {
 whiteListedCommands.add(cmd.trim());
 }
 }
+// It is sad that isro and srvr are used by ZooKeeper itself. 
Need fix this
+// before deprecating 4lw.
+whiteListedCommands.add("isro");
--- End diff --

@rakeshadr thanks for feedback, updated patch. I also did an optimization 
that only conditionally enables "isro" only when read only mode is enabled 
(readonly mode is disabled by default.) with a test case, so we don't have to 
say that "isro" is also enabled by default in doc (the less implementation 
details we mention there the better imo.).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r103139365
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -216,6 +216,10 @@ public static boolean isEnabled(String command) {
 whiteListedCommands.add(cmd.trim());
 }
 }
+// It is sad that isro and srvr are used by ZooKeeper itself. 
Need fix this
+// before deprecating 4lw.
+whiteListedCommands.add("isro");
--- End diff --

I've few comments, please see:

comment-1) I agree that these commands are very much needed for ZK 
functionality and cannot be disabled. I hope you have coded with that point in 
mind. In general, default values can be overridden, but here these are 
mandatory values. Can we document conveying these thoughts. Presently the 
documentation says that `"The default value is empty, which disables all Four 
Letter Words command."`

comment-2) Say, user keeps `4lw.commands.whitelist=`, then 
`System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST)` will be null and these 
two commands will not be added to `whiteListedCommands`, right?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r102301090
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -267,10 +267,17 @@ private boolean checkFourLetterWord(final Channel 
channel,
 {
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
-String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (!FourLetterCommands.isKnown(len)) {
 return false;
 }
+
+// ZOOKEEPER-2693: don't execute 4lw if it's not enabled.
+String cmd = FourLetterCommands.getCommandString(len);
+if (!FourLetterCommands.isEnabled(cmd)) {
+LOG.debug("Command {} is not executed because it is not white 
listed.", cmd);
+return true;
--- End diff --

@rakeshadr Turns out server socket not closed is a by design. Had a chat 
with @phunt offline and the idea is we always prefer client to close socket 
first (which led to server socket close) because a socket close at server might 
be premature and led to client not getting all the data due to how TCP works. 
I'll leave these code as they are.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r102275734
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -267,10 +267,17 @@ private boolean checkFourLetterWord(final Channel 
channel,
 {
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
-String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (!FourLetterCommands.isKnown(len)) {
 return false;
 }
+
+// ZOOKEEPER-2693: don't execute 4lw if it's not enabled.
+String cmd = FourLetterCommands.getCommandString(len);
+if (!FourLetterCommands.isEnabled(cmd)) {
+LOG.debug("Command {} is not executed because it is not white 
listed.", cmd);
+return true;
--- End diff --

@rakeshadr h I think we did not even close server sockets today for 
four letter words in general, see 

https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java#L343
and

https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java#L388

This is not a problem in general for the command line interface because in 
that case client socket will close first and then server socket will close as a 
result of client socket close... however if someone writes a client that opens 
and holds a socket then server will not close the socket even after 4lw finish 
execution. This probably is a by design as it allows clients to pipe 4lw 
commands w/o re-opening sockets but I see this is another potential point of 
vulnerability where a server could run out of sockets..

I think we should probably close the sockets in the two links I posted in 
the beginning. Let me know what you think @rakeshadr.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r102273810
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -267,10 +267,17 @@ private boolean checkFourLetterWord(final Channel 
channel,
 {
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
-String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (!FourLetterCommands.isKnown(len)) {
 return false;
 }
+
+// ZOOKEEPER-2693: don't execute 4lw if it's not enabled.
+String cmd = FourLetterCommands.getCommandString(len);
+if (!FourLetterCommands.isEnabled(cmd)) {
+LOG.debug("Command {} is not executed because it is not white 
listed.", cmd);
+return true;
--- End diff --

Thanks @arshadmohammad for your suggestion, I like this approach from a 
user experience point of view as it provides nice error message on client side.

I am a little bit concerned that instead of doing a (nearly) NOP on server 
side to block a command the way the patch is doing now (which just cost a look 
up), any command including garbage now costs some string printing plus has to 
go through network stack to send the bytes back. This may lead potential 
vulnerable point, though it might also be OK as one could argue if we are in 
such case (ZK server is wide open, every bets is off.). I tend to lean towards 
the safer side though so I'll wait for a while for other comments regarding 
this issue.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r102126647
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -267,10 +267,17 @@ private boolean checkFourLetterWord(final Channel 
channel,
 {
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
-String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (!FourLetterCommands.isKnown(len)) {
 return false;
 }
+
+// ZOOKEEPER-2693: don't execute 4lw if it's not enabled.
+String cmd = FourLetterCommands.getCommandString(len);
+if (!FourLetterCommands.isEnabled(cmd)) {
+LOG.debug("Command {} is not executed because it is not white 
listed.", cmd);
+return true;
--- End diff --

Hi @hanm I have attached a patch in the jira  for your reference. the patch 
is not complete in itself, merge you changes in that patch to make it complete. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101964775
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -267,10 +267,17 @@ private boolean checkFourLetterWord(final Channel 
channel,
 {
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
-String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (!FourLetterCommands.isKnown(len)) {
 return false;
 }
+
+// ZOOKEEPER-2693: don't execute 4lw if it's not enabled.
+String cmd = FourLetterCommands.getCommandString(len);
+if (!FourLetterCommands.isEnabled(cmd)) {
+LOG.debug("Command {} is not executed because it is not white 
listed.", cmd);
+return true;
--- End diff --

We can create a dummy command(ErrorCommand extends 
AbstractFourLetterCommand ) and execute it and return true same way as 
SetTraceMaskCommand 
This ErrorCommand will take care rest of the thing, sending any error 
message to client and closing the connection etc. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101951840
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -267,10 +267,17 @@ private boolean checkFourLetterWord(final Channel 
channel,
 {
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
-String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (!FourLetterCommands.isKnown(len)) {
 return false;
 }
+
+// ZOOKEEPER-2693: don't execute 4lw if it's not enabled.
+String cmd = FourLetterCommands.getCommandString(len);
+if (!FourLetterCommands.isEnabled(cmd)) {
+LOG.debug("Command {} is not executed because it is not white 
listed.", cmd);
+return true;
--- End diff --

Should we cleanup the this selection key from the selector?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101877565
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,151 @@
+/**
+ * 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.io.IOException;
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.apache.zookeeper.server.command.FourLetterCommands;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
--- End diff --

The test should cover all cases @arshadmohammad mentioned except that 
"verify that for non-configured commands connection is close" - I'll probably 
add that test too but for now just want to upload test for feedback. Let me 
know what you think @arshadmohammad .


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101877413
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,11 +159,69 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+private static final Logger LOG = 
LoggerFactory.getLogger(FourLetterCommands.class);
+
+private static final Map cmd2String = new 
HashMap();
+
+private static final Set whiteListedCommands = new 
HashSet();
+
+private static boolean whiteListInitialized = false;
+
+// @VisibleForTesting
+public static void resetWhiteList() {
+whiteListInitialized = false;
+whiteListedCommands.clear();
+}
+
+/**
+ * Return the string representation of the specified command code.
+ */
+public static String getCommandString(int command) {
+return cmd2String.get(command);
+}
+
+/**
+ * Check if the specified command code is from a known command.
+ *
+ * @param command The integer code of command.
+ * @return true if the specified command is known, false otherwise.
+ */
+public static boolean isKnown(int command) {
+return cmd2String.containsKey(command);
+}
 
-public static Map getCmdMapView() {
--- End diff --

While I am on this, this legacy method can be optimized as a boolean query 
instead of returning a collection, so did the change for this as well (in 
addition to the white list collection.).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101877350
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,11 +159,69 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+private static final Logger LOG = 
LoggerFactory.getLogger(FourLetterCommands.class);
+
+private static final Map cmd2String = new 
HashMap();
+
+private static final Set whiteListedCommands = new 
HashSet();
+
+private static boolean whiteListInitialized = false;
--- End diff --

Introduce this instead of relying on whiteListedCommands.empty to deal with 
the case where the list is empty and initialized. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101838868
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.io.IOException;
+
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
+protected static final Logger LOG =
+LoggerFactory.getLogger(FourLetterWordsTest.class);
+
+@Rule
+public Timeout timeout = new Timeout(3);
--- End diff --

Good catch - it was a copy paste from another test.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101838800
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.io.IOException;
+
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
--- End diff --

I think all cases are already covered with a combination of this test and 
other existing test except this one "verify that for non-configured commands 
connection is close" - but I could also make all test cases explicit as well. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101838458
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +159,50 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+// A property only used in tests to turn on / off entire set of 
supported four letter word commands.
+private static final String ZOOKEEPER_4LW_TEST = 
"zookeeper.test.4lw.enabled";
+
+private static final Logger LOG = 
LoggerFactory.getLogger(FourLetterCommands.class);
+
+private static final Map cmd2String = new 
HashMap();
+
+private static final Set whiteListedCommands = new 
HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
--- End diff --

Sounds reasonable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101838431
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +159,50 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+// A property only used in tests to turn on / off entire set of 
supported four letter word commands.
+private static final String ZOOKEEPER_4LW_TEST = 
"zookeeper.test.4lw.enabled";
--- End diff --

I really like this property as it saves me tons of work - but I'll see what 
I can do.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101838329
  
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -479,7 +479,7 @@ private boolean checkFourLetterWord(final SelectionKey 
k, final int len)
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
 String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (cmd == null || 
!FourLetterCommands.getWhiteListedCmdView().contains(cmd)) {
--- End diff --

I think the original comment was not clear but I think it is a good catch - 
instead of return false here we return true because the semantic of 
checkFourLetterWord is we only return false if 4lw is not found, and in that 
case the caller will think this is a client message and proceed allocate buffer 
etc work (iiuc that was what the "it should be processed in that way only" 
meant.).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[eribeiro]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101836721
  
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -479,7 +479,7 @@ private boolean checkFourLetterWord(final SelectionKey 
k, final int len)
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
 String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (cmd == null || 
!FourLetterCommands.getWhiteListedCmdView().contains(cmd)) {
--- End diff --

What do you suggest it can be done here?

Maybe throw an exception if 
``!FourLetterCommands.getWhiteListedCmdView().contains(cmd)`` is ``true`` and 
get it in the callee?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101824634
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.io.IOException;
+
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
+protected static final Logger LOG =
+LoggerFactory.getLogger(FourLetterWordsTest.class);
+
+@Rule
+public Timeout timeout = new Timeout(3);
--- End diff --

The constructor Timeout(int) is deprecated use 
org.junit.rules.Timeout.Timeout(long timeout, TimeUnit timeUnit)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101824558
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java ---
@@ -0,0 +1,123 @@
+/**
+ * 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.io.IOException;
+
+
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+
+import static 
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FourLetterWordsWhiteListTest extends ClientBase {
--- End diff --

FourLetterWordsWhiteListTest should do testing around the configured value 
of zookeeper.4lw.commands.whitelist.
following are some scenairo which can be included
verify whether confiured commands execued properly
verify that the command which is not configured fails 
verify that for non-configured commands connection is close
verify default commands executed successfully without any configuration


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101821552
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java ---
@@ -167,6 +167,7 @@ public void 
testClientConnectionRequestDuringStartupWithNettyServerCnxn()
  */
 @Test(timeout = 3)
 public void testFourLetterWords() throws Exception {
+System.setProperty("zookeeper.test.4lw.enabled", "true");
--- End diff --

I is better to use zookeeper.4lw.commands.whitelist. This comment is for 
all the test classes where zookeeper.test.4lw.enabled used. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101819637
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +159,50 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+// A property only used in tests to turn on / off entire set of 
supported four letter word commands.
+private static final String ZOOKEEPER_4LW_TEST = 
"zookeeper.test.4lw.enabled";
+
+private static final Logger LOG = 
LoggerFactory.getLogger(FourLetterCommands.class);
+
+private static final Map cmd2String = new 
HashMap();
+
+private static final Set whiteListedCommands = new 
HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
+if (!whiteListedCommands.isEmpty()) {
+return Collections.unmodifiableSet(whiteListedCommands);
+}
+
+if (System.getProperty(ZOOKEEPER_4LW_TEST, 
"false").equals("true")) {
--- End diff --

Should be removed as suggested in previous comment


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101819527
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +159,50 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+// A property only used in tests to turn on / off entire set of 
supported four letter word commands.
+private static final String ZOOKEEPER_4LW_TEST = 
"zookeeper.test.4lw.enabled";
+
+private static final Logger LOG = 
LoggerFactory.getLogger(FourLetterCommands.class);
+
+private static final Map cmd2String = new 
HashMap();
+
+private static final Set whiteListedCommands = new 
HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
--- End diff --

I think instead of returning all the commands all the time and making 
collection object. We can write function isWhiteListedCommand(String command) 
and use it


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101818968
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +159,50 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+// A property only used in tests to turn on / off entire set of 
supported four letter word commands.
+private static final String ZOOKEEPER_4LW_TEST = 
"zookeeper.test.4lw.enabled";
--- End diff --

We should not add new property for test cases, instead use main property 
for test cases also. may be you can move repetitive test code to utility test 
class. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101818408
  
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -268,7 +268,7 @@ private boolean checkFourLetterWord(final Channel 
channel,
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
 String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (cmd == null || 
!FourLetterCommands.getWhiteListedCmdView().contains(cmd)) {
--- End diff --

same comment as for NIOServerCnxn


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[arshadmohammad]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101818292
  
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -479,7 +479,7 @@ private boolean checkFourLetterWord(final SelectionKey 
k, final int len)
 // We take advantage of the limited size of the length to look
 // for cmds. They are all 4-bytes which fits inside of an int
 String cmd = FourLetterCommands.getCmdMapView().get(len);
-if (cmd == null) {
+if (cmd == null || 
!FourLetterCommands.getWhiteListedCmdView().contains(cmd)) {
--- End diff --

if request is for 4lw command, it should be processed in that way only. If 
false is returned from here, the request will proceed as the normal request.
This is major issue in the current patch



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[eribeiro]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101667879
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
--- End diff --

Yeah, you right. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101589125
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
--- End diff --

The class name already provide context on caller site - 
FourLetterCommands.getWhiteListedFourLetterCmd sounds redundant.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[eribeiro]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101588332
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
--- End diff --

nit: I would change this method name to something akin 
`getWhiteListedFourLetterCmds()` or `getWhiteListed4lw()`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101588320
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
+if (!whiteListedCommands.isEmpty()) {
+return Collections.unmodifiableSet(whiteListedCommands);
+}
+
+String commands = 
System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST);
+if (commands != null) {
+String[] list = commands.split(",");
+for (String cmd : list) {
+whiteListedCommands.add(cmd.trim());
--- End diff --

That is fine - an empty string will not do any damage here but it is a good 
to have check and the cost is minimum. Will do.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[eribeiro]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101587320
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
--- End diff --

nit: `static final` (lines 160 and 162)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[eribeiro]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101587208
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
+if (!whiteListedCommands.isEmpty()) {
+return Collections.unmodifiableSet(whiteListedCommands);
+}
+
+String commands = 
System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST);
+if (commands != null) {
+String[] list = commands.split(",");
+for (String cmd : list) {
+whiteListedCommands.add(cmd.trim());
--- End diff --

I guess we if we have "ruok, ,cons", it will insert an empty string in the 
collection, that is, need to check `if (!cmd.trim().isEmpty())`, right?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[edwardoliveira]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101586810
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
+if (!whiteListedCommands.isEmpty()) {
+return Collections.unmodifiableSet(whiteListedCommands);
+}
+
+String commands = 
System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST);
+if (commands != null) {
+String[] list = commands.split(",");
+for (String cmd : list) {
+whiteListedCommands.add(cmd.trim());
--- End diff --

I guess we if we have "ruok, ,cons", it will insert an empty string in the 
collection.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101582350
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1155,6 +1155,30 @@ server.3=zoo3:2888:3888
 
   
 
+  
+4lw.commands.whitelist
+
+
+  (Java system property: 

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101579781
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1155,6 +1155,30 @@ server.3=zoo3:2888:3888
 
   
 
+  
+4lw.commands.whitelist
+
+
+  (Java system property: https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java#L296).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101576900
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1155,6 +1155,30 @@ server.3=zoo3:2888:3888
 
   
 
+  
+4lw.commands.whitelist
+
+
+  (Java system property: >This new configuration option is provided as both zoo.cfg option and 
system properties so users can encode the white list in zoo.cfg and that is the 
recommended approach as documented in the admin manual

Do you meant, you are supporting both options - users can either configure 
the list in `zoo.cfg` or set as `system properties`? If yes, I'm OK to this 
approach.  But in the code I could see that server reads the value from 
`System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST)` and it is not reading 
the value from `zoo.cfg`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101572954
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1650,7 +1674,16 @@ server.3=zoo3:2888:3888
 while "srvr" and "cons" give extended details on server and
 connections respectively.
 
-
+New in 3.5.3:
+  Four Letter Words need to be explicitly white listed before 
using.
+  Please refer 4lw.commands.whitelist
+   described in 
+cluster configuration section for details.
+  Moving forward, Four Letter Words will be deprecated, please use
--- End diff --

Will do.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101572993
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
+if (!whiteListedCommands.isEmpty()) {
+return Collections.unmodifiableSet(whiteListedCommands);
+}
+
+String commands = 
System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST);
+if (commands != null) {
+String[] list = commands.split(",");
+for (String cmd : list) {
+whiteListedCommands.add(cmd.trim());
+}
+}
+
+return Collections.unmodifiableSet(whiteListedCommands);
--- End diff --

Sounds good to me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101572026
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1155,6 +1155,30 @@ server.3=zoo3:2888:3888
 
   
 
+  
+4lw.commands.whitelist
+
+
+  (Java system property: 

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101489533
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1650,7 +1674,16 @@ server.3=zoo3:2888:3888
 while "srvr" and "cons" give extended details on server and
 connections respectively.
 
-
+New in 3.5.3:
+  Four Letter Words need to be explicitly white listed before 
using.
+  Please refer 4lw.commands.whitelist
+   described in 
+cluster configuration section for details.
+  Moving forward, Four Letter Words will be deprecated, please use
--- End diff --

I hope, you are planning to deprecate in 3.5.x upcoming releases and may 
stop supporting this in 3.6.x onwards, right? If yes, then can we create(if not 
yet created) a jira task to discuss the 4lws deprecation and makes the idea 
more visible to all.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101492640
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java ---
@@ -153,13 +155,33 @@
  */
 public final static int telnetCloseCmd = 0xfff4fffd;
 
-final static HashMap cmd2String =
-new HashMap();
+private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = 
"zookeeper.4lw.commands.whitelist";
+
+final static Map cmd2String = new HashMap();
+
+final static Set whiteListedCommands = new HashSet();
 
 public static Map getCmdMapView() {
 return Collections.unmodifiableMap(cmd2String);
 }
 
+// ZOOKEEPER-2693: Only allow white listed commands.
+public static Set getWhiteListedCmdView() {
+if (!whiteListedCommands.isEmpty()) {
+return Collections.unmodifiableSet(whiteListedCommands);
+}
+
+String commands = 
System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST);
+if (commands != null) {
+String[] list = commands.split(",");
+for (String cmd : list) {
+whiteListedCommands.add(cmd.trim());
+}
+}
+
+return Collections.unmodifiableSet(whiteListedCommands);
--- End diff --

Please add an INFO log message about the acceptable and configured 
`4lwords`. The log message will be printed only once during startup or first 
cmd invocation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101491680
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1155,6 +1155,30 @@ server.3=zoo3:2888:3888
 
   
 
+  
+4lw.commands.whitelist
+
+
+  (Java system property: 

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[afine]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101404866
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1155,6 +1155,27 @@ server.3=zoo3:2888:3888
 
   
 
+  
+fourLetterWordsEnabled
+
+
+  (No Java system property)
+
+  New in 3.5.3:
+This controls the enabling or disabling of 
+  Four Letter Words feature, which is
+deprecated in favor of AdminServer.
+"fourLetterWordsEnabled" 
option can be set as
+"fourLetterWordsEnabled=false" or
+"fourLetterWordsEnabled=true"
+to a server's config file, or using QuorumPeerConfig's
--- End diff --

nit: "in a server's config file or using"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[afine]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101403760
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -2125,6 +2151,18 @@ server.3=zoo3:2888:3888
 usage limit that would cause the system to swap.
   
 
+
+
+  Publicly accessible deployment
+
+  
+ZooKeeper was not designed to be opened to the outside 
world. A ZooKeeper
--- End diff --

Both of these sentences are a little bit redundant, perhaps this can be 
simplified to:
A ZooKeeper ensemble is expected to operate in a trusted computing 
environment. It is thus recommended to deploy ZooKeeper behind a firewall.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[afine]

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

https://github.com/apache/zookeeper/pull/179#discussion_r101404365
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -750,4 +759,8 @@ public static void setReconfigEnabled(boolean enabled) {
 reconfigEnabled = enabled;
 }
 
+public static boolean isFourLetterWordsEnabled() { return 
fourLetterWordsEnabled; }
+
+public static void setFourLetterWordsEnabled(boolean enabled) { 
fourLetterWordsEnabled = enabled; }
--- End diff --

nit: this should have a new line


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zookeeper pull request #179: ZOOKEEPER-2693: DOS attack on wchp/wchc four le...

[hanm]

GitHub user hanm opened a pull request:

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

ZOOKEEPER-2693: DOS attack on wchp/wchc four letter words (4lw)

This is for master / branch-3.5:

* Introduce a new configuration option that by default turn off 4lw.
* Update docs that explicitly states ZooKeeper should not be deployed open 
to world for access and deprecating 4lw in favor of Jetty.

With these combined, the attack described in ZOOKEEPER-2693 is not possible 
if ZooKeeper is put behind a firewall where Jetty AdminServer is not publicly 
accessible.

Note for tests: I did not add any unit tests to test 4lw disabling because 
it is fairly obvious (though I have to update a place to prevent existing test 
broken, because we are using 4lw extensively to query test server states. We 
could query AdminServer instead but I consider that's a future work item - and 
even if we use AdminServer exclusively we can't dump 4lw completely because 
AdminServer test depends on 4lw - chicken egg problem.). 


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

$ git pull https://github.com/hanm/zookeeper ZOOKEEPER-2693

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

https://github.com/apache/zookeeper/pull/179.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 #179


commit b70e19ecdcd8f78bb8ac2d380a07968cbb683b3b
Author: Michael Han 
Date:   2017-02-14T22:24:05Z

Initial commit that turns four letter words off by default for 3.5.x branch.
Pending test cases and doc changes.

commit 7f3420573774a23dafc8fcbbe3873392d0c9090a
Author: Michael Han 
Date:   2017-02-14T22:41:01Z

Update Admin Doc source for 4lw changes.

commit b808940d5aed3c707b50df417a558123df4a03cd
Author: Michael Han 
Date:   2017-02-15T00:02:35Z

Update doc with security guideline.

commit f296225793dcf7bc289b63b2ff9ca7d30291fb69
Author: Michael Han 
Date:   2017-02-15T00:06:07Z

Fix broken tests.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---