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

New Diceware passphrase generator #373

Merged
merged 12 commits into from
Apr 9, 2017

Conversation

TheZ3ro
Copy link
Contributor

@TheZ3ro TheZ3ro commented Mar 4, 2017

closes #21

Description

This PR adds a diceware-like passphrase generator.
It's possible to select the Word Count, the Word Separator and the Word List.
Currently 3 worlist are included (EFF Long, EFF Short and EFF Short Distant).
The Passphrase entropy is calculated by log2(wordlist.size) * wordcount.
All the options are automatically loaded and saved.

Advanced users If you want to add a different wordlist by yourself, you can add it in the KeePassXC installation folder. This will require a restart to see the wordlist in the comboBox.
If you select a wordlist with less 1000 words, the generator won't let you generate passphrase unless you select another wordlist.

I've removed the old PasswordComboBox widget since it isn't used anymore.

Problems that needs fix:

  • In the entry edit view the Password generator is very small because there isn't anymore space
  • In the entry edit view if the Generator is open you can't maximize the window.

How Has This Been Tested?

Screenshots (if appropriate):

istantanea_2017-03-04_14-53-44

Types of changes

  • ✅ New feature (non-breaking change which adds functionality)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]

m_wordlist.append(in.readLine());
}

if (m_wordlist.size() < 1000) {
Copy link
Member

@louib louib Mar 4, 2017

Choose a reason for hiding this comment

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

@TheZ3ro why this limit? Why not rely on the entropy meter to let the user know that the word list is too small?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@louib The limit is for preventing users use small wordlist. The entropy-meter will let the user use a phassphrase with 5 word from a 10-word wordlist but the user can simply ignore the "Password Quality: Bad" label and go with it.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO 1000 is also way too low.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phoerious EFF short and EFF short distant wordlist are 1200 words long

Copy link
Member

Choose a reason for hiding this comment

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

I don't really see a reason to use the short lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4000 is a fair limit?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds better than 1000.


// In case there was an error loading the wordlist
if(m_wordlist.length() == 0) {
QString empty;
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro could we do return QString(); here instead?

}

QString passphrase;
for (int i = 0; i < m_length; i ++) {
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro i++?

Copy link
Member

@phoerious phoerious Mar 4, 2017

Choose a reason for hiding this comment

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

++i!

QString passphrase;
for (int i = 0; i < m_length; i ++) {
int word_index = randomGen()->randomUInt(m_wordlist.length());
passphrase.append(m_wordlist.at(word_index));
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro purely stylistic, but here you could create a QStringList of all the randomly chosen words, and then use QStringList::join(m_separator).

QStringList words;
for (int i = 0; i < m_length; ++i) {
    int wordIndex = randomGen()->randomUInt(m_wordlist.length());
    words.append(m_wordlist.at(wordIndex))
}
return words.join(m_separator);

QString passphrase;
for (int i = 0; i < m_length; i ++) {
int word_index = randomGen()->randomUInt(m_wordlist.length());
passphrase.append(m_wordlist.at(word_index));
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro wordIndex?

@@ -54,6 +62,12 @@ PasswordGeneratorWidget::PasswordGeneratorWidget(QWidget* parent)
m_ui->entropyLabel->setFont(defaultFont);
m_ui->strengthLabel->setFont(defaultFont);
}

m_ui->comboBoxWordSeparator->addItems(QStringList() <<" " <<"#" <<";" <<"-" <<":" <<"." <<"@");
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro shouldn't there be spaces around the << operators?
<< " " << "#" << ";" << "-" << ":" << "." << "@"

@@ -65,6 +79,7 @@ PasswordGeneratorWidget::~PasswordGeneratorWidget()

void PasswordGeneratorWidget::loadSettings()
{
// Password config
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro we should remove the space at 87 to group all the password config then.

@@ -196,13 +248,13 @@ void PasswordGeneratorWidget::colorStrengthIndicator(double entropy)
// Set the color and background based on entropy
// colors are taking from the KDE breeze palette
// <https://community.kde.org/KDE_Visual_Design_Group/HIG/Color>
if (entropy < 35) {
if (entropy < 40) {
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro maybe these thresholds could be configurable at some point, for users that have different security requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I think the current entropy value are almost nice for the current password cracking technology.

Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro but a user might also want to increase those values.

m_generator->setLength(m_ui->spinBoxLength->value());
m_generator->setCharClasses(classes);
m_generator->setFlags(flags);
m_dicewareGenerator->setLength(m_ui->spinBoxWordCount->value());
Copy link
Member

@louib louib Mar 4, 2017

Choose a reason for hiding this comment

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

@TheZ3ro setLength is a bit ambiguous, since it's not really the length of the resulting passphrase, but instead its word count. In fact, the name of the spin box is wordCount. Why not setWordCount?

@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Mar 4, 2017

@louib I think I've fixed most of the thing. Should be nice to base #343 over this PR

@TheZ3ro TheZ3ro force-pushed the feature/new-password-generator branch from b59854e to 4d547d3 Compare March 4, 2017 21:36
@louib
Copy link
Member

louib commented Mar 4, 2017

@TheZ3ro I tested the new feature locally (not extensively though). Works great!

@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Mar 4, 2017

@louib have you tried the 2 problem described in the first post?

@louib
Copy link
Member

louib commented Mar 5, 2017

@TheZ3ro no, I only tested the basic functionalities (generating and changing the settings, apply, etc)

@J-J-Chiarella
Copy link
Contributor

Could you please add the underscore ( _ ) as an option for word separator?

It acts as a space, while still being a printable character.

Also, unlike the dash ( - ), it won't conflict with words in the list. Some words in the list are hyphenated. If anything, I think that dash should not be an option. But if I have to choose one thing to change, it would be to add the underscore. With popular sites like reddit allowing underscore and twitter making it the only acceptable punctuation, the underscore is not as obscure. (Certainly not as obscure as backtick or backslash or pipes.)

@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Mar 7, 2017

@J-J-Chiarella yes of course

@TheZ3ro TheZ3ro force-pushed the feature/new-password-generator branch from 4d547d3 to 1137a9e Compare March 8, 2017 15:22
@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Mar 8, 2017

Rebased on top of develop

@TheZ3ro TheZ3ro force-pushed the feature/new-password-generator branch from ea970bd to c8b8e97 Compare March 16, 2017 22:54
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

See comment.

@phoerious
Copy link
Member

phoerious commented Mar 16, 2017

Couple of things:

  • 0 words doesn't make any sense, minimum should be 1
  • I don't really like the giant Apply and Generate buttons, maybe experiment with that a little more
  • We should only ship one (very large) word list and hide the dropdown unless the user installs a custom word list
  • If there is more than one list, does the generator remember the most recently used one? If not, it should
  • The word separator setting should be a simple input field, no reason to restrict what characters a user can choose (and of course: KISS)
  • Replace tab titles Random and Diceware with Password and Passphrase
  • Labels and widgets should be arranged in a grid with the labels being aligned to the right

@TheZ3ro TheZ3ro force-pushed the feature/new-password-generator branch from c8b8e97 to 30cfb16 Compare March 16, 2017 23:13
@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Mar 17, 2017

istantanea_2017-03-17_01-25-38

@TheZ3ro TheZ3ro changed the title WIP: New Diceware passphrase generator New Diceware passphrase generator Mar 17, 2017

}

void PassphraseGenerator::setWordlist(QString path)
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro setWordList?

@TheZ3ro TheZ3ro force-pushed the feature/new-password-generator branch from f4451ab to 0c75584 Compare March 21, 2017 23:05
@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Mar 21, 2017

Rebased and fixed @louib

@louib
Copy link
Member

louib commented Mar 28, 2017

@phoerious are we good with this one?

@TheZ3ro TheZ3ro requested a review from phoerious April 1, 2017 12:17
@droidmonkey droidmonkey merged commit 5696178 into develop Apr 9, 2017
@droidmonkey droidmonkey deleted the feature/new-password-generator branch April 9, 2017 16:36
@droidmonkey
Copy link
Member

Excellent work on this everyone, this is a fantastic enhancement to the password generator!

@davad
Copy link

davad commented Apr 17, 2017

@TheZ3ro Thanks for doing this! Sorry I couldn't be more help. Life got busy 😬.

droidmonkey added a commit that referenced this pull request Jun 25, 2017
- Added YubiKey 2FA integration for unlocking databases [#127]
- Added TOTP support [#519]
- Added CSV import tool [#146, #490]
- Added KeePassXC CLI tool [#254]
- Added diceware password generator [#373]
- Added support for entry references [#370, #378]
- Added support for Twofish encryption [#167]
- Enabled DEP and ASLR for in-memory protection [#371]
- Enabled single instance mode [#510]
- Enabled portable mode [#645]
- Enabled database lock on screensaver and session lock [#545]
- Redesigned welcome screen with common features and recent databases [#292]
- Multiple updates to search behavior [#168, #213, #374, #471, #603, #654]
- Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480]
- Fixed auto-type errors on Linux [#550]
- Prompt user prior to executing a cmd:// URL [#235]
- Entry attributes can be protected (hidden) [#220]
- Added extended ascii to password generator [#538]
- Added new database icon to toolbar [#289]
- Added context menu entry to empty recycle bin in databases [#520]
- Added "apply" button to entry and group edit windows [#624]
- Added macOS tray icon and enabled minimize on close [#583]
- Fixed issues with unclean shutdowns [#170, #580]
- Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515]
- Compare window title to entry URLs [#556]
- Implemented inline error messages [#162]
- Ignore group expansion and other minor changes when making database "dirty" [#464]
- Updated license and copyright information on souce files [#632]
- Added contributors list to about dialog [#629]
@jeisoncp
Copy link

Would be great if we can use a diceware list in our mother language, like portuguese.

@droidmonkey
Copy link
Member

droidmonkey commented Aug 30, 2018

You can change the eff_large.wordlist to whatever you want.

@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Aug 31, 2018

@jeisoncp even better, place your wordlist file in the wordlist/ folder for your KeePassXC installation and a DropDown button will appear for selecting the wordlist to use

@jeisoncp
Copy link

@TheZ3ro Great, thanks by the advice. Maybe this files could be placed in default installation.

@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diceware-like passphrase generator
7 participants