Add TOTP support #519

weslly · 2017-04-21T04:35:40Z

resolves #80

Description

This PR add support for TOTP code generation (as described by rfc6238) using KeepassXC.

Motivation and context

This is a rework from the ground up of my previous TOTP PR. It has more features, tests and the code is a lot cleaner than before.

Features

GUI for easily setting up the TOTP of an entry
Auto-Type with the {TOTP} tag
Support for codes with 6 and 8 digits
Custom time period support
It supports reading and updating the format used by the 2 TOTP plugins listed on Keepass plugins page (KeeOtp and Tray TOTP) plus the URI format used by Google Authenticator and Keeweb.

How has this been tested?

I added tests and have been using this with my personal database daily.

Screenshots:

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]
✅ I have added tests to cover my changes.

TheZ3ro · 2017-04-21T08:04:42Z

Wow! This is great! 🎉

yan12125 · 2017-04-22T10:00:11Z

Cloned and tried - awesome!

Seems in the current implementation, TOTP codes in the clipboard has the same expiry as other fields like usernames and passwords. If a TOTP code expires in less than 10 seconds, the value in the clipboard gets invalid. Although most websites accepts time shift within a few seconds, I feel it better to always use the correct code. Maybe copy the new code if the old code in the clipboard is expired?

weslly · 2017-04-22T12:23:10Z

@yan12125 Thanks for the feedback! That's definitely a good point, but wouldn't it get a bit annoying if you copy the value when it is just a couple of seconds away from the expiration time and alt-tab the window to paste it somewhere only to find it has been removed from the clipboard already? I'm also not sure if it would be a good idea to automatically modify the clipboard with an updated value without the user actually asking for it. As you said, most websites have a time window for old codes, so IMHO I don't think that would be a problem.

yan12125 · 2017-04-22T12:43:22Z

You're right - overwriting the clipboard content without user involvement sounds bad. I can't figure out a good solution for now. If that really causes problems in the future, we can always revisit it :)

louib · 2017-04-25T02:55:57Z

src/CMakeLists.txt

