Merge changes from topic 'sshd'

* changes:
  sshd: add missing javadoc in SshTestGitServer
  sshd: shared reference in JGitClientSession must be volatile
  sshd: correct the protocol version exchange

org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java

@@ -58,6 +58,7 @@ import java.util.Locale;
 
 import org.apache.sshd.common.NamedFactory;
 import org.apache.sshd.common.NamedResource;
+import org.apache.sshd.common.PropertyResolverUtils;
 import org.apache.sshd.common.SshConstants;
 import org.apache.sshd.common.config.keys.AuthorizedKeyEntry;
 import org.apache.sshd.common.config.keys.KeyUtils;
@@ -69,6 +70,7 @@ import org.apache.sshd.common.util.security.SecurityUtils;
 import org.apache.sshd.common.util.threads.CloseableExecutorService;
 import org.apache.sshd.common.util.threads.ThreadUtils;
 import org.apache.sshd.server.ServerAuthenticationManager;
+import org.apache.sshd.server.ServerFactoryManager;
 import org.apache.sshd.server.SshServer;
 import org.apache.sshd.server.auth.UserAuth;
 import org.apache.sshd.server.auth.gss.GSSAuthenticator;
@@ -336,12 +338,38 @@ public class SshTestGitServer {
 		server.stop(true);
 	}
 
+	/**
+	 * Sets the test user's public key on the server.
+	 *
+	 * @param key
+	 *            to set
+	 * @throws IOException
+	 *             if the file cannot be read
+	 * @throws GeneralSecurityException
+	 *             if the public key cannot be extracted from the file
+	 */
 	public void setTestUserPublicKey(Path key)
 			throws IOException, GeneralSecurityException {
 		this.testKey = AuthorizedKeyEntry.readAuthorizedKeys(key).get(0)
 				.resolvePublicKey(null, PublicKeyEntryResolver.IGNORING);
 	}
 
+	/**
+	 * Sets the lines the server sends before its server identification in the
+	 * initial protocol version exchange.
+	 *
+	 * @param lines
+	 *            to send
+	 * @since 5.5
+	 */
+	public void setPreamble(String... lines) {
+		if (lines != null && lines.length > 0) {
+			PropertyResolverUtils.updateProperty(this.server,
+					ServerFactoryManager.SERVER_EXTRA_IDENTIFICATION_LINES,
+					String.join("|", lines));
+		}
+	}
+
 	private class GitUploadPackCommand extends AbstractCommandSupport {
 
 		protected GitUploadPackCommand(String command,

org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF

@@ -7,7 +7,8 @@ Bundle-Version: 5.5.0.qualifier
 Bundle-Vendor: %Bundle-Vendor
 Bundle-Localization: plugin
 Bundle-RequiredExecutionEnvironment: JavaSE-1.8
-Import-Package: org.eclipse.jgit.internal.transport.sshd.proxy;version="[5.5.0,5.6.0)",
+Import-Package: org.eclipse.jgit.api.errors;version="[5.5.0,5.6.0)",
+ org.eclipse.jgit.internal.transport.sshd.proxy;version="[5.5.0,5.6.0)",
  org.eclipse.jgit.junit;version="[5.5.0,5.6.0)",
  org.eclipse.jgit.junit.ssh;version="[5.5.0,5.6.0)",
  org.eclipse.jgit.lib;version="[5.5.0,5.6.0)",

org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java

@@ -47,7 +47,7 @@ import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.util.Arrays;
-
+import org.eclipse.jgit.api.errors.TransportException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.transport.SshSessionFactory;
 import org.eclipse.jgit.transport.ssh.SshTestBase;
@@ -82,11 +82,10 @@ public class ApacheSshTest extends SshTestBase {
 		}
 	}
 
-	// Using an ed25519 (unencrypted) user key is tested in the super class in
-	// testSshKeys(). sshd 2.0.0 cannot yet read encrypted ed25519 keys.
-
 	@Test
 	public void testEd25519HostKey() throws Exception {
+		// Using ed25519 user identities is tested in the super class in
+		// testSshKeys().
 		File newHostKey = new File(getTemporaryDirectory(), "newhostkey");
 		copyTestResource("id_ed25519", newHostKey);
 		server.addHostKey(newHostKey.toPath(), true);
@@ -102,4 +101,60 @@ public class ApacheSshTest extends SshTestBase {
 				"IdentityFile " + privateKey1.getAbsolutePath());
 	}
 
+	@Test
+	public void testPreamble() throws Exception {
+		// Test that the client can deal with strange lines being sent before
+		// the server identification string.
+		StringBuilder b = new StringBuilder();
+		for (int i = 0; i < 257; i++) {
+			b.append('a');
+		}
+		server.setPreamble("A line with a \000 NUL",
+				"A long line: " + b.toString());
+		cloneWith(
+				"ssh://" + TEST_USER + "@localhost:" + testPort
+						+ "/doesntmatter",
+				defaultCloneDir, null,
+				"IdentityFile " + privateKey1.getAbsolutePath());
+	}
+
+	@Test
+	public void testLongPreamble() throws Exception {
+		// Test that the client can deal with a long (about 60k) preamble.
+		StringBuilder b = new StringBuilder();
+		for (int i = 0; i < 1024; i++) {
+			b.append('a');
+		}
+		String line = b.toString();
+		String[] lines = new String[60];
+		for (int i = 0; i < lines.length; i++) {
+			lines[i] = line;
+		}
+		server.setPreamble(lines);
+		cloneWith(
+				"ssh://" + TEST_USER + "@localhost:" + testPort
+						+ "/doesntmatter",
+				defaultCloneDir, null,
+				"IdentityFile " + privateKey1.getAbsolutePath());
+	}
+
+	@Test (expected = TransportException.class)
+	public void testHugePreamble() throws Exception {
+		// Test that the connection fails when the preamble is longer than 64k.
+		StringBuilder b = new StringBuilder();
+		for (int i = 0; i < 1024; i++) {
+			b.append('a');
+		}
+		String line = b.toString();
+		String[] lines = new String[70];
+		for (int i = 0; i < lines.length; i++) {
+			lines[i] = line;
+		}
+		server.setPreamble(lines);
+		cloneWith(
+				"ssh://" + TEST_USER + "@localhost:" + testPort
+						+ "/doesntmatter",
+				defaultCloneDir, null,
+				"IdentityFile " + privateKey1.getAbsolutePath());
+	}
 }

