You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by revans2 <gi...@git.apache.org> on 2017/06/14 14:14:24 UTC
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
GitHub user revans2 opened a pull request:
https://github.com/apache/zookeeper/pull/282
ZOOKEEPER-1782: Let a SASL super user be super
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/zookeeper ZOOKEEPER-1782
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/282.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 #282
----
commit 62ee0bcedbbbf34b6baf6effac24765d714928cc
Author: Robert Evans <ev...@yahoo-inc.com>
Date: 2017-06-14T14:13:03Z
ZOOKEEPER-1782: Let a SASL super user be super
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/282
@arshadmohammad I think I have addressed your review comments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122268144
--- Diff: src/java/test/org/apache/zookeeper/test/SaslSuperUserTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * 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.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooDefs.Perms;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SaslSuperUserTest extends ClientBase {
+ private static Id otherSaslUser = new Id ("sasl", "joe");
+ private static Id otherDigestUser;
+
+ static {
+ System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.SASLAuthenticationProvider");
+
+ try {
+ File tmpDir = createTmpDir();
+ File saslConfFile = new File(tmpDir, "jaas.conf");
+ FileWriter fwriter = new FileWriter(saslConfFile);
+
+ fwriter.write("" +
+ "Server {\n" +
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+ " user_super_duper=\"test\";\n" +
+ "};\n" +
+ "Client {\n" +
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+ " username=\"super_duper\"\n" +
+ " password=\"test\";\n" +
+ "};" + "\n");
+ fwriter.close();
+ System.setProperty("java.security.auth.login.config",saslConfFile.getAbsolutePath());
+ System.setProperty("zookeeper.superUser","super_duper");
+ otherDigestUser = new Id ("digest", DigestAuthenticationProvider.generateDigest("jack:jack"));
+ }
+ catch (IOException e) {
+ // could not create tmp directory to hold JAAS conf file : test will fail now.
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e); //This should never happen, but if it does we still blow up
+ }
+ }
+
+ private AtomicInteger authFailed = new AtomicInteger(0);
+
+ @Override
+ protected TestableZooKeeper createClient(String hp)
+ throws IOException, InterruptedException
+ {
+ MyWatcher watcher = new MyWatcher();
+ return createClient(watcher, hp);
+ }
+
+ private class MyWatcher extends CountdownWatcher {
+ @Override
+ public synchronized void process(WatchedEvent event) {
+ if (event.getState() == KeeperState.AuthFailed) {
+ authFailed.incrementAndGet();
+ }
+ else {
+ super.process(event);
+ }
+ }
+ }
+
+ @Test
+ public void testSuperIsSuper() throws Exception {
--- End diff --
This test does not verify anything. Maybe put an assert in a catch block to indicate no auth error should happen during these operations?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/282
merged, thanks for the work @revans2 !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/282
`zookeeper.superUser` was added in ZOOKEEPER-938 and it was never documented. I doubt people even know about this property without looking into source code and/or the description of ZOOKEEPER-938, but I agree compatibility concern here is very reasonable.
So let's stick to `zookeeper.superUser` for this patch. We can give it a shinny name later. The only remaining work is to document this `zookeeper.superUser`. The doc source is located https://github.com/apache/zookeeper/blob/master/src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, search `zookeeper.DigestAuthenticationProvider.superDigest` or `X509AuthenticationProvider.superUser` as references of existing superuser documents.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122268277
--- Diff: src/java/test/org/apache/zookeeper/test/SaslSuperUserTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * 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.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooDefs.Perms;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SaslSuperUserTest extends ClientBase {
+ private static Id otherSaslUser = new Id ("sasl", "joe");
+ private static Id otherDigestUser;
+
+ static {
+ System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.SASLAuthenticationProvider");
+
+ try {
+ File tmpDir = createTmpDir();
+ File saslConfFile = new File(tmpDir, "jaas.conf");
+ FileWriter fwriter = new FileWriter(saslConfFile);
+
+ fwriter.write("" +
+ "Server {\n" +
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+ " user_super_duper=\"test\";\n" +
+ "};\n" +
+ "Client {\n" +
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+ " username=\"super_duper\"\n" +
+ " password=\"test\";\n" +
+ "};" + "\n");
+ fwriter.close();
+ System.setProperty("java.security.auth.login.config",saslConfFile.getAbsolutePath());
+ System.setProperty("zookeeper.superUser","super_duper");
+ otherDigestUser = new Id ("digest", DigestAuthenticationProvider.generateDigest("jack:jack"));
+ }
+ catch (IOException e) {
+ // could not create tmp directory to hold JAAS conf file : test will fail now.
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e); //This should never happen, but if it does we still blow up
+ }
+ }
+
+ private AtomicInteger authFailed = new AtomicInteger(0);
+
+ @Override
+ protected TestableZooKeeper createClient(String hp)
+ throws IOException, InterruptedException
+ {
+ MyWatcher watcher = new MyWatcher();
+ return createClient(watcher, hp);
+ }
+
+ private class MyWatcher extends CountdownWatcher {
+ @Override
+ public synchronized void process(WatchedEvent event) {
+ if (event.getState() == KeeperState.AuthFailed) {
+ authFailed.incrementAndGet();
+ }
+ else {
+ super.process(event);
+ }
+ }
+ }
+
+ @Test
+ public void testSuperIsSuper() throws Exception {
+ ZooKeeper zk = createClient();
+ try {
+ zk.create("/digest_read", null, Arrays.asList(new ACL(Perms.READ, otherDigestUser)), CreateMode.PERSISTENT);
+ zk.create("/digest_read/sub", null, Arrays.asList(new ACL(Perms.READ, otherDigestUser)), CreateMode.PERSISTENT);
+ zk.create("/sasl_read", null, Arrays.asList(new ACL(Perms.READ, otherSaslUser)), CreateMode.PERSISTENT);
+ zk.create("/sasl_read/sub", null, Arrays.asList(new ACL(Perms.READ, otherSaslUser)), CreateMode.PERSISTENT);
+ zk.delete("/digest_read/sub", -1);
+ zk.delete("/digest_read", -1);
+ zk.delete("/sasl_read/sub", -1);
+ zk.delete("/sasl_read", -1);
+ Thread.sleep(1000);
--- End diff --
Can this sleep be removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/282
@hanm Thanks for the review. I think I have addressed all of your review comments. I originally put the patch up in 2013 and I have not touched it since so I honestly don't remember a lot about why I made the test that way to begin with.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/282
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122698936
--- Diff: src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java ---
@@ -38,11 +38,6 @@ public String getScheme() {
}
public boolean matches(String id,String aclExpr) {
- if (System.getProperty("zookeeper.superUser") != null) {
- if (id.equals(System.getProperty("zookeeper.superUser")) || id.equals(aclExpr)) {
- return true;
- }
- }
if ((id.equals("super") || id.equals(aclExpr))) {
--- End diff --
Sure. It looks like I am going to have to pull out the "super" user SASL Login too.
https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java#L42
https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java#L99-L101
Do you know where the source for the documentation at https://zookeeper.apache.org/doc/r3.3.3/zookeeperAdmin.html is? Under the section "Authentication & Authorization Options" it talks about how to use the "super" user.
I also found docs for this in a few other places on the open internet and because this is a backwards incompatible change I get a little nervous, so is there anything else I need to do to document a breaking change like this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122267787
--- Diff: src/java/test/org/apache/zookeeper/test/SaslSuperUserTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * 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.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooDefs.Perms;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SaslSuperUserTest extends ClientBase {
+ private static Id otherSaslUser = new Id ("sasl", "joe");
+ private static Id otherDigestUser;
+
+ static {
+ System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.SASLAuthenticationProvider");
+
+ try {
+ File tmpDir = createTmpDir();
+ File saslConfFile = new File(tmpDir, "jaas.conf");
+ FileWriter fwriter = new FileWriter(saslConfFile);
+
+ fwriter.write("" +
+ "Server {\n" +
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+ " user_super_duper=\"test\";\n" +
+ "};\n" +
+ "Client {\n" +
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n" +
+ " username=\"super_duper\"\n" +
+ " password=\"test\";\n" +
+ "};" + "\n");
+ fwriter.close();
+ System.setProperty("java.security.auth.login.config",saslConfFile.getAbsolutePath());
+ System.setProperty("zookeeper.superUser","super_duper");
+ otherDigestUser = new Id ("digest", DigestAuthenticationProvider.generateDigest("jack:jack"));
+ }
+ catch (IOException e) {
+ // could not create tmp directory to hold JAAS conf file : test will fail now.
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e); //This should never happen, but if it does we still blow up
+ }
+ }
+
+ private AtomicInteger authFailed = new AtomicInteger(0);
--- End diff --
Do we need this? Seems it's not used anywhere.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/282
One more thing here. Looks like we don't have an existing configuration option or system property called "superUser" (or zookeeper.superUser). So this `zookeeper.superUser` should be documented.
The existing approach of how superuser is defined under various auth provider:
* For digest, it's `zookeeper.DigestAuthenticationProvider.superDigest`.
* For ssl, it's `zookeeper.X509AuthenticationProvider.superUser`.
So maybe for SASL we name it `zookeeper.SASLAuthenticationProvider.superUser`, rather than `zookeeper.superUser` given it's only applicable to SASL?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122586398
--- Diff: src/java/test/org/apache/zookeeper/test/SaslSuperUserTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * 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.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs.Ids;
--- End diff --
The import org.apache.zookeeper.ZooDefs.Ids is never used.
There are many other unused imports as well
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r123330561
--- Diff: src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java ---
@@ -38,11 +38,6 @@ public String getScheme() {
}
public boolean matches(String id,String aclExpr) {
- if (System.getProperty("zookeeper.superUser") != null) {
- if (id.equals(System.getProperty("zookeeper.superUser")) || id.equals(aclExpr)) {
- return true;
- }
- }
if ((id.equals("super") || id.equals(aclExpr))) {
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r123329247
--- Diff: src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java ---
@@ -38,11 +38,6 @@ public String getScheme() {
}
public boolean matches(String id,String aclExpr) {
- if (System.getProperty("zookeeper.superUser") != null) {
- if (id.equals(System.getProperty("zookeeper.superUser")) || id.equals(aclExpr)) {
- return true;
- }
- }
if ((id.equals("super") || id.equals(aclExpr))) {
--- End diff --
Thanks @revans2 for the details.
On second thought, it would not be appropriate to break the backward compatibility. So can you please revert changes done for this comment. Rest all looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/282
@arshadmohammad if everything looks good I will squash my commits
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122586367
--- Diff: src/java/main/org/apache/zookeeper/server/auth/SASLAuthenticationProvider.java ---
@@ -38,11 +38,6 @@ public String getScheme() {
}
public boolean matches(String id,String aclExpr) {
- if (System.getProperty("zookeeper.superUser") != null) {
- if (id.equals(System.getProperty("zookeeper.superUser")) || id.equals(aclExpr)) {
- return true;
- }
- }
if ((id.equals("super") || id.equals(aclExpr))) {
--- End diff --
I think we should remove (id.equals("super") also. Lets have only one way being super user.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by arshadmohammad <gi...@git.apache.org>.
Github user arshadmohammad commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/282#discussion_r122586475
--- Diff: src/java/test/org/apache/zookeeper/test/SaslSuperUserTest.java ---
@@ -0,0 +1,118 @@
+/**
+ * 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.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.TestableZooKeeper;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooDefs.Perms;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SaslSuperUserTest extends ClientBase {
+ private static Id otherSaslUser = new Id ("sasl", "joe");
+ private static Id otherDigestUser;
+
+ static {
+ System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.SASLAuthenticationProvider");
--- End diff --
Whatever the system properties are set in this class should be cleared at the end of the class.
Similar test class org.apache.zookeeper.SaslAuthTest could be referred.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/282
@hanm I added in the docs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #282: ZOOKEEPER-1782: Let a SASL super user be super
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/282
@hanm I didn't add in `zookeeper.superUser`. I am happy to document it if you tell me where to make that change. I don't know where the documentation source is stored. I am happy to add in an additional `zookeeper.SASLAuthenticationProvider.superUser` config and document it instead, but removing `zookeeper.superUser` will technically break backwards compatibility. If that is what you want I can do it, I just want to be sure like on the other breaking change that was suggested.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---