store/keychain: cross-platform test cli and more tests by Benehiko · Pull Request #16 · docker/secrets-engine

Benehiko · 2025-06-17T09:29:42Z

This patch adds an easy to use CLI to the keychain package. It further cleans up the keychain tests.

Copilot

Pull Request Overview

This PR introduces a Cobra-based CLI for the keychain package and refactors existing tests for better isolation and cleanup.

Adds store/keychain/cmd/main.go with commands: list, save, get, erase.
Refactors keychain_test.go by introducing setupKeychain, using t.Cleanup, and adding edge-case tests.
Updates go.mod to include Cobra and its dependencies.

Reviewed Changes

Copilot reviewed 3 out of 75 changed files in this pull request and generated 1 comment.

File	Description
store/keychain/keychain_test.go	Refactored tests with helper setup, cleanup, and added a non-existent delete test
store/keychain/cmd/main.go	Implemented cross-platform CLI using Cobra commands
store/go.mod	Added `github.com/spf13/cobra` and related indirect deps

Comments suppressed due to low confidence (3)

store/keychain/cmd/main.go:59

The literal "keystore-cli" used as the base path for IDs doesn’t match the product name "docker-example-cli" given to keychain.New. Consider defining a shared constant for the ID prefix to keep them in sync across commands.

			id, err := store.ParseID(path.Join("keystore-cli", username))

store/keychain/cmd/main.go:70

[nitpick] The flag description "The secret key" for --username is misleading; it should clearly indicate that this flag sets the username. Likewise, update the --password description to something like "Password for the credential".

	save.PersistentFlags().StringVar(&username, "username", "", "The secret key")

store/keychain/keychain_test.go:60

