Merge branch 'stable-3.10' * stable-3.10: PushOne: Remove unused addRef() method TasksStorage: Remove synchronized from methods Place the replaying flag clearing in a finally Demote delete errors when distributor is enabled distributor: Reduce log level for no-op consolidations Change-Id: I3ce1519c44ec6cbae7725527ea626e55401cc488 Release-Notes: skip

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 e9409a6..32c439b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java

@@ -14,49 +14,60 @@
 
 package com.googlesource.gerrit.plugins.replication;
 
-import static com.google.gerrit.common.FileUtil.lastModified;
-
 import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.UsedAt;
+import com.google.gerrit.common.UsedAt.Project;
 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;
 
+@UsedAt(Project.PLUGIN_PULL_REPLICATION)
 public 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 +80,11 @@
   }
 
   private boolean needsReload() {
-    return config.getConfig().getBoolean("gerrit", "autoReload", false)
-        && getSecureConfigLastEditTs() != secureCredentialsFactoryLoadTs;
+    return config.getConfig().getBoolean("gerrit", "autoReload", false) && secureStore.isOutdated();
+  }
+
+  @Override
+  public boolean validate(String remoteConfigName) {
+    return secureCredentialsFactory.get().validate(remoteConfigName);
   }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/CredentialsFactory.java b/src/main/java/com/googlesource/gerrit/plugins/replication/CredentialsFactory.java
index 3bb64ab..141fb2d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/CredentialsFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/CredentialsFactory.java

@@ -18,4 +18,8 @@
 public interface CredentialsFactory {
 
   CredentialsProvider create(String remoteName);
+
+  default boolean validate(String remoteConfigName) {
+    return true;
+  }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
index c186493..3bb1d2d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java

@@ -97,6 +97,8 @@
 
   private static final String PROJECT_NOT_AVAILABLE = "source project %s not available";
 
+  private final CredentialsFactory credentialsFactory;
+
   public interface Factory {
     Destination create(DestinationConfiguration config);
   }
@@ -149,6 +151,7 @@
       GroupIncludeCache groupIncludeCache,
       DynamicItem<EventDispatcher> eventDispatcher,
       Provider<ReplicationTasksStorage> rts,
+      CredentialsFactory credentialsFactory,
       @Assisted DestinationConfiguration cfg) {
     this.eventDispatcher = eventDispatcher;
     gitManager = gitRepositoryManager;
@@ -157,6 +160,7 @@
     this.projectCache = projectCache;
     this.stateLog = stateLog;
     this.replicationTasksStorage = rts;
+    this.credentialsFactory = credentialsFactory;
     config = cfg;
     CurrentUser remoteUser;
     if (!cfg.getAuthGroupNames().isEmpty()) {
@@ -216,6 +220,10 @@
     threadScoper = child.getInstance(PerThreadRequestScope.Scoper.class);
   }
 
+  public boolean validate() {
+    return (credentialsFactory == null || credentialsFactory.validate(getRemoteConfigName()));
+  }
+
   private void addRecursiveParents(
       AccountGroup.UUID g,
       ImmutableSet.Builder<AccountGroup.UUID> builder,

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
index 8a41945..dc3b05e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java

@@ -180,6 +180,12 @@
   public synchronized void startup(WorkQueue workQueue) {
     shuttingDown = false;
     for (Destination cfg : destinations) {
+      if (!cfg.validate()) {
+        throw new IllegalStateException(
+            "Unable to start replication plugin because remote "
+                + cfg.getRemoteConfigName()
+                + " is not valid");
+      }
       cfg.start(workQueue);
     }
   }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java
index 4220ddb..c634cce 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResource.java

@@ -17,17 +17,20 @@
 import static com.google.common.io.Files.getNameWithoutExtension;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
-import com.google.common.collect.Lists;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.hash.Hasher;
 import com.google.common.hash.Hashing;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.inject.Inject;
+import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.Optional;
+import java.util.List;
 import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.file.FileSnapshot;
@@ -40,6 +43,7 @@
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final Path remoteConfigsDirPath;
+  private final Config fanoutConfig;
 
   @Inject
   FanoutConfigResource(SitePaths site) throws IOException, ConfigInvalidException {
@@ -48,16 +52,59 @@
     removeRemotes(config);
 
     try (Stream<Path> files = Files.list(remoteConfigsDirPath)) {
-      files
-          .filter(Files::isRegularFile)
-          .filter(FanoutConfigResource::isConfig)
-          .map(FanoutConfigResource::loadConfig)
-          .filter(Optional::isPresent)
-          .map(Optional::get)
-          .filter(FanoutConfigResource::isValid)
-          .forEach(cfg -> addRemoteConfig(cfg, config));
-    } catch (IllegalStateException e) {
-      throw new ConfigInvalidException(e.getMessage());
+      Stream<String> mergedConfigs =
+          files
+              .filter(Files::isRegularFile)
+              .filter(FanoutConfigResource::isConfig)
+              .flatMap(path -> getConfigLines(path, line -> addRemoteSubsection(line, path)));
+
+      fanoutConfig = new Config(config);
+      fanoutConfig.fromText(mergedConfigs.collect(Collectors.joining("\n")));
+    }
+  }
+
+  private static String addRemoteSubsection(String line, Path path) {
+    return line.contains("[remote]")
+        ? line.replace(
+            "[remote]", "[remote \"" + getNameWithoutExtension(path.toFile().getName()) + "\"]")
+        : line;
+  }
+
+  private static Stream<String> getConfigLines(
+      Path path, Function<String, String> configLineMapping) {
+    try {
+      List<String> configLines = Files.readAllLines(path);
+      if (!isValid(path, configLines)) {
+        return Stream.empty();
+      }
+      return configLines.stream().map(configLineMapping);
+    } catch (IOException e) {
+      logger.atSevere().withCause(e).log("Unable to access replication config %s", path);
+      return Stream.empty();
+    }
+  }
+
+  @Override
+  public void update(Config updates) throws IOException {
+    super.update(filterOutRemotes(updates));
+
+    Set<String> remotes = updates.getSubsections("remote");
+    for (String remote : remotes) {
+      File remoteFile = remoteConfigsDirPath.resolve(remote + ".config").toFile();
+      FileBasedConfig remoteConfig = new FileBasedConfig(remoteFile, FS.DETECTED);
+      try {
+        remoteConfig.load();
+      } catch (ConfigInvalidException e) {
+        throw new IOException(
+            String.format("Cannot parse configuration file: %s", remoteFile.getAbsoluteFile()), e);
+      }
+      Set<String> options = updates.getNames("remote", remote);
+      for (String option : options) {
+        List<String> values = List.of(updates.getStringList("remote", remote, option));
+        remoteConfig.setStringList("remote", null, option, values);
+        config.setStringList("remote", remote, option, values);
+      }
+      remoteConfig.save();
     }
   }
 
@@ -74,42 +121,67 @@
     }
   }
 
