some authentication refactoring esp. keyboard-interactive auth

src/main/java/net/schmizz/sshj/SSHClient.java

@@ -49,7 +49,7 @@ import net.schmizz.sshj.userauth.keyprovider.FileKeyProvider;
 import net.schmizz.sshj.userauth.keyprovider.KeyPairWrapper;
 import net.schmizz.sshj.userauth.keyprovider.KeyProvider;
 import net.schmizz.sshj.userauth.keyprovider.KeyProviderUtil;
-import net.schmizz.sshj.userauth.method.AuthChallengeResponse;
+import net.schmizz.sshj.userauth.method.AuthKeyboardInteractive;
 import net.schmizz.sshj.userauth.method.AuthMethod;
 import net.schmizz.sshj.userauth.method.AuthPassword;
 import net.schmizz.sshj.userauth.method.AuthPublickey;
@@ -262,7 +262,7 @@ public class SSHClient
      */
     public void authPassword(String username, PasswordFinder pfinder)
             throws UserAuthException, TransportException {
-        auth(username, new AuthPassword(pfinder), new AuthChallengeResponse(new PasswordResponseProvider(pfinder)));
+        auth(username, new AuthPassword(pfinder), new AuthKeyboardInteractive(new PasswordResponseProvider(pfinder)));
     }
 
     /**

src/main/java/net/schmizz/sshj/common/Buffer.java

@@ -440,19 +440,19 @@ public class Buffer<T extends Buffer<T>> {
      * This is useful when a plaintext password needs to be sent. If {@code passwd} is {@code null}, an empty string is
      * written.
      *
-     * @param passwd (null-ok) the password as a character array
+     * @param str (null-ok) the password as a character array
      *
      * @return this
      */
     @SuppressWarnings("unchecked")
-    public T putPassword(char[] passwd) {
+    public T putSensitiveString(char[] str) {
-        if (passwd == null)
+        if (str == null)
             return putString("");
-        putInt(passwd.length);
+        putInt(str.length);
-        ensureCapacity(passwd.length);
+        ensureCapacity(str.length);
-        for (char c : passwd)
+        for (char c : str)
             data[wpos++] = (byte) c;
-        Arrays.fill(passwd, ' ');
+        Arrays.fill(str, ' ');
         return (T) this;
     }
 

src/main/java/net/schmizz/sshj/userauth/method/AbstractAuthMethod.java

@@ -20,6 +20,7 @@ import net.schmizz.sshj.common.SSHPacket;
 import net.schmizz.sshj.transport.TransportException;
 import net.schmizz.sshj.userauth.AuthParams;
 import net.schmizz.sshj.userauth.UserAuthException;
+import net.schmizz.sshj.userauth.password.AccountResource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -80,4 +81,8 @@ public abstract class AbstractAuthMethod
 
     }
 
+    protected AccountResource makeAccountResource() {
+        return new AccountResource(params.getUsername(), params.getTransport().getRemoteHost());
+    }
+
 }

src/main/java/net/schmizz/sshj/userauth/method/AuthKeyboardInteractive.java

@@ -18,32 +18,19 @@ package net.schmizz.sshj.userauth.method;
 import net.schmizz.sshj.common.Message;
 import net.schmizz.sshj.common.SSHPacket;
 import net.schmizz.sshj.transport.TransportException;
-import net.schmizz.sshj.userauth.AuthParams;
 import net.schmizz.sshj.userauth.UserAuthException;
-import net.schmizz.sshj.userauth.password.AccountResource;
-import net.schmizz.sshj.userauth.password.Resource;
-
-import java.util.ArrayList;
-import java.util.List;
 
 /** Implements the {@code keyboard-interactive} authentication method. */
