[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Jordan Zimmerman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16002692#comment-16002692
 ] 

Jordan Zimmerman commented on ZOOKEEPER-2014:
-

I'm terribly sorry I was so late to this issue. Now that it's released I see 
even more problems. I just sent this email to @dev

{panel}
reconfig() is limited to "super" user. Perversely, this reduces security as 
"super" user is utterly insecure. Requiring new databases to be post-applied 
via super user creates a security hole. For the time that the new ACLs for 
/zookeeper/config are to be changed the ZooKeeper instance will be in "super" 
user mode. Additionally, having to do all this is terribly cumbersome. Lastly, 
the docs only make passing mention of this. I think users will be very 
surprised by this - especially as the docs refer users to 
ReconfigExceptionTest.java which isn't part of the client distribution.
{panel}

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3, 3.6.0
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Jordan Zimmerman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15728484#comment-15728484
 ] 

Jordan Zimmerman commented on ZOOKEEPER-2014:
-

I realize I'm very late to this issue but I truly don't understand the benefit 
of this. This change has completely broken Curator and I'm now struggling to 
figure out how to fix it. How does breaking all existing clients help ZooKeeper 
usage?

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3, 3.6.0
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hudson (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15662042#comment-15662042
 ] 

Hudson commented on ZOOKEEPER-2014:
---

