Support multiple GPG keys #6
Conversation
| var message = $"Unable to find entry with the search string {param.SearchString}."; | ||
| ShowBalloonNotification(message); |
There was a problem hiding this comment.
This message is unfortunately vague since it does not state which fields of the entry are being searched. Furthermore, the predicate can perform some arbitrary check; what it checks for cannot be inferred at this point. I think that for more specific messages, the error would have to be handled in all the higher-level wrappers like GetByUserName, but that might feel like duplicate code.
|
Hey I'm still reviewing this! Was trying to fix my local build environment (Microsoft has gone and changed their docker images). Once I've got it building, I'll test it out. Thanks! |
|
Yeah I had the same problem. IIRC I did manage to change it to the right base image, but then it complained that it's not supported by my platform. This is probably because I needed to switch to Windows container mode, but that requires a system restart so bleh. I just opened Visual Studio and built it that way. Also, I realised that I forgot to update the README with new usage instructions. I suppose I will wait for your review in case something needs to be changed before I update the instructions. |
There was a problem hiding this comment.
I got my build working locally, the remote one is broken because of travis deciding to stop being nice to open source projects and force us to use their paid model. I'll move to github actions eventually.
I found this command:
gpg-connect-agent "keyinfo --list" /bye
Which prints out the keygrip, we should include that in the documentation updates.
| { | ||
| PwEntry entry = string.IsNullOrWhiteSpace(keyInfo) | ||
| ? db.GetByTitleAndUserName("GPG", "DEFAULT") | ||
| : db.GetByUserName(keyInfo); |
There was a problem hiding this comment.
We may want to fallback to the 'default' entry to support existing use cases. I use a blank username for the 'default' entry personally.
There was a problem hiding this comment.
I think we need GetByUserName to require GPG as the Title still.
Main reason will be for security. Currently only the GPG key is being 'exposed' over the gpg pinentry agent. Technically any program on your system though can fake the pinentry protocol and ask for the password (we don't have a way to detect that it's a legit request).
Therefore we don't want arbitrary keys to be able to be requested.
There was a problem hiding this comment.
We may want to fallback to the 'default' entry to support existing use cases.
Can you elaborate on what you mean? Are you referring to the previous behaviour that just gets the first "GPG"-named entry?
I think we need GetByUserName to require GPG as the Title still.
I agree. You made an excellent point. I never felt strongly about making the name flexible anyway.
| try | ||
| { | ||
| IEnumerable<PwEntry> entries = GetAll(param); | ||
| PwEntry entry = predicate is null ? entries.First() : entries.First(predicate); |
There was a problem hiding this comment.
FirstOrDefault() will let you handle the 'not found' case nicer than an exception.
There was a problem hiding this comment.
Do you think it's significantly nicer to compare against null than to catch an exception? I am more in the habit of "easier to ask for forgiveness than for permission."
Maybe if I have some time, I could help out with that as well. :)
I was using My concern with keygrips is specifically regarding the prefix. I haven't found a way to ask GPG for this prefix or any more documentation on what it means (I didn't look too hard). I'm not sure how to help users find this besides making them try to use the key and looking at the name in the error notification from KeePass. |
This feature relies on getting a key ID from
SETKEYINFO, so I assume it only works when--no-allow-external-cacheis not set as a GPG agent option. This seems to be a reliable way to get a key ID except for this case. One aspect I am unsure of is what determines that format of the ID provided by the agent. The documentation states that it could be in 3 different forms. In practice, for me, it has used then/key-gripformat. To help the user figure out the exact value, a balloon notification with the key info string is shown if a matching entry cannot be found.When the key info is available, the entry is looked up by comparing the username to the key info. Due to the nature of the key ID, it seems unlikely that there'll be a false positive match for some other unrelated entry. I figured allowing the title to be anything is nice for flexibility, but requiring "GPG" in the title too could be implemented if desired.
If for some reason the key info is cleared, the plugin instead looks up an entry by title using "GPG" as the search string. Then, it looks for the entry whose username is "default" (case-insensitive). An existing entry could be set as default by the user by using a password field reference for the default entry.
Some error handling has been added so that the GPG agent doesn't hang waiting for a response if the plugin is unable to find a suitable entry or crashes before the pin can be sent to the agent.