> On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/c/tests/TestReconfigServer.cc, line 78 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490144#file1490144line78> > > > > Is this saying that we don't need to test ACLs with reconfig for the C > > client?
Yes - These changes are pretty selfish ones just to make C test pass :) On a second thought I think it is neccessary to add at least a minimum set of tests to check the sanity of reconfig with acl for C client. I'll add more C tests. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/c/tests/TestReconfigServer.cc, line 80 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490144#file1490144line80> > > > > To confirm, setting skipACL to true isn't strictly necessary here, > > right? This option is required here because we did not set up auth for C client tests - so skipACL took the short cut and just let the test pass. As previously commented, I'll add more tests for C client. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/c/tests/ZooKeeperQuorumServer.cc, line 105 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490146#file1490146line105> > > > > The additional parameter adds optional parameters, so perhaps the name > > should reflect that it isn't a full config, but additional paramters > > instead. > > > > I'm wondering if instead of passing a string, which requires the caller > > to format it properly, we whould just pass the paramters explicitly. I > > don't feel strongly about it, but I wanted to run the idea by you. Yes agree, the naming of the parameter is confusing. Better to name it as 'optionalConfig'. Regarding the option of passing parameters explicitly vs the option of passing a string that encodes optional additional parameter, I think passing a string is more flexible but passing explicitly will be safer and more convenient to use; probably we can take an approach in the middle, for example we can pass a list of variadic key-value pairs and then genereate the config strings from the list, so we get better control over the final formatted config string while maintaining the flexibility of the interface. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 942 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490147#file1490147line942> > > > > Should we actually explain what enabling/disabling reconfig entails? Sounds good idea, will add more details here. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 152 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490148#file1490148line152> > > > > You may want to motivate why we would consider disabling the feature. > > Something along the lines of: "With reconfiguration enabled, we have a > > security concern that a malicious actor can make arbitrary changes to the > > configuration of a ZooKeeper ensemble, including adding a compromised > > server to the ensemble. We prefer to leave to the discretion of the user to > > decide whether to enable it or not and make sure that the appropriate > > security measure are in place." Agreed. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 314 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490148#file1490148line314> > > > > There is a bit of taste involved in this description. The need of > > security depends on lots of things and it is rarely black or white. I'd > > rather say like I suggested above that some users were concerned with the > > possibility of ensembles being arbitrarily reconfigured and we added the > > option of disabling it, so it is really up to the user to decide what > > he/she wants to do. Good point. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 337 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490148#file1490148line337> > > > > The contract is that by default only the super user can change the > > configuration, yes? Yes, superuser by default can do anything - if superuser is compromised, all bets are off I think. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java, line 19 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490150#file1490150line19> > > > > Should this class be in a separate zookeeper.admin package? Agreed. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java, line 129 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490150#file1490150line129> > > > > Please remove all red (spaces and tabs) from this file. This is mostly > > if not all from copying it from ZooKeeper.java. Yes will do - same problem was pointed out by Edward as well. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/server/DataTree.java, line 249 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490154#file1490154line249> > > > > You may additionally want to say in the comment that by default it is > > world readable, but not writable. I know it is documented elsewhere, but > > for emphasis. > > > > Actually, what happens if skipACL is set to true? Are we able to > > violate our contract in this case? We need to warn the user in the case > > reconfig and skipACL are both enabled, or possibly not even start the > > server. Very good point - when skipACL is set we will allow reconfig proceed from any user. This should at least be documented. Regarding the behavior of both reconfig enabled and skipACL is set, I feel gave a warning to user might be better than not starting the server, because that provides an escape hatch for users who fully control their environment and want reconfig, but don't want to go through the process of configure the credentials and ACLs. It's up to user to decide what to do by giving them all options and risks at different level, which is more flexible. > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line > > 473 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490155#file1490155line473> > > > > This isn't really a case of bad argument, it is more like an > > unauthorized operation. does it make sense to create a new keeper exception > > for this? Yes, definitely makes sense. I actually thought about this initially and ends up reusing this code as I've seen some existing cases where we use this KeeperException (those are separate problems to fix.). > On Sept. 24, 2016, 3:02 p.m., fpj wrote: > > src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java, line 161 > > <https://reviews.apache.org/r/51546/diff/2/?file=1490163#file1490163line161> > > > > This is doing more than just resetting zk admin, no? Yeah it needs a better name. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51546/#review150301 ----------------------------------------------------------- On Sept. 1, 2016, 4:24 p.m., Michael Han wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51546/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2016, 4:24 p.m.) > > > Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and > Alexander Shraer. > > > Bugs: ZOOKEEPER-2014 > https://issues.apache.org/jira/browse/ZOOKEEPER-2014 > > > Repository: zookeeper-git > > > Description > ------- > > Address various security concerns around reconfig feature (ZOOKEEPER-2014) to > unblock 3.5.3 release. > > > Diffs > ----- > > build.xml 4173550 > src/c/tests/TestReconfigServer.cc 6a429ac > src/c/tests/ZooKeeperQuorumServer.h aa8b7cc > src/c/tests/ZooKeeperQuorumServer.cc 23392cd > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml f729095 > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml 7168a01 > src/java/main/org/apache/zookeeper/ZooKeeper.java f2ec3d7 > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeperMain.java 25d61a4 > src/java/main/org/apache/zookeeper/cli/CliCommand.java 3d0a90b > src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java deb7914 > src/java/main/org/apache/zookeeper/server/DataTree.java 06c80d3 > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 33f638d > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > e772fa8 > src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643 > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java > 241af52 > > src/java/test/org/apache/zookeeper/server/quorum/ReconfigFailureCasesTest.java > e7147b3 > src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java > ee9f2e2 > > src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java > 1f6ce1f > src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4 > src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/ReconfigTest.java 248a754 > src/java/test/org/apache/zookeeper/test/StandaloneTest.java 5c95280 > > Diff: https://reviews.apache.org/r/51546/diff/ > > > Testing > ------- > > Added new Java unit tests that cover various exception cases of using > reconfig API without proper set up. > All existing tests (Java and C) passed under stress tests (minors those known > flaky tests.). > Manuelly tested reconfig commands using ZooKeeper command line tool. > > > Thanks, > > Michael Han > >
