From 72ff74c99d57803d1d5dc285336b79916c454c8d Mon Sep 17 00:00:00 2001 From: Shikhar Bhushan Date: Mon, 24 May 2010 22:48:20 +0100 Subject: [PATCH] some authentication refactoring esp. keyboard-interactive auth --- src/main/java/net/schmizz/sshj/SSHClient.java | 4 +- .../java/net/schmizz/sshj/common/Buffer.java | 14 ++--- .../userauth/method/AbstractAuthMethod.java | 5 ++ ...onse.java => AuthKeyboardInteractive.java} | 52 ++++++++----------- .../sshj/userauth/method/AuthPassword.java | 25 +++------ .../method/ChallengeResponseProvider.java | 2 +- .../method/PasswordResponseProvider.java | 8 +-- .../userauth/password/PasswordFinder.java | 2 +- .../net/schmizz/sshj/util/BufferTest.java | 4 +- 9 files changed, 50 insertions(+), 66 deletions(-) rename src/main/java/net/schmizz/sshj/userauth/method/{AuthChallengeResponse.java => AuthKeyboardInteractive.java} (65%) diff --git a/src/main/java/net/schmizz/sshj/SSHClient.java b/src/main/java/net/schmizz/sshj/SSHClient.java index 1d905288..8f4057d7 100644 --- a/src/main/java/net/schmizz/sshj/SSHClient.java +++ b/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))); } /** diff --git a/src/main/java/net/schmizz/sshj/common/Buffer.java b/src/main/java/net/schmizz/sshj/common/Buffer.java index 4f92f61f..d182745e 100644 --- a/src/main/java/net/schmizz/sshj/common/Buffer.java +++ b/src/main/java/net/schmizz/sshj/common/Buffer.java @@ -440,19 +440,19 @@ public class Buffer> { * 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) { - if (passwd == null) + public T putSensitiveString(char[] str) { + if (str == null) return putString(""); - putInt(passwd.length); - ensureCapacity(passwd.length); - for (char c : passwd) + putInt(str.length); + ensureCapacity(str.length); + for (char c : str) data[wpos++] = (byte) c; - Arrays.fill(passwd, ' '); + Arrays.fill(str, ' '); return (T) this; } diff --git a/src/main/java/net/schmizz/sshj/userauth/method/AbstractAuthMethod.java b/src/main/java/net/schmizz/sshj/userauth/method/AbstractAuthMethod.java index cc405f19..cd7130e6 100644 --- a/src/main/java/net/schmizz/sshj/userauth/method/AbstractAuthMethod.java +++ b/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()); + } + } diff --git a/src/main/java/net/schmizz/sshj/userauth/method/AuthChallengeResponse.java b/src/main/java/net/schmizz/sshj/userauth/method/AuthKeyboardInteractive.java similarity index 65% rename from src/main/java/net/schmizz/sshj/userauth/method/AuthChallengeResponse.java rename to src/main/java/net/schmizz/sshj/userauth/method/AuthKeyboardInteractive.java index f3d0592f..b5dc9875 100644 --- a/src/main/java/net/schmizz/sshj/userauth/method/AuthChallengeResponse.java +++ b/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) { - provider.init(resource, buf.readString(), buf.readString()); + if (cmd != Message.USERAUTH_60) { + super.handle(cmd, buf); + } else { + provider.init(makeAccountResource(), buf.readString(), buf.readString()); buf.readString(); // lang-tag final int numPrompts = buf.readInt(); - final List userReplies = new ArrayList(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 userReplies) + private void respond(CharArrWrap[] userReplies) throws TransportException { - final SSHPacket pkt = new SSHPacket(Message.USERAUTH_INFO_RESPONSE) - .putInt(userReplies.size()); - for (String response : userReplies) { - pkt.putString(response); - } + final SSHPacket pkt = new SSHPacket(Message.USERAUTH_INFO_RESPONSE).putInt(userReplies.length); + for (final CharArrWrap response : userReplies) + pkt.putSensitiveString(response.arr); 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(); diff --git a/src/main/java/net/schmizz/sshj/userauth/method/AuthPassword.java b/src/main/java/net/schmizz/sshj/userauth/method/AuthPassword.java index cb1797e6..4e16beee 100644 --- a/src/main/java/net/schmizz/sshj/userauth/method/AuthPassword.java +++ b/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); - char[] password = pwdf.reqPassword(resource); - if (password == null) - throw new UserAuthException("Was given null password for " + resource); - else - return super.buildReq() // the generic stuff - .putBoolean(false) // no, we are not responding to a CHANGEREQ - .putPassword(password); + final AccountResource accountResource = makeAccountResource(); + log.info("Requesting password for {}", accountResource); + return super.buildReq() // the generic stuff + .putBoolean(false) // no, we are not responding to a CHANGEREQ + .putSensitiveString(pwdf.reqPassword(accountResource)); } @Override @@ -71,7 +58,7 @@ public class AuthPassword */ @Override public boolean shouldRetry() { - return pwdf.shouldRetry(resource); + return pwdf.shouldRetry(makeAccountResource()); } } \ No newline at end of file diff --git a/src/main/java/net/schmizz/sshj/userauth/method/ChallengeResponseProvider.java b/src/main/java/net/schmizz/sshj/userauth/method/ChallengeResponseProvider.java index ff6be3ba..d3f7281f 100644 --- a/src/main/java/net/schmizz/sshj/userauth/method/ChallengeResponseProvider.java +++ b/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(); diff --git a/src/main/java/net/schmizz/sshj/userauth/method/PasswordResponseProvider.java b/src/main/java/net/schmizz/sshj/userauth/method/PasswordResponseProvider.java index 5f642039..174ef010 100644 --- a/src/main/java/net/schmizz/sshj/userauth/method/PasswordResponseProvider.java +++ b/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 diff --git a/src/main/java/net/schmizz/sshj/userauth/password/PasswordFinder.java b/src/main/java/net/schmizz/sshj/userauth/password/PasswordFinder.java index 3825c46a..aafd1b28 100644 --- a/src/main/java/net/schmizz/sshj/userauth/password/PasswordFinder.java +++ b/src/main/java/net/schmizz/sshj/userauth/password/PasswordFinder.java @@ -22,7 +22,7 @@ public interface PasswordFinder { * Request password for specified resource. *

* 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 * diff --git a/src/test/java/net/schmizz/sshj/util/BufferTest.java b/src/test/java/net/schmizz/sshj/util/BufferTest.java index d6e4996d..b5cfb275 100644 --- a/src/test/java/net/schmizz/sshj/util/BufferTest.java +++ b/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()); }