Merge changes from topic 'signed-push-cleanup'

* changes:
  Move REFS_GPG_KEYS to PublicKeyStore
  config-gerrit.txt: Organize "receive" section
  PushCertificateChecker: Don't throw exceptions
  PushCertificateChecker: Tweak some error messages

Documentation/config-gerrit.txt

@@ -2863,28 +2863,16 @@ Default is 5 seconds. Negative values will be converted to 0.
 [[receive]]
 === Section receive
 
-This section is used to set who can execute the 'receive-pack' and
-to limit the maximum Git object size that 'receive-pack' will accept.
-'receive-pack' is what runs on the server during a user's push or
-repo upload command. It also contains some advanced options for tuning the
-behavior of Gerrit's 'receive-pack' mechanism.
+This section is used to configure behavior of the 'receive-pack'
+handler, which responds to 'git push' requests.
 
-----
-[receive]
-  allowGroup = GROUP_ALLOWED_TO_EXECUTE
-  allowGroup = YET_ANOTHER_GROUP_ALLOWED_TO_EXECUTE
-  maxObjectSizeLimit = 40 m
-----
-
-[[receive.enableSignedPush]]receive.enableSignedPush::
+[[receive.allowGroup]]receive.allowGroup::
 +
-If true, server-side signed push validation is enabled.
+Name of the groups of users that are allowed to execute
+'receive-pack' on the server. One or more groups can be set.
 +
-When a client pushes with `git push --signed`, this ensures that the
-push certificate is valid and signed with a valid public key stored in
-the `refs/gpg-keys` branch of `All-Users`.
-+
-Defaults to false.
+If no groups are added, any user will be allowed to execute
+'receive-pack' on the server.
 
 [[receive.certNonceSeed]]receive.certNonceSeed::
 +
@@ -2910,6 +2898,18 @@ the client.
 +
 Default is 5 minutes.
 
+[[receive.changeUpdateThreads]]receive.changeUpdateThreads::
++
+Number of threads to perform change creation or patch set updates
+concurrently. Each thread uses its own database connection from
+the database connection pool, and if all threads are busy then
+main receive thread will also perform a change creation or patch
+set update.
++
+Defaults to 1, using only the main receive thread. This feature is for
+databases with very high latency that can benefit from concurrent
+operations when multiple changes are impacted at once.
+
 [[receive.checkMagicRefs]]receive.checkMagicRefs::
 +
 If true, Gerrit will verify the destination repository has
@@ -2938,13 +2938,30 @@ references to access commits intended to be hidden from the user.
 +
 Default is true.
 
-[[receive.allowGroup]]receive.allowGroup::
+[[receive.enableSignedPush]]receive.enableSignedPush::
 +
-Name of the groups of users that are allowed to execute
-'receive-pack' on the server. One or more groups can be set.
+If true, server-side signed push validation is enabled.
 +
-If no groups are added, any user will be allowed to execute
-'receive-pack' on the server.
+When a client pushes with `git push --signed`, this ensures that the
+push certificate is valid and signed with a valid public key stored in
+the `refs/gpg-keys` branch of `All-Users`.
++
+Defaults to false.
+
+[[receive.maxBatchChanges]]receive.maxBatchChanges::
++
+The maximum number of changes that Gerrit allows to be pushed
+in a batch for review. When this number is exceeded Gerrit rejects
+the push with an error message.
++
+May be overridden for certain groups by specifying a limit in the
+link:access-control.html#capability_batchChangesLimit['Batch Changes Limit']
+global capability.
++
+This setting can be used to prevent users from uploading large
+number of changes for review by mistake.
++
+Default is zero, no limit.
 
 [[receive.maxObjectSizeLimit]]receive.maxObjectSizeLimit::
 +
@@ -2965,21 +2982,6 @@ Default is zero.
 +
 Common unit suffixes of 'k', 'm', or 'g' are supported.
 
-[[receive.maxBatchChanges]]receive.maxBatchChanges::
-+
-The maximum number of changes that Gerrit allows to be pushed
-in a batch for review. When this number is exceeded Gerrit rejects
-the push with an error message.
-+
-May be overridden for certain groups by specifying a limit in the
-link:access-control.html#capability_batchChangesLimit['Batch Changes Limit']
-global capability.
-+
-This setting can be used to prevent users from uploading large
-number of changes for review by mistake.
-+
-Default is zero, no limit.
-
 [[receive.threadPoolSize]]receive.threadPoolSize::
 +
 Maximum size of the thread pool in which the change data in received packs is
@@ -2987,18 +2989,6 @@ processed.
 +
 Defaults to the number of available CPUs according to the Java runtime.
 
