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 support for 1Password .opvault format #1462

Closed
mdaniel opened this issue Feb 6, 2018 · 11 comments · Fixed by #2292
Closed

Add support for 1Password .opvault format #1462

mdaniel opened this issue Feb 6, 2018 · 11 comments · Fixed by #2292

Comments

@mdaniel
Copy link

mdaniel commented Feb 6, 2018

Expected Behavior

KeePassXC should either open, or at least import, the 1Password OpVault database format.

While merely importing the database would help in cross-platform situations, currently the KeePassXC Entry and EntryAttachments models would need to be extended quite a bit to capture the "entry" and attachment models used by OpVault. Thus, opening OpVault for use is likely a stretch goal.

Current Behavior

There does not appear to be a way to open the 1Password.opvault directory(database), neither as a source for importing nor as a database.

Possible Solution

I have a working branch that does the importing part, but it currently goes no further because that was the functionality I required.

Context

I have 1Password on my Mac and on my Android phone (they have an iOS app too, but I am not affected by that). However, it is a major pain to access that data while on Windows or on my Linux laptop. I have been following KeePassXC for quite a while, and thus it seemed like the perfect(?) platform to teach to read OpVault, partially to improve the security community and wholly to "scratch my itch."

Since this issue/feature is unsolicited, I am also prepared for a "closed-wontfix" outcome. I didn't want to open an issue until I knew the format could, in fact, be implemented just from their specification. But now that I have an implementation that at least appears to function, I thought I'd test for interest.

Discussion

Here is where my caveats go:

  • I am not a C++ developer, so I am quite certain I have not written idiomatic C++
  • The code on that branch is without question not what I would submit for inclusion. It is currently in a "one file to rule them all" state since it is basically a PoC that got out of hand :-) I also haven't fired up clang-format, in part because that file needs to be split into components before formatting becomes a worry
  • The test coverage is lousy, where even the one test I did initially write is broken because I made the code work correctly

But if that feature is something the KeePassXC community would value, then I will do all in my power to bring the code into alignment so it can land

@phoerious
Copy link
Member

Does 1Password not have a CSV export feature?
Supporting 1Password's format is an interesting idea, but our data model is quite tightly bound to the KDBX format. Therefore at least some features may not be easy to implement if they differ too much from what KeePass does.

@mdaniel
Copy link
Author

mdaniel commented Feb 7, 2018

Does 1Password not have a CSV export feature?

It definitely does, but that isn't practical in my case for two reasons, in this order:

  • many of my Entrys in 1P have attachments, which includes x509, ssh keys, p12 keystores, and other sensitive file-ish things that I would much prefer were colocated with the passwords and metadata for them
  • even if they exported perfectly, I would need to constantly be recreating the csv, then presumably recreating the KeePassXC database from the export. If I forgot to do so one time, I would only realize that mistake when I went to one of my other computers and found the password Entry missing, requiring a revisit to my Mac

Therefore at least some features may not be easy to implement if they differ too much from what KeePass does

Just in case it was lost in the sea of text from the issue, that mapping is already functioning, even if admittedly not elegantly: https://github.com/mdaniel/keepassxc/tree/opvault-format

I had to discard some concepts going from 1P into KPX, but I got the ones that are gravely important to me: passwords, TOTP code support, notes, metadata, and attachments. The 1P categories are also captured in KPX's Groups, although I haven't yet taken a swing at implementing 1P's concept of favorite Entrys

our data model is quite tightly bound to the KDBX format.

Thankfully, the KDBX appears to be using a subset of the functionality expressed by 1P, so those who are happy with their existing Entry structures can keep them. Supporting the "section" concept from 1P could be done with multiple EntryAttributes (say, for example, a bank Entry containing one EntryAttributes structure for housing login credentials and then a second EntryAttributes for putting those "secret question" questions and answers). It would in theory also even be API compatible to leave EntryAttributes *attributes() and augment it with EntryAttributes *attributes(QString title)

I do hope this comes across in the correct tone: I am not lobbying for that kind of change, I just wanted to offer it for your consideration.

The lobbying bit is to find out whether a PR with reasonable component boundaries that conforms to the project's code style could land.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Feb 7, 2018

