Prompt to unlock individual item when getting secret from keyring in unix system #108
Conversation
Signed-off-by: AlanD20 <aland20@pm.me>
|
After looking into it more, looks like the mock provider changes will be a breaking change. Not sure what to do here. Probably I should remove the mock changes, or should it unlock whenever it gets a secret without the user explicitly unlocking the item? |
|
@AlanD20 Thanks for the PR and the detailed description. Please allow a few days for us to review it. |
| err = svc.Unlock(item) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Would be awesome if it's possible to create a test case showing this working as expected. You can see how we do the tests currently: https://github.com/zalando/go-keyring/blob/master/.github/workflows/go.yml
There was a problem hiding this comment.
Ok, I figured out how to reproduce using the test cases but I dont know how to write the test cases for it because it's a bit tricky.
basically, a test case has to get an existing secret from the keyring. The issue with the existing TestGet case is that it initially sets the secret which it already has an open/unlocked session, and then it gets the secret, which is totally fine.
But in the case of KeepassXC, where it has the option to lock items, when the session gets closed and another session opens to only retrieve the secret, it requires a prompt from the user to confirm.
I enabled gnome-keyring-daemon locally and ran the test case but it seems to work because it looks like the gnome-keyring-daemon does not have an option to prompt when a secret is retrieved.
If you have KeepassXC enabled as a secret service. Make sure to enable the prompt option, then try this test case:
func TestGetLockedItem(t *testing.T) {
pw, err := Get(service, "locked-user")
if err != nil {
if err == ErrNotFound {
err := Set(service, "locked-user", password)
if err != nil {
t.Errorf("Should not fail, got: %s", err)
}
t.Skip("Skipping: Secret stored in keyring, rerun tests to confirm test case.")
}
t.Errorf("Should not fail, got: %s", err)
}
if password != pw {
t.Errorf("Expected password %s, got %s", password, pw)
}
}This test case has to be run twice. First to set the secret if it's not already set, and then if it's already set, get the secret. I dont like this since it requires to run go test twice.
Any idea how to test this? 😅
here is also a video
go-keyring-test-case.mp4
Signed-off-by: AlanD20 <aland20@pm.me>
|
👍 |
|
👍 |
|
Thanks for the help and review! Will there be a new release for this? I'm planning to create a new PR in gh cli repository to use this change so that it will get fixed. |
|
@mikkeloscar Awesome! Thanks again :) |
Hi, this PR unlocks individual items before getting secret from the keyring for unix systems.
Why?
I'm using KeepassXC on Arch as a secret service. And I was trying to setup gh CLI where it requires git credentials, and I wanted to use libsecret to store the credentials, so I updated my git config:
In my KeepassXC settings, I have a checkbox checked where, by default it locks all collections and items. A client has to ask for unlock (a prompt) so that I will allow the client to retrieve the secret from the item or the collection.
KeepassXC => Tools > Settings > Secret Service Integration > Confirm when passwords are retrieved by clientsgh CLI is able to use the libsecret with my KeepassXC as a secret service but after debugging and using
dbus-monitorandbusctl monitor --userto see what the requests are.I found this error from `dbus-monitor` (Exapndable)
Looking closely at the requests here, it appears that go-keyring is able to unlock the collection which is done at
go-keyring/keyring_unix.go
Lines 54 to 65 in 28657a5
dbus-monitor:PR Changes
My changes attempt to unlock the item before getting the secret, which I have tried locally, and it works with gh cli as well. (see below to reproduce my local environment and my test steps.)
Looking at the secret-service documentation and specifically the first line says:
and the fifth line:
How is gh cli related to this?
A cli/cli#8691 (comment) by GitHub CLI team confirms that they are not doing anything and simply using
go-keyringlibrary to set and get these secrets from keyrings. I have also confirmed locally that they usego-keyringwrapper here: https://github.com/cli/cli/blob/trunk/internal/keyring/keyring.goWhat issue does it resolve?
probably other packages that use
go-keyringas a dependency. I'm only using gh which usesgo-keyringas a primary dependency for getting secrets from keyring.Test Steps to Reproduce
Confirm when passwords are retrieved by clientsatKeepassXC => Tools > Settings > Secret Service Integration > Confirm when passwords are retrieved by clientsgit-credentials-libsecretat/usr/lib/git-core/git-credential-libsecret.git config --global credential.helper libsecret # or absolute path git config --global credential.helper /usr/lib/git-core/git-credential-libsecretdbus-monitorto see secret service requests.gh auth status. (This will fail and says not authenticated)Test PR Changes locally
~/buildsand the following steps will clone 2 projects in the~/buildsdirectory, and also builds gh cli locally at~/buildsas well.go.modfile to point thego-keyringpackage to the PR changes. My changes are at../go-keyringrelative to the cloned gh cli repository locally.go mod tidyjust in case.make install prefix=../ghgo-keyringchanges:cd ~builds/gh/bin && ./gh auth statusNotes
Thanks!