Skip to content

Commit

Permalink
Lean on Config.keyAlgorithms choosing between rsa-sha2-* and ssh-rsa (h…
Browse files Browse the repository at this point in the history
…ierynomus#742)

* Improve SshdContainer: log `docker build` to stdout, don't wait too long if container exited

* Fix hierynomus#740: Lean on Config.keyAlgorithms choosing between rsa-sha2-* and ssh-rsa

Previously, there was a heuristic that was choosing rsa-sha2-512 after receiving a host key of type RSA. It didn't work well when a server doesn't have an RSA host key.

OpenSSH 8.8 introduced a breaking change: it removed ssh-rsa from the default list of supported public key signature algorithms. SSHJ was unable to connect to OpenSSH 8.8 server if the server has an EcDSA or Ed25519 host key.

Current behaviour behaves the same as OpenSSH 8.8 client does. SSHJ doesn't try to determine rsa-sha2-* support on the fly. Instead, it looks only on `Config.getKeyAlgorithms()`, which may or may not contain ssh-rsa and rsa-sha2-* in any order.

Sorry, this commit mostly reverts changes from hierynomus#607.

* Introduce ConfigImpl.prioritizeSshRsaKeyAlgorithm to deal with broken backward compatibility

Co-authored-by: Jeroen van Erp <jeroen@hierynomus.com>
  • Loading branch information
vladimirlagunov and hierynomus authored Dec 6, 2021
1 parent d8697c2 commit 624747c
Show file tree
Hide file tree
Showing 13 changed files with 332 additions and 35 deletions.
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ dependencies {
testImplementation "org.apache.sshd:sshd-core:$sshdVersion"
testImplementation "org.apache.sshd:sshd-sftp:$sshdVersion"
testImplementation "org.apache.sshd:sshd-scp:$sshdVersion"
testRuntimeOnly "ch.qos.logback:logback-classic:1.2.6"
testImplementation "ch.qos.logback:logback-classic:1.2.6"
testImplementation 'org.glassfish.grizzly:grizzly-http-server:2.4.4'
testImplementation 'org.apache.httpcomponents:httpclient:4.5.9'
testImplementation 'org.testcontainers:testcontainers:1.16.2'
Expand Down
159 changes: 159 additions & 0 deletions src/itest/groovy/com/hierynomus/sshj/RsaShaKeySignatureTest.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/*
* Copyright (C)2009 - SSHJ Contributors
*
* Licensed 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 com.hierynomus.sshj

import com.hierynomus.sshj.key.KeyAlgorithms
import net.schmizz.sshj.Config
import net.schmizz.sshj.DefaultConfig
import org.testcontainers.images.builder.dockerfile.DockerfileBuilder
import spock.lang.Specification
import spock.lang.Unroll

import java.nio.file.Paths

/**
* Checks that SSHJ is able to work with OpenSSH 8.8, which removed ssh-rsa signature from the default setup.
*/
class RsaShaKeySignatureTest extends Specification {
private static final Map<String, KeyAlgorithms.Factory> SSH_HOST_KEYS_AND_FACTORIES = [
'ssh_host_ecdsa_256_key': KeyAlgorithms.ECDSASHANistp256(),
'ssh_host_ecdsa_384_key': KeyAlgorithms.ECDSASHANistp384(),
'ssh_host_ecdsa_521_key': KeyAlgorithms.ECDSASHANistp521(),
'ssh_host_ed25519_384_key': KeyAlgorithms.EdDSA25519(),
'ssh_host_rsa_2048_key': KeyAlgorithms.RSASHA512(),
]

private static void dockerfileBuilder(DockerfileBuilder it, String hostKey, String pubkeyAcceptedAlgorithms) {
it.from("archlinux:base")
it.run('yes | pacman -Sy core/openssh' +
' && (' +
' V=$(echo $(/usr/sbin/sshd -h 2>&1) | grep -o \'OpenSSH_[0-9][0-9]*[.][0-9][0-9]*p[0-9]\');' +
' if [[ "$V" < OpenSSH_8.8p1 ]]; then' +
' echo $V is too old 1>&2;' +
' exit 1;' +
' fi' +
')' +
' && set -o pipefail ' +
' && useradd --create-home sshj' +
' && echo \"sshj:ultrapassword\" | chpasswd')
it.add("authorized_keys", "/home/sshj/.ssh/")
it.add(hostKey, '/etc/ssh/')
it.run('chmod go-rwx /etc/ssh/ssh_host_*' +
' && chown -R sshj /home/sshj/.ssh' +
' && chmod -R go-rwx /home/sshj/.ssh')
it.expose(22)

def cmd = [
'/usr/sbin/sshd',
'-D',
'-e',
'-f', '/dev/null',
'-o', 'LogLevel=DEBUG2',
'-o', "HostKey=/etc/ssh/$hostKey",
]
if (pubkeyAcceptedAlgorithms != null) {
cmd += ['-o', "PubkeyAcceptedAlgorithms=$pubkeyAcceptedAlgorithms"]
}
it.cmd(cmd as String[])
}

private static SshdContainer makeSshdContainer(String hostKey, String pubkeyAcceptedAlgorithms) {
return new SshdContainer(new SshdContainer.DebugLoggingImageFromDockerfile()
.withFileFromPath("authorized_keys", Paths.get("src/itest/docker-image/authorized_keys"))
.withFileFromPath(hostKey, Paths.get("src/itest/docker-image/test-container/host_keys/$hostKey"))
.withDockerfileFromBuilder {
dockerfileBuilder(it, hostKey, pubkeyAcceptedAlgorithms)
})
}

@Unroll
def "connect to a server with host key #hostKey that does not support ssh-rsa"() {
given:
SshdContainer sshd = makeSshdContainer(hostKey, "rsa-sha2-512,rsa-sha2-256,ssh-ed25519")
sshd.start()

and:
Config config = new DefaultConfig()
config.keyAlgorithms = [
KeyAlgorithms.RSASHA512(),
KeyAlgorithms.RSASHA256(),
SSH_HOST_KEYS_AND_FACTORIES[hostKey],
]

when:
def sshClient = sshd.getConnectedClient(config)
sshClient.authPublickey("sshj", "src/itest/resources/keyfiles/id_rsa_opensshv1")

then:
sshClient.isAuthenticated()

cleanup:
sshClient?.disconnect()
sshd.stop()

where:
hostKey << SSH_HOST_KEYS_AND_FACTORIES.keySet()
}

@Unroll
def "connect to a default server with host key #hostKey using a default config"() {
given:
SshdContainer sshd = makeSshdContainer(hostKey, null)
sshd.start()

when:
def sshClient = sshd.getConnectedClient()
sshClient.authPublickey("sshj", "src/itest/resources/keyfiles/id_rsa_opensshv1")

then:
sshClient.isAuthenticated()

cleanup:
sshClient?.disconnect()
sshd.stop()

where:
hostKey << SSH_HOST_KEYS_AND_FACTORIES.keySet()
}

@Unroll
def "connect to a server with host key #hostkey that supports only ssh-rsa"() {
given:
SshdContainer sshd = makeSshdContainer(hostKey, "ssh-rsa,ssh-ed25519")
sshd.start()

and:
Config config = new DefaultConfig()
config.keyAlgorithms = [
KeyAlgorithms.SSHRSA(),
SSH_HOST_KEYS_AND_FACTORIES[hostKey],
]

when:
def sshClient = sshd.getConnectedClient(config)
sshClient.authPublickey("sshj", "src/itest/resources/keyfiles/id_rsa_opensshv1")

then:
sshClient.isAuthenticated()

cleanup:
sshClient.disconnect()
sshd.stop()

where:
hostKey << SSH_HOST_KEYS_AND_FACTORIES.keySet()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class SshServerWaitStrategy implements WaitStrategy {
@Override
public void waitUntilReady(WaitStrategyTarget waitStrategyTarget) {
long expectedEnd = System.nanoTime() + startupTimeout.toNanos();
while (true) {
while (waitStrategyTarget.isRunning()) {
long attemptStart = System.nanoTime();
IOException error = null;
byte[] buffer = new byte[7];
Expand Down
30 changes: 29 additions & 1 deletion src/itest/groovy/com/hierynomus/sshj/SshdContainer.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
*/
package com.hierynomus.sshj;

import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;
import net.schmizz.sshj.Config;
import net.schmizz.sshj.DefaultConfig;
import net.schmizz.sshj.SSHClient;
import net.schmizz.sshj.transport.verification.PromiscuousVerifier;
import org.jetbrains.annotations.NotNull;
import org.slf4j.LoggerFactory;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.images.builder.ImageFromDockerfile;
import org.testcontainers.images.builder.dockerfile.DockerfileBuilder;
import org.testcontainers.utility.DockerLoggerFactory;

import java.io.IOException;
import java.nio.file.Paths;
Expand All @@ -32,6 +36,20 @@
* A JUnit4 rule for launching a generic SSH server container.
*/
public class SshdContainer extends GenericContainer<SshdContainer> {
/**
* A workaround for strange logger names of testcontainers. They contain no dots, but contain slashes,
* square brackets, and even emoji. It's uneasy to set the logging level via the XML file of logback, the
* result would be less readable than the code below.
*/
public static class DebugLoggingImageFromDockerfile extends ImageFromDockerfile {
public DebugLoggingImageFromDockerfile() {
super();
Logger logger = (Logger) LoggerFactory.getILoggerFactory()
.getLogger(DockerLoggerFactory.getLogger(getDockerImageName()).getName());
logger.setLevel(Level.DEBUG);
}
}

public static class Builder {
public static final String DEFAULT_SSHD_CONFIG = "" +
"PermitRootLogin yes\n" +
Expand Down Expand Up @@ -95,7 +113,7 @@ public static void defaultDockerfileBuilder(@NotNull DockerfileBuilder builder)
}

private @NotNull Future<String> buildInner() {
return new ImageFromDockerfile()
return new DebugLoggingImageFromDockerfile()
.withDockerfileFromBuilder(Builder::defaultDockerfileBuilder)
.withFileFromPath(".", Paths.get("src/itest/docker-image"))
.withFileFromString("sshd_config", sshdConfig);
Expand All @@ -111,6 +129,16 @@ public SshdContainer(@NotNull Future<String> future) {
super(future);
withExposedPorts(22);
setWaitStrategy(new SshServerWaitStrategy());
withLogConsumer(outputFrame -> {
switch (outputFrame.getType()) {
case STDOUT:
logger().info("sshd stdout: {}", outputFrame.getUtf8String().stripTrailing());
break;
case STDERR:
logger().info("sshd stderr: {}", outputFrame.getUtf8String().stripTrailing());
break;
}
});
}

public SSHClient getConnectedClient(Config config) throws IOException {
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@

public class KeyAlgorithms {

public static List<String> SSH_RSA_SHA2_ALGORITHMS = Arrays.asList("rsa-sha2-512", "rsa-sha2-256");

public static Factory SSHRSA() { return new Factory("ssh-rsa", new SignatureRSA.FactorySSHRSA(), KeyType.RSA); }
public static Factory SSHRSACertV01() { return new Factory("ssh-rsa-cert-v01@openssh.com", new SignatureRSA.FactoryCERT(), KeyType.RSA_CERT); }
public static Factory RSASHA256() { return new Factory("rsa-sha2-256", new SignatureRSA.FactoryRSASHA256(), KeyType.RSA); }
Expand Down Expand Up @@ -61,6 +59,10 @@ public String getName() {
return algorithmName;
}

public KeyType getKeyType() {
return keyType;
}

@Override
public KeyAlgorithm create() {
return new BaseKeyAlgorithm(algorithmName, signatureFactory, keyType);
Expand Down
27 changes: 27 additions & 0 deletions src/main/java/net/schmizz/sshj/ConfigImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import net.schmizz.sshj.transport.random.Random;
import net.schmizz.sshj.userauth.keyprovider.FileKeyProvider;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

Expand Down Expand Up @@ -188,4 +189,30 @@ public boolean isVerifyHostKeyCertificates() {
public void setVerifyHostKeyCertificates(boolean value) {
verifyHostKeyCertificates = value;
}

/**
* Modern servers neglect the key algorithm ssh-rsa. OpenSSH 8.8 even dropped its support by default in favour
* of rsa-sha2-*. However, there are legacy servers like Apache SSHD that don't support the newer replacements
* for ssh-rsa.
*
* If ssh-rsa factory is in {@link #getKeyAlgorithms()}, this methods makes ssh-rsa key algorithm more preferred
* than any of rsa-sha2-*. Otherwise, nothing happens.
*/
public void prioritizeSshRsaKeyAlgorithm() {
List<Factory.Named<KeyAlgorithm>> keyAlgorithms = getKeyAlgorithms();
for (int sshRsaIndex = 0; sshRsaIndex < keyAlgorithms.size(); ++ sshRsaIndex) {
if ("ssh-rsa".equals(keyAlgorithms.get(sshRsaIndex).getName())) {
for (int i = 0; i < sshRsaIndex; ++i) {
final String algo = keyAlgorithms.get(i).getName();
if ("rsa-sha2-256".equals(algo) || "rsa-sha2-512".equals(algo)) {
keyAlgorithms = new ArrayList<>(keyAlgorithms);
keyAlgorithms.add(i, keyAlgorithms.remove(sshRsaIndex));
setKeyAlgorithms(keyAlgorithms);
break;
}
}
break;
}
}
}
}
1 change: 0 additions & 1 deletion src/main/java/net/schmizz/sshj/transport/KeyExchanger.java
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,6 @@ private void gotKexInit(SSHPacket buf)
negotiatedAlgs.getKeyExchangeAlgorithm());
transport.setHostKeyAlgorithm(Factory.Named.Util.create(transport.getConfig().getKeyAlgorithms(),
negotiatedAlgs.getSignatureAlgorithm()));
transport.setRSASHA2Support(negotiatedAlgs.getRSASHA2Support());

try {
kex.init(transport,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@ public final class NegotiatedAlgorithms {
private final String c2sComp;
private final String s2cComp;

private final boolean rsaSHA2Support;

NegotiatedAlgorithms(String kex, String sig, String c2sCipher, String s2cCipher, String c2sMAC, String s2cMAC,
String c2sComp, String s2cComp, boolean rsaSHA2Support) {
String c2sComp, String s2cComp) {
this.kex = kex;
this.sig = sig;
this.c2sCipher = c2sCipher;
Expand All @@ -38,7 +36,6 @@ public final class NegotiatedAlgorithms {
this.s2cMAC = s2cMAC;
this.c2sComp = c2sComp;
this.s2cComp = s2cComp;
this.rsaSHA2Support = rsaSHA2Support;
}

public String getKeyExchangeAlgorithm() {
Expand Down Expand Up @@ -73,10 +70,6 @@ public String getServer2ClientCompressionAlgorithm() {
return s2cComp;
}

public boolean getRSASHA2Support() {
return rsaSHA2Support;
}

@Override
public String toString() {
return ("[ " +
Expand All @@ -88,7 +81,6 @@ public String toString() {
"s2cMAC=" + s2cMAC + "; " +
"c2sComp=" + c2sComp + "; " +
"s2cComp=" + s2cComp + "; " +
"rsaSHA2Support=" + rsaSHA2Support +
" ]");
}

Expand Down
5 changes: 2 additions & 3 deletions src/main/java/net/schmizz/sshj/transport/Proposal.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package net.schmizz.sshj.transport;

import com.hierynomus.sshj.key.KeyAlgorithms;
import net.schmizz.sshj.Config;
import net.schmizz.sshj.common.Buffer;
import net.schmizz.sshj.common.Factory;
Expand Down Expand Up @@ -140,8 +139,8 @@ public NegotiatedAlgorithms negotiate(Proposal other)
firstMatch("Client2ServerCompressionAlgorithms", this.getClient2ServerCompressionAlgorithms(),
other.getClient2ServerCompressionAlgorithms()),
firstMatch("Server2ClientCompressionAlgorithms", this.getServer2ClientCompressionAlgorithms(),
other.getServer2ClientCompressionAlgorithms()),
other.getHostKeyAlgorithms().containsAll(KeyAlgorithms.SSH_RSA_SHA2_ALGORITHMS));
other.getServer2ClientCompressionAlgorithms())
);
}

private List<String> filterKnownHostKeyAlgorithms(List<String> configuredKeyAlgorithms, List<String> knownHostKeyAlgorithms) {
Expand Down
Loading

0 comments on commit 624747c

Please sign in to comment.