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

Add diceware and passgen to the cli interface #1406

Merged
merged 7 commits into from
Feb 7, 2018

Conversation

TheZ3ro
Copy link
Contributor

@TheZ3ro TheZ3ro commented Jan 19, 2018

Description

This PR adds the following keepassxc-cli commands:

  • passgen
  • diceware

Usage:
keepassxc-cli diceware 7
Will generate a passphrase of 7 words from the default wordlist, you can specify a custom wordlist with -w/--wordlist

keepassxc-cli passgen -lunes 20
Will generate a password of 20 letters with lowercase, uppercase, numbers, special chars and extended ascii chars

keepassxc-cli passgen -l 20
Will generate a password of 20 letters with only lowercase chars

How has this been tested?

Manually

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]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

commands.insert(QString("edit"), new Edit());
commands.insert(QString("estimate"), new Estimate());
commands.insert(QString("extract"), new Extract());
commands.insert(QString("locate"), new Locate());
commands.insert(QString("ls"), new List());
commands.insert(QString("merge"), new Merge());
commands.insert(QString("passgen"), new PassGen());
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro what about gen or generate? We already have estimate so generate isn't more verbose.

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 IDK, generate and estimate both refer to password but when I read the other commands all refers to entry or database (show, add, edit...).
Using generate feels like generating an entry instead of generating a password

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd go with estimate, but your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

QCommandLineParser parser;
parser.setApplicationDescription(this->description);
QCommandLineOption wordlistFile(QStringList() << "w"
<< "wordlist",
Copy link
Member

Choose a reason for hiding this comment

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

--word-list??

}

if (!dicewareGenerator.isValid()) {
outputTextStream << parser.helpText().replace("keepassxc-cli", "keepassxc-cli passgen");
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro passgen -> diceware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, my bad

PasswordGenerator passwordGenerator;

int length = args.at(0).toInt();
passwordGenerator.setLength(length);
Copy link
Member

Choose a reason for hiding this comment

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

}

