Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement showing password prompt if password not provided #21

Merged
merged 3 commits into from
Jan 11, 2018

Conversation

didlich
Copy link
Contributor

@didlich didlich commented Dec 20, 2017

implements feature #11

@CLAassistant
Copy link

CLAassistant commented Dec 20, 2017

CLA assistant check
All committers have signed the CLA.

String password = "";
Console console = System.console();
if (console == null) {
LOG.warn("No console: non-interactive mode, instead use insecure replacement, PW is shown!");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this warning :)

@overheadhunter
Copy link
Member

Looks good so far. Nice clean style. @markuskreusch will do a full review

Copy link
Contributor

@markuskreusch markuskreusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fine except usage of isRegularFile. I know we did that before those changes but if we touch the code anyway we can just fix it.


@Override
public void validate() throws IllegalArgumentException {
if (!Files.isReadable(pathToFile) || !Files.isRegularFile(pathToFile)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isRegularFile is not a valid way to check if a path belongs to a file. This could cause problems in some (rare) cases.

@overheadhunter Remeber what we currently use as replacement for isRegularFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if there is something you prefer, so tell me.
This condition is currently used in:

if (Files.isReadable(vaultPasswordPath) && Files.isRegularFile(vaultPasswordPath)) {

so this line is just copied and negated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remeber what we currently use as replacement for isRegularFile?

We're using Files.isReadable() now.

@@ -133,4 +114,26 @@ public static void printUsage() {
new HelpFormatter().printHelp(USAGE, OPTIONS);
}

public PasswordStrategy addPasswortStrategy(final String vaultName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we actually need this method. I am aware it is used in CryptomatorCli.validate but in fact think we should move the whole validation stuff into the Args class. Args.parse should fail with exceptions if the provided args are invalid.

Nothing to be done in the related PR though, so OK for now.

LOG.warn("No console: non-interactive mode, instead use insecure replacement, PW is shown!");

BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));
LOG.info("Enter password: ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing the prompt through a LOG call only works well if logging is done to stdout. Maybe we should explicitly use stdout / the console to prompt for the password? Not a problem as long as we do logging the way we do it now. So must not be adressed to merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I will change this.

@didlich
Copy link
Contributor Author

didlich commented Dec 21, 2017

@markuskreusch I want to ask you about the time point the password is entered in the case password is read from standard input
in the current implementation the user is asked to input the password shortly before vault is decrypted
I wonder if we should ask the password in the args processing step, it has the drawback that the password is stored in memory the whole time the cli is running, but has the advantage all the passwords are available before we start processing the vaults.

What do you think?

@overheadhunter
Copy link
Member

it has the drawback that the password is stored in memory the whole time the cli is running

Of course we try to have the password as short a time as possible in memory, but even afterwards the derived key would be in memory for as long as the vault is unlocked. An attacker able to create memory dumps would still be able to use this key to decrypt the data. In other words, if the machine is already compromised, all is lost.

So in my opinion it is acceptable to have the password in memory for a longer period, if it serves a purpose.

@didlich
Copy link
Contributor Author

didlich commented Jan 10, 2018

@markuskreusch, @overheadhunter hi, could you have a look at the changes

@markuskreusch
Copy link
Contributor

I am really sorry for the delay. Forgot about this during the holidays.
Looks fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants