Add initial Steam TOTP support #1206

joelsmith · 2017-11-20T18:25:05Z

Add initial Steam TOTP support

Add the concept of custom TOTP encoders, each with potential for custom
code alphabet, length, step interval and code direction (i.e. reversed)
Select custom encoder via overload of the digits field of a loaded entry
Allow selection of custom encoders via the "TOTP Settings" field's
size, as currently done by KeeTrayTOTP for Steam. Use "S" for the
short name of the Steam custom encoder
Allow selection of custom encoders via the "otp" field by appending
a "&encoder=" field to the URL query. For example,
"&encoder=steam"
Update TOTP set-up dialog to permit selection between (default,
steam, custom) settings.

Description

Adds the ability to generate Steam Guard TOTP codes using the secret
associated with a Steam account.

Motivation and context

Fixes #594

How has this been tested?

Unit tests added

Tested existing TOTP entries

Added new Steam TOTP entries and verified code output

Screenshots (if appropriate):

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.

weslly · 2017-11-20T20:35:47Z

Looks pretty good! I just tested and it works, ~~but the timing seems to be a few seconds off (about 10sec). My guess is the mobile app uses the server_time value as the offset instead of the device time, is this correct?~~

joelsmith · 2017-11-20T20:42:57Z

This relies upon the local device time being correct. I'm not sure if it's better to try to figure out the right time before generating a code or to just have a requirement that the user keep their system time up to date.

For what it's worth, when I run it on my PC, it's sync'ed pretty much perfectly with my phone's Steam app.

weslly · 2017-11-20T20:47:14Z

Actually my phone clock was a few seconds ahead from my computer time, I updated it and the codes are changing at the same time.

phoerious · 2017-11-20T20:31:01Z

src/core/Entry.cpp

-        QRegExp rx("(\\d+);(\\d)", Qt::CaseInsensitive, QRegExp::RegExp);
+        QString digitsRx = "(?:\\d+)";
+        // build a regex that supports any encoders that we support.
+        foreach (QTotp::Encoder encoder, QTotp::encoders) {


Don't use the foreach macro, use C++11 range-based for loops (remember to cast to const to avoid detaching).

phoerious · 2017-11-20T20:31:57Z

src/core/Entry.cpp

+        foreach (QTotp::Encoder encoder, QTotp::encoders) {
+            digitsRx.append("|").append(encoder.shortName);
+        }
+        QRegExp rx(QString("(\\d+);(%1)").arg(digitsRx));


Use QRegularExpression, not QRegExp. I'm also not too big a fan of dynamically assembled regular expressions. They can easily introduce bugs and even security issues if not escaped properly.

phoerious · 2017-11-20T20:41:03Z

src/gui/SetupTotpDialog.cpp

+
+    bool isDefault = step == QTotp::defaultStep &&
+        digits == QTotp::defaultDigits;
+    bool isSteam = digits == QTotp::ENCODER_STEAM;


Maybe use brackets here to visually separate the parts of these statements better.

phoerious · 2017-11-20T20:43:08Z

src/totp/totp.cpp

@@ -31,6 +31,28 @@
 const quint8 QTotp::defaultStep = 30;
 const quint8 QTotp::defaultDigits = 6;

+// Custom encoder types. Each should be unique and >= 128 and < 255
+// Values have no meaning outside of keepassxc
+const quint8 QTotp::ENCODER_STEAM = 254;


Use

/** * ... */

doc comments.

phoerious · 2017-11-20T20:47:38Z

src/totp/totp.cpp

-                              const quint8& digits,
-                              const quint8& step)
+                              quint8 digits,
+                              quint8 step)


phoerious · 2017-11-20T20:49:27Z

tests/TestTotp.cpp

+    for (QMap<quint8, QTotp::Encoder>::const_iterator iter = QTotp::encoders.cbegin(); iter != QTotp::encoders.cend(); ++iter) {
+        const QTotp::Encoder& enc = iter.value();
+        QVERIFY2(enc.digits != 0,
+            qPrintable(QString("Custom encoders cannot have zero-value for digits field: %1(%2)").arg(enc.name).arg(iter.key())));


Please shorten the lines here. Insert line breaks after arg() calls like so:

qPrintable(QString("Custom encoders cannot have zero-value for digits field: %1(%2)") .arg(enc.name) .arg(iter.key())));

