From 469734bf871b8307b2c83817c63b312a9e7d366d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 6 Jul 2015 15:19:42 -0400 Subject: [PATCH] BaseReceivePack: Treat all LFs as optional Discussion on the git mailing list has concluded[1] that the intended behavior for all (non-sideband) portions of the receive-pack protocol is for trailing LFs in pkt-lines to be optional. Go back to using PacketLineIn#readString() everywhere. For push certificates specifically, we agreed that the payload signed by the client is always concatenated with LFs even though the client MAY omit LFs when framing the certificate for the wire. This is still reflected in the implementation of PushCertificate#toText(). [1] http://thread.gmane.org/gmane.comp.version-control.git/273175/focus=273412 Change-Id: I817231c4d4defececb8722142fea18ff42e06e44 --- .../jgit/transport/BaseReceivePackTest.java | 7 -- .../transport/PushCertificateParserTest.java | 93 ++++++++++++++++--- .../jgit/transport/BaseReceivePack.java | 21 +---- .../jgit/transport/PushCertificate.java | 21 +++-- .../jgit/transport/PushCertificateParser.java | 45 ++++----- 5 files changed, 116 insertions(+), 71 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java index 1b3519206..7578c6e3e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BaseReceivePackTest.java @@ -51,13 +51,6 @@ import org.junit.Test; /** Tests for base receive-pack utilities. */ public class BaseReceivePackTest { - @Test - public void chomp() { - assertEquals("foo", BaseReceivePack.chomp("foo")); - assertEquals("foo", BaseReceivePack.chomp("foo\n")); - assertEquals("foo\n", BaseReceivePack.chomp("foo\n\n")); - } - @Test public void parseCommand() throws Exception { String o = "0000000000000000000000000000000000000000"; 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 6e49f47ff..8fdf386ec 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 @@ -87,6 +87,30 @@ public class PushCertificateParserTest { + "0020-----END PGP SIGNATURE-----\n" + "0012push-cert-end\n"; + // Same push certificate, with all trailing newlines stripped. + // (Note that the canonical signed payload is the same, so the same signature + // is still valid.) + private static final String INPUT_NO_NEWLINES = "001bcertificate version 0.1" + + "0040pusher Dave Borowitz 1433954361 -0700" + + "0023pushee git://localhost/repo.git" + + "0029nonce 1433954361-bde756572d665bba81d8" + + "0004" + + "00670000000000000000000000000000000000000000" + + " 6c2b981a177396fb47345b7df3e4d3f854c6bea7" + + " refs/heads/master" + + "0021-----BEGIN PGP SIGNATURE-----" + + "0015Version: GnuPG v1" + + "0004" + + "0044iQEcBAABAgAGBQJVeGg5AAoJEPfTicJkUdPkUggH/RKAeI9/i/LduuiqrL/SSdIa" + + "00449tYaSqJKLbXz63M/AW4Sp+4u+dVCQvnAt/a35CVEnpZz6hN4Kn/tiswOWVJf4CO7" + + "0044htNubGs5ZMwvD6sLYqKAnrM3WxV/2TbbjzjZW6Jkidz3jz/WRT4SmjGYiEO7aA+V" + + "00444ZdIS9f7sW5VsHHYlNThCA7vH8Uu48bUovFXyQlPTX0pToSgrWV3JnTxDNxfn3iG" + + "0044IL0zTY/qwVCdXgFownLcs6J050xrrBWIKqfcWr3u4D2aCLyR0v+S/KArr7ulZygY" + + "0044+SOklImn8TAZiNxhWtA6ens66IiammUkZYFv7SSzoPLFZT4dC84SmGPWgf94NoQ=" + + "0009=XFeC" + + "001f-----END PGP SIGNATURE-----" + + "0011push-cert-end"; + private Repository db; @Before @@ -115,11 +139,10 @@ public class PushCertificateParserTest { ObjectId newId = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); String line = oldId.name() + " " + newId.name() + " refs/heads/master"; - String rawLine = line + "\n"; - ReceiveCommand cmd = BaseReceivePack.parseCommand(line); - parser.addCommand(cmd, rawLine); - parser.addCommand(rawLine); + + parser.addCommand(cmd); + parser.addCommand(line); assertNull(parser.build()); } @@ -132,8 +155,8 @@ public class PushCertificateParserTest { assertNull(parser.build()); parser.receiveHeader(pckIn, false); - parser.addCommand(pckIn.readStringRaw()); - assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw()); + parser.addCommand(pckIn.readString()); + assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readString()); parser.receiveSignature(pckIn); assertNull(parser.build()); } @@ -162,8 +185,46 @@ public class PushCertificateParserTest { PushCertificateParser parser = new PushCertificateParser(db, newEnabledConfig()); parser.receiveHeader(pckIn, false); - parser.addCommand(pckIn.readStringRaw()); - assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw()); + parser.addCommand(pckIn.readString()); + assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readString()); + parser.receiveSignature(pckIn); + + PushCertificate cert = parser.build(); + assertEquals("0.1", cert.getVersion()); + assertEquals("Dave Borowitz", cert.getPusherIdent().getName()); + assertEquals("dborowitz@google.com", + cert.getPusherIdent().getEmailAddress()); + assertEquals(1433954361000L, cert.getPusherIdent().getWhen().getTime()); + assertEquals(-7 * 60, cert.getPusherIdent().getTimeZoneOffset()); + assertEquals("git://localhost/repo.git", cert.getPushee()); + assertEquals("1433954361-bde756572d665bba81d8", cert.getNonce()); + + assertNotEquals(cert.getNonce(), parser.getAdvertiseNonce()); + assertEquals(PushCertificate.NonceStatus.BAD, cert.getNonceStatus()); + + assertEquals(1, cert.getCommands().size()); + ReceiveCommand cmd = cert.getCommands().get(0); + assertEquals("refs/heads/master", cmd.getRefName()); + assertEquals(ObjectId.zeroId(), cmd.getOldId()); + assertEquals("6c2b981a177396fb47345b7df3e4d3f854c6bea7", + cmd.getNewId().name()); + + assertEquals(concatPacketLines(INPUT, 0, 6), cert.toText()); + + String signature = concatPacketLines(INPUT, 6, 17); + assertTrue(signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE)); + assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE + "\n")); + assertEquals(signature, cert.getSignature()); + } + + @Test + public void parseCertFromPktLineNoNewlines() throws Exception { + PacketLineIn pckIn = newPacketLineIn(INPUT_NO_NEWLINES); + 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(); @@ -186,11 +247,12 @@ public class PushCertificateParserTest { assertEquals("6c2b981a177396fb47345b7df3e4d3f854c6bea7", cmd.getNewId().name()); + // Canonical signed payload has reinserted newlines. assertEquals(concatPacketLines(INPUT, 0, 6), cert.toText()); String signature = concatPacketLines(INPUT, 6, 17); assertTrue(signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE)); - assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE)); + assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE + "\n")); assertEquals(signature, cert.getSignature()); } @@ -203,6 +265,15 @@ public class PushCertificateParserTest { assertEquals("line 2\nline 3\n", concatPacketLines(input, 1, 4)); } + @Test + public void testConcatPacketLinesInsertsNewlines() throws Exception { + String input = "000bline 1\n000aline 2000bline 3\n"; + assertEquals("line 1\n", concatPacketLines(input, 0, 1)); + assertEquals("line 1\nline 2\n", concatPacketLines(input, 0, 2)); + assertEquals("line 2\nline 3\n", concatPacketLines(input, 1, 3)); + assertEquals("line 2\nline 3\n", concatPacketLines(input, 1, 4)); + } + private static String concatPacketLines(String input, int begin, int end) throws IOException { StringBuilder result = new StringBuilder(); @@ -211,12 +282,12 @@ public class PushCertificateParserTest { while (i < end) { String line; try { - line = pckIn.readStringRaw(); + line = pckIn.readString(); } catch (EOFException e) { break; } if (++i > begin) { - result.append(line); + result.append(line).append('\n'); } } return result.toString(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java index 819f77c06..36d335503 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java @@ -1066,18 +1066,17 @@ public abstract class BaseReceivePack { PushCertificateParser certParser = getPushCertificateParser(); FirstLine firstLine = null; for (;;) { - String rawLine; + String line; try { - rawLine = pckIn.readStringRaw(); + line = pckIn.readString(); } catch (EOFException eof) { if (commands.isEmpty()) return; throw eof; } - if (rawLine == PacketLineIn.END) { + if (line == PacketLineIn.END) { break; } - String line = chomp(rawLine); if (line.length() >= 48 && line.startsWith("shallow ")) { //$NON-NLS-1$ clientShallowCommits.add(ObjectId.fromString(line.substring(8, 48))); @@ -1095,7 +1094,7 @@ public abstract class BaseReceivePack { } } - if (rawLine.equals(PushCertificateParser.BEGIN_SIGNATURE)) { + if (line.equals(PushCertificateParser.BEGIN_SIGNATURE)) { certParser.receiveSignature(pckIn); continue; } @@ -1114,21 +1113,11 @@ public abstract class BaseReceivePack { } commands.add(cmd); if (certParser.enabled()) { - // Must use raw line with optional newline so signed payload can be - // reconstructed. - certParser.addCommand(cmd, rawLine); + certParser.addCommand(cmd); } } } - static String chomp(String line) { - if (line != null && !line.isEmpty() - && line.charAt(line.length() - 1) == '\n') { - return line.substring(0, line.length() - 1); - } - return line; - } - static ReceiveCommand parseCommand(String line) throws PackProtocolException { if (line == null || line.length() < 83) { throw new PackProtocolException( 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 6dc4153d1..fdc70adc8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificate.java @@ -85,12 +85,11 @@ public class PushCertificate { private final String nonce; private final NonceStatus nonceStatus; private final List commands; - private final String rawCommands; private final String signature; PushCertificate(String version, PushCertificateIdent pusher, String pushee, String nonce, NonceStatus nonceStatus, List commands, - String rawCommands, String signature) { + String signature) { if (version == null || version.isEmpty()) { throw new IllegalArgumentException(MessageFormat.format( JGitText.get().pushCertificateInvalidField, VERSION)); @@ -112,8 +111,7 @@ public class PushCertificate { JGitText.get().pushCertificateInvalidField, "nonce status")); //$NON-NLS-1$ } - if (commands == null || commands.isEmpty() - || rawCommands == null || rawCommands.isEmpty()) { + if (commands == null || commands.isEmpty()) { throw new IllegalArgumentException(MessageFormat.format( JGitText.get().pushCertificateInvalidField, "command")); //$NON-NLS-1$ @@ -123,7 +121,7 @@ public class PushCertificate { JGitText.get().pushCertificateInvalidSignature); } if (!signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE) - || !signature.endsWith(PushCertificateParser.END_SIGNATURE)) { + || !signature.endsWith(PushCertificateParser.END_SIGNATURE + '\n')) { throw new IllegalArgumentException( JGitText.get().pushCertificateInvalidSignature); } @@ -133,7 +131,6 @@ public class PushCertificate { this.nonce = nonce; this.nonceStatus = nonceStatus; this.commands = commands; - this.rawCommands = rawCommands; this.signature = signature; } @@ -209,14 +206,18 @@ public class PushCertificate { * @since 4.1 */ public String toText() { - return new StringBuilder() + StringBuilder sb = new StringBuilder() .append(VERSION).append(' ').append(version).append('\n') .append(PUSHER).append(' ').append(getPusher()) .append('\n') .append(PUSHEE).append(' ').append(pushee).append('\n') .append(NONCE).append(' ').append(nonce).append('\n') - .append('\n') - .append(rawCommands) - .toString(); + .append('\n'); + for (ReceiveCommand cmd : commands) { + sb.append(cmd.getOldId().name()) + .append(' ').append(cmd.getNewId().name()) + .append(' ').append(cmd.getRefName()).append('\n'); + } + return sb.toString(); } } 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 661a0f094..b34fb9fc6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java @@ -42,7 +42,6 @@ */ package org.eclipse.jgit.transport; -import static org.eclipse.jgit.transport.BaseReceivePack.chomp; import static org.eclipse.jgit.transport.BaseReceivePack.parseCommand; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_PUSH_CERT; @@ -66,9 +65,9 @@ import org.eclipse.jgit.transport.PushCertificate.NonceStatus; */ public class PushCertificateParser { static final String BEGIN_SIGNATURE = - "-----BEGIN PGP SIGNATURE-----\n"; //$NON-NLS-1$ + "-----BEGIN PGP SIGNATURE-----"; //$NON-NLS-1$ static final String END_SIGNATURE = - "-----END PGP SIGNATURE-----\n"; //$NON-NLS-1$ + "-----END PGP SIGNATURE-----"; //$NON-NLS-1$ static final String VERSION = "certificate version"; //$NON-NLS-1$ @@ -80,7 +79,7 @@ public class PushCertificateParser { private static final String VERSION_0_1 = "0.1"; //$NON-NLS-1$ - private static final String END_CERT = "push-cert-end\n"; //$NON-NLS-1$ + private static final String END_CERT = "push-cert-end"; //$NON-NLS-1$ private boolean received; private String version; @@ -112,7 +111,6 @@ public class PushCertificateParser { private final NonceGenerator nonceGenerator; private final List commands; - private final StringBuilder rawCommands; PushCertificateParser(Repository into, SignedPushConfig cfg) { if (cfg != null) { @@ -124,7 +122,6 @@ public class PushCertificateParser { } db = into; commands = new ArrayList<>(); - rawCommands = new StringBuilder(); } /** @@ -139,8 +136,7 @@ public class PushCertificateParser { } try { return new PushCertificate(version, pusher, pushee, receivedNonce, - nonceStatus, Collections.unmodifiableList(commands), - rawCommands.toString(), signature); + nonceStatus, Collections.unmodifiableList(commands), signature); } catch (IllegalArgumentException e) { throw new IOException(e.getMessage(), e); } @@ -244,9 +240,9 @@ public class PushCertificateParser { * Read the PGP signature. *

