1Password OpVault format support

[mdaniel]

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]

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

[mdaniel]

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]

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/

[droidmonkey]

Needs work to bring into alignment with coding standards.

[phoerious]

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.

[mdaniel]

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]

@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]

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

[droidmonkey]

@mdaniel still working on this?

[mdaniel]

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]

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]

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]

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]

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!

[user13542]

@mdaniel any news?

[jorijn]

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

[mdaniel]

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]

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]

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]

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

old

[droidmonkey]

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.

[mdaniel]

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]

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.

stale

[droidmonkey]

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.

[mdaniel]

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]

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

[jorijn]

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]

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

[Zettt]

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]

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

[Zettt]

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]

Let's continue this discussion over in #3239