ulif / diceware

Passphrases to remember
GNU General Public License v3.0
357 stars 45 forks source link

Feature realdice command argument #25

Closed dwcoder closed 8 years ago

dwcoder commented 8 years ago

This feature doesn't add any vital functionality. It adds a command argument --dice-rolls, to be used as follows:

diceware -n 1 --dice-rolls 1 2 4 5 3 2 1

The rolls is stored in a list, which are used before the user is prompted to roll a dice. When I use diceware, I usually roll a box full of dice and I read off the values from left to right. This makes it easier for me to input all the values at once in the initial command.

Also added unit tests for all the functionality, and updated the expected_output of the --help unit test.

Now, there is one big security caveat to this method: the bash commands are stored in your bash history, so the rolls that generated your password is shown in plaintext there. I only realised this late in the process, and I would understand if the merge was rejected just on the basis of this flaw.

A possible way to fix this, while keeping the functionality, is to add an argument like --oneline that would prompt the user to input a series of dice rolls in one long line, rather than asking the user roll-by-roll. The rolls wouldn't be in the bash history then. I would be happy to change my code to reflect such a request.

As I said, I only noticed this security issue after completing the request. I learned a lot from adding this feature, and I still use it to play around and experiment myself. Thus, I would not be piqued if the feature wasn't accepted, but I thought I would submit the request rather than disqualify it myself, just in case you see a benefit to it.

ulif commented 8 years ago

Hey, @dwcoder, thank you very much!

I really like the general idea to support non-interactive input with dice. These endless lines of questions ("what number shows x?") can become tedious and could keep people from generating longer passphrases. Something nobody wants. Hm, there are plenty of other good reasons for this feature. So, yes, generally I am very happy with it.

The "readable-in-shell-history" problem, however, is really serious. I would not have spotted this so quick, so, thanks for telling! It is in fact the only reason for rejecting this PR.

What can we do? You said: the rolls are stored in a list. So, why not create a list (as file) and read its content? This could allow to generate/input much longer lists of dice rolls without getting too tedious. I could imagine to accept a path to a file which contains numbers separated by whitespace (simple spaces, tabs, newlines, ...). After using such numbers from a file we might want to tell users, how many we used.

You might also want to keep in mind, that there are some people using non-six-sided dice. I yet tried to keep the code in a state so that support for n-sided dice is at least not completely blocked.

My suggestion therefore: I will add support for n-sided dice in the next days. Meanwhile you could review your code and make it to read data from a file instead from a commandline list.

What do you think?

dwcoder commented 8 years ago

Yeah @ulif, I think the read from file is a great idea to keep the functionality without the security flaw. I will look into the specifics thereof (haven't done that before, but now is a great time to learn). I'll work on it a few days and submit a new request when ready.

You might also want to keep in mind, that there are some people using non-six-sided dice. I yet tried to keep the code in a state so that support for n-sided dice is at least not completely blocked.

That is crucial. I believe that all the code I have written to date used your options.dice_sides variable for everything, so I think it should support an n-sided die as standard. But I will at some unit tests to that effect as well, just to be sure.

Thanks for the positive feedback.

ulif commented 8 years ago

Thank you, @dwcoder. I noticed you kept dice_sides :-)

Just released 0.8, so that users easily get your fixes and don't have to wait until we finished with the upcoming stuff. Please take your time!