Add diceware and passgen to the cli interface by TheZ3ro · Pull Request #1406 · keepassxreboot/keepassxc

TheZ3ro · 2018-01-19T15:25:42Z

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]

louib · 2018-01-19T15:29:50Z

src/cli/Command.cpp

        commands.insert(QString("locate"), new Locate());
        commands.insert(QString("ls"), new List());
        commands.insert(QString("merge"), new Merge());
+        commands.insert(QString("passgen"), new PassGen());


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

@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?

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

louib · 2018-01-19T15:30:28Z

src/cli/Diceware.cpp

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


--word-list??

louib · 2018-01-19T15:31:01Z

src/cli/Diceware.cpp

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


@TheZ3ro passgen -> diceware

Woops, my bad

louib · 2018-01-19T15:33:05Z

src/cli/PassGen.cpp

+    PasswordGenerator passwordGenerator;
+
+    int length = args.at(0).toInt();
+    passwordGenerator.setLength(length);


@TheZ3ro the length should be optional and use the default defined at https://github.com/keepassxreboot/keepassxc/blob/develop/src/core/PasswordGenerator.h#L61

louib · 2018-01-19T15:34:33Z

src/cli/PassGen.cpp

+    }
+
+    if (classes == 0x0) {
+        passwordGenerator.setCharClasses(PasswordGenerator::LowerLetters | PasswordGenerator::UpperLetters |


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.

Seems like a good idea

louib · 2018-01-19T16:51:38Z

src/cli/keepassxc-cli.1

+
+.SS "PassGen options"
+
+.IP "-l"


@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

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

louib · 2018-01-21T19:36:36Z

src/cli/Diceware.cpp

+    PassphraseGenerator dicewareGenerator;
+
+    int words = args.at(0).toInt();
+    dicewareGenerator.setWordCount(words);


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.

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

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.

but before they were both positional 😝

ok you got me, I will fix that :)

louib · 2018-01-31T03:19:45Z

src/cli/keepassxc-cli.1

 .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>"


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

louib · 2018-01-31T03:20:01Z

src/cli/keepassxc-cli.1

 .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>"


@TheZ3ro words is no longer a positional argument

louib · 2018-01-31T03:20:58Z

src/cli/keepassxc-cli.1


+.SS "Diceware options"
+
+.IP "-w, --word-list <path>"


@TheZ3ro we need -W in the options here.

louib · 2018-01-31T15:43:17Z

@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 · 2018-01-31T16:11:02Z

@louib you mean replacing this?

keepassxc/src/browser/BrowserSettings.cpp

Line 264 in ee3ccf1

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

louib · 2018-01-31T16:44:04Z

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

TheZ3ro · 2018-02-05T11:32:41Z

@louib this should be it 😉

louib

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

louib · 2018-02-05T18:33:09Z

src/cli/keepassxc-cli.1

 Extracts and prints the contents of a database to standard output in XML format.

+.IP "generate [options]"
+Generate a random password


@TheZ3ro missing point at the end of the description.

louib · 2018-02-05T18:33:55Z

src/cli/Diceware.cpp

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


@TheZ3ro Generate a random diceware passphrase.

louib · 2018-02-05T18:35:06Z

src/cli/Generate.cpp

+        classes |= PasswordGenerator::EASCII; 
+    }
+
+    passwordGenerator.setCharClasses(classes);


@TheZ3ro shouldn't we also set the default flags? If so we should do it in edit and add also.

phoerious · 2018-02-05T19:02:50Z

src/cli/Diceware.cpp

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


phoerious · 2018-02-05T19:03:10Z

src/cli/Diceware.cpp