org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties

@@ -71,6 +71,9 @@ proxySocksPasswordTooLong=Password for proxy {0} must be at most 255 bytes long,
 proxySocksUnexpectedMessage=Unexpected message received from SOCKS5 proxy {0}; client state {1}: {2}
 proxySocksUnexpectedVersion=Expected SOCKS version 5, got {0}
 proxySocksUsernameTooLong=User name for proxy {0} must be at most 255 bytes long, is {1} bytes: {2}
+serverIdNotReceived=No server identification received within {0} bytes
+serverIdTooLong=Server identification is longer than 255 characters (including line ending): {0}
+serverIdWithNul=Server identification contains a NUL character: {0}
 sessionCloseFailed=Closing the session failed
 sshClosingDown=Apache MINA sshd session factory is closing down; cannot create new ssh sessions on this factory
 sshCommandTimeout={0} timed out after {1} seconds while opening the channel

org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2018, Thomas Wolf <thomas.wolf@paranor.ch>
+ * Copyright (C) 2018, 2019 Thomas Wolf <thomas.wolf@paranor.ch>
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -46,6 +46,7 @@ import static java.text.MessageFormat.format;
 
 import java.io.IOException;
 import java.net.SocketAddress;
+import java.nio.charset.StandardCharsets;
 import java.security.GeneralSecurityException;
 import java.security.PublicKey;
 import java.util.ArrayList;
@@ -60,11 +61,13 @@ import org.apache.sshd.client.keyverifier.KnownHostsServerKeyVerifier.HostEntryP
 import org.apache.sshd.client.keyverifier.ServerKeyVerifier;
 import org.apache.sshd.client.session.ClientSessionImpl;
 import org.apache.sshd.common.FactoryManager;
+import org.apache.sshd.common.PropertyResolverUtils;
 import org.apache.sshd.common.SshException;
 import org.apache.sshd.common.config.keys.KeyUtils;
 import org.apache.sshd.common.io.IoSession;
 import org.apache.sshd.common.io.IoWriteFuture;
 import org.apache.sshd.common.util.Readable;
+import org.apache.sshd.common.util.buffer.Buffer;
 import org.eclipse.jgit.errors.InvalidPatternException;
 import org.eclipse.jgit.fnmatch.FileNameMatcher;
 import org.eclipse.jgit.internal.transport.sshd.proxy.StatefulProxyConnector;
