store/keychain: windows support#67
Conversation
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
…d error Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Benehiko
force-pushed
the
keychain-windows
branch
from
c934207 to
1f57153
Compare
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the Windows Credential Manager API by introducing a dedicated keychain implementation for Windows, updating module dependencies, and enabling Windows tests in the workflow.
- Introduces a new implementation in store/keychain/keychain_windows.go to manage credentials using the wincred library.
- Updates go.mod to include the wincred dependency and bumps versions for related packages.
- Uncomments and adjusts the Windows tests in the GitHub Actions workflow.
Reviewed Changes
Copilot reviewed 3 out of 24 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| store/keychain/keychain_windows.go | Implements Windows support for handling credentials using the Windows Credential Manager API. |
| store/go.mod | Adds the wincred dependency and updates versions for golang.org/x/sys and golang.org/x/text. |
| .github/workflows/keychain.yml | Activates Windows-based tests by uncommenting and adjusting the workflow configuration. |
Comments suppressed due to low confidence (1)
store/keychain/keychain_windows.go:156
- [nitpick] Consider renaming the variable 'g' to a more descriptive name such as 'credential' to improve code clarity.
g := wincred.NewGenericCredential(k.itemLabel(id))
| if err != nil && !errors.Is(err, wincred.ErrElementNotFound) { | ||
| return mapWindowsCredentialError(err) | ||
| } | ||
| if g == nil { |
There was a problem hiding this comment.
For readability: could we change that to g == nil || errors.Is(err, winced.ErrElementNotFound)? (it probably doesn't change logic but makes reading the code easier)
There was a problem hiding this comment.
But also, should deleting a secret that doesn't exist return an error? 🤔
Regardless of the answer, is that consistent across all OS and can we add verifying that it's consistent to the unit tests that check across all OS?
There was a problem hiding this comment.
It's not consistent, windows can return an error yes 🙈
https://learn.microsoft.com/en-us/windows/win32/api/wincred/nf-wincred-creddeletew#return-value
There was a problem hiding this comment.
What I meant is, can you add to keychain_test.go a test case that verifies deleting a non existing secret behaves the same across all implementations? We don't have that test right now.
There was a problem hiding this comment.
We can add more tests, but i don't think it's consistent in terms of when it errors. e.g. on success it returns nil error, but if it does error it can be for various reasons.
I'll do that in a follow-up when refactoring the tests.
There was a problem hiding this comment.
simplified the if check - instead of querying first, I let the delete handle the ErrNotFound.
| if err != nil && !errors.Is(err, wincred.ErrElementNotFound) { | ||
| return mapWindowsCredentialError(err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The answer above might affect this as well.
joe0BAB
left a comment
joe0BAB
left a comment
There was a problem hiding this comment.
LGTM! Could we add the missing test in keychain_test.go to the TODO/issue that is going to improve those tests? I believe we already have that somewhere.
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Benehiko
force-pushed
the
keychain-windows
branch
from
87d000a to
6c380cb
Compare
Closes #51
Closes #52
This patch adds support for the Windows Credential Manager API. It uses the danieljoos/wincred library. All credentials stored by the Windows Credential Manager are encoded in UTF16 format - since most applications accessing the credential manager expect this format.
Relates to #16