]> source.dussan.org Git - jgit.git/commitdiff
sshd: implement ssh config PubkeyAcceptedAlgorithms 41/178041/2
authorThomas Wolf <thomas.wolf@paranor.ch>
Thu, 18 Mar 2021 20:16:48 +0000 (21:16 +0100)
committerThomas Wolf <thomas.wolf@paranor.ch>
Fri, 19 Mar 2021 16:27:03 +0000 (17:27 +0100)
Apache MINA sshd 2.6.0 appears to use only the first appropriate
public key signature algorithm for a particular key. See [1]. For
RSA keys, that is rsa-sha2-512. This breaks authentication at servers
that only know the older (and deprecated) ssh-rsa algorithm.

With PubkeyAcceptedAlgorithms, users can re-order algorithms in
the ssh config file per host, if needed. Setting

  PubkeyAcceptedAlgorithms ^ssh-rsa

will put "ssh-rsa" at the front of the list of algorithms, and then
authentication at such servers with RSA keys works again.

[1] https://issues.apache.org/jira/browse/SSHD-1105

Bug: 572056
Change-Id: I86c3b93f05960c68936e80642965815926bb2532
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF
org.eclipse.jgit.ssh.apache.test/build.properties
org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java
org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java

index 30eb2bf8b681221432b00630860536c5060c4072..b03545352961b3ae8d3bd0d633bc4722b0e9aac4 100644 (file)
@@ -14,6 +14,7 @@ Import-Package: org.apache.sshd.client.config.hosts;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.helpers;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.keyprovider;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.session;version="[2.6.0,2.7.0)",
+ org.apache.sshd.common.signature;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.util.net;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.util.security;version="[2.6.0,2.7.0)",
  org.apache.sshd.core;version="[2.6.0,2.7.0)",
index 9ffa0caf782151dbf6dc831b27bd9c6013c748fc..406c5a768f627bc7dd0a0ebd4d409bf586e6df36 100644 (file)
@@ -3,3 +3,5 @@ output.. = bin/
 bin.includes = META-INF/,\
                .,\
                plugin.properties
+additional.bundles = org.apache.log4j,\
+                     org.slf4j.binding.log4j12
index 97f97f902887344d42793bf1d3caa2e4db807516..09d048b4fa4224a87745c49409e6cf2baa63a0d9 100644 (file)
@@ -47,7 +47,9 @@ import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.TransportException;
 import org.eclipse.jgit.junit.ssh.SshTestBase;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.transport.RemoteSession;
 import org.eclipse.jgit.transport.SshSessionFactory;
+import org.eclipse.jgit.transport.URIish;
 import org.eclipse.jgit.util.FS;
 import org.junit.Test;
 import org.junit.experimental.theories.Theories;
@@ -232,64 +234,89 @@ public class ApacheSshTest extends SshTestBase {
        }
 
        /**
-        * Creates a simple proxy server. Accepts only publickey authentication from
-        * the given user with the given key, allows all forwardings. Adds the
-        * proxy's host key to {@link #knownHosts}.
+        * Creates a simple SSH server without git setup.
         *
         * @param user
         *            to accept
         * @param userKey
         *            public key of that user at this server
-        * @param report
-        *            single-element array to report back the forwarded address.
-        * @return the started server
+        * @return the {@link SshServer}, not yet started
         * @throws Exception
         */
