You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/06/01 19:51:09 UTC
[GitHub] [solr] HoustonPutman commented on a diff in pull request #857: SOLR-16192: Add ZK credentials injectors support
HoustonPutman commented on code in PR #857:
URL: https://github.com/apache/solr/pull/857#discussion_r887204381
##########
solr/bin/solr.in.cmd:
##########
@@ -224,4 +236,5 @@ REM then enable the following setting to address CVE-2021-44228
REM set SOLR_OPTS=%SOLR_OPTS% -Dlog4j2.formatMsgNoLookups=true
REM The bundled plugins in the "modules" folder can easily be enabled as a comma-separated list in SOLR_MODULES variable
-REM set SOLR_MODULES=extraction,ltr
\ No newline at end of file
+REM set SOLR_MODULES=extraction,ltr
+REM set SOLRJ_MODULES=aws-secret-provider
Review Comment:
I don't think this belongs here (in this PR at least)
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -323,22 +329,29 @@ public ZkController(
ZkClientConnectionStrategy strat =
ZkClientConnectionStrategy.forName(connectionStrategy, new DefaultConnectionStrategy());
+ String zkCredentialsInjectorClass = cloudConfig.getZkCredentialsInjectorClass();
+ ZkCredentialsInjector zkCredentialsInjector =
+ !StringUtils.isEmpty(zkCredentialsInjectorClass)
Review Comment:
I'm ok with the ternary operator. But for these 3 checks, could you remove the `!` and swap the true/false conditions? It will make it easier to read.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/DigestZkCredentialsProvider.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.acl;
+
+import java.lang.invoke.MethodHandles;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.cloud.DefaultZkCredentialsProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A class that expects a {@link ZkCredentialsInjector} to create Zookeeper credentials using digest
+ * scheme
+ */
+public class DigestZkCredentialsProvider extends DefaultZkCredentialsProvider {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public DigestZkCredentialsProvider() {
+ this(new DefaultZkCredentialsInjector());
+ }
+
+ public DigestZkCredentialsProvider(ZkCredentialsInjector zkCredentialsInjector) {
+ this.zkCredentialsInjector = zkCredentialsInjector;
+ }
+
+ @Override
+ protected Collection<ZkCredentials> createCredentials() {
+ List<ZkCredentials> result = new ArrayList<>(1);
+ List<ZkCredentialsInjector.ZkCredential> zkCredentials =
+ zkCredentialsInjector.getZkCredentials();
+ log.debug("createCredentials using zkCredentials: {}", zkCredentials);
+ for (ZkCredentialsInjector.ZkCredential zkCredential : zkCredentials) {
+ if (zkCredential.isAll()) {
+ // this is the "user" with all perms that SolrZooKeeper uses to connect to zookeeper
+ if (!StringUtils.isEmpty(zkCredential.getUsername())
+ && !StringUtils.isEmpty(zkCredential.getPassword())) {
+ result.add(
+ new ZkCredentials(
+ "digest",
+ (zkCredential.getUsername() + ":" + zkCredential.getPassword())
Review Comment:
Should this also use `generateDigest()`? (like the ACL implementation)
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -236,8 +240,11 @@ protected ZkCredentialsProvider createZkCredentialsToAddAutomatically() {
if (!StringUtils.isEmpty(zkCredentialsProviderClassName)) {
try {
log.info("Using ZkCredentialsProvider: {}", zkCredentialsProviderClassName);
- return (ZkCredentialsProvider)
- Class.forName(zkCredentialsProviderClassName).getConstructor().newInstance();
+ ZkCredentialsProvider zkCredentialsProvider =
+ (ZkCredentialsProvider)
+ Class.forName(zkCredentialsProviderClassName).getConstructor().newInstance();
Review Comment:
We can't use Class's classloader here. At the very least we need to use `Thread.currentThread().getContextClassLoader().loadClass(zkCredentialsProviderClassName)`.
Same for the zkAclProvider below
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -529,6 +529,9 @@ private static CloudConfig fillSolrCloudSection(
case "zkCredentialsProvider":
builder.setZkCredentialsProviderClass(value);
break;
+ case "zkCredentialsInjector":
Review Comment:
If you are adding something to the Solr XML, you need to add an entry for it in the documentation:
https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/package-info.java:
##########
@@ -0,0 +1,19 @@
+/*
+ * 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.
+ */
+
+/** Classes for managing ACLs to Zookeeper */
+package org.apache.solr.common.cloud.acl;
Review Comment:
Should this package be called `acl`? It also manages ZKCredentials, right? Maybe `credentials` would be better.
Also is the current setup only to help with backwards compatibility? I'm unsure why some of the classes are in this `acl` package, and others are in the parent package.
##########
solr/bin/solr.in.sh:
##########
@@ -199,12 +199,25 @@
#SOLR_AUTHENTICATION_OPTS="-Dbasicauth=solr:SolrRocks"
# Settings for ZK ACL
-#SOLR_ZK_CREDS_AND_ACLS="-DzkACLProvider=org.apache.solr.common.cloud.VMParamsAllAndReadonlyDigestZkACLProvider \
-# -DzkCredentialsProvider=org.apache.solr.common.cloud.VMParamsSingleSetCredentialsDigestZkCredentialsProvider \
+#SOLR_ZK_CREDS_AND_ACLS="-DzkACLProvider=org.apache.solr.common.cloud.acl.DigestZkACLProvider \
+# -DzkCredentialsProvider=org.apache.solr.common.cloud.acl.DigestZkCredentialsProvider \
+# -DzkCredentialsInjector=org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector \
# -DzkDigestUsername=admin-user -DzkDigestPassword=CHANGEME-ADMIN-PASSWORD \
# -DzkDigestReadonlyUsername=readonly-user -DzkDigestReadonlyPassword=CHANGEME-READONLY-PASSWORD"
#SOLR_OPTS="$SOLR_OPTS $SOLR_ZK_CREDS_AND_ACLS"
+# optionally, you can use using a a Java properties file 'zkDigestCredentialsFile'
+#...
+# -DzkDigestCredentialsFile=/path/to/zkDigestCredentialsFile.properties
Review Comment:
Does this require a separate injector, or does it also use the `VMParamsZkCredentialsInjector`?
##########
solr/solrj/src/java/org/apache/solr/common/cloud/VMParamsAllAndReadonlyDigestZkACLProvider.java:
##########
@@ -16,134 +16,96 @@
*/
package org.apache.solr.common.cloud;
-import static org.apache.solr.common.cloud.VMParamsSingleSetCredentialsDigestZkCredentialsProvider.DEFAULT_DIGEST_FILE_VM_PARAM_NAME;
-import static org.apache.solr.common.cloud.VMParamsSingleSetCredentialsDigestZkCredentialsProvider.readCredentialsFile;
-
-import java.security.NoSuchAlgorithmException;
-import java.util.ArrayList;
import java.util.List;
-import java.util.Properties;
-import org.apache.solr.common.StringUtils;
-import org.apache.zookeeper.ZooDefs;
+import org.apache.solr.common.cloud.acl.*;
Review Comment:
Please list out all of the imports, we try to follow that as much as we can.
##########
solr/core/src/test/org/apache/solr/cloud/VMParamsZkACLAndCredentialsProvidersTest.java:
##########
@@ -17,4 +17,4 @@
package org.apache.solr.cloud;
public class VMParamsZkACLAndCredentialsProvidersTest
Review Comment:
Should this be renamed `VMParamsZKCredentialsInjectorTest`?
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/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.acl;
+
+import static org.apache.solr.common.cloud.acl.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.acl.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.acl.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();
+ }
+
+ public static Properties readCredentialsFile(String pathToFile) throws SolrException {
+ Properties props = new Properties();
+ try (Reader reader =
+ new InputStreamReader(new FileInputStream(pathToFile), StandardCharsets.UTF_8)) {
+ props.load(reader);
+ } catch (IOException ioExc) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, ioExc);
+ }
+ return props;
+ }
+
+ @Override
+ public List<ZkCredential> getZkCredentials() {
+ final String digestAllUsername = credentialsProps.getProperty(zkDigestAllUsernameVMParamName);
+ final String digestAllPassword = credentialsProps.getProperty(zkDigestAllPasswordVMParamName);
+ final String digestReadonlyUsername =
+ credentialsProps.getProperty(zkDigestReadonlyUsernameVMParamName);
+ final String digestReadonlyPassword =
+ credentialsProps.getProperty(zkDigestReadonlyPasswordVMParamName);
+
+ List<ZkCredential> zkCredentials =
+ new ArrayList<>(2) {
+ {
+ ZkCredential allUser =
+ new ZkCredential(digestAllUsername, digestAllPassword, Perms.ALL);
+ ZkCredential readUser =
+ new ZkCredential(digestReadonlyUsername, digestReadonlyPassword, Perms.READ);
+ add(allUser);
+ add(readUser);
+ }
+ };
+ log.info(
Review Comment:
You aren't caching the credentials, so this will get logged very very often.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org