diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index b856c733e8..14523ec438 100644 --- a/Documentation/config-gerrit.txt +++ b/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 diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index c6482a9304..5baaa18c6a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/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); diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyStore.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyStore.java index 3736a7c74b..a36052ee62 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyStore.java +++ b/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()); diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java index 80b87f7175..86a33ab05e 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java +++ b/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 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); } diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java index 8618f208f6..b2dca8bf3a 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java +++ b/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 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"); } } diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyStoreTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyStoreTest.java index f48f9d806a..d936a318f9 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyStoreTest.java +++ b/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); diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java index 238156c43b..8a633ae661 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java +++ b/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() { diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java index 9468d2d8cc..da66929e9c 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/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; }