-  private static void addRemoteConfig(FileBasedConfig source, Config destination) {
-    String remoteName = getNameWithoutExtension(source.getFile().getName());
-    for (String name : source.getNames("remote")) {
-      destination.setStringList(
-          "remote",
-          remoteName,
-          name,
-          Lists.newArrayList(source.getStringList("remote", null, name)));
-    }
+  private static Config filterOutRemotes(Config config) {
+    Config filteredConfig = new Config();
+    Set<String> sections = config.getSections();
+    sections.stream()
+        .filter(Predicate.not("remote"::equalsIgnoreCase))
+        .forEach(
+            section -> {
+              config
+                  .getNames(section)
+                  .forEach(
+                      sectionName ->
+                          filteredConfig.setStringList(
+                              section,
+                              null,
+                              sectionName,
+                              List.of(config.getStringList(section, null, sectionName))));
+              config
+                  .getSubsections(section)
+                  .forEach(
+                      subsection ->
+                          config
+                              .getNames(section, subsection)
+                              .forEach(
+                                  name ->
+                                      filteredConfig.setStringList(
+                                          section,
+                                          subsection,
+                                          name,
+                                          List.of(
+                                              config.getStringList(section, subsection, name)))));
+            });
+    return filteredConfig;
   }
 
-  private static boolean isValid(Config cfg) {
-    if (cfg.getSections().size() != 1 || !cfg.getSections().contains("remote")) {
-      logger.atSevere().log(
-          "Remote replication configuration file %s must contain only one remote section.", cfg);
-      return false;
-    }
-    if (cfg.getSubsections("remote").size() > 0) {
-      logger.atSevere().log(
-          "Remote replication configuration file %s cannot contain remote subsections.", cfg);
-      return false;
+  private static boolean isValid(Path path, List<String> remoteConfigLines) {
+    int numRemoteSections = 0;
+    int numRemoteSectionsWithName = 0;
+    boolean valid = true;
+    for (String configLine : remoteConfigLines) {
+      if (configLine.contains("[remote]")) {
+        numRemoteSections++;
+      }
+
+      if (configLine.contains("[remote \"")) {
+        numRemoteSectionsWithName++;
+      }
     }
 
-    return true;
-  }
-
-  private static Optional<FileBasedConfig> loadConfig(Path path) {
-    FileBasedConfig cfg = new FileBasedConfig(path.toFile(), FS.DETECTED);
-    try {
-      cfg.load();
-    } catch (IOException | ConfigInvalidException e) {
-      logger.atSevere().withCause(e).log(
-          "Cannot load remote replication configuration file %s.", path);
-      return Optional.empty();
+    if (numRemoteSectionsWithName > 0) {
+      logger.atSevere().log(
+          "Remote replication configuration file %s cannot contain remote subsections.", path);
+      valid = false;
     }
-    return Optional.of(cfg);
+
+    if (numRemoteSections != 1) {
+      logger.atSevere().log(
+          "Remote replication configuration file %s must contain only one remote section.", path);
+      valid = false;
+    }
+
+    return valid;
   }
 
   private static boolean isConfig(Path p) {
@@ -140,4 +212,9 @@
       return parentVersion;
     }
   }
+
+  @Override
+  public Config getConfig() {
+    return fanoutConfig;
+  }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/FileConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/FileConfigResource.java
index baccb83..7a14cd1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/FileConfigResource.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/FileConfigResource.java

@@ -17,6 +17,7 @@
 import static com.googlesource.gerrit.plugins.replication.ReplicationQueue.repLog;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.Lists;
 import com.google.gerrit.common.UsedAt;
 import com.google.gerrit.common.UsedAt.Project;
 import com.google.gerrit.server.config.SitePaths;
@@ -24,6 +25,7 @@
 import com.googlesource.gerrit.plugins.replication.api.ConfigResource;
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.List;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
@@ -54,6 +56,25 @@
   }
 
   @Override
+  public void update(Config updates) throws IOException {
+    for (String section : updates.getSections()) {
+      for (String subsection : updates.getSubsections(section)) {
+        for (String name : updates.getNames(section, subsection, true)) {
+          List<String> values =
+              Lists.newArrayList(updates.getStringList(section, subsection, name));
+          config.setStringList(section, subsection, name, values);
+        }
+      }
+
+      for (String name : updates.getNames(section, true)) {
+        List<String> values = Lists.newArrayList(updates.getStringList(section, null, name));
+        config.setStringList(section, null, name, values);
+      }
+    }
+    config.save();
+  }
+
+  @Override
   public String getVersion() {
     return Long.toString(config.getFile().lastModified());
   }

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/MergedConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java
index 43d90a5..58a10f0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java

@@ -26,6 +26,7 @@
 import com.google.inject.util.Providers;
 import com.googlesource.gerrit.plugins.replication.api.ConfigResource;
 import com.googlesource.gerrit.plugins.replication.api.ReplicationConfigOverrides;
+import java.io.IOException;
 import java.util.function.Supplier;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
@@ -77,7 +78,15 @@
     return baseVersion + overrides.get().getVersion();
   }
 
-  private boolean noOverrides() {
+  boolean noOverrides() {
     return overrides == null || overrides.get() == null;
   }
+
+  void update(Config remotesConfig) throws IOException {
+    if (noOverrides()) {
+      base.get().update(remotesConfig);
+    } else {
+      overrides.get().update(remotesConfig);
+    }
+  }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
index 2b60145..a648845 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java

@@ -79,7 +79,6 @@
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.transport.CredentialsProvider;
 import org.eclipse.jgit.transport.FetchConnection;
 import org.eclipse.jgit.transport.PushResult;
 import org.eclipse.jgit.transport.RefSpec;
@@ -119,7 +118,7 @@
   private final Destination pool;
   private final RemoteConfig config;
   private final ReplicationConfig replConfig;
-  private final CredentialsProvider credentialsProvider;
+  private final CredentialsFactory credentialsFactory;
   private final PerThreadRequestScope.Scoper threadScoper;
 
   private final Project.NameKey projectName;
@@ -166,7 +165,7 @@
     pool = p;
     config = c;
     replConfig = rc;
-    credentialsProvider = cpFactory.create(c.getName());
+    credentialsFactory = cpFactory;
     threadScoper = ts;
     projectName = d;
     uri = u;
@@ -563,7 +562,7 @@
 
   private PushResult pushVia(Transport tn) throws IOException, PermissionBackendException {
     tn.applyConfig(config);
-    tn.setCredentialsProvider(credentialsProvider);
+    tn.setCredentialsProvider(credentialsFactory.create(config.getName()));
 
     List<RemoteRefUpdate> todo = generateUpdates(tn);
     if (todo.isEmpty()) {

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/ReplicationModule.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
index ac27280..e3e4021 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationModule.java

@@ -23,6 +23,7 @@
 import com.google.gerrit.extensions.events.HeadUpdatedListener;
 import com.google.gerrit.extensions.events.LifecycleListener;
 import com.google.gerrit.extensions.events.ProjectDeletedListener;
+import com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.server.events.EventTypes;
 import com.google.inject.AbstractModule;
@@ -30,6 +31,7 @@
 import com.google.inject.Scopes;
 import com.google.inject.assistedinject.FactoryModuleBuilder;
 import com.google.inject.internal.UniqueAnnotations;
+import com.googlesource.gerrit.plugins.replication.api.ReplicationRemotesApi;
 import com.googlesource.gerrit.plugins.replication.events.ProjectDeletionReplicationDoneEvent;
 import com.googlesource.gerrit.plugins.replication.events.ProjectDeletionReplicationFailedEvent;
 import com.googlesource.gerrit.plugins.replication.events.ProjectDeletionReplicationScheduledEvent;
@@ -52,9 +54,7 @@
 
   @Override
   protected void configure() {
-    install(new FactoryModuleBuilder().build(Destination.Factory.class));
     install(configModule);
-    bind(ReplicationQueue.class).in(Scopes.SINGLETON);
     bind(ObservableQueue.class).to(ReplicationQueue.class);
     bind(LifecycleListener.class)
         .annotatedWith(UniqueAnnotations.create())
@@ -63,6 +63,7 @@
     DynamicSet.bind(binder(), GitBatchRefUpdateListener.class).to(ReplicationQueue.class);
     DynamicSet.bind(binder(), ProjectDeletedListener.class).to(ReplicationQueue.class);
     DynamicSet.bind(binder(), HeadUpdatedListener.class).to(ReplicationQueue.class);
+    DynamicItem.bind(binder(), ReplicationRemotesApi.class).to(ReplicationRemotesApiImpl.class);
 
     bind(OnStartStop.class).in(Scopes.SINGLETON);
     bind(LifecycleListener.class).annotatedWith(UniqueAnnotations.create()).to(OnStartStop.class);
@@ -77,10 +78,8 @@
         .to(StartReplicationCapability.class);
 
     install(new FactoryModuleBuilder().build(PushAll.Factory.class));
-    install(new FactoryModuleBuilder().build(ProjectDeletionState.Factory.class));
 
     bind(EventBus.class).in(Scopes.SINGLETON);
-    bind(ReplicationDestinations.class).to(DestinationsCollection.class);
     bind(ConfigParser.class).to(DestinationConfigParser.class).in(Scopes.SINGLETON);
 
     DynamicSet.setOf(binder(), ReplicationStateListener.class);
@@ -103,5 +102,11 @@
 
     bind(TransportFactory.class).to(TransportFactoryImpl.class).in(Scopes.SINGLETON);
     bind(CloseableHttpClient.class).toProvider(HttpClientProvider.class).in(Scopes.SINGLETON);
+
+    bind(ReplicationQueue.class).in(Scopes.SINGLETON);
+    bind(ReplicationDestinations.class).to(DestinationsCollection.class);
+
+    install(new FactoryModuleBuilder().build(Destination.Factory.class));
+    install(new FactoryModuleBuilder().build(ProjectDeletionState.Factory.class));
   }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java
new file mode 100644
index 0000000..cc6bcad
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java

@@ -0,0 +1,100 @@
+// 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 com.google.gerrit.server.securestore.SecureStore;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.googlesource.gerrit.plugins.replication.api.ReplicationRemotesApi;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import org.eclipse.jgit.lib.Config;
+
+@Singleton
+class ReplicationRemotesApiImpl implements ReplicationRemotesApi {
+
+  private final SecureStore secureStore;
+  private final MergedConfigResource mergedConfigResource;
+
+  @Inject
+  ReplicationRemotesApiImpl(SecureStore secureStore, MergedConfigResource mergedConfigResource) {
+    this.secureStore = secureStore;
+    this.mergedConfigResource = mergedConfigResource;
+  }
+
+  @Override
+  public Config get(String... remoteNames) {
+    Config replicationConfig = mergedConfigResource.getConfig();
+    Config remoteConfig = new Config();
+    for (String remoteName : remoteNames) {
+      Set<String> configNames = replicationConfig.getNames("remote", remoteName);
+      for (String configName : configNames) {
+        String[] values = replicationConfig.getStringList("remote", remoteName, configName);
+        if (values.length > 0) {
+          remoteConfig.setStringList("remote", remoteName, configName, Arrays.asList(values));
+        }
+      }
+    }
+    return remoteConfig;
+  }
+
+  @Override
+  public void update(Config remoteConfig) throws IOException {
+    if (remoteConfig.getSubsections("remote").isEmpty()) {
+      throw new IllegalArgumentException(
+          "configuration update must have at least one 'remote' section");
+    }
+
+    SeparatedRemoteConfigs configs = onlyRemoteSectionsWithSeparatedCredentials(remoteConfig);
+    persistRemotesCredentials(configs);
+
+    mergedConfigResource.update(configs.remotes);
+  }
+
+  private SeparatedRemoteConfigs onlyRemoteSectionsWithSeparatedCredentials(Config configUpdates) {
+    SeparatedRemoteConfigs configs = new SeparatedRemoteConfigs();
+    for (String subSection : configUpdates.getSubsections("remote")) {
+      for (String name : configUpdates.getNames("remote", subSection)) {
+        List<String> values = List.of(configUpdates.getStringList("remote", subSection, name));
+        if ("password".equals(name) || "username".equals(name)) {
+          configs.credentials.setStringList("remote", subSection, name, values);
+        } else {
+          configs.remotes.setStringList("remote", subSection, name, values);
+        }
+      }
+    }
+
+    return configs;
+  }
+
+  private void persistRemotesCredentials(SeparatedRemoteConfigs configs) {
+    for (String subSection : configs.credentials.getSubsections("remote")) {
+      copyRemoteStringList(configs.credentials, subSection, "username");
+      copyRemoteStringList(configs.credentials, subSection, "password");
+    }
+  }
+
+  private void copyRemoteStringList(Config config, String subSection, String key) {
+    secureStore.setList(
+        "remote", subSection, key, List.of(config.getStringList("remote", subSection, key)));
+  }
+
+  private static class SeparatedRemoteConfigs {
+    private final Config remotes = new Config();
+    private final Config credentials = new Config();
+  }
+}

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 ed15b92..3e00089 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,43 @@
 
 package com.googlesource.gerrit.plugins.replication;
 
-import com.google.gerrit.server.config.SitePaths;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.UsedAt;
+import com.google.gerrit.common.UsedAt.Project;
+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 */
+@UsedAt(Project.PLUGIN_PULL_REPLICATION)
 public class SecureCredentialsFactory implements CredentialsFactory {
-  private final Config config;
+  private static final FluentLogger log = FluentLogger.forEnclosingClass();
+  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);
   }
+
+  @Override
+  public boolean validate(String remoteName) {
+    try {
+      String unused = secureStore.get("remote", remoteName, "username");
+      unused = secureStore.get("remote", remoteName, "password");
+      return true;
+    } catch (Throwable t) {
+      log.atSevere().withCause(t).log(
+          "Credentials for replication remote %s are invalid", remoteName);
+      return false;
+    }
+  }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ApiModule.java b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ApiModule.java
index 3764ede..44ec88c 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ApiModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ApiModule.java

@@ -22,5 +22,6 @@
   protected void configure() {
     DynamicItem.itemOf(binder(), ReplicationPushFilter.class);
     DynamicItem.itemOf(binder(), ReplicationConfigOverrides.class);
+    DynamicItem.itemOf(binder(), ReplicationRemotesApi.class);
   }
 }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java
index 43733bc..0052fc2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java

@@ -14,6 +14,7 @@
 
 package com.googlesource.gerrit.plugins.replication.api;
 
+import java.io.IOException;
 import org.eclipse.jgit.lib.Config;
 
 /**
@@ -37,6 +38,16 @@
   Config getConfig();
 
   /**
+   * Update the configuration resource.
+   *
+   * <p>Allows to persist changes to the configuration resource.
+   *
+   * @param config updated configuration
+   * @throws IOException when configuration cannot be persisted
+   */
+  void update(Config config) throws IOException;
+
+  /**
    * Current logical version string of the current configuration on the persistent storage.
    *
    * @return latest logical version number on the persistent storage

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/java/com/googlesource/gerrit/plugins/replication/api/ReplicationRemotesApi.java b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationRemotesApi.java
new file mode 100644
index 0000000..76d6d35
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationRemotesApi.java

@@ -0,0 +1,55 @@
+// 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.api;
+
+import static com.google.gerrit.common.UsedAt.Project.PLUGIN_GITHUB;
+
+import com.google.gerrit.common.UsedAt;
+import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.server.securestore.SecureStore;
+import java.io.IOException;
+import org.eclipse.jgit.lib.Config;
+
+/** Public API to update replication plugin remotes configurations programmatically. */
+@UsedAt(PLUGIN_GITHUB)
+@DynamicItem.Final(implementedByPlugin = "replication")
+public interface ReplicationRemotesApi {
+
+  /**
+   * Retrieves the configuration for a remote by name.
+   *
+   * <p>Builds a JGit {@link Config} with the remote's configuration obtained by parsing the
+   * replication configuration sources.
+   *
+   * <p>NOTE: The remotes secrets are excluded for security reasons.
+   *
+   * @param remoteNames the remote names to retrieve.
+   * @return {@link Config} associated with the remoteName(s)
+   */
+  Config get(String... remoteNames);
+
+  /**
+   * Adds or updates the remote configuration for the replication plugin.
+   *
+   * <p>Provided JGit {@link Config} object should contain at least one named <em>remote</em>
+   * section. All other configurations will be ignored.
+   *
+   * <p>NOTE: The {@code remote.$name.password} will be stored using {@link SecureStore}.
+   *
+   * @param remoteConfig remotes to add or update
+   * @throws IOException when persisting fails
+   */
+  void update(Config remoteConfig) throws IOException;
+}

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..935fdb4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java

@@ -57,7 +57,8 @@
     public final DestinationConfiguration config;
 
     protected FakeDestination(DestinationConfiguration config) {
-      super(injectorMock(), null, null, null, null, null, null, null, null, null, null, config);
+      super(
+          injectorMock(), null, null, null, null, null, null, null, null, null, null, null, config);
       this.config = config;
     }
 
@@ -78,7 +79,7 @@
   }
 
   @Before
-  public void setup() {
+  public void setup() throws IOException {
     when(destinationFactoryMock.create(any(DestinationConfiguration.class)))
         .thenAnswer(
             new Answer<Destination>() {
@@ -103,6 +104,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);
+  }
+}

diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResourceTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResourceTest.java
index 9147ea1..a3d811f 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResourceTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResourceTest.java

@@ -24,6 +24,7 @@
 import java.io.IOException;
 import java.util.List;
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.util.FS;
 import org.junit.Before;
@@ -285,9 +286,73 @@
     assertThat(objectUnderTest.getVersion()).isEqualTo(replicationConfigVersion);
   }
 
+  @Test
+  public void shouldAddConfigOptionToMainConfig() throws Exception {
+    FanoutConfigResource objectUnderTest = new FanoutConfigResource(sitePaths);
+    Config update = new Config();
+    update.setString("new", null, "value", "set");
+
+    objectUnderTest.update(update);
+    Config updatedConfig = objectUnderTest.getConfig();
+
+    assertThat(updatedConfig.getString("new", null, "value")).isEqualTo("set");
+  }
+
+  @Test
+  public void shouldUpdateConfigOptionInMainConfig() throws Exception {
+    FileBasedConfig config = newReplicationConfig();
+    config.setString("updatable", null, "value", "orig");
+    config.save();
+    FanoutConfigResource objectUnderTest = new FanoutConfigResource(sitePaths);
+    Config update = new Config();
+    update.setString("updatable", null, "value", "updated");
+
+    objectUnderTest.update(update);
+    Config updatedConfig = objectUnderTest.getConfig();
+
+    assertThat(updatedConfig.getString("updatable", null, "value")).isEqualTo("updated");
+  }
+
+  @Test
+  public void shouldAddNewRemoteFile() throws Exception {
+    FanoutConfigResource objectUnderTest = new FanoutConfigResource(sitePaths);
+    Config update = new Config();
+    update.setString("remote", remoteName1, "url", remoteUrl1);
+
+    objectUnderTest.update(update);
+
+    assertThat(objectUnderTest.getConfig().getString("remote", remoteName1, "url"))
+        .isEqualTo(remoteUrl1);
+    Config actual = loadRemoteConfig(remoteName1);
+    assertThat(actual.getString("remote", null, "url")).isEqualTo(remoteUrl1);
+  }
+
+  @Test
+  public void shouldUpdateExistingRemote() throws Exception {
+    FileBasedConfig rawRemoteConfig = newRemoteConfig(remoteName1);
+    rawRemoteConfig.setString("remote", remoteName1, "url", remoteUrl1);
+    rawRemoteConfig.save();
+    FanoutConfigResource objectUnderTest = new FanoutConfigResource(sitePaths);
+    Config update = new Config();
+    update.setString("remote", remoteName1, "url", remoteUrl2);
+
+    objectUnderTest.update(update);
+
+    assertThat(objectUnderTest.getConfig().getString("remote", remoteName1, "url"))
+        .isEqualTo(remoteUrl2);
+    Config actual = loadRemoteConfig(remoteName1);
+    assertThat(actual.getString("remote", null, "url")).isEqualTo(remoteUrl2);
+  }
+
   protected FileBasedConfig newRemoteConfig(String configFileName) {
     return new FileBasedConfig(
         sitePaths.etc_dir.resolve("replication/" + configFileName + ".config").toFile(),
         FS.DETECTED);
   }
+
+  private Config loadRemoteConfig(String siteName) throws Exception {
+    FileBasedConfig config = newRemoteConfig(siteName);
+    config.load();
+    return config;
+  }
 }

diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/FileConfigResourceTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/FileConfigResourceTest.java
new file mode 100644
index 0000000..92bf58d
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/FileConfigResourceTest.java

@@ -0,0 +1,121 @@
+// 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.io.RecursiveDeleteOption.ALLOW_INSECURE;
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.io.MoreFiles;
+import com.google.gerrit.server.config.SitePaths;
+import com.googlesource.gerrit.plugins.replication.api.ConfigResource;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.util.FS;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class FileConfigResourceTest {
+  private static final String VALUE_KEY = "value";
+  private static final String INITIAL_KEY = "initial";
+  private static final String UPDATABLE_KEY = "updatable";
+
+  private Path testDir;
+  private SitePaths sitePaths;
+  private ConfigResource configResource;
+
+  @Before
+  public void setUp() throws Exception {
+    testDir = Files.createTempDirectory("fileConfigResourceTest");
+    sitePaths = new SitePaths(testDir);
+    configResource = newFileConfigResource();
+  }
+
+  @After
+  public void tearDown() throws Exception {
+    MoreFiles.deleteRecursively(testDir, ALLOW_INSECURE);
+  }
+
+  @Test
+  public void updateEmptyFile() throws Exception {
+    Config configUpdate = newConfigUpdate();
+
+    Config beforeUpdate = configResource.getConfig();
+    assertThat(beforeUpdate.getSections()).isEmpty();
+
+    configResource.update(configUpdate);
+    Config updatedConfig = newFileConfigResource().getConfig();
+
+    assertConfigUpdate(updatedConfig);
+  }
+
+  @Test
+  public void appendOptionToConfig() throws Exception {
+    FileBasedConfig rawConfig = getRawReplicationConfig();
+    rawConfig.setInt(INITIAL_KEY, null, VALUE_KEY, 10);
+    rawConfig.save();
+    configResource = newFileConfigResource();
+    Config configUpdate = newConfigUpdate();
+
+    configResource.update(configUpdate);
+    Config updatedConfig = configResource.getConfig();
+
+    assertConfigUpdate(updatedConfig);
+    assertThat(updatedConfig.getInt(INITIAL_KEY, null, VALUE_KEY, -1)).isEqualTo(10);
+  }
+
+  @Test
+  public void updateExistingOption() throws Exception {
+    int expectedValue = 20;
+    Config configUpdate = new Config();
+    configUpdate.setInt(UPDATABLE_KEY, null, VALUE_KEY, expectedValue);
+    FileBasedConfig rawConfig = getRawReplicationConfig(newConfigUpdate());
+    rawConfig.save();
+
+    configResource.update(configUpdate);
+    Config updatedConfig = configResource.getConfig();
+
+    assertConfigUpdate(updatedConfig, expectedValue);
+  }
+
+  private FileConfigResource newFileConfigResource() {
+    return new FileConfigResource(sitePaths);
+  }
+
+  private Config newConfigUpdate() {
+    Config configUpdate = new Config();
+    configUpdate.setInt(UPDATABLE_KEY, null, VALUE_KEY, 1);
+    return configUpdate;
+  }
+
+  private void assertConfigUpdate(Config config) {
+    assertConfigUpdate(config, 1);
+  }
+
+  private void assertConfigUpdate(Config config, int expectedValue) {
+    assertThat(config.getInt(UPDATABLE_KEY, null, VALUE_KEY, -1)).isEqualTo(expectedValue);
+  }
+
+  private FileBasedConfig getRawReplicationConfig() {
+    return getRawReplicationConfig(new Config());
+  }
+
+  private FileBasedConfig getRawReplicationConfig(Config base) {
+    Path configPath = sitePaths.etc_dir.resolve(FileConfigResource.CONFIG_NAME);
+    return new FileBasedConfig(base, configPath.toFile(), FS.DETECTED);
+  }
+}

diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/MergedConfigResourceTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/MergedConfigResourceTest.java
index 18bb603..cae78e3 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/MergedConfigResourceTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/MergedConfigResourceTest.java

@@ -22,6 +22,7 @@
 import com.googlesource.gerrit.plugins.replication.api.ApiModule;
 import com.googlesource.gerrit.plugins.replication.api.ConfigResource;
 import com.googlesource.gerrit.plugins.replication.api.ReplicationConfigOverrides;
+import org.apache.commons.lang3.NotImplementedException;
 import org.eclipse.jgit.lib.Config;
 import org.junit.Test;
 
@@ -78,6 +79,11 @@
     }
 
     @Override
+    public void update(Config config) {
+      throw new NotImplementedException("not implemented");
+    }
+
+    @Override
     public String getVersion() {
       return "base";
     }
@@ -93,6 +99,11 @@
     }
 
     @Override
+    public void update(Config config) {
+      throw new NotImplementedException("not implemented");
+    }
+
+    @Override
     public String getVersion() {
       return "override";
     }

diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java
new file mode 100644
index 0000000..40d9846
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java

@@ -0,0 +1,212 @@
+// 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.io.RecursiveDeleteOption.ALLOW_INSECURE;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+import com.google.common.io.MoreFiles;
+import com.google.common.truth.StringSubject;
+import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.securestore.SecureStore;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.googlesource.gerrit.plugins.replication.api.ConfigResource;
+import com.googlesource.gerrit.plugins.replication.api.ReplicationConfigOverrides;
+import com.googlesource.gerrit.plugins.replication.api.ReplicationRemotesApi;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+import org.eclipse.jgit.lib.Config;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ReplicationRemotesApiTest {
+
+  private Path testSite;
+  private SecureStore secureStoreMock;
+  private FileConfigResource baseConfig;
+  private Injector testInjector;
+  private AtomicReference<ReplicationConfigOverrides> testOverrides;
+  private String url1;
+  private String remoteName1;
+  private String url2;
+  private String remoteName2;
+
+  @Before
+  public void setUp() throws Exception {
+    testSite = Files.createTempDirectory("replicationRemotesUpdateTest");
+    secureStoreMock = mock(SecureStore.class);
+    baseConfig = new FileConfigResource(new SitePaths(testSite));
+    testOverrides = new AtomicReference<>(new TestReplicationConfigOverrides());
+    testInjector =
+        Guice.createInjector(
+            new AbstractModule() {
+              @Override
+              protected void configure() {
+                bind(ConfigResource.class).toInstance(baseConfig);
+                bind(SecureStore.class).toInstance(secureStoreMock);
+                bind(ReplicationRemotesApi.class).to(ReplicationRemotesApiImpl.class);
+                DynamicItem.itemOf(binder(), ReplicationConfigOverrides.class);
+                DynamicItem.bind(binder(), ReplicationConfigOverrides.class)
+                    .toProvider(testOverrides::get);
+              }
+            });
+    url1 = "fake_url1";
+    remoteName1 = "site1";
+    url2 = "fake_url2";
+    remoteName2 = "site2";
+  }
+
+  @After
+  public void tearDown() throws Exception {
+    MoreFiles.deleteRecursively(testSite, ALLOW_INSECURE);
+  }
+
+  @Test
+  public void shouldThrowWhenNoRemotesInTheUpdate() {
+    Config update = new Config();
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+    assertThrows(IllegalArgumentException.class, () -> objectUnderTest.update(update));
+
+    update.setString("non-remote", null, "value", "one");
+
+    assertThrows(IllegalArgumentException.class, () -> objectUnderTest.update(update));
+  }
+
+  @Test
+  public void shouldReturnEmptyConfigWhenNoRemotes() {
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+    assertThat(objectUnderTest.get("site").getSections()).isEmpty();
+  }
+
+  @Test
+  public void addRemoteSectionsToBaseConfigWhenNoOverrides() throws Exception {
+    testOverrides.set(null);
+
+    Config update = new Config();
+    setRemoteSite(update, remoteName1, "url", url1);
+    setRemoteSite(update, remoteName2, "url", url2);
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+    objectUnderTest.update(update);
+
+    assertRemoteSite(baseConfig.getConfig(), remoteName1, "url").isEqualTo(url1);
+    assertRemoteSite(baseConfig.getConfig(), remoteName2, "url").isEqualTo(url2);
+  }
+
+  @Test
+  public void shouldReturnRemotesFromBaseConfigWhenNoOverrides() {
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+    baseConfig.getConfig().setString("remote", remoteName1, "url", url1);
+    baseConfig.getConfig().setString("remote", remoteName2, "url", url2);
+
+    Config remotesConfig = objectUnderTest.get(remoteName1, remoteName2);
+    assertRemoteSite(remotesConfig, remoteName1, "url").isEqualTo(url1);
+    assertRemoteSite(remotesConfig, remoteName2, "url").isEqualTo(url2);
+  }
+
+  @Test
+  public void addRemotesSectionToBaseOverridesConfig() throws Exception {
+    Config update = new Config();
+    setRemoteSite(update, remoteName1, "url", url1);
+    setRemoteSite(update, remoteName2, "url", url2);
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+    objectUnderTest.update(update);
+
+    assertRemoteSite(testOverrides.get().getConfig(), remoteName1, "url").isEqualTo(url1);
+    assertRemoteSite(testOverrides.get().getConfig(), remoteName2, "url").isEqualTo(url2);
+    assertRemoteSite(baseConfig.getConfig(), remoteName1, "url").isNull();
+    assertRemoteSite(baseConfig.getConfig(), remoteName2, "url").isNull();
+
+    Config remotesConfig = objectUnderTest.get(remoteName1, remoteName2);
+    assertRemoteSite(remotesConfig, remoteName1, "url").isEqualTo(url1);
+    assertRemoteSite(remotesConfig, remoteName2, "url").isEqualTo(url2);
+  }
+
+  @Test
+  public void shouldSetPasswordViaSecureStoreButNotStoreInConfig() throws Exception {
+    Config update = new Config();
+    String password = "encrypted-password";
+    String remoteName = "site";
+    setRemoteSite(update, remoteName, "password", password);
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+    objectUnderTest.update(update);
+
+    verify(secureStoreMock).setList("remote", remoteName, "password", List.of(password));
+    assertRemoteSite(baseConfig.getConfig(), remoteName, "password").isNull();
+    assertRemoteSite(testOverrides.get().getConfig(), remoteName, "password").isNull();
+    assertRemoteSite(objectUnderTest.get(remoteName), remoteName, "password").isNull();
+  }
+
+  @Test
+  public void shouldSetUsernameViaSecureStoreButNotStoreInConfig() throws Exception {
+    Config update = new Config();
+    String username = "encrypted-username";
+    String remoteName = "site";
+    setRemoteSite(update, remoteName, "username", username);
+    ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+    objectUnderTest.update(update);
+
+    verify(secureStoreMock).setList("remote", remoteName, "username", List.of(username));
+    assertRemoteSite(baseConfig.getConfig(), remoteName, "username").isNull();
+    assertRemoteSite(testOverrides.get().getConfig(), remoteName, "username").isNull();
+    assertRemoteSite(objectUnderTest.get(remoteName), remoteName, "username").isNull();
+  }
+
+  private void setRemoteSite(Config config, String remoteName, String name, String value) {
+    config.setString("remote", remoteName, name, value);
+  }
+
+  private StringSubject assertRemoteSite(Config config, String remoteName, String name) {
+    return assertThat(config.getString("remote", remoteName, name));
+  }
+
+  private ReplicationRemotesApi getReplicationRemotesApi() {
+    return testInjector.getInstance(ReplicationRemotesApi.class);
+  }
+
+  static class TestReplicationConfigOverrides implements ReplicationConfigOverrides {
+    private Config config = new Config();
+
+    @Override
+    public Config getConfig() {
+      return config;
+    }
+
+    @Override
+    public void update(Config update) throws IOException {
+      config = update;
+    }
+
+    @Override
+    public String getVersion() {
+      return "none";
+    }
+  }
+}