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

Sharing groups #2109

Merged

Conversation

Projects
None yet
9 participants
@ckieschnick
Copy link
Contributor

ckieschnick commented Jul 12, 2018

In many cases passwords should be kept secret and should not be shared with anyone, sometimes it is tiresome to give everybody access to the specific passwords without publishing the rest. Since keeping multiple databases does not do any good, especially when many people may need to access and update them, we created an extension which allows to share groups of entries with multiple peers while keeping others private without the need of multiple databases and regardless of the file location.

Description

The extension allows to export/import or synchronize(=import+export) groups with their contained entries to other parties. The shared data is placed in a share container which may be published by any means (i.e. cloud, network share, ...). Any client can import this container as long he has the password.
The sharing container is protected using a certificate which at least ensures that the source is the expected one, so the client may decline to import data from an untrusted source.
The features does make use of the synchronization (#1992) and merges data in time order (newest data will be on top, older data will be moved to the history).
All data required to work for KeeShare is stored either in KeePassXC-Settings or CustomData, so the extension is fully compatible with existing databases.

Motivation and context

It is often necessary to publish certain secrets to a selected group of people but protecting those secret none the less. Just imagine a company which wants to distribute passwords to some employee groups (i.e. banking data just for the managers, root passwords for admins). Those passwords can be published on internal sites, but keeping them in a password manager makes it easy to protect them. If the password manager takes care, that the passwords are up to date, even when it is not centralized, it would be even better. That's why we introduced the group sharing feature (KeeShare) to KeePassXC. There is no need to keep multiple databases around which may have problems when shared using some technologies.

Furthermore, the extension addresses some more wishes and improvements from the issue list:

  • import features resolves #90 Option to Sync (on changes) from another external file (just make the root group an import group)
  • export feature resolves #2062 Allow user to define automatic DB backup location and name (just make the root group an export group and specify the location - to open the backup, one has to import it using the password)
  • the merge may be used feature to resolve #637 (Bidirectional Merge/Synchronization)

Problems and Tickets which may gain in importance:

  • sharing without history support may lead to data loss
  • sharing with autosave on change prevents the user from reviewing and discarding the changes of an import and may lead to unnecessary exports
  • #1162 (Provide a visual indication for entries that contain unsaved changes) may make the change tracking of imports easier (depends on the visualization of changes during the import)
  • #852 (User should be warned before deleting entry with a UID that is being REF'd by another object)/#1744 (Warn user if deleting entries that are referenced.) may improve sharing since references would be used to export an entry to multiple shares

Possible use cases are:

  • bidirectional sharing between different individuals or groups without the need of maintaining several databases at the same time (i.e. using a network share or dropbox)
  • simple backup of parts of a database or the whole database to a specific location (using only the export feature)
  • unidirectional sharing of specific entries (using export on the master and import on the slave client) There may be more use cases for the sync feature which may help with certain restrictions/adaptions (i.e. auto import of a CSV file into a group - may need an adaption of the merge logic to reduce duplicates)

How has this been tested?

There are extensions to the test suite, but a lot of functionality is tested manually. I'm note sure if a UI-test would be justified.

Screenshots (if appropriate):

Please look in the updated QUICKSTART

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • All new and existing tests passed.
  • I have compiled and verified my code with -DWITH_ASAN=ON.
  • My change requires a change to the documentation and I have updated it accordingly.
  • I have added tests to cover my changes. - As far as the current abstraction allowed.
@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jul 24, 2018

Hey, thanks for your follow-up PR. Could you please check why the tests are failing and resolve the merge conflicts? Then we can start reviewing your changes.

@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

ckieschnick commented Jul 25, 2018

CodeFactor fails currently due to an intended duplication in FileDialog. I'm not sure how it was intended to use the methods, therefore I didn't try to put my changes into an existing method to prevent unintended changes at other places.
TeamCity has just one issue (for now) - Release [GCC, AppImage] couldn't fetch packages from a specific ubuntu repository - seems to be a transient failure to me. The other images seem to work now.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jul 26, 2018

That means we need to increment the version number in the docker script so it rebuilds the image. You can ignore the code factor finding as long as what you wrote here is documented in comments.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 26, 2018

@ckieschnick sorry for the incredible delay on this PR. We had to clean house a little first. I will rebase this after we merge your first PR. Any additional comments?

@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

ckieschnick commented Sep 26, 2018

I'm currently quite busy, so I couldn't track the progress - therefore I have nothing to comment. In case problems arise, I'll try to support you.

@ratcashdev

This comment has been minimized.

Copy link

ratcashdev commented Sep 26, 2018

Please consider adding the option of keeping a configurable number of backups (e.g. 3 older backups), not just one.

@droidmonkey droidmonkey requested review from phoerious and droidmonkey and removed request for phoerious Sep 30, 2018

@droidmonkey droidmonkey added this to the v2.4.0 milestone Sep 30, 2018

@droidmonkey droidmonkey force-pushed the hicknhack-software:feature/sharing_groups branch from 92f18ff to 49164b8 Oct 1, 2018

ckieschnick and others added some commits Oct 1, 2018

Add sharing of groups between databases
* Add source folder keeshare for sharing with corresponding define WITH_XC_KEESHARE
* Move common crypto parts to src/crypto/ssh
* Extended OpenSSHKey
* Move filewatching to own file (currently in two related classes DelayedFileWatcher and BulkFileWatcher)
* Small improvements for style and code in several classes
* Sharing is secured using RSA-Keys which are generated on demand
* Publisher signs the container using their private key
* Client can verify the signed container and choose to decline an import,
import only once or trust the publisher and automatically import all
data of this source henceforth
* Integration of settings into Group-Settings, Database-Settings and Application-Settings
* Introduced dependency QuaZip as dependency to allow combined export of
key container and the (custom format) certificate
Add documentation for sharing groups
* Add Sharing section to Quickstart Guide
* Add pictures and text in quickstart to match extended functionality

@droidmonkey droidmonkey force-pushed the hicknhack-software:feature/sharing_groups branch from 49164b8 to b9d6cb5 Oct 1, 2018

Update ci files
* Removed libquazip-headers from trusty ci images
Trusty does not provide libquazip-headers (they are part of
libquazip-dev most likely)
* Bump rebuild counter

@droidmonkey droidmonkey force-pushed the hicknhack-software:feature/sharing_groups branch from b9d6cb5 to 28e7540 Oct 1, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 2, 2018

Some issues I found right away:

  • When adding sync to a group, all the other groups were instantly deleted
  • It is not possible for two folders in the same database to import from one that exports.
  • Inconsistent syncing occurs when two folders in the same database point to the same kdbx.share, they should be independent. One entry went to one folder, the others went to another folder.
  • quazip does not have a Qt5 build on Trusty. Investigate storing the signing certificate within the kdbx metadata itself.
@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

ckieschnick commented Oct 2, 2018

For clarification:

  • The feature is not intended to sync groups within a database but to sync groups between different databases. It may be possible to have a master slave setup with one group exporting while the slave do only import, but this is more a side effect than an intended feature.
  • "Merging" multiple groups into a single kdbx.share (using sync or export) is no use case - each group overwrites the share therefore exporting multiple times from within one database into the same share would lead to lost updates.
  • We used QuaZip for simplicity, since the container format is more easy to read and validate (no need to read anything of the container). Another advantage is, that it is easy to use the unpacked container as backup and even checking the signature with third party tools would be easier when the signature is not stored within the container.

Regarding your first point - I didn't have time to check the implementation against the current develop in the last month, but this problem seems weird. I'm currently quite busy, but I may be able to help, if you can provide some more information on this issue.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 2, 2018

@ckieschnick no problems, these were more notes for me to investigate further and provide fixes. It is very very likely we are going to drop the QuaZip usage, it currently does not have a port for Qt5 on Trusty and possibly not on macOS either. We could integrate it directly, but I'm sure the Debian folks will have a cow about that.

I am mainly trying to find where the plugin breaks down so we can either add guards to warn or prevent the user from making a mistake.

Show resolved Hide resolved INSTALL.md Outdated
@dsonck92

This comment has been minimized.

Copy link

dsonck92 commented Oct 5, 2018

I've been testing this feature today. Very good functionality. However, the first thing which struck me as odd (even though it's mentioned in the docs) is the flat initial syncing.

  • While I like the idea of being able to move the items outside of the sync group while maintaining the sync, this should be visually represented in a way. I currently have no "dumb trust" it will remain connected with the sync container.
  • Another thing. When I first used it, and it synced everything without hierarchy, I first thought it didn't sync the subitems. I later realized it did in fact sync over, but as I have a huge but organized collection of passwords, it basically makes the first sync very messy. All items are merged into the top group. That means I need to recreate the full structure on the other sync. It might be useful to include a checkmark "Sync subgroup structure". To allow both syncing with and without preserving the paths of the keys. For those that do want to keep the hierarchy synced as well.
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 5, 2018

@dsonck92 great points, this does require a little bit of work. I think the default behavior should be to maintain structure, it does not make any sense to eliminate structure that is created.

ckieschnick added some commits Jan 4, 2019

Fixed issues detected by test suite and ci
Fixed serialization for KeeShareSettings::ScopedCertificate
Fixed tests for KeeShareSettings serialization
Fixed tests Cli features - tests translation for recycle bin since the
tests are executed with the system locale
Fixed initialization issue in ShareObserver
More ci issues (missing initialize for const)
Fixed issues detected in TestSharing
Fixed GCC issues for const initialization
Fixed issues reported by GCC for initialization of const variables
@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

ckieschnick commented Jan 6, 2019

Quazip is now an optional dependency. KeeShare is activated enabling the KeeShare-feature. In case no Quazip is found, KeeShare exports simple databases, If Quazip was found, KeeShare allows and defaults to secured exports.

For insecure KeeShare, it is not possible to sync with secured containers but for secure KeeShare it is possible to sync with insecure containers. The default for secure builds is to use secured containers.

To reduce the risk of unwanted imports, we added a scope to the certificates which is bound to the share container path - this way only accepted containers are automatically synced.

Concerning the complexity fails of CodeFactor, you may have a look, but I think these cases are tolerable since more separation does not ease reading or understanding of the code.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 6, 2019

