Uploaded image for project: 'ZooKeeper'
  1. ZooKeeper
  2. ZOOKEEPER-2693

DOS attack on wchp/wchc four letter words (4lw)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.4.0, 3.5.1, 3.5.2
    • Fix Version/s: 3.4.10, 3.5.3, 3.6.0
    • Component/s: security, server
    • Labels:
      None

      Description

      The wchp/wchc four letter words can be exploited in a DOS attack on the ZK client port - typically 2181. The following POC attack was recently published on the web:

      https://vulners.com/exploitdb/EDB-ID:41277

      The most straightforward way to block this attack is to not allow access to the client port to non-trusted clients - i.e. firewall the ZooKeeper service and only allow access to trusted applications using it for coordination.

        Issue Links

          Activity

          Hide
          phunt Patrick Hunt added a comment -

          A simple patch would provide a configuration option that disables these 4lw by default. We should include this in 3.4/3/5 code lines. Additionally in 3.5 Jetty metrics support was added, replacing the legacy 4lw. In 3.5.3 and later we should disable 4lw entirely (through a second config option) and encourage people to use Jetty instead.

          Show
          phunt Patrick Hunt added a comment - A simple patch would provide a configuration option that disables these 4lw by default. We should include this in 3.4/3/5 code lines. Additionally in 3.5 Jetty metrics support was added, replacing the legacy 4lw. In 3.5.3 and later we should disable 4lw entirely (through a second config option) and encourage people to use Jetty instead.
          Hide
          hanm Michael Han added a comment -

          One option is to enable TLS for ZooKeeper client-server communication. Because TLS is on a different port (secureClientPort) than the four letter words port (clientPort), we can then put clientPort behind a firewall which effectively makes only admin (or whoever that's trusted) can execute four letter words.

          This however does not address the issues completely, because TLS is only supported since ZooKeeper 3.5.1 so it would not work for 3.4.x. Also even with TLS presence, it might still be possible that a ZooKeeper ensemble is attacked as long as the ensemble is open to the world: we don't have rate limiting built in, so if a set of rogue clients making connection requests, when in large scale, could possibly keep ensemble too busy to serve other requests. Although we do have IP based authentication scheme we don't have any rate limiters implemented to enforce accesses in such case.

          I believe the original design consideration for ZooKeeper was that ZooKeeper ensembles should only be accessible from a trusted computing environment instead of opening to the world. This design decision made perfect sense in the Hadoop context but might not be ideal given today's thread models (e.g., it is never safe to trust any network communication, attackers exist inside the corporate network, and so on.). As such, we may need to change the assumption on where ZooKeeper is going to be deployed and operated, but before that, to address this specific issue documented in the JIRA, we could do a combination of:

          • As Patrick suggested, add configurations options to turn off four letter words and / or specific commands.
          • Document explicitly the recommended security practice on deploy and operate ZooKeeper - basically admin should not leave the ZK wide open to the world.

          I think a patch contain both should be enough for resolving the current issue.

          In long term, we need reevaluate the design decisions and security thread models and to bridge the gap between the current design and what ZooKeeper is actually used in prod today (e.g. adding rate limiter, enforced access control, not just for TLS but also for Kerberos, like what ZOOKEEPER-1634 is trying to solve.).

          Show
          hanm Michael Han added a comment - One option is to enable TLS for ZooKeeper client-server communication. Because TLS is on a different port (secureClientPort) than the four letter words port (clientPort), we can then put clientPort behind a firewall which effectively makes only admin (or whoever that's trusted) can execute four letter words. This however does not address the issues completely, because TLS is only supported since ZooKeeper 3.5.1 so it would not work for 3.4.x. Also even with TLS presence, it might still be possible that a ZooKeeper ensemble is attacked as long as the ensemble is open to the world: we don't have rate limiting built in, so if a set of rogue clients making connection requests, when in large scale, could possibly keep ensemble too busy to serve other requests. Although we do have IP based authentication scheme we don't have any rate limiters implemented to enforce accesses in such case. I believe the original design consideration for ZooKeeper was that ZooKeeper ensembles should only be accessible from a trusted computing environment instead of opening to the world. This design decision made perfect sense in the Hadoop context but might not be ideal given today's thread models (e.g., it is never safe to trust any network communication, attackers exist inside the corporate network, and so on.). As such, we may need to change the assumption on where ZooKeeper is going to be deployed and operated, but before that, to address this specific issue documented in the JIRA, we could do a combination of: As Patrick suggested, add configurations options to turn off four letter words and / or specific commands. Document explicitly the recommended security practice on deploy and operate ZooKeeper - basically admin should not leave the ZK wide open to the world. I think a patch contain both should be enough for resolving the current issue. In long term, we need reevaluate the design decisions and security thread models and to bridge the gap between the current design and what ZooKeeper is actually used in prod today (e.g. adding rate limiter, enforced access control, not just for TLS but also for Kerberos, like what ZOOKEEPER-1634 is trying to solve.).
          Hide
          phunt Patrick Hunt added a comment -

          I had another thought on how to handle this - rather than on/off we could limit the number of 4lw that would be allowed to run in parallel. For example we could allow only a single 4lw to run at any one time. This would still provide the ability to run a 4lw, but with protection against overuse. It seems like this should be fairly simple to implement. Additionally we could provide this as an option (e.g. some folks might want to allow 5 to run in parallel). Regardless if we take this approach I think we would still want the on/off switches I already detailed.

          Show
          phunt Patrick Hunt added a comment - I had another thought on how to handle this - rather than on/off we could limit the number of 4lw that would be allowed to run in parallel. For example we could allow only a single 4lw to run at any one time. This would still provide the ability to run a 4lw, but with protection against overuse. It seems like this should be fairly simple to implement. Additionally we could provide this as an option (e.g. some folks might want to allow 5 to run in parallel). Regardless if we take this approach I think we would still want the on/off switches I already detailed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 <hanm@apache.org>
          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 <hanm@apache.org>
          Date: 2017-02-14T22:41:01Z

          Update Admin Doc source for 4lw changes.

          commit b808940d5aed3c707b50df417a558123df4a03cd
          Author: Michael Han <hanm@apache.org>
          Date: 2017-02-15T00:02:35Z

          Update doc with security guideline.

          commit f296225793dcf7bc289b63b2ff9ca7d30291fb69
          Author: Michael Han <hanm@apache.org>
          Date: 2017-02-15T00:06:07Z

          Fix broken tests.


          Show
          githubbot ASF GitHub Bot added a comment - 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 <hanm@apache.org> 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 <hanm@apache.org> Date: 2017-02-14T22:41:01Z Update Admin Doc source for 4lw changes. commit b808940d5aed3c707b50df417a558123df4a03cd Author: Michael Han <hanm@apache.org> Date: 2017-02-15T00:02:35Z Update doc with security guideline. commit f296225793dcf7bc289b63b2ff9ca7d30291fb69 Author: Michael Han <hanm@apache.org> Date: 2017-02-15T00:06:07Z Fix broken tests.
          Hide
          hanm Michael Han added a comment -

          Patrick Hunt I think what you are talking to is to add rate limiting to 4lw / admin server. There are various schemes and trade offs for rate limiter implementation, and I'd prefer using a well tested one like the rate limiter from Guava if we are ok to pull in it as a dependency.

          Show
          hanm Michael Han added a comment - Patrick Hunt I think what you are talking to is to add rate limiting to 4lw / admin server. There are various schemes and trade offs for rate limiter implementation, and I'd prefer using a well tested one like the rate limiter from Guava if we are ok to pull in it as a dependency.
          Hide
          phunt Patrick Hunt added a comment -

          Yes, effectively a rate limiter. However I was thinking in terms of number of 4lw that we would allow to run concurrently, vs number of operations per second. This would address outliers - possibly very long running 4lw. "only let one 4lw run at a time, even it if takes 20 seconds".

          Historically we've limited our dependency on other components. Neither 3.4 nor 3.5 are pulling in guava today. One of the issues we've heard from users is that (component versioning issues in particular) complicates deployment. I think adding new dependencies to something like 3.4.10, ostensibly a fix release, would be unwise.

          I threw the idea out there as a suggestion. We can take it or leave it for the various releases.

          Show
          phunt Patrick Hunt added a comment - Yes, effectively a rate limiter. However I was thinking in terms of number of 4lw that we would allow to run concurrently, vs number of operations per second. This would address outliers - possibly very long running 4lw. "only let one 4lw run at a time, even it if takes 20 seconds". Historically we've limited our dependency on other components. Neither 3.4 nor 3.5 are pulling in guava today. One of the issues we've heard from users is that (component versioning issues in particular) complicates deployment. I think adding new dependencies to something like 3.4.10, ostensibly a fix release, would be unwise. I threw the idea out there as a suggestion. We can take it or leave it for the various releases.
          Hide
          phunt Patrick Hunt added a comment -

          I looked at the patch. It is reasonable, but I don't think we can go with an all/nothing approach, many users would still want to be able to monitor their system using existing 4lw based infra, while mitigating the attack, and this doesn't allow any middle ground.

          How about on/off as you have in the current patch, but also allow a list of "whitelisted" 4lw - the default could be "ruok" and "srvr" or something of that nature. Notice we are building the list today here FourLetterCommands, and could build a list containing the configured whitelist instead? What do you think.

          Show
          phunt Patrick Hunt added a comment - I looked at the patch. It is reasonable, but I don't think we can go with an all/nothing approach, many users would still want to be able to monitor their system using existing 4lw based infra, while mitigating the attack, and this doesn't allow any middle ground. How about on/off as you have in the current patch, but also allow a list of "whitelisted" 4lw - the default could be "ruok" and "srvr" or something of that nature. Notice we are building the list today here FourLetterCommands, and could build a list containing the configured whitelist instead? What do you think.
          Hide
          hanm Michael Han added a comment - - edited

          I don't think we can go with an all/nothing approach, many users would still want to be able to monitor their system using existing 4lw based infra.

          Patrick Hunt The current patch is for branch 3.5, where we have AdminServer, which is designed to replace four letter words. That is why the patch provides only an option to completely disable the entire four letter words instead of only disabling a specific subset. The AdminServer will make four letter words irrelevant and because AdminServer does not share the ZooKeeper client port (which sometimes have to be exposed publicly), admin of ensemble protected AdminServer port with firewall without interrupting ZooKeeper clients. Besides, this seems a good opportunity to push for deprecating four letter words in favor of AdminServer which is around for quite a while given the security concerns.

          Do you think we still need provide a middle ground for 4lws for 3.5 release / master branch instead of completely shut it off?

          Show
          hanm Michael Han added a comment - - edited I don't think we can go with an all/nothing approach, many users would still want to be able to monitor their system using existing 4lw based infra. Patrick Hunt The current patch is for branch 3.5, where we have AdminServer, which is designed to replace four letter words. That is why the patch provides only an option to completely disable the entire four letter words instead of only disabling a specific subset. The AdminServer will make four letter words irrelevant and because AdminServer does not share the ZooKeeper client port (which sometimes have to be exposed publicly), admin of ensemble protected AdminServer port with firewall without interrupting ZooKeeper clients. Besides, this seems a good opportunity to push for deprecating four letter words in favor of AdminServer which is around for quite a while given the security concerns. Do you think we still need provide a middle ground for 4lws for 3.5 release / master branch instead of completely shut it off?
          Hide
          hanm Michael Han added a comment -

          As for patch for branch-3.4, I am thinking instead of disabling a subset of commands, we could just add a rate limiter. All commands will still be available to use (including the wchp/wchc ones), but they are rate limited not to cause any damages. Disabling a sub set of commands does not solve the root issue, and I imagine it might still be possible to do DOS on servers acceptor threads by just utilizing white listed four letter words at massive scale on client side.

          The configuration option of disabling 4lw or a subset of it seems an ultimate escape hatch - I guess it does not hurt to provide both as option for users, but for branch-3.4 it looks like rate limiter is a must have to address current and potential issues when the server client port is accessible from public.

          Show
          hanm Michael Han added a comment - As for patch for branch-3.4, I am thinking instead of disabling a subset of commands, we could just add a rate limiter. All commands will still be available to use (including the wchp/wchc ones), but they are rate limited not to cause any damages. Disabling a sub set of commands does not solve the root issue, and I imagine it might still be possible to do DOS on servers acceptor threads by just utilizing white listed four letter words at massive scale on client side. The configuration option of disabling 4lw or a subset of it seems an ultimate escape hatch - I guess it does not hurt to provide both as option for users, but for branch-3.4 it looks like rate limiter is a must have to address current and potential issues when the server client port is accessible from public.
          Hide
          phunt Patrick Hunt added a comment -

          Do you think we still need provide a middle ground for 4lws for 3.5 release / master branch instead of completely shut it off?

          Here's another way to think about this: we provide b/w compat when moving from X.Y.Z to X.Y+1.# As such we can deprecate 4lw in 3.5 (even turn it off by default), but we can't remove it entirely. If we want to provide a fix for this issue, and still maintain b/w compat in 3.5 then we should make it configurable. Does that make sense?

          Show
          phunt Patrick Hunt added a comment - Do you think we still need provide a middle ground for 4lws for 3.5 release / master branch instead of completely shut it off? Here's another way to think about this: we provide b/w compat when moving from X.Y.Z to X.Y+1.# As such we can deprecate 4lw in 3.5 (even turn it off by default), but we can't remove it entirely. If we want to provide a fix for this issue, and still maintain b/w compat in 3.5 then we should make it configurable. Does that make sense?
          Hide
          phunt Patrick Hunt added a comment -

          The configuration option of disabling 4lw or a subset of it seems an ultimate escape hatch

          yes, this was my thought as well.

          Your argument around having the rate limiter makes sense, that's one of the things I was thinking about this morning when I originally recommended it. Now I'm also leaning toward the "whitelist" approach because I think it's a very clean solution to the problem. What I mean is no on/off config, just a single configuration listing the whitelisted 4lw. If the list is empty it's off (the map is empty), otw the user can select the commands they would like to expose.

          If we implement something for 3.4 the same b/w compat argument should hold. i.e. if we do rate limiting in 3.4 we should also have the same functionality in 3.5.

          Show
          phunt Patrick Hunt added a comment - The configuration option of disabling 4lw or a subset of it seems an ultimate escape hatch yes, this was my thought as well. Your argument around having the rate limiter makes sense, that's one of the things I was thinking about this morning when I originally recommended it. Now I'm also leaning toward the "whitelist" approach because I think it's a very clean solution to the problem. What I mean is no on/off config, just a single configuration listing the whitelisted 4lw. If the list is empty it's off (the map is empty), otw the user can select the commands they would like to expose. If we implement something for 3.4 the same b/w compat argument should hold. i.e. if we do rate limiting in 3.4 we should also have the same functionality in 3.5.
          Hide
          hanm Michael Han added a comment -

          The compatibility argument makes most of sense - so how about this:

          • For both 3.5/3.4, provide a new configuration option that allow admin of ensemble to white list 4lw commands exposed to the world.
          • If such an option is not provided in zoo.cfg ZK will choose a list of default commands in white list. Otherwise what's put in zoo.cfg will overwrite default values.

          I think this is a good scoping that balances addressing this specific issue and ease of implementation.

          Rate limiter and deprecating 4lw in favor of AdminServer (for 3.5) can be done as separate issues later (after 3.4.10 / 3.5.3 released).

          Show
          hanm Michael Han added a comment - The compatibility argument makes most of sense - so how about this: For both 3.5/3.4, provide a new configuration option that allow admin of ensemble to white list 4lw commands exposed to the world. If such an option is not provided in zoo.cfg ZK will choose a list of default commands in white list. Otherwise what's put in zoo.cfg will overwrite default values. I think this is a good scoping that balances addressing this specific issue and ease of implementation. Rate limiter and deprecating 4lw in favor of AdminServer (for 3.5) can be done as separate issues later (after 3.4.10 / 3.5.3 released).
          Hide
          phunt Patrick Hunt added a comment -

          That makes sense to me (scoping). What will the default list of 4lw be?

          3.4: ruok,srvr,crst,srst,isro,mntr
          3.5: <empty>

          Note: the full list of supported commands is different from 3.4 to 3.5 (possibly trunk?) - we'll need to ensure the docs reflect correctly.

          Do you think that makes sense? From what I can see these are low cost lookups. Some of the items I left off are more expensive or questionable in terms of whether they should be exposed if a firewall is not used.

          Or should 3.4 include all commands aside from the two identified in this jira? I'm thinking be safe (smaller list), document this clearly in the docs and in the release notes, and allow the users interesting in exposing more 4lw to do so. Downside is that users may be impacted, i.e. would have to update production configurations.

          What should we call this? zookeeper.4lw.commands.whitelist ?

          We'll need to verify the list; 4 letters, from the possible set of commands available.

          Show
          phunt Patrick Hunt added a comment - That makes sense to me (scoping). What will the default list of 4lw be? 3.4: ruok,srvr,crst,srst,isro,mntr 3.5: <empty> Note: the full list of supported commands is different from 3.4 to 3.5 (possibly trunk?) - we'll need to ensure the docs reflect correctly. Do you think that makes sense? From what I can see these are low cost lookups. Some of the items I left off are more expensive or questionable in terms of whether they should be exposed if a firewall is not used. Or should 3.4 include all commands aside from the two identified in this jira? I'm thinking be safe (smaller list), document this clearly in the docs and in the release notes, and allow the users interesting in exposing more 4lw to do so. Downside is that users may be impacted, i.e. would have to update production configurations. What should we call this? zookeeper.4lw.commands.whitelist ? We'll need to verify the list; 4 letters, from the possible set of commands available.
          Hide
          hanm Michael Han added a comment -

          I'm thinking be safe (smaller list)

          I am leaning towards this directions as well.

          zookeeper.4lw.commands.whitelist

          Sounds a good name.

          I'll send patch shortly.

          Show
          hanm Michael Han added a comment - I'm thinking be safe (smaller list) I am leaning towards this directions as well. zookeeper.4lw.commands.whitelist Sounds a good name. I'll send patch shortly.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          usage limit that would cause the system to swap.</para>
          </listitem>
          </varlistentry>
          +
          + <varlistentry>
          + <term>Publicly accessible deployment</term>
          +
          + <listitem>
          + <para>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.

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> usage limit that would cause the system to swap.</para> </listitem> </varlistentry> + + <varlistentry> + <term>Publicly accessible deployment</term> + + <listitem> + <para>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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          </listitem>
          </varlistentry>

          + <varlistentry>
          + <term>fourLetterWordsEnabled</term>
          +
          + <listitem>
          + <para>(No Java system property)</para>
          +
          + <para><emphasis role="bold">New in 3.5.3:</emphasis>
          + This controls the enabling or disabling of <ulink url="#sc_4lw">
          + Four Letter Words feature</ulink>, which is
          + deprecated in favor of <ulink url="#sc_adminserver">AdminServer</ulink>.
          + <emphasis role="bold">"fourLetterWordsEnabled"</emphasis> option can be set as
          + <emphasis role="bold">"fourLetterWordsEnabled=false"</emphasis> or
          + <emphasis role="bold">"fourLetterWordsEnabled=true"</emphasis>
          + to a server's config file, or using QuorumPeerConfig's
          — End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> </listitem> </varlistentry> + <varlistentry> + <term>fourLetterWordsEnabled</term> + + <listitem> + <para>(No Java system property)</para> + + <para><emphasis role="bold">New in 3.5.3:</emphasis> + This controls the enabling or disabling of <ulink url="#sc_4lw"> + Four Letter Words feature</ulink>, which is + deprecated in favor of <ulink url="#sc_adminserver">AdminServer</ulink>. + <emphasis role="bold">"fourLetterWordsEnabled"</emphasis> option can be set as + <emphasis role="bold">"fourLetterWordsEnabled=false"</emphasis> or + <emphasis role="bold">"fourLetterWordsEnabled=true"</emphasis> + to a server's config file, or using QuorumPeerConfig's — End diff – nit: "in a server's config file or using"
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          hanm Michael Han added a comment -

          Patrick Hunt Updated patch (for 3.5 only) is here, please let me know your feedback. If it looks good, I'll create another pull request for branch-3.4.

          Show
          hanm Michael Han added a comment - Patrick Hunt Updated patch (for 3.5 only) is here , please let me know your feedback. If it looks good, I'll create another pull request for branch-3.4.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          @rakeshadr , @arshadmohammad feedback on this patch will be appreciated. It is a blocker for both ongoing 3.5 and 3.4 releases.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 @rakeshadr , @arshadmohammad feedback on this patch will be appreciated. It is a blocker for both ongoing 3.5 and 3.4 releases.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on the issue:

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

          +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/179 +1
          Hide
          phunt Patrick Hunt added a comment -

          I believe we need to get a CVE number assigned, step 8 here:
          https://www.apache.org/security/committers.html

          How about:

          "ZooKeeper DOS attack on four letter words (4lw)"

          Show
          phunt Patrick Hunt added a comment - I believe we need to get a CVE number assigned, step 8 here: https://www.apache.org/security/committers.html How about: "ZooKeeper DOS attack on four letter words (4lw)"
          Hide
          arshad.mohammad Mohammad Arshad added a comment -

          Can we restrict 4lw commands based on IP
          By default we can allow access to the IP on which server is running.
          It can be configured to allow individual IPs(192.168.1.2,192.168.1.3 etc)
          It can also be configured to allow group of IPs like 192.168.1.*

          Show
          arshad.mohammad Mohammad Arshad added a comment - Can we restrict 4lw commands based on IP By default we can allow access to the IP on which server is running. It can be configured to allow individual IPs(192.168.1.2,192.168.1.3 etc) It can also be configured to allow group of IPs like 192.168.1.*
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          while "srvr" and "cons" give extended details on server and
          connections respectively.</para>

          • <variablelist>
            + <para><emphasis role="bold">New in 3.5.3:</emphasis>
            + Four Letter Words need to be explicitly white listed before using.
            + Please refer <emphasis role="bold">4lw.commands.whitelist</emphasis>
            + described in <ulink url="#sc_clusterOptions">
            + cluster configuration section</ulink> 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> while "srvr" and "cons" give extended details on server and connections respectively.</para> <variablelist> + <para><emphasis role="bold">New in 3.5.3:</emphasis> + Four Letter Words need to be explicitly white listed before using. + Please refer <emphasis role="bold">4lw.commands.whitelist</emphasis> + described in <ulink url="#sc_clusterOptions"> + cluster configuration section</ulink> 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          </listitem>
          </varlistentry>

          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          — End diff –

          Could we stop making `Java system property` for configurations. There are nearly `17 4lwords` now and this list may grow in future. It would be easy for the users to configure comma separated values in `zoo.cfg` rather than passing via command line options, right?

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> </listitem> </varlistentry> + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis — End diff – Could we stop making `Java system property` for configurations. There are nearly `17 4lwords` now and this list may grow in future. It would be easy for the users to configure comma separated values in `zoo.cfg` rather than passing via command line options, right?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          </listitem>
          </varlistentry>

          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          — End diff –

          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, so I don't see why users have to "passing via command line options".

          Let's see if we don't make it as a system property - then what need change:

          • Implementation is complicated. We have to add additional logic in QuorumPeerConfig to parse this option explicitly. Right now it naturally fall through the generic system property parsing code.
          • Unit tests are harder to write. We'd need a way to set these values in unit tests and that implies we either have to add additional setter methods, or have to create zoo.cfg manually which is cumbersome.

          Thus I tend to keep the current implementation unless there is a strong reason not to make this also available as a system property.

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> </listitem> </varlistentry> + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis — End diff – 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, so I don't see why users have to "passing via command line options". Let's see if we don't make it as a system property - then what need change: Implementation is complicated. We have to add additional logic in QuorumPeerConfig to parse this option explicitly. Right now it naturally fall through the generic system property parsing code. Unit tests are harder to write. We'd need a way to set these values in unit tests and that implies we either have to add additional setter methods, or have to create zoo.cfg manually which is cumbersome. Thus I tend to keep the current implementation unless there is a strong reason not to make this also available as a system property.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          while "srvr" and "cons" give extended details on server and
          connections respectively.</para>

          • <variablelist>
            + <para><emphasis role="bold">New in 3.5.3:</emphasis>
            + Four Letter Words need to be explicitly white listed before using.
            + Please refer <emphasis role="bold">4lw.commands.whitelist</emphasis>
            + described in <ulink url="#sc_clusterOptions">
            + cluster configuration section</ulink> for details.
            + Moving forward, Four Letter Words will be deprecated, please use
              • End diff –

          Will do.

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> while "srvr" and "cons" give extended details on server and connections respectively.</para> <variablelist> + <para><emphasis role="bold">New in 3.5.3:</emphasis> + Four Letter Words need to be explicitly white listed before using. + Please refer <emphasis role="bold">4lw.commands.whitelist</emphasis> + described in <ulink url="#sc_clusterOptions"> + cluster configuration section</ulink> for details. + Moving forward, Four Letter Words will be deprecated, please use End diff – Will do.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          </listitem>
          </varlistentry>

          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          — End diff –

          >>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`

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> </listitem> </varlistentry> + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis — End diff – >>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`
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          </listitem>
          </varlistentry>

          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          — End diff –

          Yes, it is a nice trick - if a configuration option 'foobar' in zoo.cfg does not require explicitly handling, then it will implicitly being added as 'zookeeper.foobar' system property during parsing zoo.cfg, see [here](https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java#L296).

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> </listitem> </varlistentry> + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis — End diff – Yes, it is a nice trick - if a configuration option 'foobar' in zoo.cfg does not require explicitly handling, then it will implicitly being added as 'zookeeper.foobar' system property during parsing zoo.cfg, see [here] ( https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java#L296 ).
          Hide
          phunt Patrick Hunt added a comment -

          I wasn't very clear on my prior CVE comment. Clarification:

          Please review my suggestion for the summary of the CVE and comment here whether you think it's good or not. If I get a +1 I'll request a CVE number from the Apache security list.

          https://issues.apache.org/jira/browse/ZOOKEEPER-2693?focusedCommentId=15868897&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15868897

          Show
          phunt Patrick Hunt added a comment - I wasn't very clear on my prior CVE comment. Clarification: Please review my suggestion for the summary of the CVE and comment here whether you think it's good or not. If I get a +1 I'll request a CVE number from the Apache security list. https://issues.apache.org/jira/browse/ZOOKEEPER-2693?focusedCommentId=15868897&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15868897
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 17 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/339//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/339//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/339//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 17 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/339//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/339//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/339//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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</programlisting>
          </listitem>
          </varlistentry>

          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          — End diff –

          OK. +1 for the current approach.

          Show
          githubbot ASF GitHub Bot added a comment - 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</programlisting> </listitem> </varlistentry> + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis — End diff – OK. +1 for the current approach.
          Hide
          hanm Michael Han added a comment -

          If I get a +1 I'll request a CVE number from the Apache security list.

          +1. Thanks Patrick Hunt!

          Show
          hanm Michael Han added a comment - If I get a +1 I'll request a CVE number from the Apache security list. +1. Thanks Patrick Hunt !
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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?

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
              • End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); End diff – nit: `static final` (lines 160 and 162)
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> getWhiteListedCmdView() {
          — End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> getWhiteListedCmdView() { — End diff – nit: I would change this method name to something akin `getWhiteListedFourLetterCmds()` or `getWhiteListed4lw()`
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> getWhiteListedCmdView() {
          — End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> getWhiteListedCmdView() { — End diff – The class name already provide context on caller site - FourLetterCommands.getWhiteListedFourLetterCmd sounds redundant.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 20 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/340//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/340//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/340//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 20 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/340//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/340//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/340//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          Thanks everyone for feedback. Updated pull request to address your review comments. One change I made on latest update is to introduce an internal Java system property zookeeper.test.4lw.enabled for tests so we don't have to copy paste the lengthy set up code for zookeeper.4lw.commands.whitelist property and use zookeeper.test.4lw.enabled instead providing an elegant switch. zookeeper.4lw.commands.whitelist is still used in some tests to provide complete code coverage for new code paths introduced.

          All tests should be green now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 Thanks everyone for feedback. Updated pull request to address your review comments. One change I made on latest update is to introduce an internal Java system property zookeeper.test.4lw.enabled for tests so we don't have to copy paste the lengthy set up code for zookeeper.4lw.commands.whitelist property and use zookeeper.test.4lw.enabled instead providing an elegant switch. zookeeper.4lw.commands.whitelist is still used in some tests to provide complete code coverage for new code paths introduced. All tests should be green now.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 29 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/342//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/342//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/342//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 29 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/342//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/342//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/342//console This message is automatically generated.
          Hide
          hanm Michael Han added a comment -

          The test report seems wrong, all test passed here.

          Show
          hanm Michael Han added a comment - The test report seems wrong, all test passed here .
          Hide
          hanm Michael Han added a comment - - edited

          Can we restrict 4lw commands based on IP By default we can allow access to the IP on which server is running.

          Mohammad Arshad Thanks for feedback, this is one way of addressing the issue. I still prefer the current command white list approach because:

          • It has a smaller scope than the IP-restriction based approach. It is simpler, less cases to test, and easier to understand.
          • One case about IP based approach - what if the access point which IP is white listed gets compromised and admins are not aware of such case (so reconfigure the IP white list will not be done in time)? In that case, this exploit is still possible from the compromised and white listed access point. On the other side, the command white list approach does not have this issue, if the watcher monitoring commands listed in this issue are not white listed, there is no way to exploit.

          Overall I think the IP white list approach is a nice to have as it provides the option to use the entire sets of commands while mitigating the potential risk of being exploited - while the command white list approach is a must have based on my previous arguments. I propose we get the command white list patch in, and then the release out, and then think about how to improve the overall access control of ZK in the wild, unless the current command white list does not address the security concern raised by this JIRA.

          Show
          hanm Michael Han added a comment - - edited Can we restrict 4lw commands based on IP By default we can allow access to the IP on which server is running. Mohammad Arshad Thanks for feedback, this is one way of addressing the issue. I still prefer the current command white list approach because: It has a smaller scope than the IP-restriction based approach. It is simpler, less cases to test, and easier to understand. One case about IP based approach - what if the access point which IP is white listed gets compromised and admins are not aware of such case (so reconfigure the IP white list will not be done in time)? In that case, this exploit is still possible from the compromised and white listed access point. On the other side, the command white list approach does not have this issue, if the watcher monitoring commands listed in this issue are not white listed, there is no way to exploit. Overall I think the IP white list approach is a nice to have as it provides the option to use the entire sets of commands while mitigating the potential risk of being exploited - while the command white list approach is a must have based on my previous arguments. I propose we get the command white list patch in, and then the release out, and then think about how to improve the overall access control of ZK in the wild, unless the current command white list does not address the security concern raised by this JIRA.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist";
            +
            + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + final static Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> getWhiteListedCmdView() {
          — End diff –

          Yeah, you right.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + private static final String ZOOKEEPER_4LW_COMMANDS_WHITELIST = "zookeeper.4lw.commands.whitelist"; + + final static Map<Integer, String> cmd2String = new HashMap<Integer, String>(); + + final static Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> getWhiteListedCmdView() { — End diff – Yeah, you right.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eribeiro commented on the issue:

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

          +1. Great work, @hanm. Congrats.

          Show
          githubbot ASF GitHub Bot added a comment - Github user eribeiro commented on the issue: https://github.com/apache/zookeeper/pull/179 +1. Great work, @hanm. Congrats.
          Hide
          arshad.mohammad Mohammad Arshad added a comment -

          I propose we get the command white list patch in, and then the release out, and then think about how to improve the overall access control of ZK in the wild, unless the current command white list does not address the security concern raised by this JIRA.

          Michael Han, This makes sense to me. I have create new jira ZOOKEEPER-2699 and have put some more detail there.
          Sure, we can handle after this JIRA is merged. I will review this jira today

          Show
          arshad.mohammad Mohammad Arshad added a comment - I propose we get the command white list patch in, and then the release out, and then think about how to improve the overall access control of ZK in the wild, unless the current command white list does not address the security concern raised by this JIRA. Michael Han , This makes sense to me. I have create new jira ZOOKEEPER-2699 and have put some more detail there. Sure, we can handle after this JIRA is merged. I will review this jira today
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + private static final Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + + private static final Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + private static final Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> 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

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + + private static final Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 = 30000)
          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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 = 30000) 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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(30000);
          — End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - 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(30000); — End diff – The constructor Timeout(int) is deprecated use org.junit.rules.Timeout.Timeout(long timeout, TimeUnit timeUnit)
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.).

          Show
          githubbot ASF GitHub Bot added a comment - 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.).
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + private static final Set<String> whiteListedCommands = new HashSet<String>();

          public static Map<Integer, String> getCmdMapView()

          { return Collections.unmodifiableMap(cmd2String); }

          + // ZOOKEEPER-2693: Only allow white listed commands.
          + public static Set<String> getWhiteListedCmdView() {
          — End diff –

          Sounds reasonable.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + + private static final Set<String> whiteListedCommands = new HashSet<String>(); public static Map<Integer, String> getCmdMapView() { return Collections.unmodifiableMap(cmd2String); } + // ZOOKEEPER-2693 : Only allow white listed commands. + public static Set<String> getWhiteListedCmdView() { — End diff – Sounds reasonable.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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(30000);
          — End diff –

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

          Show
          githubbot ASF GitHub Bot added a comment - 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(30000); — End diff – Good catch - it was a copy paste from another test.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          Thanks @arshadmohammad for review. I'll update the patch soon.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 Thanks @arshadmohammad for review. I'll update the patch soon.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          Patch updated to address review comments from @arshadmohammad.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 Patch updated to address review comments from @arshadmohammad.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + private static final Set<String> whiteListedCommands = new HashSet<String>();
            +
            + 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + + private static final Set<String> whiteListedCommands = new HashSet<String>(); + + 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<Integer, String> cmd2String =
          • new HashMap<Integer, String>();
            + 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<Integer, String> cmd2String = new HashMap<Integer, String>();
            +
            + private static final Set<String> whiteListedCommands = new HashSet<String>();
            +
            + 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<Integer, String> 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.).

          Show
          githubbot ASF GitHub Bot added a comment - 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + 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<Integer, String> cmd2String = new HashMap<Integer, String>(); + + private static final Set<String> whiteListedCommands = new HashSet<String>(); + + 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<Integer, String> 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.).
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 .

          Show
          githubbot ASF GitHub Bot added a comment - 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 .
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/351//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/351//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/351//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/351//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/351//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/351//console This message is automatically generated.
          Hide
          arshad.mohammad Mohammad Arshad added a comment -

          3.4: ruok,srvr,crst,srst,isro,mntr, 3.5: <empty>

          There are some 4lw commands which ZooKeeper is using by itself
          For example

          1. srvr is used in zookeeper/bin/zkServer.sh status
          2. isro is used in org.apache.zookeeper.ClientCnxn.SendThread.pingRwServer()

          If we do not enable those commands by default, related funtionalities will not work, so we have to include in the default list
          But if we enable, I do not know if whole purpose of this fix is defeated because the attacker can call the these commands, even though we are not doing much work in these commands but still the connections will be created for every call.
          Any comments on which option to choose?

          Show
          arshad.mohammad Mohammad Arshad added a comment - 3.4: ruok,srvr,crst,srst,isro,mntr, 3.5: <empty> There are some 4lw commands which ZooKeeper is using by itself For example srvr is used in zookeeper/bin/zkServer.sh status isro is used in org.apache.zookeeper.ClientCnxn.SendThread.pingRwServer() If we do not enable those commands by default, related funtionalities will not work, so we have to include in the default list But if we enable, I do not know if whole purpose of this fix is defeated because the attacker can call the these commands, even though we are not doing much work in these commands but still the connections will be created for every call. Any comments on which option to choose?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          rakeshr Rakesh R added a comment -

          There are some 4lw commands which ZooKeeper is using by itself

          Good catch, Mohammad Arshad.

          even though we are not doing much work in these commands but still the connections will be created for every call.

          IIUC, these are two problems -> case-1) restrict 4lw cmd execution as few cmds taking too much time for execution. case-2) protection against overuse because it creates many connections.

          I think, case-1 is matching with this jira reported issue and we could provide whitelist config in both branches 3.4 and 3.5+ to solve this problem.

          In branch-3.4, we could give a simple fix by exposing whitelist configuration and include srvr, isro cmds in default list. Since we have plans to deprecate 4lws in branch-3.5, we could find alternative ways instead of using srvr, isro cmd internally. Probably, we could raise separate jira task and handle this case.
          Michael Han, could you create a PR for branch-3.4, if no objection from anyone about whitelist idea and that would help to unblock 3.4.10 releasing.

          case-2, I like Patrick Hunt's idea of introducing configuration to limit the number of 4lw that would be allowed to run in parallel. For example, allows only 1-n number of 4lw cmds to run in parallel. How about raising another jira to implement this instead of clubbing with this issue?

          Show
          rakeshr Rakesh R added a comment - There are some 4lw commands which ZooKeeper is using by itself Good catch, Mohammad Arshad . even though we are not doing much work in these commands but still the connections will be created for every call. IIUC, these are two problems -> case-1) restrict 4lw cmd execution as few cmds taking too much time for execution. case-2) protection against overuse because it creates many connections. I think, case-1 is matching with this jira reported issue and we could provide whitelist config in both branches 3.4 and 3.5+ to solve this problem. In branch-3.4, we could give a simple fix by exposing whitelist configuration and include srvr, isro cmds in default list. Since we have plans to deprecate 4lws in branch-3.5, we could find alternative ways instead of using srvr, isro cmd internally. Probably, we could raise separate jira task and handle this case. Michael Han , could you create a PR for branch-3.4, if no objection from anyone about whitelist idea and that would help to unblock 3.4.10 releasing. case-2 , I like Patrick Hunt 's idea of introducing configuration to limit the number of 4lw that would be allowed to run in parallel. For example, allows only 1-n number of 4lw cmds to run in parallel. How about raising another jira to implement this instead of clubbing with this issue?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 hmmmm 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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 hmmmm 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          hanm Michael Han added a comment -

          IIUC, these are two problems -> case-1) restrict 4lw cmd execution as few cmds taking too much time for execution. case-2) protection against overuse because it creates many connections.

          Yes, this is a good summary. Two problems - one is to fix the obvious exploits related to watcher 4lw and the other is to prevent abuse of 4lw in general. This JIRA's scope is targeting the first one, which fixes immediate issue and unblocks two important ongoing releases. We can easily get out of scope if we want to completely fix the security of the 4lw which was not designed with security in mind while balancing compatibility and minimize disrupt to existing users, so I'd recommend we stick to the current scope (unless as I mentioned earlier folks feel strongly against the white list approach.).

          could you create a PR for branch-3.4

          I will once I get this landed in 3.5. PR to 3.4 will not be much different, but I'd like to finalize this PR first to avoid potential duplicated efforts.

          Meanwhile, I'll create a set of follow JIRAs to address concerns of abusing 4lw in general:

          • A new config option to turn on / off 4lw w/o a middle ground (sure we can use empty white list for this purpose but a separate option is better IMO from the point of view of deprecating a feature.).
          • 4lw rate limiting including concurrent command runs configuration.
          • Fix client / script to avoid using 4lw - it is unfortunate ZK itself depends on 4lw.
          Show
          hanm Michael Han added a comment - IIUC, these are two problems -> case-1) restrict 4lw cmd execution as few cmds taking too much time for execution. case-2) protection against overuse because it creates many connections. Yes, this is a good summary. Two problems - one is to fix the obvious exploits related to watcher 4lw and the other is to prevent abuse of 4lw in general. This JIRA's scope is targeting the first one, which fixes immediate issue and unblocks two important ongoing releases. We can easily get out of scope if we want to completely fix the security of the 4lw which was not designed with security in mind while balancing compatibility and minimize disrupt to existing users, so I'd recommend we stick to the current scope (unless as I mentioned earlier folks feel strongly against the white list approach.). could you create a PR for branch-3.4 I will once I get this landed in 3.5. PR to 3.4 will not be much different, but I'd like to finalize this PR first to avoid potential duplicated efforts. Meanwhile, I'll create a set of follow JIRAs to address concerns of abusing 4lw in general: A new config option to turn on / off 4lw w/o a middle ground (sure we can use empty white list for this purpose but a separate option is better IMO from the point of view of deprecating a feature.). 4lw rate limiting including concurrent command runs configuration. Fix client / script to avoid using 4lw - it is unfortunate ZK itself depends on 4lw.
          Hide
          hanm Michael Han added a comment -

          srvr is used in zookeeper/bin/zkServer.sh status

          isro is used in org.apache.zookeeper.ClientCnxn.SendThread.pingRwServer()

          Good catch Mohammad Arshad - I hope this is an exhaustive list of 4lw used by ZK are there other commands used by ZK itself if you may know?

          Read only server is disabled by default, so we can leave isro out of white list by default and document in admin manual that if read only server is enabled, this command must be put back in white list. We can use a separate JIRA to get ride of isro from ZooKeeper client library later.

          For srvr, it is only used in zkServer.sh's stat option - not sure if anyone actually use this feature but we could just remove the Stat option from zkServer.sh so we don't have to include srvr in whitelist. Another option is to include srvr in white list by default for 3.4/3.5. I think include it by default in whitelist sounds the way to go from a compatibility point of view.

          Show
          hanm Michael Han added a comment - srvr is used in zookeeper/bin/zkServer.sh status isro is used in org.apache.zookeeper.ClientCnxn.SendThread.pingRwServer() Good catch Mohammad Arshad - I hope this is an exhaustive list of 4lw used by ZK are there other commands used by ZK itself if you may know? Read only server is disabled by default, so we can leave isro out of white list by default and document in admin manual that if read only server is enabled, this command must be put back in white list. We can use a separate JIRA to get ride of isro from ZooKeeper client library later. For srvr, it is only used in zkServer.sh's stat option - not sure if anyone actually use this feature but we could just remove the Stat option from zkServer.sh so we don't have to include srvr in whitelist. Another option is to include srvr in white list by default for 3.4/3.5. I think include it by default in whitelist sounds the way to go from a compatibility point of view.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          Update patch to address review comments from @arshadmohammad and @rakeshadr

          • Added a nop command that does nothing but print message back to client for better user experience.
          • Test case update.

          I think the remaining issue is to decide how to deal with isro and srvr which is being discussed on jira.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 Update patch to address review comments from @arshadmohammad and @rakeshadr Added a nop command that does nothing but print message back to client for better user experience. Test case update. I think the remaining issue is to decide how to deal with isro and srvr which is being discussed on jira.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/366//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/366//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/366//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/366//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/366//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/366//console This message is automatically generated.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks Michael Han for making good progress.

          I will once I get this landed in 3.5. PR to 3.4 will not be much different, but I'd like to finalize this PR first to avoid potential duplicated efforts.
          Meanwhile, I'll create a set of follow JIRAs to address concerns of abusing 4lw in general:

          Makes sense to me. If I understand your comment correctly, we could push whitelist config changes first which will unblock 3.4.10 releasing. But for 3.5.3 releasing, we need both whitelist config and deprecating 4lw tasks together. Probably, 4lw rate limiting task is nice to have feature(keep it low priority compare to other tasks), right?

          Show
          rakeshr Rakesh R added a comment - Thanks Michael Han for making good progress. I will once I get this landed in 3.5. PR to 3.4 will not be much different, but I'd like to finalize this PR first to avoid potential duplicated efforts. Meanwhile, I'll create a set of follow JIRAs to address concerns of abusing 4lw in general: Makes sense to me. If I understand your comment correctly, we could push whitelist config changes first which will unblock 3.4.10 releasing. But for 3.5.3 releasing, we need both whitelist config and deprecating 4lw tasks together. Probably, 4lw rate limiting task is nice to have feature(keep it low priority compare to other tasks), right?
          Hide
          hanm Michael Han added a comment -

          But for 3.5.3 releasing, we need both whitelist config and deprecating 4lw tasks together.

          Rakesh R I am thinking we can deprecating 4lw usage for 3.5 later, it does not sound a must have for addressing the specific security concern raised in this JIRA. I think the current patch is ready to land after we figure out how to deal with isro and srvr command currently used by ZK.

          Show
          hanm Michael Han added a comment - But for 3.5.3 releasing, we need both whitelist config and deprecating 4lw tasks together. Rakesh R I am thinking we can deprecating 4lw usage for 3.5 later, it does not sound a must have for addressing the specific security concern raised in this JIRA. I think the current patch is ready to land after we figure out how to deal with isro and srvr command currently used by ZK.
          Hide
          rakeshr Rakesh R added a comment -

          I'm trying an attempt to unblock 3.4.10 and 3.5.3 releases. Following are few proposals to make the release happen by including the reported issue.

          3.4.10 requirement:
          Expose whitelist configuration with the default values,
          4lw.commands.whitelist=ruok,srvr,crst,srst,isro,mntr
          Also, properly documenting the Publicly accessible deployment part from the current PR_179 changes.

          3.5.3 requirement:
          IMHO, there are two possible proposals:

          1. Proposal-1)
            Expose whitelist configuration with the default values,
            4lw.commands.whitelist=srvr,isro
            Also, recommend users to use admin server rather than 4lw cmds considering that 4lw will deprecated in future.
            Then, later in 3.5.4, we could make this whitelist <empty> and while deprecating will replace srvr, isro calls with better solution. That way, we would get enough time to think about better solutions.
          2. Proposal-2)
            Expose whitelist configuration with the empty default value,
            4lw.commands.whitelist=<empty>
          • (a) How about zkServer.sh usage of "srvr" can be achieved like,
            Introduce a new admin API, admin#getServerStatus("host:port"), which will return running stat of that server(probably, the same string format where srvr command is returning)
            "version=<full_version>
            read_only=false
            server_stats=<statistics>
            node_count=<count>"
            
          • (b) Client#pingRwServer=> Just a plain thought, probably, readonly client code can internally tries to establish new client session with all other servers round robin fashion. Then, check whether that the connected server is in rw mode and act upon.
          Show
          rakeshr Rakesh R added a comment - I'm trying an attempt to unblock 3.4.10 and 3.5.3 releases. Following are few proposals to make the release happen by including the reported issue. 3.4.10 requirement: Expose whitelist configuration with the default values, 4lw.commands.whitelist=ruok,srvr,crst,srst,isro,mntr Also, properly documenting the Publicly accessible deployment part from the current PR_179 changes. 3.5.3 requirement: IMHO, there are two possible proposals: Proposal-1) Expose whitelist configuration with the default values, 4lw.commands.whitelist=srvr,isro Also, recommend users to use admin server rather than 4lw cmds considering that 4lw will deprecated in future. Then, later in 3.5.4, we could make this whitelist <empty> and while deprecating will replace srvr, isro calls with better solution. That way, we would get enough time to think about better solutions. Proposal-2) Expose whitelist configuration with the empty default value, 4lw.commands.whitelist=<empty> (a) How about zkServer.sh usage of "srvr" can be achieved like, Introduce a new admin API, admin#getServerStatus("host:port"), which will return running stat of that server(probably, the same string format where srvr command is returning) "version=<full_version> read_only= false server_stats=<statistics> node_count=<count>" (b) Client#pingRwServer=> Just a plain thought, probably, readonly client code can internally tries to establish new client session with all other servers round robin fashion. Then, check whether that the connected server is in rw mode and act upon.
          Hide
          hanm Michael Han added a comment -

          Patch updated following proposal 1 for branch-3.5 (put isro and srvr in white list by default.). I'll create PR to branch-3.4 once patch lands to 3.5.

          Show
          hanm Michael Han added a comment - Patch updated following proposal 1 for branch-3.5 (put isro and srvr in white list by default.). I'll create PR to branch-3.4 once patch lands to 3.5.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/368//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/368//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/368//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/368//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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=<empty>`, then `System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST)` will be null and these two commands will not be added to `whiteListedCommands`, right?

          Show
          githubbot ASF GitHub Bot added a comment - 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=<empty>`, then `System.getProperty(ZOOKEEPER_4LW_COMMANDS_WHITELIST)` will be null and these two commands will not be added to `whiteListedCommands`, right?
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/371//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/371//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/371//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/371//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/371//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/371//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.).

          Show
          githubbot ASF GitHub Bot added a comment - 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.).
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/372//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/372//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/372//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/372//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/372//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/372//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr commented on the issue:

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

          Thanks @hanm, +1 LGTM.

          Hi @phunt, @arshadmohammad, do you have some cycle to review the final patch, would be great to see another +1 votes as this is critical . Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on the issue: https://github.com/apache/zookeeper/pull/179 Thanks @hanm, +1 LGTM. Hi @phunt, @arshadmohammad, do you have some cycle to review the final patch, would be great to see another +1 votes as this is critical . Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user arshadmohammad commented on the issue:

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

          Currently enabling all the 4lw commands is little inconvenient. We have to put all the commands one by one to enable all the commands. Given the fact that ZooKeeper is generally installed in private network within secure boundaries. Can we introduce some keyword to include all the commands? For example
          4lw.commands.whitelist=all
          or
          4lw.commands.whitelist=*

          Show
          githubbot ASF GitHub Bot added a comment - Github user arshadmohammad commented on the issue: https://github.com/apache/zookeeper/pull/179 Currently enabling all the 4lw commands is little inconvenient. We have to put all the commands one by one to enable all the commands. Given the fact that ZooKeeper is generally installed in private network within secure boundaries. Can we introduce some keyword to include all the commands? For example 4lw.commands.whitelist=all or 4lw.commands.whitelist=*
          Hide
          githubbot ASF GitHub Bot added a comment -

          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=30000)
          + 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

          Show
          githubbot ASF GitHub Bot added a comment - 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=30000) + 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          >> Can we introduce some keyword to include all the commands?
          Sounds reasonable. Will add 4lw.commands.whitelist=* as an option.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 >> Can we introduce some keyword to include all the commands? Sounds reasonable. Will add 4lw.commands.whitelist=* as an option.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/373//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/373//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/373//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/373//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/373//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/373//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

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

          Another stab to address comments from @arshadmohammad

          • Introduced 4lw.commands.whitelist=* as a convient configuration option to enable all 4lw, with new test.
          • Remove dead code.
          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/179 Another stab to address comments from @arshadmohammad Introduced 4lw.commands.whitelist=* as a convient configuration option to enable all 4lw, with new test. Remove dead code.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/374//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/374//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/374//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/374//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/374//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/374//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user arshadmohammad commented on the issue:

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

          Thanks @hanm for working on this issue. +1 LGTM.

          Show
          githubbot ASF GitHub Bot added a comment - Github user arshadmohammad commented on the issue: https://github.com/apache/zookeeper/pull/179 Thanks @hanm for working on this issue. +1 LGTM.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/179
          Hide
          hanm Michael Han added a comment -

          Thanks everyone for spending time on review / providing feedback.
          Committed to master: https://github.com/apache/zookeeper/commit/5fe68506f217246c7ebd96803f9c78e13ec2f11a
          Committed to branch-3.5: https://github.com/apache/zookeeper/commit/6d9fc04c052adbc79bbbb1c63f3f00c816fb8e56
          Patch to branch-3.4 will be based on the committed patch with small tweaks, and will be created as a separate pull request.

          Show
          hanm Michael Han added a comment - Thanks everyone for spending time on review / providing feedback. Committed to master: https://github.com/apache/zookeeper/commit/5fe68506f217246c7ebd96803f9c78e13ec2f11a Committed to branch-3.5: https://github.com/apache/zookeeper/commit/6d9fc04c052adbc79bbbb1c63f3f00c816fb8e56 Patch to branch-3.4 will be based on the committed patch with small tweaks, and will be created as a separate pull request.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3299 (See https://builds.apache.org/job/ZooKeeper-trunk/3299/)
          ZOOKEEPER-2693: DOS attack on wchp/wchc four letter words (4lw) (hanm: rev 5fe68506f217246c7ebd96803f9c78e13ec2f11a)

          • (edit) src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
          • (edit) src/java/test/org/apache/zookeeper/ZKTestCase.java
          • (edit) src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java
          • (edit) src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
          • (add) src/java/main/org/apache/zookeeper/server/command/NopCommand.java
          • (edit) src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java
          • (add) src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3299 (See https://builds.apache.org/job/ZooKeeper-trunk/3299/ ) ZOOKEEPER-2693 : DOS attack on wchp/wchc four letter words (4lw) (hanm: rev 5fe68506f217246c7ebd96803f9c78e13ec2f11a) (edit) src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (edit) src/java/test/org/apache/zookeeper/ZKTestCase.java (edit) src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java (edit) src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java (add) src/java/main/org/apache/zookeeper/server/command/NopCommand.java (edit) src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java (add) src/java/test/org/apache/zookeeper/test/FourLetterWordsWhiteListTest.java
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user hanm opened a pull request:

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

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

          Similar as pull request 179, this PR introduces new property zookeeper.4lw.commands.whitelist to branch-3.4.
          Unlike branch-3.5 where all 4lw (with few exceptions) is disabled by default, for branch-3.4 only "wchp" and "wchc" are disabled by default - since 4lw is widely used and there is no alternatives in branch-3.4 so we just disable the exploitable ones.

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

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

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

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



          Show
          githubbot ASF GitHub Bot added a comment - GitHub user hanm opened a pull request: https://github.com/apache/zookeeper/pull/183 ZOOKEEPER-2693 : DOS attack on wchp/wchc four letter words (4lw). Similar as pull request 179, this PR introduces new property zookeeper.4lw.commands.whitelist to branch-3.4. Unlike branch-3.5 where all 4lw (with few exceptions) is disabled by default, for branch-3.4 only "wchp" and "wchc" are disabled by default - since 4lw is widely used and there is no alternatives in branch-3.4 so we just disable the exploitable ones. You can merge this pull request into a Git repository by running: $ git pull https://github.com/hanm/zookeeper ZOOKEEPER-2693 -br-3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/183.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 #183
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/zookeeper/pull/183#discussion_r104572803

          — Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml —
          @@ -1042,6 +1042,40 @@ server.3=zoo3:2888:3888</programlisting>
          </note>
          </listitem>
          </varlistentry>
          +
          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          + role="bold">zookeeper.4lw.commands.whitelist</emphasis>)</para>
          +
          + <para><emphasis role="bold">New in 3.4.10:</emphasis>
          + This property contains a list of comma separated
          + <ulink url="#sc_4lw">Four Letter Words</ulink> commands. It is introduced
          + to provide fine grained control over the set of commands ZooKeeper can execute,
          + so users can turn off certain commands if necessary.
          + By default it contains all supported four letter word commands except "wchp" and "wchc",
          + if the property is not specified. If the property is specified, then only commands listed
          + in the whitelist are enabled.
          + </para>
          +
          + <para>Here's an example of the configuration that enables stat, ruok, conf, and isro
          + command while disabling the rest of Four Letter Words command:</para>
          + <programlisting>
          + 4lw.commands.whitelist=stat, ruok, conf, isro
          + </programlisting>
          +
          + <para>Users can also use asterisk option so they don't have to include every command one by one in the list.
          + As an example, this will enable all four letter word commands:
          + </para>
          + <programlisting>
          + 4lw.commands.whitelist=*
          + </programlisting>
          +
          + </listitem>
          + </varlistentry>
          +
          </variablelist>
          — End diff –

          The below section is not included in br-3.4 patch, can we include this also?
          ```
          + <varlistentry>
          + <term>Publicly accessible deployment</term>
          + <listitem>
          + <para>
          + A ZooKeeper ensemble is expected to operate in a trusted computing environment.
          + It is thus recommended to deploy ZooKeeper behind a firewall.
          + </para>
          + </listitem>
          + </varlistentry>
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/183#discussion_r104572803 — Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml — @@ -1042,6 +1042,40 @@ server.3=zoo3:2888:3888</programlisting> </note> </listitem> </varlistentry> + + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis + role="bold">zookeeper.4lw.commands.whitelist</emphasis>)</para> + + <para><emphasis role="bold">New in 3.4.10:</emphasis> + This property contains a list of comma separated + <ulink url="#sc_4lw">Four Letter Words</ulink> commands. It is introduced + to provide fine grained control over the set of commands ZooKeeper can execute, + so users can turn off certain commands if necessary. + By default it contains all supported four letter word commands except "wchp" and "wchc", + if the property is not specified. If the property is specified, then only commands listed + in the whitelist are enabled. + </para> + + <para>Here's an example of the configuration that enables stat, ruok, conf, and isro + command while disabling the rest of Four Letter Words command:</para> + <programlisting> + 4lw.commands.whitelist=stat, ruok, conf, isro + </programlisting> + + <para>Users can also use asterisk option so they don't have to include every command one by one in the list. + As an example, this will enable all four letter word commands: + </para> + <programlisting> + 4lw.commands.whitelist=* + </programlisting> + + </listitem> + </varlistentry> + </variablelist> — End diff – The below section is not included in br-3.4 patch, can we include this also? ``` + <varlistentry> + <term>Publicly accessible deployment</term> + <listitem> + <para> + A ZooKeeper ensemble is expected to operate in a trusted computing environment. + It is thus recommended to deploy ZooKeeper behind a firewall. + </para> + </listitem> + </varlistentry> ```
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/zookeeper/pull/183#discussion_r104579958

          — Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml —
          @@ -1042,6 +1042,40 @@ server.3=zoo3:2888:3888</programlisting>
          </note>
          </listitem>
          </varlistentry>
          +
          + <varlistentry>
          + <term>4lw.commands.whitelist</term>
          +
          + <listitem>
          + <para>(Java system property: <emphasis
          + role="bold">zookeeper.4lw.commands.whitelist</emphasis>)</para>
          +
          + <para><emphasis role="bold">New in 3.4.10:</emphasis>
          + This property contains a list of comma separated
          + <ulink url="#sc_4lw">Four Letter Words</ulink> commands. It is introduced
          + to provide fine grained control over the set of commands ZooKeeper can execute,
          + so users can turn off certain commands if necessary.
          + By default it contains all supported four letter word commands except "wchp" and "wchc",
          + if the property is not specified. If the property is specified, then only commands listed
          + in the whitelist are enabled.
          + </para>
          +
          + <para>Here's an example of the configuration that enables stat, ruok, conf, and isro
          + command while disabling the rest of Four Letter Words command:</para>
          + <programlisting>
          + 4lw.commands.whitelist=stat, ruok, conf, isro
          + </programlisting>
          +
          + <para>Users can also use asterisk option so they don't have to include every command one by one in the list.
          + As an example, this will enable all four letter word commands:
          + </para>
          + <programlisting>
          + 4lw.commands.whitelist=*
          + </programlisting>
          +
          + </listitem>
          + </varlistentry>
          +
          </variablelist>
          — End diff –

          Included.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/183#discussion_r104579958 — Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml — @@ -1042,6 +1042,40 @@ server.3=zoo3:2888:3888</programlisting> </note> </listitem> </varlistentry> + + <varlistentry> + <term>4lw.commands.whitelist</term> + + <listitem> + <para>(Java system property: <emphasis + role="bold">zookeeper.4lw.commands.whitelist</emphasis>)</para> + + <para><emphasis role="bold">New in 3.4.10:</emphasis> + This property contains a list of comma separated + <ulink url="#sc_4lw">Four Letter Words</ulink> commands. It is introduced + to provide fine grained control over the set of commands ZooKeeper can execute, + so users can turn off certain commands if necessary. + By default it contains all supported four letter word commands except "wchp" and "wchc", + if the property is not specified. If the property is specified, then only commands listed + in the whitelist are enabled. + </para> + + <para>Here's an example of the configuration that enables stat, ruok, conf, and isro + command while disabling the rest of Four Letter Words command:</para> + <programlisting> + 4lw.commands.whitelist=stat, ruok, conf, isro + </programlisting> + + <para>Users can also use asterisk option so they don't have to include every command one by one in the list. + As an example, this will enable all four letter word commands: + </para> + <programlisting> + 4lw.commands.whitelist=* + </programlisting> + + </listitem> + </varlistentry> + </variablelist> — End diff – Included.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user rakeshadr commented on the issue:

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

          Thanks @hanm. +1 LGTM

          Show
          githubbot ASF GitHub Bot added a comment - Github user rakeshadr commented on the issue: https://github.com/apache/zookeeper/pull/183 Thanks @hanm. +1 LGTM
          Hide
          rakeshr Rakesh R added a comment -

          I will merge this shortly.

          Show
          rakeshr Rakesh R added a comment - I will merge this shortly.
          Hide
          rakeshr Rakesh R added a comment -

          Issue resolved by pull request 183
          https://github.com/apache/zookeeper/pull/183

          Show
          rakeshr Rakesh R added a comment - Issue resolved by pull request 183 https://github.com/apache/zookeeper/pull/183
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm closed the pull request at: https://github.com/apache/zookeeper/pull/183
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +0 tests included. The patch appears to be a documentation patch that doesn't require tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/427//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/427//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/427//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +0 tests included. The patch appears to be a documentation patch that doesn't require tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/427//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/427//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/427//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +0 tests included. The patch appears to be a documentation patch that doesn't require tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          -1 release audit. The applied patch generated 2 release audit warnings (more than the trunk's current 0 warnings).

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +0 tests included. The patch appears to be a documentation patch that doesn't require tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. -1 release audit. The applied patch generated 2 release audit warnings (more than the trunk's current 0 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/429//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +0 tests included. The patch appears to be a documentation patch that doesn't require tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 97 new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/428//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/428//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/428//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +0 tests included. The patch appears to be a documentation patch that doesn't require tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 97 new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/428//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/428//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/428//console This message is automatically generated.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3319 (See https://builds.apache.org/job/ZooKeeper-trunk/3319/)
          ZOOKEEPER-2726 ZOOKEEPER-2693: Patch for introduces potential race (hanm: rev 0313a0e0b6c47b316271533165e5830d1ca04478)

          • (edit) src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3319 (See https://builds.apache.org/job/ZooKeeper-trunk/3319/ ) ZOOKEEPER-2726 ZOOKEEPER-2693 : Patch for introduces potential race (hanm: rev 0313a0e0b6c47b316271533165e5830d1ca04478) (edit) src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java
          Hide
          breed Benjamin Reed added a comment -

          can someone put a good link to the exploit in the description? a cache isn't an appropriate link to use.

          Show
          breed Benjamin Reed added a comment - can someone put a good link to the exploit in the description? a cache isn't an appropriate link to use.
          Show
          hanm Michael Han added a comment - Benjamin Reed https://vulners.com/exploitdb/EDB-ID:41277

            People

            • Assignee:
              hanm Michael Han
              Reporter:
              phunt Patrick Hunt
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development