[wagon-3.x] Move wagon-ssh to the maintained JSch fork and stop a stray key shadowing the agent

wagon-providers/wagon-ssh-common/src/main/java/org/apache/maven/wagon/providers/ssh/ScpHelper.java

@@ -23,6 +23,8 @@
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
 import java.util.List;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
@@ -103,23 +105,63 @@ public static File getPrivateKey(AuthenticationInfo authenticationInfo) throws F
         return privateKey;
     }
 
+    /**
+     * The key types <code>ssh-keygen</code> produces, most recent first. <code>id_dsa</code> is not among
+     * them: ssh-dss has been disabled by default in OpenSSH for years, so a DSA key is the one least likely
+     * to be accepted by the server we are about to reach.
+     */
+    private static final String[] PRIVATE_KEY_NAMES = {"id_ed25519", "id_ecdsa", "id_rsa"};
+
+    /**
+     * The same names, with Ed25519 demoted to last. Used when this runtime cannot do Ed25519, so that a key
+     * it can actually use wins -- while still falling back to <code>id_ed25519</code> when that is the only
+     * key present, which is the right answer for wagon-ssh-external, where the host's own
+     * <code>scp</code> does the cryptography and the JVM's capabilities do not apply.
+     */
+    private static final String[] PRIVATE_KEY_NAMES_WITHOUT_ED25519 = {"id_ecdsa", "id_rsa", "id_ed25519"};
+
+    /**
+     * The names to look for, in order. Package-private so both orderings can be tested on any runtime.
+     */
+    static String[] preferredPrivateKeyNames(boolean ed25519Available) {
+        return ed25519Available ? PRIVATE_KEY_NAMES : PRIVATE_KEY_NAMES_WITHOUT_ED25519;
+    }
+
+    /**
+     * Whether this runtime can use an Ed25519 key. The JDK grew EdDSA in Java 15; before that a provider
+     * such as Bouncy Castle has to supply it, which JSch will use when it is on the class path.
+     */
+    private static boolean isEd25519Available() {
+        try {
+            KeyFactory.getInstance("Ed25519");
+            return true;
+        } catch (NoSuchAlgorithmException e) {
+            // not in this JDK; a provider may still supply it
+        }
+
+        try {
+            Class.forName("org.bouncycastle.jce.provider.BouncyCastleProvider");
+            return true;
+        } catch (ClassNotFoundException e) {
+            return false;
+        }
+    }
+
     private static File findPrivateKey() {
         String privateKeyDirectory = System.getProperty("wagon.privateKeyDirectory");
 
         if (privateKeyDirectory == null) {
             privateKeyDirectory = System.getProperty("user.home");
         }
 
-        File privateKey = new File(privateKeyDirectory, ".ssh/id_dsa");
-
-        if (!privateKey.exists()) {
-            privateKey = new File(privateKeyDirectory, ".ssh/id_rsa");
-            if (!privateKey.exists()) {
-                privateKey = null;
+        for (String name : preferredPrivateKeyNames(isEd25519Available())) {
+            File privateKey = new File(privateKeyDirectory, ".ssh/" + name);
+            if (privateKey.exists()) {
+                return privateKey;
             }
         }
 
-        return privateKey;
+        return null;
     }
 
     public static void createZip(List<String> files, File zipName, File basedir) throws IOException {

wagon-providers/wagon-ssh-common/src/test/java/org/apache/maven/wagon/providers/ssh/ScpHelperTest.java

@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.wagon.providers.ssh;
+
+import java.util.Arrays;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * The order in which a key file is picked out of <code>~/.ssh</code>.
+ */
+public class ScpHelperTest {
+
+    @Test
+    public void ed25519IsPreferredWhenTheRuntimeCanUseIt() {
+        assertArrayEquals(new String[] {"id_ed25519", "id_ecdsa", "id_rsa"}, ScpHelper.preferredPrivateKeyNames(true));
+    }
+
+    /**
+     * The JDK grew EdDSA in Java 15. On anything older, and without a provider supplying it, an Ed25519 key
+     * cannot be used at all -- so preferring it would fail the connection outright even though a usable
+     * id_rsa is sitting next to it.
+     */
+    @Test
+    public void ed25519IsDemotedWhenTheRuntimeCannotUseIt() {
+        assertArrayEquals(new String[] {"id_ecdsa", "id_rsa", "id_ed25519"}, ScpHelper.preferredPrivateKeyNames(false));
+    }
+
+    /**
+     * Demoted, not dropped: when it is the only key present it still has to be found. wagon-ssh-external
+     * shells out to the host's scp, which does its own cryptography, so the JVM's capabilities say nothing
+     * about whether the key is usable there.
+     */
+    @Test
+    public void ed25519IsStillOfferedWhenTheRuntimeCannotUseIt() {
+        assertTrue(Arrays.asList(ScpHelper.preferredPrivateKeyNames(false)).contains("id_ed25519"));
+    }
+}

wagon-providers/wagon-ssh/pom.xml

@@ -35,19 +35,10 @@ under the License.
 
   <dependencies>
     <dependency>
-      <groupId>com.jcraft</groupId>
+      <!-- maintained fork of com.jcraft:jsch, same com.jcraft.jsch package, with the SSH agent built in -->
+      <groupId>com.github.mwiede</groupId>
       <artifactId>jsch</artifactId>
-      <version>0.1.55</version>
-    </dependency>
-    <dependency>
-      <groupId>com.jcraft</groupId>
-      <artifactId>jsch.agentproxy.connector-factory</artifactId>
-      <version>0.0.9</version>
-    </dependency>
-    <dependency>
-      <groupId>com.jcraft</groupId>
-      <artifactId>jsch.agentproxy.jsch</artifactId>
-      <version>0.0.9</version>
+      <version>2.28.6</version>
     </dependency>
     <dependency>
       <groupId>org.codehaus.plexus</groupId>

wagon-providers/wagon-ssh/src/main/java/org/apache/maven/wagon/providers/ssh/jsch/AbstractJschWagon.java

@@ -29,22 +29,24 @@
 import java.util.List;
 import java.util.Properties;
 
+import com.jcraft.jsch.AgentConnector;
+import com.jcraft.jsch.AgentIdentityRepository;
+import com.jcraft.jsch.AgentProxyException;
 import com.jcraft.jsch.ChannelExec;
 import com.jcraft.jsch.HostKey;
 import com.jcraft.jsch.HostKeyRepository;
 import com.jcraft.jsch.IdentityRepository;
 import com.jcraft.jsch.JSch;
 import com.jcraft.jsch.JSchException;
+import com.jcraft.jsch.PageantConnector;
 import com.jcraft.jsch.Proxy;
 import com.jcraft.jsch.ProxyHTTP;
 import com.jcraft.jsch.ProxySOCKS5;
+import com.jcraft.jsch.SSHAgentConnector;
 import com.jcraft.jsch.Session;
 import com.jcraft.jsch.UIKeyboardInteractive;
 import com.jcraft.jsch.UserInfo;
-import com.jcraft.jsch.agentproxy.AgentProxyException;
-import com.jcraft.jsch.agentproxy.Connector;
-import com.jcraft.jsch.agentproxy.ConnectorFactory;
-import com.jcraft.jsch.agentproxy.RemoteIdentityRepository;
+import com.jcraft.jsch.WindowsSSHAgentConnector;
 import org.apache.maven.wagon.CommandExecutionException;
 import org.apache.maven.wagon.CommandExecutor;
 import org.apache.maven.wagon.ResourceDoesNotExistException;
@@ -123,23 +125,20 @@ public void openConnectionInternal() throws AuthenticationException {
             throw new AuthenticationException(e.getMessage());
         }
 
-        // can only pick one method of authentication
-        if (privateKey != null && privateKey.exists()) {
-            fireSessionDebug("Using private key: " + privateKey);
-            try {
-                sch.addIdentity(privateKey.getAbsolutePath(), authenticationInfo.getPassphrase());
-            } catch (JSchException e) {
-                throw new AuthenticationException("Cannot connect. Reason: " + e.getMessage(), e);
-            }
+        // Can only pick one method of authentication, so pick them in order of how deliberate they are:
+        // a key named in the settings first, then the agent, and only then a key file that merely happened
+        // to be lying in ~/.ssh. Letting a found key file outrank the agent means an agent is never reached
+        // on a machine that has one of those files, which is most of them.
+        boolean privateKeyConfigured = authenticationInfo.getPrivateKey() != null;
+
+        if (privateKeyConfigured && privateKey != null && privateKey.exists()) {
+            useIdentityFile(sch, privateKey);
         } else {
-            try {
-                Connector connector = ConnectorFactory.getDefault().createConnector();
-                if (connector != null) {
-                    IdentityRepository repo = new RemoteIdentityRepository(connector);
-                    sch.setIdentityRepository(repo);
-                }
-            } catch (AgentProxyException e) {
-                fireSessionDebug("Unable to connect to agent: " + e.toString());
+            IdentityRepository agent = agentIdentityRepository();
+            if (agent != null) {
+                sch.setIdentityRepository(agent);
+            } else if (privateKey != null && privateKey.exists()) {
+                useIdentityFile(sch, privateKey);
             }
         }
 
@@ -252,6 +251,78 @@ public void openConnectionInternal() throws AuthenticationException {
         }
     }
 
+    private void useIdentityFile(JSch sch, File privateKey) throws AuthenticationException {
+        fireSessionDebug("Using private key: " + privateKey);
+
+        // getPrivateKey() substitutes an empty passphrase when the settings name none, which is
+        // indistinguishable from "this key is not encrypted". Handing that on rejects every encrypted key
+        // outright. Passing null instead lets an unencrypted key through and leaves an encrypted one to be
+        // unlocked when it is used, which is where the user can still be asked for the passphrase.
+        String passphrase = authenticationInfo.getPassphrase();
+        if (passphrase != null && passphrase.isEmpty()) {
+            passphrase = null;
+        }
+
+        try {
+            sch.addIdentity(privateKey.getAbsolutePath(), passphrase);
+        } catch (JSchException e) {
+            throw new AuthenticationException("Cannot connect. Reason: " + e.getMessage(), e);
+        }
+    }
+
+    /**
+     * The identities held by a running SSH agent, or <code>null</code> when no agent can be reached or the
+     * one reached holds none, in which case the caller falls back to a key file.
+     * <p>
+     * Three kinds of agent are tried in turn: the OpenSSH agent named by <code>SSH_AUTH_SOCK</code>, the
+     * Win32 OpenSSH agent, and Pageant. Every one of them is optional and each has its own prerequisites --
+     * the first needs a Unix domain socket, which the JDK itself provides only from Java 16, and Pageant
+     * needs JNA, which is not a dependency of this provider. Each is therefore both constructed and used
+     * inside its own guard, so that a missing class is one skipped agent rather than a failure to load.
+     */
+    private IdentityRepository agentIdentityRepository() {
+        IdentityRepository repository = identitiesFrom(() -> new SSHAgentConnector());
+        if (repository == null) {
+            repository = identitiesFrom(() -> new WindowsSSHAgentConnector());
+        }
+        if (repository == null) {
+            repository = identitiesFrom(() -> new PageantConnector());
+        }
+        return repository;
+    }
+
+    /**
+     * Creates one kind of {@link AgentConnector}. The implementations must be written as lambda bodies
+     * rather than as {@code X::new} constructor references: a constructor reference resolves the class when
+     * the reference itself is evaluated, which happens at the call site and so outside the guard in
+     * {@link #identitiesFrom}, letting a {@link NoClassDefFoundError} escape it. A lambda body compiles to a
+     * synthetic method, so the class is not mentioned until that body runs, which is inside the guard.
+     */
+    private interface AgentConnectorFactory {
+        AgentConnector create() throws AgentProxyException;
+    }
+
+    private IdentityRepository identitiesFrom(AgentConnectorFactory factory) {
+        try {
+            AgentConnector connector = factory.create();
+            if (!connector.isAvailable()) {
+                return null;
+            }
+
+            AgentIdentityRepository repository = new AgentIdentityRepository(connector);
+            if (repository.getIdentities().isEmpty()) {
+                fireSessionDebug(connector.getName() + " holds no identities");
+                return null;
+            }
+
+            fireSessionDebug("Using the identities held by " + connector.getName());
+            return repository;
+        } catch (AgentProxyException | RuntimeException | LinkageError e) {
+            fireSessionDebug("Unable to use an SSH agent: " + e);
+            return null;
+        }
+    }
+
     public void closeConnection() {
         if (session != null) {
             session.disconnect();