I think a feature to import 1P database directly is very nice and useful. (That is what this issue aims for)
Note that the database opened in KeePassXC afterwards will be saved as KDBX, so it's a gateway feature for new users or alternatively for 1P users that need a read-only access to their database/vault on platforms supported by KPXC.

About the code style, we are pretty active in offering support and fix problems when PR are submitted.

For completeness, I think Reading/Writing/Fully supporting 1P database is a little bit out of scope of this project and an Issue/PR in this direction should be discussed

@hifi
Copy link
Member

hifi commented Feb 7, 2018

@mdaniel If there is a plugin for KeePass that implements some of the 1P features that you are using it might be worth importing the data in that format so that a KeePass plugin would work with KeePass and later on if KeePassXC gains the same feature it can be made compatible with the existing KeePass plugin and everyone wins.

It's rather easy to test then by saving the database in KeePassXC after importing and testing with KeePass if a plugin you target works with the imported data.

This is/was true for SSH keys for example as 2.3.0 will be mostly compatible with the KeeAgent plugin for KeePass.

@mdaniel
Copy link
Author

mdaniel commented Feb 8, 2018

I think a feature to import 1P database directly is very nice and useful. (That is what this issue aims for)

It is, and that sounds like the green light I was looking for. I'll try to clean up the source and put the PR in flight.

For completeness, I think Reading/Writing/Fully supporting 1P database is a little bit out of scope of this project and an Issue/PR in this direction should be discussed

Yes, I understand, and based on the information I have right now, I think that would be quite a bit more work

@mdaniel
Copy link
Author

mdaniel commented Feb 8, 2018

If there is a plugin for KeePass

Pardon my inexperience, but I thought this was the "current" KeePass implementation; do you mean KeePass at SourceForge? While it is kind of them to offer zipped source downloads, their Subversion repo is quite obviously not the real source tree and a cursory search didn't produce any alternative to the zip. I'm building Mono now, as I haven't touched that ecosystem in a long time, but their platform choice leads me to believe that making a KeePass plugin would be just for that project, and wouldn't be easily shared with this one -- is my mental model correct?

Or did I overlook a plugin mechanism for KeePassXC?

@hifi
Copy link
Member

hifi commented Feb 8, 2018

@mdaniel Ah, you misunderstood me.

KeePass is still an actively worked project and I also agree their development method isn't the best by just providing snapshots of the history rather than having an open model like KeePassXC has.

KeePassX was originally started to have a better KeePass alternative for non-Windows platforms and KeePassXC is a fork of the project. Original KeePass 1.x was a native Windows application and the current 2.x is a managed .NET application that does run on Mono on Linux and Mac but somewhat poorly.

What I meant was that if there are existing plugins for KeePass 2 that provide some of the features 1P has they also store the data somehow in the KDBX file and if you are importing data from 1P through KeePassXC it would be a good idea to convert to the same internal format for KDBX even if KeePassXC does not currently support the feature. That makes the imported data work with existing KeePass 2 plugins.

It might very well be that there are no existing KeePass 2 plugins for the features of 1P that you would need to import and I'm just unnecessarily confusing you.

So no, I didn't mean creating a KeePass 2 plugin but looking if some of the 1P features exist as plugins for KeePass 2 that are not yet in KeePassXC and trying to keep the imported data in the same format as the plugins use.

@droidmonkey
Copy link
Member

Off topic, but want to echo how decidedly un-open KeePass is....

@user13542
Copy link

@mdaniel
Are you currently working on this? Just wanting to know, as I am hoping for a more thorough way to switch from 1Password than the csv method.

@droidmonkey
Copy link
Member

No but I would definitely welcome a PR for this functionality.

@mdaniel
Copy link
Author

mdaniel commented Jun 27, 2018

"Currently" is a bit strong, but I have some local bugfixes that I need to push up. I also only recently upgraded to 1P 7 and need to ensure they haven't silently switched out something that changed the specification or their implementation.

My original reservation in not opening the PR was that the code is very proof-of-concept-y and not well structured. That is what I was hoping to get cleaned up, because so far the functionality does what I have asked it to do -- (aka "works on my machine" :-) )

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 a pull request may close this issue.

6 participants