sshd: handle IdentitiesOnly with an SSH agent

If an SSH agent is used but "IdentitiesOnly yes" is set, only those
keys from the agent that correspond to one of the keys explicitly given
via an IdentityFile directive are to be used.

Implement this by filtering the list of keys obtained from the agent
against the list of IdentityFiles, each entry suffixed with ".pub".
Load the public keys from these files, and ignore all other keys from
the agent. Keys without ".pub" file are also ignored.

Apache MINA sshd has no operation to load only the public key from a
private key file, so we have to rely on *.pub files.

Bug: 577053
Change-Id: I75c2c0b3ce35781c933ec2944bd6da1b94f4caf9
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

2 files changed, 137 insertions, 32 deletions

diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthentication.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthentication.java
index 2996a221ce..bfe11cb745 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthentication.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthentication.java
@@ -12,25 +12,49 @@ package org.eclipse.jgit.internal.transport.sshd;
 import static java.text.MessageFormat.format;
 import static org.eclipse.jgit.transport.SshConstants.PUBKEY_ACCEPTED_ALGORITHMS;
 
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
+import java.nio.file.LinkOption;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
+import java.security.PublicKey;
+import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.stream.Collectors;
 
+import org.apache.sshd.agent.SshAgent;
+import org.apache.sshd.agent.SshAgentFactory;
 import org.apache.sshd.client.auth.pubkey.KeyAgentIdentity;
 import org.apache.sshd.client.auth.pubkey.PublicKeyIdentity;
 import org.apache.sshd.client.auth.pubkey.UserAuthPublicKey;
+import org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator;
 import org.apache.sshd.client.config.hosts.HostConfigEntry;
 import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.common.FactoryManager;
 import org.apache.sshd.common.NamedFactory;
+import org.apache.sshd.common.config.keys.AuthorizedKeyEntry;
+import org.apache.sshd.common.config.keys.KeyUtils;
+import org.apache.sshd.common.config.keys.PublicKeyEntryResolver;
 import org.apache.sshd.common.signature.Signature;
+import org.apache.sshd.common.signature.SignatureFactoriesManager;
 import org.eclipse.jgit.util.StringUtils;
 
 /**
  * Custom {@link UserAuthPublicKey} implementation for handling SSH config
- * PubkeyAcceptedAlgorithms.
+ * PubkeyAcceptedAlgorithms and interaction with the SSH agent.
  */
 public class JGitPublicKeyAuthentication extends UserAuthPublicKey {
 
+	private SshAgent agent;
+
+	private HostConfigEntry hostConfig;
+
 	JGitPublicKeyAuthentication(List<NamedFactory<Signature>> factories) {
 		super(factories);
 	}
@@ -43,7 +67,7 @@ public class JGitPublicKeyAuthentication extends UserAuthPublicKey {
 					+ rawSession.getClass().getCanonicalName());
 		}
 		JGitClientSession session = (JGitClientSession) rawSession;