if (classes == 0x0) {
passwordGenerator.setCharClasses(PasswordGenerator::LowerLetters | PasswordGenerator::UpperLetters |
Copy link
Member

Choose a reason for hiding this comment

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

These default char classes are defined at a couple of locations in the code. We should extract them to a constant in the PasswordGenerator class, just like we did for the default password length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a good idea


.SS "PassGen options"

.IP "-l"
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 have the same options in the add and edit commands when generating a new password. That way these options would be standard across the CLI, and this section could be renamed Password generation options

Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro not sure if you want to address this one in this PR, but if not, can you create an issue?

PassphraseGenerator dicewareGenerator;

int words = args.at(0).toInt();
dicewareGenerator.setWordCount(words);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a default word count in PasswordGenerator and make an option with the word count, using the default if the option is not set. That would be consistent with passgen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setWordCount already address the case where words == 0 and use a safe default. In the last commit I explicitly declare that

Copy link
Member

Choose a reason for hiding this comment

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

I'd still use an option for the word count instead of a positional argument, to be consistent with passgen, and to not have to explicitly pass 0 to use the defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but before they were both positional 😝

ok you got me, I will fix that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@TheZ3ro TheZ3ro force-pushed the feature/cli-passgen branch from 29105ff to b2cc34c Compare January 22, 2018 13:06
@@ -37,6 +40,9 @@ Lists the contents of a group in a database. If no group is specified, it will d
.IP "merge [options] <database1> <database2>"
Merges two databases together. The first database file is going to be replaced by the result of the merge, for that reason it is advisable to keep a backup of the two database files before attempting a merge. In the case that both databases make use of the same credentials, the \fI--same-credentials\fP or \fI-s\fP option can be used.

.IP "passgen [options] <length>"
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro update to generate, and length is no longer a positional argument

@@ -19,6 +19,9 @@ Adds a new entry to a database. A password can be generated (\fI-g\fP option), o
.IP "clip [options] <database> <entry> [timeout]"
Copies the password of a database entry to the clipboard. If multiple entries with the same name exist in different groups, only the password for the first one is going to be copied. For copying the password of an entry in a specific group, the group path to the entry should be specified as well, instead of just the name. Optionally, a timeout in seconds can be specified to automatically clear the clipboard.

.IP "diceware [options] <words>"
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro words is no longer a positional argument

@@ -104,6 +110,33 @@ with each attribute shown one-per-line in the given order. If no attributes are
specified, a summary of the default attributes is given.


.SS "Diceware options"

.IP "-w, --word-list <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 we need -W in the options here.

@louib
Copy link
Member

louib commented Jan 31, 2018

@TheZ3ro one last thing (I swear 😄), there are still some hardcoded password generation defaults in the http integration and the browser integration. I think we should use the defaults in PasswordGenerator everywhere.

@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Jan 31, 2018

@louib you mean replacing this?

return config()->get("generator/WordCount", 6).toInt();

@louib
Copy link
Member

louib commented Jan 31, 2018

@TheZ3ro yep, and the defaults for the char classes are also hardcoded, for example here and here.

@TheZ3ro TheZ3ro changed the base branch from develop to release/2.3.0 February 5, 2018 10:34
@TheZ3ro TheZ3ro force-pushed the feature/cli-passgen branch from 2c0ba13 to cd18a2b Compare February 5, 2018 11:32
@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Feb 5, 2018

@louib this should be it 😉

Copy link
Member

@louib louib left a comment

Choose a reason for hiding this comment

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

Good job! some small comments, but it'll be good for merging after!

@@ -28,6 +31,9 @@ Estimates the entropy of a password. The password to estimate can be provided as
.IP "extract [options] <database>"
Extracts and prints the contents of a database to standard output in XML format.

.IP "generate [options]"
Generate a random password
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro missing point at the end of the description.

Diceware::Diceware()
{
this->name = QString("diceware");
this->description = QObject::tr("Generate a new random password.");
Copy link
Member

Choose a reason for hiding this comment

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

@TheZ3ro Generate a random diceware passphrase.

classes |= PasswordGenerator::EASCII;
}

passwordGenerator.setCharClasses(classes);
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 we also set the default flags? If so we should do it in edit and add also.

parser.addOption(words);
QCommandLineOption wordlistFile(QStringList() << "w"
<< "word-list",
QObject::tr("Wordlist fot the diceware generator.\n[Default: EFF English]"),
Copy link
Member

Choose a reason for hiding this comment

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

for

parser.process(arguments);

const QStringList args = parser.positionalArguments();
if (args.size() != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

!args.isEmpty()

Generate::Generate()
{
this->name = QString("generate");
this->description = QObject::tr("Generate a new random password.");
Copy link
Member

@phoerious phoerious Feb 5, 2018

Choose a reason for hiding this comment

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

Remove explicit this (why don't you use the constructor's initializer list anyway?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is like all other cli Command do it, do I have to change them all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also fixed this for every cli command 🎉

Copy link
Member

Choose a reason for hiding this comment

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

Removing this is one thing, but both statements should actually be moved to the initializer list.

QObject::tr("length"));
parser.addOption(len);
QCommandLineOption lower(QStringList() << "l",
QObject::tr("Use lowercase in the generated password."));
Copy link
Member

Choose a reason for hiding this comment

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

...characters...

QObject::tr("Use lowercase in the generated password."));
parser.addOption(lower);
QCommandLineOption upper(QStringList() << "u",
QObject::tr("Use uppercase in the generated password."));
Copy link
Member

Choose a reason for hiding this comment

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

DItto

QObject::tr("Use special characters in the generated password."));
parser.addOption(special);
QCommandLineOption extended(QStringList() << "e",
QObject::tr("Use extended ascii in the generated password."));
Copy link
Member

Choose a reason for hiding this comment

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

ASCII characters

Use special characters for the generated password. [Default: Disabled]

.IP "-e"
Use extended ascii characters for the generated password. [Default: Disabled]
Copy link
Member

Choose a reason for hiding this comment

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

ASCII

static const bool DefaultSpecial = (DefaultCharset & SpecialCharacters) != 0;
static const bool DefaultEASCII = (DefaultCharset & EASCII) != 0;
static const bool DefaultLookAlike = (DefaultFlags & ExcludeLookAlike) != 0;
static const bool DefaultFromEveryGroup = (DefaultFlags & CharFromEveryGroup) != 0;
Copy link
Member

Choose a reason for hiding this comment

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

These should be constexpr.

};
Q_DECLARE_FLAGS(CharClasses, CharClass)

enum GeneratorFlag
{
ExcludeLookAlike = 0x1,
CharFromEveryGroup = 0x2
CharFromEveryGroup = 0x2,
DefaultFlags = ExcludeLookAlike | CharFromEveryGroup
};
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to convert these two enums to enum classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pretty big change, maybe we can ship this specific one in 2.4.0 or a 2.3 minor

Copy link
Member

Choose a reason for hiding this comment

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

It should be more or less an automatic refactor. You may have to add the class name at several locations where this enum is used without its full name, but if you encounter an instance where this creates problems, because both enums were mixed, you most certainly found a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that with enum class you don't have automatic casting error: no match for ‘operator&’ (operand types are PasswordGenerator::CharClass’ and ‘PasswordGenerator::CharClass’)
so we have to replace all the calls with static_casts.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'm fine with leaving it as-is for now.

{
}

int Generate::execute(QStringList arguments)
Copy link
Member

Choose a reason for hiding this comment

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

const QStringList&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this for every cli command

@phoerious phoerious added this to the v2.3.0 milestone Feb 5, 2018
@TheZ3ro TheZ3ro force-pushed the feature/cli-passgen branch from 37aba58 to 3027288 Compare February 6, 2018 22:31
@TheZ3ro
Copy link
Contributor Author

TheZ3ro commented Feb 6, 2018

Rebased. Ready to merge

@TheZ3ro TheZ3ro force-pushed the feature/cli-passgen branch from 3027288 to 1bfbb92 Compare February 7, 2018 16:36
@phoerious phoerious merged commit 13dc1e0 into release/2.3.0 Feb 7, 2018
@phoerious phoerious deleted the feature/cli-passgen branch February 7, 2018 19:26
phoerious added a commit that referenced this pull request Feb 27, 2018
- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494]
- Add SSH Agent feature [#1098, #1450, #1463]
- Add preview panel with details of the selected entry [#879, #1338]
- Add more and configurable columns to entry table and allow copying of values by double click [#1305]
- Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608]
- Deprecate KeePassHTTP [#1392]
- Add support for Steam one-time passwords [#1206]
- Add support for multiple Auto-Type sequences for a single entry [#1390]
- Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060]
- Replace qHttp with cURL for website icon downloads [#1460]
- Remove lock file [#1231]
- Add option to create backup file before saving [#1385]
- Ask to save a generated password before closing the entry password generator [#1499]
- Resolve placeholders recursively [#1078]
- Add Auto-Type button to the toolbar [#1056]
- Improve window focus handling for Auto-Type dialogs [#1204, #1490]
- Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412]
- Add optional dark tray icon [#1154]
- Add new "Unsafe saving" option to work around saving problems with file sync services [#1385]
- Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537]
- Add diceware password generator to CLI [#1406]
- Add --key-file option to CLI [#816, #824]
- Add DBus interface for opening and closing KeePassXC databases [#283]
- Add KDBX compression options to database settings [#1419]
- Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327]
- Correct reference resolution in entry fields [#1486]
- Fix window state and recent databases not being remembered on exit [#1453]
- Correct history item generation when configuring TOTP for an entry [#1446]
- Correct multiple TOTP bugs [#1414]
- Automatic saving after every change is now a default [#279]
- Allow creation of new entries during search [#1398]
- Correct menu issues on macOS [#1335]
- Allow compilation on OpenBSD [#1328]
- Improve entry attachments view [#1139, #1298]
- Fix auto lock for Gnome and Xfce [#910, #1249]
- Don't remember key files in file dialogs when the setting is disabled [#1188]
- Improve database merging and conflict resolution [#807, #1165]
- Fix macOS pasteboard issues [#1202]
- Improve startup times on some platforms [#1205]
- Hide the notes field by default [#1124]
- Toggle main window by clicking tray icon with the middle mouse button [#992]
- Fix custom icons not copied over when databases are merged [#1008]
- Allow use of DEL key to delete entries [#914]
- Correct intermittent crash due to stale history items [#1527]
- Sanitize newline characters in title, username and URL fields [#1502]
- Reopen previously opened databases in correct order [#774]
- Use system's zxcvbn library if available [#701]
- Implement various i18n improvements [#690, #875, #1436]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants