#131 Extend abilities of Key Provider plugins.

[Artur Pragacz]

Hello.
I was trying to integrate my Yubikey with a KeePass database. Unfortunately I was not satisfied with the already existing solutions, so I decided to write my own.
This effort can be seen here: KeePassChallenge.
In order to achieve all the advantages that my plugin has over the others, I needed to extend the KeePass Key Provider Plugin API.
This post is a proposal to integrate my changes into the official KeePass source.
That will allow my plugin to work with it and potentially allow others to create more feature rich plugins as well.
I tried to keep the API fully backwards compatible, so that the already existing plugins don't suffer from any breakage.
The changes are included as patches under this post and are also available on GitHub.
I'm looking forward to hearing what you think.

[Dominik Reichl]

Thanks for the proposal! However, I'm not planning to include these changes, for the following reasons.

Key providers should be independent of databases. There isn't necessarily a database reference that could be passed to a key provider. An example would be an import/export module for an encrypted file format (there is no such module right now, but I don't want to make such developments in the future impossible).

AES-KDF and Argon2 use a salt, but this isn't a general requirement for a key derivation function, and thus it doesn't make sense to add a method to KdfEngine for getting a salt.

Your plugin seems to reuse a key derivation function salt (if one exists) as a challenge. Although this might be secure, using one salt for multiple things in general is dangerous; I don't really like this.

Your proposal changes various public methods/types, which can break compatibility with existing plugins. This should only be done for significant reasons, which I don't see here.

If your key provider needs additional data, my suggestion would be to store this data in an auxiliary file. An example is my OtpKeyProv plugin. One advantage is that a small auxiliary file can be updated more easily than the database file (e.g. OtpKeyProv updates the auxiliary file each time a database is opened; writing a small file is more efficient and less problematic than rewriting a large database file, especially when they are stored in a cloud service).

Best regards,
Dominik

[Artur Pragacz]

Hi, I would like to discuss this further, if you don't mind. I put a nontrivial amount of work behind this, and so would like to see the way forward.

The current integration of KeePass with Yubikey is honestly in a sad state and leaves a lot to be desired. This is especially obvious when compared to KeePassXC, which does a much better job in my view. I would really like to keep using KeePass though, especially on my Windows machine.

Key providers should be independent of databases. There isn't necessarily a database reference that could be passed to a key provider. An example would be an import/export module for an encrypted file format (there is no such module right now, but I don't want to make such developments in the future impossible).

Then you can just pass null as a parameter. This way the specific IUserKey can decide what do with it. The ones coming from existing plugins will keep working the same way they do, since they don't need such a reference. In my plugin I will be able to detect such a scenario and provide with a message for the user.

We can off course use a different mechanic of signalling, but the point remains the same - we should be providing plugins with an option to include knowledge about the database in a key provider. This opens many possibilities.

AES-KDF and Argon2 use a salt, but this isn't a general requirement for a key derivation function, and thus it doesn't make sense to add a method to KdfEngine for getting a salt.
Your plugin seems to reuse a key derivation function salt (if one exists) as a challenge. Although this might be secure, using one salt for multiple things in general is dangerous; I don't really like this.

I have to correct you here. I use unencrypted CustomData of the database to store the challenge. The only reason I'm accessing the key derivation function salt at all is to provide compatibility with KeePassXC/KeePass2Android/KeeWeb and others, who already use such a method. We seem to have similar tastes in this regard as I also didn't like it and so decided to make my implementation different. The compatibility would be nice to have though, but is not essential for this proposal going forward. That's why the change is in a separate patch.

Your proposal changes various public methods/types, which can break compatibility with existing plugins. This should only be done for significant reasons, which I don't see here.

I don't believe that many plugins would use the methods that I changed. I provided for compatibility with already existing key provider plugins, so the most common use case is definitely covered.

If your key provider needs additional data, my suggestion would be to store this data in an auxiliary file. An example is my OtpKeyProv plugin. One advantage is that a small auxiliary file can be updated more easily than the database file (e.g. OtpKeyProv updates the auxiliary file each time a database is opened; writing a small file is more efficient and less problematic than rewriting a large database file, especially when they are stored in a cloud service).

I strongly disagree. Having a single database file and not having to worry about some additional files is essential for the convenience of the end user and the general user experience.

There is a reason we have unencrypted place for plugins to store information in a database. That data should be provided to plugins as soon as it's read and therefore be accessible in a key provider. Whether that information should be provided as a PwDatabase reference, like in this proposal, can off course be debated. While other solutions are also technically possible, this one definitely provides the most flexibility for future plugins to use, which is why I designed it this way, but I'm happy to discuss that aspect going forward.