phoerious · 2017-11-20T20:51:14Z

tests/TestTotp.cpp

+            qPrintable(QString("nameToEncoder doesn't reference this custome encoder: %1(%2) %3").arg(enc.name).arg(iter.key()).arg(enc.shortName)));
+    }
+
+    for (QMap<QString, quint8>::const_iterator iter = QTotp::nameToEncoder.cbegin(); iter != QTotp::nameToEncoder.cend(); ++iter) {


Use range-based for on QTotp::nameToEncoder.keys().

I'm happy to change it (especially since the data set is small).

IMO, that's one that Qt got wrong. Doing an allocation to create a temporary list of keys, then iterating over the keys then doing a map lookup to get the value makes what should be an O(n) operation into an O(n*log(n)) operation. But Qt doesn't provide a way to iterate over the key/value pair in O(n) except by using the iterators in a way that isn't compatible with range-based for loops.

The keys() approach is a lot more readable, though. Not too happy with either solution. I guess it's fine as is.

phoerious · 2017-11-20T20:51:20Z

tests/TestTotp.cpp

+                qPrintable(QString("nameToEncoder doesn't reference the right custom encoder: %1(%2)").arg(iter.value()).arg(iter.key())));
+    }
+
+    for (QMap<QString, quint8>::const_iterator iter = QTotp::shortNameToEncoder.cbegin(); iter != QTotp::shortNameToEncoder.cend(); ++iter) {


phoerious · 2017-11-20T20:52:20Z

tests/TestTotp.cpp

+    QString secret = "otpauth://totp/"
+                     "test:[email protected]?secret=63BEDWCQZKTQWPESARIERL5DTTQFCJTK&issuer=Valve&algorithm="
+                     "SHA1&digits=5&period=30&encoder=steam";
+    QCOMPARE(QTotp::parseOtpString(secret, digits, step), QString("63BEDWCQZKTQWPESARIERL5DTTQFCJTK"));


Are there any official references which say that these values are correct? If yes, please reference them.

There are not. I created a throw-away Steam account, then used their Steam app and captured their codes at two separate times. Should I add a note to that effect?

Perhaps not a bad idea.

joelsmith · 2017-11-20T21:50:44Z

@phoerious changes pushed based upon your feedback. PTAL.

phoerious

You forgot something. 😉

phoerious · 2017-11-20T23:36:45Z

src/totp/totp.cpp

+// in the corresponding Encoder
+// NOTE: when updating this map, a corresponding edit to the settings regex must be made
+//       in Entry::totpSeed()
+const QMap<QString, quint8> QTotp::shortNameToEncoder{


Please use doc comments here, too.

phoerious · 2017-11-20T23:36:52Z

src/totp/totp.cpp

+// These map the "encoder=" URL parameter of the "otp" field to our internal encoder number
+// that overloads the digits field. Make sure that the key matches the name value
+// in the corresponding Encoder
+const QMap<QString, quint8> QTotp::nameToEncoder{


adolfogc · 2017-11-20T23:56:09Z

Will this be a good time to, respectively, rename src/totp/totp.h and src/totp/totp.cpp to src/totp/QTotp.h and src/totp/QTotp.cpp?

joelsmith · 2017-11-21T00:02:13Z

@phoerious thanks for your further review. I have added the comments that I missed. If you're happy with the changes, I'll rebase and squash.

joelsmith · 2017-11-21T00:04:58Z

@adolfogc IMO, the file and the class should be renamed Totp since it has nothing to do with Qt. I'd say that now is a fine time for a rename (but in a separate PR).

phoerious · 2017-11-21T00:06:30Z

I agree that QTotp should be renamed to Totp. QTotp is totally misleading. I think that should be done in a separate PR, though.

* Add the concept of custom TOTP encoders, each with potential for custom code alphabet, length, step interval and code direction (i.e. reversed) * Select custom encoder via overload of the digits field of a loaded entry * Allow selection of custom encoders via the "TOTP Settings" field's size, as currently done by KeeTrayTOTP for Steam. Use "S" for the short name of the Steam custom encoder * Allow selection of custom encoders via the "otp" field by appending a "&encoder=<name>" field to the URL query. For example, "&encoder=steam" * Update TOTP set-up dialog to permit selection between (default, steam, custom) settings.

joelsmith · 2017-11-21T00:09:36Z

rebased/squashed

adolfogc · 2017-11-21T00:09:41Z

@phoerious @joelsmith I agree with you, using the "Q prefix" can be misleading.

weslly · 2017-11-21T00:24:48Z

I agree with the name change. I'll submit a PR.

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

gpchelkin · 2018-03-01T10:55:55Z

May we expect some docs (or maybe a link) on how to set this up, please? Couldn't find anything about how to get the Key. Thanks for the work!

weslly · 2018-03-01T13:59:06Z

@gpchelkin https://github.com/victor-rds/KeeTrayTOTP/blob/master/README.md

joelsmith force-pushed the feature/steamtotp branch 2 times, most recently from 3f78d25 to 34bc71b Compare November 20, 2017 19:00

weslly self-requested a review November 20, 2017 19:06

joelsmith force-pushed the feature/steamtotp branch from 34bc71b to e53421d Compare November 20, 2017 20:05

weslly approved these changes Nov 20, 2017

View reviewed changes

weslly requested a review from a team November 20, 2017 20:51

phoerious requested changes Nov 20, 2017

View reviewed changes

weslly added this to the v2.3.0 milestone Nov 20, 2017

weslly added new feature feature: TOTP labels Nov 20, 2017

phoerious requested changes Nov 20, 2017

View reviewed changes

phoerious approved these changes Nov 21, 2017

View reviewed changes

joelsmith force-pushed the feature/steamtotp branch from df3b6b5 to 8ca52ba Compare November 21, 2017 00:08

phoerious merged commit 0eb7936 into keepassxreboot:develop Nov 21, 2017

weslly mentioned this pull request Nov 21, 2017

Rename QTotp class name to Totp #1207

Merged

joelsmith deleted the feature/steamtotp branch November 21, 2017 17:58

skolarianer mentioned this pull request Dec 11, 2017

Can not work with Mono KeeTrayTOTP/KeeTrayTOTP#5

Closed

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 initial Steam TOTP support #1206

Add initial Steam TOTP support #1206

joelsmith commented Nov 20, 2017 •

edited

Loading

weslly commented Nov 20, 2017 •

edited

Loading

joelsmith commented Nov 20, 2017 •

edited

Loading

weslly commented Nov 20, 2017

phoerious Nov 20, 2017

phoerious Nov 20, 2017 •

edited

Loading

phoerious Nov 20, 2017

phoerious Nov 20, 2017

phoerious Nov 20, 2017

phoerious Nov 20, 2017

phoerious Nov 20, 2017

joelsmith Nov 20, 2017

phoerious Nov 20, 2017

phoerious Nov 20, 2017

phoerious Nov 20, 2017

joelsmith Nov 20, 2017

phoerious Nov 20, 2017

joelsmith commented Nov 20, 2017

phoerious left a comment

phoerious Nov 20, 2017

phoerious Nov 20, 2017

adolfogc commented Nov 20, 2017

joelsmith commented Nov 21, 2017

joelsmith commented Nov 21, 2017

phoerious commented Nov 21, 2017 •

edited

Loading

joelsmith commented Nov 21, 2017

adolfogc commented Nov 21, 2017

weslly commented Nov 21, 2017

gpchelkin commented Mar 1, 2018

weslly commented Mar 1, 2018

Add initial Steam TOTP support #1206

Add initial Steam TOTP support #1206

Conversation

joelsmith commented Nov 20, 2017 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

weslly commented Nov 20, 2017 • edited Loading

joelsmith commented Nov 20, 2017 • edited Loading

weslly commented Nov 20, 2017

Choose a reason for hiding this comment

phoerious Nov 20, 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

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

joelsmith commented Nov 20, 2017

phoerious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adolfogc commented Nov 20, 2017

joelsmith commented Nov 21, 2017

joelsmith commented Nov 21, 2017

phoerious commented Nov 21, 2017 • edited Loading

joelsmith commented Nov 21, 2017

adolfogc commented Nov 21, 2017

weslly commented Nov 21, 2017

gpchelkin commented Mar 1, 2018

weslly commented Mar 1, 2018

joelsmith commented Nov 20, 2017 •

edited

Loading

weslly commented Nov 20, 2017 •

edited

Loading

joelsmith commented Nov 20, 2017 •

edited

Loading

phoerious Nov 20, 2017 •

edited

Loading

phoerious commented Nov 21, 2017 •

edited

Loading