@@ -82,11 +85,20 @@ import org.eclipse.jgit.transport.SshConstants;
  */
 public class JGitClientSession extends ClientSessionImpl {
 
+	/**
+	 * Default setting for the maximum number of bytes to read in the initial
+	 * protocol version exchange. 64kb is what OpenSSH < 8.0 read; OpenSSH 8.0
+	 * changed it to 8Mb, but that seems excessive for the purpose stated in RFC
+	 * 4253. The Apache MINA sshd default in
+	 * {@link FactoryManager#DEFAULT_MAX_IDENTIFICATION_SIZE} is 16kb.
+	 */
+	private static final int DEFAULT_MAX_IDENTIFICATION_SIZE = 64 * 1024;
+
 	private HostConfigEntry hostConfig;
 
 	private CredentialsProvider credentialsProvider;
 
-	private StatefulProxyConnector proxyHandler;
+	private volatile StatefulProxyConnector proxyHandler;
 
 	/**
 	 * @param manager
@@ -332,4 +344,123 @@ public class JGitClientSession extends ClientSessionImpl {
 		return newNames;
 	}
 
+	@Override
+	protected boolean readIdentification(Buffer buffer) throws IOException {
+		// Propagate a failure from doReadIdentification.
+		// TODO: remove for sshd > 2.3.0; its doReadIdentification throws
+		// StreamCorruptedException instead of IllegalStateException.
+		try {
+			return super.readIdentification(buffer);
+		} catch (IllegalStateException e) {
+			throw new IOException(e.getLocalizedMessage(), e);
+		}
+	}
+
+	/**
+	 * Reads the RFC 4253, section 4.2 protocol version identification. The
+	 * Apache MINA sshd default implementation checks for NUL bytes also in any
+	 * preceding lines, whereas RFC 4253 requires such a check only for the
+	 * actual identification string starting with "SSH-". Likewise, the 255
+	 * character limit exists only for the identification string, not for the
+	 * preceding lines. CR-LF handling is also relaxed.
+	 *
+	 * @param buffer
+	 *            to read from
+	 * @param server
+	 *            whether we're an SSH server (should always be {@code false})
+	 * @return the lines read, with the server identification line last, or
+	 *         {@code null} if no identification line was found and more bytes
+	 *         are needed
+	 *
+	 * @see <a href="https://tools.ietf.org/html/rfc4253#section-4.2">RFC 4253,
+	 *      section 4.2</a>
+	 */
+	@Override
+	protected List<String> doReadIdentification(Buffer buffer, boolean server) {
+		if (server) {
+			// Should never happen. No translation; internal bug.
+			throw new IllegalStateException(
+					"doReadIdentification of client called with server=true"); //$NON-NLS-1$
+		}
+		int maxIdentSize = PropertyResolverUtils.getIntProperty(this,
+				FactoryManager.MAX_IDENTIFICATION_SIZE,
+				DEFAULT_MAX_IDENTIFICATION_SIZE);
+		int current = buffer.rpos();
+		int end = current + buffer.available();
+		if (current >= end) {
+			return null;
+		}
+		byte[] raw = buffer.array();
+		List<String> ident = new ArrayList<>();
+		int start = current;
+		boolean hasNul = false;
+		for (int i = current; i < end; i++) {
+			switch (raw[i]) {
+			case 0:
+				hasNul = true;
+				break;
+			case '\n':
+				int eol = 1;
+				if (i > start && raw[i - 1] == '\r') {
+					eol++;
+				}
+				String line = new String(raw, start, i + 1 - eol - start,
+						StandardCharsets.UTF_8);
+				start = i + 1;
+				if (log.isDebugEnabled()) {
+					log.debug(format("doReadIdentification({0}) line: ", this) + //$NON-NLS-1$
+							escapeControls(line));
+				}
+				ident.add(line);
+				if (line.startsWith("SSH-")) { //$NON-NLS-1$
+					if (hasNul) {
+						throw new IllegalStateException(
+								format(SshdText.get().serverIdWithNul,
+										escapeControls(line)));
+					}
+					if (line.length() + eol > 255) {
+						throw new IllegalStateException(
+								format(SshdText.get().serverIdTooLong,
+										escapeControls(line)));
+					}
+					buffer.rpos(start);
+					return ident;
+				}
+				// If this were a server, we could throw an exception here: a
+				// client is not supposed to send any extra lines before its
+				// identification string.
+				hasNul = false;
+				break;
+			default:
+				break;
+			}
+			if (i - current + 1 >= maxIdentSize) {
+				String msg = format(SshdText.get().serverIdNotReceived,
+						Integer.toString(maxIdentSize));
+				if (log.isDebugEnabled()) {
+					log.debug(msg);
+					log.debug(buffer.toHex());
+				}
+				throw new IllegalStateException(msg);
+			}
+		}
+		// Need more data
+		return null;
+	}
+
+	private static String escapeControls(String s) {
+		StringBuilder b = new StringBuilder();
+		int l = s.length();
+		for (int i = 0; i < l; i++) {
+			char ch = s.charAt(i);
+			if (Character.isISOControl(ch)) {
+				b.append(ch <= 0xF ? "\\u000" : "\\u00") //$NON-NLS-1$ //$NON-NLS-2$
+						.append(Integer.toHexString(ch));
+			} else {
+				b.append(ch);
+			}
+		}
+		return b.toString();
+	}
+
 }

org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java

@@ -83,6 +83,9 @@ public final class SshdText extends TranslationBundle {
 	/***/ public String proxySocksUnexpectedMessage;
 	/***/ public String proxySocksUnexpectedVersion;
 	/***/ public String proxySocksUsernameTooLong;
+	/***/ public String serverIdNotReceived;
+	/***/ public String serverIdTooLong;
+	/***/ public String serverIdWithNul;
 	/***/ public String sessionCloseFailed;
 	/***/ public String sshClosingDown;
 	/***/ public String sshCommandTimeout;