+    parser.process(arguments);
+
+    const QStringList args = parser.positionalArguments();
+    if (args.size() != 0) {


!args.isEmpty()

phoerious · 2018-02-05T19:03:47Z

src/cli/Generate.cpp

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


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

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

Also fixed this for every cli command 🎉

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

phoerious · 2018-02-05T19:04:16Z

src/cli/Generate.cpp

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


...characters...

phoerious · 2018-02-05T19:04:21Z

src/cli/Generate.cpp

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


phoerious · 2018-02-05T19:04:39Z

src/cli/Generate.cpp

+                               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."));


ASCII characters

phoerious · 2018-02-05T19:05:44Z

src/cli/keepassxc-cli.1

+Use special characters for the generated password. [Default: Disabled]
+
+.IP "-e"
+Use extended ascii characters for the generated password. [Default: Disabled]


phoerious · 2018-02-05T19:06:48Z

src/core/PasswordGenerator.h

+    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;


These should be constexpr.

phoerious · 2018-02-05T19:07:02Z

src/core/PasswordGenerator.h

-        CharFromEveryGroup = 0x2
+        CharFromEveryGroup = 0x2,
+        DefaultFlags       = ExcludeLookAlike | CharFromEveryGroup
    };


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

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

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.

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.

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

phoerious · 2018-02-05T19:09:48Z

src/cli/Generate.cpp

+{
+}
+
+int Generate::execute(QStringList arguments)


const QStringList&

Fixed this for every cli command

TheZ3ro · 2018-02-06T22:31:34Z

Rebased. Ready to merge

explicitly state the wordcount default value

…I to XC_HTTP generator

- 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]

TheZ3ro requested review from droidmonkey, louib and phoerious January 19, 2018 15:25

TheZ3ro added the feature: CLI label Jan 19, 2018

louib reviewed Jan 19, 2018

View reviewed changes

louib reviewed Jan 21, 2018

View reviewed changes

TheZ3ro force-pushed the feature/cli-passgen branch from 29105ff to b2cc34c Compare January 22, 2018 13:06

louib reviewed Jan 31, 2018

View reviewed changes

src/cli/keepassxc-cli.1

.SS "Diceware options"

.IP "-w, --word-list <path>"

Copy link
Copy Markdown

Member

louib Jan 31, 2018

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.

TheZ3ro changed the base branch from develop to release/2.3.0 February 5, 2018 10:34

TheZ3ro force-pushed the feature/cli-passgen branch from 2c0ba13 to cd18a2b Compare February 5, 2018 11:32

louib requested changes Feb 5, 2018

View reviewed changes

phoerious requested changes Feb 5, 2018

View reviewed changes

phoerious reviewed Feb 5, 2018

View reviewed changes

phoerious added this to the v2.3.0 milestone Feb 5, 2018

louib approved these changes Feb 6, 2018

View reviewed changes

TheZ3ro force-pushed the feature/cli-passgen branch from 37aba58 to 3027288 Compare February 6, 2018 22:31

TheZ3ro added 2 commits February 7, 2018 17:35

add diceware and passgen to the cli interface

27f8aa0

use default password lenght + minor fixes

e9612ee

TheZ3ro added 5 commits February 7, 2018 17:35

add default charset when not specified

e57a2e0

explicitly state the wordcount default value

renamed passgen to generate and use diceware default wordcount

4782b20

update cli manpage

33b9583

centralize every password generator default option. add extended ASCI…

6723f42

…I to XC_HTTP generator

fix cli commands, translations and codestyle

1bfbb92

TheZ3ro force-pushed the feature/cli-passgen branch from 3027288 to 1bfbb92 Compare February 7, 2018 16:36

phoerious approved these changes Feb 7, 2018

View reviewed changes

phoerious merged commit 13dc1e0 into release/2.3.0 Feb 7, 2018

phoerious deleted the feature/cli-passgen branch February 7, 2018 19:26

Uh oh!

Conversation

TheZ3ro commented Jan 19, 2018

Description

How has this been tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louib commented Jan 31, 2018

Uh oh!

TheZ3ro commented Jan 31, 2018

Uh oh!

louib commented Jan 31, 2018

Uh oh!

TheZ3ro commented Feb 5, 2018

Uh oh!

louib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

phoerious Feb 5, 2018 •

edited

Loading