diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index 1381785a..25538245 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -107,6 +107,53 @@ func (s *SecretService) Obj(path dbus.ObjectPath) dbus.BusObject { return s.conn.Object(SecretServiceInterface, path) } +// dbus interface members used by the high-level helpers below. +// +// https://specifications.freedesktop.org/secret-service-spec/latest/index.html +const ( + collectionsProperty = "org.freedesktop.Secret.Service.Collections" + readAliasMethod = "org.freedesktop.Secret.Service.ReadAlias" + collectionLockedProp = "org.freedesktop.Secret.Collection.Locked" +) + +// Collections returns the object paths of every collection known to the secret +// service. +func (s *SecretService) Collections() ([]dbus.ObjectPath, error) { + variant, err := s.ServiceObj().GetProperty(collectionsProperty) + if err != nil { + return nil, err + } + collections, ok := variant.Value().([]dbus.ObjectPath) + if !ok { + return nil, errors.New("could not list keychain collections") + } + return collections, nil +} + +// ReadAlias resolves an alias (e.g. "default") to the collection object path it +// points at. The secret service returns the null path "/" when the alias is not +// assigned to any collection. +func (s *SecretService) ReadAlias(alias string) (dbus.ObjectPath, error) { + var path dbus.ObjectPath + if err := s.ServiceObj().Call(readAliasMethod, NilFlags, alias).Store(&path); err != nil { + return "", err + } + return path, nil +} + +// IsLocked reports whether the given collection is currently locked. +func (s *SecretService) IsLocked(collection dbus.ObjectPath) (bool, error) { + variant, err := s.Obj(collection).GetProperty(collectionLockedProp) + if err != nil { + return false, err + } + locked, ok := variant.Value().(bool) + if !ok { + return false, errors.New("unexpected type for collection locked property") + } + return locked, nil +} + type sessionOpenResponse struct { algorithmOutput dbus.Variant path dbus.ObjectPath diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index cfa89161..9d1498ca 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -45,22 +45,35 @@ const ( // // https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html#org.freedesktop.Secret.Service.ReadAlias nullObjectPath = dbus.ObjectPath("/") +) - // used to list all available collections on the secret service API - // - // https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html - secretServiceCollectionProperty = "org.freedesktop.Secret.Service.Collections" +// secretService is the subset of [kc.SecretService] the keychain store depends +// on. It exists so the store can be unit tested against a fake implementation +// without a live secret service over dbus — see the fake in the linux tests. +// +// Every method maps to a method on [kc.SecretService]; none expose a +// dbus.BusObject, so a fake never needs to talk to the bus. +type secretService interface { + Collections() ([]dbus.ObjectPath, error) + ReadAlias(alias string) (dbus.ObjectPath, error) + IsLocked(collection dbus.ObjectPath) (bool, error) + OpenSession(mode kc.AuthenticationMode) (*kc.Session, error) + CloseSession(session *kc.Session) + Unlock(items []dbus.ObjectPath) error + SearchCollection(collection dbus.ObjectPath, attributes kc.Attributes) ([]dbus.ObjectPath, error) + CreateItem(collection dbus.ObjectPath, properties map[string]dbus.Variant, secret kc.Secret, replaceBehavior kc.ReplaceBehavior) (dbus.ObjectPath, error) + DeleteItem(item dbus.ObjectPath) error + GetAttributes(item dbus.ObjectPath) (kc.Attributes, error) + GetSecret(item dbus.ObjectPath, session kc.Session) ([]byte, error) + Close() error +} - // used to get the dbus object path of an aliased collection - // An common alias would be 'default' - // https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html - secretServiceGetAliasObjectPath = "org.freedesktop.Secret.Service.ReadAlias" +// the concrete secret service must satisfy the interface the store depends on. +var _ secretService = (*kc.SecretService)(nil) - // used to check if the collection is locked - // - // https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Collection.html - secretServiceIsCollectionLockedProperty = "org.freedesktop.Secret.Collection.Locked" -) +// newService dials a fresh secret service. It is a package var so tests can +// substitute a fake; production always returns a real [kc.SecretService]. +var newService = func() (secretService, error) { return kc.NewService() } // getDefaultCollection gets the secret service collection dbus object path. // @@ -70,24 +83,17 @@ const ( // As a fallback it queries the secret service for the default collection. // It is possible that the host does not have a collection set up, in that case // the only option is to error. -func getDefaultCollection(service *kc.SecretService) (dbus.ObjectPath, error) { - variant, err := service.ServiceObj().GetProperty(secretServiceCollectionProperty) +func getDefaultCollection(service secretService) (dbus.ObjectPath, error) { + collections, err := service.Collections() if err != nil { return "", err } - collections, ok := variant.Value().([]dbus.ObjectPath) - if !ok { - return "", errors.New("could not list keychain collections") - } // choose the 'login' collection if it exists if slices.Contains(collections, loginKeychainObjectPath) { return loginKeychainObjectPath, nil } // we need to fallback to the default collection - var defaultKeychainObjectPath dbus.ObjectPath - err = service.ServiceObj(). - Call(secretServiceGetAliasObjectPath, 0, "default"). - Store(&defaultKeychainObjectPath) + defaultKeychainObjectPath, err := service.ReadAlias("default") if err != nil { return "", err } @@ -128,12 +134,12 @@ var errCollectionLocked = errors.New("collection is locked") // // It returns the errCollectionLocked error by default if the collection is locked. // On any other error, it returns the underlying error instead. -func isCollectionUnlocked(collectionPath dbus.ObjectPath, service *kc.SecretService) error { - variant, err := service.Obj(collectionPath).GetProperty(secretServiceIsCollectionLockedProperty) +func isCollectionUnlocked(collectionPath dbus.ObjectPath, service secretService) error { + locked, err := service.IsLocked(collectionPath) if err != nil { return err } - if locked, ok := variant.Value().(bool); ok && !locked { + if !locked { return nil } return errCollectionLocked @@ -146,7 +152,7 @@ type keychainStore[T store.Secret] struct { } func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error { - service, err := kc.NewService() + service, err := newService() if err != nil { return err } @@ -193,7 +199,7 @@ func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error { } func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, error) { - service, err := kc.NewService() + service, err := newService() if err != nil { return nil, err } @@ -260,7 +266,7 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, } func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]store.Secret, error) { - service, err := kc.NewService() + service, err := newService() if err != nil { return nil, err } @@ -332,7 +338,7 @@ func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]sto } func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Secret) error { - service, err := kc.NewService() + service, err := newService() if err != nil { return err } @@ -395,7 +401,7 @@ func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store // loadSecret fetches the raw secret value for itemPath, zeroes it after use, // and returns a fully populated Secret. -func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc *kc.SecretService, itemPath dbus.ObjectPath, session *kc.Session, attributes map[string]string) (store.Secret, error) { +func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc secretService, itemPath dbus.ObjectPath, session *kc.Session, attributes map[string]string) (store.Secret, error) { value, err := svc.GetSecret(itemPath, *session) if err != nil { return nil, err @@ -413,7 +419,7 @@ func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc *kc. //gocyclo:ignore func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { - service, err := kc.NewService() + service, err := newService() if err != nil { return nil, err } diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index d9aff5a7..d6ab98be 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -17,13 +17,105 @@ package keychain import ( + "sync/atomic" "testing" dbus "github.com/godbus/dbus/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/docker/secrets-engine/store" + kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain/secretservice" ) +// fakeService is a pure in-memory [secretService]. It never talks to a real +// secret service over dbus, so tests that drive the store through it are +// deterministic and need no keyring on the host. +// +// It records how many connections were opened and closed so a test can assert +// the store balances every open with a Close. The counters are atomic so the +// fake is safe to share across concurrent operations. +type fakeService struct { + // items is returned verbatim by SearchCollection; leave empty to drive the + // "credential not found" path. + items []dbus.ObjectPath + + opened atomic.Int64 + closed atomic.Int64 +} + +func (f *fakeService) Collections() ([]dbus.ObjectPath, error) { + return []dbus.ObjectPath{loginKeychainObjectPath}, nil +} +func (f *fakeService) ReadAlias(string) (dbus.ObjectPath, error) { return loginKeychainObjectPath, nil } +func (f *fakeService) IsLocked(dbus.ObjectPath) (bool, error) { return false, nil } +func (f *fakeService) OpenSession(kc.AuthenticationMode) (*kc.Session, error) { + return &kc.Session{}, nil +} +func (f *fakeService) CloseSession(*kc.Session) {} +func (f *fakeService) Unlock([]dbus.ObjectPath) error { return nil } +func (f *fakeService) SearchCollection(dbus.ObjectPath, kc.Attributes) ([]dbus.ObjectPath, error) { + return f.items, nil +} + +func (f *fakeService) CreateItem(dbus.ObjectPath, map[string]dbus.Variant, kc.Secret, kc.ReplaceBehavior) (dbus.ObjectPath, error) { + return "", nil +} +func (f *fakeService) DeleteItem(dbus.ObjectPath) error { return nil } +func (f *fakeService) GetAttributes(dbus.ObjectPath) (kc.Attributes, error) { return nil, nil } +func (f *fakeService) GetSecret(dbus.ObjectPath, kc.Session) ([]byte, error) { return nil, nil } +func (f *fakeService) Close() error { + f.closed.Add(1) + return nil +} + +// withFakeService swaps the package newService seam for one that hands out the +// given fake (counting each open) and restores the original on cleanup. +func withFakeService(t *testing.T, fake *fakeService) { + t.Helper() + orig := newService + t.Cleanup(func() { newService = orig }) + newService = func() (secretService, error) { + fake.opened.Add(1) + return fake, nil + } +} + +// TestKeychainGetNotFound exercises the full Get path against the fake — open, +// resolve collection, search — and asserts an empty search maps to +// ErrCredentialNotFound, all without a live keyring. +func TestKeychainGetNotFound(t *testing.T) { + fake := &fakeService{} // no items -> not found + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + _, err := ks.Get(t.Context(), store.MustParseID("com.test.test/test/missing")) + assert.ErrorIs(t, err, store.ErrCredentialNotFound) +} + +// TestKeychainClosesEveryConnection is a deterministic regression test for the +// D-Bus connection leak: each keychain operation dials a fresh connection via +// newService and must Close it. Driving the store through a fake lets us assert +// the contract directly — every opened connection is closed — instead of +// counting host file descriptors. +func TestKeychainClosesEveryConnection(t *testing.T) { + fake := &fakeService{} // no items -> Get returns ErrCredentialNotFound + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + missing := store.MustParseID("com.test.test/test/missing") + + const iterations = 30 + for range iterations { + _, err := ks.Get(t.Context(), missing) + require.ErrorIs(t, err, store.ErrCredentialNotFound) + } + + opened, closed := fake.opened.Load(), fake.closed.Load() + require.Equal(t, int64(iterations), opened) + assert.Equal(t, opened, closed, "every opened connection must be closed") +} + func TestResolveDefaultCollection(t *testing.T) { const customCollection = dbus.ObjectPath("/org/freedesktop/secrets/collection/custom")