Use SecureStore to access replication credentials Gerrit introduced the SecureStore in Ibbb15ad2aa over 10 years ago, however, the replication plugin was never adapted and then unable to access the remote endpoint credentials when Gerrit has a custom secure provider installed that would provide data encryption at rest. Replace the direct reading of the secure.config with the abstract implementation of the Gerrit SecureStore, so that it can still be working as expected with encrypted credentials. Existing installations may have used a mix of encrypted and clear text credentials in secure.config, leveraging the replication plugin bug that was not accessing it using the correct API. Introduce a legacy feature flag 'gerrit.useLegacyCredentials' that allow the Gerrit admin to still use the legacy mode. Whenever the replication plugin detects the legacy mode, it displays a warning explaining what is happening and how to adjust the configuration and enable full encryption in secure.config. Release-Notes: Use SecureStore for reading username/password credentials Bug: Issue 320715545 Change-Id: Ie5b6339d65d144536416cf070d52f11342b39fe6
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java index 2c049fc..5e2c758 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
@@ -128,4 +128,9 @@ public Config getConfig() { return currentConfig.getConfig(); } + + @Override + public boolean useLegacyCredentials() { + return currentConfig.useLegacyCredentials(); + } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java index 94fb910..8e21a81 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java
@@ -14,14 +14,12 @@ package com.googlesource.gerrit.plugins.replication; -import static com.google.gerrit.common.FileUtil.lastModified; - import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.securestore.SecureStore; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.replication.api.ReplicationConfig; import java.io.IOException; -import java.nio.file.Files; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.transport.CredentialsProvider; @@ -29,34 +27,44 @@ class AutoReloadSecureCredentialsFactoryDecorator implements CredentialsFactory { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private final AtomicReference<SecureCredentialsFactory> secureCredentialsFactory; - private volatile long secureCredentialsFactoryLoadTs; + private final AtomicReference<CredentialsFactory> secureCredentialsFactory; + private final SecureStore secureStore; private final SitePaths site; - private ReplicationConfig config; + private final ReplicationConfig config; @Inject - public AutoReloadSecureCredentialsFactoryDecorator(SitePaths site, ReplicationConfig config) + public AutoReloadSecureCredentialsFactoryDecorator( + SitePaths site, SecureStore secureStore, ReplicationConfig config) throws ConfigInvalidException, IOException { this.site = site; + this.secureStore = secureStore; this.config = config; - this.secureCredentialsFactory = new AtomicReference<>(new SecureCredentialsFactory(site)); - this.secureCredentialsFactoryLoadTs = getSecureConfigLastEditTs(); + this.secureCredentialsFactory = + new AtomicReference<>(newSecureCredentialsFactory(site, secureStore, config)); + if (config.useLegacyCredentials()) { + logger.atWarning().log( + "Using legacy credentials in clear text in secure.config. Please encrypt your credentials using " + + "'java -jar gerrit.war passwd' for each remote, remove the gerrit.useLegacyCredentials in replication.config " + + "and then reload the replication plugin."); + } } - private long getSecureConfigLastEditTs() { - if (!Files.exists(site.secure_config)) { - return 0L; + private static CredentialsFactory newSecureCredentialsFactory( + SitePaths site, SecureStore secureStore, ReplicationConfig config) + throws ConfigInvalidException, IOException { + if (config.useLegacyCredentials()) { + return new LegacyCredentialsFactory(site); } - return lastModified(site.secure_config); + return new SecureCredentialsFactory(secureStore); } @Override public CredentialsProvider create(String remoteName) { try { if (needsReload()) { + secureStore.reload(); secureCredentialsFactory.compareAndSet( - secureCredentialsFactory.get(), new SecureCredentialsFactory(site)); - secureCredentialsFactoryLoadTs = getSecureConfigLastEditTs(); + secureCredentialsFactory.get(), newSecureCredentialsFactory(site, secureStore, config)); logger.atInfo().log("secure.config reloaded as it was updated on the file system"); } } catch (Exception e) { @@ -69,7 +77,6 @@ } private boolean needsReload() { - return config.getConfig().getBoolean("gerrit", "autoReload", false) - && getSecureConfigLastEditTs() != secureCredentialsFactoryLoadTs; + return config.getConfig().getBoolean("gerrit", "autoReload", false) && secureStore.isOutdated(); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/LegacyCredentialsFactory.java b/src/main/java/com/googlesource/gerrit/plugins/replication/LegacyCredentialsFactory.java new file mode 100644 index 0000000..a89c936 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/LegacyCredentialsFactory.java
@@ -0,0 +1,66 @@ +// Copyright (C) 2011 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.plugins.replication; + +import com.google.gerrit.server.config.SitePaths; +import com.google.inject.Inject; +import java.io.IOException; +import java.util.Objects; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.transport.CredentialsProvider; +import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; +import org.eclipse.jgit.util.FS; + +/** + * Looks up a remote's password in secure.config. + * + * @deprecated This class is not secure and should no longer be used; it was allowing to record and + * read credentials in clear text in secure.config even though the Gerrit administrator + * installed a proper SecureStore implementation such as the secure-config.jar libModule. + */ +@Deprecated(forRemoval = true) +class LegacyCredentialsFactory implements CredentialsFactory { + private final Config config; + + @Inject + public LegacyCredentialsFactory(SitePaths site) throws ConfigInvalidException, IOException { + config = load(site); + } + + private static Config load(SitePaths site) throws ConfigInvalidException, IOException { + FileBasedConfig cfg = new FileBasedConfig(site.secure_config.toFile(), FS.DETECTED); + if (cfg.getFile().exists() && cfg.getFile().length() > 0) { + try { + cfg.load(); + } catch (ConfigInvalidException e) { + throw new ConfigInvalidException( + String.format("Config file %s is invalid: %s", cfg.getFile(), e.getMessage()), e); + } catch (IOException e) { + throw new IOException( + String.format("Cannot read %s: %s", cfg.getFile(), e.getMessage()), e); + } + } + return cfg; + } + + @Override + public CredentialsProvider create(String remoteName) { + String user = Objects.toString(config.getString("remote", remoteName, "username"), ""); + String pass = Objects.toString(config.getString("remote", remoteName, "password"), ""); + return new UsernamePasswordCredentialsProvider(user, pass); + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java index 1223db0..8f9b805 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java
@@ -31,6 +31,7 @@ private final SitePaths site; private final MergedConfigResource configResource; + private final boolean useLegacyCredentials; private boolean replicateAllOnPluginStart; private boolean defaultForceUpdate; private int maxRefsToLog; @@ -62,6 +63,7 @@ DEFAULT_SSH_CONNECTION_TIMEOUT_MS, MILLISECONDS); this.pluginDataDir = pluginDataDir; + this.useLegacyCredentials = config.getBoolean("gerrit", "useLegacyCredentials", false); } @Nullable @@ -125,6 +127,11 @@ } @Override + public boolean useLegacyCredentials() { + return useLegacyCredentials; + } + + @Override public String getVersion() { return configResource.getVersion(); }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/SecureCredentialsFactory.java b/src/main/java/com/googlesource/gerrit/plugins/replication/SecureCredentialsFactory.java index a4bd209..210c4d8 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/SecureCredentialsFactory.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/SecureCredentialsFactory.java
@@ -1,4 +1,4 @@ -// Copyright (C) 2011 The Android Open Source Project +// Copyright (C) 2024 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. @@ -14,46 +14,25 @@ package com.googlesource.gerrit.plugins.replication; -import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.securestore.SecureStore; import com.google.inject.Inject; -import java.io.IOException; import java.util.Objects; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; -import org.eclipse.jgit.util.FS; -/** Looks up a remote's password in secure.config. */ +/** Looks up a remote's password in SecureStore */ class SecureCredentialsFactory implements CredentialsFactory { - private final Config config; + private final SecureStore secureStore; @Inject - public SecureCredentialsFactory(SitePaths site) throws ConfigInvalidException, IOException { - config = load(site); - } - - private static Config load(SitePaths site) throws ConfigInvalidException, IOException { - FileBasedConfig cfg = new FileBasedConfig(site.secure_config.toFile(), FS.DETECTED); - if (cfg.getFile().exists() && cfg.getFile().length() > 0) { - try { - cfg.load(); - } catch (ConfigInvalidException e) { - throw new ConfigInvalidException( - String.format("Config file %s is invalid: %s", cfg.getFile(), e.getMessage()), e); - } catch (IOException e) { - throw new IOException( - String.format("Cannot read %s: %s", cfg.getFile(), e.getMessage()), e); - } - } - return cfg; + public SecureCredentialsFactory(SecureStore secureStore) { + this.secureStore = secureStore; } @Override public CredentialsProvider create(String remoteName) { - String user = Objects.toString(config.getString("remote", remoteName, "username"), ""); - String pass = Objects.toString(config.getString("remote", remoteName, "password"), ""); + String user = Objects.toString(secureStore.get("remote", remoteName, "username"), ""); + String pass = Objects.toString(secureStore.get("remote", remoteName, "password"), ""); return new UsernamePasswordCredentialsProvider(user, pass); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationConfig.java b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationConfig.java index 8ef4e2b..d5ccb02 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationConfig.java
@@ -102,4 +102,16 @@ * @return the config. */ Config getConfig(); + + /** + * Use legacy credentials for migrating existing sites. + * + * <p>Existing installations may have used a mix of encrypted and clear text credentials in + * secure.config, leveraging the replication plugin bug that was not accessing it using the + * correct API. The legacy feature flag 'gerrit.useLegacyCredentials' allows Gerrit to still use + * direct access to secure.config without decrypting its values. + * + * @return true if the secure.config should be read always directly and without decryption + */ + boolean useLegacyCredentials(); }
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 6f9a49d..bb4be9a 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -170,6 +170,14 @@ : Timeout for SSH connections. If 0, there is no timeout and the client waits indefinitely. By default, 2 minutes. +gerrit.useLegacyCredentials +: Use legacy credentials on secure.config which did not allow encryption + through a SecureStore provider. Legacy credentials should not be used in + production and are provided purely for backward compatibility with existing + secure.config files. When enabling the SecureStore in Gerrit, for instance, + the secure-config.jar libModule, make sure that all existing replication + credentials are encrypted. By default, false. + replication.distributionInterval : Interval in seconds for running the replication distributor. When run, the replication distributor will add all persisted waiting tasks
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java index b8bbc08..9244f87 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
@@ -78,7 +78,7 @@ } @Before - public void setup() { + public void setup() throws IOException { when(destinationFactoryMock.create(any(DestinationConfiguration.class))) .thenAnswer( new Answer<Destination>() { @@ -103,6 +103,10 @@ return newReplicationConfig("replication.config"); } + protected FileBasedConfig newSecureConfig() { + return newReplicationConfig("secure.config"); + } + protected FileBasedConfig newReplicationConfig(String path) { FileBasedConfig replicationConfig = new FileBasedConfig(sitePaths.etc_dir.resolve(path).toFile(), FS.DETECTED);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecoratorTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecoratorTest.java new file mode 100644 index 0000000..c959cc9 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecoratorTest.java
@@ -0,0 +1,97 @@ +// Copyright (C) 2024 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.plugins.replication; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.when; + +import com.google.gerrit.server.securestore.SecureStore; +import com.googlesource.gerrit.plugins.replication.api.ReplicationConfig; +import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.transport.CredentialItem; +import org.eclipse.jgit.transport.CredentialsProvider; +import org.eclipse.jgit.transport.URIish; +import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class AutoReloadSecureCredentialsFactoryDecoratorTest extends AbstractConfigTest { + private static final String REMOTE_TEST = "remote-test"; + private static final String USERNAME_CLEARTEXT = "username-clear"; + private static final String PASSWORD_CLEARTEXT = "password-clear"; + private static final String USERNAME_CIPHERTEXT = "username-encrypted"; + private static final String PASSWORD_CIPHERTEXT = "password-encrypted"; + + @Mock private SecureStore secureStoreMock; + @Mock private ReplicationConfig replicationConfigMock; + + public AutoReloadSecureCredentialsFactoryDecoratorTest() throws IOException { + super(); + } + + @Override + @Before + public void setup() throws IOException { + when(replicationConfigMock.getConfig()).thenReturn(new Config()); + + when(secureStoreMock.get("remote", REMOTE_TEST, "username")).thenReturn(USERNAME_CIPHERTEXT); + when(secureStoreMock.get("remote", REMOTE_TEST, "password")).thenReturn(PASSWORD_CIPHERTEXT); + + FileBasedConfig secureConfig = newSecureConfig(); + secureConfig.setString("remote", REMOTE_TEST, "username", USERNAME_CLEARTEXT); + secureConfig.setString("remote", REMOTE_TEST, "password", PASSWORD_CLEARTEXT); + secureConfig.save(); + } + + @Test + public void shouldCreateLegacyCredentials() throws Exception { + when(replicationConfigMock.useLegacyCredentials()).thenReturn(true); + assertUsernamePasswordCredentials( + getCredentialsProvider(), USERNAME_CLEARTEXT, PASSWORD_CLEARTEXT); + } + + @Test + public void shouldCreateEncryptedCredentialsByDefault() throws Exception { + assertUsernamePasswordCredentials( + getCredentialsProvider(), USERNAME_CIPHERTEXT, PASSWORD_CIPHERTEXT); + } + + private UsernamePasswordCredentialsProvider getCredentialsProvider() + throws ConfigInvalidException, IOException { + AutoReloadSecureCredentialsFactoryDecorator credFactory = + new AutoReloadSecureCredentialsFactoryDecorator( + sitePaths, secureStoreMock, replicationConfigMock); + CredentialsProvider legacyCredentials = credFactory.create(REMOTE_TEST); + assertThat(legacyCredentials).isInstanceOf(UsernamePasswordCredentialsProvider.class); + return (UsernamePasswordCredentialsProvider) legacyCredentials; + } + + private static void assertUsernamePasswordCredentials( + UsernamePasswordCredentialsProvider credentials, String username, String password) { + CredentialItem.Username usernameItem = new CredentialItem.Username(); + CredentialItem.Password passwordItem = new CredentialItem.Password(); + assertThat(credentials.get(new URIish(), usernameItem, passwordItem)).isTrue(); + + assertThat(usernameItem.getValue()).isEqualTo(username); + assertThat(new String(passwordItem.getValue())).isEqualTo(password); + } +}