CLI: Cleanup create options (#4313)

* Add ability to create database with an empty password
* Add password repeat check
* Standardize process between `db-create` and `import` commands
* Improve db-create tests with new password repeat

Co-authored-by: Jonathan White <support@dmapps.us>

CHANGELOG.md

@@ -7,6 +7,8 @@
 
 ### Changed
 - Renamed CLI create command to db-create [#4231]
+- Added --set-password option for CLI db-create command
+- Added --set-key-file option for CLI db-create command (replacing --key-file option)
 
 ## 2.5.3 (2020-01-19)
 

share/docs/man/keepassxc-cli.1

@@ -29,7 +29,7 @@ Copies an attribute or the current TOTP (if the \fI-t\fP option is specified) of
 In interactive mode, closes the currently opened database (see \fIopen\fP).
 
 .IP "\fBdb-create\fP [options] <database>"
-Creates a new database with a key file and/or password. The key file will be created if the file that is referred to does not exist. If both the key file and password are empty, no database will be created.
+Creates a new database with a password and/or a key file. The key file will be created if the file that is referred to does not exist. If both the key file and password are empty, no database will be created.
 
 .IP "\fBdb-info\fP [options] <database>"
 Show a database's information.
@@ -185,6 +185,12 @@ Will report an error if no TOTP is configured for the entry.
 
 .SS "Create options"
 
+.IP "\fB-k\fP, \fB--set-key-file\fP <path>"
+Set the key file for the database.
+
+.IP "\fB-p\fP, \fB--set-password\fP"
+Set a password for the database.
+
 .IP "\fB-t\fP, \fB--decryption-time\fP <time>"
 Target decryption time in MS for the database.
 

src/cli/Create.cpp

@@ -36,12 +36,24 @@ const QCommandLineOption Create::DecryptionTimeOption =
                        QObject::tr("Target decryption time in MS for the database."),
                        QObject::tr("time"));
 
+const QCommandLineOption Create::SetKeyFileOption =
+    QCommandLineOption(QStringList() << "k"
+                                     << "set-key-file",
+                       QObject::tr("Set the key file for the database."),
+                       QObject::tr("path"));
+
+const QCommandLineOption Create::SetPasswordOption =
+    QCommandLineOption(QStringList() << "p"
+                                     << "set-password",
+                       QObject::tr("Set a password for the database."));
+
 Create::Create()
 {
     name = QString("db-create");
     description = QObject::tr("Create a new database.");
     positionalArguments.append({QString("database"), QObject::tr("Path of the database."), QString("")});
-    options.append(Command::KeyFileOption);
+    options.append(Create::SetKeyFileOption);
+    options.append(Create::SetPasswordOption);
     options.append(Create::DecryptionTimeOption);
 }
 
@@ -97,21 +109,26 @@ int Create::execute(const QStringList& arguments)
 
     auto key = QSharedPointer<CompositeKey>::create();
 
-    auto password = Utils::getPasswordFromStdin();
+    if (parser->isSet(Create::SetPasswordOption)) {
-    if (!password.isNull()) {
+        auto passwordKey = Utils::getPasswordFromStdin();
-        key->addKey(password);
+        if (passwordKey.isNull()) {
+            err << QObject::tr("Failed to set database password.") << endl;
+            return EXIT_FAILURE;
+        }
+        key->addKey(passwordKey);
     }
 
-    QSharedPointer<FileKey> fileKey;
+    if (parser->isSet(Create::SetKeyFileOption)) {
-    if (parser->isSet(Command::KeyFileOption)) {
+        QSharedPointer<FileKey> fileKey;
-        if (!loadFileKey(parser->value(Command::KeyFileOption), fileKey)) {
+
+        if (!loadFileKey(parser->value(Create::SetKeyFileOption), fileKey)) {
             err << QObject::tr("Loading the key file failed") << endl;
             return EXIT_FAILURE;
         }
-    }
 
-    if (!fileKey.isNull()) {
+        if (!fileKey.isNull()) {
-        key->addKey(fileKey);
+            key->addKey(fileKey);
+        }
     }
 
     if (key->isEmpty()) {

src/cli/Create.h

@@ -28,6 +28,8 @@ public:
     Create();
     int execute(const QStringList& arguments) override;
 
+    static const QCommandLineOption SetKeyFileOption;
+    static const QCommandLineOption SetPasswordOption;
     static const QCommandLineOption DecryptionTimeOption;
 
 private:

src/cli/Import.cpp

@@ -70,10 +70,12 @@ int Import::execute(const QStringList& arguments)
 
     auto key = QSharedPointer<CompositeKey>::create();
 
-    auto password = Utils::getPasswordFromStdin();
+    auto passwordKey = Utils::getPasswordFromStdin();
-    if (!password.isNull()) {
+    if (passwordKey.isNull()) {
-        key->addKey(password);
+        errorTextStream << QObject::tr("Failed to set database password.") << endl;
+        return EXIT_FAILURE;
     }
+    key->addKey(passwordKey);
 
     if (key->isEmpty()) {
         errorTextStream << QObject::tr("No key is set. Aborting database creation.") << endl;

src/cli/Utils.cpp

@@ -93,9 +93,12 @@ namespace Utils
          *
          * @param password password to return next
          */
-        void setNextPassword(const QString& password)
+        void setNextPassword(const QString& password, bool repeat)
         {
             nextPasswords.append(password);
+            if (repeat) {
+                nextPasswords.append(password);
+            }
         }
     } // namespace Test
 
@@ -232,10 +235,31 @@ namespace Utils
 
         out << QObject::tr("Enter password to encrypt database (optional): ");
         out.flush();
-        QString password = Utils::getPassword();
+        auto password = Utils::getPassword();
 
-        if (!password.isEmpty()) {
+        if (password.isEmpty()) {
-            passwordKey = QSharedPointer<PasswordKey>(new PasswordKey(password));
+            out << QObject::tr("Do you want to create a database with an empty password? [y/N]: ");
+            out.flush();
+            TextStream ts(STDIN, QIODevice::ReadOnly);
+            if (!ts.device()->isSequential()) {
+                // This is required for testing on macOS
+                ts.seek(0);
+            }
+            auto ans = ts.readLine();
+            if (ans.toLower().startsWith("y")) {
+                passwordKey = QSharedPointer<PasswordKey>::create("");
+            }
+            out << endl;
+        } else {
+            out << QObject::tr("Repeat password: ");
+            out.flush();
+            auto repeat = Utils::getPassword();
+
+            if (password == repeat) {
+                passwordKey = QSharedPointer<PasswordKey>::create(password);
+            } else {
+                out << QObject::tr("Error: Passwords do not match.") << endl;
+            }
         }
 
         return passwordKey;

src/cli/Utils.h

@@ -62,7 +62,7 @@ namespace Utils
 
     namespace Test
     {
-        void setNextPassword(const QString& password);
+        void setNextPassword(const QString& password, bool repeat = false);
     }
 }; // namespace Utils
 

tests/TestCli.cpp

@@ -566,73 +566,133 @@ void TestCli::testCreate()
     QVERIFY(createCmd.getDescriptionLine().contains(createCmd.name));
 
     QScopedPointer<QTemporaryDir> testDir(new QTemporaryDir());
+    QString dbFilename;
 
-    QString databaseFilename = testDir->path() + "/testCreate1.kdbx";
+    // Testing password option, password mismatch
-    // Password
+    dbFilename = testDir->path() + "/testCreate_pw.kdbx";
     Utils::Test::setNextPassword("a");
-    createCmd.execute({"db-create", databaseFilename});
+    Utils::Test::setNextPassword("b");
-
+    createCmd.execute({"db-create", dbFilename, "-p"});
     m_stderrFile->reset();
     m_stdoutFile->reset();
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Error: Passwords do not match.\n"));
+    QCOMPARE(m_stderrFile->readLine(), QByteArray("Failed to set database password.\n"));
+
+    // Testing password option
+    Utils::Test::setNextPassword("a", true);
+    qint64 pos = m_stdoutFile->pos();
+    qint64 errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename, "-p"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
 
     Utils::Test::setNextPassword("a");
-    auto db = QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename, true, "", "", Utils::DEVNULL));
+    auto db = Utils::unlockDatabase(dbFilename, true, "", "", Utils::DEVNULL);
+    QVERIFY(db);
+
+    // Testing with empty password (deny it)
+    dbFilename = testDir->path() + "/testCreate_blankpw.kdbx";
+    Utils::Test::setNextPassword("");
+    m_stdinFile->reset();
+    m_stdinFile->write("n\n");
+    m_stdinFile->reset();
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename, "-p"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QVERIFY(m_stdoutFile->readLine().contains("empty password"));
+    QCOMPARE(m_stderrFile->readLine(), QByteArray("Failed to set database password.\n"));
+
+    // Testing with empty password (accept it)
+    Utils::Test::setNextPassword("");
+    m_stdinFile->reset();
+    m_stdinFile->write("y\n");
+    m_stdinFile->reset();
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename, "-p"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QVERIFY(m_stdoutFile->readLine().contains("empty password"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
+
+    Utils::Test::setNextPassword("");
+    db = Utils::unlockDatabase(dbFilename, true, "", "", Utils::DEVNULL);
     QVERIFY(db);
 
     // Should refuse to create the database if it already exists.
-    qint64 pos = m_stdoutFile->pos();
+    pos = m_stdoutFile->pos();
-    qint64 errPos = m_stderrFile->pos();
+    errPos = m_stderrFile->pos();
-    createCmd.execute({"db-create", databaseFilename});
+    createCmd.execute({"db-create", "-p", dbFilename});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
     // Output should be empty when there is an error.
     QCOMPARE(m_stdoutFile->readAll(), QByteArray(""));
-    QString errorMessage = QString("File " + databaseFilename + " already exists.\n");
+    QString errorMessage = QString("File " + dbFilename + " already exists.\n");
     QCOMPARE(m_stderrFile->readAll(), errorMessage.toUtf8());
 
+    // Should refuse to create without any key provided.
+    dbFilename = testDir->path() + "/testCreate_key.kdbx";
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+    QCOMPARE(m_stdoutFile->readAll(), QByteArray(""));
+    QCOMPARE(m_stderrFile->readLine(), QByteArray("No key is set. Aborting database creation.\n"));
+
     // Testing with keyfile creation
-    QString databaseFilename2 = testDir->path() + "/testCreate2.kdbx";
+    dbFilename = testDir->path() + "/testCreate_key2.kdbx";
     QString keyfilePath = testDir->path() + "/keyfile.txt";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
-    createCmd.execute({"db-create", databaseFilename2, "-k", keyfilePath});
+    createCmd.execute({"db-create", dbFilename, "-p", "-k", keyfilePath});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
 
     Utils::Test::setNextPassword("a");
-    auto db2 =
+    db = Utils::unlockDatabase(dbFilename, true, keyfilePath, "", Utils::DEVNULL);
-        QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename2, true, keyfilePath, "", Utils::DEVNULL));
+    QVERIFY(db);
-    QVERIFY(db2);
 
     // Testing with existing keyfile
-    QString databaseFilename3 = testDir->path() + "/testCreate3.kdbx";
+    dbFilename = testDir->path() + "/testCreate_key3.kdbx";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
-    createCmd.execute({"db-create", databaseFilename3, "-k", keyfilePath});
+    createCmd.execute({"db-create", dbFilename, "-p", "-k", keyfilePath});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
 
     Utils::Test::setNextPassword("a");
-    auto db3 =
+    db = Utils::unlockDatabase(dbFilename, true, keyfilePath, "", Utils::DEVNULL);
-        QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename3, true, keyfilePath, "", Utils::DEVNULL));
+    QVERIFY(db);
-    QVERIFY(db3);
 
     // Invalid decryption time (format).
-    QString databaseFilename4 = testDir->path() + "/testCreate4.kdbx";
+    dbFilename = testDir->path() + "/testCreate_time.kdbx";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    createCmd.execute({"create", databaseFilename4, "-t", "NAN"});
+    createCmd.execute({"db-create", dbFilename, "-p", "-t", "NAN"});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
@@ -642,7 +702,7 @@ void TestCli::testCreate()
     // Invalid decryption time (range).
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    createCmd.execute({"create", databaseFilename4, "-t", "10"});
+    createCmd.execute({"db-create", dbFilename, "-p", "-t", "10"});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
@@ -653,9 +713,9 @@ void TestCli::testCreate()
     // Custom encryption time
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
     int epochBefore = QDateTime::currentMSecsSinceEpoch();
-    createCmd.execute({"create", databaseFilename4, "-t", QString::number(encryptionTime)});
+    createCmd.execute({"db-create", dbFilename, "-p", "-t", QString::number(encryptionTime)});
     // Removing 100ms to make sure we account for changes in computation time.
     QVERIFY(QDateTime::currentMSecsSinceEpoch() > (epochBefore + encryptionTime - 100));
     m_stdoutFile->seek(pos);
@@ -663,12 +723,13 @@ void TestCli::testCreate()
 
     QCOMPARE(m_stderrFile->readAll(), QByteArray(""));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Benchmarking key derivation function for 500ms delay.\n"));
     QVERIFY(m_stdoutFile->readLine().contains(QByteArray("rounds for key derivation function.\n")));
 
     Utils::Test::setNextPassword("a");
-    auto db4 = QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename4, true, "", "", Utils::DEVNULL));
+    db = Utils::unlockDatabase(dbFilename, true, "", "", Utils::DEVNULL);
-    QVERIFY(db4);
+    QVERIFY(db);
 }
 
 void TestCli::testInfo()