-       private SshServer createProxy(String user, File userKey,
-                       SshdSocketAddress[] report) throws Exception {
-               SshServer proxy = SshServer.setUpDefaultServer();
+       private SshServer createServer(String user, File userKey) throws Exception {
+               SshServer srv = SshServer.setUpDefaultServer();
                // Give the server its own host key
                KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA");
                generator.initialize(2048);
                KeyPair proxyHostKey = generator.generateKeyPair();
-               proxy.setKeyPairProvider(
+               srv.setKeyPairProvider(
                                session -> Collections.singletonList(proxyHostKey));
                // Allow (only) publickey authentication
-               proxy.setUserAuthFactories(Collections.singletonList(
+               srv.setUserAuthFactories(Collections.singletonList(
                                ServerAuthenticationManager.DEFAULT_USER_AUTH_PUBLIC_KEY_FACTORY));
                // Install the user's public key
                PublicKey userProxyKey = AuthorizedKeyEntry
                                .readAuthorizedKeys(userKey.toPath()).get(0)
                                .resolvePublicKey(null, PublicKeyEntryResolver.IGNORING);
-               proxy.setPublickeyAuthenticator(
+               srv.setPublickeyAuthenticator(
                                (userName, publicKey, session) -> user.equals(userName)
                                                && KeyUtils.compareKeys(userProxyKey, publicKey));
-               // Allow forwarding
-               proxy.setForwardingFilter(new StaticDecisionForwardingFilter(true) {
+               return srv;
+       }
 
-                       @Override
-                       protected boolean checkAcceptance(String request, Session session,
-                                       SshdSocketAddress target) {
-                               report[0] = target;
-                               return super.checkAcceptance(request, session, target);
-                       }
-               });
-               proxy.start();
+       /**
+        * Writes the server's host key to our knownhosts file.
+        *
+        * @param srv to register
+        * @throws Exception
+        */
+       private void registerServer(SshServer srv) throws Exception {
                // Add the proxy's host key to knownhosts
                try (BufferedWriter writer = Files.newBufferedWriter(
                                knownHosts.toPath(), StandardCharsets.US_ASCII,
                                StandardOpenOption.WRITE, StandardOpenOption.APPEND)) {
                        writer.append('\n');
                        KnownHostHashValue.appendHostPattern(writer, "localhost",
-                                       proxy.getPort());
+                                       srv.getPort());
                        writer.append(',');
                        KnownHostHashValue.appendHostPattern(writer, "127.0.0.1",
-                                       proxy.getPort());
+                                       srv.getPort());
                        writer.append(' ');
                        PublicKeyEntry.appendPublicKeyEntry(writer,
-                                       proxyHostKey.getPublic());
+                                       srv.getKeyPairProvider().loadKeys(null).iterator().next().getPublic());
                        writer.append('\n');
                }
+       }
+
+       /**
+        * Creates a simple proxy server. Accepts only publickey authentication from
+        * the given user with the given key, allows all forwardings. Adds the
+        * proxy's host key to {@link #knownHosts}.
+        *
+        * @param user
+        *            to accept
+        * @param userKey
+        *            public key of that user at this server
+        * @param report
+        *            single-element array to report back the forwarded address.
+        * @return the started server
+        * @throws Exception
+        */
+       private SshServer createProxy(String user, File userKey,
+                       SshdSocketAddress[] report) throws Exception {
+               SshServer proxy = createServer(user, userKey);
+               // Allow forwarding
+               proxy.setForwardingFilter(new StaticDecisionForwardingFilter(true) {
+
+                       @Override
+                       protected boolean checkAcceptance(String request, Session session,
+                                       SshdSocketAddress target) {
+                               report[0] = target;
+                               return super.checkAcceptance(request, session, target);
+                       }
+               });
+               proxy.start();
+               registerServer(proxy);
                return proxy;
        }
 
@@ -606,4 +633,35 @@ public class ApacheSshTest extends SshTestBase {
                        }
                }
        }
+
+       /**
+        * Tests that one can log in to an old server that doesn't handle
+        * rsa-sha2-512 if one puts ssh-rsa first in the client's list of public key
+        * signature algorithms.
+        *
+        * @see <a href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=572056">bug
+        *      572056</a>
+        * @throws Exception
+        *             on failure
+        */
+       @Test
+       public void testConnectAuthSshRsaPubkeyAcceptedAlgorithms()
+                       throws Exception {
+               try (SshServer oldServer = createServer(TEST_USER, publicKey1)) {
+                       oldServer.setSignatureFactoriesNames("ssh-rsa");
+                       oldServer.start();
+                       registerServer(oldServer);
+                       installConfig("Host server", //
+                                       "HostName localhost", //
+                                       "Port " + oldServer.getPort(), //
+                                       "User " + TEST_USER, //
+                                       "IdentityFile " + privateKey1.getAbsolutePath(), //
+                                       "PubkeyAcceptedAlgorithms ^ssh-rsa");
+                       RemoteSession session = getSessionFactory().getSession(
+                                       new URIish("ssh://server/doesntmatter"), null, FS.DETECTED,
+                                       10000);
+                       assertNotNull(session);
+                       session.disconnect();
+               }
+       }
 }
index f810fd40e4ab63fbbb4188934e5fdfa4e5a73602..16b5738332758ebe843c446c483d439e38cc8ff1 100644 (file)
@@ -5,8 +5,7 @@ configInvalidPath=Invalid path in ssh config key {0}: {1}
 configInvalidPattern=Invalid pattern in ssh config key {0}: {1}
 configInvalidPositive=Ssh config entry {0} must be a strictly positive number but is ''{1}''
 configInvalidProxyJump=Ssh config, host ''{0}'': Cannot parse ProxyJump ''{1}''
-configNoKnownHostKeyAlgorithms=No implementations for any of the algorithms ''{0}'' given in HostKeyAlgorithms in the ssh config; using the default.
-configNoRemainingHostKeyAlgorithms=Ssh config removed all host key algorithms: HostKeyAlgorithms ''{0}''
+configNoKnownAlgorithms=Ssh config ''{0}'' ''{1}'' resulted in empty list (none known, or all known removed); using default.
 configProxyJumpNotSsh=Non-ssh URI in ProxyJump ssh config
 configProxyJumpWithPath=ProxyJump ssh config: jump host specification must not have a path
 ftpCloseFailed=Closing the SFTP channel failed