@@ -100,6 +100,8 @@ set(keepassx_SOURCES
    gui/SettingsWidget.cpp
    gui/SearchWidget.cpp
    gui/SortFilterHideProxyModel.cpp
+    gui/TotpDialog.cpp
+    gui/SetupTotpDialog.cpp


@weslly this line should go before gui/TotpDialog.cpp

louib · 2017-04-25T02:56:54Z

src/CMakeLists.txt

@@ -151,6 +155,8 @@ set(keepassx_FORMS
    gui/SearchWidget.ui
    gui/SettingsWidgetGeneral.ui
    gui/SettingsWidgetSecurity.ui
+    gui/TotpDialog.ui
+    gui/SetupTotpDialog.ui


@weslly this line should go before gui/TotpDialog.ui

louib · 2017-04-25T03:02:34Z

src/core/Entry.cpp

+    }
+
+    m_data.totpDigits = 6;
+    m_data.totpStep = 30;


@weslly the default steps and digits are defined 3 times in this file. Maybe we could extract them as constants and place them somewhere in the totp files.

louib · 2017-04-25T03:04:44Z

src/gui/DatabaseWidget.cpp

@@ -42,6 +42,8 @@
 #include "gui/ChangeMasterKeyWidget.h"
 #include "gui/Clipboard.h"
 #include "gui/CloneDialog.h"
+#include "gui/TotpDialog.h"
+#include "gui/SetupTotpDialog.h"


@weslly I also think this import list is sorted alphabetically.

louib · 2017-04-25T03:12:13Z

src/gui/SetupTotpDialog.cpp

@@ -0,0 +1,74 @@
+#include "SetupTotpDialog.h"


@weslly the copyright header is missing from this file (and some other new files in this PR)

@louib added and rebased

louib · 2017-04-25T03:13:12Z

src/totp/totp.cpp

+    }
+
+    if (step == 0) {
+        step = 30;


@weslly default steps and digits also defined here.

JulienLB · 2017-04-27T09:57:15Z

Hello and sincere thanks for all this superb work. I am an end-user of KeepassXC and need help to get this new totp feature working. Where should I post my question ?

TheZ3ro · 2017-04-27T10:26:07Z

src/gui/SetupTotpDialog.cpp

+
+void SetupTotpDialog::setStep(quint8 step)
+{
+    if (step != 30) {


Shouldn't this be QTotp::defaultStep ?

TheZ3ro · 2017-04-27T10:27:14Z

src/gui/SetupTotpDialog.cpp

+
+void SetupTotpDialog::setDigits(quint8 digits)
+{
+    if (digits != 6) {


Shouldn't this be QTotp::defaultDigits ?

TheZ3ro · 2017-04-27T10:27:34Z

src/gui/SetupTotpDialog.cpp

+
+void SetupTotpDialog::setupTotp()
+{
+    quint8 digits = 6;


Also this QTotp::defaultDigits

the-nic · 2017-04-28T13:18:45Z

Would it be possible do group the TOTP code (in groups of 3 digits), similar to what google authenticato does? Simply adding a small space would be sufficient, I guess and makes reading/remembering much easier in my opinion.

louib · 2017-05-02T17:15:18Z

src/core/Entry.cpp

+
+    if (m_attributes->hasKey("otp")) {
+        secret = m_attributes->value("otp");
+    } else {


@weslly here we assume the TOTP seed is present in the attributes. Shouldn't we check this with m_attributes->hasKey("TOTP Seed")?

louib · 2017-05-02T17:24:22Z

src/gui/SetupTotpDialog.cpp

+        digits = 8;
+    } else {
+        digits = 6;
+    }


@weslly small nitpick, but why not use the default here also?

quint8 digits = QTotp::defaultDigits; if (m_ui->radio8Digits->isChecked()) { digits = 8; }

@louib That would assume the default was always 6 but it wouldn't work if the default was 8 and the user checked 6 instead. Realistically the default is always 6 but that could cause problems if someone compiled their own version with 8 as the default at QTotp::defaultDigits.

louib · 2017-05-02T17:25:57Z

src/gui/SetupTotpDialog.cpp

+
+void SetupTotpDialog::setDigits(quint8 digits)
+{
+    if (digits == 8) {


@weslly you could test using digits != QTotp::defaultDigits here as well.

@louib Same as above

louib · 2017-05-02T17:30:27Z

src/gui/TotpDialog.cpp

+    uCounter = resetCounter();
+    updateProgressBar();
+
+    QTimer *timer = new QTimer(this);


@weslly QTimer* timer

louib · 2017-05-02T17:46:20Z

src/totp/totp.cpp

+}
+
+
+QByteArray QTotp::base32_decode(const QByteArray encoded)


@weslly This is taken from google authenticator? Maybe it would be better to leave it in a separate file and preserve the original copyright notice? Also maybe adding a link to the original source of the file.

louib · 2017-05-02T17:50:03Z

src/totp/totp.cpp

+}
+
+
+QString QTotp::generateTotp(const QByteArray key, quint64 time, const quint8 numDigits = defaultDigits, const quint8 step = defaultStep)


@weslly you could split that line.

louib · 2017-05-02T17:54:07Z

tests/TestTotp.cpp

+}
+
+
+void TestTotp::testSecret()


@weslly maybe name it testParseSecret, since it's testing the QTotp::parseOtpString function.

TheZ3ro · 2017-05-02T21:25:12Z

I think when the requests from @louib are adressed we can rebase and merge this! @weslly

louib · 2017-05-03T01:54:28Z

@weslly @TheZ3ro displaying the code as 343 343 might also be a good idea, as @the-nic proposed. What do you guys think?

droidmonkey · 2017-05-03T03:46:14Z

I like the display trick as long as copy paste still works properly.

weslly · 2017-05-04T00:28:20Z

I finished the review fixes and added @the-nic's suggestion to display the code in halfs.

ping @louib and @TheZ3ro

louib · 2017-05-04T01:54:08Z

I took the time to test this, and it works great so far! Good job @weslly

TheZ3ro

🎉

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

jrodan · 2018-04-04T14:38:57Z

Hi guys,

regarding AutoType - how can I configure this OTP with AutoType?

Thank you!

droidmonkey · 2018-04-04T14:56:39Z

Use the {TOTP} fill in, see Autotype Wiki for more info.

TheZ3ro requested review from phoerious, droidmonkey and TheZ3ro and removed request for phoerious April 24, 2017 23:39

louib reviewed Apr 25, 2017

View reviewed changes

src/totp/totp.cpp Outdated

}

if (step == 0) {

step = 30;

Copy link

Member

louib Apr 25, 2017

Choose a reason for hiding this comment

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

@weslly default steps and digits also defined here.

TheZ3ro reviewed Apr 27, 2017

View reviewed changes

src/gui/SetupTotpDialog.cpp Outdated

void SetupTotpDialog::setupTotp()

{

quint8 digits = 6;

Copy link

Contributor

TheZ3ro Apr 27, 2017

Choose a reason for hiding this comment

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

Also this QTotp::defaultDigits

louib reviewed May 2, 2017

View reviewed changes

weslly added 3 commits May 3, 2017 20:55

Add TOTP support

bf57a28

Review fixes

4c9b8c7

Display TOTP code split in halfs

d3ed14e

Parse TOTP input string before first save

3640053

louib approved these changes May 4, 2017

View reviewed changes

TheZ3ro approved these changes May 4, 2017

View reviewed changes

TheZ3ro merged commit 1870b95 into keepassxreboot:develop May 4, 2017

phoerious added this to the v2.2.0 milestone May 7, 2017

phoerious added the new feature label May 7, 2017

weslly deleted the feature/totp branch June 21, 2017 23:15

weslly added the feature: TOTP label Aug 4, 2017

SVilgelm mentioned this pull request Jan 22, 2018

Added basic support for OTP generation based on the KeeOTP data format MiniKeePass/MiniKeePass#344

Open

phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TOTP support #519

Add TOTP support #519

weslly commented Apr 21, 2017

TheZ3ro commented Apr 21, 2017

yan12125 commented Apr 22, 2017

weslly commented Apr 22, 2017 •

edited

Loading

yan12125 commented Apr 22, 2017

louib Apr 25, 2017 •

edited

Loading

louib Apr 25, 2017

louib Apr 25, 2017

louib Apr 25, 2017

louib Apr 25, 2017

weslly Apr 25, 2017

louib Apr 25, 2017

JulienLB commented Apr 27, 2017

TheZ3ro Apr 27, 2017

TheZ3ro Apr 27, 2017

TheZ3ro Apr 27, 2017

the-nic commented Apr 28, 2017

louib May 2, 2017 •

edited

Loading

louib May 2, 2017

weslly May 2, 2017 •

edited

Loading

louib May 2, 2017

weslly May 2, 2017

louib May 2, 2017

louib May 2, 2017 •

edited

Loading

louib May 2, 2017

louib May 2, 2017

TheZ3ro commented May 2, 2017

louib commented May 3, 2017

droidmonkey commented May 3, 2017

weslly commented May 4, 2017

louib commented May 4, 2017

TheZ3ro left a comment

jrodan commented Apr 4, 2018

droidmonkey commented Apr 4, 2018 •

edited

Loading

		}


		QString QTotp::generateTotp(const QByteArray key, quint64 time, const quint8 numDigits = defaultDigits, const quint8 step = defaultStep)

Add TOTP support #519

Add TOTP support #519

Conversation

weslly commented Apr 21, 2017

Description

Motivation and context

Features

How has this been tested?

Screenshots:

Types of changes

Checklist:

TheZ3ro commented Apr 21, 2017

yan12125 commented Apr 22, 2017

weslly commented Apr 22, 2017 • edited Loading

yan12125 commented Apr 22, 2017

louib Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulienLB commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-nic commented Apr 28, 2017

louib May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weslly May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louib May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented May 2, 2017

louib commented May 3, 2017

droidmonkey commented May 3, 2017

weslly commented May 4, 2017

louib commented May 4, 2017

TheZ3ro left a comment

Choose a reason for hiding this comment

jrodan commented Apr 4, 2018

droidmonkey commented Apr 4, 2018 • edited Loading

weslly commented Apr 22, 2017 •

edited

Loading

louib Apr 25, 2017 •

edited

Loading

louib May 2, 2017 •

edited

Loading

weslly May 2, 2017 •

edited

Loading

louib May 2, 2017 •

edited

Loading

droidmonkey commented Apr 4, 2018 •

edited

Loading