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

1Password OpVault format support #2292

Merged
merged 3 commits into from May 19, 2019

Conversation

@mdaniel
Copy link

mdaniel commented Sep 13, 2018

Description

The on disk format used by 1Password from AgileBits is mostly JSON, but that JSON contains a boatload of base64 proprietary binary blobs that are encoded and encrypted in a non-obvious way. Thankfully, AgileBits has published the specification for the format, which they call opvault, and thus it is reasonable to teach KeePassXC to ingest that disk format.

For the time being, this code only imports the disk format, since writing out any changes would be a monster amount of effort and -- certainly for my purposes -- wouldn't be an obvious win.

Motivation and context

There is no support for reading the 1Password .opvault format on Linux unless one is subscribed to the 1password.com product, which for sure isn't everyone who uses 1Password. There was only recently a Windows client added for it, and I can't speak to how well it works in order to know if this PR would benefit Windows users [beyond saving them the money for a second 1Password license, of course].

Fixes #1462

How has this been tested?

They publish a sample file with a known password presumably expressly for testing. I have a happy-path test that gets in the ballpark of 75% code coverage, with the remaining lines concerning error conditions.

There will need to be some discussion around that ☝️ since it wasn't super clear if I should just unpack that tar into tests/data, or have the test try and download it, or what. So this very second, the testopvaultformat will fail if you clone this branch and run the tests.

And, as one might suspect, I've been using this to read both my home 1Password opvault and a separate one from work, so it is good enough for a daily driver.

There is currently some kind of misunderstanding about the relationship between Group and Entry, such that searching for an Entry finds it, and reports it in the correct Group, but clicking on the Group in the sidebar lists no Entry lines. Again, the search behavior is good enough for what I wanted it to do, but I can try to fix that if it's important

