Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions store/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,19 @@ require (
github.com/docker/secrets-engine v0.0.0-00010101000000-000000000000
github.com/keybase/dbus v0.0.0-20220506165403-5aa21ea2c23a
github.com/keybase/go-keychain v0.0.1
github.com/spf13/cobra v1.9.1
github.com/stretchr/testify v1.10.0
golang.org/x/sys v0.29.0
golang.org/x/text v0.21.0
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/kr/text v0.2.0 // indirect
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/spf13/pflag v1.0.6 // indirect
golang.org/x/crypto v0.32.0 // indirect
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
Expand Down
8 changes: 8 additions & 0 deletions store/go.sum
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/danieljoos/wincred v1.2.2 h1:774zMFJrqaeYCK2W57BgAem/MLi6mtSE47MB6BOJ0i0=
github.com/danieljoos/wincred v1.2.2/go.mod h1:w7w4Utbrz8lqeMbDAK0lkNJUv5sAOkFi7nd/ogr0Uh8=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
github.com/keybase/dbus v0.0.0-20220506165403-5aa21ea2c23a h1:K0EAzgzEQHW4Y5lxrmvPMltmlRDzlhLfGmots9EHUTI=
github.com/keybase/dbus v0.0.0-20220506165403-5aa21ea2c23a/go.mod h1:YPNKjjE7Ubp9dTbnWvsP3HT+hYnY6TfXzubYTBeUxc8=
github.com/keybase/go-keychain v0.0.1 h1:way+bWYa6lDppZoZcgMbYsvC7GxljxrskdNInRtuthU=
Expand All @@ -15,6 +18,11 @@ github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWb
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/spf13/cobra v1.9.1 h1:CXSaggrXdbHK9CF+8ywj8Amf7PBRmPCOJugH954Nnlo=
github.com/spf13/cobra v1.9.1/go.mod h1:nDyEzZ8ogv936Cinf6g1RU9MRY64Ir93oCnqb9wxYW0=
github.com/spf13/pflag v1.0.6 h1:jFzHGLGAlb3ruxLB8MhbI6A8+AQX/2eW4qeyNZXNp2o=
github.com/spf13/pflag v1.0.6/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA=
Expand Down
12 changes: 12 additions & 0 deletions store/keychain/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,15 @@ func main() {
The `keychain` assumes that any secret stored would conform to the `store.Secret`
interface. This allows the `keychain` to store secrets of any type and leaves
it up to the implementer to decide how they would like their secret parsed.

## Example CLI

The `keychain` package also contains an example CLI tool to test out how a real
application might interact with the host keychain.

You can build the CLI by running `go build` inside the `store/` root directory.

```console
$ go build -o keychain-cli ./keychain/cmd/
$ ./keychain-cli
```
126 changes: 126 additions & 0 deletions store/keychain/cmd/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package main

import (
"context"
"errors"
"fmt"
"log"
"path"

"github.com/docker/secrets-engine/store"
"github.com/docker/secrets-engine/store/keychain"
"github.com/docker/secrets-engine/store/mocks"
"github.com/spf13/cobra"
)

// newCommand creates an example CLI that uses the keychain library
// It supports windows, linux and macOS.
func newCommand() (*cobra.Command, error) {
kc, err := keychain.New(
"io.docker.Secrets",
"docker-example-cli",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

func() *mocks.MockCredential {
return &mocks.MockCredential{}
},
)
if err != nil {
return nil, err
}
list := &cobra.Command{
Use: "list",
Aliases: []string{"ls"},
Comment thread
Benehiko marked this conversation as resolved.
RunE: func(cmd *cobra.Command, args []string) error {
secrets, err := kc.GetAll(cmd.Context())
if errors.Is(err, store.ErrCredentialNotFound) {
fmt.Println("No Secrets found")
return nil
}
if err != nil {
return err
}

var e error
for k, v := range secrets {
vv, err := v.Marshal()
if err != nil {
e = errors.Join(e, err)
}
fmt.Printf("\nID: %s\nValue: %s\n", k, vv)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

[Sensitive data returned by an access to Password](1) flows to a logging call.

Copilot Autofix

AI over 1 year ago

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:

  1. 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.
  2. Update the fmt.Printf statement in store/keychain/cmd/main.go to log only the username and ID, omitting the sensitive value.

Suggested changeset 2
store/keychain/cmd/main.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/store/keychain/cmd/main.go b/store/keychain/cmd/main.go
--- a/store/keychain/cmd/main.go
+++ b/store/keychain/cmd/main.go
@@ -47,3 +47,3 @@
 				}
-				fmt.Printf("\nID: %s\nValue: %s\n", k, vv)
+				fmt.Printf("\nID: %s\nUsername: %s\n", k, string(vv))
 			}
EOF
@@ -47,3 +47,3 @@
}
fmt.Printf("\nID: %s\nValue: %s\n", k, vv)
fmt.Printf("\nID: %s\nUsername: %s\n", k, string(vv))
}
store/mocks/mock_credential.go
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/store/mocks/mock_credential.go b/store/mocks/mock_credential.go
--- a/store/mocks/mock_credential.go
+++ b/store/mocks/mock_credential.go
@@ -18,3 +18,3 @@
 func (m *MockCredential) Marshal() ([]byte, error) {
-	return []byte(m.Username + ":" + m.Password), nil
+	return []byte(m.Username), nil
 }
EOF
@@ -18,3 +18,3 @@
func (m *MockCredential) Marshal() ([]byte, error) {
return []byte(m.Username + ":" + m.Password), nil
return []byte(m.Username), nil
}
Copilot is powered by AI and may make mistakes. Always verify output.
}
return e
},
}