I made a couple minor edits to fix a couple bugs. Here are my observations:

  • There is no way to build insecure-only when QuaZip is installed. My opinion is that building KeeShare secure should be opt-in, not based on the presence of libraries. Propose eliminating WITH_XC_KEESHARE_INSECURE variable since WITH_XC_KEESHARE implies this state. Make WITH_XC_KEESHARE_SECURE an option variable that is NOT enabled when WITH_XC_ALL=ON. When building you have to explicitly define WITH_XC_KEESHARE_SECURE=ON which then enables the QuaZip dependency.
  • There is no way to test that the sync is working until you press OK to accept the group settings. Even then, when no settings are changed it does not trigger a re-sync which makes it difficult to determine if there is an issue. Suggest a persistent banner that shows when viewing a group that is synced which identifies the group as synced and its current status.
  • There is no way to import a certificate to trust. I think a button was accidentally left off the refactor.
  • Notices in the group edit should persist and not include a close button.
  • Received multiple popup warnings about insecure containers. Even one warning seems excessive considering I know its "insecure". keeshare_multiwarn
  • I do not agree with the terminology of "insecure" vs "secure" since that implied the database file itself is not secure. Its just "unsigned" or "untrusted". Plain kdbx files should not have any qualifiers and signed shares should be called "Signed KDBX Share *.kdbx.share" or something similar.
  • FindQuaZip.cmake makes it impossible to build secure with MSYS2. Quazip5 is not deployed with "debug" versions (ie, with the d suffix).

Please let me know your thoughts on the QuaZip. I can assist with fixing these issues as well.

@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

