Merge "Fix signed push failing when using push options"
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java
index 5db4563..118ce18 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java
@@ -57,6 +57,7 @@
import java.io.InputStreamReader;
import java.io.Reader;
import java.io.StringReader;
+import java.util.List;
import org.eclipse.jgit.errors.PackProtocolException;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
@@ -364,6 +365,77 @@ public void testMissingPusheeField() throws Exception {
assertFalse(cert.toText().contains(PushCertificateParser.PUSHEE));
}
+ @Test
+ public void testWithPushOptions() throws Exception {
+ // Same cert as INPUT, but with push-option lines added after nonce
+ String inputWithOptions = "001ccertificate version 0.1\n"
+ + "0041pusher Dave Borowitz <dborowitz@google.com> 1433954361 -0700\n"
+ + "0024pushee git://localhost/repo.git\n"
+ + "002anonce 1433954361-bde756572d665bba81d8\n"
+ + "0024push-option label=Code-Review+2\n"
+ + "0021push-option label=Verified+1\n"
+ + "0017push-option submit\n"
+ + "0020push-option hashtag=bug fix\n"
+ + "0005\n"
+ + "00680000000000000000000000000000000000000000"
+ + " 6c2b981a177396fb47345b7df3e4d3f854c6bea7"
+ + " refs/heads/master\n"
+ + "0022-----BEGIN PGP SIGNATURE-----\n"
+ + "0016Version: GnuPG v1\n"
+ + "0005\n"
+ + "0045iQEcBAABAgAGBQJVeGg5AAoJEPfTicJkUdPkUggH/RKAeI9/i/LduuiqrL/SSdIa\n"
+ + "00459tYaSqJKLbXz63M/AW4Sp+4u+dVCQvnAt/a35CVEnpZz6hN4Kn/tiswOWVJf4CO7\n"
+ + "0045htNubGs5ZMwvD6sLYqKAnrM3WxV/2TbbjzjZW6Jkidz3jz/WRT4SmjGYiEO7aA+V\n"
+ + "00454ZdIS9f7sW5VsHHYlNThCA7vH8Uu48bUovFXyQlPTX0pToSgrWV3JnTxDNxfn3iG\n"
+ + "0045IL0zTY/qwVCdXgFownLcs6J050xrrBWIKqfcWr3u4D2aCLyR0v+S/KArr7ulZygY\n"
+ + "0045+SOklImn8TAZiNxhWtA6ens66IiammUkZYFv7SSzoPLFZT4dC84SmGPWgf94NoQ=\n"
+ + "000a=XFeC\n"
+ + "0020-----END PGP SIGNATURE-----\n"
+ + "0012push-cert-end\n";
+
+ PacketLineIn pckIn = newPacketLineIn(inputWithOptions);
+ PushCertificateParser parser =
+ new PushCertificateParser(db, newEnabledConfig());
+ parser.receiveHeader(pckIn, false);
+
+ parser.addCommand(pckIn.readString());
+ assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readString());
+ parser.receiveSignature(pckIn);
+
+ PushCertificate cert = parser.build();
+ assertNotNull(cert);
+ assertEquals("0.1", cert.getVersion());
+ assertEquals("Dave Borowitz", cert.getPusherIdent().getName());
+
+ // Verify push options were parsed and are available on the certificate
+ List<String> pushOptions = cert.getPushOptions();
+ assertEquals(4, pushOptions.size());
+ assertEquals("label=Code-Review+2", pushOptions.get(0));
+ assertEquals("label=Verified+1", pushOptions.get(1));
+ assertEquals("submit", pushOptions.get(2));
+ assertEquals("hashtag=bug fix", pushOptions.get(3));
+
+ // Verify that toText() includes push options (critical for signature verification)
+ String text = cert.toText();
+ assertTrue("toText() must include push-option lines",
+ text.contains("push-option label=Code-Review+2"));
+ assertTrue("toText() must include push-option lines",
+ text.contains("push-option label=Verified+1"));
+ assertTrue("toText() must include push-option lines",
+ text.contains("push-option submit"));
+ assertTrue("toText() must include push-option lines",
+ text.contains("push-option hashtag=bug fix"));
+
+ // Verify push options appear after nonce and before empty line
+ int nonceIndex = text.indexOf("nonce");
+ int pushOptionIndex = text.indexOf("push-option");
+ int emptyLineIndex = text.indexOf("\n\n");
+ assertTrue("push-option must appear after nonce",
+ pushOptionIndex > nonceIndex);
+ assertTrue("push-option must appear before empty line separator",
+ pushOptionIndex < emptyLineIndex);
+ }
+
private static String concatPacketLines(String input, int begin, int end)
throws IOException {
StringBuilder result = new StringBuilder();
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackTest.java
index 156746d..595b970 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackTest.java
@@ -45,8 +45,15 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
import org.eclipse.jgit.errors.PackProtocolException;
+import org.eclipse.jgit.internal.JGitText;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.transport.ReceiveCommand.Result;
import org.junit.Test;
/** Tests for receive-pack utilities. */
@@ -82,4 +89,62 @@ private void assertParseCommandFails(String input) {
// Expected.
}
}
+
+ @Test
+ public void testCertificatePushOptionsMismatch() throws Exception {
+ ReceiveCommand cmd1 = new ReceiveCommand(
+ ObjectId.fromString("0000000000000000000000000000000000000000"),
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ "refs/heads/master");
+ ReceiveCommand cmd2 = new ReceiveCommand(
+ ObjectId.fromString("0000000000000000000000000000000000000000"),
+ ObjectId.fromString("badc0ffebadc0ffebadc0ffebadc0ffebadc0ffe"),
+ "refs/heads/branch");
+
+ List<ReceiveCommand> commands = new ArrayList<>();
+ commands.add(cmd1);
+ commands.add(cmd2);
+
+ ReceivePack.validateCertificatePushOptions(
+ Arrays.asList("option1", "option2"),
+ Arrays.asList("option1", "different"), commands);
+
+ assertEquals(Result.REJECTED_OTHER_REASON, cmd1.getResult());
+ assertEquals(Result.REJECTED_OTHER_REASON, cmd2.getResult());
+ assertEquals(JGitText.get().pushCertificateInconsistentPushOptions,
+ cmd1.getMessage());
+ assertEquals(JGitText.get().pushCertificateInconsistentPushOptions,
+ cmd2.getMessage());
+ }
+
+ @Test
+ public void testCertificatePushOptionsMatch() throws Exception {
+ ReceiveCommand cmd = new ReceiveCommand(
+ ObjectId.fromString("0000000000000000000000000000000000000000"),
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ "refs/heads/master");
+
+ List<ReceiveCommand> commands = Collections.singletonList(cmd);
+
+ ReceivePack.validateCertificatePushOptions(
+ Arrays.asList("option1", "option2"),
+ Arrays.asList("option1", "option2"), commands);
+
+ assertEquals(Result.NOT_ATTEMPTED, cmd.getResult());
+ }
+
+ @Test
+ public void testCertificatePushOptionsNullTreatedAsEmpty() throws Exception {
+ ReceiveCommand cmd = new ReceiveCommand(
+ ObjectId.fromString("0000000000000000000000000000000000000000"),
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ "refs/heads/master");
+
+ List<ReceiveCommand> commands = Collections.singletonList(cmd);
+
+ ReceivePack.validateCertificatePushOptions(Collections.emptyList(),
+ null, commands);
+
+ assertEquals(Result.NOT_ATTEMPTED, cmd.getResult());
+ }
}
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index e24cba6..6158032 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -638,6 +638,7 @@
pushCertificateInvalidFieldValue=Push certificate has missing or invalid value for {0}: {1}
pushCertificateInvalidHeader=Push certificate has invalid header format
pushCertificateInvalidSignature=Push certificate has invalid signature format
+pushCertificateInconsistentPushOptions=Push certificate push options do not match command push options
pushDefaultNothing=No refspec given and push.default=nothing; no upstream branch can be determined
pushDefaultNoUpstream=No upstream branch found for local branch ''{0}''
pushDefaultSimple=push.default=simple requires local branch name ''{0}'' to be equal to upstream tracked branch name ''{1}''
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 8928f47..9bd99ed 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -669,6 +669,7 @@ public static JGitText get() {
/***/ public String pushCertificateInvalidFieldValue;
/***/ public String pushCertificateInvalidHeader;
/***/ public String pushCertificateInvalidSignature;
+ /***/ public String pushCertificateInconsistentPushOptions;
/***/ public String pushDefaultNothing;
/***/ public String pushDefaultNoUpstream;
/***/ public String pushDefaultSimple;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java
index 437abb0..b95d094 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java
@@ -53,11 +53,12 @@ public enum NonceStatus {
private final String nonce;
private final NonceStatus nonceStatus;
private final List<ReceiveCommand> commands;
+ private final List<String> pushOptions;
private final String signature;
PushCertificate(String version, PushCertificateIdent pusher, String pushee,
String nonce, NonceStatus nonceStatus, List<ReceiveCommand> commands,
- String signature) {
+ List<String> pushOptions, String signature) {
if (version == null || version.isEmpty()) {
throw new IllegalArgumentException(MessageFormat.format(
JGitText.get().pushCertificateInvalidField, VERSION));
@@ -95,6 +96,7 @@ public enum NonceStatus {
this.nonce = nonce;
this.nonceStatus = nonceStatus;
this.commands = commands;
+ this.pushOptions = pushOptions;
this.signature = signature;
}
@@ -171,6 +173,16 @@ public List<ReceiveCommand> getCommands() {
}
/**
+ * Get the list of push options that were included in the push certificate.
+ *
+ * @return the push options, or an empty list if none.
+ * @since 7.5
+ */
+ public List<String> getPushOptions() {
+ return pushOptions;
+ }
+
+ /**
* Get the raw signature
*
* @return the raw signature, consisting of the lines received between the
@@ -212,8 +224,12 @@ private StringBuilder toStringBuilder() {
if (pushee != null) {
sb.append(PUSHEE).append(' ').append(pushee).append('\n');
}
- sb.append(NONCE).append(' ').append(nonce).append('\n')
- .append('\n');
+ sb.append(NONCE).append(' ').append(nonce).append('\n');
+ for (String option : pushOptions) {
+ sb.append(PushCertificateParser.PUSH_OPTION).append(' ')
+ .append(option).append('\n');
+ }
+ sb.append('\n');
for (ReceiveCommand cmd : commands) {
sb.append(cmd.getOldId().name())
.append(' ').append(cmd.getNewId().name())
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java
index 463d053..c9f0dec 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java
@@ -47,6 +47,8 @@ public class PushCertificateParser {
static final String NONCE = "nonce"; //$NON-NLS-1$
+ static final String PUSH_OPTION = "push-option"; //$NON-NLS-1$
+
static final String END_CERT = "push-cert-end"; //$NON-NLS-1$
private static final String VERSION_0_1 = "0.1"; //$NON-NLS-1$
@@ -173,6 +175,7 @@ public static PushCertificate fromString(String str)
private final boolean enabled;
private final NonceGenerator nonceGenerator;
private final List<ReceiveCommand> commands = new ArrayList<>();
+ private final List<String> pushOptions = new ArrayList<>();
/**
* <p>Constructor for PushCertificateParser.</p>
@@ -251,7 +254,8 @@ public PushCertificate build() throws IOException {
}
try {
return new PushCertificate(version, pusher, pushee, receivedNonce,
- nonceStatus, Collections.unmodifiableList(commands), signature);
+ nonceStatus, Collections.unmodifiableList(commands),
+ Collections.unmodifiableList(pushOptions), signature);
} catch (IllegalArgumentException e) {
throw new IOException(e.getMessage(), e);
}
@@ -368,10 +372,16 @@ private void receiveHeader(StringReader reader, boolean stateless)
? nonceGenerator.verify(
receivedNonce, sentNonce(), db, stateless, nonceSlopLimit)
: NonceStatus.UNSOLICITED;
- // An empty line.
- if (!reader.read().isEmpty()) {
- throw new PackProtocolException(
- JGitText.get().pushCertificateInvalidHeader);
+ // Read push-option lines (if any) before the empty line separator
+ String line;
+ while (!(line = reader.read()).isEmpty()) {
+ if (line.startsWith(PUSH_OPTION + " ")) {
+ pushOptions.add(parseHeader(line, PUSH_OPTION));
+ } else {
+ // Not a push-option, should be empty line
+ throw new PackProtocolException(
+ JGitText.get().pushCertificateInvalidHeader);
+ }
}
} catch (EOFException eof) {
throw new PackProtocolException(
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
index 6f211e0..3d9c81c 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
@@ -40,6 +40,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
@@ -1375,6 +1376,11 @@ private void recvCommands() throws IOException {
if (hasCommands()) {
readPostCommands(pck);
}
+ // Verify that push options in the certificate match the post-command ones.
+ if (pushCert != null) {
+ validateCertificatePushOptions(pushCert.getPushOptions(),
+ pushOptions, commands);
+ }
} catch (Throwable t) {
discardCommands();
throw t;
@@ -1406,6 +1412,35 @@ private void parseShallow(String idStr) throws PackProtocolException {
}
/**
+ * Validates that push options in the certificate match the post-command
+ * push options. If they don't match, marks all commands as rejected.
+ * <p>
+ * This prevents tampering with signed push certificates by ensuring the
+ * options that were signed match exactly the options sent after the
+ * commands.
+ *
+ * @param certPushOptions
+ * the push options from the certificate
+ * @param postCommandPushOptions
+ * the push options sent after commands (may be null)
+ * @param commands
+ * the list of commands to reject if validation fails
+ */
+ static void validateCertificatePushOptions(List<String> certPushOptions,
+ List<String> postCommandPushOptions, List<ReceiveCommand> commands) {
+ List<String> postOptions = postCommandPushOptions == null
+ ? Collections.emptyList()
+ : postCommandPushOptions;
+ if (!Objects.equals(certPushOptions, postOptions)) {
+ // Mark all commands as rejected like in C Git.
+ for (ReceiveCommand cmd : commands) {
+ cmd.setResult(Result.REJECTED_OTHER_REASON,
+ JGitText.get().pushCertificateInconsistentPushOptions);
+ }
+ }
+ }
+
+ /**
* @param in
* request stream.
* @throws IOException
