Prefer char[] for passwords on client

Using a char[] allows zeroing the password after use, whereas a String change would just create a copy. There is still no guarantee the JVM has not made copies of the char[], but it should stick around less.
square · Apr 20, 2015 · 630afcc · alokmenghrajani · Apr 20, 2015 · sul3n3t
1 parent c246354
commit 630afcc
Show file tree

Hide file tree

Showing 15 changed files with 32 additions and 29 deletions.
diff --git a/api/src/main/java/keywhiz/api/LoginRequest.java b/api/src/main/java/keywhiz/api/LoginRequest.java
@@ -17,7 +17,8 @@
 package keywhiz.api;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.Objects;
+import java.util.Arrays;
+import java.util.Objects;
 import javax.validation.constraints.NotNull;
 
 public class LoginRequest {
@@ -27,24 +28,24 @@ public class LoginRequest {
 
   @NotNull
   @JsonProperty
-  public final String password;
+  public final char[] password;
 
   public LoginRequest(@JsonProperty("username") String username,
-      @JsonProperty("password") String password) {
+      @JsonProperty("password") char[] password) {
     this.username = username;
     this.password = password;
   }
 
   @Override public int hashCode() {
-    return Objects.hashCode(username, password);
+    return Objects.hash(username, password);
   }
 
   @Override public boolean equals(Object o) {
     if (this == o) return true;
     if (o instanceof LoginRequest) {
       LoginRequest that = (LoginRequest) o;
-      if (Objects.equal(this.username, that.username) &&
-          Objects.equal(this.password, that.password)) {
+      if (Objects.equals(this.username, that.username) &&
+          Arrays.equals(this.password, that.password)) {
         return true;
       }
     }

diff --git a/api/src/test/java/keywhiz/api/LoginRequestTest.java b/api/src/test/java/keywhiz/api/LoginRequestTest.java
@@ -24,7 +24,7 @@
 
 public class LoginRequestTest {
   @Test public void deserializesCorrectly() throws Exception {
-    LoginRequest loginRequest = new LoginRequest("keywhizAdmin", "adminPass");
+    LoginRequest loginRequest = new LoginRequest("keywhizAdmin", "adminPass".toCharArray());
     assertThat(fromJson(jsonFixture("fixtures/loginRequest.json"), LoginRequest.class))
         .isEqualTo(loginRequest);
   }

diff --git a/cli/src/main/java/keywhiz/cli/ClientUtils.java b/cli/src/main/java/keywhiz/cli/ClientUtils.java
@@ -182,13 +182,13 @@ public static List<HttpCookie> loadCookies(Path path) throws IOException {
    *
    * @return user-inputted password
    */
-  public static String readPassword() throws IOException {
+  public static char[] readPassword() {
     Console console = System.console();
     if (console != null) {
       System.out.format("password for '%s': ", USER_NAME.value());
-      return String.copyValueOf(System.console().readPassword());
+      return System.console().readPassword();
     } else {
-      throw new IOException("Please login by running a command without piping.\n"
+      throw new RuntimeException("Please login by running a command without piping.\n"
           + "For example: keywhiz.cli login");
     }
   }
@@ -230,4 +230,4 @@ public XsrfTokenInterceptor(String xsrfCookieName, String xsrfHeaderName) {
       return chain.proceed(request);
     }
   }
-}
+}
diff --git a/cli/src/main/java/keywhiz/cli/CommandExecutor.java b/cli/src/main/java/keywhiz/cli/CommandExecutor.java
@@ -28,6 +28,7 @@
 import java.net.URL;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -115,8 +116,9 @@ public void executeCommand() throws IOException {
       encapsulatedClient = ClientUtils.sslOkHttpClient(ImmutableList.of());
       client = new KeywhizClient(mapper, ClientUtils.hostBoundWrappedHttpClient(host,
           encapsulatedClient));
-      String password = ClientUtils.readPassword();
+      char[] password = ClientUtils.readPassword();
       client.login(USER_NAME.value(), password);
+      Arrays.fill(password, '\0');
     }
     // Save/update the cookies if we logged in successfully
     ClientUtils.saveCookies((CookieManager) encapsulatedClient.getCookieHandler(), COOKIE_PATH);

diff --git a/client/src/main/java/keywhiz/client/KeywhizClient.java b/client/src/main/java/keywhiz/client/KeywhizClient.java
@@ -107,7 +107,7 @@ public KeywhizClient(ObjectMapper mapper, OkHttpClient client) {
    * @param password login password
    * @throws IOException if a network IO error occurs
    */
-  public void login(String username, String password) throws IOException {
+  public void login(String username, char[] password) throws IOException {
     httpPost("/admin/login", new LoginRequest(username, password));
   }
 

diff --git a/server/src/main/java/keywhiz/commands/DbSeedCommand.java b/server/src/main/java/keywhiz/commands/DbSeedCommand.java
@@ -51,7 +51,7 @@
 public class DbSeedCommand extends ConfiguredCommand<KeywhizConfig> {
   // Didn't we say not to keep passwords in code? ;)
   public static String defaultUser = "keywhizAdmin";
-  public static String defaultPassword = "adminPass";
+  public static char[] defaultPassword = "adminPass".toCharArray();
 
   public DbSeedCommand() {
     super("db-seed", "Populates database with development data.");
@@ -152,7 +152,7 @@ public static void doImport(DSLContext dslContext) {
     dslContext
         .insertInto(USERS)
         .set(USERS.USERNAME, defaultUser)
-        .set(USERS.PASSWORD_HASH, BCrypt.hashpw(defaultPassword, BCrypt.gensalt()))
+        .set(USERS.PASSWORD_HASH, BCrypt.hashpw(String.copyValueOf(defaultPassword), BCrypt.gensalt()))
         .set(USERS.CREATED_AT, OffsetDateTime.parse("2012-06-22T14:38:09.982586Z"))
         .set(USERS.UPDATED_AT, OffsetDateTime.parse("2012-06-22T14:38:09.982586Z"))
         .execute();

diff --git a/server/src/main/java/keywhiz/service/resources/SessionLoginResource.java b/server/src/main/java/keywhiz/service/resources/SessionLoginResource.java
@@ -77,7 +77,7 @@ public SessionLoginResource(@Readonly Authenticator<BasicCredentials, User> user
   public Response login(@Valid LoginRequest request) {
 
     String username = request.username;
-    String password = request.password;
+    String password = String.copyValueOf(request.password);
 
     Optional<User> optionalUser = Optional.empty();
     try {

diff --git a/server/src/test/java/keywhiz/AuthHelper.java b/server/src/test/java/keywhiz/AuthHelper.java
@@ -46,14 +46,14 @@ public class AuthHelper {
    * @param password login password
    * @return valid login request, given a valid username and password
    */
-  public static Request buildLoginPost(@Nullable String username, @Nullable String password) {
+  public static Request buildLoginPost(@Nullable String username, @Nullable char[] password) {
     Map<String, String> login = Maps.newHashMap();
     if (username != null) {
       login.put("username", username);
     }
 
     if (password != null) {
-      login.put("password", password);
+      login.put("password", String.copyValueOf(password));
     }
 
     RequestBody body;

diff --git a/server/src/test/java/keywhiz/service/resources/ClientsResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/ClientsResourceIntegrationTest.java
@@ -61,7 +61,7 @@ private static List<String> clientsToNames(List<Client> response) {
 
   @Test(expected = KeywhizClient.UnauthorizedException.class)
   public void adminRejectsNonKeywhizUsers() throws IOException {
-    keywhizClient.login("username", "password");
+    keywhizClient.login("username", "password".toCharArray());
     keywhizClient.allClients();
   }
 

diff --git a/server/src/test/java/keywhiz/service/resources/GroupsResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/GroupsResourceIntegrationTest.java
@@ -58,7 +58,7 @@ public class GroupsResourceIntegrationTest {
 
   @Test(expected = KeywhizClient.UnauthorizedException.class)
   public void adminRejectsNonKeywhizUsers() throws IOException {
-    keywhizClient.login("username", "password");
+    keywhizClient.login("username", "password".toCharArray());
     keywhizClient.allGroups();
   }
 

diff --git a/server/src/test/java/keywhiz/service/resources/MembershipResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/MembershipResourceIntegrationTest.java
@@ -63,7 +63,7 @@ public void allowingMissingSecretInGroup() throws IOException {
 
   @Test(expected = KeywhizClient.UnauthorizedException.class)
   public void adminRejectsNonKeywhizUsers() throws IOException {
-    keywhizClient.login("username", "password");
+    keywhizClient.login("username", "password".toCharArray());
     keywhizClient.grantSecretToGroupByIds(741, 916);
   }
 

diff --git a/server/src/test/java/keywhiz/service/resources/SecretsResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/SecretsResourceIntegrationTest.java
@@ -64,7 +64,7 @@ public class SecretsResourceIntegrationTest {
 
   @Test(expected = KeywhizClient.UnauthorizedException.class)
   public void adminRejectsNonKeywhizUsers() throws IOException {
-    keywhizClient.login("username", "password");
+    keywhizClient.login("username", "password".toCharArray());
     keywhizClient.allSecrets();
   }
 

diff --git a/server/src/test/java/keywhiz/service/resources/SessionLoginResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/SessionLoginResourceIntegrationTest.java
@@ -82,15 +82,15 @@ public void setsValidCookieForValidCredentials() throws Exception {
 
   @Test
   public void invalidCredentialsAreUnauthorized() throws Exception {
-    Request request = buildLoginPost("username", "badpassword");
+    Request request = buildLoginPost("username", "badpassword".toCharArray());
 
     Response response = client.newCall(request).execute();
     assertThat(response.code()).isEqualTo(401);
   }
 
   @Test
   public void insufficientRolesAreUnauthorized() throws Exception {
-    Request request = buildLoginPost("username", "password");
+    Request request = buildLoginPost("username", "password".toCharArray());
 
     Response response = client.newCall(request).execute();
     assertThat(response.code()).isEqualTo(401);
@@ -106,7 +106,7 @@ public void noFormDataIsBadRequest() throws Exception {
 
   @Test
   public void missingUsernameIsBadRequest() throws Exception {
-    Request request = buildLoginPost(null, "password");
+    Request request = buildLoginPost(null, "password".toCharArray());
 
     Response response = client.newCall(request).execute();
     assertThat(response.code()).isEqualTo(422);

diff --git a/server/src/test/java/keywhiz/service/resources/SessionLoginResourceTest.java b/server/src/test/java/keywhiz/service/resources/SessionLoginResourceTest.java
@@ -73,15 +73,15 @@ public void setUp() throws Exception {
   public void badCredentialsThrowUnauthorized() throws Exception {
     when(ldapAuthenticator.authenticate(badCredentials)).thenReturn(Optional.empty());
 
-    sessionLoginResource.login(new LoginRequest("bad", "credentials"));
+    sessionLoginResource.login(new LoginRequest("bad", "credentials".toCharArray()));
   }
 
   @Test
   public void goodCredentialsSetsCookie() throws Exception {
     User user = User.named("goodUser");
     when(ldapAuthenticator.authenticate(goodCredentials)).thenReturn(Optional.of(user));
 
-    Response response = sessionLoginResource.login(new LoginRequest("good", "credentials"));
+    Response response = sessionLoginResource.login(new LoginRequest("good", "credentials".toCharArray()));
     assertThat(response.getStatus()).isEqualTo(SEE_OTHER.getStatusCode());
 
     Map<String, NewCookie> responseCookies = response.getCookies();
@@ -94,7 +94,7 @@ public void goodCredentialsSetsCookie() throws Exception {
 
   @Test(expected = NullPointerException.class)
   public void missingUsernameThrowsException() throws Exception {
-    sessionLoginResource.login(new LoginRequest(null, "password"));
+    sessionLoginResource.login(new LoginRequest(null, "password".toCharArray()));
   }
 
   @Test(expected = NullPointerException.class)

diff --git a/server/src/test/java/keywhiz/service/resources/SessionMeResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/SessionMeResourceIntegrationTest.java
@@ -61,7 +61,7 @@ public class SessionMeResourceIntegrationTest {
   }
 
   @Test public void adminRejectsNonLoggedInUser() throws IOException {
-    client.newCall(buildLoginPost("username", "password")).execute();
+    client.newCall(buildLoginPost("username", "password".toCharArray())).execute();
 
     Request get = new Request.Builder()
         .get()