-		HostConfigEntry hostConfig = session.getHostConfigEntry();
+		hostConfig = session.getHostConfigEntry();
 		// Set signature algorithms for public key authentication
 		String pubkeyAlgos = hostConfig.getProperty(PUBKEY_ACCEPTED_ALGORITHMS);
 		if (!StringUtils.isEmptyOrNull(pubkeyAlgos)) {
@@ -64,46 +88,126 @@ public class JGitPublicKeyAuthentication extends UserAuthPublicKey {
 		// If we don't set signature factories here, the default ones from the
 		// session will be used.
 		super.init(session, service);
-		// In sshd 2.7.0, we end up now with a key iterator that uses keys
-		// provided by an ssh-agent even if IdentitiesOnly is true. So if
-		// needed, filter out any KeyAgentIdentity.
-		if (hostConfig.isIdentitiesOnly()) {
-			Iterator<PublicKeyIdentity> original = keys;
-			// The original iterator will already have gotten the identities
-			// from the agent. Unfortunately there's nothing we can do about
-			// that; it'll have to be fixed upstream. (As will, ultimately,
-			// respecting isIdentitiesOnly().) At least we can simply not
-			// use the keys the agent provided.
-			//
-			// See https://issues.apache.org/jira/browse/SSHD-1218
-			keys = new Iterator<>() {
-
-				private PublicKeyIdentity value;
+	}
+
+	@Override
+	protected Iterator<PublicKeyIdentity> createPublicKeyIterator(
+			ClientSession session, SignatureFactoriesManager manager)
+			throws Exception {
+		agent = getAgent(session);
+		return new KeyIterator(session, manager);
+	}
+
+	private SshAgent getAgent(ClientSession session) throws Exception {
+		FactoryManager manager = Objects.requireNonNull(
+				session.getFactoryManager(), "No session factory manager"); //$NON-NLS-1$
+		SshAgentFactory factory = manager.getAgentFactory();
+		if (factory == null) {
+			return null;
+		}
+		return factory.createClient(session, manager);
+	}
+
+	@Override
+	protected void releaseKeys() throws IOException {
+		try {
+			if (agent != null) {
+				try {
+					agent.close();
+				} finally {
+					agent = null;
+				}
+			}
+		} finally {
+			super.releaseKeys();
+		}
+	}
+
+	private class KeyIterator extends UserAuthPublicKeyIterator {
+
+		private Iterable<? extends Map.Entry<PublicKey, String>> agentKeys;
+
+		// If non-null, all the public keys from explicitly given key files. Any
+		// agent key not matching one of these public keys will be ignored in
+		// getIdentities().
+		private Collection<PublicKey> identityFiles;
+
+		public KeyIterator(ClientSession session,
+				SignatureFactoriesManager manager)
+				throws Exception {
+			super(session, manager);
+		}
+
+		private List<PublicKey> getExplicitKeys(
+				Collection<String> explicitFiles) {
+			if (explicitFiles == null) {
+				return null;
+			}
+			return explicitFiles.stream().map(s -> {
+				try {
+					Path p = Paths.get(s + ".pub"); //$NON-NLS-1$
+					if (Files.isRegularFile(p, LinkOption.NOFOLLOW_LINKS)) {
+						return AuthorizedKeyEntry.readAuthorizedKeys(p).get(0)
+								.resolvePublicKey(null,
+										PublicKeyEntryResolver.IGNORING);
+					}
+				} catch (InvalidPathException | IOException
+						| GeneralSecurityException e) {
+					log.warn(format(SshdText.get().cannotReadPublicKey, s), e);
+				}
+				return null;
+			}).filter(Objects::nonNull).collect(Collectors.toList());
+		}
+
+		@Override
+		protected Iterable<KeyAgentIdentity> initializeAgentIdentities(
+				ClientSession session) throws IOException {
+			if (agent == null) {
+				return null;
+			}
+			agentKeys = agent.getIdentities();
+			if (hostConfig != null && hostConfig.isIdentitiesOnly()) {
+				identityFiles = getExplicitKeys(hostConfig.getIdentities());
+			}
+			return () -> new Iterator<>() {
+
+				private final Iterator<? extends Map.Entry<PublicKey, String>> iter = agentKeys
+						.iterator();
+
+				private Map.Entry<PublicKey, String> next;
 
 				@Override
 				public boolean hasNext() {
-					if (value != null) {
-						return true;
-					}
-					PublicKeyIdentity next = null;
-					while (original.hasNext()) {
-						next = original.next();
-						if (!(next instanceof KeyAgentIdentity)) {
-							value = next;
+					while (next == null && iter.hasNext()) {
+						Map.Entry<PublicKey, String> val = iter.next();
+						PublicKey pk = val.getKey();
+						// This checks against all explicit keys for any agent
+						// key, but since identityFiles.size() is typically 1,
+						// it should be fine.
+						if (identityFiles == null || identityFiles.stream()
+								.anyMatch(k -> KeyUtils.compareKeys(k, pk))) {
+							next = val;
 							return true;
 						}
+						if (log.isTraceEnabled()) {
+							log.trace(
+									"Ignoring SSH agent {} key not in explicit IdentityFile in SSH config: {}", //$NON-NLS-1$
+									KeyUtils.getKeyType(pk),
+									KeyUtils.getFingerPrint(pk));
+						}
 					}
-					return false;
+					return next != null;
 				}
 
 				@Override
-				public PublicKeyIdentity next() {
-					if (hasNext()) {
-						PublicKeyIdentity result = value;
-						value = null;
-						return result;
+				public KeyAgentIdentity next() {
+					if (!hasNext()) {
+						throw new NoSuchElementException();
 					}
-					throw new NoSuchElementException();
+					KeyAgentIdentity result = new KeyAgentIdentity(agent,
+							next.getKey(), next.getValue());
+					next = null;
+					return result;
 				}
 			};
 		}
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
index 00ee62d6dd..f7b6f6acaa 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
@@ -30,6 +30,7 @@ public final class SshdText extends TranslationBundle {
 	/***/ public String authenticationCanceled;
 	/***/ public String authenticationOnClosedSession;
 	/***/ public String closeListenerFailed;
+	/***/ public String cannotReadPublicKey;
 	/***/ public String configInvalidPath;
 	/***/ public String configInvalidPattern;
 	/***/ public String configInvalidPositive;