From 8a829820dd62687968c3681323e247c2673254b0 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Fri, 5 Feb 2021 11:10:07 +0100 Subject: [PATCH 1/4] Return error for setting a credential The old behaviour of ignoring the error, resulted in errors during loading a credential (for example, a wrong passphrase), from being ignored. This resulted in getting an "unauthenticated client" error. Creating an unauthenticated client is no longer needed now that the signup process is removed from the SDK and CLI. --- pkg/secrethub/client.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/secrethub/client.go b/pkg/secrethub/client.go index 14bbc2f8..a7cacf8d 100644 --- a/pkg/secrethub/client.go +++ b/pkg/secrethub/client.go @@ -157,10 +157,8 @@ func NewClient(with ...ClientOption) (*Client, error) { } err := client.with(WithCredentials(provider)) - // nolint: staticcheck if err != nil { - // TODO: log that default credential was not loaded. - // Do go on because we want to allow an unauthenticated client. + return nil, err } } From 1a490aa1a0c5f92022fad61b52d5195c75c9c2d6 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Fri, 5 Feb 2021 11:12:29 +0100 Subject: [PATCH 2/4] Remove setting default passphrase reader from client constructor This is no longer necessary because the environment variable is already checked in the decryption logic. By removing the default passphraseReader, it is also easier to determine if a passphrase has been explicitly set by just checking if the passphraseReader is nil. --- pkg/secrethub/client.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/secrethub/client.go b/pkg/secrethub/client.go index a7cacf8d..70b09167 100644 --- a/pkg/secrethub/client.go +++ b/pkg/secrethub/client.go @@ -121,11 +121,11 @@ func (i AppInfo) ValidateName() error { // If no key credential could be found, a Client is returned that can only be used for unauthenticated routes. func NewClient(with ...ClientOption) (*Client, error) { client := &Client{ - httpClient: http.NewClient(), - repoIndexKeys: make(map[api.RepoPath]*crypto.SymmetricKey), - appInfo: []*AppInfo{}, - defaultPassphraseReader: credentials.FromEnv("SECRETHUB_CREDENTIAL_PASSPHRASE"), + httpClient: http.NewClient(), + repoIndexKeys: make(map[api.RepoPath]*crypto.SymmetricKey), + appInfo: []*AppInfo{}, } + err := client.with(with...) if err != nil { return nil, err From 9f74af23d494992c43babf04895da951c1ba4612 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Fri, 5 Feb 2021 11:17:27 +0100 Subject: [PATCH 3/4] Fix "incorrect passphrase" error for env sourced passphrases Without this statement, you'd get an error about the message authentication of the encrypted credential being invalid. That's not really useful. --- pkg/secrethub/credentials/key.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index 23e039d9..4139d8d0 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -85,6 +85,9 @@ func ImportKey(credentialReader, passphraseReader Reader) (Key, error) { if envPassphrase != "" { credential, err := decryptKey([]byte(envPassphrase), encoded) if err != nil { + if crypto.IsWrongKey(err) { + err = ErrCannotDecryptCredential + } return Key{}, fmt.Errorf("decrypting credential with passphrase read from $%s: %v", credentialPassphraseEnvVar, err) } return Key{key: credential}, nil From 429188205d41d0ee9f0f2f7915ee424e3e59de9f Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Fri, 5 Feb 2021 11:32:15 +0100 Subject: [PATCH 4/4] Fix UserAgent test This test created a normal client without having a credential available. By decoupling the tested functionality from NewClient, the credential loading functionality is no longer triggered and this error is avoided. --- pkg/secrethub/client.go | 29 ++++++++++++++++------------- pkg/secrethub/client_test.go | 8 ++++---- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/pkg/secrethub/client.go b/pkg/secrethub/client.go index 70b09167..d175ecec 100644 --- a/pkg/secrethub/client.go +++ b/pkg/secrethub/client.go @@ -162,19 +162,7 @@ func NewClient(with ...ClientOption) (*Client, error) { } } - appName := os.Getenv("SECRETHUB_APP_INFO_NAME") - if appName != "" { - appVersion := os.Getenv("SECRETHUB_APP_INFO_VERSION") - topLevelAppInfo := &AppInfo{ - Name: appName, - Version: appVersion, - } - // Ignore app info from environment variable if name is invalid - if err = topLevelAppInfo.ValidateName(); err == nil { - client.appInfo = append(client.appInfo, topLevelAppInfo) - } - } - + client.loadAppInfoFromEnv() userAgent := client.userAgent() client.httpClient.Options(http.WithUserAgent(userAgent)) @@ -286,6 +274,21 @@ func (c *Client) isKeyed() bool { return c.decrypter != nil } +func (c *Client) loadAppInfoFromEnv() { + appName := os.Getenv("SECRETHUB_APP_INFO_NAME") + if appName != "" { + appVersion := os.Getenv("SECRETHUB_APP_INFO_VERSION") + topLevelAppInfo := &AppInfo{ + Name: appName, + Version: appVersion, + } + // Ignore app info from environment variable if name is invalid + if err := topLevelAppInfo.ValidateName(); err == nil { + c.appInfo = append(c.appInfo, topLevelAppInfo) + } + } +} + func (c *Client) userAgent() string { userAgent := userAgentPrefix for _, info := range c.appInfo { diff --git a/pkg/secrethub/client_test.go b/pkg/secrethub/client_test.go index 3b924356..a31477a9 100644 --- a/pkg/secrethub/client_test.go +++ b/pkg/secrethub/client_test.go @@ -62,11 +62,11 @@ func TestClient_userAgent(t *testing.T) { for _, info := range tc.appInfo { opts = append(opts, WithAppInfo(info)) } - client, err := NewClient(opts...) + client := &Client{} + err := client.with(opts...) assert.Equal(t, err, tc.err) - if err != nil { - return - } + + client.loadAppInfoFromEnv() userAgent := client.userAgent() pattern := tc.expected + " \\(.*\\)"