index 66713ba632d0eb77878a19a53672a7871331fd57..8183a92b9fb3d0d6d079082db95302b364857db7 100644 (file)
@@ -21,6 +21,7 @@ import java.security.PublicKey;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -45,6 +46,7 @@ import org.eclipse.jgit.fnmatch.FileNameMatcher;
 import org.eclipse.jgit.internal.transport.sshd.proxy.StatefulProxyConnector;
 import org.eclipse.jgit.transport.CredentialsProvider;
 import org.eclipse.jgit.transport.SshConstants;
+import org.eclipse.jgit.util.StringUtils;
 
 /**
  * A {@link org.apache.sshd.client.session.ClientSession ClientSession} that can
@@ -201,48 +203,23 @@ public class JGitClientSession extends ClientSessionImpl {
        @Override
        protected String resolveAvailableSignaturesProposal(
                        FactoryManager manager) {
-               Set<String> defaultSignatures = new LinkedHashSet<>();
-               defaultSignatures.addAll(getSignatureFactoriesNames());
+               List<String> defaultSignatures = getSignatureFactoriesNames();
                HostConfigEntry config = resolveAttribute(
                                JGitSshClient.HOST_CONFIG_ENTRY);
-               String hostKeyAlgorithms = config
+               String algorithms = config
                                .getProperty(SshConstants.HOST_KEY_ALGORITHMS);
-               if (hostKeyAlgorithms != null && !hostKeyAlgorithms.isEmpty()) {
-                       char first = hostKeyAlgorithms.charAt(0);
-                       switch (first) {
-                       case '+':
-                               // Additions make not much sense -- it's either in
-                               // defaultSignatures already, or we have no implementation for
-                               // it. No point in proposing it.
-                               return String.join(",", defaultSignatures); //$NON-NLS-1$
-                       case '-':
-                               // This takes wildcard patterns!
-                               removeFromList(defaultSignatures,
-                                               SshConstants.HOST_KEY_ALGORITHMS,
-                                               hostKeyAlgorithms.substring(1));
-                               if (defaultSignatures.isEmpty()) {
-                                       // Too bad: user config error. Warn here, and then fail
-                                       // later.
-                                       log.warn(format(
-                                                       SshdText.get().configNoRemainingHostKeyAlgorithms,
-                                                       hostKeyAlgorithms));
-                               }
-                               return String.join(",", defaultSignatures); //$NON-NLS-1$
-                       default:
-                               // Default is overridden -- only accept the ones for which we do
-                               // have an implementation.
-                               List<String> newNames = filteredList(defaultSignatures,
-                                               hostKeyAlgorithms);
-                               if (newNames.isEmpty()) {
-                                       log.warn(format(
-                                                       SshdText.get().configNoKnownHostKeyAlgorithms,
-                                                       hostKeyAlgorithms));
-                                       // Use the default instead.
-                               } else {
-                                       return String.join(",", newNames); //$NON-NLS-1$
+               if (!StringUtils.isEmptyOrNull(algorithms)) {
+                       List<String> result = modifyAlgorithmList(defaultSignatures,
+                                       algorithms, SshConstants.HOST_KEY_ALGORITHMS);
+                       if (!result.isEmpty()) {
+                               if (log.isDebugEnabled()) {
+                                       log.debug(SshConstants.HOST_KEY_ALGORITHMS + ' ' + result);
                                }
-                               break;
+                               return String.join(",", result); //$NON-NLS-1$
                        }
+                       log.warn(format(SshdText.get().configNoKnownAlgorithms,
+                                       SshConstants.HOST_KEY_ALGORITHMS,
+                                       algorithms));
                }
                // No HostKeyAlgorithms; using default -- change order to put existing
                // keys first.
@@ -262,11 +239,67 @@ public class JGitClientSession extends ClientSessionImpl {
                                }
                        }
                        reordered.addAll(defaultSignatures);
+                       if (log.isDebugEnabled()) {
+                               log.debug(SshConstants.HOST_KEY_ALGORITHMS + ' ' + reordered);
+                       }
                        return String.join(",", reordered); //$NON-NLS-1$
                }
+               if (log.isDebugEnabled()) {
+                       log.debug(
+                                       SshConstants.HOST_KEY_ALGORITHMS + ' ' + defaultSignatures);
+               }
                return String.join(",", defaultSignatures); //$NON-NLS-1$
        }
 
+       /**
+        * Modifies a given algorithm list according to a list from the ssh config,
+        * including remove ('-') and reordering ('^') operators. Addition ('+') is
+        * not handled since we have no way of adding dynamically implementations,
+        * and the defaultList is supposed to contain all known implementations
+        * already.
+        *
+        * @param defaultList
+        *            to modify
+        * @param fromConfig
+        *            telling how to modify the {@code defaultList}, must not be
+        *            {@code null} or empty
+        * @param overrideKey
+        *            ssh config key; used for logging
+        * @return the modified list or {@code null} if {@code overrideKey} is not
+        *         set
+        */
+       public List<String> modifyAlgorithmList(List<String> defaultList,
+                       String fromConfig, String overrideKey) {
+               Set<String> defaults = new LinkedHashSet<>();
+               defaults.addAll(defaultList);
+               switch (fromConfig.charAt(0)) {
+               case '+':
+                       // Additions make not much sense -- it's either in
+                       // defaultList already, or we have no implementation for
+                       // it. No point in proposing it.
+                       return defaultList;
+               case '-':
+                       // This takes wildcard patterns!
+                       removeFromList(defaults, overrideKey, fromConfig.substring(1));
+                       return new ArrayList<>(defaults);
+               case '^':
+                       // Specified entries go to the front of the default list
+                       List<String> allSignatures = filteredList(defaults,
+                                       fromConfig.substring(1));
+                       Set<String> atFront = new HashSet<>(allSignatures);
+                       for (String sig : defaults) {
+                               if (!atFront.contains(sig)) {
+                                       allSignatures.add(sig);
+                               }
+                       }
+                       return allSignatures;
+               default:
+                       // Default is overridden -- only accept the ones for which we do
+                       // have an implementation.
+                       return filteredList(defaults, fromConfig);
+               }
+       }
+
        private void removeFromList(Set<String> current, String key,
                        String patterns) {
                for (String toRemove : patterns.split("\\s*,\\s*")) { //$NON-NLS-1$
index 74455dc808183d7d95076cb6a044baac736391d7..071e1979d3873a2634d46ada2d936639fd5f504e 100644 (file)
@@ -267,6 +267,24 @@ public class JGitSshClient extends SshClient {
                session.setUsername(username);
                session.setConnectAddress(address);
                session.setHostConfigEntry(hostConfig);
+               // Set signature algorithms for public key authentication
+               String pubkeyAlgos = hostConfig
+                               .getProperty(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS);
+               if (!StringUtils.isEmptyOrNull(pubkeyAlgos)) {
+                       List<String> signatures = getSignatureFactoriesNames();
+                       signatures = session.modifyAlgorithmList(signatures, pubkeyAlgos,
+                                       SshConstants.PUBKEY_ACCEPTED_ALGORITHMS);
+                       if (!signatures.isEmpty()) {
+                               if (log.isDebugEnabled()) {
+                                       log.debug(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS + ' '
+                                                       + signatures);
+                               }
+                               session.setSignatureFactoriesNames(signatures);
+                       } else {
+                               log.warn(format(SshdText.get().configNoKnownAlgorithms,
+                                               SshConstants.PUBKEY_ACCEPTED_ALGORITHMS, pubkeyAlgos));
+                       }
+               }
                if (session.getCredentialsProvider() == null) {
                        session.setCredentialsProvider(getCredentialsProvider());
                }
index 13bb3ebe7518ead0e22adf7f4d7e4a49f26d1444..4c4ff5949df814046cd564bce652168f88359ddc 100644 (file)
@@ -25,8 +25,7 @@ public final class SshdText extends TranslationBundle {
        /***/ public String configInvalidPattern;
        /***/ public String configInvalidPositive;
        /***/ public String configInvalidProxyJump;
-       /***/ public String configNoKnownHostKeyAlgorithms;
-       /***/ public String configNoRemainingHostKeyAlgorithms;
+       /***/ public String configNoKnownAlgorithms;
        /***/ public String configProxyJumpNotSsh;
        /***/ public String configProxyJumpWithPath;
        /***/ public String ftpCloseFailed;
index fff2938e5d9940b8eeb4317ad4c27d7a4b33c04d..be55cd1b81d3bc977bdac5c8f7be2e1f079ad7a4 100644 (file)
@@ -114,6 +114,14 @@ public final class SshConstants {
        /** Key in an ssh config file. */
        public static final String PREFERRED_AUTHENTICATIONS = "PreferredAuthentications";
 
+       /**
+        * Key in an ssh config file; defines signature algorithms for public key
+        * authentication as a comma-separated list.
+        *
+        * @since 5.11
+        */
+       public static final String PUBKEY_ACCEPTED_ALGORITHMS = "PubkeyAcceptedAlgorithms";
+
        /** Key in an ssh config file. */
        public static final String PROXY_COMMAND = "ProxyCommand";