ckieschnick commented Jan 7, 2019

I'll try to address the points in the same order as given:

  • Your proposal using KEESHARE and KEESHARE_SECURE is okay for me from the build configuration perspective. Within the code, I'll would like to keep the separation of KEESHARE_SECURE / KEESHARE_INSECURE (it is written in a way which would allow to exclude KeeShare completely, only the secure, only the insecure or include both secure and insecure).
  • What do you want to check? For me, the import seems to work as soon as the configuration for the group is accepted. The activated sharing on a group is indicated by the small badge and the "share" tab in the group details.
  • I'm not sure which button do you mean.
  • Persistent warnings - seems better. I'll look into it.
  • Seems like a bug to me, but I'm have no idea how you were able to create multiple warnings for the same Sync-Container since the scope seems to be identical.
  • I'm open for suggestions since I'm not happy with terminology too. Secure means that beside the password the source of the container is trusted. Insecure means, that the password matches, but the source cannot be verified so you have to trust the container blind. As long as only trusted people can access the shared filesystem where container resides, it is not insecure - but in an "open" scenario where anybody may read or write the shared container, one may import data blindly which may lead to undesired results (i.e. a changed entry with enough history to remove any valid data, so the data is lost).
  • I forgot to check the MSYS-build.

ckieschnick added some commits Jan 7, 2019

Implemented feedback regarding build and ui
Changed build options to use only WITH_XC_KEESHARE and
WITH_XC_KEESHARE_SECURE - WITH_XC_KEESHARE_INSECURE remains as internal
variable to highlight differences (may allow to build schemes later)

Message widget in KeeShare settings for groups is not closeable anymore
Fixed QuaZip for windows, renaming
QuaZip should now usable under windows (fixed include in FindQuaZip)

Renamed the representation from secure and unsecure to signed and
unsigned
@ckieschnick

This comment has been minimized.

Copy link
Contributor Author

ckieschnick commented Jan 9, 2019

I could reproduce the problem with the multiple dialogs - it's a problem with the FileWatcher which does signal a file change multiple times, but it is expected to be send exactly once. The quick and dirty way would be to delay the permission for some milliseconds to lunch the dialog after all signals were emitted, but I'll try to locate the root cause for now.

Fixed BulkFileWatcher multi signal problem
BulkFileWatcher emitted multiple file change signals (like
QFileSystemWatcher) for the watched files. Introduced a delay by waiting
until the end of the event loop to aggregate signals emitted by
QFileSystemWatcher before emitting custom signals.
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 9, 2019

Awesome, I tested everything out and it works much better. I think we can do a code review and merge it in afterwards. Any minor ui/ux issues can be cleaned up in a new PR.

Thank you so much for your work on this and the previous PR. Many many people around the world are looking forward to this feature!

droidmonkey added some commits Jan 10, 2019

@droidmonkey
Copy link
Member

droidmonkey left a comment

Reviewed all files and tested functionality, this is ready for merge.

droidmonkey added some commits Jan 19, 2019

@droidmonkey droidmonkey merged commit b59fd6d into keepassxreboot:develop Jan 19, 2019

3 of 4 checks passed

CodeFactor 5 issues fixed. 11 issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 19, 2019

@ckieschnick great work on this, it is now a part of 2.4.0

@droidmonkey droidmonkey referenced this pull request Mar 13, 2019

Open

Add Tag Feature #508

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
@Guite

This comment has been minimized.

Copy link

Guite commented Mar 20, 2019

Can someone please briefly explain the essential difference of this new addition and the AutoOpen functionality? If I understood the quickstart document correctly there is an additional kdbx file here, too.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 20, 2019

AutoOpen keeps the database OUTSIDE of the currently used database. KeeShare brings the database INSIDE to the currently used database. Also the ability to IMPORT, EXPORT, SYNCHRONIZE versus just SYNCHRONIZE with AutoOpen.

@Guite

This comment has been minimized.

Copy link

Guite commented Mar 20, 2019

Thank you 👍

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.