From a798022c7f9722d28b8c8b21199330c32b78689f Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 31 Jan 2020 22:37:07 +0000 Subject: [PATCH] Move random Change-Id generation out of Change The Change-Id generation has been broken for years because two calls for creating two changes with the same content would have resulted in the same Change-Id. The master version of the Change-Id generation uses instead a SecureRandom which gives a lot more entropy when generating Change-Id, however, the method was implemented in the Change class that is used in v2.16 by the GWT UI. Move the random generation to CommitMessageUtil which gives more guarantees to be outside the GWT UI when cherry-picked to v2.16. Bug: Issue 12246 Change-Id: I9de36332ef1d29a97ee459345fb3351b33b7af67 --- java/com/google/gerrit/entities/Change.java | 29 ----------------- .../gerrit/server/change/ChangeInserter.java | 3 +- .../google/gerrit/server/git/CommitUtil.java | 3 +- .../server/git/meta/VersionedMetaData.java | 5 +-- .../restapi/change/CherryPickChange.java | 5 ++- .../server/restapi/change/CreateChange.java | 3 +- .../restapi/change/RevertSubmission.java | 3 +- .../gerrit/server/util/CommitMessageUtil.java | 31 +++++++++++++++++++ 8 files changed, 46 insertions(+), 36 deletions(-) diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java index c768094151..b36b5f906d 100644 --- a/java/com/google/gerrit/entities/Change.java +++ b/java/com/google/gerrit/entities/Change.java @@ -16,20 +16,14 @@ package com.google.gerrit.entities; import static com.google.common.base.Preconditions.checkArgument; import static com.google.gerrit.entities.RefNames.REFS_CHANGES; -import static java.nio.charset.StandardCharsets.UTF_8; import com.google.auto.value.AutoValue; import com.google.common.primitives.Ints; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.client.ChangeStatus; -import java.security.NoSuchAlgorithmException; -import java.security.SecureRandom; import java.sql.Timestamp; import java.util.Arrays; import java.util.Optional; -import org.eclipse.jgit.lib.Constants; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectInserter; /** * A change proposed to be merged into a branch. @@ -101,15 +95,6 @@ import org.eclipse.jgit.lib.ObjectInserter; * notice of a replacement patch set is sent, or when notice of the change submission occurs. */ public final class Change { - private static final SecureRandom rng; - - static { - try { - rng = SecureRandom.getInstance("SHA1PRNG"); - } catch (NoSuchAlgorithmException e) { - throw new RuntimeException("Cannot create RNG for Change-Id generator", e); - } - } public static Id id(int id) { return new AutoValue_Change_Id(id); @@ -282,20 +267,6 @@ public final class Change { } } - public static ObjectId generateChangeId() { - byte[] rand = new byte[Constants.OBJECT_ID_STRING_LENGTH]; - rng.nextBytes(rand); - String randomString = new String(rand, UTF_8); - - try (ObjectInserter f = new ObjectInserter.Formatter()) { - return f.idFor(Constants.OBJ_COMMIT, Constants.encode(randomString)); - } - } - - public static Key generateKey() { - return key("I" + generateChangeId().name()); - } - public static Key key(String key) { return new AutoValue_Change_Key(key); } diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java index 770f36c758..f0a3024ac3 100644 --- a/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/java/com/google/gerrit/server/change/ChangeInserter.java @@ -68,6 +68,7 @@ import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.server.update.Context; import com.google.gerrit.server.update.InsertChangeOp; import com.google.gerrit.server.update.RepoContext; +import com.google.gerrit.server.util.CommitMessageUtil; import com.google.gerrit.server.util.RequestScopePropagator; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -207,7 +208,7 @@ public class ChangeInserter implements InsertChangeOp { } // A Change-Id is generated for the review, but not appended to the commit message. // This can happen if requireChangeId is false. - return Change.generateKey(); + return CommitMessageUtil.generateKey(); } public PatchSet.Id getPatchSetId() { diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java index 818212c771..df53133a7c 100644 --- a/java/com/google/gerrit/server/git/CommitUtil.java +++ b/java/com/google/gerrit/server/git/CommitUtil.java @@ -49,6 +49,7 @@ import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.server.update.Context; import com.google.gerrit.server.update.UpdateException; +import com.google.gerrit.server.util.CommitMessageUtil; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -151,7 +152,7 @@ public class CommitUtil { ObjectInserter oi = git.newObjectInserter(); ObjectReader reader = oi.newReader(); RevWalk revWalk = new RevWalk(reader)) { - ObjectId generatedChangeId = Change.generateChangeId(); + ObjectId generatedChangeId = CommitMessageUtil.generateChangeId(); ObjectId revCommit = createRevertCommit(message, notes, user, timestamp, oi, revWalk, generatedChangeId); return createRevertChangeFromCommit( diff --git a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java index c52bbbce37..2ed755ae9b 100644 --- a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java +++ b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java @@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.base.MoreObjects; import com.google.gerrit.common.Nullable; -import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; import com.google.gerrit.git.GitUpdateFailureException; import com.google.gerrit.git.LockFailureException; @@ -26,6 +25,7 @@ import com.google.gerrit.git.ObjectIds; import com.google.gerrit.server.logging.Metadata; import com.google.gerrit.server.logging.TraceContext; import com.google.gerrit.server.logging.TraceContext.TraceTimer; +import com.google.gerrit.server.util.CommitMessageUtil; import java.io.BufferedReader; import java.io.File; import java.io.IOException; @@ -330,7 +330,8 @@ public abstract class VersionedMetaData { } if (update.insertChangeId()) { - commit.setMessage(ChangeIdUtil.insertId(commit.getMessage(), Change.generateChangeId())); + commit.setMessage( + ChangeIdUtil.insertId(commit.getMessage(), CommitMessageUtil.generateChangeId())); } src = rw.parseCommit(inserter.insert(commit)); diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java index ac81b45c1b..935472738b 100644 --- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java +++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java @@ -60,6 +60,7 @@ import com.google.gerrit.server.submit.IntegrationException; import com.google.gerrit.server.submit.MergeIdenticalTreeException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.UpdateException; +import com.google.gerrit.server.util.CommitMessageUtil; import com.google.gerrit.server.util.time.TimeUtil; import com.google.inject.Inject; import com.google.inject.Provider; @@ -304,7 +305,9 @@ public class CherryPickChange { PersonIdent committerIdent = identifiedUser.newCommitterIdent(timestamp, serverTimeZone); final ObjectId generatedChangeId = - changeIdForNewChange != null ? changeIdForNewChange : Change.generateChangeId(); + changeIdForNewChange != null + ? changeIdForNewChange + : CommitMessageUtil.generateChangeId(); String commitMessage = ChangeIdUtil.insertId(message, generatedChangeId).trim() + '\n'; CodeReviewCommit cherryPickCommit; diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java index 0d3aaac435..60631d2078 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateChange.java +++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java @@ -76,6 +76,7 @@ import com.google.gerrit.server.restapi.project.CommitsCollection; import com.google.gerrit.server.restapi.project.ProjectsCollection; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.UpdateException; +import com.google.gerrit.server.util.CommitMessageUtil; import com.google.gerrit.server.util.time.TimeUtil; import com.google.inject.Inject; import com.google.inject.Provider; @@ -461,7 +462,7 @@ public class CreateChange // Add a Change-Id line if there isn't already one String commitMessage = subject; if (ChangeIdUtil.indexOfChangeId(commitMessage, "\n") == -1) { - ObjectId id = Change.generateChangeId(); + ObjectId id = CommitMessageUtil.generateChangeId(); commitMessage = ChangeIdUtil.insertId(commitMessage, id); } diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java index d67fc6aea6..3cd1f28a8f 100644 --- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java +++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java @@ -68,6 +68,7 @@ import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.server.update.Context; import com.google.gerrit.server.update.UpdateException; +import com.google.gerrit.server.util.CommitMessageUtil; import com.google.gerrit.server.util.time.TimeUtil; import com.google.inject.Inject; import com.google.inject.Provider; @@ -302,7 +303,7 @@ public class RevertSubmission implements RestModifyView