From 8f2f80bbb3c35fef036dce3162f4f03bf465e5f2 Mon Sep 17 00:00:00 2001 From: Tomas Fernandez Lobbe Date: Thu, 1 Oct 2020 14:13:40 -0700 Subject: [PATCH] SOLR-14663: Copy ConfigSet root data from base ConfigSet when using CREATE command --- solr/CHANGES.txt | 2 + .../solr/handler/admin/ConfigSetsHandler.java | 50 +++++-- .../apache/solr/cloud/TestConfigSetsAPI.java | 136 ++++++++++++------ .../cloud/TestConfigSetsAPIExclusivity.java | 3 + .../cloud/TestConfigSetsAPIZkFailure.java | 1 + .../solr/handler/admin/TestConfigsApi.java | 4 - .../solr/common/cloud/ZkConfigManager.java | 23 ++- 7 files changed, 150 insertions(+), 69 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 32be5fcba51..66361d82f05 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -302,6 +302,8 @@ Bug Fixes --------------------- * SOLR-14751: Zookeeper Admin screen not working for old ZK versions (janhoy) +* SOLR-14663: Copy ConfigSet root data from base ConfigSet when using CREATE command (Andras Salamon, Tomás Fernández Löbbe) + ================== 8.6.1 ================== Bug Fixes diff --git a/solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java index ff1a255102c..6d94fbabe49 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java @@ -73,6 +73,7 @@ import static org.apache.solr.common.params.ConfigSetParams.ConfigSetAction.LIST * A {@link org.apache.solr.request.SolrRequestHandler} for ConfigSets API requests. */ public class ConfigSetsHandler extends RequestHandlerBase implements PermissionNameProvider { + final public static Boolean DISABLE_CREATE_AUTH_CHECKS = Boolean.getBoolean("solr.disableConfigSetsCreateAuthChecks"); // this is for back compat only final public static String DEFAULT_CONFIGSET_NAME = "_default"; final public static String AUTOCREATED_CONFIGSET_SUFFIX = ".AUTOCREATED"; private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); @@ -192,7 +193,7 @@ public class ConfigSetsHandler extends RequestHandlerBase implements PermissionN } else { filesToDelete = Collections.emptySet(); } - createBaseZnode(zkClient, overwritesExisting, isTrusted(req), cleanup, configPathInZk); + createBaseZnode(zkClient, overwritesExisting, isTrusted(req, coreContainer.getAuthenticationPlugin()), cleanup, configPathInZk); ZipInputStream zis = new ZipInputStream(inputStream, StandardCharsets.UTF_8); ZipEntry zipEntry = null; @@ -259,9 +260,19 @@ public class ConfigSetsHandler extends RequestHandlerBase implements PermissionN * Fail if an untrusted request tries to update a trusted ConfigSet */ private void ensureOverwritingUntrustedConfigSet(SolrZkClient zkClient, String configSetZkPath) { + boolean isCurrentlyTrusted = isCurrentlyTrusted(zkClient, configSetZkPath); + if (isCurrentlyTrusted) { + throw new SolrException(ErrorCode.BAD_REQUEST, "Trying to make an unstrusted ConfigSet update on a trusted configSet"); + } + } + + private static boolean isCurrentlyTrusted(SolrZkClient zkClient, String configSetZkPath) { byte[] configSetNodeContent; try { configSetNodeContent = zkClient.getData(configSetZkPath, null, null, true); + if (configSetNodeContent == null || configSetNodeContent.length == 0) { + return true; + } } catch (KeeperException e) { throw new SolrException(ErrorCode.SERVER_ERROR, "Exception while fetching current configSet at " + configSetZkPath, e); } catch (InterruptedException e) { @@ -270,21 +281,15 @@ public class ConfigSetsHandler extends RequestHandlerBase implements PermissionN } @SuppressWarnings("unchecked") Map contentMap = (Map) Utils.fromJSON(configSetNodeContent); - boolean isCurrentlyTrusted = (boolean) contentMap.getOrDefault("trusted", true); - if (isCurrentlyTrusted) { - throw new SolrException(ErrorCode.BAD_REQUEST, "Trying to make an unstrusted ConfigSet update on a trusted configSet"); - } + return (boolean) contentMap.getOrDefault("trusted", true); } - boolean isTrusted(SolrQueryRequest req) { - AuthenticationPlugin authcPlugin = coreContainer.getAuthenticationPlugin(); - if (log.isInfoEnabled()) { - log.info("Trying to upload a configset. authcPlugin: {}, user principal: {}", - authcPlugin, req.getUserPrincipal()); - } - if (authcPlugin != null && req.getUserPrincipal() != null) { + static boolean isTrusted(SolrQueryRequest req, AuthenticationPlugin authPlugin) { + if (authPlugin != null && req.getUserPrincipal() != null) { + log.debug("Trusted configset request"); return true; } + log.debug("Untrusted configset request"); return false; } @@ -361,8 +366,29 @@ public class ConfigSetsHandler extends RequestHandlerBase implements PermissionN @Override public Map call(SolrQueryRequest req, SolrQueryResponse rsp, ConfigSetsHandler h) throws Exception { String baseConfigSetName = req.getParams().get(BASE_CONFIGSET, DEFAULT_CONFIGSET_NAME); + String newConfigSetName = req.getParams().get(NAME); + if (newConfigSetName == null || newConfigSetName.length() == 0) { + throw new SolrException(ErrorCode.BAD_REQUEST, "ConfigSet name not specified"); + } + + ZkConfigManager zkConfigManager = new ZkConfigManager(h.coreContainer.getZkController().getZkStateReader().getZkClient()); + if (zkConfigManager.configExists(newConfigSetName)) { + throw new SolrException(ErrorCode.BAD_REQUEST, "ConfigSet already exists: " + newConfigSetName); + } + + // is there a base config that already exists + if (!zkConfigManager.configExists(baseConfigSetName)) { + throw new SolrException(ErrorCode.BAD_REQUEST, + "Base ConfigSet does not exist: " + baseConfigSetName); + } + Map props = CollectionsHandler.copy(req.getParams().required(), null, NAME); props.put(BASE_CONFIGSET, baseConfigSetName); + if (!DISABLE_CREATE_AUTH_CHECKS && + !isTrusted(req, h.coreContainer.getAuthenticationPlugin()) && + isCurrentlyTrusted(h.coreContainer.getZkController().getZkClient(), ZkConfigManager.CONFIGS_ZKNODE + "/" + baseConfigSetName)) { + throw new SolrException(ErrorCode.UNAUTHORIZED, "Can't create a configset with an unauthenticated request from a trusted " + BASE_CONFIGSET); + } return copyPropertiesWithPrefix(req.getParams(), props, PROPERTY_PREFIX + "."); } }, diff --git a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java index 6fea2cbae23..48f0dc8e521 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java @@ -35,6 +35,7 @@ import java.nio.ByteBuffer; import java.security.Principal; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Deque; import java.util.HashSet; import java.util.Iterator; @@ -68,6 +69,7 @@ import org.apache.solr.client.solrj.request.ConfigSetAdminRequest.Delete; import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.response.CollectionAdminResponse; import org.apache.solr.client.solrj.response.ConfigSetAdminResponse; +import org.apache.solr.common.SolrException; import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.cloud.SolrZkClient; import org.apache.solr.common.cloud.ZkConfigManager; @@ -136,51 +138,84 @@ public class TestConfigSetsAPI extends SolrCloudTestCase { @Test public void testCreateErrors() throws Exception { final String baseUrl = cluster.getJettySolrRunners().get(0).getBaseUrl().toString(); - final SolrClient solrClient = getHttpSolrClient(baseUrl); - zkConfigManager.uploadConfigDir(configset("configset-2"), "configSet"); + try (final SolrClient solrClient = getHttpSolrClient(baseUrl)) { + zkConfigManager.uploadConfigDir(configset("configset-2"), "configSet"); - // no action - CreateNoErrorChecking createNoAction = new CreateNoErrorChecking(); - createNoAction.setAction(null); - verifyException(solrClient, createNoAction, "action"); + // no action + CreateNoErrorChecking createNoAction = new CreateNoErrorChecking(); + createNoAction.setAction(null); + verifyException(solrClient, createNoAction, "action"); - // no ConfigSet name - CreateNoErrorChecking create = new CreateNoErrorChecking(); - verifyException(solrClient, create, NAME); + // no ConfigSet name + CreateNoErrorChecking create = new CreateNoErrorChecking(); + verifyException(solrClient, create, NAME); - // set ConfigSet - create.setConfigSetName("configSetName"); + // set ConfigSet + create.setConfigSetName("configSetName"); - // ConfigSet already exists - Create alreadyExists = new Create(); - alreadyExists.setConfigSetName("configSet").setBaseConfigSetName("baseConfigSet"); - verifyException(solrClient, alreadyExists, "ConfigSet already exists"); + // ConfigSet already exists + Create alreadyExists = new Create(); + alreadyExists.setConfigSetName("configSet").setBaseConfigSetName("baseConfigSet"); + verifyException(solrClient, alreadyExists, "ConfigSet already exists"); - // Base ConfigSet does not exist - Create baseConfigNoExists = new Create(); - baseConfigNoExists.setConfigSetName("newConfigSet").setBaseConfigSetName("baseConfigSet"); - verifyException(solrClient, baseConfigNoExists, "Base ConfigSet does not exist"); - - solrClient.close(); + // Base ConfigSet does not exist + Create baseConfigNoExists = new Create(); + baseConfigNoExists.setConfigSetName("newConfigSet").setBaseConfigSetName("baseConfigSet"); + verifyException(solrClient, baseConfigNoExists, "Base ConfigSet does not exist"); + } } @Test public void testCreate() throws Exception { // no old, no new - verifyCreate(null, "configSet1", null, null); + verifyCreate(null, "configSet1", null, null, "solr"); // no old, new verifyCreate("baseConfigSet2", "configSet2", - null, ImmutableMap.of("immutable", "true", "key1", "value1")); + null, ImmutableMap.of("immutable", "true", "key1", "value1"), "solr"); // old, no new verifyCreate("baseConfigSet3", "configSet3", - ImmutableMap.of("immutable", "false", "key2", "value2"), null); + ImmutableMap.of("immutable", "false", "key2", "value2"), null, "solr"); // old, new verifyCreate("baseConfigSet4", "configSet4", ImmutableMap.of("immutable", "true", "onlyOld", "onlyOldValue"), - ImmutableMap.of("immutable", "false", "onlyNew", "onlyNewValue")); + ImmutableMap.of("immutable", "false", "onlyNew", "onlyNewValue"), "solr"); + } + + @Test + public void testCreateWithTrust() throws Exception { + String configsetName = "regular"; + String configsetSuffix = "testCreateWithTrust"; + String configsetSuffix2 = "testCreateWithTrust2"; + uploadConfigSetWithAssertions(configsetName, configsetSuffix, "solr"); + uploadConfigSetWithAssertions(configsetName, configsetSuffix2, null); + try (SolrZkClient zkClient = new SolrZkClient(cluster.getZkServer().getZkAddress(), + AbstractZkTestCase.TIMEOUT, 45000, null)) { + assertTrue(isTrusted(zkClient, configsetName, configsetSuffix)); + assertFalse(isTrusted(zkClient, configsetName, configsetSuffix2)); + try { + ignoreException("unauthenticated request"); + // trusted -> unstrusted + createConfigSet(configsetName + configsetSuffix, "foo", Collections.emptyMap(), cluster.getSolrClient(), null); + fail("Expecting exception"); + } catch (SolrException e) { + assertEquals(SolrException.ErrorCode.UNAUTHORIZED.code, e.code()); + unIgnoreException("unauthenticated request"); + } + // trusted -> trusted + verifyCreate(configsetName + configsetSuffix, "foo2", Collections.emptyMap(), Collections.emptyMap(), "solr"); + assertTrue(isTrusted(zkClient, "foo2", "")); + + // unstrusted -> unstrusted + verifyCreate(configsetName + configsetSuffix2, "bar", Collections.emptyMap(), Collections.emptyMap(), null); + assertFalse(isTrusted(zkClient, "bar", "")); + + // unstrusted -> trusted + verifyCreate(configsetName + configsetSuffix2, "bar2", Collections.emptyMap(), Collections.emptyMap(), "solr"); + assertFalse(isTrusted(zkClient, "bar2", "")); + } } private void setupBaseConfigSet(String baseConfigSetName, Map oldProps) throws Exception { @@ -196,33 +231,40 @@ public class TestConfigSetsAPI extends SolrCloudTestCase { } private void verifyCreate(String baseConfigSetName, String configSetName, - Map oldProps, Map newProps) throws Exception { + Map oldProps, Map newProps, String username) throws Exception { final String baseUrl = cluster.getJettySolrRunners().get(0).getBaseUrl().toString(); - final SolrClient solrClient = getHttpSolrClient(baseUrl); - setupBaseConfigSet(baseConfigSetName, oldProps); + try (final SolrClient solrClient = getHttpSolrClient(baseUrl)) { + setupBaseConfigSet(baseConfigSetName, oldProps); - SolrZkClient zkClient = new SolrZkClient(cluster.getZkServer().getZkAddress(), - AbstractZkTestCase.TIMEOUT, AbstractZkTestCase.TIMEOUT, null); - try { - ZkConfigManager configManager = new ZkConfigManager(zkClient); - assertFalse(configManager.configExists(configSetName)); + SolrZkClient zkClient = new SolrZkClient(cluster.getZkServer().getZkAddress(), + AbstractZkTestCase.TIMEOUT, AbstractZkTestCase.TIMEOUT, null); + try { + ZkConfigManager configManager = new ZkConfigManager(zkClient); + assertFalse(configManager.configExists(configSetName)); - Create create = new Create(); - create.setBaseConfigSetName(baseConfigSetName).setConfigSetName(configSetName); - if (newProps != null) { - Properties p = new Properties(); - p.putAll(newProps); - create.setNewConfigSetProperties(p); + ConfigSetAdminResponse response = createConfigSet(baseConfigSetName, configSetName, newProps, solrClient, username); + assertNotNull(response.getResponse()); + assertTrue(configManager.configExists(configSetName)); + + verifyProperties(configSetName, oldProps, newProps, zkClient); + } finally { + zkClient.close(); } - ConfigSetAdminResponse response = create.process(solrClient); - assertNotNull(response.getResponse()); - assertTrue(configManager.configExists(configSetName)); - - verifyProperties(configSetName, oldProps, newProps, zkClient); - } finally { - zkClient.close(); } - solrClient.close(); + } + + private ConfigSetAdminResponse createConfigSet(String baseConfigSetName, String configSetName, Map newProps, SolrClient solrClient, String username) throws SolrServerException, IOException { + Create create = new Create(); + create.setBaseConfigSetName(baseConfigSetName).setConfigSetName(configSetName); + if (newProps != null) { + Properties p = new Properties(); + p.putAll(newProps); + create.setNewConfigSetProperties(p); + } + if (username != null) { + create.addHeader("user", username); + } + return create.process(solrClient); } @SuppressWarnings({"rawtypes"}) diff --git a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIExclusivity.java b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIExclusivity.java index 20a88e0dcac..bb75e7b4dbe 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIExclusivity.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIExclusivity.java @@ -17,6 +17,7 @@ package org.apache.solr.cloud; import java.lang.invoke.MethodHandles; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.LinkedList; import java.util.List; @@ -92,6 +93,8 @@ public class TestConfigSetsAPIExclusivity extends SolrTestCaseJ4 { private void setupBaseConfigSet(String baseConfigSetName) throws Exception { solrCluster.uploadConfigSet(configset("configset-2"), baseConfigSetName); + //Make configset untrusted + solrCluster.getZkClient().setData("/configs/" + baseConfigSetName, "{\"trusted\": false}".getBytes(StandardCharsets.UTF_8), true); } private Exception getFirstExceptionOrNull(List list) { diff --git a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIZkFailure.java b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIZkFailure.java index 692c82a4290..e23f71a7a0a 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIZkFailure.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIZkFailure.java @@ -137,6 +137,7 @@ public class TestConfigSetsAPIZkFailure extends SolrTestCaseJ4 { getConfigSetProps(oldProps), StandardCharsets.UTF_8); } solrCluster.uploadConfigSet(tmpConfigDir.toPath(), baseConfigSetName); + solrCluster.getZkClient().setData("/configs/" + baseConfigSetName, "{\"trusted\": false}".getBytes(StandardCharsets.UTF_8), true); } private StringBuilder getConfigSetProps(Map map) { diff --git a/solr/core/src/test/org/apache/solr/handler/admin/TestConfigsApi.java b/solr/core/src/test/org/apache/solr/handler/admin/TestConfigsApi.java index a77830f0c5b..f0ce7ef5a9a 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/TestConfigsApi.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/TestConfigsApi.java @@ -27,7 +27,6 @@ import org.apache.solr.handler.ClusterAPI; import org.apache.solr.response.SolrQueryResponse; import static org.apache.solr.client.solrj.SolrRequest.METHOD.DELETE; -import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST; import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION; import static org.apache.solr.handler.admin.TestCollectionAPIs.compareOutput; @@ -58,9 +57,6 @@ public class TestConfigsApi extends SolrTestCaseJ4 { // for (Api api : handler.getApis()) apiBag.register(api, EMPTY_MAP); compareOutput(apiBag, "/cluster/configs/sample", DELETE, null, null, "{name :sample, operation:delete}"); - - compareOutput(apiBag, "/cluster/configs", POST, "{create:{name : newconf, baseConfigSet: sample }}", null, - "{operation:create, name :newconf, baseConfigSet: sample, immutable: false }"); } } } diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkConfigManager.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkConfigManager.java index aa6404bf0a7..72dacf22776 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkConfigManager.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkConfigManager.java @@ -138,11 +138,7 @@ public class ZkConfigManager { for (String file : files) { List children = zkClient.getChildren(fromZkPath + "/" + file, null, true); if (children.size() == 0) { - final String toZkFilePath = toZkPath + "/" + file; - log.info("Copying zk node {}/{} to {}", fromZkPath, file, toZkFilePath); - byte[] data = zkClient.getData(fromZkPath + "/" + file, null, null, true); - zkClient.makePath(toZkFilePath, data, true); - if (copiedToZkPaths != null) copiedToZkPaths.add(toZkFilePath); + copyData(copiedToZkPaths, fromZkPath + "/" + file, toZkPath + "/" + file); } else { copyConfigDirFromZk(fromZkPath + "/" + file, toZkPath + "/" + file, copiedToZkPaths); } @@ -153,6 +149,13 @@ public class ZkConfigManager { } } + private void copyData(Set copiedToZkPaths, String fromZkFilePath, String toZkFilePath) throws KeeperException, InterruptedException { + log.info("Copying zk node {} to {}", fromZkFilePath, toZkFilePath); + byte[] data = zkClient.getData(fromZkFilePath, null, null, true); + zkClient.makePath(toZkFilePath, data, true); + if (copiedToZkPaths != null) copiedToZkPaths.add(toZkFilePath); + } + /** * Copy a config in ZooKeeper * @@ -174,7 +177,15 @@ public class ZkConfigManager { * @throws IOException if an I/O error occurs */ public void copyConfigDir(String fromConfig, String toConfig, Set copiedToZkPaths) throws IOException { - copyConfigDirFromZk(CONFIGS_ZKNODE + "/" + fromConfig, CONFIGS_ZKNODE + "/" + toConfig, copiedToZkPaths); + String fromConfigPath = CONFIGS_ZKNODE + "/" + fromConfig; + String toConfigPath = CONFIGS_ZKNODE + "/" + toConfig; + try { + copyData(copiedToZkPaths, fromConfigPath, toConfigPath); + } catch (KeeperException | InterruptedException e) { + throw new IOException("Error config " + fromConfig + " to " + toConfig, + SolrZkClient.checkInterrupted(e)); + } + copyConfigDirFromZk(fromConfigPath, toConfigPath, copiedToZkPaths); } // This method is used by configSetUploadTool and CreateTool to resolve the configset directory.