-public class AuthChallengeResponse
+public class AuthKeyboardInteractive
         extends AbstractAuthMethod {
 
     private final ChallengeResponseProvider provider;
-    private Resource resource;
 
-    public AuthChallengeResponse(ChallengeResponseProvider provider) {
+    public AuthKeyboardInteractive(ChallengeResponseProvider provider) {
         super("keyboard-interactive");
         this.provider = provider;
     }
 
-    @Override
-    public void init(AuthParams params) {
-        super.init(params);
-        resource = new AccountResource(params.getUsername(), params.getTransport().getRemoteHost());
-    }
-
     @Override
     public SSHPacket buildReq()
             throws UserAuthException {
@@ -62,36 +49,39 @@ public class AuthChallengeResponse
         return sb.toString();
     }
 
+    private static class CharArrWrap {
+        private final char[] arr;
+
+        private CharArrWrap(char[] arr) {
+            this.arr = arr;
+        }
+    }
+
     @Override
     public void handle(Message cmd, SSHPacket buf)
             throws UserAuthException, TransportException {
-        if (cmd == Message.USERAUTH_60) {
+        if (cmd != Message.USERAUTH_60) {
-            provider.init(resource, buf.readString(), buf.readString());
+            super.handle(cmd, buf);
+        } else {
+            provider.init(makeAccountResource(), buf.readString(), buf.readString());
             buf.readString(); // lang-tag
             final int numPrompts = buf.readInt();
-            final List<String> userReplies = new ArrayList<String>(numPrompts);
+            final CharArrWrap[] userReplies = new CharArrWrap[numPrompts];
             for (int i = 0; i < numPrompts; i++) {
-                userReplies.add(provider.getResponse(buf.readString(), buf.readBoolean()));
+                userReplies[i] = new CharArrWrap(provider.getResponse(buf.readString(), buf.readBoolean()));
             }
             respond(userReplies);
-        } else
+        }
-            super.handle(cmd, buf);
     }
 
-    private void respond(List<String> userReplies)
+    private void respond(CharArrWrap[] userReplies)
             throws TransportException {
-        final SSHPacket pkt = new SSHPacket(Message.USERAUTH_INFO_RESPONSE)
+        final SSHPacket pkt = new SSHPacket(Message.USERAUTH_INFO_RESPONSE).putInt(userReplies.length);
-                .putInt(userReplies.size());
+        for (final CharArrWrap response : userReplies)
-        for (String response : userReplies) {
+            pkt.putSensitiveString(response.arr);
-            pkt.putString(response);
-        }
         params.getTransport().write(pkt);
     }
 
-    /**
-     * Returns {@code true} if the associated {@link net.schmizz.sshj.userauth.password.PasswordFinder} tells that we
-     * should retry with a new password that it will supply.
-     */
     @Override
     public boolean shouldRetry() {
         return provider.shouldRetry();

src/main/java/net/schmizz/sshj/userauth/method/AuthPassword.java

@@ -18,42 +18,29 @@ package net.schmizz.sshj.userauth.method;
 import net.schmizz.sshj.common.Message;
 import net.schmizz.sshj.common.SSHPacket;
 import net.schmizz.sshj.transport.TransportException;
-import net.schmizz.sshj.userauth.AuthParams;
 import net.schmizz.sshj.userauth.UserAuthException;
 import net.schmizz.sshj.userauth.password.AccountResource;
 import net.schmizz.sshj.userauth.password.PasswordFinder;
-import net.schmizz.sshj.userauth.password.Resource;
 
 /** Implements the {@code password} authentication method. Password-change request handling is not currently supported. */
 public class AuthPassword
         extends AbstractAuthMethod {
 
     private final PasswordFinder pwdf;
-    private Resource resource;
 
     public AuthPassword(PasswordFinder pwdf) {
         super("password");
         this.pwdf = pwdf;
-
-    }
-
-    @Override
-    public void init(AuthParams params) {
-        super.init(params);
-        resource = new AccountResource(params.getUsername(), params.getTransport().getRemoteHost());
     }
 
     @Override
     public SSHPacket buildReq()
             throws UserAuthException {
-        log.info("Requesting password for {}", resource);
+        final AccountResource accountResource = makeAccountResource();
-        char[] password = pwdf.reqPassword(resource);
+        log.info("Requesting password for {}", accountResource);
-        if (password == null)
+        return super.buildReq() // the generic stuff
-            throw new UserAuthException("Was given null password for " + resource);
+                .putBoolean(false) // no, we are not responding to a CHANGEREQ
-        else
+                .putSensitiveString(pwdf.reqPassword(accountResource));
-            return super.buildReq() // the generic stuff
-                    .putBoolean(false) // no, we are not responding to a CHANGEREQ
-                    .putPassword(password);
     }
 
     @Override
@@ -71,7 +58,7 @@ public class AuthPassword
      */
     @Override
     public boolean shouldRetry() {
-        return pwdf.shouldRetry(resource);
+        return pwdf.shouldRetry(makeAccountResource());
     }
 
 }

src/main/java/net/schmizz/sshj/userauth/method/ChallengeResponseProvider.java

@@ -25,7 +25,7 @@ public interface ChallengeResponseProvider {
 
     void init(Resource resource, String name, String instruction);
 
-    String getResponse(String prompt, boolean echo);
+    char[] getResponse(String prompt, boolean echo);
 
     boolean shouldRetry();
 

src/main/java/net/schmizz/sshj/userauth/method/PasswordResponseProvider.java

@@ -28,6 +28,8 @@ public class PasswordResponseProvider
 
     private final Logger log = LoggerFactory.getLogger(getClass());
 
+    private static final char[] EMPTY_RESPONSE = new char[0];
+
     private final PasswordFinder pwdf;
     private Resource resource;
     private boolean gaveOnce;
@@ -48,13 +50,13 @@ public class PasswordResponseProvider
     }
 
     @Override
-    public String getResponse(String prompt, boolean echo) {
+    public char[] getResponse(String prompt, boolean echo) {
         if (!gaveOnce && !echo && prompt.toLowerCase().contains("password")) {
             gaveOnce = true;
             log.debug("prompt = `{}`", prompt);
-            return new String(pwdf.reqPassword(resource));
+            return pwdf.reqPassword(resource);
         }
-        return "";
+        return EMPTY_RESPONSE;
     }
 
     @Override

src/main/java/net/schmizz/sshj/userauth/password/PasswordFinder.java

@@ -22,7 +22,7 @@ public interface PasswordFinder {
      * Request password for specified resource.
      * <p/>
      * This method may return {@code null} when the request cannot be serviced, e.g. when the user cancels a password
-     * prompt. The consequences of returning {@code null} are specific to the requestor.
+     * prompt.
      *
      * @param resource the resource for which password is being requested
      *

src/test/java/net/schmizz/sshj/util/BufferTest.java

@@ -29,7 +29,7 @@ import java.security.GeneralSecurityException;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 
-/** Tests {@link SSHPacket} functionality */
+/** Tests {@link Buffer} functionality */
 public class BufferTest {
     private Buffer.PlainBuffer posBuf;
     private Buffer.PlainBuffer handyBuf;
@@ -69,7 +69,7 @@ public class BufferTest {
     public void testPassword() {
         char[] pass = "lolcatz".toCharArray();
         // test if put correctly as a string
-        assertEquals(new Buffer.PlainBuffer().putPassword(pass).readString(), "lolcatz");
+        assertEquals(new Buffer.PlainBuffer().putSensitiveString(pass).readString(), "lolcatz");
         // test that char[] was blanked out
         assertArrayEquals(pass, "       ".toCharArray());
     }