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

Display passwords as QR codes #421

Merged
merged 2 commits into from Apr 17, 2019

Conversation

Projects
None yet
4 participants
@frawi
Copy link

commented Sep 29, 2018

While there are apps to use pass on an Android phone, I don't feel comfortable with that. It requires GPG on my phone, and I want to use only a small subset of the passwords on my phone. This is all possible, but I think I don't need a complex solution. Most passwords will be stored in the corresponding app anyway.

So I was often using qrencode to copy passwords to my phone. This allows me to control which passwords are on my phone very easily, and allows me to use complicated passwords. I mostly using something like the following command to get passwords out of pass to the screen:

pass wifi/home | qrencode -o- | display

Since I mostly use QtPass as GUI, I thought it would be nice to have this feature built in.

This pull request is my implementation of this feature. I am not very familiar with Qt, I tried my best to keep the changes minimal but complete. I made the following modifications:

  • added an icon for the QR code
  • the icon will be displayed next to the "copy to clipboard" icon
  • when the icon is clicked, a new window is opened containing a PNG with the QR code of the text
  • the QR code is generated by calling the external tool qrencode https://github.com/fukuchi/libqrencode/
  • text input and png output are passed through stdin/stdout, so there should be no leaks on the file system
  • the feature is configurable in the config dialog, similar to OTP
  • if qrencode is not installed, the feature will be disabled and a tooltip is displayed in the config

I used it for some time now and thought it works quite well. There are some issues where I am currently not so sure how to solve them:

  • Opening a new window is nice, because it allows resizing, but maybe it is not the best way to present the code. Maybe the window should also close after a certain timeout.
  • The password is passed to the external program qrencode. If this program is malicious, it could sniff the passwords.
  • The QR code could be generated directly in QtPass, but this would probably add more dependencies.

Please give me your feedback. I would be very happy if this is adopted.

Frank Gabriel added some commits Sep 28, 2018

Frank Gabriel
Frank Gabriel
@coveralls

This comment has been minimized.

Copy link

commented Sep 29, 2018

Coverage Status

Coverage decreased (-0.1%) to 7.126% when pulling 57c24f5 on frawi:master into 2ed533a on IJHack:master.

@codecov

This comment has been minimized.

Copy link

commented Sep 29, 2018

Codecov Report

Merging #421 into master will decrease coverage by 0.09%.
The diff coverage is 1.96%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #421     +/-   ##
========================================
- Coverage    7.25%   7.15%   -0.1%     
========================================
  Files          41      43      +2     
  Lines        2662    2712     +50     
========================================
+ Hits          193     194      +1     
- Misses       2469    2518     +49
Impacted Files Coverage Δ
src/qtpass.h 0% <ø> (ø) ⬆️
src/configdialog.h 0% <ø> (ø) ⬆️
src/qtpasssettings.h 28.57% <ø> (ø) ⬆️
src/mainwindow.cpp 0% <0%> (ø) ⬆️
src/qpushbuttonasqrcode.cpp 0% <0%> (ø)
src/qpushbuttonasqrcode.h 0% <0%> (ø)
src/qtpasssettings.cpp 7.48% <0%> (-0.1%) ⬇️
src/qtpass.cpp 0% <0%> (ø) ⬆️
src/configdialog.cpp 0% <0%> (ø) ⬆️
src/settingsconstants.cpp 98.11% <100%> (+0.03%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ed533a...57c24f5. Read the comment docs.

@jounathaen

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

I basically like the idea.
I have not tested it or looked at it in detail, but a few comments from my side:

  • This would make libqrencode a (loose) dependency of QtPass. Are there any other tools for generating qrcodes or is this the standard?
  • How common is libqrencode? Can it be installed with common packet managers?
  • You hardcoded the feature for non-apple/non-windows installations. Why? Can't libqrencode be installed on those Systems or didn't you test it on these?

My comments on your questions:

Opening a new window is nice, because it allows resizing, but maybe it is not the best way to present the code. Maybe the window should also close after a certain timeout.

My main concern would be, that the QR code should only open when a button is pressed. If it opens by default, it would make the password more vulnerable to "over-the-shoulder" viewing cameras. (I guess you already implemented it in this way.)

The password is passed to the external program qrencode. If this program is malicious, it could sniff the passwords.

Yes, but if you have malicious software on your computer, everything is lost anyway. It could act as a keylogger and steal your keychain password, it could make screenshots of QtPass and read the password,...

The QR code could be generated directly in QtPass, but this would probably add more dependencies.

I think so too. QtPass already uses external programs like pass or pwgen. Another optional external dependency does not hurt.
On the other side, there is some effort in faking these external programs for Windows, as they can't be used on that platform...

@annejan

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

You hardcoded the feature for non-apple/non-windows installations. Why? Can't libqrencode be installed on those Systems or didn't you test it on these?

On macOS it's as simple as brew install qrencode
The display command seems to be part of ImageMagick®

@annejan annejan added the feature label Oct 10, 2018

@annejan annejan merged commit 0789259 into IJHack:master Apr 17, 2019

3 of 6 checks passed

CodeFactor 1 issue found.
Details
codecov/patch 1.96% of diff hit (target 7.25%)
Details
codecov/project 7.15% (-0.1%) compared to 2ed533a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linthub Linthub has cleared the pull request, no code style suggestions found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.