@@ -1105,13 +1166,14 @@ void TestCli::testImport()
     QScopedPointer<QTemporaryDir> testDir(new QTemporaryDir());
     QString databaseFilename = testDir->path() + "testImport1.kdbx";
 
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
     importCmd.execute({"import", m_xmlFile->fileName(), databaseFilename});
 
     m_stderrFile->reset();
     m_stdoutFile->reset();
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully imported database.\n"));
 
     Utils::Test::setNextPassword("a");
@@ -1137,11 +1199,13 @@ void TestCli::testImport()
     QString databaseFilenameQuiet = testDirQuiet->path() + "testImport2.kdbx";
 
     pos = m_stdoutFile->pos();
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
     importCmd.execute({"import", "-q", m_xmlFile->fileName(), databaseFilenameQuiet});
     m_stdoutFile->seek(pos);
 
-    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray());
 
     Utils::Test::setNextPassword("a");
     auto dbQuiet = QSharedPointer<Database>(Utils::unlockDatabase(databaseFilenameQuiet, true, "", "", Utils::DEVNULL));
@@ -1544,11 +1608,11 @@ void TestCli::testMergeWithKeys()
 
     qint64 pos = m_stdoutFile->pos();
 
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
-    createCmd.execute({"db-create", sourceDatabaseFilename, "-k", sourceKeyfilePath});
+    createCmd.execute({"db-create", sourceDatabaseFilename, "-p", "-k", sourceKeyfilePath});
 
-    Utils::Test::setNextPassword("b");
+    Utils::Test::setNextPassword("b", true);
-    createCmd.execute({"db-create", targetDatabaseFilename, "-k", targetKeyfilePath});
+    createCmd.execute({"db-create", targetDatabaseFilename, "-p", "-k", targetKeyfilePath});
 
     Utils::Test::setNextPassword("a");
     auto sourceDatabase = QSharedPointer<Database>(