Skip to content
This repository has been archived by the owner on Oct 31, 2021. It is now read-only.

Commit

Permalink
Safer handling of passwords on the command line (closes #408)
Browse files Browse the repository at this point in the history
  • Loading branch information
triceo committed Apr 20, 2019
1 parent 5ce72f7 commit 38f75b0
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 14 deletions.
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@
<dependency>
<groupId>info.picocli</groupId>
<artifactId>picocli</artifactId>
<version>3.9.5</version>
<version>3.9.6</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ public class CommandLine implements Callable<Optional<InvestmentMode>> {
private String notificationConfigLocation;
@picocli.CommandLine.Option(names = {"-n", "--name"}, description = "Name of this RoboZonky session.")
private String name = "Unnamed";
@picocli.CommandLine.Option(names = {"-p", "--password"}, required = true,
description = "Enter Zonky account password or secure storage password.")
private String password = null;
@picocli.CommandLine.Option(names = {"-p", "--password"}, required = true, interactive = true,
arity = "0..1", description = "Enter Zonky account password or secure storage password.")
private char[] password = null;
@picocli.CommandLine.Option(names = {"-d", "--dry"},
description = "RoboZonky will simulate investments, but never actually spend money.")
private boolean dryRunEnabled = false;
Expand Down Expand Up @@ -119,7 +119,7 @@ Optional<URL> getNotificationConfigLocation() {
}

char[] getPassword() {
return password.toCharArray();
return password;
}

Optional<File> getKeystore() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.github.robozonky.app.configuration;

import java.util.Arrays;
import java.util.Optional;

import com.github.robozonky.common.secrets.KeyStoreHandler;
Expand All @@ -37,14 +38,16 @@ private SecretProviderFactory() {
* @return KeyStore-based secret provider or empty in case of a problem happened inside the keystore.
*/
public static Optional<SecretProvider> getSecretProvider(final CommandLine cli) {
final char[] password = cli.getPassword();
return cli.getKeystore().map(keystore -> {
final char[] password = cli.getPassword();
try {
final KeyStoreHandler ksh = KeyStoreHandler.open(keystore, password);
return Optional.of(SecretProvider.keyStoreBased(ksh));
} catch (final Exception ex) {
LOGGER.error("Failed opening guarded storage.", ex);
return Optional.<SecretProvider>empty();
} finally {
Arrays.fill(password, ' '); // clear the password from memory
}
}).orElseThrow(() -> new IllegalStateException("Could not find keystore."));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,16 @@ void nonexistentKeyStoreProvided() throws Exception {
@Test
void wrongFormatKeyStoreProvided() throws Exception {
final File tmp = File.createTempFile("robozonky-", ".keystore"); // empty key store
final CommandLine cli = SecretProviderFactoryTest.mockCli(tmp, "password".toCharArray());
final char[] password = "pass".toCharArray();
final CommandLine cli = SecretProviderFactoryTest.mockCli(tmp, password);
assertThat(SecretProviderFactory.getSecretProvider(cli)).isEmpty();
assertThat(password).isEqualTo(" ".toCharArray());
}

@Test
void noFallback() {
final String password = "pass";
final CommandLine cli = SecretProviderFactoryTest.mockCli(null, password.toCharArray());
void nullKeystoreProvided() {
final char[] password = "pass".toCharArray();
final CommandLine cli = SecretProviderFactoryTest.mockCli(null, password);
assertThatThrownBy(() -> SecretProviderFactory.getSecretProvider(cli))
.isInstanceOf(IllegalStateException.class);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ abstract class KeyStoreLeveragingFeature extends AbstractFeature {
required = true)
private File keystore = new File("robozonky.keystore");
@CommandLine.Option(names = {"-s", "--secret"}, description = "Secret to use to access the keystore.",
required = true, interactive = true)
required = true, interactive = true, arity = "0..1")
private char[] secret = null;
private KeyStoreHandler storage;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ public final class MasterPasswordFeature extends KeyStoreLeveragingFeature {

static final String DESCRIPTION = "Change password of the master keystore.";
@CommandLine.Option(names = {"-n", "--new-secret"},
description = "Username to use to authenticate with Zonky servers.", required = true, interactive = true)
description = "Username to use to authenticate with Zonky servers.", required = true, interactive = true,
arity = "0..1")
private char[] newSecret = null;

public MasterPasswordFeature(final File keystore, final char[] keystoreSecret, final char... newSecret) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public final class ZonkoidPasswordFeature extends KeyStoreLeveragingFeature {
static final String ZONKOID_ID = "zonkoid";
private final String id;
@CommandLine.Option(names = {"-p", "--password"},
description = "Code generated in the Zonkoid mobile application.", required = true, interactive = true)
description = "Code generated in the Zonkoid mobile application.", required = true, interactive = true,
arity = "0..1")
private char[] password = null;

public ZonkoidPasswordFeature(final File keystore, final char[] keystoreSecret, final char... password) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public final class ZonkyPasswordFeature extends KeyStoreLeveragingFeature {
description = "Username to use to authenticate with Zonky servers.", required = true)
private String username = null;
@CommandLine.Option(names = {"-p", "--password"},
description = "Password to use to authenticate with Zonky servers.", required = true, interactive = true)
description = "Password to use to authenticate with Zonky servers.", required = true, interactive = true,
arity = "0..1")
private char[] password = null;

public ZonkyPasswordFeature(final File keystore, final char[] keystoreSecret, final String username,
Expand Down

0 comments on commit 38f75b0

Please sign in to comment.