From ee7017df6af926f87f2f6bd663ae45c99cb4ed1d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 27 Feb 2025 13:43:54 +0100 Subject: [PATCH 1/5] secretservice: fix SecretService.PromptAndWait discarding error This code was always returning a nil error instead of the error produced by the org.freedesktop.Secret.Prompt.Prompt call. Signed-off-by: Sebastiaan van Stijn --- secretservice/secretservice.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/secretservice/secretservice.go b/secretservice/secretservice.go index 3696fc0..3a8a32b 100644 --- a/secretservice/secretservice.go +++ b/secretservice/secretservice.go @@ -342,7 +342,7 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia } call := s.Obj(prompt).Call("org.freedesktop.Secret.Prompt.Prompt", NilFlags, "Keyring Prompt") if call.Err != nil { - return nil, errors.Wrap(err, "failed to prompt") + return nil, errors.Wrap(call.Err, "failed to prompt") } for { var result PromptCompletedResult From 14785d606567bf5385df8c83b3f9a1e728fc6682 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 27 Feb 2025 13:55:00 +0100 Subject: [PATCH 2/5] secretservice: SecretService.openSessionRaw explicitly handle error pkg/errors' errors.Wrap function implicitly discards nil-errors. While this is convenient, it also can be err-prone, as this behavior differs from go stdlib, making it easy to miss conditions where the reader assumes an error is returned (but in reality no error). This patch updates the code to explicitly handle non-nil errors to prevent accidental regressions if this code would be rewritten using go stdlib. Signed-off-by: Sebastiaan van Stijn --- secretservice/secretservice.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/secretservice/secretservice.go b/secretservice/secretservice.go index 3a8a32b..9c1d672 100644 --- a/secretservice/secretservice.go +++ b/secretservice/secretservice.go @@ -101,7 +101,10 @@ func (s *SecretService) openSessionRaw(mode AuthenticationMode, sessionAlgorithm err = s.ServiceObj(). Call("org.freedesktop.Secret.Service.OpenSession", NilFlags, mode, sessionAlgorithmInput). Store(&resp.algorithmOutput, &resp.path) - return resp, errors.Wrap(err, "failed to open secretservice session") + if err != nil { + return sessionOpenResponse{}, errors.Wrap(err, "failed to open secretservice session") + } + return resp, nil } // OpenSession From b2579cb52fbc13379a88c86ff892788e1ac45935 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 27 Feb 2025 13:57:35 +0100 Subject: [PATCH 3/5] secretservice: rename var that shadowed type Signed-off-by: Sebastiaan van Stijn --- secretservice/secretservice.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/secretservice/secretservice.go b/secretservice/secretservice.go index 9c1d672..ec26356 100644 --- a/secretservice/secretservice.go +++ b/secretservice/secretservice.go @@ -134,11 +134,11 @@ func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, sessionOpenCh := make(chan sessionOpenResponse) errCh := make(chan error) go func() { - sessionOpenResponse, err := s.openSessionRaw(mode, sessionAlgorithmInput) + resp, err := s.openSessionRaw(mode, sessionAlgorithmInput) if err != nil { errCh <- err } else { - sessionOpenCh <- sessionOpenResponse + sessionOpenCh <- resp } }() From 48bf12d4e29f518a0a95093990b0b22fb9bd4726 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 27 Feb 2025 13:58:51 +0100 Subject: [PATCH 4/5] secretservice: fix GoDoc comment Signed-off-by: Sebastiaan van Stijn --- secretservice/secretservice.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/secretservice/secretservice.go b/secretservice/secretservice.go index ec26356..1963f60 100644 --- a/secretservice/secretservice.go +++ b/secretservice/secretservice.go @@ -185,7 +185,7 @@ func (s *SecretService) CloseSession(session *Session) { s.Obj(session.Path).Call("org.freedesktop.Secret.Session.Close", NilFlags) } -// SearchColleciton +// SearchCollection func (s *SecretService) SearchCollection(collection dbus.ObjectPath, attributes Attributes) (items []dbus.ObjectPath, err error) { err = s.Obj(collection). Call("org.freedesktop.Secret.Collection.SearchItems", NilFlags, attributes). From a97d349175a82fd08ab2d241736a142b98615c4c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 27 Feb 2025 13:33:34 +0100 Subject: [PATCH 5/5] secretservice: replace pkg/errors for Go stdlib errors The pkg/errors dependency was introduced with the secretservice implementation in 7f2ef9fddce668397ae2791cde4cadd6136c1fbd in March 2019. go1.13 (September 2019) introduced native support for unwrapping errors, no longer requiring this dependency to be used, and the pkg/errors module has been archived (as feature complete). While pkg/errors does have some advantages (for example, it can provide a stack trace), this functionality doesn't appear to be used in this module, and the pkg/errors package is not used in other implementations (for macOS). This patch removes the dependency, replacing its use for the equivalent in Go stdlib. Signed-off-by: Sebastiaan van Stijn --- go.mod | 1 - go.sum | 2 - .../dh_ietf1024_sha256_aes128_cbc_pkcs7.go | 2 +- secretservice/secretservice.go | 47 ++++++++++--------- 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index 33f95a1..cbaa2fe 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,6 @@ go 1.21 require ( github.com/keybase/dbus v0.0.0-20220506165403-5aa21ea2c23a - github.com/pkg/errors v0.9.1 github.com/stretchr/testify v1.10.0 golang.org/x/crypto v0.32.0 ) diff --git a/go.sum b/go.sum index 0c6c2f2..53611f3 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,6 @@ 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/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/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= -github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= 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/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= diff --git a/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go b/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go index 21a4f05..77ef893 100644 --- a/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go +++ b/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go @@ -17,11 +17,11 @@ import ( "crypto/cipher" cryptorand "crypto/rand" "crypto/sha256" + "errors" "fmt" "io" "math/big" - errors "github.com/pkg/errors" "golang.org/x/crypto/hkdf" ) diff --git a/secretservice/secretservice.go b/secretservice/secretservice.go index 1963f60..f7f1836 100644 --- a/secretservice/secretservice.go +++ b/secretservice/secretservice.go @@ -1,11 +1,12 @@ package secretservice import ( + "errors" + "fmt" "math/big" "time" dbus "github.com/keybase/dbus" - errors "github.com/pkg/errors" ) // SecretServiceInterface @@ -69,7 +70,7 @@ const DefaultSessionOpenTimeout = 10 * time.Second func NewService() (*SecretService, error) { conn, err := dbus.ConnectSessionBus() if err != nil { - return nil, errors.Wrap(err, "failed to open dbus connection") + return nil, fmt.Errorf("failed to open dbus connection: %w", err) } signalCh := make(chan *dbus.Signal, 16) conn.Signal(signalCh) @@ -102,7 +103,7 @@ func (s *SecretService) openSessionRaw(mode AuthenticationMode, sessionAlgorithm Call("org.freedesktop.Secret.Service.OpenSession", NilFlags, mode, sessionAlgorithmInput). Store(&resp.algorithmOutput, &resp.path) if err != nil { - return sessionOpenResponse{}, errors.Wrap(err, "failed to open secretservice session") + return sessionOpenResponse{}, fmt.Errorf("failed to open secretservice session: %w", err) } return resp, nil } @@ -128,7 +129,7 @@ func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, session.Public = public sessionAlgorithmInput = dbus.MakeVariant(public.Bytes()) // math/big.Int.Bytes is big endian default: - return nil, errors.Errorf("unknown authentication mode %v", mode) + return nil, fmt.Errorf("unknown authentication mode %v", mode) } sessionOpenCh := make(chan sessionOpenResponse) @@ -155,7 +156,7 @@ func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, case err := <-errCh: return nil, err case <-time.After(s.sessionOpenTimeout): - return nil, errors.Errorf("timed out after %s", s.sessionOpenTimeout) + return nil, fmt.Errorf("timed out after %s", s.sessionOpenTimeout) } switch mode { @@ -163,7 +164,7 @@ func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, case AuthenticationDHAES: theirPublicBigEndian, ok := sessionAlgorithmOutput.Value().([]byte) if !ok { - return nil, errors.Errorf("failed to coerce algorithm output value to byteslice") + return nil, errors.New("failed to coerce algorithm output value to byteslice") } group := rfc2409SecondOakleyGroup() theirPublic := new(big.Int) @@ -174,7 +175,7 @@ func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, } session.AESKey = aesKey default: - return nil, errors.Errorf("unknown authentication mode %v", mode) + return nil, fmt.Errorf("unknown authentication mode %v", mode) } return session, nil @@ -191,7 +192,7 @@ func (s *SecretService) SearchCollection(collection dbus.ObjectPath, attributes Call("org.freedesktop.Secret.Collection.SearchItems", NilFlags, attributes). Store(&items) if err != nil { - return nil, errors.Wrap(err, "failed to search collection") + return nil, fmt.Errorf("failed to search collection: %w", err) } return items, nil } @@ -214,7 +215,7 @@ func (s *SecretService) CreateItem(collection dbus.ObjectPath, properties map[st case ReplaceBehaviorReplace: replace = true default: - return "", errors.Errorf("unknown replace behavior %v", replaceBehavior) + return "", fmt.Errorf("unknown replace behavior %d", replaceBehavior) } var prompt dbus.ObjectPath @@ -222,7 +223,7 @@ func (s *SecretService) CreateItem(collection dbus.ObjectPath, properties map[st Call("org.freedesktop.Secret.Collection.CreateItem", NilFlags, properties, secret, replace). Store(&item, &prompt) if err != nil { - return "", errors.Wrap(err, "failed to create item") + return "", fmt.Errorf("failed to create item: %w", err) } _, err = s.PromptAndWait(prompt) if err != nil { @@ -238,7 +239,7 @@ func (s *SecretService) DeleteItem(item dbus.ObjectPath) (err error) { Call("org.freedesktop.Secret.Item.Delete", NilFlags). Store(&prompt) if err != nil { - return errors.Wrap(err, "failed to delete item") + return fmt.Errorf("failed to delete item: %w", err) } _, err = s.PromptAndWait(prompt) if err != nil { @@ -251,11 +252,11 @@ func (s *SecretService) DeleteItem(item dbus.ObjectPath) (err error) { func (s *SecretService) GetAttributes(item dbus.ObjectPath) (attributes Attributes, err error) { attributesV, err := s.Obj(item).GetProperty("org.freedesktop.Secret.Item.Attributes") if err != nil { - return nil, errors.Wrap(err, "failed to get attributes") + return nil, fmt.Errorf("failed to get attributes: %w", err) } attributesMap, ok := attributesV.Value().(map[string]string) if !ok { - return nil, errors.Errorf("failed to coerce item attributes") + return nil, errors.New("failed to coerce item attributes") } return Attributes(attributesMap), nil } @@ -267,12 +268,12 @@ func (s *SecretService) GetSecret(item dbus.ObjectPath, session Session) (secret Call("org.freedesktop.Secret.Item.GetSecret", NilFlags, session.Path). Store(&secretI) if err != nil { - return nil, errors.Wrap(err, "failed to get secret") + return nil, fmt.Errorf("failed to get secret: %w", err) } secret := new(Secret) err = dbus.Store(secretI, &secret.Session, &secret.Parameters, &secret.Value, &secret.ContentType) if err != nil { - return nil, errors.Wrap(err, "failed to unmarshal get secret result") + return nil, fmt.Errorf("failed to unmarshal get secret result: %w", err) } switch session.Mode { @@ -285,7 +286,7 @@ func (s *SecretService) GetSecret(item dbus.ObjectPath, session Session) (secret } secretPlaintext = plaintext default: - return nil, errors.Errorf("cannot make secret for authentication mode %v", session.Mode) + return nil, fmt.Errorf("cannot make secret for authentication mode %v", session.Mode) } return secretPlaintext, nil @@ -302,11 +303,11 @@ func (s *SecretService) Unlock(items []dbus.ObjectPath) (err error) { Call("org.freedesktop.Secret.Service.Unlock", NilFlags, items). Store(&dummy, &prompt) if err != nil { - return errors.Wrap(err, "failed to unlock items") + return fmt.Errorf("failed to unlock items: %w", err) } _, err = s.PromptAndWait(prompt) if err != nil { - return errors.Wrap(err, "failed to prompt") + return fmt.Errorf("failed to prompt: %w", err) } return nil } @@ -319,11 +320,11 @@ func (s *SecretService) LockItems(items []dbus.ObjectPath) (err error) { Call("org.freedesktop.Secret.Service.Lock", NilFlags, items). Store(&dummy, &prompt) if err != nil { - return errors.Wrap(err, "failed to lock items") + return fmt.Errorf("failed to lock items: %w", err) } _, err = s.PromptAndWait(prompt) if err != nil { - return errors.Wrap(err, "failed to prompt") + return fmt.Errorf("failed to prompt: %w", err) } return nil } @@ -345,7 +346,7 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia } call := s.Obj(prompt).Call("org.freedesktop.Secret.Prompt.Prompt", NilFlags, "Keyring Prompt") if call.Err != nil { - return nil, errors.Wrap(call.Err, "failed to prompt") + return nil, fmt.Errorf("failed to prompt: %w", call.Err) } for { var result PromptCompletedResult @@ -362,7 +363,7 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia } err = dbus.Store(signal.Body, &result.Dismissed, &result.Paths) if err != nil { - return nil, errors.Wrap(err, "failed to unmarshal prompt result") + return nil, fmt.Errorf("failed to unmarshal prompt result: %w", err) } if result.Dismissed { return nil, PromptDismissedError{errors.New("prompt dismissed")} @@ -404,6 +405,6 @@ func (session *Session) NewSecret(secretBytes []byte) (Secret, error) { ContentType: "application/octet-stream", }, nil default: - return Secret{}, errors.Errorf("cannot make secret for authentication mode %v", session.Mode) + return Secret{}, fmt.Errorf("cannot make secret for authentication mode %v", session.Mode) } }