madrob commented on code in PR #857:
URL: https://github.com/apache/solr/pull/857#discussion_r906356207
##########
solr/solr-ref-guide/modules/deployment-guide/pages/zookeeper-access-control.adoc:
##########
@@ -56,9 +56,37 @@ ACLs describe who is allowed to read, update, delete,
create, etc.
Each piece of information (znode/content) in ZooKeeper has its own set of
ACLs, and inheritance or sharing is not possible.
The default behavior in Solr is to add one ACL on all the content it creates -
one ACL that gives anyone the permission to do anything (in ZooKeeper terms
this is called "the open-unsafe ACL").
+
+
+== Solr to Zookeeper ACLs Workflow
+
+* Solr to Zookeeper credentials and ACLs are controlled trough 3 interfaces:
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsInjector.html[`ZkCredentialsInjector`],
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsProvider.html[`ZkCredentialsProvider`]
and
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkACLProvider.html[`ZkACLProvider`].
Review Comment:
```suggestion
* Solr to Zookeeper credentials and ACLs are controlled through 3
interfaces:
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsInjector.html[`ZkCredentialsInjector`],
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsProvider.html[`ZkCredentialsProvider`]
and
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkACLProvider.html[`ZkACLProvider`].
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/zookeeper-access-control.adoc:
##########
@@ -56,9 +56,37 @@ ACLs describe who is allowed to read, update, delete,
create, etc.
Each piece of information (znode/content) in ZooKeeper has its own set of
ACLs, and inheritance or sharing is not possible.
The default behavior in Solr is to add one ACL on all the content it creates -
one ACL that gives anyone the permission to do anything (in ZooKeeper terms
this is called "the open-unsafe ACL").
+
+
+== Solr to Zookeeper ACLs Workflow
+
+* Solr to Zookeeper credentials and ACLs are controlled trough 3 interfaces:
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsInjector.html[`ZkCredentialsInjector`],
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsProvider.html[`ZkCredentialsProvider`]
and
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkACLProvider.html[`ZkACLProvider`].
+
+* The workflow is as follow: Credentials source → `ZkCredentialsInjector` →
`ZkCredentialsProvider/ZkACLProvider` → Zookeeper.
+
+`ZkCredentialsInjector` gets the credentials from an external source which in
turn get injected into `ZkCredentialsProvider`
Review Comment:
We could say "source" because I guess every source is external given how we
define it.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/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.acl;
+
+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.StringUtils;
+import org.apache.solr.common.cloud.SecurityAwareZkACLProvider;
+import org.apache.solr.common.cloud.acl.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());
+
+ public DigestZkACLProvider() {
+ this(new DefaultZkCredentialsInjector());
+ }
+
+ public DigestZkACLProvider(ZkCredentialsInjector zkCredentialsInjector) {
+ this.zkCredentialsInjector = 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) {
+ try {
+ 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;
+ result.add(new ACL(perms, id));
+ }
+ if (result.isEmpty()) {
+ result = ZooDefs.Ids.OPEN_ACL_UNSAFE;
+ }
+ return result;
+ } catch (NoSuchAlgorithmException e) {
+ throw new RuntimeException("JVM mis-configured: missing SHA-1
algorithm", e);
+ }
+ }
+
+ protected Id createACLId(String username, String password) throws
NoSuchAlgorithmException {
+ return new Id("digest", generateDigest(username + ":" + password));
Review Comment:
probably makes sense to try/catch here and rethrow as InternalError.
Zookeeper already tries to recheck this for us, so if we're erroring here then
it means somebody is hotswapping security providers and all bets are off.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/zookeeper-access-control.adoc:
##########
@@ -56,9 +56,37 @@ ACLs describe who is allowed to read, update, delete,
create, etc.
Each piece of information (znode/content) in ZooKeeper has its own set of
ACLs, and inheritance or sharing is not possible.
The default behavior in Solr is to add one ACL on all the content it creates -
one ACL that gives anyone the permission to do anything (in ZooKeeper terms
this is called "the open-unsafe ACL").
+
+
+== Solr to Zookeeper ACLs Workflow
+
+* Solr to Zookeeper credentials and ACLs are controlled trough 3 interfaces:
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsInjector.html[`ZkCredentialsInjector`],
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsProvider.html[`ZkCredentialsProvider`]
and
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkACLProvider.html[`ZkACLProvider`].
+
+* The workflow is as follow: Credentials source → `ZkCredentialsInjector` →
`ZkCredentialsProvider/ZkACLProvider` → Zookeeper.
+
+`ZkCredentialsInjector` gets the credentials from an external source which in
turn get injected into `ZkCredentialsProvider`
+and `ZkACLProvider`. The "external source" here can be System Properties, a
file, a Secret Manager, or any other local or remote source.
+
+* Those credentials are then passed to Solr trough System Properties using the
following properties names:
Review Comment:
This seems strange, even if I use a file for example, credentials still end
up in system properties?
##########
solr/solr-ref-guide/modules/deployment-guide/pages/zookeeper-access-control.adoc:
##########
@@ -69,64 +97,361 @@ Solr nodes, clients, and tools (e.g., ZkCLI) always use a
java class called {sol
The implementation of the solution described here is all about changing
`SolrZkClient`.
If you use `SolrZkClient` in your application, the descriptions below will be
true for your application too.
-=== Controlling Credentials
-You control which credentials provider will be used by configuring the
`zkCredentialsProvider` property in the `<solrcloud>` section of
xref:configuration-guide:configuring-solr-xml.adoc[`solr.xml`] to the name of a
class (on the classpath) implementing the
{solr-javadocs}/solrj/org/apache/solr/common/cloud/ZkCredentialsProvider.html[`ZkCredentialsProvider`]
interface.
+* Controlling credentials and ACLs is done in 3 steps: Set a
`ZkCredentialsInjector` that reads the credentials from
+some source and then inject them into a `ZkCredentialsProvider` that Solr uses
to connect to Zookeeper. ZkACLProvider
+uses the same credentials to set the ACLs.
+
+
+We will describe these 3 steps in details before giving some ready to use
examples.
+
+
+. Set the `ZkCredentialsInjector`.
+. Set the `ZkCredentialsProvider`.
+. Set the `ZkACLProvider`.
+
+
+=== Set a Credentials Injector
+
+* A credentials injector gets the credentials from an external source and
injects them into Solr.
+
+
+** You control which credentials will be injected by configuring
`zkCredentialsInjector` property in the `<solrcloud>` section of
xref:configuration-guide:configuring-solr-xml.adoc[`solr.xml`] to the name of a
class (on the classpath) implementing the
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsInjector.html[`ZkCredentialsInjector`]
interface. +
+`server/solr/solr.xml` file in the Solr distribution defines
the`zkCredentialsInjector`such that it will take on the value
Review Comment:
```suggestion
`server/solr/solr.xml` file in the Solr distribution defines the
`zkCredentialsInjector` such that it will take on the value
```
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/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.acl;
+
+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.StringUtils;
+import org.apache.solr.common.cloud.SecurityAwareZkACLProvider;
+import org.apache.solr.common.cloud.acl.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());
+
+ public DigestZkACLProvider() {
Review Comment:
This constructor appears unused but I think we need it for reflective
instantiation? Please add a comment noting this.
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -59,6 +59,7 @@
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Supplier;
+import org.apache.commons.lang3.StringUtils;
Review Comment:
```suggestion
import org.apache.solr.common.StringUtils;
```
##########
solr/solrj/src/java/org/apache/solr/common/cloud/VMParamsAllAndReadonlyDigestZkACLProvider.java:
##########
@@ -16,134 +16,82 @@
*/
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.zookeeper.data.ACL;
-import org.apache.zookeeper.data.Id;
-import org.apache.zookeeper.server.auth.DigestAuthenticationProvider;
-public class VMParamsAllAndReadonlyDigestZkACLProvider extends
SecurityAwareZkACLProvider {
+/**
+ * Deprecated in favor of a combination of {@link DigestZkACLProvider} and
{@link
+ * VMParamsZkCredentialsInjector}.
+ *
+ * <pre>
+ * Current implementation delegates to {@link DigestZkACLProvider} with an
injected {@link VMParamsZkCredentialsInjector}
+ * </pre>
+ */
+@Deprecated
Review Comment:
We typically include a `since` annotation on our deprecations. I guess this
would be `since=10.0`?
##########
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);
Review Comment:
Is this safe? Will it log a password?
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkACLProvider.java:
##########
@@ -17,9 +17,14 @@
package org.apache.solr.common.cloud;
import java.util.List;
+import org.apache.solr.common.cloud.acl.ZkCredentialsInjector;
import org.apache.zookeeper.data.ACL;
public interface ZkACLProvider {
List<ACL> getACLsToAdd(String zNodePath);
+
+ default void setZkCredentialsInjector(ZkCredentialsInjector
zkCredentialsInjector) {
Review Comment:
javadoc please
##########
solr/solr-ref-guide/modules/deployment-guide/pages/zookeeper-access-control.adoc:
##########
@@ -56,9 +56,37 @@ ACLs describe who is allowed to read, update, delete,
create, etc.
Each piece of information (znode/content) in ZooKeeper has its own set of
ACLs, and inheritance or sharing is not possible.
The default behavior in Solr is to add one ACL on all the content it creates -
one ACL that gives anyone the permission to do anything (in ZooKeeper terms
this is called "the open-unsafe ACL").
+
+
+== Solr to Zookeeper ACLs Workflow
+
+* Solr to Zookeeper credentials and ACLs are controlled trough 3 interfaces:
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsInjector.html[`ZkCredentialsInjector`],
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkCredentialsProvider.html[`ZkCredentialsProvider`]
and
{solr-javadocs}solrj/org/apache/solr/common/cloud/ZkACLProvider.html[`ZkACLProvider`].
+
+* The workflow is as follow: Credentials source → `ZkCredentialsInjector` →
`ZkCredentialsProvider/ZkACLProvider` → Zookeeper.
Review Comment:
Should this be "data flow" instead of workflow? I'm not sure what the right
word would be but workflow doesn't feel right.
--
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
