[GitHub] [zookeeper] eolivelli commented on a change in pull request #1318: ZOOKEEPER-3696: support multiple algorithms for ACL digest

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #1318:
URL: https://github.com/apache/zookeeper/pull/1318#discussion_r508395559



##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/AuthSHA2Test.java
##########
@@ -0,0 +1,82 @@
+/*
+ * 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 static org.junit.jupiter.api.Assertions.assertEquals;
+import java.security.NoSuchAlgorithmException;
+import java.security.Security;
+import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class AuthSHA2Test extends AuthTest {
+
+    @BeforeEach
+    @Override
+    public void setup() {
+        // use the BouncyCastle's Provider for testing
+        Security.addProvider(new BouncyCastleProvider());

Review comment:
       this is going to pollute all of the other tests in an unpredictable way.
   is there a way to remove it ? 

##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1461,6 +1461,33 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     localhost (not over the network) or over an encrypted
     connection.
 
+* *DigestAuthenticationProvider.digestAlg* :
+    (Java system property: **zookeeper.DigestAuthenticationProvider.digestAlg**)
+     **New in 3.7.0:**
+    Set ACL digest algorithm. The default value is: `SHA1` which will be deprecated in the future for security issues.
+    Set this property the same value in all the servers.
+
+    - How to support other more algorithms?
+        - modify the `java.security` configuration file under `$JAVA_HOME/jre/lib/security/java.security` by specifying:
+             `security.provider.<n>=<provider class name>`.
+
+             ```
+             For example:
+             set zookeeper.DigestAuthenticationProvider.digestAlg=RipeMD160
+             security.provider.3=org.bouncycastle.jce.provider.BouncyCastleProvider
+             ```
+
+        - copy the jar file to `$JAVA_HOME/jre/lib/ext/`.

Review comment:
       isn't it enough to add it to the classpath ?
   
   Modifying the JRE is something that usually no one can do, because people have the JDK/JRE pre-installed or managed by some system and modifying the contents of the directory can be forbidden in many  environments.
   
   Also how to deal with this stuff in a Docker image ?

##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/AuthTest.java
##########
@@ -18,27 +18,45 @@
 
 package org.apache.zookeeper.test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.fail;
 import java.io.IOException;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.TestableZooKeeper;
 import org.apache.zookeeper.WatchedEvent;
 import org.apache.zookeeper.Watcher.Event.KeeperState;
+import org.apache.zookeeper.ZooDefs;
 import org.apache.zookeeper.ZooDefs.Ids;
 import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 public class AuthTest extends ClientBase {
 
-    static {
+    @BeforeEach
+    public void setup() {
         // password is test
+        // the default digestAlg is: SHA1
         System.setProperty("zookeeper.DigestAuthenticationProvider.superDigest", "super:D/InIHSb7yEEbrWz8b9l71RjZJU=");
         System.setProperty("zookeeper.authProvider.1", "org.apache.zookeeper.test.InvalidAuthProvider");
     }
 
+    @AfterEach
+    public void teardown() {
+        System.clearProperty("zookeeper.DigestAuthenticationProvider.superDigest");

Review comment:
       what about adding a constant for zookeeper.DigestAuthenticationProvider.superDigest ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org