var (
username string
password string
)
save := &cobra.Command{
Use: "store",
Aliases: []string{"set", "save"},
RunE: func(cmd *cobra.Command, args []string) error {
id, err := store.ParseID(path.Join("keystore-cli", username))
if err != nil {
return err
}
creds := &mocks.MockCredential{
Username: username,
Password: password,
}
return kc.Save(cmd.Context(), id, creds)
},
}
save.PersistentFlags().StringVar(&username, "username", "", "The secret key")
save.PersistentFlags().StringVar(&password, "password", "", "The secret value")
save.MarkFlagsRequiredTogether("username", "password")

get := &cobra.Command{
Use: "get",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
id, err := store.ParseID(path.Join("keystore-cli", args[0]))
if err != nil {
return err
}
secret, err := kc.Get(cmd.Context(), id)
if err != nil {
return err
}
val, err := secret.Marshal()
if err != nil {
return err
}
fmt.Printf("Secret:\nID:%s\nValue:%s\n", id.String(), val)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

[Sensitive data returned by an access to Password](1) flows to a logging call.

Copilot Autofix

AI over 1 year ago

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.


Suggested changeset 2
store/keychain/cmd/main.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/store/keychain/cmd/main.go b/store/keychain/cmd/main.go
--- a/store/keychain/cmd/main.go
+++ b/store/keychain/cmd/main.go
@@ -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
EOF
@@ -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
store/mocks/mock_credential.go
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/store/mocks/mock_credential.go b/store/mocks/mock_credential.go
--- a/store/mocks/mock_credential.go
+++ b/store/mocks/mock_credential.go
@@ -18,3 +18,3 @@
 func (m *MockCredential) Marshal() ([]byte, error) {
-	return []byte(m.Username + ":" + m.Password), nil
+	return []byte(m.Username + ":********"), nil
 }
EOF
@@ -18,3 +18,3 @@
func (m *MockCredential) Marshal() ([]byte, error) {
return []byte(m.Username + ":" + m.Password), nil
return []byte(m.Username + ":********"), nil
}
Copilot is powered by AI and may make mistakes. Always verify output.
return nil
},
}