* This method assumes the line - * {@code "-----BEGIN PGP SIGNATURE-----\n"} has already been parsed, - * and continues parsing until an {@code "-----END PGP SIGNATURE-----\n"} is - * found, followed by {@code "push-cert-end\n"}. + * {@code "-----BEGIN PGP SIGNATURE-----"} has already been parsed, + * and continues parsing until an {@code "-----END PGP SIGNATURE-----"} is + * found, followed by {@code "push-cert-end"}. * * @param pckIn * where we read the signature from. @@ -257,13 +253,13 @@ public class PushCertificateParser { public void receiveSignature(PacketLineIn pckIn) throws IOException { received = true; try { - StringBuilder sig = new StringBuilder(BEGIN_SIGNATURE); + StringBuilder sig = new StringBuilder(BEGIN_SIGNATURE).append('\n'); String line; - while (!(line = pckIn.readStringRaw()).equals(END_SIGNATURE)) { - sig.append(line); + while (!(line = pckIn.readString()).equals(END_SIGNATURE)) { + sig.append(line).append('\n'); } - signature = sig.append(END_SIGNATURE).toString(); - if (!pckIn.readStringRaw().equals(END_CERT)) { + signature = sig.append(END_SIGNATURE).append('\n').toString(); + if (!pckIn.readString().equals(END_CERT)) { throw new PackProtocolException( JGitText.get().pushCertificateInvalidSignature); } @@ -278,28 +274,23 @@ public class PushCertificateParser { * * @param cmd * the command. - * @param rawLine - * the exact line read from the wire that produced this - * command, including trailing newline if present. * @since 4.1 */ - public void addCommand(ReceiveCommand cmd, String rawLine) { + public void addCommand(ReceiveCommand cmd) { commands.add(cmd); - rawCommands.append(rawLine); } /** * Add a command to the signature. * - * @param rawLine - * the exact line read from the wire that produced this - * command, including trailing newline if present. + * @param line + * the line read from the wire that produced this + * command, with optional trailing newline already trimmed. * @throws PackProtocolException * if the raw line cannot be parsed to a command. * @since 4.0 */ - public void addCommand(String rawLine) throws PackProtocolException { - commands.add(parseCommand(chomp(rawLine))); - rawCommands.append(rawLine); + public void addCommand(String line) throws PackProtocolException { + commands.add(parseCommand(line)); } }