match Get() API in libsecret and keychain, and add tests

[joemiller]

This PR includes several commits that came out of my attempt to address #45

• adds a GetMetadata() stub to pass.go which was necessary to build or run tests on the package.
• adds a GetMetadata() stub to wincred.go which was necessary to build or run tests on the package.
• Added new libsecret_test.go to assert behavior of the secret-service backend (NOTE: these tests can't be run in CI or a headless environment right now because they require an app providing the 'Prompt' service on DBus. I left some notes about this and future directions in the comments.)
• adds tests to wincred_test.go, pass_test.go, and keychain_test.go to assert behavior or get/remove/keys when the backing store is empty.
• adds directory vagrant/ which contains GUI VM's for fedora and windows10 to provide a way to dev/test on the libsecret and wincred backends.
• Finally, libsecret.go API behavior around Get/Remove/Keys has changed. Get() now returns ErrKeyNotFound just like the keychain and wincred backends. I also added a new public ErrCollectionNotFound that is returned by Remove() and Keys() instead of the previous private secretsError.

Current master branch behavior of all backends when each of the API calls are made with an empty backing store (ie: before the first call to Set() occurs:

backend	get	set	remove	keys
keychain	ErrKeyNotFound	nil	gokeychain.ErrorNoSuchKeychain	gokeychain.ErrorNoSuchKeychain
libsecret	secretsError	nil	secretsError	secretsError
wincred	ErrKeyNotFound	nil	ErrKeyNotFound	nil
pass	exec.ExitError	nil	exec.ExitError	nil

After change to libsecret.go in this PR. Changes are highlighted:

backend	get	set	remove	keys
keychain	ErrKeyNotFound	nil	gokeychain.ErrorNoSuchKeychain	gokeychain.ErrorNoSuchKeychain
libsecret	ErrKeyNotFound	nil	ErrCollectionNotFound	ErrCollectionNotFound
wincred	ErrKeyNotFound	nil	ErrKeyNotFound	nil
pass	exec.ExitError	nil	exec.ExitError	nil

I have not included any changes to pass, kwallet, or file in this PR.

This doesn't address the API inconsistencies in the kwallet.go backend. I could probably take a crack at that in another PR but in my own application I'm considering not even supporting KWallet. I'm not a Linux desktop user and don't know the details behind it but it sounds like the project might be orphaned? And might have a replacement in KSecretsManager (or some such) that speaks the DBus Secrets API protocol like Gnome Keyring. If that's true then KDE users should be serviceable via the libsecret.go/"secret-service" backend.

[joemiller]

Here is the thread I found on the status of KWallet:

https://mail.kde.org/pipermail/plasma-devel/2016-July/055671.html
https://mail.kde.org/pipermail/plasma-devel/2016-July/055791.html
https://mail.kde.org/pipermail/plasma-devel/2016-July/055668.html

The last link in that chain indicates maybe there's another thing, QtKeyChain, that replaces KWallet. 🤷

[lox]

I'd be pretty open to removing the kwallet backend, frankly. I wonder if we can figure out how many folks are using it.

[joemiller]

Added a for windows 10 Vagrant box to help with dev/test on the wincred backend

[joemiller]

@lox I removed the '[WIP] tag from the PR title and updated the summary in the top comment of the PR. I would like to review/discuss what, if anything, you'd like to do with this PR.

[lox]

Yeah, let's do that

[lox]

Perhaps with wrapping from https://godoc.org/github.com/pkg/errors? I guess type checking gets harder then. We could go for keychain.IsErrNoKeyChain()?

[joemiller]

Will do. I had a thought that perhaps something along the lines of BackendNotInitialized instead of 'NoKeyChain' since "not initialized" is more generic.

RE: pkg/errors. I don't follow. Can you provide an example? Using the existing pattern for no such key is easy to check for errors:

i, err := kr.Get(vaultAddr)
		if err == keyring.ErrKeyNotFound {
			...
		}

[lox]

All good, ignore me for now, I think constant errors are fine here.

[joemiller]

@lox While setting about implementing something like a new error I thought that perhaps the best thing to do for all of these cases is to return the existing ErrNoSuchKey err whenever any of these methods is called before data has been added to the backend, so the upgraded grid for proposed behavior change would look like:

current:

backend	get	set	remove	keys
keychain	ErrKeyNotFound	nil	gokeychain.ErrorNoSuchKeychain	gokeychain.ErrorNoSuchKeychain
libsecret	secretsError	nil	secretsError	secretsError
wincred	ErrKeyNotFound	nil	ErrKeyNotFound	nil
pass	exec.ExitError	nil	exec.ExitError	nil

proposed:

backend	get	set	remove	keys
keychain	ErrKeyNotFound	nil	ErrKeyNotFound	ErrKeyNotFound
libsecret	ErrKeyNotFound	nil	ErrKeyNotFound	ErrKeyNotFound
wincred	ErrKeyNotFound	nil	ErrKeyNotFound	ErrKeyNotFound
pass	ErrKeyNotFound	nil	ErrKeyNotFound	ErrKeyNotFound

(behaviors in bold would change)

My rationale is that it is easier to reason about if all the read methods return a consistent error when the backend does not have any data yet.

[lox]

That makes sense to me too @joemiller

[joemiller]

@lox pushed a new ☝️ commit that implements the ErrKeyNotFound change

[joemiller]

rebased from master and fixed a new unit test covering the pass backend that was breaking on travis

[joemiller]

I realize this ended up being quite a large surface area to change when it started out as just the Get() implementation in libsecret.go. The changes make sense for the app I'm using but it's difficult for me to reason about how it make impact the other caller, aws-vault. I've never used aws-vault but I wonder if I could help test this PR against aws-vault. I may give that a shot. Any pointers on a test strategy would be appreciated.

[lox]

@joemiller I'm pretty close to merging this, as I 100% agree with your direction, but if you had the chance to test with aws-vault, it would rapidly increase that happening :)

[joemiller]

@lox I ran some tests to compare the output and exit codes of aws-vault with the current keyring lib and the proposed changes in this fork. There are some differences. I think the most notable differences is with the remove cmd since the exit code changes from 0 to 1, example:

## current keyring master branch:
$ aws-vault --backend=keychain --keychain="aws-vault-test" remove test1
Deleted credentials.
Deleted 0 sessions.
# (exit: 0)

## this PR #46 
$ aws-vault --backend=pass --pass-prefix="aws-vault-test" remove test1
Deleted credentials.
aws-vault: error: The specified item could not be found in the keyring
# (exit: 1)

The full output from my tests are in this gist: https://gist.github.com/joemiller/c26b1092a3d58ef09f5c65c692edbe6a There are 3 files. I included a diff because I can't think of a better way to do a side-by-side comparison of the differences. Hopefully the 2 test output files and the diff can help.

I had a feeling that perhaps the best approach might be for the list command to return nil and an empty list instead of ErrKeyNotFound. This is already the case for the wincred and pass backends, so I would revert to that behavior and so keychain and libsecret would be updated to return nil. The updated table would look like this:

backend	get	set	remove	keys
keychain	ErrKeyNotFound	nil	ErrKeyNotFound	nil
libsecret	ErrKeyNotFound	nil	ErrKeyNotFound	nil
wincred	ErrKeyNotFound	nil	ErrKeyNotFound	nil
pass	ErrKeyNotFound	nil	ErrKeyNotFound	nil

I'm going to try this out and run the same series of tests and add to the gist

[joemiller]

Ok, I re-ran the suite of tests and I think that aligning all the backends to return nil and empty list in Keys() is the right way to go. The results with these changes in aws-vault are almost identical to the current master version. The error on 'remove' is also fixed now. There are two exceptions: keychain and libsecret are now identical to the other backends instead of returning their own unique error, which I think is a good thing.

The full diff is small enough to include here:

@@ -37,7 +37,9 @@
 # keychain
 $ aws-vault --backend=keychain --keychain="aws-vault-test" list
-aws-vault: error: No such keychain (-25294)
+Profile                  Credentials              Sessions                 
+=======                  ===========              ========                 
+aws-vault: error: No credentials found
 # (exit: 1)
 
 $ aws-vault --backend=keychain --keychain="aws-vault-test" add test1
@@ -122,7 +124,9 @@
 
 $ aws-vault --backend=secret-service list
-aws-vault: error: The collection "awsvault" does not exist. Please add a key first
+Profile                  Credentials              Sessions
+=======                  ===========              ========
+aws-vault: error: No credentials found
 # (exit: 1)

Another gist with 3 files here - https://gist.github.com/joemiller/b099b6043f19957fac4fdfacf400f656

[akerl-unpriv]

Just went to rebuild my codebase with the latest keyring source, and it fails to build on windows due to lack of GetMetadata. Would rock to get this merged in.

Additionally, it looks like the tagged release isn't getting picked up by go mod since it lacks a leading "v"

[joemiller]

fwiw, just released my own project that uses this lib - https://github.com/joemiller/vault-token-helper running off my fork of keyring for now but hoping to get back to upstream soon =)

[joemiller]

@akerl-unpriv In the meantime, if you need to use this fork with go mod you should be able to do something like this: https://github.com/joemiller/vault-token-helper/blob/master/go.mod#L17

[akerl-unpriv]

Hah, yup; I just did similarly with github.com/akerl/keyring 😄

[lox]

Can I merge this @joemiller?

[mtibben]

Hey @joemiller I've made you a collaborator to this repo, fee free to merge

[joemiller]

@lox I feel pretty confident this is ready to merge. I tested this branch with aws-vault a few comments above and the results looked good to me. The only difference in behavior I expect is the different error messages from keychain and secret-service backends outlined in the diff in this comment - #46 (comment) - which actually make them identical to the error messages emitted by the other backends (pass, wincred) given the same context. 👍

[lox]

🚢 and welcome as a collaborator! Thanks for such a thoughtful and well executed PR.