Fix matching of pubkeys to key algorithms (#607)

* Fix matching of pubkeys to key algorithms Allow all configured key algorithms for pubkey authentication, even if these algorithms are not supported as host key algorithms by the server. Preference is given to the modern rsa-sha2-* signature algorithms if the server indicates support for them as host keys signature algorithms. * Replace Boolean with primitive boolean * Add integration tests for ecdsa-sha2-nistp384/521 * Remove redundant import * Clean up Transport interface Co-authored-by: Jeroen van Erp <jeroen@hierynomus.com>
hierynomus · Jul 28, 2020 · 6becee1 · 6becee1
1 parent 4b1619d
commit 6becee1
Show file tree

Hide file tree

Showing 13 changed files with 86 additions and 34 deletions.
diff --git a/src/itest/docker-image/authorized_keys b/src/itest/docker-image/authorized_keys
@@ -5,3 +5,5 @@ ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJ8ww4hJG/gHJYdkjTTBDF1GNz+228nuWprPV+NbQauA
 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOaWrwt3drIOjeBq2LSHRavxAT7ja2f+5soOUJl/zKSI ajvanerp@Heimdall.xebialabs.com
 ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAoZ9l6Tkm2aL1tSBy2yw4xU5s8BE9MfqS/4J7DzvsYJxF6oQmTIjmStuhH/CT7UjuDtKXdXZUsIhKtafiizxGO8kHSzKDeitpth2RSr8ddMzZKyD6RNs7MfsgjA3UTtrrSrCXEY6O43S2cnuJrWzkPxtwxaQ3zOvDbS2tiulzyq0VzYmuhA/a4CyuQtJBuu+P2oqmu6pU/VB6IzONpvBvYbNPsH1WDmP7zko5wHPihXPCliztspKxS4DRtOZ7BGXyvg44UmIy0Kf4jOkaBV/eCCA4qH7ZHz71/5ceMOpszPcNOEmLGGYhwI+P3OuGMpkrSAv1f8IY6R8spZNncP6UaQ== no-passphrase
 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDKRyZAtOJJfAhPU6xE6ZXY564vwErAI3n3Yn4lTHL9bxev9Ily6eCqPLcV0WbSV04pztngFn9MjT7yb8mcXheHpIaWEH569sMpmpOtyfn4p68SceuXBGyyPGMIcfOTknkASd1JYSD4EPkd9rZmCzcx3vEnLu8ChnA/G221xSVQ5VC/jD/c/CgNUayhQ+xbn57qHKKtZwfTa21QmwIabGYJNwlVjlKTCdddeVnZfKqKrG7cxHQApsxd21rhM9IT/C/f4Y/Tx3WUUVeam0iZ265oiPHoPALqJIWSQIUheRYAxYAQqJwSQ0Or9MM8XXun2Iy3RUSGk6eIvrCsFbNURsHNs7Pu0UnpYv6FZ3vCkFep/1pAT6fQvY7pDOOWDHKXArD4watc9gIWaQBH73wDW/KgBcnMRSoGWgQjsYqIamP4oV1+HqUI3lRAsXZaX+eiBGt3+3A5KebP27UJ1YUwhwlzs7wzTKaCu0OaL+hOsP1F2AxAa995bgFksMd23645ux3YCJKXG4sGpJ1Z/Hs49K72gv+QjLZVxXqY623c8+3OUhlixqoEFd4iG7UMc5a552ch/VA+jaspmLZoFhPz99aBRVb1oCSPxSwLw+Q/wxv6pZmT+14rqTzY2farjU53hM+CsUPh7dnWXhGG7RuA5wCdeOXOYjuksfzAoHIZhPqTgQ== ajvanerp@Heimdall.local
+ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBMvfRYSe44VQGwxexOMibcM3+fWeUP1jrBofOxFDRRrzRF8dK/vll2svqTPXMRnITnT1UoemEcB5OHtvH4hzfh/HFeDxJ5S7UncYxoClTSa8MeMFG2Zj9CoUZs1SHbwSGg== root@sshj
+ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAHquUYgkU9wJrcxDWVtdqtfqf6SBDdPDRxwDl7OCohV2UNu2KdjJwSj8j0fsPeMdHjSiv9OCnHYrVilQ+W5WW5q5wGXwk10oIcV0JJscohLA0nS7mKinBrxUwVHnNZbPExFciicnEArcYRb1BuT7HF8hfjuSSpWS0rob6kloSSi/jV7ZA== root@sshj
diff --git a/src/itest/groovy/com/hierynomus/sshj/IntegrationSpec.groovy b/src/itest/groovy/com/hierynomus/sshj/IntegrationSpec.groovy
@@ -77,6 +77,8 @@ class IntegrationSpec extends IntegrationBaseSpec {
         "id_ed25519_opensshv1_protected" | "sshjtest"
         "id_rsa" | null
         "id_rsa_opensshv1" | null
+        "id_ecdsa_nistp384_opensshv1" | null
+        "id_ecdsa_nistp521_opensshv1" | null
     }
 
    def "should not authenticate with wrong key"() {

diff --git a/src/itest/resources/keyfiles/id_ecdsa_nistp384_opensshv1 b/src/itest/resources/keyfiles/id_ecdsa_nistp384_opensshv1
@@ -0,0 +1,10 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAiAAAABNlY2RzYS
+1zaGEyLW5pc3RwMzg0AAAACG5pc3RwMzg0AAAAYQTL30WEnuOFUBsMXsTjIm3DN/n1nlD9
+Y6waHzsRQ0Ua80RfHSv75ZdrL6kz1zEZyE509VKHphHAeTh7bx+Ic34fxxXg8SeUu1J3GM
+aApU0mvDHjBRtmY/QqFGbNUh28EhoAAADYHWlHLx1pRy8AAAATZWNkc2Etc2hhMi1uaXN0
+cDM4NAAAAAhuaXN0cDM4NAAAAGEEy99FhJ7jhVAbDF7E4yJtwzf59Z5Q/WOsGh87EUNFGv
+NEXx0r++WXay+pM9cxGchOdPVSh6YRwHk4e28fiHN+H8cV4PEnlLtSdxjGgKVNJrwx4wUb
+ZmP0KhRmzVIdvBIaAAAAMQD3sx28SrtkuhN+Yu06BAoFLMMgneIqguM3jowaz0LWfP1Nhx
+Rnh9tNKM6YYvygCggAAAAPZmhlbm5la2VATGFwdG9w
+-----END OPENSSH PRIVATE KEY-----
diff --git a/src/itest/resources/keyfiles/id_ecdsa_nistp521_opensshv1 b/src/itest/resources/keyfiles/id_ecdsa_nistp521_opensshv1
@@ -0,0 +1,12 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAArAAAABNlY2RzYS
+1zaGEyLW5pc3RwNTIxAAAACG5pc3RwNTIxAAAAhQQB6rlGIJFPcCa3MQ1lbXarX6n+kgQ3
+Tw0ccA5ezgqIVdlDbtinYycEo/I9H7D3jHR40or/Tgpx2K1YpUPluVluaucBl8JNdKCHFd
+CSbHKISwNJ0u5iopwa8VMFR5zWWzxMRXIonJxAK3GEW9Qbk+xxfIX47kkqVktK6G+pJaEk
+ov41e2QAAAEQwljQZcJY0GUAAAATZWNkc2Etc2hhMi1uaXN0cDUyMQAAAAhuaXN0cDUyMQ
+AAAIUEAeq5RiCRT3AmtzENZW12q1+p/pIEN08NHHAOXs4KiFXZQ27Yp2MnBKPyPR+w94x0
+eNKK/04KcditWKVD5blZbmrnAZfCTXSghxXQkmxyiEsDSdLuYqKcGvFTBUec1ls8TEVyKJ
+ycQCtxhFvUG5PscXyF+O5JKlZLSuhvqSWhJKL+NXtkAAAAQVXt20tSeLzMU1U2nMv8CEEY
+Oyl1WIkGAcRatDBAfsE0+NcJ/eSbPXywWAqCzOElQ5ftNFz9t1kNXwW5qiLaaIBpAAAAD2
+ZoZW5uZWtlQExhcHRvcAECAwQ=
+-----END OPENSSH PRIVATE KEY-----
diff --git a/src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java b/src/main/java/com/hierynomus/sshj/key/KeyAlgorithms.java
@@ -22,8 +22,13 @@
 import net.schmizz.sshj.signature.SignatureECDSA;
 import net.schmizz.sshj.signature.SignatureRSA;
 
+import java.util.Arrays;
+import java.util.List;
+
 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); }