FAILURE: Integrated in Jenkins build ZooKeeper-trunk #3155 (See 
[https://builds.apache.org/job/ZooKeeper-trunk/3155/])
ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster. (fpj: rev 
73e102a58d01b27bc6208bbfbde2d12f0deba1f4)
* (edit) src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
* (edit) 
src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java
* (edit) src/java/main/org/apache/zookeeper/ZooKeeper.java
* (edit) src/java/main/org/apache/zookeeper/cli/CliCommand.java
* (edit) src/java/test/org/apache/zookeeper/TestableZooKeeper.java
* (edit) src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java
* (edit) 
src/java/test/org/apache/zookeeper/server/quorum/ReconfigDuringLeaderSyncTest.java
* (edit) 
src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java
* (edit) src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java
* (edit) src/java/main/org/apache/zookeeper/KeeperException.java
* (edit) src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java
* (edit) src/java/test/org/apache/zookeeper/server/DataTreeTest.java
* (edit) src/c/tests/ZooKeeperQuorumServer.h
* (add) src/java/test/org/apache/zookeeper/test/ReconfigMisconfigTest.java
* (edit) src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml
* (edit) src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java
* (edit) src/c/tests/TestReconfigServer.cc
* (add) src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java
* (edit) src/c/include/zookeeper.h
* (edit) src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java
* (add) src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java
* (edit) src/java/test/org/apache/zookeeper/test/ACLTest.java
* (edit) build.xml
* (edit) src/java/main/org/apache/zookeeper/ZooKeeperMain.java
* (edit) src/java/main/org/apache/zookeeper/server/DataTree.java
* (edit) 
src/java/test/org/apache/zookeeper/server/quorum/ReconfigFailureCasesTest.java
* (edit) src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
* (edit) src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
* (edit) src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java
* (edit) src/java/test/org/apache/zookeeper/test/ReconfigTest.java
* (edit) src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml
* (edit) src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
* (edit) 
src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java
* (edit) src/java/test/org/apache/zookeeper/test/StandaloneTest.java
* (edit) src/c/tests/ZooKeeperQuorumServer.cc
* (edit) src/java/main/org/apache/zookeeper/ClientCnxn.java


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3, 3.6.0
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15662002#comment-15662002
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

Github user asfgit closed the pull request at:

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


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15656309#comment-15656309
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r87543684
  
--- Diff: src/java/test/org/apache/zookeeper/server/DataTreeTest.java ---
@@ -200,29 +198,34 @@ public void 
testSerializeDoesntLockDataNodeWhileWriting() throws Exception {
 BinaryOutputArchive oa = new BinaryOutputArchive(out) {
 @Override
 public void writeRecord(Record r, String tag) throws 
IOException {
-DataNode node = (DataNode) r;
-if (node.data.length == 1 && node.data[0] == 42) {
-final Semaphore semaphore = new Semaphore(0);
-new Thread(new Runnable() {
-@Override
-public void run() {
-synchronized (markerNode) {
-//When we lock markerNode, allow 
writeRecord to continue
-semaphore.release();
+// Need check if the record is a DataNode instance because 
of changes in ZOOKEEPER-2014
+// which adds default ACL to config node.
+if (r instanceof DataNode) {
--- End diff --

@fpj - we were not setting ACLs on intrinsic znodes (i.e. 
/zookeeper/config) ZooKeeper implicitly created while initializing a DataTree 
before. And for this test case, it only creates znodes, not ACLs. As a result, 
it's reasonable for the previous test case to assume every record that's 
serializing is a DataNode record. Now with this patch, there is an ACL 
implicitly created when /zookeeper/config node is created, so the previous 
assumption (that all records to be serialized are DataNode record) does not 
hold. Thus, a change is required.

For reference, you could put a break point on 
https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java#L133
 while running this test case, and you will see there is one ACL that's 
serialized. Now you can remove the ACL associated with /zookeeper/config at 
https://github.com/apache/zookeeper/pull/96/files#diff-a676d93082759105dd8c79c0a76a8007R259,
 and you will see the break point on ReferenceCountedACLCache.java previous set 
not get hit. That is the difference.

Another way to experiment this is to create an ACL in this test (without 
applying this pull request first), something like:
`final DataNode markerNode = tree.getNode("/marker");
  tree.setACL("/marker", ZooDefs.Ids.READ_ACL_UNSAFE, -1);` will do. Then 
we will see the same type casting failure - this simulates what this PR will do 
in terms of changing the type of records. Basically I think the root cause is 
the test itself could be made more robust, by eliminate the assumptions (that 
every record is a DataNode) that might not always hold.



> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15656130#comment-15656130
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r87536973
  
--- Diff: src/java/test/org/apache/zookeeper/server/DataTreeTest.java ---
@@ -200,29 +198,34 @@ public void 
testSerializeDoesntLockDataNodeWhileWriting() throws Exception {
 BinaryOutputArchive oa = new BinaryOutputArchive(out) {
 @Override
 public void writeRecord(Record r, String tag) throws 
IOException {
-DataNode node = (DataNode) r;
-if (node.data.length == 1 && node.data[0] == 42) {
-final Semaphore semaphore = new Semaphore(0);
-new Thread(new Runnable() {
-@Override
-public void run() {
-synchronized (markerNode) {
-//When we lock markerNode, allow 
writeRecord to continue
-semaphore.release();
+// Need check if the record is a DataNode instance because 
of changes in ZOOKEEPER-2014
+// which adds default ACL to config node.
+if (r instanceof DataNode) {
--- End diff --

@hanm hmm, I'm not sure about this. In the changes for `DataTree`, we only 
set the ACL of the `/zookeeper/config` znode, but setting ACLs was something we 
were doing before, so I'm confused about why we can have a mix of znode records 
and ACL records with the changes proposed here. Could you clarify, please?


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15645990#comment-15645990
 ] 

Michael Han commented on ZOOKEEPER-2014:


[~breed] We were previously waiting for ZOOKEEPER-2624 to land and for more 
review comments. Now ZOOKEEPER-2624 landed and more review comments were 
collected and addressed, I think this one is ready to land :-). cc [~fpj].

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Benjamin Reed (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15645962#comment-15645962
 ] 

Benjamin Reed commented on ZOOKEEPER-2014:
--

shall i commit it or are we waiting on something else?


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641347#comment-15641347
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12837643/ZOOKEEPER-2014.patch
  against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

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

+1 tests included.  The patch appears to include 57 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 appears to introduce 20 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-Build/3524//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3524//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3524//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641319#comment-15641319
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

Github user hanm commented on the issue:

https://github.com/apache/zookeeper/pull/96
  
Thanks @breed and @rgs1 for your time and review feedback. Pull request, 
patch, and review board is now updated.


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641216#comment-15641216
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86679034
  
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -245,15 +245,25 @@ public DataTree() {
 addConfigNode();
 }
 
- public void addConfigNode() {
-DataNode zookeeperZnode = nodes.get(procZookeeper);
- if (zookeeperZnode!=null) { // should always be the case
-zookeeperZnode.addChild(configChildZookeeper);
- } else {
-LOG.error("There's no /zookeeper znode - this should never 
happen");
- }
- nodes.put(configZookeeper, configDataNode);   
- }
+public void addConfigNode() {
+DataNode zookeeperZnode = nodes.get(procZookeeper);
+if (zookeeperZnode!=null) { // should always be the case
+zookeeperZnode.addChild(configChildZookeeper);
+} else {
+LOG.error("There's no /zookeeper znode - this should never 
happen.");
+}
+
+nodes.put(configZookeeper, configDataNode);
+try {
+// Reconfig node is access controlled by default 
(ZOOKEEPER-2014).
+setACL(configZookeeper, ZooDefs.Ids.READ_ACL_UNSAFE, -1);
+} catch (KeeperException.NoNodeException e) {
+LOG.error("Fail to set ACL on {} - this should never happen: 
{}", configZookeeper, e);
--- End diff --

actually if we are asserting above, perhaps we should also assert here.


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641220#comment-15641220
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86679058
  
--- Diff: src/java/test/org/apache/zookeeper/server/DataTreeTest.java ---
@@ -200,29 +198,34 @@ public void 
testSerializeDoesntLockDataNodeWhileWriting() throws Exception {
 BinaryOutputArchive oa = new BinaryOutputArchive(out) {
 @Override
 public void writeRecord(Record r, String tag) throws 
IOException {
-DataNode node = (DataNode) r;
-if (node.data.length == 1 && node.data[0] == 42) {
-final Semaphore semaphore = new Semaphore(0);
-new Thread(new Runnable() {
-@Override
-public void run() {
-synchronized (markerNode) {
-//When we lock markerNode, allow 
writeRecord to continue
-semaphore.release();
+// Need check if the record is a DataNode instance because 
of changes in ZOOKEEPER-2014
+// which adds default ACL to config node.
+if (r instanceof DataNode) {
--- End diff --

is there a reason we added the instanceof here? if we didn't need it 
before, why do we need it now?


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641217#comment-15641217
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86678939
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1108,6 +1109,42 @@ server.3=zoo3:2888:3888
   
 
   
+
+  
+reconfigEnabled
+
+
+  (No Java system property)
+
+  New in 3.5.3:
+This controls the enabling or disabling of
+
+  Dynamic Reconfiguration feature. When the feature
+is enabled, users can perform reconfigure operations 
through
+the ZooKeeper client API or through ZooKeeper command line 
tools
+assuming users are authorized to perform such operations.
+When the feature is disabled, no user, including the super 
user,
+can perform a reconfiguration. Any attempt to reconfigure 
will return an error.
+"reconfigEnabled" option 
can be set as
+"reconfigEnabled=false" or
+"reconfigEnabled=true"
+to a server's config file, or using QuorumPeerConfig's
+setReconfigEnabled method. The default value is false.
+
+If present, the value should be consistent across every 
server in
+the entire ensemble. Setting the value as true on some 
servers and false
+on other servers will cause inconsistent behavior depends 
on which server
--- End diff --

depends -> depending


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641219#comment-15641219
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86679094
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java ---
@@ -0,0 +1,220 @@
+/**
+ * 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 java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeoutException;
+
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.admin.ZooKeeperAdmin;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReconfigExceptionTest extends ZKTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(ReconfigExceptionTest.class);
+private static String authProvider = 
"zookeeper.DigestAuthenticationProvider.superDigest";
+// Use DigestAuthenticationProvider.base64Encode or
+// run ZooKeeper jar with 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider to generate 
password.
+// An example:
+// java -cp 
zookeeper-3.6.0-SNAPSHOT.jar:lib/log4j-1.2.17.jar:lib/slf4j-log4j12-1.7.5.jar:
+// lib/slf4j-api-1.7.5.jar 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider super:test
+// The password here is 'test'.
+private static String superDigest = 
"super:D/InIHSb7yEEbrWz8b9l71RjZJU=";
+private QuorumUtil qu;
+private ZooKeeperAdmin zkAdmin;
+
+@Before
+public void setup() throws InterruptedException, 
KeeperException.NoNodeException {
--- End diff --

we don't throw NoNodeException anymore


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641218#comment-15641218
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86679106
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java ---
@@ -0,0 +1,220 @@
+/**
+ * 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 java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeoutException;
+
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.admin.ZooKeeperAdmin;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReconfigExceptionTest extends ZKTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(ReconfigExceptionTest.class);
+private static String authProvider = 
"zookeeper.DigestAuthenticationProvider.superDigest";
+// Use DigestAuthenticationProvider.base64Encode or
+// run ZooKeeper jar with 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider to generate 
password.
+// An example:
+// java -cp 
zookeeper-3.6.0-SNAPSHOT.jar:lib/log4j-1.2.17.jar:lib/slf4j-log4j12-1.7.5.jar:
+// lib/slf4j-api-1.7.5.jar 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider super:test
+// The password here is 'test'.
+private static String superDigest = 
"super:D/InIHSb7yEEbrWz8b9l71RjZJU=";
+private QuorumUtil qu;
+private ZooKeeperAdmin zkAdmin;
+
+@Before
+public void setup() throws InterruptedException, 
KeeperException.NoNodeException {
--- End diff --

+1 awesome work! i only found little nits! thanx for sticking with this!


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640612#comment-15640612
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86673475
  
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -245,15 +245,25 @@ public DataTree() {
 addConfigNode();
 }
 
- public void addConfigNode() {
-DataNode zookeeperZnode = nodes.get(procZookeeper);
- if (zookeeperZnode!=null) { // should always be the case
-zookeeperZnode.addChild(configChildZookeeper);
- } else {
-LOG.error("There's no /zookeeper znode - this should never 
happen");
- }
- nodes.put(configZookeeper, configDataNode);   
- }
+public void addConfigNode() {
+DataNode zookeeperZnode = nodes.get(procZookeeper);
+if (zookeeperZnode!=null) { // should always be the case
--- End diff --

nit: spaces around `!=`


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640613#comment-15640613
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

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

https://github.com/apache/zookeeper/pull/96#discussion_r86673489
  
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -245,15 +245,25 @@ public DataTree() {
 addConfigNode();
 }
 
- public void addConfigNode() {
-DataNode zookeeperZnode = nodes.get(procZookeeper);
- if (zookeeperZnode!=null) { // should always be the case
-zookeeperZnode.addChild(configChildZookeeper);
- } else {
-LOG.error("There's no /zookeeper znode - this should never 
happen");
- }
- nodes.put(configZookeeper, configDataNode);   
- }
+public void addConfigNode() {
+DataNode zookeeperZnode = nodes.get(procZookeeper);
+if (zookeeperZnode!=null) { // should always be the case
--- End diff --

i think this should be an assertion (and we can drop the LOG.error call)


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15637058#comment-15637058
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12837180/ZOOKEEPER-2014.patch
  against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

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

+1 tests included.  The patch appears to include 60 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 appears to introduce 20 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-Build/3517//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3517//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3517//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15636990#comment-15636990
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12837178/ZOOKEEPER-2014.patch
  against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

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

+1 tests included.  The patch appears to include 62 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 appears to introduce 20 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-Build/3516//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3516//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3516//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15635390#comment-15635390
 ] 

Michael Han commented on ZOOKEEPER-2014:


Good point on throwing an unchecked exception which does not contaminate method 
signatures. The benefit of throwing a KeeperException here is minimum as the 
higher level code in ZooKeeperServerMain that processed typed exceptions 
currently does not specifically react to KeeperException (and there seems not 
much need to do so), so processing a KeeperException in ZooKeeperServerMain 
will end up with same code path as a RuntimeException. Let me update the patch 
again to keep it lean.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Benjamin Reed (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15635265#comment-15635265
 ] 

Benjamin Reed commented on ZOOKEEPER-2014:
--

ah that makes sense. i didn't dig deep enough :) it is sad that an exception 
"that should never happen" has such a big impact on the code. shouldn't we have 
thrown a runtime exception? i think it would have eliminated a lot of this 
patch...

this is just an observation not a vote :)

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15634520#comment-15634520
 ] 

Michael Han commented on ZOOKEEPER-2014:


NoNodeException is thrown from within DataTree's constructor, 
[addConfigNode|https://github.com/apache/zookeeper/pull/96/files#diff-a676d93082759105dd8c79c0a76a8007R264].
 The intention is in cases such exception happens, we bubble exception up to 
abort the server startup instead of letting the server starting up with 
incorrect ACLs configured on the config node. It's unfortunate that the 
exception has to be thrown from DataTree's constructor which is on the critical 
path that leads to NoNodeException being scattered everywhere.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Benjamin Reed (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15634422#comment-15634422
 ] 

Benjamin Reed commented on ZOOKEEPER-2014:
--

just a curious observer: why are we propagating the NoNodeException everywhere? 
i wasn't clear from the patch why that suddenly popped up as part of the change.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15624126#comment-15624126
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12836113/ZOOKEEPER-2014.patch
  against trunk revision f6349d16fcd5f04b560095417fd2a1813ac3e855.

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

+1 tests included.  The patch appears to include 96 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 2.0.3) 
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-Build/3507//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3507//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3507//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15621244#comment-15621244
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12836113/ZOOKEEPER-2014.patch
  against trunk revision f6349d16fcd5f04b560095417fd2a1813ac3e855.

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

+1 tests included.  The patch appears to include 96 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 2.0.3) 
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-Build/3504//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3504//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3504//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15621217#comment-15621217
 ] 

Michael Han commented on ZOOKEEPER-2014:


Thank you [~rakeshr] for your code review. Updated patch and PR to address your 
review comments. Will create a new JIRA to track the discrepancy between C / 
Java client regarding error code mismatch.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15621149#comment-15621149
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

GitHub user hanm opened a pull request:

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

ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster.

This PR implements ZOOKEEPER-2014. For details, please refer to

JIRA: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
Review board: https://reviews.apache.org/r/51546/

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

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

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

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


commit 6d18cffde99d4cf5298e045d6c0f23b36fd62925
Author: Michael Han 
Date:   2016-10-31T03:58:11Z

ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster.




> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15621145#comment-15621145
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

Github user hanm commented on the issue:

https://github.com/apache/zookeeper/pull/94
  
@fpj Sure, closing and resubmitting with a new one that also addressing 
Rakesh's comments today.


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Rakesh R (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15620175#comment-15620175
 ] 

Rakesh R commented on ZOOKEEPER-2014:
-

Thank you [~hanm] for the continuous effort. Sorry for the delay in reviews. 
I've looked at the latest patch and added few minor comments in the RB. 

+1 from me after addressing these comments.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15620024#comment-15620024
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

Github user fpj commented on the issue:

https://github.com/apache/zookeeper/pull/94
  
@hanm since there is no comment, would you mind closing this PR and 
resubmitting it to see if QA picks it up?


> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15612555#comment-15612555
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2014:
---

GitHub user hanm opened a pull request:

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

ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster.

This PR implements ZOOKEEPER-2014. For details, please refer to
* JIRA: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
* Previous review board: https://reviews.apache.org/r/51546/

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

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

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

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


commit 616e1275ac38890c2bf1e3ac27465172cf1c52d5
Author: Michael Han 
Date:   2016-10-27T16:16:27Z

ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster.




> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15584609#comment-15584609
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

+1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12833880/ZOOKEEPER-2014.patch
  against trunk revision cef5978969bedfe066f903834a9ea4af6d508844.

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

+1 tests included.  The patch appears to include 96 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 2.0.3) 
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-Build/3493//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3493//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3493//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15581886#comment-15581886
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

Thanks for the updates, [~hanm]. I want to do a few manual checks, but 
otherwise, the changes look good. I encourage others to have a look as well 
before we check it in.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15566912#comment-15566912
 ] 

Michael Han commented on ZOOKEEPER-2014:


Failed tests are known flaky..

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15566907#comment-15566907
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12832782/ZOOKEEPER-2014.patch
  against trunk revision f78061aafb19b102c37cb6d744ec6258d5f5b66e.

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

+1 tests included.  The patch appears to include 45 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 2.0.3) 
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-Build/3483//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3483//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3483//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15566745#comment-15566745
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12832770/ZOOKEEPER-2014.patch
  against trunk revision f78061aafb19b102c37cb6d744ec6258d5f5b66e.

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

+1 tests included.  The patch appears to include 45 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 appears to introduce 2 new Findbugs (version 2.0.3) 
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-Build/3481//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3481//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3481//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15566674#comment-15566674
 ] 

Michael Han commented on ZOOKEEPER-2014:


Yes keeping a single connection is right thing to do. Patch on the way.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15565419#comment-15565419
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

I checked that it works for me when reconfig is enabled. I have a couple of 
other things I wanted to raise:

# When I tried with reconfig disabled, I got this message:
{noformat}
reconfig -add server.5=127.0.0.1:1234:1235;1236
KeeperErrorCode = Reconfig is disabled for
{noformat}
And it should be only {{Reconfig disabled}}, unless we want to convey some 
other information.

# I have also verified that to get the reconfig command to go through we only 
need the leader to have {{reconfigEnabled = true}}. There is no way around it 
unless the replicas coordinate to use the same value. We need it well 
documented, though.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15565322#comment-15565322
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

Thanks for fixing it. I see the point of stability, but I don't think it is 
much of a concern in this case. You're just building on top of the ZooKeeper, 
you won't be re-wiring the existing ZooKeeper client code, I think. If I'm 
right, then it is better to have a single connection to the service rather than 
two, one for the ZooKeeper object and another for ZooKeeperAdmin. In any case, 
it might be worth giving it a shot to get a sense of whether it'd cause trouble.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15563874#comment-15563874
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

+1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12832546/ZOOKEEPER-2014.patch
  against trunk revision f78061aafb19b102c37cb6d744ec6258d5f5b66e.

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

+1 tests included.  The patch appears to include 42 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 2.0.3) 
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-Build/3480//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3480//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3480//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15563598#comment-15563598
 ] 

Michael Han commented on ZOOKEEPER-2014:


[~fpj] It's a bug, thanks for pointing it out! When we do reconfigure through 
command line, the {{addauth}} command will set the auth packet for the 
connection associated with the {{ZooKeeper}} object of the {{CliCommand}}. The 
reconfig command however is going to be executed by the {{ZooKeeperAdmin}} 
object of the {{CliCommand}}, and because addauth currently does not associate 
the auth packet to the underlying connection managed by {{ZooKeeperAdmin}}, 
when reconfig request hits server it will miss auth info, thus yield auth 
failure. The fix is to teach {{AddAuthCommand}} to set auth packet for 
{{ZooKeeperAdmin}} as well.

As we previously discussed in review board, an alternative is to have a single 
ZooKeeper object (since ZooKeeperAdmin is also a ZooKeeper) maintained in 
{{CliCommand}} so every command / request will converge to a single path. I did 
not do that for stability purposes - with a separate ZooKeeperAdmin object all 
existing commands (except reconfig) should not be impacted at all. I might be 
over cautious on this one though and maintaining a single ZK object inside 
CliCommand might be a better solution overall. Please let me know if you prefer 
one over the other.

Attaching updated patch (with one line change) that fixes the issue. Verified 
with zkCli.sh / reconfig command on a local 3 server cluster.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15561850#comment-15561850
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

[~hanm] I'm probably missing something obvious, but when I try to reconfigure, 
I get an auth failure, but I think I'm setting up everything right. This is the 
output of ZooKeeperMain plus my commands:

{noformat}
Connecting to localhost:2181
log4j:WARN No appenders could be found for logger 
(org.apache.zookeeper.ZooKeeper).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more 
info.
Welcome to ZooKeeper!
JLine support is disabled

WATCHER::

WATCHER::

WatchedEvent state:SyncConnected type:None path:null

WatchedEvent state:SyncConnected type:None path:null
addauth digest super:test
getAcl /zookeeper/config
'digest,'super:D/InIHSb7yEEbrWz8b9l71RjZJU=
: a
reconfig -add server.5=127.0.0.1:1234:1235;1236
Authentication is not valid : 
{noformat}

I have {{reconfigEnabled=true}} in the configuration of the servers.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15558418#comment-15558418
 ] 

Michael Han commented on ZOOKEEPER-2014:


[~fpj]: This is a flaky test, and it happened before this patch with exact same 
error message in Apache builds and precommit build. I did a search over my 
email archive and found a couple of instances (e.g. ZooKeeper_branch35_openjdk7 
- Build # 159). I'll create a separate JIRA and put it under ZOOKEEPER-2135.

On a side note, I have an internal Jenkins job that set up and stress test my 
patch branch of ZOOKEEPER-2014. There is no test failure except the one that's 
already tracked in ZOOKEEPER-2080. So this flaky testQuorumSystemChange one 
might also hard to reproduce.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15558081#comment-15558081
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

[~hanm] One of the tests that failed in the QA run is modified in the patch: 
{{testQuorumSystemChange}}. Could you have a look, please?

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15543981#comment-15543981
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12831433/ZOOKEEPER-2014.patch
  against trunk revision ec20c5434cc8a334b3fd25e27d26dccf4793c8f3.

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

+1 tests included.  The patch appears to include 42 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 2.0.3) 
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-Build/3462//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3462//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3462//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15531585#comment-15531585
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

+1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12830832/ZOOKEEPER-2014.patch
  against trunk revision ec20c5434cc8a334b3fd25e27d26dccf4793c8f3.

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

+1 tests included.  The patch appears to include 40 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 2.0.3) 
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-Build/3459//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3459//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3459//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15531402#comment-15531402
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12830808/ZOOKEEPER-2014.patch
  against trunk revision ec20c5434cc8a334b3fd25e27d26dccf4793c8f3.

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

+1 tests included.  The patch appears to include 40 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 2.0.3) 
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-Build/3458//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3458//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3458//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15531257#comment-15531257
 ] 

Michael Han commented on ZOOKEEPER-2014:


[~fpj]: Sounds good - actually just updated patch that incorporates some C side 
changes. PTAL when you have a chance, thanks a lot!

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15528898#comment-15528898
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

I'll wait until you have the C client changes to make another pass, unless you 
need feedback on some specific parts to make progress. [~hanm]

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Rakesh R (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15527075#comment-15527075
 ] 

Rakesh R commented on ZOOKEEPER-2014:
-

Thanks [~hanm] for working on this. I've added few comments in RB, please take 
a look at it.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15524413#comment-15524413
 ] 

Michael Han commented on ZOOKEEPER-2014:


Thanks a lot [~fpj] for your review comments. Just upload a new patch that 
address all of your review comments (minors the C client ones). The remaining 
work is on C client side where I plan to add more tests as Java side, which 
will be done in next few days and then I will upload final patch for review.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Flavio Junqueira (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15509210#comment-15509210
 ] 

Flavio Junqueira commented on ZOOKEEPER-2014:
-

I'll get to it before the end of this week, this is really important for the 
progress of the 3.5 branch.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15507418#comment-15507418
 ] 

Michael Han commented on ZOOKEEPER-2014:


Nudge nudge - anyone has cycles to take a look at the patch and provide 
feedback?
cc [~shralex], [~rgs], [~fpj], [~phunt] 

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 
> Review board https://reviews.apache.org/r/51546/



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15455970#comment-15455970
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

+1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12826655/ZOOKEEPER-2014.patch
  against trunk revision 1757584.

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

+1 tests included.  The patch appears to include 37 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 2.0.3) 
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-Build/3378//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3378//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3378//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15450446#comment-15450446
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12826264/ZOOKEEPER-2014.patch
  against trunk revision 1757584.

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

+1 tests included.  The patch appears to include 37 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 appears to introduce 1 new Findbugs (version 2.0.3) 
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-Build/3377//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3377//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3377//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15450116#comment-15450116
 ] 

Hadoop QA commented on ZOOKEEPER-2014:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12826240/ZOOKEEPER-2014.patch
  against trunk revision 1757584.

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

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

-1 patch.  The patch command could not apply the patch.

Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3376//console

This message is automatically generated.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch, ZOOKEEPER-2014.patch, 
> ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15432124#comment-15432124
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

I think that the general idea is that clients should be able to track the 
latest configuration and do something when it changes. For example if the 
server to which the client is connected goes away, it should have a way to 
query the system for the new list of servers and connect to one of them. The 
method you described is one option, outlined in the reconfig manual (right at 
the end there's some code for this). 

It would be nice to automate these steps for the user and to support a user 
defined policy instead of these steps (see some ideas in ZOOKEEPER-2016). For 
example, if you want the user to only connect to near-by servers, you'd need to 
filter the new list of servers before calling updateServerList. This can be 
done as part of a user-supplied policy.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15432063#comment-15432063
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

Hi Michael, all of this sounds good to me except for moving getConfig to 
ZooKeeperAdmin. I've explained why several times in the thread above (search 
for "getconfig"), please let me know if you disagree.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15384994#comment-15384994
 ] 

Michael Han commented on ZOOKEEPER-2014:


Had an offline discussion with [~phunt], [~fpj] yesterday regarding this issue, 
the conversation is captured as following points (with some of my thoughts as 
well):

* We can't fix security issue unless we enforce authentication and 
authorization. Just by moving client APIs around is not enough because at 
protocol level the server still open to reconfiguration and someone can exploit 
this easily (by writing their own ZK client instead of using ZooKeeper client 
for example.).

* That said though, the ZooKeeper::reconfig API should be moved out of 
ZooKeeper class (for Java client) anyway, because it's more about an admin 
feature rather than a client API. Moving reconfig out of ZooKeeper will also 
remove constraints we possibly put on normal ZooKeeper clients (such as having 
to use zookeeper.DigestAuthenticationProvider.superDigest), which is a bonus. 
Due to API backward compatibility concerns, this refactoring should happen 
before we move to beta.

* We'd like to introduce a new configuration option in zoo.cfg to turn off 
reconfig feature by default. ZK users who needs use this feature need turn it 
on explicitly. Because dynamic reconfig feature brings something new (e.g. 
cfg.dynamic file), have this feature off is good for 'the principal of least 
surprise'. Having the feature off by default will also buy us sometime to 
fortify and stabilize the feature without having it being a blocker issue.

The action items / plans I am thinking in the time frame of 3.5.3:
* Move ZooKeeper::reconfig into ServerAdminClient. ZooKeeper::getConfig will 
remain as is. 
* To run ServerAdminClient with reconfig command, clients need to use 
zookeeper.DigestAuthenticationProvider.superDigest and on server side we check 
superuser. This is basically what the current patch does today.  
* Introduce a new configuration option to turn on / off reconfig code path.

Are these good enough for now to address all security concerns or we still miss 
something?
Comments?

CC [~shralex], [~rgs]

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Michael Han
>Priority: Blocker
> Fix For: 3.5.3
>
> Attachments: ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Patrick Hunt (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15198842#comment-15198842
 ] 

Patrick Hunt commented on ZOOKEEPER-2014:
-

We can't (or at least promise we won't) change the api in a non b/w compatible 
way once it's post-alpha. As such I believe at a minimum we need to clean up 
the API (should be simple - move it out of ZooKeeper class) and ensure there 
are no security issues.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Raul Gutierrez Segales
>Priority: Blocker
> Fix For: 3.5.2
>
> Attachments: ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15198757#comment-15198757
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

[~rgs], does your patch solve the issue ? if not, what is still missing ? If I 
remember correctly my concern was that I'd like getConfig to be available to 
regular clients, not only admin, so they can react to configuration changes.

If this JIRA is what's blocking 3.5 perhaps we could reconsider the approach 
and go with something
simpler to start with, such as relying on ACLs. Or setting default ACLs for the 
config znode and requiring
client admins to have these permissions.

> Only admin should be allowed to reconfig a cluster
> --
>
> Key: ZOOKEEPER-2014
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.0
>Reporter: Raul Gutierrez Segales
>Assignee: Raul Gutierrez Segales
>Priority: Blocker
> Fix For: 3.5.2
>
> Attachments: ZOOKEEPER-2014.patch
>
>
> ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
> should, at the very least, ensure that only the Admin can reconfigure a 
> cluster. Perhaps restricting access to /zookeeper/config as well, though this 
> is debatable. Surely one could ensure Admin only access via an ACL, but that 
> would leave everyone who doesn't use ACLs unprotected. We could also force a 
> default ACL to make it a bit more consistent (maybe).
> Finally, making reconfig() only available to Admins means they have to run 
> with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
> if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Patrick Hunt (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14107813#comment-14107813
 ] 

Patrick Hunt commented on ZOOKEEPER-2014:
-

Yes, this is a concern for me as well.

1) mixing the client api and the admin api is not great. It would be better to 
have them separate. We should fix this asap.

2) this (controlling access to reconfig) is a big issue from a security 
perspective IMO.

A few comments on the comments so far:

bq. ensure that only the Admin can reconfigure a cluster

sounds sensible to me

bq. Perhaps restricting access to /zookeeper/config as well

in the past we've (ben in particular) tried to limit the amount of information 
we provide to the client/session. For example we don't tell them which server 
they are connected to. I see this in the same vein.

bq. one could ensure Admin only access via an ACL, but that would leave 
everyone who doesn't use ACLs unprotected.

well, you're already unprotected in this situation so I don't really see it as 
a sticking point.

bq.  clients may need read access for example in order to run the new 
client-side load balancing functionality

perhaps this argues for pushing this to the the server? encapsulate the 
information on the service I mean and expose as a specific api.

bq. Actually perhaps we should open a JIRA to hide the client-side rebalancing 
from clients (not for 3.5.0). 

yes, that's what I was trying to get at in the previous item in this comment. 
Although I think we'd need to fix this now - while we can still change the apis 
w/o worrying about b/w compat (we can change new apis during the alpha period 
w/o worrying about back compat)

bq. should this be similar to how updating the quota znode works ? or do you 
think changing configuration is different ?

if quotas are admin functions we probably need to fix those as well - lock 
them down to just admin level authz I mean.


 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14107837#comment-14107837
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

Everything you guys are saying about admin-only controls sounds very 
reasonable. I just want to clarify about the special reconfig znode. IMHO we 
should not allow write permissions to this node. I don't even see why an admin 
should have it :) its set only through the reconfig API. 

I do think that all clients should have read permissions, and here's why - the 
information they get from this znode is the up-to-date connection string. When 
the configuration changes this is the bare minimum they need in order not to 
loose track of the system. When their server crashes they need to know whom to 
connect to next. The new connection string is exactly the information we 
exploit for rebalancing. It is even implemented inside the updateServerList 
method, which is also needed in any case. 

Regarding my suggestion - Hongchao opened a JIRA for it and you can read the 
discussion there https://issues.apache.org/jira/browse/ZOOKEEPER-2016
Please especially see Marshall's comment.
All I'm proposing there is to implement some default behaviour that will save 
most clients from setting a watch and invoking updateServerList - I suggest 
that the client-side-library does it for them if they opt-in for the default 
behaviour. It doesn't change APIs, just adds one more feature, so it doesn't 
delay 3.5.0.







 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14106404#comment-14106404
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

[~rgs], should this be similar to how updating the quota znode works ? or do 
you think changing configuration is different ?

 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Raul Gutierrez Segales (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14106504#comment-14106504
 ] 

Raul Gutierrez Segales commented on ZOOKEEPER-2014:
---

(by which I mean, you can really bring a cluster down with this).

 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Raul Gutierrez Segales (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14106503#comment-14106503
 ] 

Raul Gutierrez Segales commented on ZOOKEEPER-2014:
---

[~shralex]: well, quotas rely on ACLs. But, quotas are (currently) advisory 
only (i.e.: we don't reject traffic) so no damage can be done by non-admins. So 
I think we need a stricter enforcement here, no?

 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14105041#comment-14105041
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

Thanks for starting this thread, Raul. I'm not sure what's the right way to 
handle the issue, but just wanted to mention a couple of things.

1. Regarding access to /zookeeper/config - clients may need read access for 
example in order to run the new client-side load balancing functionality, they 
need to detect a config change and get a new connection string. They can use 
getConfig for this. It is probably a good idea to restrict access for writes. 
Writing it probably won't break the system but may cause all clients to migrate 
to any server I choose to mention there, if clients are using the client-side 
load balancing algorithm.

2. Notice that when the leader processes reconfig in PrepRequestProcessor it 
already checks ACL:

   nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE);  
 
   checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, 
request.authInfo);  


 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Hongchao Deng (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14105040#comment-14105040
 ] 

Hongchao Deng commented on ZOOKEEPER-2014:
--

{quote}
 ensure that only the Admin can reconfigure a cluster.
{quote}

This change will reduce much flexibility in reconfig.
A counter scenario would be I have a process that detects permanent failed ZK 
servers and removes them to make a smaller quorum (better fault tolerance). 
Does the process have to be Admin? Or would a default ACL be a better option 
here?

 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Commented] (ZOOKEEPER-2014) Only admin should be allowed to reconfig a cluster

[Alexander Shraer (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14105052#comment-14105052
 ] 

Alexander Shraer commented on ZOOKEEPER-2014:
-

+[~fpj], [~michim]

 Only admin should be allowed to reconfig a cluster
 --

 Key: ZOOKEEPER-2014
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2014
 Project: ZooKeeper
  Issue Type: Bug
  Components: server
Affects Versions: 3.5.0
Reporter: Raul Gutierrez Segales
Assignee: Raul Gutierrez Segales
Priority: Blocker
 Attachments: ZOOKEEPER-2014.patch


 ZOOKEEPER-107 introduces reconfiguration support via the reconfig() call. We 
 should, at the very least, ensure that only the Admin can reconfigure a 
 cluster. Perhaps restricting access to /zookeeper/config as well, though this 
 is debatable. Surely one could ensure Admin only access via an ACL, but that 
 would leave everyone who doesn't use ACLs unprotected. We could also force a 
 default ACL to make it a bit more consistent (maybe).
 Finally, making reconfig() only available to Admins means they have to run 
 with zookeeper.DigestAuthenticationProvider.superDigest (which I am not sure 
 if everyone does, or how would it work with other authentication providers). 



--
This message was sent by Atlassian JIRA
(v6.2#6252)