From ffc1f9b02618a59ee72298e9af15f64fe157fa8a Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Thu, 18 Mar 2021 21:16:48 +0100 Subject: [PATCH] sshd: implement ssh config PubkeyAcceptedAlgorithms 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 --- .../META-INF/MANIFEST.MF | 1 + .../build.properties | 2 + .../jgit/transport/sshd/ApacheSshTest.java | 108 ++++++++++++++---- .../transport/sshd/SshdText.properties | 3 +- .../transport/sshd/JGitClientSession.java | 107 +++++++++++------ .../transport/sshd/JGitSshClient.java | 18 +++ .../internal/transport/sshd/SshdText.java | 3 +- .../eclipse/jgit/transport/SshConstants.java | 8 ++ 8 files changed, 184 insertions(+), 66 deletions(-) diff --git a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF index 30eb2bf8b6..b035453529 100644 --- a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF +++ b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF @@ -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)", diff --git a/org.eclipse.jgit.ssh.apache.test/build.properties b/org.eclipse.jgit.ssh.apache.test/build.properties index 9ffa0caf78..406c5a768f 100644 --- a/org.eclipse.jgit.ssh.apache.test/build.properties +++ b/org.eclipse.jgit.ssh.apache.test/build.properties @@ -3,3 +3,5 @@ output.. = bin/ bin.includes = META-INF/,\ .,\ plugin.properties +additional.bundles = org.apache.log4j,\ + org.slf4j.binding.log4j12 diff --git a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java index 97f97f9028..09d048b4fa 100644 --- a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java +++ b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java @@ -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 bug + * 572056 + * @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(); + } + } } diff --git a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties index f810fd40e4..16b5738332 100644 --- a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties +++ b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties @@ -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 diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java index 66713ba632..8183a92b9f 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java @@ -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 defaultSignatures = new LinkedHashSet<>(); - defaultSignatures.addAll(getSignatureFactoriesNames()); + List 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 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 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 modifyAlgorithmList(List defaultList, + String fromConfig, String overrideKey) { + Set 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 allSignatures = filteredList(defaults, + fromConfig.substring(1)); + Set 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 current, String key, String patterns) { for (String toRemove : patterns.split("\\s*,\\s*")) { //$NON-NLS-1$ diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java index 74455dc808..071e1979d3 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java @@ -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 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()); } 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 13bb3ebe75..4c4ff5949d 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 @@ -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; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java index fff2938e5d..be55cd1b81 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java @@ -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"; -- 2.39.5