diff --git a/src/main/java/net/schmizz/sshj/transport/KeyExchanger.java b/src/main/java/net/schmizz/sshj/transport/KeyExchanger.java
@@ -231,12 +231,9 @@ private void gotKexInit(SSHPacket buf)
         }
         kex = Factory.Named.Util.create(transport.getConfig().getKeyExchangeFactories(),
                                         negotiatedAlgs.getKeyExchangeAlgorithm());
-
-        List<KeyAlgorithm> keyAlgorithms = new ArrayList<KeyAlgorithm>();
-        for (String signatureAlgorithm : negotiatedAlgs.getSignatureAlgorithms()) {
-            keyAlgorithms.add(Factory.Named.Util.create(transport.getConfig().getKeyAlgorithms(), signatureAlgorithm));
-        }
-        transport.setKeyAlgorithms(keyAlgorithms);
+        transport.setHostKeyAlgorithm(Factory.Named.Util.create(transport.getConfig().getKeyAlgorithms(),
+                                      negotiatedAlgs.getSignatureAlgorithm()));
+        transport.setRSASHA2Support(negotiatedAlgs.getRSASHA2Support());
 
         try {
             kex.init(transport,

diff --git a/src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java b/src/main/java/net/schmizz/sshj/transport/NegotiatedAlgorithms.java
@@ -15,37 +15,38 @@
  */
 package net.schmizz.sshj.transport;
 
-import java.util.List;
-
 public final class NegotiatedAlgorithms {
 
     private final String kex;
-    private final List<String> availableSigs;
+    private final String sig;
     private final String c2sCipher;
     private final String s2cCipher;
     private final String c2sMAC;
     private final String s2cMAC;
     private final String c2sComp;
     private final String s2cComp;
 
-    NegotiatedAlgorithms(String kex, List<String> availableSigs, String c2sCipher, String s2cCipher, String c2sMAC, String s2cMAC,
-                         String c2sComp, 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) {
         this.kex = kex;
-        this.availableSigs = availableSigs;
+        this.sig = sig;
         this.c2sCipher = c2sCipher;
         this.s2cCipher = s2cCipher;
         this.c2sMAC = c2sMAC;
         this.s2cMAC = s2cMAC;
         this.c2sComp = c2sComp;
         this.s2cComp = s2cComp;
+        this.rsaSHA2Support = rsaSHA2Support;
     }
 
     public String getKeyExchangeAlgorithm() {
         return kex;
     }
 
-    public List<String> getSignatureAlgorithms() {
-        return availableSigs;
+    public String getSignatureAlgorithm() {
+        return sig;
     }
 
     public String getClient2ServerCipherAlgorithm() {
@@ -72,17 +73,22 @@ public String getServer2ClientCompressionAlgorithm() {
         return s2cComp;
     }
 
+    public boolean getRSASHA2Support() {
+        return rsaSHA2Support;
+    }
+
     @Override
     public String toString() {
         return ("[ " +
                 "kex=" + kex + "; " +
-                "availableSigs=" + availableSigs + "; " +
+                "sig=" + sig + "; " +
                 "c2sCipher=" + c2sCipher + "; " +
                 "s2cCipher=" + s2cCipher + "; " +
                 "c2sMAC=" + c2sMAC + "; " +
                 "s2cMAC=" + s2cMAC + "; " +
                 "c2sComp=" + c2sComp + "; " +
-                "s2cComp=" + s2cComp +
+                "s2cComp=" + s2cComp + "; " +
+                "rsaSHA2Support=" + rsaSHA2Support +
                 " ]");
     }
 

diff --git a/src/main/java/net/schmizz/sshj/transport/Proposal.java b/src/main/java/net/schmizz/sshj/transport/Proposal.java
@@ -15,6 +15,7 @@
  */
 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;
@@ -91,7 +92,7 @@ public List<String> getKeyExchangeAlgorithms() {
         return kex;
     }
 
-    public List<String> getSignatureAlgorithms() {
+    public List<String> getHostKeyAlgorithms() {
         return sig;
     }
 
@@ -127,13 +128,14 @@ public NegotiatedAlgorithms negotiate(Proposal other)
             throws TransportException {
         return new NegotiatedAlgorithms(
                 firstMatch(this.getKeyExchangeAlgorithms(), other.getKeyExchangeAlgorithms()),
-                allMatch(this.getSignatureAlgorithms(), other.getSignatureAlgorithms()),
+                firstMatch(this.getHostKeyAlgorithms(), other.getHostKeyAlgorithms()),
                 firstMatch(this.getClient2ServerCipherAlgorithms(), other.getClient2ServerCipherAlgorithms()),
                 firstMatch(this.getServer2ClientCipherAlgorithms(), other.getServer2ClientCipherAlgorithms()),
                 firstMatch(this.getClient2ServerMACAlgorithms(), other.getClient2ServerMACAlgorithms()),
                 firstMatch(this.getServer2ClientMACAlgorithms(), other.getServer2ClientMACAlgorithms()),
                 firstMatch(this.getClient2ServerCompressionAlgorithms(), other.getClient2ServerCompressionAlgorithms()),
-                firstMatch(this.getServer2ClientCompressionAlgorithms(), other.getServer2ClientCompressionAlgorithms())
+                firstMatch(this.getServer2ClientCompressionAlgorithms(), other.getServer2ClientCompressionAlgorithms()),
+                other.getHostKeyAlgorithms().containsAll(KeyAlgorithms.SSH_RSA_SHA2_ALGORITHMS)
         );
     }
 

diff --git a/src/main/java/net/schmizz/sshj/transport/Transport.java b/src/main/java/net/schmizz/sshj/transport/Transport.java
@@ -238,5 +238,6 @@ long write(SSHPacket payload)
      */
     void die(Exception e);
 
-    KeyAlgorithm getKeyAlgorithm(KeyType keyType) throws TransportException;
+    KeyAlgorithm getHostKeyAlgorithm();
+    KeyAlgorithm getClientKeyAlgorithm(KeyType keyType) throws TransportException;
 }
diff --git a/src/main/java/net/schmizz/sshj/transport/TransportImpl.java b/src/main/java/net/schmizz/sshj/transport/TransportImpl.java
@@ -16,6 +16,7 @@
 package net.schmizz.sshj.transport;
 
 import com.hierynomus.sshj.key.KeyAlgorithm;
+import com.hierynomus.sshj.key.KeyAlgorithms;
 import com.hierynomus.sshj.transport.IdentificationStringParser;
 import net.schmizz.concurrent.ErrorDeliveryUtil;
 import net.schmizz.concurrent.Event;
@@ -89,7 +90,9 @@ static final class ConnInfo {
 
     private final Decoder decoder;
 
-    private List<KeyAlgorithm> keyAlgorithms;
+    private KeyAlgorithm hostKeyAlgorithm;
+
+    private boolean rsaSHA2Support;
 
     private final Event<TransportException> serviceAccept;
 
@@ -657,18 +660,30 @@ ConnInfo getConnInfo() {
         return connInfo;
     }
 
+    public void setHostKeyAlgorithm(KeyAlgorithm keyAlgorithm) {
+        this.hostKeyAlgorithm = keyAlgorithm;
+    }
+
     @Override
-    public KeyAlgorithm getKeyAlgorithm(KeyType keyType) throws TransportException {
-        for (KeyAlgorithm ka : keyAlgorithms) {
-            if (ka.getKeyFormat().equals(keyType)) {
-                return ka;
-            }
-        }
+    public KeyAlgorithm getHostKeyAlgorithm() {
+        return this.hostKeyAlgorithm;
+    }
 
-        throw new TransportException("Cannot find an available KeyAlgorithm for type " + keyType);
+    public void setRSASHA2Support(boolean rsaSHA2Support) {
+        this.rsaSHA2Support = rsaSHA2Support;
     }
 
-    public void setKeyAlgorithms(List<KeyAlgorithm> keyAlgorithms) {
-        this.keyAlgorithms = keyAlgorithms;
+    @Override
+    public KeyAlgorithm getClientKeyAlgorithm(KeyType keyType) throws TransportException {
+        if (keyType != KeyType.RSA || !rsaSHA2Support) {
+            return Factory.Named.Util.create(getConfig().getKeyAlgorithms(), keyType.toString());
+        }
+
+        List<Factory.Named<KeyAlgorithm>> factories = getConfig().getKeyAlgorithms();
+        if (factories != null)
+            for (Factory.Named<KeyAlgorithm> f : factories)
+                if (f.getName().equals("ssh-rsa") || KeyAlgorithms.SSH_RSA_SHA2_ALGORITHMS.contains(f.getName()))
+                    return f.create();
+        throw new TransportException("Cannot find an available KeyAlgorithm for type " + keyType);
     }
 }
diff --git a/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHG.java b/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHG.java
@@ -79,7 +79,7 @@ public boolean next(Message msg, SSHPacket packet)
         H = digest.digest();
 
 
-        Signature signature = trans.getKeyAlgorithm(KeyType.fromKey(hostKey)).newSignature();
+        Signature signature = trans.getHostKeyAlgorithm().newSignature();
         signature.initVerify(hostKey);
         signature.update(H, 0, H.length);
         if (!signature.verify(sig))

diff --git a/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHGex.java b/src/main/java/net/schmizz/sshj/transport/kex/AbstractDHGex.java
@@ -85,7 +85,7 @@ private boolean parseGexReply(SSHPacket buffer) throws Buffer.BufferException, G
                 .putMPInt(k);
         digest.update(buf.array(), buf.rpos(), buf.available());
         H = digest.digest();
-        KeyAlgorithm keyAlgorithm = trans.getKeyAlgorithm(KeyType.fromKey(hostKey));
+        KeyAlgorithm keyAlgorithm = trans.getHostKeyAlgorithm();
         Signature signature = keyAlgorithm.newSignature();
         signature.initVerify(hostKey);
         signature.update(H, 0, H.length);

diff --git a/src/main/java/net/schmizz/sshj/userauth/method/KeyedAuthMethod.java b/src/main/java/net/schmizz/sshj/userauth/method/KeyedAuthMethod.java
@@ -50,7 +50,7 @@ protected SSHPacket putPubKey(SSHPacket reqBuf)
         // public key as 2 strings: [ key type | key blob ]
         KeyType keyType = KeyType.fromKey(key);
         try {
-            KeyAlgorithm ka = params.getTransport().getKeyAlgorithm(keyType);
+            KeyAlgorithm ka = params.getTransport().getClientKeyAlgorithm(keyType);
             reqBuf.putString(ka.getKeyAlgorithm())
                     .putString(new Buffer.PlainBuffer().putPublicKey(key).getCompactData());
             return reqBuf;
@@ -71,7 +71,7 @@ protected SSHPacket putSig(SSHPacket reqBuf)
         final KeyType kt = KeyType.fromKey(key);
         Signature signature;
         try {
-            signature = params.getTransport().getKeyAlgorithm(kt).newSignature();
+            signature = params.getTransport().getClientKeyAlgorithm(kt).newSignature();
         } catch (TransportException e) {
             throw new UserAuthException("No KeyAlgorithm configured for key " + kt);
         }