gerlowskija commented on code in PR #5287:
URL: https://github.com/apache/solr/pull/5287#discussion_r4512520297
##########
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java:
##########
@@ -96,13 +97,22 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
final var configSetName = req.getParams().required().get(NAME);
final var overwrite =
req.getParams().getBool(ConfigSetParams.OVERWRITE, false);
final var cleanup = req.getParams().getBool(ConfigSetParams.CLEANUP,
false);
- final var configSetData =
ConfigSetAPIBase.ensureNonEmptyInputStream(req);
SolrJerseyResponse uploadResponse;
- if (req.getParams()
- .get(ConfigSetParams.FILE_PATH, "")
- .isEmpty()) { // Uploading a whole configset
+ final var repository =
req.getParams().get(CoreAdminParams.BACKUP_REPOSITORY);
+ final var location =
req.getParams().get(CoreAdminParams.BACKUP_LOCATION);
+ if ((repository == null) != (location == null)) {
Review Comment:
[0] This validation should probably live in the v2 API implementation
class(es). Or if you have added it there elsewhere in this diff, then it
probably isn't needed here.
##########
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##########
@@ -125,48 +148,184 @@ public SolrJerseyResponse uploadConfigSet(
} finally {
Files.deleteIfExists(tempZip);
}
- deleteUnusedFiles(configSetService, configSetName, filesToDelete);
+ }
- return response;
+ private void uploadFromRepository(
+ String configSetName,
+ String repositoryName,
+ String requestedLocation,
+ List<String> filesToDelete)
+ throws IOException {
+ try (BackupRepository repository =
coreContainer.newBackupRepository(repositoryName)) {
+ final String location = resolveRepositoryLocation(repository,
requestedLocation);
+ final URI source = repository.createDirectoryURI(location);
+ if (!repository.exists(source)) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The location " + source + " does not exist in the backup
repository");
+ }
+
+ // Read everything before writing so a failure doesn't leave a
half-uploaded configset.
+ final Map<String, byte[]> files = new LinkedHashMap<>();
Review Comment:
[0] Could this be an OOM risk? if a user accidentally provides a truncated
`location` value it seems like nothing would stop Solr from copying a
(potentially large) file tree and keeping each byte on-heap.
##########
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##########
@@ -125,48 +148,184 @@ public SolrJerseyResponse uploadConfigSet(
} finally {
Files.deleteIfExists(tempZip);
}
- deleteUnusedFiles(configSetService, configSetName, filesToDelete);
+ }
- return response;
+ private void uploadFromRepository(
+ String configSetName,
+ String repositoryName,
+ String requestedLocation,
+ List<String> filesToDelete)
+ throws IOException {
+ try (BackupRepository repository =
coreContainer.newBackupRepository(repositoryName)) {
+ final String location = resolveRepositoryLocation(repository,
requestedLocation);
+ final URI source = repository.createDirectoryURI(location);
+ if (!repository.exists(source)) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The location " + source + " does not exist in the backup
repository");
+ }
+
+ // Read everything before writing so a failure doesn't leave a
half-uploaded configset.
+ final Map<String, byte[]> files = new LinkedHashMap<>();
+ readConfigFilesFromRepository(repository, source, "", files);
+ if (files.isEmpty()) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "No configset files were found at " + source + " in the backup
repository");
+ }
+
+ for (var file : files.entrySet()) {
+ filesToDelete.remove(file.getKey());
+ configSetService.uploadFileToConfig(configSetName, file.getKey(),
file.getValue(), true);
+ }
+ }
+ }
+
+ /** Whether the upload reads from a backup repository; 'repository' and
'location' go together. */
+ private static boolean isFromRepository(String repository, String location) {
+ if ((repository == null) != (location == null)) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The 'repository' and 'location' parameters must be provided
together.");
+ }
+ return repository != null;
+ }
+
+ private static void ensureSingleSource(boolean fromRepository, InputStream
requestBody)
+ throws IOException {
+ if (fromRepository && requestBody != null && requestBody.available() > 0) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "An upload must provide either a request body or a repository
location, not both.");
+ }
+ }
+
+ private static void readConfigFilesFromRepository(
+ BackupRepository repository, URI dir, String prefix, Map<String, byte[]>
files)
+ throws IOException {
+ for (String name : repository.listAll(dir)) {
+ final String filePath = prefix + name;
+ switch (repository.getPathType(repository.resolve(dir, name))) {
+ case DIRECTORY:
+ if (!name.startsWith(".")) {
+ readConfigFilesFromRepository(
+ repository, repository.resolveDirectory(dir, name), filePath +
"/", files);
+ }
+ break;
+ case FILE:
+ if (ConfigSetService.isFileForbiddenInConfigSets(filePath)) {
+ log.warn("Not uploading file from repository, as it is a forbidden
type: {}", filePath);
+ break;
+ }
+ final byte[] data = readRepositoryFile(repository, dir, name);
+ if (FileTypeMagicUtil.isFileForbiddenInConfigset(data)) {
+ log.warn(
+ "Not uploading file {} from repository, as it matched the
MAGIC signature of a forbidden mime type",
+ filePath);
+ } else {
+ files.put(filePath, data);
+ }
+ break;
+ }
+ }
+ }
+
+ private String resolveRepositoryLocation(BackupRepository repository, String
requestedLocation) {
+ final String location = repository.getBackupLocation(requestedLocation);
+ if (location == null) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "'location' is not specified as a parameter or as a default
repository property");
+ }
+ if ("file".equals(repository.createURI("x").getScheme())) {
Review Comment:
[0] Creating a URI for a non-existent file seems a bit hacky. Could we just
use the location string itself, or is there a reason to avoid that?
```
if ("file".equals(locationUri.getScheme())) {
core.getCoreContainer().assertPathAllowed(Paths.get(locationUri));
}
```
##########
solr/core/src/test/org/apache/solr/cloud/LocalFSConfigSetUploadFromRepositoryTest.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
+
+import java.net.URI;
+import java.nio.file.Path;
+import
org.apache.solr.cloud.api.collections.AbstractConfigSetUploadFromRepositoryTest;
+import org.apache.solr.cloud.api.collections.AbstractInstallShardTest;
+import org.eclipse.jetty.client.ContentResponse;
+import org.eclipse.jetty.client.StringRequestContent;
+import org.eclipse.jetty.http.HttpMethod;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Tests uploading configsets from a backup repository with {@link
+ * org.apache.solr.core.backup.repository.LocalFileSystemRepository}.
+ */
+public class LocalFSConfigSetUploadFromRepositoryTest
+ extends AbstractConfigSetUploadFromRepositoryTest {
+
+ private static final String REPOSITORY_NAME = "local";
+
+ private static final String SOLR_XML =
+ AbstractInstallShardTest.defaultSolrXmlTextWithBackupRepository(
+ "<str name=\"allowPaths\">${solr.security.allow.paths:}</str>\n"
Review Comment:
```suggestion
"<str name=\"allowPaths\">*</str>\n"
```
If we're always going to set this property to `*`, we don't need the
placeholder variable here...
##########
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractConfigSetUploadFromRepositoryTest.java:
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.api.collections;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.net.URI;
+import java.util.UUID;
+import org.apache.solr.client.solrj.RemoteSolrException;
+import org.apache.solr.client.solrj.request.ConfigsetsApi;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.core.backup.repository.BackupRepository;
+import org.junit.Test;
+
+/**
+ * Base class for testing configset uploads from a backup repository with
various repository types.
+ *
+ * <p>Subclasses bootstrap a Solr cluster with a single configured backup
repository, and tell this
+ * class its name and where in it test data can be written. This class stages
the files through the
+ * cluster's own {@link BackupRepository}, so it works for any repository
implementation.
+ *
+ * @see org.apache.solr.handler.configsets.UploadConfigSet
+ */
+public abstract class AbstractConfigSetUploadFromRepositoryTest extends
SolrCloudTestCase {
+
+ /** The name of the backup repository, as configured in solr.xml, to read
from. */
+ protected abstract String repositoryName();
+
+ /** A location in the repository under which each test stages its own files.
*/
+ protected abstract String baseLocation();
+
+ /** The 'location' parameter that refers to {@code uri}; some repositories
want a plain path. */
+ protected String locationParam(URI uri) {
+ return uri.toString();
+ }
+
+ /** Writes a file, and any parent directories, under the test's own
directory. */
+ protected URI stageFile(URI testDir, String relativePath, String content)
throws Exception {
+ try (BackupRepository repository = newRepository()) {
+ URI dir = testDir;
+ repository.createDirectory(dir);
+ final String[] parts = relativePath.split("/");
+ for (int i = 0; i < parts.length - 1; i++) {
+ dir = repository.resolveDirectory(dir, parts[i]);
+ repository.createDirectory(dir);
+ }
+ final URI file = repository.resolve(dir, parts[parts.length - 1]);
+ try (OutputStream os = repository.createOutput(file)) {
+ os.write(content.getBytes(UTF_8));
+ }
+ return file;
+ }
+ }
+
+ /** A fresh, not yet created, directory in the repository for the current
test to stage into. */
+ protected URI newTestDir() throws Exception {
+ try (BackupRepository repository = newRepository()) {
+ return repository.resolveDirectory(
+ repository.createDirectoryURI(baseLocation()),
UUID.randomUUID().toString());
+ }
+ }
+
+ private BackupRepository newRepository() {
+ return
cluster.getJettySolrRunner(0).getCoreContainer().newBackupRepository(repositoryName());
+ }
+
+ private ConfigsetsApi.UploadConfigSet uploadRequest(String configSetName,
URI source) {
+ final var request = new ConfigsetsApi.UploadConfigSet(configSetName,
emptyBody());
Review Comment:
[+1] Love that you're using the generated client objects here, great to get
test coverage on these!
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configsets-api.adoc:
##########
@@ -167,6 +167,40 @@ When overwriting an existing configset (`overwrite=true`),
this parameter tells
When uploading an entire configset, the body of the request should be a zip
file that contains the configset.
The zip file must be created from within the `conf` directory (i.e.,
`solrconfig.xml` must be the top level entry in the zip file).
+[[configsets-upload-repository]]
+=== Upload from a Backup Repository
+
+Instead of a zip file, the configset can be read from a directory in a
xref:deployment-guide:backup-restore.adoc#backuprestore-storage-repositories[backup
repository] (such as S3 or GCS), with `solrconfig.xml` at its top level.
+Pass these parameters and no request body:
+
+`repository`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: none
+|===
++
+The name of the backup repository. Required if `location` is given.
+
+`location`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: none
+|===
++
+The directory in the repository that holds the configset's files. Required if
`repository` is given.
+
+Providing both a request body and `repository`/`location` is an error.
+The `overwrite` and `cleanup` parameters work the same way as for zip uploads.
+
+[source,bash]
+----
Review Comment:
[0] Love the docs. I'll push a change to reformat the v1/v2 examples so
they use the "tabbed" formatting we've been adopting more and more for "paired"
examples (e.g. v1/v2, Windows/Linux, etc.)
##########
solr/core/src/test/org/apache/solr/cloud/LocalFSConfigSetUploadFromRepositoryTest.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
+
+import java.net.URI;
+import java.nio.file.Path;
+import
org.apache.solr.cloud.api.collections.AbstractConfigSetUploadFromRepositoryTest;
+import org.apache.solr.cloud.api.collections.AbstractInstallShardTest;
+import org.eclipse.jetty.client.ContentResponse;
+import org.eclipse.jetty.client.StringRequestContent;
+import org.eclipse.jetty.http.HttpMethod;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Tests uploading configsets from a backup repository with {@link
+ * org.apache.solr.core.backup.repository.LocalFileSystemRepository}.
+ */
+public class LocalFSConfigSetUploadFromRepositoryTest
+ extends AbstractConfigSetUploadFromRepositoryTest {
+
+ private static final String REPOSITORY_NAME = "local";
+
+ private static final String SOLR_XML =
+ AbstractInstallShardTest.defaultSolrXmlTextWithBackupRepository(
+ "<str name=\"allowPaths\">${solr.security.allow.paths:}</str>\n"
+ + "<backup>\n"
+ + " <repository name=\""
+ + REPOSITORY_NAME
+ + "\"
class=\"org.apache.solr.core.backup.repository.LocalFileSystemRepository\"/>\n"
+ + "</backup>\n");
+
+ private static Path repositoryDir;
+
+ @BeforeClass
+ public static void setupClass() throws Exception {
+ System.setProperty("solr.security.allow.paths", "*");
Review Comment:
```suggestion
```
(Only accept this if you take the proposed change on L40 above)
##########
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##########
@@ -125,48 +148,184 @@ public SolrJerseyResponse uploadConfigSet(
} finally {
Files.deleteIfExists(tempZip);
}
- deleteUnusedFiles(configSetService, configSetName, filesToDelete);
+ }
- return response;
+ private void uploadFromRepository(
+ String configSetName,
+ String repositoryName,
+ String requestedLocation,
+ List<String> filesToDelete)
+ throws IOException {
+ try (BackupRepository repository =
coreContainer.newBackupRepository(repositoryName)) {
+ final String location = resolveRepositoryLocation(repository,
requestedLocation);
+ final URI source = repository.createDirectoryURI(location);
+ if (!repository.exists(source)) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The location " + source + " does not exist in the backup
repository");
Review Comment:
[Q] This line got me thinking: "Oh I wonder if users could use this to sniff
out files that exist up in the repository", and that got me thinking about the
security implications of this change more broadly.
With these params any users with upload-configset perms now have the ability
to essentially read arbitrary content out of whatever repository the admins
have set up.
A few disjointed thoughts along these lines:
- I guess, technically, arbitrary-read is a risk with the existing
backup-restore code, but that seems significantly mitigated by the fact that
backups have a lot of structure and the API is validating that "location"
values point to a valid backup before its doing any file copies, etc.
Configsets don't have that structure necessarily, so we can't do much similar
validation here.
- Many repositories take a default or "root" location. The risk would be
much reduced if S3BackupRepository and friends were coded to prevent
path-escape from that root location. But at a glance that doesn't appear to
the case?
- The community has generally agreed that "configset-upload" perms are
pretty dangerous and should only be given out to very trusted users. So maybe
this isn't a huge deal in practice, and could just be solved with documentation
saying that "upload-configset allows users to read out of backupRepositories"
- If it's not just me and folks feel like there are some valid security
concerns here, maybe a solr.xml setting or sysprop to gate the use of these
parameters would help? i.e.
`<allowBackupRepoConfigsetUpload>true</allowBackupRepoConfigsetUpload>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
