Add pwdf retry logic to OpenSSHKeyV1KeyFile (#587)

* Add pwdf retry logic to OpenSSHKeyV1KeyFile

While PKCS8KeyFile uses PasswordFinder's shouldRetry to determine
whether it should call reqPassword again if decryption of they key file
fails, OpenSSHKeyV1KeyFile simply gives up and throws an exception.

With this commit, retry logic similar to that of PKCS8KeyFile is added
to OpenSSHKeyV1KeyFile. The PasswordFinder's reqPassword is called
again if the validation of the "checkint" fails, which indicates an
incorrect passphrase.

* Use new exception to signal incorrect passphrase

* Throw common exception on key decryption failure

* Add test coverage for retry logic

Co-authored-by: Jeroen van Erp <jeroen@hierynomus.com>

Author: FabianHenneke

src/main/java/com/hierynomus/sshj/common/KeyDecryptionFailedException.java

@@ -0,0 +1,38 @@
+/*
+ * 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.common;
+
+import org.bouncycastle.openssl.EncryptionException;
+
+import java.io.IOException;
+
+/**
+ * Thrown when a key file could not be decrypted correctly, e.g. if its checkInts differed in the case of an OpenSSH
+ * key file.
+ */
+public class KeyDecryptionFailedException extends IOException {
+
+    public static final String MESSAGE = "Decryption of the key failed. A supplied passphrase may be incorrect.";
+
+    public KeyDecryptionFailedException() {
+        super(MESSAGE);
+    }
+
+    public KeyDecryptionFailedException(EncryptionException cause) {
+        super(MESSAGE, cause);
+    }
+
+}

src/main/java/com/hierynomus/sshj/userauth/keyprovider/OpenSSHKeyV1KeyFile.java

@@ -15,6 +15,7 @@
  */
 package com.hierynomus.sshj.userauth.keyprovider;
 
+import com.hierynomus.sshj.common.KeyDecryptionFailedException;
 import com.hierynomus.sshj.transport.cipher.BlockCiphers;
 import net.i2p.crypto.eddsa.EdDSAPrivateKey;
 import net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable;
@@ -111,8 +112,16 @@ private KeyPair readDecodedKeyPair(final PlainBuffer keyBuffer) throws IOExcepti
             return readUnencrypted(privateKeyBuffer, publicKey);
         } else {
             logger.info("Keypair is encrypted with: " + cipherName + ", " + kdfName + ", " + Arrays.toString(kdfOptions));
-            PlainBuffer decrypted = decryptBuffer(privateKeyBuffer, cipherName, kdfName, kdfOptions);
-            return readUnencrypted(decrypted, publicKey);
+            while (true) {
+                PlainBuffer decryptionBuffer = new PlainBuffer(privateKeyBuffer);
+                PlainBuffer decrypted = decryptBuffer(decryptionBuffer, cipherName, kdfName, kdfOptions);
+                try {
+                    return readUnencrypted(decrypted, publicKey);
+                } catch (KeyDecryptionFailedException e) {
+                    if (pwdf == null || !pwdf.shouldRetry(resource))
+                        throw e;
+                }
+            }
 //            throw new IOException("Cannot read encrypted keypair with " + cipherName + " yet.");
         }
     }
@@ -184,7 +193,7 @@ private KeyPair readUnencrypted(final PlainBuffer keyBuffer, final PublicKey pub
         int checkInt1 = keyBuffer.readUInt32AsInt(); // uint32 checkint1
         int checkInt2 = keyBuffer.readUInt32AsInt(); // uint32 checkint2
         if (checkInt1 != checkInt2) {
-            throw new IOException("The checkInts differed, the key was not correctly decoded.");
+            throw new KeyDecryptionFailedException();
         }
         // The private key section contains both the public key and the private key
         String keyType = keyBuffer.readString(); // string keytype

src/main/java/net/schmizz/sshj/userauth/keyprovider/PKCS8KeyFile.java

@@ -15,6 +15,7 @@
  */
 package net.schmizz.sshj.userauth.keyprovider;
 
+import com.hierynomus.sshj.common.KeyDecryptionFailedException;
 import net.schmizz.sshj.common.IOUtils;
 import net.schmizz.sshj.common.SecurityUtils;
 import net.schmizz.sshj.userauth.password.PasswordUtils;
@@ -85,7 +86,7 @@ protected KeyPair readKeyPair()
                 if (pwdf != null && pwdf.shouldRetry(resource))
                     continue;
                 else
-                    throw e;
+                    throw new KeyDecryptionFailedException(e);
             } finally {
                 IOUtils.closeQuietly(r);
             }

src/test/java/net/schmizz/sshj/keyprovider/OpenSSHKeyFileTest.java

@@ -15,6 +15,7 @@
  */
 package net.schmizz.sshj.keyprovider;
 
+import com.hierynomus.sshj.common.KeyDecryptionFailedException;
 import com.hierynomus.sshj.userauth.certificate.Certificate;
 import com.hierynomus.sshj.userauth.keyprovider.OpenSSHKeyV1KeyFile;
 import net.schmizz.sshj.common.KeyType;
@@ -200,12 +201,34 @@ public void shouldLoadED25519PrivateKey() throws IOException {
 
     @Test
     public void shouldLoadProtectedED25519PrivateKeyAes256CTR() throws IOException {
-        checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_protected", "sshjtest");
+        checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_protected", "sshjtest", false);
+        checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_protected", "sshjtest", true);
     }
 
     @Test
     public void shouldLoadProtectedED25519PrivateKeyAes256CBC() throws IOException {
-        checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_aes256cbc.pem", "foobar");
+        checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_aes256cbc.pem", "foobar", false);
+        checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_aes256cbc.pem", "foobar", true);
+    }
+
+    @Test(expected = KeyDecryptionFailedException.class)
+    public void shouldFailOnIncorrectPassphraseAfterRetries() throws IOException {
+        OpenSSHKeyV1KeyFile keyFile = new OpenSSHKeyV1KeyFile();
+        keyFile.init(new File("src/test/resources/keytypes/ed25519_aes256cbc.pem"), new PasswordFinder() {
+            private int reqCounter = 0;
+
+            @Override
+            public char[] reqPassword(Resource<?> resource) {
+                reqCounter++;
+                return "incorrect".toCharArray();
+            }
+
+            @Override
+            public boolean shouldRetry(Resource<?> resource) {
+                return reqCounter <= 3;
+            }
+        });
+        keyFile.getPrivate();
     }
 
     @Test
@@ -224,17 +247,25 @@ public void shouldLoadECDSAPrivateKeyAsOpenSSHV1() throws IOException {
         assertThat(aPrivate.getAlgorithm(), equalTo("ECDSA"));
     }
 
-    private void checkOpenSSHKeyV1(String key, final String password) throws IOException {
+    private void checkOpenSSHKeyV1(String key, final String password, boolean withRetry) throws IOException {
         OpenSSHKeyV1KeyFile keyFile = new OpenSSHKeyV1KeyFile();
         keyFile.init(new File(key), new PasswordFinder() {
+            private int reqCounter = 0;
+
             @Override
             public char[] reqPassword(Resource<?> resource) {
-                return password.toCharArray();
+                if (withRetry && reqCounter < 3) {
+                    reqCounter++;
+                    // Return an incorrect password three times before returning the correct one.
+                    return (password + "incorrect").toCharArray();
+                } else {
+                    return password.toCharArray();
+                }
             }
 
             @Override
             public boolean shouldRetry(Resource<?> resource) {
-                return false;
+                return withRetry && reqCounter <= 3;
             }
         });
         PrivateKey aPrivate = keyFile.getPrivate();