It would be valuable to add a test for listing when there are no credentials saved (e.g., expecting an empty map or a specific error) to cover the GetAll edge case.

	t.Run("list all credentials", func(t *testing.T) {

joe0BAB · 2025-07-02T08:47:07Z

+func newCommand() (*cobra.Command, error) {
+	kc, err := keychain.New(
+		"io.docker.Secrets",
+		"docker-example-cli",


not sure this matters much, but I'd be inclined to be more specific here, e.g., docker-local-secrets-example-cli or something as docker-example-cli without context feels very generic (the circumstances of the code would give more context, but with a more explicit name it wouldn't even need that)

it's kind of supposed to be generic - it's just used to basically test the keychain library manually.

But wouldn't it appear in the keychain, eg, if I'd open the mac keychain app, wouldn't I see it there? I was mostly wondering from that perspective, if it would be preferable to have something less generic than docker-example-cli show there.

i mean, this is is never automatically executed - so that shouldn't be a problem.

joe0BAB · 2025-07-02T08:54:09Z

 		require.NoError(t, ks.Save(t.Context(), id, creds))
 	})

 	t.Run("get credential", func(t *testing.T) {


Suggested change

t.Run("get credential", func(t *testing.T) {

t.Run("save/get credential", func(t *testing.T) {

aren't we testing both?

we kind of are, i guess having a separate run at least for get might have better debugging / failure logs?

I don't have a strong opinion here, up to you

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

+				if err != nil {
+					e = errors.Join(e, err)
+				}
+				fmt.Printf("\nID: %s\nValue: %s\n", k, vv)


To fix the issue, the sensitive data (password) should be omitted, obfuscated, or encrypted before being logged. In this case, the best approach is to log only the username and omit the password entirely. This ensures that sensitive information is not exposed in the logs while maintaining the functionality of the CLI tool.

Steps to implement the fix:

Modify the Marshal method in store/mocks/mock_credential.go to exclude the password when converting the credential to a byte slice for logging purposes.

Update the fmt.Printf statement in store/keychain/cmd/main.go to log only the username and ID, omitting the sensitive value.

+			if err != nil {
+				return err
+			}
+			fmt.Printf("Secret:\nID:%s\nValue:%s\n", id.String(), val)


To fix the issue, sensitive information (e.g., Password) should be excluded from logging. Instead of logging the full marshaled value, the code should log only non-sensitive information, such as the ID or Username. This ensures that sensitive data is not exposed in plaintext logs.

The fix involves modifying the fmt.Printf call on line 93 in store/keychain/cmd/main.go to exclude the Password field from the logged output. Additionally, the Marshal method in store/mocks/mock_credential.go should be updated to obfuscate or exclude sensitive data when marshaling.

joe0BAB · 2025-07-02T11:40:16Z

+		},
+	}
+
+	erase := &cobra.Command{


Suggested change

erase := &cobra.Command{

delete := &cobra.Command{

nit

delete is a keyword in Go, that's why i kept it 🙈

joe0BAB

LGTM (just a few non-blocking nits)

Co-authored-by: Johannes Großmann <grossmann.johannes@t-online.de>

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

Benehiko self-assigned this Jun 17, 2025

github-advanced-security AI found potential problems Jun 18, 2025

View reviewed changes

Comment thread keychain/cmd/main.go Fixed

Benehiko force-pushed the keychain branch 2 times, most recently from a67bf98 to 021ad13 Compare June 20, 2025 14:59

github-advanced-security AI found potential problems Jun 20, 2025

View reviewed changes

Comment thread keychain/cmd/main.go Fixed

Comment thread keychain/cmd/main.go Fixed

Benehiko force-pushed the keychain branch 2 times, most recently from acfd741 to 527cd3b Compare June 23, 2025 09:41

github-advanced-security AI found potential problems Jun 23, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go Fixed

Comment thread store/keychain/cmd/main.go Fixed

Benehiko force-pushed the keychain branch 2 times, most recently from afab762 to 03148de Compare June 23, 2025 10:01

Benehiko mentioned this pull request Jun 23, 2025

store: add keychain package #25

Merged

Benehiko force-pushed the keychain branch 2 times, most recently from 185fa01 to 4deef40 Compare June 23, 2025 14:16

Benehiko mentioned this pull request Jun 24, 2025

store/keychain: macOS support #37

Merged

Benehiko force-pushed the keychain branch 2 times, most recently from b2cbfbf to a275d39 Compare June 24, 2025 11:25

github-advanced-security AI found potential problems Jun 24, 2025

View reviewed changes

Comment thread .github/workflows/keychain.yml Fixed

Benehiko force-pushed the keychain branch 6 times, most recently from 0b771e9 to f174d37 Compare June 25, 2025 06:51

Benehiko mentioned this pull request Jun 25, 2025

store/keychain: support linux #46

Merged

Benehiko force-pushed the keychain branch 5 times, most recently from 4c170a6 to dd24d05 Compare June 26, 2025 08:34

Benehiko mentioned this pull request Jun 26, 2025

store/keychain: fix linux check collection locked status #55

Merged

Benehiko requested a review from Copilot July 2, 2025 08:10

Benehiko marked this pull request as ready for review July 2, 2025 08:11

Benehiko requested review from joe0BAB and wmluke July 2, 2025 08:11

Copilot AI reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/cmd/main.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/keychain_test.go

Benehiko added 2 commits July 2, 2025 11:50

store/keychain(test): id and secret marshal/unmarshal errors

b07782e

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

store/keychain(cli): improve command names and CLI documentation

ae544b4

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

Benehiko force-pushed the keychain branch from 7ae708d to ae544b4 Compare July 2, 2025 11:28

github-advanced-security AI found potential problems Jul 2, 2025

View reviewed changes

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/README.md Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/README.md Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/keychain_test.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/keychain_test.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/keychain_test.go Outdated

joe0BAB reviewed Jul 2, 2025

View reviewed changes

Comment thread store/keychain/keychain_test.go Outdated

joe0BAB approved these changes Jul 2, 2025

View reviewed changes

Benehiko and others added 3 commits July 2, 2025 13:58

store/keychain: update README

159b64f

Co-authored-by: Johannes Großmann <grossmann.johannes@t-online.de>

store/keychain: update README

ecd1f81

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

store/keychain(test): refactor mock credential types

63ac2a4

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

Benehiko merged commit 0fc3322 into main Jul 2, 2025

Benehiko deleted the keychain branch July 2, 2025 12:02

@@ -18,3 +18,3 @@
             func (m *MockCredential) Marshal() ([]byte, error) {
-            	return []byte(m.Username + ":" + m.Password), nil
+            	return []byte(m.Username), nil
             }

@@ -92,3 +92,3 @@
             			}
-            			fmt.Printf("Secret:\nID:%s\nValue:%s\n", id.String(), val)
+            			fmt.Printf("Secret:\nID:%s\nValue: [REDACTED]\n", id.String())
             			return nil

@@ -18,3 +18,3 @@
             func (m *MockCredential) Marshal() ([]byte, error) {
-            	return []byte(m.Username + ":" + m.Password), nil
+            	return []byte(m.Username + ":********"), nil
             }

	t.Run("get credential", func(t *testing.T) {
	t.Run("save/get credential", func(t *testing.T) {

@@ -47,3 +47,3 @@
             				}
-            				fmt.Printf("\nID: %s\nValue: %s\n", k, vv)
+            				fmt.Printf("\nID: %s\nUsername: %s\n", k, string(vv))
             			}

Conversation

Benehiko commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check failure

Copilot Autofix

Check failure

Copilot Autofix

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joe0BAB left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Benehiko commented Jun 17, 2025 •

edited

Loading