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

[WIP, RFC] Add support for displaying TOTP keys as QR codes #964

Closed
wants to merge 6 commits into from
Closed

[WIP, RFC] Add support for displaying TOTP keys as QR codes #964

wants to merge 6 commits into from

Conversation

adolfogc
Copy link
Contributor

@adolfogc adolfogc commented Sep 19, 2017

Description

This PR is for adding the feature requested in #764. It is currently a work-in-progress.

What's pending (please provide feedback)

  • Decide how to include the qtqrencode dependency. I was thinking on using DownloadProject for having CMake include it as an external dependency. This approach proves beneficial in that the external dependency doesn't need to be included in the project source tree and yet it can be compiled using the same compiler flags and settings that are used for the project (as if it were a CMake subdirectory).

  • Decide how to add support for a base32 encoder that complies with RFC 4648. We need this to generate QR codes in the "Key Uri Format" that is expected by authenticator apps such as Google Authenticator (read more on the Google Authenticator wiki). I was thinking on using the cppcodec library, but we need to figure out how to integrate it with Keepassxc, as their code uses C++ exceptions but Keepassxc is compiled using the -fno-exceptions flag.

  • Instead of using qtqrencode, a new class, QRCode is placed in src/core for enabling support for QR code generation using libqrencode. Besides not having another dependency, we have the benefit of not linking against Qt::QtSvg --which qtqrencode requires.

  • QR code size is selected automatically by libqrencode based on input data.

  • QR code error correction level is set to its highest.

  • Our local base32 implementation is compliant with RFC 4648. Three new methods were added for compatibility with Google Authenticator's "Key Uri Format" (which requires no padding characters):

    • Base32::addPadding/1 This function adds padding to valid encoded data missing those characters. Otherwise, it returns the same input value.
    • Base32::removePadding/1 This function removes padding from valid encoded data. Otherwise, it returns the same input value.
    • Base32::sanitizeInput/1 This function sanitizes the encoded data so that it can then be decoded correctly. It also corrects "typos" (1 -> L, 8 -> B, 0 -> O).

Motivation and context

This is best explained by #764.

How has this been tested?

Currently, it is build for testing using Fedora 26 and the following nix environment:

with import <nixpkgs> {};
stdenv.mkDerivation rec {
    name="keepassxc-env";
    env = buildEnv { name = name; paths = buildInputs; };
    buildInputs = [
        cmake
        libgcrypt
        zlib
        qt5.qtbase
        qt5.qtsvg
        qt5.qttools
        libgpgerror
        glibcLocales
        libmicrohttpd
        xorg.libXtst
        libyubikey
        libqrencode
        clang-tools
    ];
}

You need to enable the option WITH_CX_TOTPSEED_QRCODE:

You need to enable the option WITH_CX_TOTPDISPLAYKEY:

mkdir build
cd build
cmake -DWITH_ASAN=ON -DWITH_XC_TOTPDISPLAYKEY=ON -DCMAKE_BUILD_TYPE=Debug ..

Test barcodes

  • Generate test data using: https://freeotp.github.io/qrcode.html and load it into a test DB.
  • Test:
    • Barcode reads correctly from authenticator app
    • Authenticator app generates same OTPs as KeePassXC
  • Tested using:
    • Google Authenticator (iOS) ✅
    • FreeOTP (iOS) ✅
    • Microsoft Authenticator (iOS) ✅
    • Yandex Authenticator AKA Ya.Key (iOS) ✅
    • LastPass Authenticator (iOS) ✅

Screenshots (if appropriate):

Below are two images that illustrate the proposed interface for this feature.

Screenshot 1
Screenshot 2
Note: you can actually read the barcode above! (Tested with the Ya.Key iOS app).

Types of changes

  • ✅ New feature (non-breaking change which adds functionality)
  • ✅ Breaking change (fix or feature that would cause existing functionality to change)

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]
  • ✅ My change requires a change to the documentation and I have updated it accordingly.
  • ✅ I have added tests to cover my changes. [[ 📝 Except for the UI]].

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Sep 19, 2017

If we can find a proper base32 encoder/decoder I would be very happy to remove the current decoder

Alternatively we can write our own.

Anyway every dependency must be Debian-friendly (no embedded lib) like #701

@weslly weslly self-requested a review September 19, 2017 15:36
@weslly
Copy link
Contributor

weslly commented Sep 19, 2017

@TheZ3ro I've been looking for a better base32 lib but most are either too complex and bloated or just a remix of Google's library with small tweaks (like the current we have). We'll probably have to write our own.

@adolfogc
Copy link
Contributor Author

@TheZ3ro @weslly Please review the new implementation provided in #984.

@adolfogc
Copy link
Contributor Author

I replaced this PR with #1001, as something strange happened with the tree of this branch --I think I didn't rebase correctly. I'm closing this PR.

@adolfogc adolfogc closed this Sep 27, 2017
@adolfogc adolfogc deleted the feature/764-totpseed-qrcode branch November 6, 2017 05:12
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.

4 participants