erase := &cobra.Command{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
erase := &cobra.Command{
delete := &cobra.Command{

nit

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Use: "delete",
Args: cobra.ExactArgs(1),
Aliases: []string{"rm", "remove"},
RunE: func(cmd *cobra.Command, args []string) error {
id, err := store.ParseID(path.Join("keystore-cli", args[0]))
if err != nil {
return err
}
return kc.Delete(cmd.Context(), id)
},
}
root := &cobra.Command{}
root.AddCommand(list, save, get, erase)

return root, nil
}

func main() {
Comment thread
Benehiko marked this conversation as resolved.
ctx := context.Background()
cmd, err := newCommand()
if err != nil {
log.Fatalf("could not create CLI: %v", err)
}
cmd.SetContext(ctx)
if err := cmd.Execute(); err != nil {
log.Fatal(err)
}
}
150 changes: 132 additions & 18 deletions store/keychain/keychain_test.go
Original file line number Diff line number Diff line change
@@ -1,50 +1,96 @@
package keychain

import (
"maps"
"errors"
"testing"

"github.com/docker/secrets-engine/store"
"github.com/docker/secrets-engine/store/mocks"
"github.com/stretchr/testify/require"
)

func TestKeychain(t *testing.T) {
ks, err := New("com.test.test", "test",
func() *mocks.MockCredential {
type mustMarshalError struct{}

var _ store.Secret = &mustMarshalError{}

func (m *mustMarshalError) Marshal() ([]byte, error) {
return nil, errors.New("i am failing on purpose")
}

func (m *mustMarshalError) Unmarshal(data []byte) error {
return nil
}

type mustUnmarshalError struct{}

var _ store.Secret = &mustUnmarshalError{}

func (m *mustUnmarshalError) Marshal() ([]byte, error) {
return []byte("eeyyy"), nil
}

func (m *mustUnmarshalError) Unmarshal(data []byte) error {
return errors.New("i am failing on purpose")
}

func setupKeychain(t *testing.T, secretFactory func() store.Secret) store.Store {
t.Helper()
if secretFactory == nil {
secretFactory = func() store.Secret {
return &mocks.MockCredential{}
},
)
}
}

ks, err := New("com.test.test", "test", secretFactory)
require.NoError(t, err)
return ks
}

func TestKeychain(t *testing.T) {
t.Run("save credentials", func(t *testing.T) {
ks := setupKeychain(t, nil)
id := store.ID("com.test.test/test/bob")
require.NoError(t, id.Valid())
creds := &mocks.MockCredential{
Username: "bob",
Password: "bob-password",
}
t.Cleanup(func() {
require.NoError(t, ks.Delete(t.Context(), id))
})
require.NoError(t, ks.Save(t.Context(), id, creds))
})

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

aren't we testing both?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ks := setupKeychain(t, nil)
id := store.ID("com.test.test/test/bob")
creds := &mocks.MockCredential{
Username: "bob",
Password: "bob-password",
}
t.Cleanup(func() {
require.NoError(t, ks.Delete(t.Context(), id))
})
require.NoError(t, ks.Save(t.Context(), id, creds))
require.NoError(t, id.Valid())
secret, err := ks.Get(t.Context(), id)
require.NoError(t, err)

actual, ok := secret.(*mocks.MockCredential)
require.True(t, ok)

expected := &mocks.MockCredential{
Username: "bob",
Password: "bob-password",
}
expected := creds
require.EqualValues(t, expected, actual)
})

t.Run("list all credentials", func(t *testing.T) {
ks := setupKeychain(t, nil)

moreCreds := map[store.ID]*mocks.MockCredential{
"com.test.test/test/bob": {
Username: "bob",
Password: "bob-password",
},
"com.test.test/test/jeff": {
Username: "jeff",
Password: "jeff-password",
Expand All @@ -54,6 +100,11 @@ func TestKeychain(t *testing.T) {
Password: "pete-password",
},
}
t.Cleanup(func() {
for id := range moreCreds {
require.NoError(t, ks.Delete(t.Context(), id))
}
})

for id, anotherCred := range moreCreds {
require.NoError(t, ks.Save(t.Context(), id, anotherCred))
Expand All @@ -67,22 +118,85 @@ func TestKeychain(t *testing.T) {
}
require.Len(t, actual, 3)

expected := map[store.ID]*mocks.MockCredential{
"com.test.test/test/bob": {
Username: "bob",
Password: "bob-password",
},
}
maps.Copy(expected, moreCreds)
require.Len(t, expected, 3)
expected := moreCreds
require.Equal(t, expected, actual)
})

t.Run("delete credential", func(t *testing.T) {
ks := setupKeychain(t, nil)
id := store.ID("com.test.test/test/bob")
require.NoError(t, id.Valid())
creds := &mocks.MockCredential{
Username: "bob",
Password: "bob-password",
}
require.NoError(t, ks.Save(t.Context(), id, creds))
require.NoError(t, ks.Delete(t.Context(), id))
_, err := ks.Get(t.Context(), id)
require.ErrorIs(t, err, store.ErrCredentialNotFound)
})

t.Run("delete non-existent credential", func(t *testing.T) {
ks := setupKeychain(t, nil)
id := store.ID("com.test.test/test/does-not-exist")
require.NoError(t, id.Valid())
require.NoError(t, ks.Delete(t.Context(), id))
})
Comment thread
Benehiko marked this conversation as resolved.

t.Run("invalid ID", func(t *testing.T) {
id := store.ID("completely*&@#$@invalid")
kc := setupKeychain(t, nil)

operations := []string{"save", "get", "delete"}

for _, op := range operations {
t.Run(op, func(t *testing.T) {
var err error
switch op {
case "save":
err = kc.Save(t.Context(), id, nil)
case "delete":
err = kc.Delete(t.Context(), id)
case "get":
_, err = kc.Get(t.Context(), id)
}
require.ErrorContains(t, err, "invalid identifier")
})
}
})

t.Run("marshal error on save", func(t *testing.T) {
kc := setupKeychain(t, nil)
id, err := store.ParseID("something/will/fail")
require.NoError(t, err)
require.ErrorContains(t, kc.Save(t.Context(), id, &mustMarshalError{}), "i am failing on purpose")
})

t.Run("unmarshal error on get", func(t *testing.T) {
kc := setupKeychain(t, func() store.Secret {
return &mustUnmarshalError{}
})
id, err := store.ParseID("something/will/fail")
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, kc.Delete(t.Context(), id))
})
require.NoError(t, kc.Save(t.Context(), id, &mustUnmarshalError{}))
_, err = kc.Get(t.Context(), id)
require.ErrorContains(t, err, "i am failing on purpose")
})

t.Run("unmarshal error on getAll", func(t *testing.T) {
kc := setupKeychain(t, func() store.Secret {
return &mustUnmarshalError{}
})
id, err := store.ParseID("something/will/fail")
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, kc.Delete(t.Context(), id))
})
require.NoError(t, kc.Save(t.Context(), id, &mustUnmarshalError{}))
_, err = kc.GetAll(t.Context())
require.ErrorContains(t, err, "i am failing on purpose")
})
}
Loading