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 2021/03/25 17:42:21 UTC
[GitHub] [solr] bruno-roustant opened a new pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
bruno-roustant opened a new pull request #47:
URL: https://github.com/apache/solr/pull/47
There are many touched files. Most of them are modified only for the renaming of 'shardHandlerFactory:shardsWhitelist' to a top level property 'allowUrls'. I chose 'allowUrls' name to stay consistent with the existing 'allowPaths', otherwise I would have named it 'urlAllowList'.
The doc is modified accordingly.
At the core of this PR:
- New AllowListUrlChecker (extraction of HttpShardHandlerFactory.WhitelistChecker).
- HttpShardHandlerFactory and HttpShardHandler adapted.
- IndexFetcher now uses AllowListUrlChecker.
- New AllowListUrlCheckerTest.
- New TestReplicationHandler.testUrlAllowList().
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r615963406
##########
File path: solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
##########
@@ -352,12 +358,19 @@ private static NodeConfig fillSolrSection(NodeConfig.NodeConfigBuilder builder,
return builder.build();
}
- private static Set<Path> stringToPaths(String commaSeparatedString) {
+ private static List<String> separateStrings(String commaSeparatedString) {
+ if (Strings.isNullOrEmpty(commaSeparatedString)) {
+ return Collections.emptyList();
+ }
+ return Arrays.asList(COMMA_SEPARATED_PATTERN.split(commaSeparatedString));
+ }
+
+ private static Set<Path> separatePaths(String commaSeparatedString) {
if (Strings.isNullOrEmpty(commaSeparatedString)) {
return Collections.emptySet();
}
// Parse list of paths. The special value '*' is mapped to _ALL_ to mean all paths
- return Arrays.stream(commaSeparatedString.split(",\\s?"))
+ return Arrays.stream(COMMA_SEPARATED_PATTERN.split(commaSeparatedString))
.map(p -> Paths.get("*".equals(p) ? "_ALL_" : p)).collect(Collectors.toSet());
Review comment:
Ok. allowPaths is not part of the initial goal, but we can improve a bit here.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r616637322
##########
File path: solr/solr-ref-guide/src/the-terms-component.adoc
##########
@@ -344,16 +344,4 @@ Result:
== Distributed Search Support
Review comment:
Ok
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r618272385
##########
File path: solr/core/src/test/org/apache/solr/cloud/UnloadDistributedZkTest.java
##########
@@ -65,7 +67,11 @@ protected String getSolrXml() {
@Test
public void test() throws Exception {
- jettys.forEach(j -> j.getCoreContainer().getAllowPaths().add(Path.of("_ALL_"))); // Allow non-standard core instance path
+ jettys.forEach(j -> {
+ Set<Path> allowPath = j.getCoreContainer().getAllowPaths();
+ allowPath.clear();
+ allowPath.addAll(SolrPaths.ALL_PATHS); // Allow non-standard core instance path
Review comment:
I was also surprised to see that. There are many tests (and only tests) that use this mutablity to add test dirs in the allowed list. I don't like that very much, but I don't think this should be changed in this PR.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant closed pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant closed pull request #47:
URL: https://github.com/apache/solr/pull/47
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r618245496
##########
File path: solr/solr-ref-guide/src/major-changes-in-solr-8.adoc
##########
@@ -283,9 +283,9 @@ The login screen's purpose is cosmetic only - Admin UI-triggered Solr requests w
*Distributed Requests*
-* The `shards` parameter, used to manually select the shards and replicas that receive distributed requests, now checks nodes against a whitelist of acceptable values for security reasons.
+* The `shards` parameter, used to manually select the shards and replicas that receive distributed requests, now checks nodes against an allow-list of acceptable values for security reasons.
+
-In SolrCloud mode this whitelist is automatically configured to contain all live nodes. In standalone mode the whitelist is empty by default. Upgrading users who use the `shards` parameter in standalone mode can correct this value by setting the `shardsWhitelist` property in any `shardHandler` configurations in their `solrconfig.xml` file.
+In SolrCloud mode this allow-list is automatically configured to contain all live nodes. In standalone mode the allow-list is empty by default. Upgrading users who use the `shards` parameter in standalone mode can correct this value by setting the `allowUrls` property in their `solrconfig.xml` file.
Review comment:
I'll also add a sentence in major-changes-in-solr-9.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r616936428
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
The initial code you wrote `new HashSet<>((int) (urls.size() / 0.7f) + 1)` looked to me and Mike too complicated to warrant it; and I suspect it was incorrect, like you were over-thinking it based on your internal knowledge of hash sets/tables. This constructor is merely the "initialCapacity". If you want to use this constructor and you know how many items you want to add (you do), just pass `urls.size()`; no?
You might also use `Set.of(hostPorts.toArray())` which is a new JDK 10 feature and is leaner. I learned about some of these the JDK APIs from this video https://www.youtube.com/watch?v=lwp2RZ__0ko and another presentation from Oracle. Cool stuff!
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602355865
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
If specified, this lists limits what nodes can be requested in the `shards` request parameter.
+
-In SolrCloud mode this whitelist is automatically configured to include all live nodes in the cluster.
+In SolrCloud mode this allow-list is automatically configured to include all live nodes in the cluster.
+
-In standalone mode the whitelist defaults to empty (sharding not allowed).
+In standalone mode the allow-list defaults to empty (sharding not allowed).
+
-If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.shardsWhitelist=true`. The value of this parameter is a comma separated list of the nodes that will be whitelisted, i.e.,
+If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.allowUrls=true`. The value of this parameter is a comma separated list of the nodes that will be allowed, i.e.,
`10.0.0.1:8983/solr,10.0.0.1:8984/solr`.
+
-NOTE: In SolrCloud mode, if at least one node is included in the whitelist, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the whitelist.
+NOTE: In SolrCloud mode, if at least one node is included in the allow-list, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the allow-list.
Review comment:
Yes, I don't understand neither. Is this a security matter? Indeed I prefer to handle that separately but that would be nice to open the discussion. Technically it is really easy to have the union of the allow-list and the live nodes.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r615855246
##########
File path: solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
##########
@@ -74,6 +75,8 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final Pattern COMMA_SEPARATED_PATTERN = Pattern.compile(",\\s*");
Review comment:
Yes
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602716554
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
If specified, this lists limits what nodes can be requested in the `shards` request parameter.
+
-In SolrCloud mode this whitelist is automatically configured to include all live nodes in the cluster.
+In SolrCloud mode this allow-list is automatically configured to include all live nodes in the cluster.
+
-In standalone mode the whitelist defaults to empty (sharding not allowed).
+In standalone mode the allow-list defaults to empty (sharding not allowed).
+
-If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.shardsWhitelist=true`. The value of this parameter is a comma separated list of the nodes that will be whitelisted, i.e.,
+If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.allowUrls=true`. The value of this parameter is a comma separated list of the nodes that will be allowed, i.e.,
`10.0.0.1:8983/solr,10.0.0.1:8984/solr`.
+
-NOTE: In SolrCloud mode, if at least one node is included in the whitelist, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the whitelist.
+NOTE: In SolrCloud mode, if at least one node is included in the allow-list, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the allow-list.
Review comment:
I know, the local cluster’s live_nodes set should probably always be added to the manually configured allow-list. Should not be too hard?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r604216469
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
If specified, this lists limits what nodes can be requested in the `shards` request parameter.
+
-In SolrCloud mode this whitelist is automatically configured to include all live nodes in the cluster.
+In SolrCloud mode this allow-list is automatically configured to include all live nodes in the cluster.
+
-In standalone mode the whitelist defaults to empty (sharding not allowed).
+In standalone mode the allow-list defaults to empty (sharding not allowed).
+
-If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.shardsWhitelist=true`. The value of this parameter is a comma separated list of the nodes that will be whitelisted, i.e.,
+If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.allowUrls=true`. The value of this parameter is a comma separated list of the nodes that will be allowed, i.e.,
`10.0.0.1:8983/solr,10.0.0.1:8984/solr`.
+
-NOTE: In SolrCloud mode, if at least one node is included in the whitelist, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the whitelist.
+NOTE: In SolrCloud mode, if at least one node is included in the allow-list, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the allow-list.
Review comment:
Ok, I have modified AllowListUrlChecker to always add the cluster live nodes to the configured allow-list. It actually reduces the code.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r617490297
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
Unfortunately the HashSet(int) constructor capacity defines the capacity of the internal key array (internally a power of 2), which can only be loaded up to load factor (75%) before it enlarges and rehashes all keys. This means if we use new HashSet(urls.size()) we have high probability to rehash when adding the elements. I find this constructor misleading.
The formula I used is to ensure we don't reach the load factor. This is what is done in com.google.common.collect.Maps.capacity(int expectedSize), and what is used when we call Sets.newHashSetWithExpectedSize(int).
That said, here we talk about a small map so it is not even worth this discussion. That was my remark, I wished this newHashSetWithExpectedSize() was part of the jdk and we wouldn't talk about that.
Thanks for the link.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r604264829
##########
File path: solr/core/src/java/org/apache/solr/core/NodeConfig.java
##########
@@ -44,6 +44,8 @@
private final Set<Path> allowPaths;
+ private final String allowUrls;
Review comment:
I made it a List and not a Set. It is a list in NodeConfig and this list elements are parsed by AllowListUrlChecker to build a Set with simplified <host:port> strings.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r618244929
##########
File path: solr/solr-ref-guide/src/major-changes-in-solr-8.adoc
##########
@@ -283,9 +283,9 @@ The login screen's purpose is cosmetic only - Admin UI-triggered Solr requests w
*Distributed Requests*
-* The `shards` parameter, used to manually select the shards and replicas that receive distributed requests, now checks nodes against a whitelist of acceptable values for security reasons.
+* The `shards` parameter, used to manually select the shards and replicas that receive distributed requests, now checks nodes against an allow-list of acceptable values for security reasons.
+
-In SolrCloud mode this whitelist is automatically configured to contain all live nodes. In standalone mode the whitelist is empty by default. Upgrading users who use the `shards` parameter in standalone mode can correct this value by setting the `shardsWhitelist` property in any `shardHandler` configurations in their `solrconfig.xml` file.
+In SolrCloud mode this allow-list is automatically configured to contain all live nodes. In standalone mode the allow-list is empty by default. Upgrading users who use the `shards` parameter in standalone mode can correct this value by setting the `allowUrls` property in their `solrconfig.xml` file.
Review comment:
Nice catch! Abusive find & replace.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602670860
##########
File path: solr/server/solr/solr.xml
##########
@@ -53,7 +54,6 @@
class="HttpShardHandlerFactory">
<int name="socketTimeout">${socketTimeout:600000}</int>
<int name="connTimeout">${connTimeout:60000}</int>
- <str name="shardsWhitelist">${solr.shardsWhitelist:}</str>
Review comment:
Yes; thanks.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r604247449
##########
File path: solr/solr-ref-guide/src/format-of-solr-xml.adoc
##########
@@ -103,6 +103,9 @@ Specifies the path to a common library directory that will be shared across all
`allowPaths`::
Solr will normally only access folders relative to `$SOLR_HOME`, `$SOLR_DATA_HOME` or `coreRootDir`. If you need to e.g., create a core outside of these paths, you can explicitly allow the path with `allowPaths`. It is a comma separated string of file system paths to allow. The special value of `*` will allow any path on the system.
+`allowUrls``:
+When running Solr in non-cloud mode and if planning to do distributed search (using the "shards" parameter), the list of hosts needs to be allowed or Solr will forbid the request. The allow-list can also be configured in `solr.in.sh`.
Review comment:
I'll move the doc here and add links to here.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602352717
##########
File path: solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java
##########
@@ -1231,6 +1231,9 @@ private void setupPolling(String intervalStr) {
@Override
@SuppressWarnings({"resource"})
public void inform(SolrCore core) {
+
+ //TODO core.getCoreContainer().getAllowListUrlChecker()
Review comment:
Yes. This TODO was a remaining of some investigation. I'l just remove it.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r616517842
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
Ok. I understand here the Set is small so we don't care that it enlarges and rehashes during the construction. I always try to initiallize with the right capacity when I have it ahead. It's sad that there is no such constructor in the jdk otherwise we would use it without questioning.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r615854245
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -38,6 +24,14 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
+import java.util.*;
Review comment:
Ah yes
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r604244484
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
Review comment:
+1
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602345194
##########
File path: solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java
##########
@@ -220,7 +220,7 @@ protected String getReplicateLeaderUrl(ZkNodeProps leaderprops) {
final private void replicate(String nodeName, SolrCore core, ZkNodeProps leaderprops)
throws SolrServerException, IOException {
- final String leaderUrl = getReplicateLeaderUrl(leaderprops);
+ final String leaderUrl = getReplicateLeaderUrl(leaderprops);//TODO?
Review comment:
Exactly. Thanks for checking!
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r615976926
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
+ for (String urlString : urls) {
+ hostPorts.add(parseHostPort(urlString));
+ }
+ return hostPorts;
+ }
+
+ private static String parseHostPort(String url) throws MalformedURLException {
+ url = url.trim();
+ URL u;
+ if (!url.startsWith("http://") && !url.startsWith("https://")) {
Review comment:
+1
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r615978981
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
I don't think that's necessary - just use the no-args HashSet constructor for now, unless we are convinced that this is a problem?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602408258
##########
File path: solr/core/src/java/org/apache/solr/core/NodeConfig.java
##########
@@ -44,6 +44,8 @@
private final Set<Path> allowPaths;
+ private final String allowUrls;
Review comment:
Nevermind what I said. allowPaths is actually a single string in the xml file. It is parsed in SolrXmlConfig. I'll do the same for allowUrls.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r617730095
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
I didn't know that! Wow; that "initialCapacity" arg is trappy/misleading; I will not use it henceforth. Using more of the new Set.of / Map.of will help avoid the discussion as well.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r616025799
##########
File path: solr/solr-ref-guide/src/the-terms-component.adoc
##########
@@ -344,16 +344,4 @@ Result:
== Distributed Search Support
Review comment:
I think you can just remove this heading and the sentence you added. All of Solr's Search Components support distributed search.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602362895
##########
File path: solr/core/src/java/org/apache/solr/core/NodeConfig.java
##########
@@ -44,6 +44,8 @@
private final Set<Path> allowPaths;
+ private final String allowUrls;
Review comment:
I'll see what it means to change the allowUrls to behave like allowPaths.
That means that users will be a little more verbose when listing the urls in xml, but it would be more consistent.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r617717842
##########
File path: solr/solr-ref-guide/src/major-changes-in-solr-8.adoc
##########
@@ -283,9 +283,9 @@ The login screen's purpose is cosmetic only - Admin UI-triggered Solr requests w
*Distributed Requests*
-* The `shards` parameter, used to manually select the shards and replicas that receive distributed requests, now checks nodes against a whitelist of acceptable values for security reasons.
+* The `shards` parameter, used to manually select the shards and replicas that receive distributed requests, now checks nodes against an allow-list of acceptable values for security reasons.
+
-In SolrCloud mode this whitelist is automatically configured to contain all live nodes. In standalone mode the whitelist is empty by default. Upgrading users who use the `shards` parameter in standalone mode can correct this value by setting the `shardsWhitelist` property in any `shardHandler` configurations in their `solrconfig.xml` file.
+In SolrCloud mode this allow-list is automatically configured to contain all live nodes. In standalone mode the allow-list is empty by default. Upgrading users who use the `shards` parameter in standalone mode can correct this value by setting the `allowUrls` property in their `solrconfig.xml` file.
Review comment:
You are going back in time and changing upgrade notes for Solr 8 where there was a `shardsWhitelist` and saying it is `allowUrls` when this isn't the case for 8.x. I think the terminology change is okay but don't change the particular setting name or that it goes on shardHandler which is true for 8.x.
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,215 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Regex pattern to match any protocol, e.g. http:// https:// s3://.
+ * After a match, regex group 1 contains the protocol and group 2 the rest.
+ */
+ private static final Pattern PROTOCOL_PATTERN = Pattern.compile("(\\w+)(://.*)");
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
Review comment:
Woah; 4-space indent. Please fix this and change your IDE settings. Use "Google Java Format" IntelliJ plugin, which may help and is practically required on the Lucene side any way.
##########
File path: solr/core/src/test/org/apache/solr/cloud/UnloadDistributedZkTest.java
##########
@@ -65,7 +67,11 @@ protected String getSolrXml() {
@Test
public void test() throws Exception {
- jettys.forEach(j -> j.getCoreContainer().getAllowPaths().add(Path.of("_ALL_"))); // Allow non-standard core instance path
+ jettys.forEach(j -> {
+ Set<Path> allowPath = j.getCoreContainer().getAllowPaths();
+ allowPath.clear();
+ allowPath.addAll(SolrPaths.ALL_PATHS); // Allow non-standard core instance path
Review comment:
Do you think this Set should be mutable? Hmm. Not a blocker to me but worth contemplating.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #47:
URL: https://github.com/apache/solr/pull/47#issuecomment-823236189
I had to merge and rebase to be able to recompile with new Lucene snapshot changes, so I force pushed. But the list of commits is the same. I hope it does not disrupt your review.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r614157585
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -38,6 +24,14 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
+import java.util.*;
Review comment:
Please don't use wildcard imports.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
##########
@@ -74,6 +75,8 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final Pattern COMMA_SEPARATED_PATTERN = Pattern.compile(",\\s*");
Review comment:
Should this accept whitespace on either side of the comma? `\\s*,\\s*`
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
+ for (String urlString : urls) {
+ hostPorts.add(parseHostPort(urlString));
+ }
+ return hostPorts;
+ }
+
+ private static String parseHostPort(String url) throws MalformedURLException {
+ url = url.trim();
+ URL u;
+ if (!url.startsWith("http://") && !url.startsWith("https://")) {
Review comment:
Are there circumstances where we would be looking at a different protocol? Since we're using this for more than just shards now, maybe this will be part of BackupRestore and will need to accept `s3://` URLs?
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
This feels like a weird optimization.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
##########
@@ -352,12 +358,19 @@ private static NodeConfig fillSolrSection(NodeConfig.NodeConfigBuilder builder,
return builder.build();
}
- private static Set<Path> stringToPaths(String commaSeparatedString) {
+ private static List<String> separateStrings(String commaSeparatedString) {
+ if (Strings.isNullOrEmpty(commaSeparatedString)) {
+ return Collections.emptyList();
+ }
+ return Arrays.asList(COMMA_SEPARATED_PATTERN.split(commaSeparatedString));
+ }
+
+ private static Set<Path> separatePaths(String commaSeparatedString) {
if (Strings.isNullOrEmpty(commaSeparatedString)) {
return Collections.emptySet();
}
// Parse list of paths. The special value '*' is mapped to _ALL_ to mean all paths
- return Arrays.stream(commaSeparatedString.split(",\\s?"))
+ return Arrays.stream(COMMA_SEPARATED_PATTERN.split(commaSeparatedString))
.map(p -> Paths.get("*".equals(p) ? "_ALL_" : p)).collect(Collectors.toSet());
Review comment:
If `*` (or `_ALL_`) is present, can we drop the rest of the paths?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602671024
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
If specified, this lists limits what nodes can be requested in the `shards` request parameter.
+
-In SolrCloud mode this whitelist is automatically configured to include all live nodes in the cluster.
+In SolrCloud mode this allow-list is automatically configured to include all live nodes in the cluster.
+
-In standalone mode the whitelist defaults to empty (sharding not allowed).
+In standalone mode the allow-list defaults to empty (sharding not allowed).
+
-If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.shardsWhitelist=true`. The value of this parameter is a comma separated list of the nodes that will be whitelisted, i.e.,
+If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.allowUrls=true`. The value of this parameter is a comma separated list of the nodes that will be allowed, i.e.,
`10.0.0.1:8983/solr,10.0.0.1:8984/solr`.
+
-NOTE: In SolrCloud mode, if at least one node is included in the whitelist, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the whitelist.
+NOTE: In SolrCloud mode, if at least one node is included in the allow-list, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the allow-list.
Review comment:
I don't know why it is what it is. But once we want to start using this checker to check connecting to other hosts outside of the SolrCloud cluster, we'll then want to change this at that time. Ideally this changes at 9.0 (major release boundary) so I suggest raising the question on the dev list now.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r604260181
##########
File path: solr/solr-ref-guide/src/the-terms-component.adoc
##########
@@ -349,11 +349,11 @@ The TermsComponent also supports distributed indexes. For the `/terms` request h
`shards`::
Specifies the shards in your distributed indexing configuration. For more information about distributed indexing, see <<distributed-search-with-index-sharding.adoc#,Distributed Search with Index Sharding>>.
+
-The `shards` parameter is subject to a host whitelist that has to be configured in the component's parameters using the configuration key `shardsWhitelist` and the list of hosts as values.
+The `shards` parameter is subject to a host allow-list that has to be configured in the component's parameters using the configuration key `allowUrls` and the list of hosts as values.
Review comment:
I'll remove both `shards` and `shards.qt`.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602348555
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -385,6 +388,9 @@ public CoreContainer(NodeConfig config, CoresLocator locator, boolean asyncSolrC
}
}
+ this.allowListUrlChecker = AllowListUrlChecker.create(config);
+ log.info("URL allow-list initialized: {}", this.allowListUrlChecker);
Review comment:
I agree. I copied it from the old WhitelistChecker, but that's probably too much.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r618268515
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,215 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Regex pattern to match any protocol, e.g. http:// https:// s3://.
+ * After a match, regex group 1 contains the protocol and group 2 the rest.
+ */
+ private static final Pattern PROTOCOL_PATTERN = Pattern.compile("(\\w+)(://.*)");
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
Review comment:
Strange my IDE lost some settings. I'll install this plugin, thanks!
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602355865
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
If specified, this lists limits what nodes can be requested in the `shards` request parameter.
+
-In SolrCloud mode this whitelist is automatically configured to include all live nodes in the cluster.
+In SolrCloud mode this allow-list is automatically configured to include all live nodes in the cluster.
+
-In standalone mode the whitelist defaults to empty (sharding not allowed).
+In standalone mode the allow-list defaults to empty (sharding not allowed).
+
-If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.shardsWhitelist=true`. The value of this parameter is a comma separated list of the nodes that will be whitelisted, i.e.,
+If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.allowUrls=true`. The value of this parameter is a comma separated list of the nodes that will be allowed, i.e.,
`10.0.0.1:8983/solr,10.0.0.1:8984/solr`.
+
-NOTE: In SolrCloud mode, if at least one node is included in the whitelist, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the whitelist.
+NOTE: In SolrCloud mode, if at least one node is included in the allow-list, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the allow-list.
Review comment:
Yes, I don't understand either. Is this a security matter? Indeed I prefer to handle that separately but that would be nice to open the discussion. Technically it is really easy to have the union of the allow-list and the live nodes.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15340: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r615963839
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,201 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(Collections.emptyList()) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList List of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * An empty list means there is no explicit allow-list of URLs, in this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(List<String> urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '{}' instead.", DISABLE_URL_ALLOW_LIST);
+ }
+ try {
+ return new AllowListUrlChecker(config.getAllowUrls());
+ } catch (MalformedURLException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid URL syntax in '" + URL_ALLOW_LIST + "' configuration: " + config.getAllowUrls(), e);
+ }
+ }
+
+ /**
+ * @see #checkAllowList(List, ClusterState)
+ */
+ public void checkAllowList(List<String> urls) throws MalformedURLException {
+ checkAllowList(urls, null);
+ }
+
+ /**
+ * Checks that the given URLs are present in the configured allow-list or in the provided {@link ClusterState}
+ * (in case of cloud mode).
+ *
+ * @param urls The list of urls to check.
+ * @param clusterState The up to date {@link ClusterState}, can be null in case of non-cloud mode.
+ * @throws MalformedURLException If an URL is invalid.
+ * @throws SolrException If an URL is not present in the allow-list or in the provided {@link ClusterState}.
+ */
+ public void checkAllowList(List<String> urls, @Nullable ClusterState clusterState) throws MalformedURLException {
+ Set<String> clusterHostAllowList = clusterState == null ? Collections.emptySet() : clusterState.getHostAllowList();
+ for (String url : urls) {
+ String hostPort = parseHostPort(url);
+ if (!clusterHostAllowList.contains(hostPort) && !hostAllowList.contains(hostPort)) {
+ throw new SolrException(SolrException.ErrorCode.FORBIDDEN, "URL " + url +
+ " is neither a live node of the cluster nor in the configured '" +
+ URL_ALLOW_LIST + "' " + hostAllowList);
+ }
+ }
+ }
+
+ /**
+ * Whether this checker has been created with a non-empty allow-list of URLs.
+ */
+ public boolean hasExplicitAllowList() {
+ return !hostAllowList.isEmpty();
+ }
+
+ /**
+ * Whether the URL checking is enabled. Only {@link #ALLOW_ALL} returns false.
+ */
+ public boolean isEnabled() {
+ return true;
+ }
+
+ /**
+ * Only for testing.
+ */
+ @VisibleForTesting
+ public Set<String> getHostAllowList() {
+ return hostAllowList == null ? null : Collections.unmodifiableSet(hostAllowList);
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allowList=" + hostAllowList + "]";
+ }
+
+ @VisibleForTesting
+ static Set<String> parseHostPorts(List<String> urls) throws MalformedURLException {
+ if (urls == null || urls.isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> hostPorts = new HashSet<>((int) (urls.size() / 0.7f) + 1);
Review comment:
I'll use Guava instead.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602231577
##########
File path: solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java
##########
@@ -220,7 +220,7 @@ protected String getReplicateLeaderUrl(ZkNodeProps leaderprops) {
final private void replicate(String nodeName, SolrCore core, ZkNodeProps leaderprops)
throws SolrServerException, IOException {
- final String leaderUrl = getReplicateLeaderUrl(leaderprops);
+ final String leaderUrl = getReplicateLeaderUrl(leaderprops);//TODO?
Review comment:
Did you say TODO because maybe this URL needs to be checked here? I did a "Analyze Data Flow To Here" in IntelliJ at the spot where we fetch the URL. This URL is always sourced from the SolrCloud cluster state, and not a parameter. So we're good. That said, it could be worth a comment in some places to say that we "trust" the leaderUrl from the cluster state (needn't be checked)
##########
File path: solr/solr-ref-guide/src/format-of-solr-xml.adoc
##########
@@ -103,6 +103,9 @@ Specifies the path to a common library directory that will be shared across all
`allowPaths`::
Solr will normally only access folders relative to `$SOLR_HOME`, `$SOLR_DATA_HOME` or `coreRootDir`. If you need to e.g., create a core outside of these paths, you can explicitly allow the path with `allowPaths`. It is a comma separated string of file system paths to allow. The special value of `*` will allow any path on the system.
+`allowUrls``:
+When running Solr in non-cloud mode and if planning to do distributed search (using the "shards" parameter), the list of hosts needs to be allowed or Solr will forbid the request. The allow-list can also be configured in `solr.in.sh`.
Review comment:
Instead of making reference so `solr.in.sh`, it's better to refer to the particular environment variable that is used. People can set that env variable even if they don't touch solr.in.sh.
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
If specified, this lists limits what nodes can be requested in the `shards` request parameter.
+
-In SolrCloud mode this whitelist is automatically configured to include all live nodes in the cluster.
+In SolrCloud mode this allow-list is automatically configured to include all live nodes in the cluster.
+
-In standalone mode the whitelist defaults to empty (sharding not allowed).
+In standalone mode the allow-list defaults to empty (sharding not allowed).
+
-If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.shardsWhitelist=true`. The value of this parameter is a comma separated list of the nodes that will be whitelisted, i.e.,
+If you need to disable this feature for backwards compatibility, you can set the system property `solr.disable.allowUrls=true`. The value of this parameter is a comma separated list of the nodes that will be allowed, i.e.,
`10.0.0.1:8983/solr,10.0.0.1:8984/solr`.
+
-NOTE: In SolrCloud mode, if at least one node is included in the whitelist, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the whitelist.
+NOTE: In SolrCloud mode, if at least one node is included in the allow-list, then the `live_nodes` will no longer be used as source for the list. This means that if you need to do a cross-cluster request using the `shards` parameter in SolrCloud mode (in addition to regular within-cluster requests), you'll need to add all nodes (local cluster + remote nodes) to the allow-list.
Review comment:
IMO this is a shame but I understand we don't want to change this approach in this PR.
##########
File path: solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
##########
@@ -413,139 +384,4 @@ public void initializeMetrics(SolrMetricsContext parentContext, String scope) {
solrMetricsContext.getMetricRegistry(),
SolrMetricManager.mkName("httpShardExecutor", expandedScope, "threadPool"));
}
-
Review comment:
It's nice to see all this stuff moved out of HttpShardHandlerFactory which already has tons of concerns to deal with.
##########
File path: solr/server/solr/solr.xml
##########
@@ -53,7 +54,6 @@
class="HttpShardHandlerFactory">
<int name="socketTimeout">${socketTimeout:600000}</int>
<int name="connTimeout">${connTimeout:60000}</int>
- <str name="shardsWhitelist">${solr.shardsWhitelist:}</str>
Review comment:
If a user has a legacy config with this defined, what do we do now? Log a warning would be nice.
##########
File path: solr/core/src/java/org/apache/solr/security/AllowListUrlChecker.java
##########
@@ -0,0 +1,208 @@
+/*
+ * 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.security;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.core.NodeConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.lang.invoke.MethodHandles;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Validates URLs based on an allow list or a {@link ClusterState} in SolrCloud.
+ */
+public class AllowListUrlChecker {
+
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ /**
+ * {@link org.apache.solr.core.SolrXmlConfig} property to configure the allowed URLs.
+ */
+ public static final String URL_ALLOW_LIST = "allowUrls";
+
+ /**
+ * System property to disable URL checking and {@link #ALLOW_ALL} instead.
+ */
+ public static final String DISABLE_URL_ALLOW_LIST = "solr.disable." + URL_ALLOW_LIST;
+
+ /**
+ * Clue given in URL-forbidden exceptions messages.
+ */
+ public static final String SET_SOLR_DISABLE_URL_ALLOW_LIST_CLUE = "Set -D" + DISABLE_URL_ALLOW_LIST + "=true to disable URL allow-list checks.";
+
+ /**
+ * Singleton checker which allows all URLs. {@link #isEnabled()} returns false.
+ */
+ public static final AllowListUrlChecker ALLOW_ALL;
+
+ static {
+ try {
+ ALLOW_ALL = new AllowListUrlChecker(null) {
+ @Override
+ public void checkAllowList(List<String> urls, ClusterState clusterState) {
+ // Allow.
+ }
+
+ @Override
+ public boolean isEnabled() {
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + " [allow all]";
+ }
+ };
+ } catch (MalformedURLException e) {
+ // Never thrown.
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ /**
+ * Allow list of hosts. Elements in the list will be host:port (no protocol or context).
+ */
+ private final Set<String> hostAllowList;
+
+ /**
+ * @param urlAllowList Comma-separated list of allowed URLs. URLs must be well-formed, missing protocol is tolerated.
+ * Empty or null is supported and means there is no explicit allow-list of URLs. In this case no
+ * URL is allowed unless a {@link ClusterState} is provided in
+ * {@link #checkAllowList(List, ClusterState)}.
+ * @throws MalformedURLException If an URL is invalid.
+ */
+ public AllowListUrlChecker(@Nullable String urlAllowList) throws MalformedURLException {
+ hostAllowList = parseHostPorts(urlAllowList);
+ }
+
+ /**
+ * Creates a URL checker based on the {@link NodeConfig} property to configure the allowed URLs.
+ */
+ public static AllowListUrlChecker create(NodeConfig config) {
+ if (Boolean.getBoolean(DISABLE_URL_ALLOW_LIST)) {
+ return AllowListUrlChecker.ALLOW_ALL;
+ } else if (System.getProperty("solr.disable.shardsWhitelist") != null) {
+ log.warn("Property 'solr.disable.shardsWhitelist' is deprecated, please use '" + DISABLE_URL_ALLOW_LIST + "' instead.");
Review comment:
nice
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -385,6 +388,9 @@ public CoreContainer(NodeConfig config, CoresLocator locator, boolean asyncSolrC
}
}
+ this.allowListUrlChecker = AllowListUrlChecker.create(config);
+ log.info("URL allow-list initialized: {}", this.allowListUrlChecker);
Review comment:
I question that this worth logging at INFO for. Solr has *tons* of plugins.
##########
File path: solr/solr-ref-guide/src/distributed-requests.adoc
##########
@@ -115,17 +115,19 @@ If specified, the thread pool will use a backing queue instead of a direct hando
`fairnessPolicy`::
Chooses the JVM specifics dealing with fair policy queuing, if enabled distributed searches will be handled in a First in First out fashion at a cost to throughput. If disabled throughput will be favored over latency. The default is `false`.
-`shardsWhitelist`::
+In addition, `HttpShardHandlerFactory` also depends on the following top-level property:
+
+`allowUrls`::
Review comment:
I think you should be making reference to `format-of-solr-xml.adoc` where this setting should be canonically documented. That said, I understand why this distributed-requests page has information on this.
##########
File path: solr/core/src/java/org/apache/solr/core/NodeConfig.java
##########
@@ -44,6 +44,8 @@
private final Set<Path> allowPaths;
+ private final String allowUrls;
Review comment:
I can't help but notice that just above here we have a `Set` of allowed paths, yet here you are adding a single String that is apparently more than one thing. This is a bit out of balance. If it's not much work, can this be harmonized?
##########
File path: solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java
##########
@@ -1231,6 +1231,9 @@ private void setupPolling(String intervalStr) {
@Override
@SuppressWarnings({"resource"})
public void inform(SolrCore core) {
+
+ //TODO core.getCoreContainer().getAllowListUrlChecker()
Review comment:
I see you managed to get ahold of the checker from IndexFetcher so maybe it's not needed here in ReplicationHandler?
##########
File path: solr/solr-ref-guide/src/format-of-solr-xml.adoc
##########
@@ -103,6 +103,9 @@ Specifies the path to a common library directory that will be shared across all
`allowPaths`::
Solr will normally only access folders relative to `$SOLR_HOME`, `$SOLR_DATA_HOME` or `coreRootDir`. If you need to e.g., create a core outside of these paths, you can explicitly allow the path with `allowPaths`. It is a comma separated string of file system paths to allow. The special value of `*` will allow any path on the system.
+`allowUrls``:
+When running Solr in non-cloud mode and if planning to do distributed search (using the "shards" parameter), the list of hosts needs to be allowed or Solr will forbid the request. The allow-list can also be configured in `solr.in.sh`.
Review comment:
And since there are more extensive docs on the `distributed-requests.adoc` page; lets link to there. Or move more of that info here as you prefer.
##########
File path: solr/solr-ref-guide/src/the-terms-component.adoc
##########
@@ -349,11 +349,11 @@ The TermsComponent also supports distributed indexes. For the `/terms` request h
`shards`::
Specifies the shards in your distributed indexing configuration. For more information about distributed indexing, see <<distributed-search-with-index-sharding.adoc#,Distributed Search with Index Sharding>>.
+
-The `shards` parameter is subject to a host whitelist that has to be configured in the component's parameters using the configuration key `shardsWhitelist` and the list of hosts as values.
+The `shards` parameter is subject to a host allow-list that has to be configured in the component's parameters using the configuration key `allowUrls` and the list of hosts as values.
Review comment:
Information on this page is obsolete; we can remove `shards` here. See https://issues.apache.org/jira/browse/SOLR-14036
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a change in pull request #47: SOLR-15217: Rename shardsWhitelist and extract AllowListUrlChecker to use it more broadly.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #47:
URL: https://github.com/apache/solr/pull/47#discussion_r602360428
##########
File path: solr/server/solr/solr.xml
##########
@@ -53,7 +54,6 @@
class="HttpShardHandlerFactory">
<int name="socketTimeout">${socketTimeout:600000}</int>
<int name="connTimeout">${connTimeout:60000}</int>
- <str name="shardsWhitelist">${solr.shardsWhitelist:}</str>
Review comment:
Yes, I added a warning in HttpShardHandlerFactory#init() line 228. Is it what you thought of?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org