[Dominik Reichl]

I'm not convinced. In my opinion, the disadvantages outweigh the advantages, and think that an auxiliary file is the better solution.

Anyway, if you really want to use PublicCustomData/KdfParameters: I've now added a static method LoadHeader for the PwDatabase class (requiring an IOConnectionInfo parameter, returning a PwDatabase), which loads only the unencrypted header of a database file. So, in the GetKey method of your KeyProvider, you can do something like this:

public override byte[] GetKey(KeyProviderQueryContext ctx)
{
    PwDatabase pd = PwDatabase.LoadHeader(ctx.DatabaseIOInfo);
    // Use pd.PublicCustomData, pd.KdfParameters, ...
}

With this implementation, you can access all the data stored in the unencrypted header of a database file, and we don't break any existing plugins. Side note: adding a Database property to KeyProviderQueryContext isn't possible, because KeePass hasn't opened the database yet when calling the GetKey method (the construction of the master key must be separate from opening the database, e.g. in order to allow a key provider to display a dialog on the secure desktop).

Here's the latest development snapshot for testing:
https://keepass.info/filepool/KeePass_220608.zip
(includes the modified source code files).

Thanks and best regards,
Dominik

[Artur Pragacz]

I'm not convinced. In my opinion, the disadvantages outweigh the advantages, and think that an auxiliary file is the better solution.

Hmmm, honestly I fail to see any advantages of an auxiliary file in this specific case of my plugin. The only thing you explicitly mentioned is that a small auxiliary file can be updated more easily than the database file, but that is irrelevant in this case: every time the challenge changes, so does the key that is used to encrypt the database, so it has to be rewritten anyway. This is good though - it more closely mimics the properties of the 2FA known from online systems.

I've now added a static method LoadHeader for the PwDatabase class

One problem I see with this approach is that as far as I understand (correct me if I'm wrong) it requires us to read the database twice, first just the header, then the whole thing. This seems weirdly wasteful to me.
On the other hand I do understand the value of backwards compatibility and so I can appreciate why you would opt for this type of change. I want to thank you for willing to work with me to find a good compromise solution.

the construction of the master key must be separate from opening the database, e.g. in order to allow a key provider to display a dialog on the secure desktop

I don't quite understand this point. It admittedly may be because I don't know enough about how secure desktop operates. But what exactly prevents us from first reading the unencrypted header of the database, then asking for the key, and then reading the rest of the database? How would secure desktop interfere with this workflow?

[Dominik Reichl]

Opening the database file must occur on the regular desktop (for example because an interaction with a storage service may be required that doesn't work on a secure desktop). Entering the master key should occur on the secure desktop though, including getting a key from a key provider (in order to allow the key provider to display a dialog on the secure desktop, if necessary).

It would indeed be possible to pass around the database stream or partial data, but that would be an awkward design. Furthermore, the database stream would need to be kept open while the user enters the master key, which could result in a server timeout, more collisions when multiple users are working with one database file, etc.

Reading the header (or an auxiliary file) separately avoids that, and the performance shouldn't really be a problem (as the header is usually very small).

Best regards,
Dominik

[Artur Pragacz]

OK, I changed the plugin to use PwDatabase.LoadHeader. I'm still not exactly a fan of reading it twice separately, but it hopefully won't be a problem in practice, so let's leave it at that.

However I still need some additional small changes to KeePass source to make my plugin work. I include them below. Both changes are backwards compatible, so no existing plugins should break.

The first patch allows plugins to create their own Key types. I need it to reliably detect when the database uses my plugin as a KeyProvider and also to store some information relevant to the key, most importantly the challenge used to generate the response.

The second patch adds a simple event fired when the database MasterKey is changed. I need it to adequately update the header when it happens.

Like I already said, both patches are backwards compatible and should break no existing plugins. Let me know whether you have any objections or see any issues with either of them.

[Dominik Reichl]

The MasterKeyChanged event is fine. I've added it.

Here's the latest development snapshot for testing:
https://keepass.info/filepool/KeePass_220614.zip

I'm not going to include your GetCustomKey method; this does not fit with the current design (KeePass should know all key types, etc.).

In order to detect whether your key provider is used as part of a CompositeKey, you can search its UserKeys list for a KcpCustomKey having the name of your key provider (KeePass also identifies a key provider by its name; if you fear name collisions, make your name more unique, e.g. 'YubiKey Challenge-Response (KeePassChallenge)' instead of 'Yubikey challenge-response'). Your plugin could remember the challenge and then store it during the MasterKeyChanged event (this adds a bit of complexity in your plugin, but keeps the key provider API clean for everyone).

Thanks and best regards,
Dominik