-[[receive.changeUpdateThreads]]receive.changeUpdateThreads::
-+
-Number of threads to perform change creation or patch set updates
-concurrently. Each thread uses its own database connection from
-the database connection pool, and if all threads are busy then
-main receive thread will also perform a change creation or patch
-set update.
-+
-Defaults to 1, using only the main receive thread. This feature is for
-databases with very high latency that can benefit from concurrent
-operations when multiple changes are impacted at once.
-
 [[receive.timeout]]receive.timeout::
 +
 Overall timeout on the time taken to process the change data in

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.api.accounts;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS;
 import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
@@ -39,7 +40,6 @@ import com.google.gerrit.gpg.server.GpgKeys;
 import com.google.gerrit.gpg.testutil.TestKey;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountExternalId;
-import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.testutil.ConfigSuite;
@@ -99,9 +99,9 @@ public class AccountIT extends AbstractDaemonTest {
   @After
   public void clearPublicKeyStore() throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {
-      Ref ref = repo.getRef(RefNames.REFS_GPG_KEYS);
+      Ref ref = repo.getRef(REFS_GPG_KEYS);
       if (ref != null) {
-        RefUpdate ru = repo.updateRef(RefNames.REFS_GPG_KEYS);
+        RefUpdate ru = repo.updateRef(REFS_GPG_KEYS);
         ru.setForceUpdate(true);
         assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
       }
@@ -115,7 +115,7 @@ public class AccountIT extends AbstractDaemonTest {
 
   @After
   public void deleteGpgKeys() throws Exception {
-    String ref = RefNames.REFS_GPG_KEYS;
+    String ref = REFS_GPG_KEYS;
     try (Repository repo = repoManager.openRepository(allUsers)) {
       if (repo.getRefDatabase().exactRef(ref) != null) {
         RefUpdate ru = repo.updateRef(ref);

gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyStore.java

@@ -17,8 +17,6 @@ package com.google.gerrit.gpg;
 import static com.google.common.base.Preconditions.checkState;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
-import com.google.gerrit.reviewdb.client.RefNames;
-
 import org.bouncycastle.bcpg.ArmoredInputStream;
 import org.bouncycastle.bcpg.ArmoredOutputStream;
 import org.bouncycastle.openpgp.PGPException;
@@ -73,6 +71,9 @@ public class PublicKeyStore implements AutoCloseable {
   private static final ObjectId EMPTY_TREE =
       ObjectId.fromString("4b825dc642cb6eb9a060e54bf8d69288fbee4904");
 
+  /** Ref where GPG public keys are stored. */
+  public static final String REFS_GPG_KEYS = "refs/meta/gpg-keys";
+
   private final Repository repo;
   private ObjectReader reader;
   private RevCommit tip;
@@ -104,7 +105,7 @@ public class PublicKeyStore implements AutoCloseable {
     reset();
     reader = repo.newObjectReader();
 
-    Ref ref = repo.getRefDatabase().exactRef(RefNames.REFS_GPG_KEYS);
+    Ref ref = repo.getRefDatabase().exactRef(REFS_GPG_KEYS);
     if (ref == null) {
       return;
     }
@@ -249,7 +250,7 @@ public class PublicKeyStore implements AutoCloseable {
       ins.flush();
     }
 
-    RefUpdate ru = repo.updateRef(RefNames.REFS_GPG_KEYS);
+    RefUpdate ru = repo.updateRef(PublicKeyStore.REFS_GPG_KEYS);
     ru.setExpectedOldObjectId(tip);
     ru.setNewObjectId(newTip);
     ru.setRefLogIdent(cb.getCommitter());

gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java

@@ -31,6 +31,8 @@ import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.PushCertificate;
 import org.eclipse.jgit.transport.PushCertificate.NonceStatus;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
@@ -39,6 +41,9 @@ import java.util.List;
 
 /** Checker for push certificates. */
 public abstract class PushCertificateChecker {
+  private static final Logger log =
+      LoggerFactory.getLogger(PushCertificateChecker.class);
+
   private final PublicKeyChecker publicKeyChecker;
 
   protected PushCertificateChecker(PublicKeyChecker publicKeyChecker) {
@@ -49,28 +54,34 @@ public abstract class PushCertificateChecker {
    * Check a push certificate.
    *
    * @return result of the check.
-   * @throws PGPException if an error occurred during GPG checks.
-   * @throws IOException if an error occurred reading from the repository.
    */
-  public final CheckResult check(PushCertificate cert) throws PGPException, IOException {
+  public final CheckResult check(PushCertificate cert) {
     if (cert.getNonceStatus() != NonceStatus.OK) {
       return new CheckResult("Invalid nonce");
     }
-    PGPSignature sig = readSignature(cert);
-    if (sig == null) {
-      return new CheckResult("Invalid signature format");
-    }
-    Repository repo = getRepository();
     List<String> problems = new ArrayList<>();
-    try (PublicKeyStore store = new PublicKeyStore(repo)) {
-      checkSignature(sig, cert, store.get(sig.getKeyID()), problems);
-      checkCustom(repo, problems);
-      return new CheckResult(problems);
-    } finally {
-      if (shouldClose(repo)) {
-        repo.close();
+    try {
+      PGPSignature sig = readSignature(cert);
+      if (sig != null) {
+        @SuppressWarnings("resource")
+        Repository repo = getRepository();
+        try (PublicKeyStore store = new PublicKeyStore(repo)) {
+          checkSignature(sig, cert, store.get(sig.getKeyID()), problems);
+          checkCustom(repo, problems);
+        } finally {
+          if (shouldClose(repo)) {
+            repo.close();
+          }
+        }
+      } else {
+        problems.add("Invalid signature format");
       }
+    } catch (PGPException | IOException e) {
+      String msg = "Internal error checking push certificate";
+      log.error(msg, e);
+      problems.add(msg);
     }
+    return new CheckResult(problems);
   }
 
   /**
@@ -140,9 +151,9 @@ public abstract class PushCertificateChecker {
         if (result.isOk()) {
           return;
         }
-        StringBuilder err = new StringBuilder("Invalid public key (")
+        StringBuilder err = new StringBuilder("Invalid public key ")
             .append(keyToString(k))
-            .append("):");
+            .append(":");
         for (int i = 0; i < result.getProblems().size(); i++) {
           err.append('\n').append("  ").append(result.getProblems().get(i));
         }
@@ -150,13 +161,13 @@ public abstract class PushCertificateChecker {
         return;
       } catch (PGPException e) {
         deferredProblems.add(
-            "Error checking signature with public key (" + keyToString(k)
+            "Error checking signature with public key " + keyToString(k)
             + ": " + e.getMessage());
       }
     }
     if (!anyKeys) {
       problems.add(
-          "No public keys found for Key ID " + keyIdToString(sig.getKeyID()));
+          "No public keys found for key ID " + keyIdToString(sig.getKeyID()));
     } else {
       problems.addAll(deferredProblems);
     }

gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java

@@ -19,14 +19,11 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
-import org.bouncycastle.openpgp.PGPException;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.PreReceiveHook;
 import org.eclipse.jgit.transport.PushCertificate;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.eclipse.jgit.transport.ReceivePack;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Collection;
@@ -40,9 +37,6 @@ import java.util.Collection;
  */
 @Singleton
 public class SignedPushPreReceiveHook implements PreReceiveHook {
-  private static final Logger log =
-      LoggerFactory.getLogger(SignedPushPreReceiveHook.class);
-
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsers;
   private final PublicKeyChecker keyChecker;
@@ -60,32 +54,27 @@ public class SignedPushPreReceiveHook implements PreReceiveHook {
   @Override
   public void onPreReceive(ReceivePack rp,
       Collection<ReceiveCommand> commands) {
-    try {
-      PushCertificate cert = rp.getPushCertificate();
-      if (cert == null) {
-        return;
+    PushCertificate cert = rp.getPushCertificate();
+    if (cert == null) {
+      return;
+    }
+    PushCertificateChecker checker = new PushCertificateChecker(keyChecker) {
+      @Override
+      protected Repository getRepository() throws IOException {
+        return repoManager.openRepository(allUsers);
       }
-      PushCertificateChecker checker = new PushCertificateChecker(keyChecker) {
-        @Override
-        protected Repository getRepository() throws IOException {
-          return repoManager.openRepository(allUsers);
-        }
 
-        @Override
-        protected boolean shouldClose(Repository repo) {
-          return true;
-        }
-      };
-      CheckResult result = checker.check(cert);
-      if (!result.isOk()) {
-        for (String problem : result.getProblems()) {
-          rp.sendMessage(problem);
-        }
-        reject(commands, "invalid push cert");
+      @Override
+      protected boolean shouldClose(Repository repo) {
+        return true;
       }
-    } catch (PGPException | IOException e) {
-      log.error("Error checking push certificate", e);
-      reject(commands, "push cert error");
+    };
+    CheckResult result = checker.check(cert);
+    if (!result.isOk()) {
+      for (String problem : result.getProblems()) {
+        rp.sendMessage(problem);
+      }
+      reject(commands, "invalid push cert");
     }
   }
 

gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyStoreTest.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.gpg;
 
+import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS;
 import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString;
 import static com.google.gerrit.gpg.PublicKeyStore.keyObjectId;
 import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
@@ -23,7 +24,6 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import com.google.gerrit.gpg.testutil.TestKey;
-import com.google.gerrit.reviewdb.client.RefNames;
 
 import org.bouncycastle.openpgp.PGPPublicKey;
 import org.bouncycastle.openpgp.PGPPublicKeyRing;
@@ -84,13 +84,13 @@ public class PublicKeyStoreTest {
   @Test
   public void testGet() throws Exception {
     TestKey key1 = TestKey.key1();
-    tr.branch(RefNames.REFS_GPG_KEYS)
+    tr.branch(REFS_GPG_KEYS)
         .commit()
         .add(keyObjectId(key1.getKeyId()).name(),
           key1.getPublicKeyArmored())
         .create();
     TestKey key2 = TestKey.key2();
-    tr.branch(RefNames.REFS_GPG_KEYS)
+    tr.branch(REFS_GPG_KEYS)
         .commit()
         .add(keyObjectId(key2.getKeyId()).name(),
           key2.getPublicKeyArmored())
@@ -104,7 +104,7 @@ public class PublicKeyStoreTest {
   public void testGetMultiple() throws Exception {
     TestKey key1 = TestKey.key1();
     TestKey key2 = TestKey.key2();
-    tr.branch(RefNames.REFS_GPG_KEYS)
+    tr.branch(REFS_GPG_KEYS)
         .commit()
         .add(keyObjectId(key1.getKeyId()).name(),
             key1.getPublicKeyArmored()
@@ -131,7 +131,7 @@ public class PublicKeyStoreTest {
   public void saveAppendsToExistingList() throws Exception {
     TestKey key1 = TestKey.key1();
     TestKey key2 = TestKey.key2();
-    tr.branch(RefNames.REFS_GPG_KEYS)
+    tr.branch(REFS_GPG_KEYS)
         .commit()
         // Mismatched for this key ID, but we can still read it out.
         .add(keyObjectId(key1.getKeyId()).name(), key2.getPublicKeyArmored())
@@ -146,7 +146,7 @@ public class PublicKeyStoreTest {
         RevWalk rw = new RevWalk(reader)) {
       NoteMap notes = NoteMap.read(
           reader, tr.getRevWalk().parseCommit(
-            tr.getRepository().getRef(RefNames.REFS_GPG_KEYS).getObjectId()));
+            tr.getRepository().getRef(REFS_GPG_KEYS).getObjectId()));
       String contents = new String(
           reader.open(notes.get(keyObjectId(key1.getKeyId()))).getBytes(),
           UTF_8);

gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java

@@ -14,13 +14,13 @@
 
 package com.google.gerrit.gpg;
 
+import static com.google.gerrit.gpg.PublicKeyStore.REFS_GPG_KEYS;
 import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString;
 import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertEquals;
 
 import com.google.gerrit.gpg.testutil.TestKey;
-import com.google.gerrit.reviewdb.client.RefNames;
 
 import org.bouncycastle.bcpg.ArmoredOutputStream;
 import org.bouncycastle.bcpg.BCPGOutputStream;
@@ -56,7 +56,7 @@ public class PushCertificateCheckerTest {
     TestKey key3 = TestKey.key3();
     tr = new TestRepository<>(new InMemoryRepository(
         new DfsRepositoryDescription("repo")));
-    tr.branch(RefNames.REFS_GPG_KEYS).commit()
+    tr.branch(REFS_GPG_KEYS).commit()
         .add(PublicKeyStore.keyObjectId(key1.getPublicKey().getKeyID()).name(),
             key1.getPublicKeyArmored())
         .add(PublicKeyStore.keyObjectId(key3.getPublicKey().getKeyID()).name(),
@@ -96,7 +96,7 @@ public class PushCertificateCheckerTest {
     TestKey key2 = TestKey.key2();
     PushCertificate cert = newSignedCert(validNonce(), key2);
     assertProblems(cert,
-        "No public keys found for Key ID " + keyIdToString(key2.getKeyId()));
+        "No public keys found for key ID " + keyIdToString(key2.getKeyId()));
   }
 
   @Test
@@ -104,8 +104,8 @@ public class PushCertificateCheckerTest {
     TestKey key3 = TestKey.key3();
     PushCertificate cert = newSignedCert(validNonce(), key3);
     assertProblems(cert,
-        "Invalid public key (" + keyToString(key3.getPublicKey())
-          + "):\n  Key is expired");
+        "Invalid public key " + keyToString(key3.getPublicKey())
+          + ":\n  Key is expired");
   }
 
   private String validNonce() {

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java

@@ -57,12 +57,6 @@ public class RefNames {
 
   public static final String EDIT_PREFIX = "edit-";
 
-  /**
-   * Special ref for GPG public keys used by {@link
-   * com.google.gerrit.gpg.SignedPushPreReceiveHook}.
-   */
-  public static final String REFS_GPG_KEYS = "refs/meta/gpg-keys";
-
   public static String fullName(String ref) {
     return ref.startsWith(REFS) ? ref : REFS_HEADS + ref;
   }