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

Add pass store signing key feature #634

Merged
merged 7 commits into from
Aug 31, 2023

Conversation

timegrid
Copy link
Contributor

@timegrid timegrid commented Jun 16, 2023

Reason

The pass store signing key feature would enhance the security of qtpass/pass when used by teams. Teams need fine-grained control over the .gpg-id user lists of subpaths/stores, but they also have to be able to verify the .gpg-id user lists to prevent accidental/malicious privilege escalation. By setting the PASSWORD_STORE_SIGNING_KEY envvar to a trusted team signing key id, password admins can provide prepared and signed .gpg-id[.sig] files and team members can easily enable the verification by configuring the new signing key id in the corresponding profile. If the verification fails, the error Signature for [...]/.gpg-id is invalid. is visible and further actions are prevented. This PR would fix #624, see further discussion there.

Changes

Adds the pass store signing key feature by

  • changing the profile type from QString to QHash<QString, QString> in order to be able to represent
    more key/value pairs for one profile (profile['path'], profile['signingKey'])
  • changing the profiles type from QHash<QString, QString> to QHash<QString, QHash<QString, QString>>
    in order to store the new data structure
  • changing the profiles settings from plain keys (childKeys) to a group (childGroups) in order to store more
    key/values for one profile (profile/NAME/path, profile/NAME/signingKey)
  • adding a column to the profile ui table
  • adding a new setting for passSigningKey for the current value of the profile
  • updating the environment on profile switch in updateEnv() (adding/updating the envvar
    PASSWORD_STORE_SIGNING_KEY to the value from the passSigningKey setting or removing the envvar,
    if the passSigningKey setting is empty)
  • adding a comment to the README.md

Notes

  • Unfortunately the change of profile(s) data structure results in an empty profile list on upgrade, so this PR might
    be considered a breaking change (consider this, if you want to test this PR with your real qtpass configuration). But to introduce another parallel data structure would have been awkward/messy
    and this way in the future it is easy to add further key/value pairs for profiles as needed.
  • This is pass only. Using git/gpg would be possible, but quite more time-consuming
  • After qmake, all translation file items included additional location filenames for header files ../src/*.h. I saw Translation cleanup #606,
    but I have no idea, why this happened again and I'm unsure how to proceed, as some line number references
    need to be updated
  • All tests pass

@timegrid
Copy link
Contributor Author

I added a simple migration for the old profile settings, so old profiles are not lost anymore.
(Caution: If you want to test this, still consider, that the data structure is changed persistently)

@timegrid
Copy link
Contributor Author

There is still the issue with the translation files. If i add those files, #606 would essentially be reverted. Adding only those hunks with the needed changes for all translation files would be very tedious. Can I prevent, that the header files are added as location filenames? How should I proceed?

@timegrid
Copy link
Contributor Author

I mean those lines, that got added again:

M localization/localization_af_ZA.ts
@@ -5,32 +5,39 @@
     <name>ConfigDialog</name>
     <message>
         <location filename="../src/configdialog.ui" line="20"/>
+        <location filename="../src/ui_configdialog.h" line="860"/>
         <source>Configuration</source>
         <translation>Konfigurasie</translation>
     </message>
     [...]

@timegrid
Copy link
Contributor Author

Ii also added native gpg/git functions to implement both signature verification and the signing of gpgid file.

timegrid and others added 2 commits June 26, 2023 16:10
1. harmoized path separators in Pass::getGpgIdPath() as the pass store path couldn't match with the gpgIdDir because of trailing backslashes so two absolute paths were concatenated, leading to checkmarks not set properly in usersdialog.cpp, for example.
2. added an optional \r in regex of ImitatePass::verifyGpgIdFile() to comply with Windows \r\n linebreaks as QRegularExpression::MultilineOption won't honor \r as part of a line separator.
@thomiel
Copy link
Contributor

thomiel commented Jul 9, 2023

We had some testing on Windows and stumbled upon two issues (the first one was kind of inherited and I'd recommend a more general code review to avoind mixing qt-internal and native path separators). This is what I did:

  1. Harmoized path separators in Pass::getGpgIdPath() as the pass store path couldn't match with the gpgIdDir because of trailing backslashes so two absolute paths were concatenated, leading to checkmarks not set properly in usersdialog.cpp, for example.
  2. Added an optional \r in regex of ImitatePass::verifyGpgIdFile() to comply with Windows \r\n linebreaks as QRegularExpression::MultilineOption won't honor \r as part of a line separator.

timegrid@a45da0a

@annejan annejan merged commit 58f886e into IJHack:main Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support PASSWORD_STORE_SIGNING_KEY with profiles
3 participants