Skip to content

Commit 624747c

Browse files
vladimirlagunovhierynomus
vladimirlagunov
and
hierynomus
authored
Lean on Config.keyAlgorithms choosing between rsa-sha2-* and ssh-rsa (#742)
* Improve SshdContainer: log `docker build` to stdout, don't wait too long if container exited * Fix #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 #607. * Introduce ConfigImpl.prioritizeSshRsaKeyAlgorithm to deal with broken backward compatibility Co-authored-by: Jeroen van Erp <[email protected]>
1 parent d8697c2 commit 624747c

File tree

13 files changed

+332
-35
lines changed

13 files changed

+332
-35
lines changed

‎build.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ dependencies {
5353
testImplementation "org.apache.sshd:sshd-core:$sshdVersion"
5454
testImplementation "org.apache.sshd:sshd-sftp:$sshdVersion"
5555
testImplementation "org.apache.sshd:sshd-scp:$sshdVersion"
56-
testRuntimeOnly "ch.qos.logback:logback-classic:1.2.6"
56+
testImplementation "ch.qos.logback:logback-classic:1.2.6"
5757
testImplementation 'org.glassfish.grizzly:grizzly-http-server:2.4.4'
5858
testImplementation 'org.apache.httpcomponents:httpclient:4.5.9'
5959
testImplementation 'org.testcontainers:testcontainers:1.16.2'

‎src/itest/groovy/com/hierynomus/sshj/RsaShaKeySignatureTest.groovy

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
/*
2+
* Copyright (C)2009 - SSHJ Contributors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.hierynomus.sshj
17+
18+
import com.hierynomus.sshj.key.KeyAlgorithms
19+
import net.schmizz.sshj.Config
20+
import net.schmizz.sshj.DefaultConfig
21+
import org.testcontainers.images.builder.dockerfile.DockerfileBuilder
22+
import spock.lang.Specification
23+
import spock.lang.Unroll
24+
25+
import java.nio.file.Paths
26+
27+
/**
28+
* Checks that SSHJ is able to work with OpenSSH 8.8, which removed ssh-rsa signature from the default setup.
29+
*/
30+
class RsaShaKeySignatureTest extends Specification {
31+
private static final Map<String, KeyAlgorithms.Factory> SSH_HOST_KEYS_AND_FACTORIES = [
32+
'ssh_host_ecdsa_256_key': KeyAlgorithms.ECDSASHANistp256(),
33+
'ssh_host_ecdsa_384_key': KeyAlgorithms.ECDSASHANistp384(),
34+
'ssh_host_ecdsa_521_key': KeyAlgorithms.ECDSASHANistp521(),
35+
'ssh_host_ed25519_384_key': KeyAlgorithms.EdDSA25519(),
36+
'ssh_host_rsa_2048_key': KeyAlgorithms.RSASHA512(),
37+
]
38+
39+
private static void dockerfileBuilder(DockerfileBuilder it, String hostKey, String pubkeyAcceptedAlgorithms) {
40+
it.from("archlinux:base")
41+
it.run('yes | pacman -Sy core/openssh' +
42+
' && (' +
43+
' V=$(echo $(/usr/sbin/sshd -h 2>&1) | grep -o \'OpenSSH_[0-9][0-9]*[.][0-9][0-9]*p[0-9]\');' +
44+
' if [[ "$V" < OpenSSH_8.8p1 ]]; then' +
45+
' echo $V is too old 1>&2;' +
46+
' exit 1;' +
47+
' fi' +
48+
')' +
49+
' && set -o pipefail ' +
50+
' && useradd --create-home sshj' +
51+
' && echo \"sshj:ultrapassword\" | chpasswd')
52+
it.add("authorized_keys", "/home/sshj/.ssh/")
53+
it.add(hostKey, '/etc/ssh/')
54+
it.run('chmod go-rwx /etc/ssh/ssh_host_*' +
55+
' && chown -R sshj /home/sshj/.ssh' +
56+
' && chmod -R go-rwx /home/sshj/.ssh')
57+
it.expose(22)
58+
59+
def cmd = [
60+
'/usr/sbin/sshd',
61+
'-D',
62+
'-e',
63+
'-f', '/dev/null',
64+
'-o', 'LogLevel=DEBUG2',
65+
'-o', "HostKey=/etc/ssh/$hostKey",
66+
]
67+
if (pubkeyAcceptedAlgorithms != null) {
68+
cmd += ['-o', "PubkeyAcceptedAlgorithms=$pubkeyAcceptedAlgorithms"]
69+
}
70+
it.cmd(cmd as String[])
71+
}
72+
73+
private static SshdContainer makeSshdContainer(String hostKey, String pubkeyAcceptedAlgorithms) {
74+
return new SshdContainer(new SshdContainer.DebugLoggingImageFromDockerfile()
75+
.withFileFromPath("authorized_keys", Paths.get("src/itest/docker-image/authorized_keys"))
76+
.withFileFromPath(hostKey, Paths.get("src/itest/docker-image/test-container/host_keys/$hostKey"))
77+
.withDockerfileFromBuilder {
78+
dockerfileBuilder(it, hostKey, pubkeyAcceptedAlgorithms)
79+
})
80+
}
81+
82+
@Unroll
83+
def "connect to a server with host key #hostKey that does not support ssh-rsa"() {
84+
given:
85+
SshdContainer sshd = makeSshdContainer(hostKey, "rsa-sha2-512,rsa-sha2-256,ssh-ed25519")
86+
sshd.start()
87+
88+
and:
89+
Config config = new DefaultConfig()
90+
config.keyAlgorithms = [
91+
KeyAlgorithms.RSASHA512(),
92+
KeyAlgorithms.RSASHA256(),
93+
SSH_HOST_KEYS_AND_FACTORIES[hostKey],
94+
]
95+
96+
when:
97+
def sshClient = sshd.getConnectedClient(config)
98+
sshClient.authPublickey("sshj", "src/itest/resources/keyfiles/id_rsa_opensshv1")
99+
100+
then:
101+
sshClient.isAuthenticated()
102+
103+
cleanup:
104+
sshClient?.disconnect()
105+
sshd.stop()
106+
107+
where:
108+
hostKey << SSH_HOST_KEYS_AND_FACTORIES.keySet()
109+
}
110+
111+
@Unroll
112+
def "connect to a default server with host key #hostKey using a default config"() {
113+
given:
114+
SshdContainer sshd = makeSshdContainer(hostKey, null)
115+
sshd.start()
116+
117+
when:
118+
def sshClient = sshd.getConnectedClient()
119+
sshClient.authPublickey("sshj", "src/itest/resources/keyfiles/id_rsa_opensshv1")
120+
121+
then:
122+
sshClient.isAuthenticated()
123+
124+
cleanup:
125+
sshClient?.disconnect()
126+
sshd.stop()
127+
128+
where:
129+
hostKey << SSH_HOST_KEYS_AND_FACTORIES.keySet()
130+
}
131+
132+
@Unroll
133+
def "connect to a server with host key #hostkey that supports only ssh-rsa"() {
134+
given:
135+
SshdContainer sshd = makeSshdContainer(hostKey, "ssh-rsa,ssh-ed25519")
136+
sshd.start()
137+
138+
and:
139+
Config config = new DefaultConfig()
140+
config.keyAlgorithms = [
141+
KeyAlgorithms.SSHRSA(),
142+
SSH_HOST_KEYS_AND_FACTORIES[hostKey],
143+
]
144+
145+
when:
146+
def sshClient = sshd.getConnectedClient(config)
147+
sshClient.authPublickey("sshj", "src/itest/resources/keyfiles/id_rsa_opensshv1")
148+
149+
then:
150+
sshClient.isAuthenticated()
151+
152+
cleanup:
153+
sshClient.disconnect()
154+
sshd.stop()
155+
156+
where:
157+
hostKey << SSH_HOST_KEYS_AND_FACTORIES.keySet()
158+
}
159+
}

‎src/itest/groovy/com/hierynomus/sshj/SshServerWaitStrategy.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public class SshServerWaitStrategy implements WaitStrategy {
3535
@Override
3636
public void waitUntilReady(WaitStrategyTarget waitStrategyTarget) {
3737
long expectedEnd = System.nanoTime() + startupTimeout.toNanos();
38-
while (true) {
38+
while (waitStrategyTarget.isRunning()) {
3939
long attemptStart = System.nanoTime();
4040
IOException error = null;
4141
byte[] buffer = new byte[7];

‎src/itest/groovy/com/hierynomus/sshj/SshdContainer.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,18 @@
1515
*/
1616
package com.hierynomus.sshj;
1717

18+
import ch.qos.logback.classic.Level;
19+
import ch.qos.logback.classic.Logger;
1820
import net.schmizz.sshj.Config;
1921
import net.schmizz.sshj.DefaultConfig;
2022
import net.schmizz.sshj.SSHClient;
2123
import net.schmizz.sshj.transport.verification.PromiscuousVerifier;
2224
import org.jetbrains.annotations.NotNull;
25+
import org.slf4j.LoggerFactory;
2326
import org.testcontainers.containers.GenericContainer;
2427
import org.testcontainers.images.builder.ImageFromDockerfile;
2528
import org.testcontainers.images.builder.dockerfile.DockerfileBuilder;
29+
import org.testcontainers.utility.DockerLoggerFactory;
2630

2731
import java.io.IOException;
2832
import java.nio.file.Paths;
@@ -32,6 +36,20 @@
3236
* A JUnit4 rule for launching a generic SSH server container.
3337
*/
3438
public class SshdContainer extends GenericContainer<SshdContainer> {
39+
/**
40+
* A workaround for strange logger names of testcontainers. They contain no dots, but contain slashes,
41+
* square brackets, and even emoji. It's uneasy to set the logging level via the XML file of logback, the
42+
* result would be less readable than the code below.
43+
*/
44+
public static class DebugLoggingImageFromDockerfile extends ImageFromDockerfile {
45+
public DebugLoggingImageFromDockerfile() {
46+
super();
47+
Logger logger = (Logger) LoggerFactory.getILoggerFactory()
48+
.getLogger(DockerLoggerFactory.getLogger(getDockerImageName()).getName());
49+
logger.setLevel(Level.DEBUG);
50+
}
51+
}
52+
3553
public static class Builder {
3654
public static final String DEFAULT_SSHD_CONFIG = "" +
3755
"PermitRootLogin yes\n" +
@@ -95,7 +113,7 @@ public static void defaultDockerfileBuilder(@NotNull DockerfileBuilder builder)
95113
}
96114

97115
private @NotNull Future<String> buildInner() {
98-
return new ImageFromDockerfile()
116+
return new DebugLoggingImageFromDockerfile()
99117
.withDockerfileFromBuilder(Builder::defaultDockerfileBuilder)
100118
.withFileFromPath(".", Paths.get("src/itest/docker-image"))
101119
.withFileFromString("sshd_config", sshdConfig);
@@ -111,6 +129,16 @@ public SshdContainer(@NotNull Future<String> future) {
111129
super(future);
112130
withExposedPorts(22);
113131
setWaitStrategy(new SshServerWaitStrategy());
132+
withLogConsumer(outputFrame -> {
133+
switch (outputFrame.getType()) {
134+
case STDOUT:
135+
logger().info("sshd stdout: {}", outputFrame.getUtf8String().stripTrailing());
136+
break;
137+
case STDERR:
138+
logger().info("sshd stderr: {}", outputFrame.getUtf8String().stripTrailing());
139+
break;
140+
}
141+
});
114142
}
115143

116144
public SSHClient getConnectedClient(Config config) throws IOException {

‎src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,6 @@
2727

2828
public class KeyAlgorithms {
2929

30-
public static List<String> SSH_RSA_SHA2_ALGORITHMS = Arrays.asList("rsa-sha2-512", "rsa-sha2-256");
31-
3230
public static Factory SSHRSA() { return new Factory("ssh-rsa", new SignatureRSA.FactorySSHRSA(), KeyType.RSA); }
3331
public static Factory SSHRSACertV01() { return new Factory("[email protected]", new SignatureRSA.FactoryCERT(), KeyType.RSA_CERT); }
3432
public static Factory RSASHA256() { return new Factory("rsa-sha2-256", new SignatureRSA.FactoryRSASHA256(), KeyType.RSA); }
@@ -61,6 +59,10 @@ public String getName() {
6159
return algorithmName;
6260
}
6361

62+
public KeyType getKeyType() {
63+
return keyType;
64+
}
65+
6466
@Override
6567
public KeyAlgorithm create() {
6668
return new BaseKeyAlgorithm(algorithmName, signatureFactory, keyType);

‎src/main/java/net/schmizz/sshj/ConfigImpl.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import net.schmizz.sshj.transport.random.Random;
2727
import net.schmizz.sshj.userauth.keyprovider.FileKeyProvider;
2828

29+
import java.util.ArrayList;
2930
import java.util.Arrays;
3031
import java.util.List;
3132

@@ -188,4 +189,30 @@ public boolean isVerifyHostKeyCertificates() {
188189
public void setVerifyHostKeyCertificates(boolean value) {
189190
verifyHostKeyCertificates = value;
190191
}
192+
193+
/**
194+
* Modern servers neglect the key algorithm ssh-rsa. OpenSSH 8.8 even dropped its support by default in favour
195+
* of rsa-sha2-*. However, there are legacy servers like Apache SSHD that don't support the newer replacements
196+
* for ssh-rsa.
197+
*
198+
* If ssh-rsa factory is in {@link #getKeyAlgorithms()}, this methods makes ssh-rsa key algorithm more preferred
199+
* than any of rsa-sha2-*. Otherwise, nothing happens.
200+
*/
201+
public void prioritizeSshRsaKeyAlgorithm() {
202+
List<Factory.Named<KeyAlgorithm>> keyAlgorithms = getKeyAlgorithms();
203+
for (int sshRsaIndex = 0; sshRsaIndex < keyAlgorithms.size(); ++ sshRsaIndex) {
204+
if ("ssh-rsa".equals(keyAlgorithms.get(sshRsaIndex).getName())) {
205+
for (int i = 0; i < sshRsaIndex; ++i) {
206+
final String algo = keyAlgorithms.get(i).getName();
207+
if ("rsa-sha2-256".equals(algo) || "rsa-sha2-512".equals(algo)) {
208+
keyAlgorithms = new ArrayList<>(keyAlgorithms);
209+
keyAlgorithms.add(i, keyAlgorithms.remove(sshRsaIndex));
210+
setKeyAlgorithms(keyAlgorithms);
211+
break;
212+
}
213+
}
214+
break;
215+
}
216+
}
217+
}
191218
}

‎src/main/java/net/schmizz/sshj/transport/KeyExchanger.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,6 @@ private void gotKexInit(SSHPacket buf)
243243
negotiatedAlgs.getKeyExchangeAlgorithm());
244244
transport.setHostKeyAlgorithm(Factory.Named.Util.create(transport.getConfig().getKeyAlgorithms(),
245245
negotiatedAlgs.getSignatureAlgorithm()));
246-
transport.setRSASHA2Support(negotiatedAlgs.getRSASHA2Support());
247246

248247
try {
249248
kex.init(transport,

‎src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,8 @@ public final class NegotiatedAlgorithms {
2626
private final String c2sComp;
2727
private final String s2cComp;
2828

29-
private final boolean rsaSHA2Support;
30-
3129
NegotiatedAlgorithms(String kex, String sig, String c2sCipher, String s2cCipher, String c2sMAC, String s2cMAC,
32-
String c2sComp, String s2cComp, boolean rsaSHA2Support) {
30+
String c2sComp, String s2cComp) {
3331
this.kex = kex;
3432
this.sig = sig;
3533
this.c2sCipher = c2sCipher;
@@ -38,7 +36,6 @@ public final class NegotiatedAlgorithms {
3836
this.s2cMAC = s2cMAC;
3937
this.c2sComp = c2sComp;
4038
this.s2cComp = s2cComp;
41-
this.rsaSHA2Support = rsaSHA2Support;
4239
}
4340

4441
public String getKeyExchangeAlgorithm() {
@@ -73,10 +70,6 @@ public String getServer2ClientCompressionAlgorithm() {
7370
return s2cComp;
7471
}
7572

76-
public boolean getRSASHA2Support() {
77-
return rsaSHA2Support;
78-
}
79-
8073
@Override
8174
public String toString() {
8275
return ("[ " +
@@ -88,7 +81,6 @@ public String toString() {
8881
"s2cMAC=" + s2cMAC + "; " +
8982
"c2sComp=" + c2sComp + "; " +
9083
"s2cComp=" + s2cComp + "; " +
91-
"rsaSHA2Support=" + rsaSHA2Support +
9284
" ]");
9385
}
9486

‎src/main/java/net/schmizz/sshj/transport/Proposal.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
*/
1616
package net.schmizz.sshj.transport;
1717

18-
import com.hierynomus.sshj.key.KeyAlgorithms;
1918
import net.schmizz.sshj.Config;
2019
import net.schmizz.sshj.common.Buffer;
2120
import net.schmizz.sshj.common.Factory;
@@ -140,8 +139,8 @@ public NegotiatedAlgorithms negotiate(Proposal other)
140139
firstMatch("Client2ServerCompressionAlgorithms", this.getClient2ServerCompressionAlgorithms(),
141140
other.getClient2ServerCompressionAlgorithms()),
142141
firstMatch("Server2ClientCompressionAlgorithms", this.getServer2ClientCompressionAlgorithms(),
143-
other.getServer2ClientCompressionAlgorithms()),
144-
other.getHostKeyAlgorithms().containsAll(KeyAlgorithms.SSH_RSA_SHA2_ALGORITHMS));
142+
other.getServer2ClientCompressionAlgorithms())
143+
);
145144
}
146145

147146
private List<String> filterKnownHostKeyAlgorithms(List<String> configuredKeyAlgorithms, List<String> knownHostKeyAlgorithms) {

0 commit comments

Comments
 (0)