madrob commented on code in PR #857:
URL: https://github.com/apache/solr/pull/857#discussion_r930348564
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -256,18 +261,44 @@ protected ZkACLProvider createZkACLProvider() {
if (!StringUtils.isEmpty(zkACLProviderClassName)) {
try {
log.info("Using ZkACLProvider: {}", zkACLProviderClassName);
- return (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ ZkACLProvider zkACLProvider =
+ (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ zkACLProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkACLProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkACLProvider does not point to a class implementing
ZkACLProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkACLProvider");
+ log.warn(
+ "Using default ZkACLProvider. DefaultZkACLProvider is not secure, it
creates 'OPEN_ACL_UNSAFE' ACLs to Zookeeper nodes");
return new DefaultZkACLProvider();
}
+ public static final String ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME =
+ "zkCredentialsInjector";
+
+ protected ZkCredentialsInjector createZkCredentialsInjector() {
+ String zkCredentialsInjectorClassName =
+ System.getProperty(ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME);
+ if (!StringUtils.isEmpty(zkCredentialsInjectorClassName)) {
+ try {
+ log.info("Using ZkCredentialsInjector: {}",
zkCredentialsInjectorClassName);
+ return (ZkCredentialsInjector)
+
Class.forName(zkCredentialsInjectorClassName).getConstructor().newInstance();
+ } catch (Throwable t) {
+ // just ignore - go default
+ log.warn(
+ "VM param ZkCredentialsInjector does not point to a class
implementing ZkCredentialsInjector and with a non-arg constructor",
+ t);
+ }
+ }
+ log.warn("Using default ZkCredentialsInjector");
Review Comment:
is default ZKCI unsecure? add something actionable to this log statement -
why does the admin care?
##########
solr/core/src/java/org/apache/solr/core/CloudConfig.java:
##########
@@ -227,12 +236,19 @@ public CloudConfigBuilder
setLeaderConflictResolveWait(int leaderConflictResolve
}
public CloudConfigBuilder setZkCredentialsProviderClass(String
zkCredentialsProviderClass) {
- this.zkCredentialsProviderClass = zkCredentialsProviderClass;
+ this.zkCredentialsProviderClass =
+ zkCredentialsProviderClass != null ?
zkCredentialsProviderClass.trim() : null;
Review Comment:
Was this a bug before? I think I agree with this change, but wanted to make
sure.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -256,18 +261,44 @@ protected ZkACLProvider createZkACLProvider() {
if (!StringUtils.isEmpty(zkACLProviderClassName)) {
try {
log.info("Using ZkACLProvider: {}", zkACLProviderClassName);
- return (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ ZkACLProvider zkACLProvider =
+ (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ zkACLProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkACLProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkACLProvider does not point to a class implementing
ZkACLProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkACLProvider");
+ log.warn(
+ "Using default ZkACLProvider. DefaultZkACLProvider is not secure, it
creates 'OPEN_ACL_UNSAFE' ACLs to Zookeeper nodes");
return new DefaultZkACLProvider();
}
+ public static final String ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME =
+ "zkCredentialsInjector";
+
+ protected ZkCredentialsInjector createZkCredentialsInjector() {
+ String zkCredentialsInjectorClassName =
+ System.getProperty(ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME);
+ if (!StringUtils.isEmpty(zkCredentialsInjectorClassName)) {
+ try {
+ log.info("Using ZkCredentialsInjector: {}",
zkCredentialsInjectorClassName);
+ return (ZkCredentialsInjector)
+
Class.forName(zkCredentialsInjectorClassName).getConstructor().newInstance();
+ } catch (Throwable t) {
Review Comment:
We shouldn't be catching throwable, it will eat OOM and other Errors as
well. Please catch Exception at most (or the specific reflective exceptions).
We could fix it in the other usages too in this class or leave that for later
if you'd prefer, but let's not introduce new sloppiness.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -256,18 +261,44 @@ protected ZkACLProvider createZkACLProvider() {
if (!StringUtils.isEmpty(zkACLProviderClassName)) {
try {
log.info("Using ZkACLProvider: {}", zkACLProviderClassName);
- return (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ ZkACLProvider zkACLProvider =
+ (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ zkACLProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkACLProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkACLProvider does not point to a class implementing
ZkACLProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkACLProvider");
+ log.warn(
+ "Using default ZkACLProvider. DefaultZkACLProvider is not secure, it
creates 'OPEN_ACL_UNSAFE' ACLs to Zookeeper nodes");
return new DefaultZkACLProvider();
}
+ public static final String ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME =
+ "zkCredentialsInjector";
+
+ protected ZkCredentialsInjector createZkCredentialsInjector() {
+ String zkCredentialsInjectorClassName =
+ System.getProperty(ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME);
+ if (!StringUtils.isEmpty(zkCredentialsInjectorClassName)) {
+ try {
+ log.info("Using ZkCredentialsInjector: {}",
zkCredentialsInjectorClassName);
+ return (ZkCredentialsInjector)
+
Class.forName(zkCredentialsInjectorClassName).getConstructor().newInstance();
Review Comment:
use Class.forName.asSubclass instead of casting it.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -256,18 +261,44 @@ protected ZkACLProvider createZkACLProvider() {
if (!StringUtils.isEmpty(zkACLProviderClassName)) {
try {
log.info("Using ZkACLProvider: {}", zkACLProviderClassName);
- return (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ ZkACLProvider zkACLProvider =
+ (ZkACLProvider)
Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ zkACLProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkACLProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkACLProvider does not point to a class implementing
ZkACLProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkACLProvider");
+ log.warn(
+ "Using default ZkACLProvider. DefaultZkACLProvider is not secure, it
creates 'OPEN_ACL_UNSAFE' ACLs to Zookeeper nodes");
return new DefaultZkACLProvider();
}
+ public static final String ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME =
+ "zkCredentialsInjector";
+
+ protected ZkCredentialsInjector createZkCredentialsInjector() {
+ String zkCredentialsInjectorClassName =
+ System.getProperty(ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME);
Review Comment:
A bit confused on the order of precedence here... If this is used in a
client application without `solr.xml` defined then we pull the class name from
a system property which is _fine_, but if we are in the context of a Solr
Server where we _do_ have potential for custom configuration, shouldn't we
consult that instead?
I see that this same pattern is used for the other properties, can you
create a follow up issue for somebody to verify and address it?
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DigestZkACLProvider.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.solr.common.cloud;
+
+import static
org.apache.zookeeper.server.auth.DigestAuthenticationProvider.generateDigest;
+
+import java.lang.invoke.MethodHandles;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.cloud.ZkCredentialsInjector.ZkCredential;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A class that expects a {@link ZkCredentialsInjector} to create Zookeeper
ACLs using digest scheme
+ */
+public class DigestZkACLProvider extends SecurityAwareZkACLProvider {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /** Called by reflective instantiation */
+ public DigestZkACLProvider() {}
+
+ public DigestZkACLProvider(ZkCredentialsInjector zkCredentialsInjector) {
+ super(zkCredentialsInjector);
+ }
+
+ /**
+ * @return Set of ACLs to return for non-security related znodes
+ */
+ @Override
+ protected List<ACL> createNonSecurityACLsToAdd() {
+ return createACLsToAdd(true);
+ }
+
+ /**
+ * Provider ACL Credential
+ *
+ * @return Set of ACLs to return security-related znodes
+ */
+ @Override
+ protected List<ACL> createSecurityACLsToAdd() {
+ return createACLsToAdd(false);
+ }
+
+ protected List<ACL> createACLsToAdd(boolean includeReadOnly) {
+ List<ACL> result = new ArrayList<>();
+ List<ZkCredential> zkCredentials =
zkCredentialsInjector.getZkCredentials();
+ log.debug("createACLsToAdd using ZkCredentials: {}", zkCredentials);
+ for (ZkCredential zkCredential : zkCredentials) {
+ if (StringUtils.isEmpty(zkCredential.getUsername())
+ || StringUtils.isEmpty(zkCredential.getPassword())) {
+ continue;
+ }
+ Id id = createACLId(zkCredential.getUsername(),
zkCredential.getPassword());
+ int perms;
+ if (zkCredential.isAll()) {
+ // Not to have to provide too much credentials and ACL information to
the process it is
+ // assumed that you want "ALL"-acls
+ // added to the user you are using to connect to ZK
+ perms = ZooDefs.Perms.ALL;
+ } else if (includeReadOnly && zkCredential.isReadonly()) {
+ // Besides, that support for adding additional "READONLY"-acls for
another user
+ perms = ZooDefs.Perms.READ;
+ } else continue;
Review Comment:
since credentials are an enum, this is only possible in the don't include
read only case? can we document that better with a code comment? and add braces
for the continue, since I think it's very easy to miss while reading.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/VMParamsZkCredentialsInjector.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.solr.common.cloud;
+
+import static
org.apache.solr.common.cloud.ZkCredentialsInjector.ZkCredential.Perms;
+
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.lang.invoke.MethodHandles;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
+import org.apache.solr.common.SolrException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Reads credentials from System Properties and injects them into {@link
+ * DigestZkCredentialsProvider} & {@link DigestZkACLProvider} Usage:
+ *
+ * <pre>
+ *
-DzkCredentialsInjector=org.apache.solr.common.cloud.VMParamsZkCredentialsInjector
\
+ * -DzkDigestUsername=admin-user -DzkDigestPassword=CHANGEME-ADMIN-PASSWORD \
+ * -DzkDigestReadonlyUsername=readonly-user
-DzkDigestReadonlyPassword=CHANGEME-READONLY-PASSWORD
+ * </pre>
+ *
+ * Or from a Java property file:
+ *
+ * <pre>
+ *
-DzkCredentialsInjector=org.apache.solr.common.cloud.VMParamsZkCredentialsInjector
\
+ *
-DzkDigestCredentialsFile=SOLR_HOME_DIR/server/etc/zookeepercredentials.properties
+ * </pre>
+ *
+ * Example of a Java property file:
+ *
+ * <pre>
+ * zkDigestUsername=admin-user
+ * zkDigestPassword=CHANGEME-ADMIN-PASSWORD
+ * zkDigestReadonlyUsername=readonly-user
+ * zkDigestReadonlyPassword=CHANGEME-READONLY-PASSWORD
+ * </pre>
+ */
+public class VMParamsZkCredentialsInjector implements ZkCredentialsInjector {
+
+ public static final String DEFAULT_DIGEST_USERNAME_VM_PARAM_NAME =
"zkDigestUsername";
+ public static final String DEFAULT_DIGEST_PASSWORD_VM_PARAM_NAME =
"zkDigestPassword";
+ public static final String DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME =
+ "zkDigestReadonlyUsername";
+ public static final String DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME =
+ "zkDigestReadonlyPassword";
+ public static final String DEFAULT_DIGEST_FILE_VM_PARAM_NAME =
"zkDigestCredentialsFile";
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ final String zkDigestAllUsernameVMParamName;
+ final String zkDigestAllPasswordVMParamName;
+ final String zkDigestReadonlyUsernameVMParamName;
+ final String zkDigestReadonlyPasswordVMParamName;
+ final Properties credentialsProps;
+
+ public VMParamsZkCredentialsInjector() {
+ this(
+ DEFAULT_DIGEST_USERNAME_VM_PARAM_NAME,
+ DEFAULT_DIGEST_PASSWORD_VM_PARAM_NAME,
+ DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
+ DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME);
+ }
+
+ public VMParamsZkCredentialsInjector(
+ String zkDigestAllUsernameVMParamName,
+ String zkDigestAllPasswordVMParamName,
+ String zkDigestReadonlyUsernameVMParamName,
+ String zkDigestReadonlyPasswordVMParamName) {
+ this.zkDigestAllUsernameVMParamName = zkDigestAllUsernameVMParamName;
+ this.zkDigestAllPasswordVMParamName = zkDigestAllPasswordVMParamName;
+ this.zkDigestReadonlyUsernameVMParamName =
zkDigestReadonlyUsernameVMParamName;
+ this.zkDigestReadonlyPasswordVMParamName =
zkDigestReadonlyPasswordVMParamName;
+
+ String pathToFile = System.getProperty(DEFAULT_DIGEST_FILE_VM_PARAM_NAME);
+ credentialsProps =
+ (pathToFile != null) ? readCredentialsFile(pathToFile) :
System.getProperties();
Review Comment:
Does this need to be a live copy or a snapshot. If the file changes, values
don't get updated, but if the system properties change then this will be live.
Do we rely on this in our tests?
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractDigestZkACLAndCredentialsProvidersTestBase.java:
##########
@@ -0,0 +1,555 @@
+/*
+ * 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.solr.cloud;
+
+import static org.apache.solr.common.cloud.VMParamsZkCredentialsInjector.*;
+import static org.apache.zookeeper.ZooDefs.Ids.OPEN_ACL_UNSAFE;
+
+import java.io.FileWriter;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.*;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.cloud.*;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException.NoAuthException;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class AbstractDigestZkACLAndCredentialsProvidersTestBase extends
SolrTestCaseJ4 {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private static final Charset DATA_ENCODING = StandardCharsets.UTF_8;
+
+ private static final String ALL_USERNAME = "connectAndAllACLUsername";
+ private static final String ALL_PASSWORD = "connectAndAllACLPassword";
+ private static final String READONLY_USERNAME = "readonlyACLUsername";
+ private static final String READONLY_PASSWORD = "readonlyACLPassword";
+
+ public static final String SECRET_NAME = "zkCredentialsSecret";
+
+ protected ZkTestServer zkServer;
+
+ protected Path zkDir;
+
+ @BeforeClass
+ public static void beforeClass() {
+ System.setProperty("solrcloud.skip.autorecovery", "true");
+ }
+
+ @AfterClass
+ public static void afterClass() {
+ System.clearProperty("solrcloud.skip.autorecovery");
+ }
+
+ @Override
+ public void setUp() throws Exception {
+ super.setUp();
Review Comment:
Does all of this setup need to happen for each test case, or can it be done
once for the class? (i.e. `@BeforeClass`) and maybe some minor reset logic in
setup, instead of full startup and teardown of a new ZkTestServer in each cycle?
It's copied from the old class, can you leave a `//TODO` in the code for
future maintainers please :)
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