Screenshots (if appropriate):

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Add a new import button on the welcome screen
  • Add a new import menu item under Database > Import
  • Add the supporting classes for reading the opvault format
  • And the aforementioned test case

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project. (FYI, make format dirties 103 files that I didn't touch)
  • 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. <-- this one is open for discussion; I didn't look into any documentation, but I'll be glad to do so if you feel it would be helpful
  • I have added tests to cover my changes.
@TheZ3ro

This comment has been minimized.

Copy link
Contributor

TheZ3ro commented Sep 14, 2018

Very nice, I really like this but it will take a while to review 😄

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Sep 14, 2018

Let me know if there is any way I can help that process, including if you want me to take a swing at those code complexity issues reported by CodeFactor. CLion whined about the complexity, too, but I wasn't sure if it was being overly pedantic.

Also, as mentioned, I welcome input on dealing with the sample data for the test. The unpacked .opvault is 804K, and of that size, the majority is attachment payloads. Downloading and unpacking it also seems reasonable, I just don't know what the opinion is on extra-repo dependencies like that

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 14, 2018

The sample vault for unit testing should be stored in the test data folder with all the other sample databases. Specifically, store it in tests/data/opvault/

src/format/OpData01.cpp Outdated Show resolved Hide resolved
src/format/OpVaultReaderBandEntry.cpp Outdated Show resolved Hide resolved
Copy link
Member

droidmonkey left a comment

Needs work to bring into alignment with coding standards.

src/format/OpData01.cpp Outdated Show resolved Hide resolved
Copy link
Member

phoerious left a comment

I've read through a portion of the code so far. So first, thank you for your contribution. However, I have marked quite a few changes up to the point where I stopped my first read-through. Please adjust your code accordingly and fix similar issues in the remaining code before further review.

In addition to these comments, I have some general remarks I'd like you to think about:
First, this is a lot of code only for reading a password file. Our KDBX code is a fraction of that. Is it really necessary to merge so much code only for adding another file format?

Secondly, please don't use so many sub scopes. Better think about the variables you need and use rvalues where applicable. Too many indentations make the code harder to grasp and lead to sloppy variable use.

And lastly, please use smart pointers or QObject parenting. Don't create freestore instances without immediately taking ownership. Returning pointers to new-allocated objects from functions is a very good recipe for memory leaks and dangling pointers.

share/translations/keepassx_en_US.ts Outdated Show resolved Hide resolved
src/format/OpData01.cpp Outdated Show resolved Hide resolved
src/format/OpData01.cpp Outdated Show resolved Hide resolved
src/format/OpData01.cpp Outdated Show resolved Hide resolved
src/format/OpData01.cpp Outdated Show resolved Hide resolved
src/format/OpVaultReader.cpp Outdated Show resolved Hide resolved
src/format/OpVaultReader.cpp Outdated Show resolved Hide resolved
src/format/OpVaultReader.cpp Outdated Show resolved Hide resolved
src/format/OpVaultReader.cpp Outdated Show resolved Hide resolved
src/format/OpVaultReader.cpp Outdated Show resolved Hide resolved
@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Sep 28, 2018

I am working on incorporating these changes, and I am grateful for all the feedback. I just didn't want you to think the feedback went to /dev/null. I'll of course fix those conflicts, too.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 30, 2018

@mdaniel any update for this? Would love to see this in 2.4.0, but needs to be ready within the next couple weeks.

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Oct 31, 2018

My apologies, I let this slip off my radar; I'll try to get those concerns cleaned up, and fix the merge conflicts.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 17, 2018

@mdaniel still working on this?

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Nov 19, 2018

Wow, I had no idea it had been 20 days since that comment :-( But I am hoping Friday will be a solid block of time where I can focus, as that's been the obstacle

@mdaniel mdaniel force-pushed the mdaniel:opvault-format branch from 0ddde45 to 63fd37f Nov 26, 2018
@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Nov 26, 2018

I took a light swing at replacing Foo* with QSharedPointer<Foo> but there is no way I'll get that to even run tonight. So, I guess we can put this back in the icebox until I have the mental bandwidth to learn QSharedPointer or have that as a stretch goal

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Nov 26, 2018

In addition to these comments, I have some general remarks I'd like you to think about:
First, this is a lot of code only for reading a password file. Our KDBX code is a fraction of that. Is it really necessary to merge so much code only for adding another file format?

Up to you (collectively). I only put this in flight because I thought others would benefit from someone taking the time to actually implement the OpVault reader. I actually did mention that this kind of thing is perfect for a plug-in, but it was my understanding that KeePassXC has no such concept, so that's how I ended up wiring it into this repo.

Secondly, please don't use so many sub scopes. Better think about the variables you need and use rvalues where applicable. Too many indentations make the code harder to grasp and lead to sloppy variable use.

I don't know what an rvalues is, and I readily and wholeheartedly admit that the structure of the code is a lot more chaotic than I'd like. I can't say offhand how much bandwidth I'll have to straighten that stuff out, although it's a lot more likely in deep December early January.

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Nov 26, 2018

Finally, I wanted to point out that

My code follows the code style of this project. (FYI, make format dirties 103 files that I didn't touch)

is still true and makes keeping my changes in sync with clang-format a monster PITA since the workflow is more or less make format && git checkout $(all files that are not mine)

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 29, 2018

I was wondering why you were having so many format issues and I was reminded that we didn't merge the code cleanup part 2 PR. That is now merged!

@droidmonkey droidmonkey added this to the v2.5.0 milestone Jan 21, 2019
@user13542

This comment has been minimized.

Copy link

user13542 commented Feb 20, 2019

@mdaniel any news?

@Jorijn

This comment has been minimized.

Copy link

Jorijn commented Mar 13, 2019

I'm also eagerly waiting for this feature to be completed.

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Mar 13, 2019

I finally realized why this PR currently feels overwhelming: I don't have a list of tactical changes that I would make to it. I am not a C++ developer, so I don't have a deep understanding of QSharedPointer<> or its hazards, and as I mentioned, I don't know what an rvalue is.

If phoerious (et al) could lay out the criteria for accepting this, then it might make it a little more approachable, since -- aside from the sub-scopes and generally monolithic functions -- I don't know what I would open my editor and do in order to clear the merge blocker

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 13, 2019

Frankly this needs to be built from the "ground up" again. With the database refactor and methods you used it might not be easily salvageable in this state.

@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented Mar 13, 2019

FWIW, I just merged in develop (0f1be60) into my branch and it built cleanly and behaved as I expected (it even looks nicer; maybe the icons got a refresh?), so I don't know if the database refactor you mention is still pending, or is on develop already

I guess once I learn when the refactor is coming, or more about that, then I can evaluate how to proceed. But if you're unhappy with this branch, then I guess we'll just close this so its state will be more obvious to those waiting for it to land.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 13, 2019

Ah good to know, I guess you started this after the refactor was merged. I take back my statement.

@droidmonkey droidmonkey force-pushed the mdaniel:opvault-format branch from 2b07d43 to ca66e1a May 13, 2019
@droidmonkey droidmonkey requested review from droidmonkey and removed request for TheZ3ro May 13, 2019
@droidmonkey droidmonkey dismissed their stale review May 13, 2019

old

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented May 13, 2019

I am taking this under my wing. I have squashed all the extraneous commits and split the non-opvault code changes from opvault. I took a first round at getting rid of all the raw pointers and primitive objects.

This still needs A TON of work, but it does open an OPVault at this time. It does not save the resulting database and there are crashes.

@droidmonkey droidmonkey self-assigned this May 13, 2019
@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented May 13, 2019

It does not save the resulting database

That wasn't ever one of my objectives, as I wasn't trying to import the opvault, just open it for use

I let this linger because I wasn't sure what action to take on it to make it move forward, so I'm glad you have something specific in mind. Let me know how I can help, even if it's just sending you good vibes :-)

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented May 14, 2019

OK the code is pretty much where I want it to be right now. I also fixed saving the database.

The only other thing I want to do is actually move some of the attributes into username/password/notes instead of cramming everything into incredibly unhelpful attributes.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented May 19, 2019

OK I believe this to be ready for prime time. Going to let CI work its magic then merge when its over. Anyone with 1Password, I would really appreciate you testing the import feature once the snapshot is available.

droidmonkey added 3 commits May 19, 2019
* Support importing 1Password vaults (.opvault folders) into KDBX database
* Entry attributes are filled based on section and field name
* Expiration dates are set for entries
* Entry URL's are set from a wider array of fields
@droidmonkey droidmonkey force-pushed the mdaniel:opvault-format branch from 100bfc2 to d0807e3 May 19, 2019
@droidmonkey droidmonkey merged commit 2ee97ed into keepassxreboot:develop May 19, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@mdaniel mdaniel deleted the mdaniel:opvault-format branch May 20, 2019
@mdaniel

This comment has been minimized.

Copy link
Author

mdaniel commented May 20, 2019

I sincerely appreciate your help in shepherding this change in, and I'm glad it didn't take a monster amount of rewrite to bring it up to your satisfaction

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented May 20, 2019

You made a very good core! Thank you for bringing this in, a lot of people will benefit from this feature.

@Jorijn

This comment has been minimized.

Copy link

Jorijn commented May 20, 2019

I just tested this feature on a mac. Seems to work as intended. Kudos for even including the MFA tokens.

Just a small remark: On Macs, .opvault directories are registered as files. I was unable to select it until I removed the ".opvault" part to turn it into a directory again.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented May 20, 2019

That's interesting, we could display a file selection dialog on macOS instead of a directory selection.

@Zettt

This comment has been minimized.

Copy link

Zettt commented Jun 7, 2019

Guys, great work first of all, but I'm wondering why you're messing with the opvault, which normal users cannot easily access? Why is it not better to simply support the normal (1Password preferred) way of exchanging this information through 1pif files?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 7, 2019

Great "now you tell us" lol. Is 1pif wven documented?

@Zettt

This comment has been minimized.

Copy link

Zettt commented Jun 7, 2019

You guys didn't know this? Oh shishkebab. To be honest I have no idea how it is documented at all. All I can say is that from what I've read here I wouldn't even be able to test the feature. I'm on their subscription plan. As far as I'm aware, there's no real way to access the vault at all. But, and that's the reason I've commented, there's an entry in the file menu which says Export → All Items. The 1pif format is in JSON-ish format. You guys might have luck to check out the Bitwarden repositories, they also have an importer for the format. I'm not a programmer by day, but I work as manager for teams. My gut feeling tells that what the 1pif has in it, is probably just the already-encrypted content of the opvault. Perhaps. Or so.

Here's an example:

{"uuid":"vthnyeitfvb6pc6iyc4a5i7rrm","updatedAt":1559854808,"locationKey":"riot.app","securityLevel":"SL5","contentsHash":"92c59ef6","title":"Riot","location":"app:\/\/im.riot.app","secureContents":{"URLs":[{"url":"app:\/\/im.riot.app"},{"label":"","url":"riot.im"}],"fields":[{"value":"obviously this is where the password goes","name":"password","type":"P","designation":"password"},{"value":"azeitler","name":"username","type":"T","designation":"username"}],"sections":[{"title":"Related Items","name":"linked items"}]},"createdAt":1559853537,"typeName":"webforms.WebForm"}
***5642bee8-a5ff-11dc-8314-0800200c9a66***
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 7, 2019

Let's continue this discussion over in #3239

phoerious added a commit that referenced this pull request Oct 26, 2019
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
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.