Allow migrating H2 to ChronicleMap from non-admin Non-admins should be delegated to be able to migrate the H2 caches to ChronicleMap, so that service users can be safely used without having to compromise the admin credentials. Also fix a security hole where all registered users were allowed to run any SSH commands provided by the plugin and apply instead the same authorization policy enforced for the H2 migration REST-API. Bug: Issue 14793 Change-Id: I2935403ca23f833af8b1e8065f6eee5f9e4dd7fe
diff --git a/BUILD b/BUILD index 7029515..612cb3d 100644 --- a/BUILD +++ b/BUILD
@@ -11,6 +11,7 @@ name = "cache-chroniclemap", srcs = glob(["src/main/java/**/*.java"]), manifest_entries = [ + "Gerrit-Module: com.googlesource.gerrit.modules.cache.chroniclemap.CapabilityModule", "Gerrit-SshModule: com.googlesource.gerrit.modules.cache.chroniclemap.SSHCommandModule", "Gerrit-HttpModule: com.googlesource.gerrit.modules.cache.chroniclemap.HttpModule", ],
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AdministerCachePermission.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AdministerCachePermission.java new file mode 100644 index 0000000..8cc06d5 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AdministerCachePermission.java
@@ -0,0 +1,63 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed 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 com.googlesource.gerrit.modules.cache.chroniclemap; + +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.annotations.PluginName; +import com.google.gerrit.extensions.api.access.PluginPermission; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.inject.Inject; +import java.util.function.Consumer; + +class AdministerCachePermission { + private final PermissionBackend permissionBackend; + private final String pluginName; + + @Inject + AdministerCachePermission(PermissionBackend permissionBackend, @PluginName String pluginName) { + this.permissionBackend = permissionBackend; + this.pluginName = pluginName; + } + + boolean isCurrentUserAllowed() { + try { + checkCurrentUserAllowed(null); + return true; + } catch (AuthException | PermissionBackendException e) { + return false; + } + } + + void checkCurrentUserAllowed(@Nullable Consumer<Exception> failureFunction) + throws AuthException, PermissionBackendException { + try { + permissionBackend + .currentUser() + .checkAny( + ImmutableSet.of( + GlobalPermission.ADMINISTRATE_SERVER, + new PluginPermission(pluginName, AdministerCachesCapability.ID))); + } catch (AuthException | PermissionBackendException e) { + if (failureFunction != null) { + failureFunction.accept(e); + } + throw e; + } + } +}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AdministerCachesCapability.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AdministerCachesCapability.java new file mode 100644 index 0000000..3d818b0 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AdministerCachesCapability.java
@@ -0,0 +1,26 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed 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 com.googlesource.gerrit.modules.cache.chroniclemap; + +import com.google.gerrit.extensions.config.CapabilityDefinition; + +public class AdministerCachesCapability extends CapabilityDefinition { + static final String ID = "administerCaches"; + + @Override + public String getDescription() { + return "Administer Caches"; + } +}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2Caches.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2Caches.java index 6f501a9..0ec4d2e 100644 --- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2Caches.java +++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2Caches.java
@@ -36,15 +36,22 @@ private final Config gerritConfig; private final SitePaths site; + private final AdministerCachePermission adminCachePermission; @Inject - AnalyzeH2Caches(@GerritServerConfig Config cfg, SitePaths site) { + AnalyzeH2Caches( + @GerritServerConfig Config cfg, + SitePaths site, + AdministerCachePermission adminCachePermission) { this.gerritConfig = cfg; this.site = site; + this.adminCachePermission = adminCachePermission; } @Override protected void run() throws Exception { + adminCachePermission.checkCurrentUserAllowed(e -> stderr.println(e.getLocalizedMessage())); + Set<Path> h2Files = getH2CacheFiles(); stdout.println("Extracting information from H2 caches...");
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java index d668884..a9f8bb7 100644 --- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java +++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCaches.java
@@ -43,6 +43,7 @@ private final DynamicMap<Cache<?, ?>> cacheMap; private final ChronicleMapCacheConfig.Factory configFactory; private final Path cacheDir; + private final AdministerCachePermission adminCachePermission; @Option( name = "--dry-run", @@ -55,14 +56,18 @@ @GerritServerConfig Config cfg, SitePaths site, DynamicMap<Cache<?, ?>> cacheMap, - ChronicleMapCacheConfig.Factory configFactory) { + ChronicleMapCacheConfig.Factory configFactory, + AdministerCachePermission adminCachePermission) { this.cacheMap = cacheMap; this.configFactory = configFactory; this.cacheDir = getCacheDir(site, cfg.getString("cache", null, "directory")); + this.adminCachePermission = adminCachePermission; } @Override protected void run() throws Exception { + adminCachePermission.checkCurrentUserAllowed(e -> stderr.println(e.getLocalizedMessage())); + Config outputChronicleMapConfig = new Config(); Map<String, ChronicleMapCacheImpl<Object, Object>> chronicleMapCaches = getChronicleMapCaches();
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CapabilityModule.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CapabilityModule.java new file mode 100644 index 0000000..9764571 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CapabilityModule.java
@@ -0,0 +1,28 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed 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 com.googlesource.gerrit.modules.cache.chroniclemap; + +import com.google.gerrit.extensions.annotations.Exports; +import com.google.gerrit.extensions.config.CapabilityDefinition; +import com.google.inject.AbstractModule; + +public class CapabilityModule extends AbstractModule { + @Override + protected void configure() { + bind(CapabilityDefinition.class) + .annotatedWith(Exports.named(AdministerCachesCapability.ID)) + .to(AdministerCachesCapability.class); + } +}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java index 3dc7ef8..39d90cb 100644 --- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java +++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java
@@ -26,7 +26,6 @@ import com.google.gerrit.entities.CachedProjectConfig; import com.google.gerrit.extensions.auth.oauth.OAuthToken; import com.google.gerrit.extensions.client.ChangeKind; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.httpd.WebSessionManager; import com.google.gerrit.metrics.DisabledMetricMaker; @@ -46,9 +45,6 @@ import com.google.gerrit.server.patch.IntraLineDiffKey; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListKey; -import com.google.gerrit.server.permissions.GlobalPermission; -import com.google.gerrit.server.permissions.PermissionBackend; -import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.query.change.ConflictKey; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -81,7 +77,7 @@ private final ChronicleMapCacheConfig.Factory configFactory; private final SitePaths site; private final Config gerritConfig; - private final PermissionBackend permissionBackend; + private final AdministerCachePermission adminCachePermission; public static int DEFAULT_SIZE_MULTIPLIER = 3; public static int DEFAULT_MAX_BLOAT_FACTOR = 3; @@ -96,7 +92,7 @@ @GerritServerConfig Config cfg, SitePaths site, ChronicleMapCacheConfig.Factory configFactory, - PermissionBackend permissionBackend, + AdministerCachePermission permissionBackend, @Named("web_sessions") PersistentCacheDef<String, WebSessionManager.Val> webSessionsCacheDef, @Named("accounts") PersistentCacheDef<CachedAccountDetails.Key, CachedAccountDetails> accountsCacheDef, @@ -121,7 +117,7 @@ this.configFactory = configFactory; this.site = site; this.gerritConfig = cfg; - this.permissionBackend = permissionBackend; + this.adminCachePermission = permissionBackend; this.persistentCacheDefs = Stream.of( webSessionsCacheDef, @@ -150,15 +146,11 @@ return; } - try { - permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER); - } catch (AuthException | PermissionBackendException e) { - setResponse( - rsp, - HttpServletResponse.SC_FORBIDDEN, - "administrateServer for plugin cache-chroniclemap not permitted"); + if (!adminCachePermission.isCurrentUserAllowed()) { + setResponse(rsp, HttpServletResponse.SC_FORBIDDEN, "not permitted to administer caches"); return; } + Optional<Path> cacheDir = getCacheDir(); int maxBloatFactor =
diff --git a/src/main/resources/Documentation/migration.md b/src/main/resources/Documentation/migration.md index 0370232..11ed5b5 100644 --- a/src/main/resources/Documentation/migration.md +++ b/src/main/resources/Documentation/migration.md
@@ -1,8 +1,8 @@ ## Migration from H2 Caches This module provides a REST API to help converting existing cache from H2 to -chronicle-map, which requires the `Administrate Server` capability to be -executed. +chronicle-map, which requires the `Administrate Caches` or `Administrate Server` +capabilities to be executed. The migration must be executed _before_ switching to use chronicle-map, while Gerrit cache is still backed by H2.
diff --git a/src/main/resources/Documentation/tuning.md b/src/main/resources/Documentation/tuning.md index 1571723..7ed7868 100644 --- a/src/main/resources/Documentation/tuning.md +++ b/src/main/resources/Documentation/tuning.md
@@ -23,6 +23,9 @@ The idea is to read from the _actual_ H2 persisted files and output the information that will be required to configure chronicle-map as an alternative. +The Gerrit/SSH command to analyze H2 caches requires the user to have + `Administrate Caches` or `Administrate Server` capabilities. + You can do this _before_ installing cache-chroniclemap as a lib module so that your Gerrit server will not need downtime. As follows: @@ -130,6 +133,9 @@ suboptimal, chronicle-map caches and migrate into new ones for which a more realistic configuration is generated based on data. +The Gerrit/SSH command to tuning the caches requires the user to have + `Administrate Caches` or `Administrate Server` capabilities. + * Symlink the `cache-chroniclemap.jar` file in the `plugins/` directory (from the `lib/` directory). * Wait for the pluginLoader to acknowledge and load the new plugin. You will see
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2CachesIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2CachesIT.java index c3d39f7..15140b8 100644 --- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2CachesIT.java +++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AnalyzeH2CachesIT.java
@@ -57,6 +57,12 @@ } @Test + public void shouldDenyAccessToAnalyzeH2Cache() throws Exception { + userSshSession.exec(cmd); + userSshSession.assertFailure("not permitted"); + } + + @Test public void shouldProduceWarningWhenCacheFileIsEmpty() throws Exception { List<String> expected = ImmutableList.of(
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java index 88bce20..aa88a6d 100644 --- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java +++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/AutoAdjustCachesIT.java
@@ -99,6 +99,12 @@ assertThat(tunedCaches.size()).isEqualTo(EXPECTED_CACHES.size()); } + @Test + public void shouldDenyAccessToCreateNewCacheFiles() throws Exception { + userSshSession.exec(cmd); + userSshSession.assertFailure("not permitted"); + } + private Config configResult(String result) throws ConfigInvalidException { Config configResult = new Config(); configResult.fromText((result.split(CONFIG_HEADER))[1]);
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesInMemoryIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesInMemoryIT.java index e72d598..dbca0df 100644 --- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesInMemoryIT.java +++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesInMemoryIT.java
@@ -48,8 +48,7 @@ public void shouldFailWhenUserHasNoAdminServerCapability() throws Exception { RestResponse result = runMigration(userRestSession); result.assertForbidden(); - assertThat(result.getEntityContent()) - .contains("administrateServer for plugin cache-chroniclemap not permitted"); + assertThat(result.getEntityContent()).contains("not permitted"); } @Test
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesLocalDiskIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesLocalDiskIT.java index 4aff246..b19c622 100644 --- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesLocalDiskIT.java +++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/MigrateH2CachesLocalDiskIT.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.modules.cache.chroniclemap; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability; import static com.googlesource.gerrit.modules.cache.chroniclemap.H2CacheCommand.H2_SUFFIX; import static com.googlesource.gerrit.modules.cache.chroniclemap.H2MigrationServlet.DEFAULT_MAX_BLOAT_FACTOR; import static com.googlesource.gerrit.modules.cache.chroniclemap.H2MigrationServlet.DEFAULT_SIZE_MULTIPLIER; @@ -31,6 +32,7 @@ import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.acceptance.WaitUtil; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.entities.CachedProjectConfig; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; @@ -41,9 +43,11 @@ import com.google.gerrit.server.cache.proto.Cache; import com.google.gerrit.server.cache.serialize.ObjectIdConverter; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.group.SystemGroupBackend; import com.google.inject.Binding; import com.google.inject.Inject; import com.google.inject.Key; +import com.google.inject.Module; import java.io.IOException; import java.lang.annotation.Annotation; import java.nio.file.Path; @@ -66,6 +70,7 @@ private String MIGRATION_ENDPOINT = "/plugins/cache-chroniclemap/migrate"; @Inject private SitePaths sitePaths; + @Inject private ProjectOperations projectOperations; private ChronicleMapCacheConfig.Factory chronicleMapCacheConfigFactory; @@ -75,6 +80,12 @@ plugin.getHttpInjector().getInstance(ChronicleMapCacheConfig.Factory.class); } + /** Override to bind an additional Guice module */ + @Override + public Module createModule() { + return new CapabilityModule(); + } + @Test public void shouldRunAndCompleteSuccessfullyWhenCacheDirectoryIsDefined() throws Exception { runMigration(adminRestSession).assertOK(); @@ -116,6 +127,25 @@ } @Test + public void shouldDenyH2MigrationForNonAdminsAndUsersWithoutAdministerCachePermission() + throws Exception { + waitForCacheToLoad(ACCOUNTS_CACHE_NAME); + waitForCacheToLoad(PERSISTED_PROJECTS_CACHE_NAME); + + runMigration(userRestSession).assertForbidden(); + + projectOperations + .project(allProjects) + .forUpdate() + .add( + allowCapability("cache-chroniclemap-" + AdministerCachesCapability.ID) + .group(SystemGroupBackend.REGISTERED_USERS)) + .update(); + + runMigration(userRestSession).assertOK(); + } + + @Test public void shouldOutputChronicleMapBloatedProvidedConfiguration() throws Exception { waitForCacheToLoad(ACCOUNTS_CACHE_NAME); waitForCacheToLoad(PERSISTED_PROJECTS_CACHE_NAME);