From b6966241abf732f2994754ef821214e36820c784 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Sun, 7 Jun 2020 23:55:02 +0200 Subject: [PATCH 01/46] Update create user and create service requests to include account key --- pkg/secrethub/account.go | 8 +++++++- pkg/secrethub/service.go | 11 +++-------- pkg/secrethub/user.go | 9 +-------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/pkg/secrethub/account.go b/pkg/secrethub/account.go index bcd67c2c..a4f5be5b 100644 --- a/pkg/secrethub/account.go +++ b/pkg/secrethub/account.go @@ -71,17 +71,23 @@ func (c *Client) createAccountKeyRequest(encrypter credentials.Encrypter, accoun }, nil } -func (c *Client) createCredentialRequest(verifier credentials.Verifier, metadata map[string]string) (*api.CreateCredentialRequest, error) { +func (c *Client) createCredentialRequest(encrypter credentials.Encrypter, accountKey crypto.RSAPrivateKey, verifier credentials.Verifier, metadata map[string]string) (*api.CreateCredentialRequest, error) { bytes, fingerprint, err := verifier.Export() if err != nil { return nil, errio.Error(err) } + accountKeyReq, err := c.createAccountKeyRequest(encrypter, accountKey) + if err != nil { + return nil, err + } + req := api.CreateCredentialRequest{ Fingerprint: fingerprint, Verifier: bytes, Type: verifier.Type(), Metadata: metadata, + AccountKey: accountKeyReq, } err = verifier.AddProof(&req) if err != nil { diff --git a/pkg/secrethub/service.go b/pkg/secrethub/service.go index 0e32d391..b76f4c0b 100644 --- a/pkg/secrethub/service.go +++ b/pkg/secrethub/service.go @@ -54,17 +54,12 @@ func (s serviceService) Create(path string, description string, credentialCreato return nil, errio.Error(err) } - credentialRequest, err := s.client.createCredentialRequest(credentialCreator.Verifier(), credentialCreator.Metadata()) + credentialRequest, err := s.client.createCredentialRequest(credentialCreator.Encrypter(), accountKey, credentialCreator.Verifier(), credentialCreator.Metadata()) if err != nil { return nil, errio.Error(err) } - accountKeyRequest, err := s.client.createAccountKeyRequest(credentialCreator.Encrypter(), accountKey) - if err != nil { - return nil, errio.Error(err) - } - - serviceRepoMemberRequest, err := s.client.createRepoMemberRequest(repoPath, accountKeyRequest.PublicKey) + serviceRepoMemberRequest, err := s.client.createRepoMemberRequest(repoPath, credentialRequest.AccountKey.PublicKey) if err != nil { return nil, errio.Error(err) } @@ -72,7 +67,7 @@ func (s serviceService) Create(path string, description string, credentialCreato in := &api.CreateServiceRequest{ Description: description, Credential: credentialRequest, - AccountKey: accountKeyRequest, + AccountKey: credentialRequest.AccountKey, RepoMember: serviceRepoMemberRequest, } diff --git a/pkg/secrethub/user.go b/pkg/secrethub/user.go index 887f5a6a..ffd28882 100644 --- a/pkg/secrethub/user.go +++ b/pkg/secrethub/user.go @@ -63,7 +63,7 @@ func (s userService) Create(username, email, fullName string, credentials creden } func (s userService) create(username, email, fullName string, accountKey crypto.RSAPrivateKey, verifier credentials.Verifier, encrypter credentials.Encrypter, metadata map[string]string, credentials credentials.Provider) (*api.User, error) { - credentialRequest, err := s.client.createCredentialRequest(verifier, metadata) + credentialRequest, err := s.client.createCredentialRequest(encrypter, accountKey, verifier, metadata) if err != nil { return nil, errio.Error(err) } @@ -91,13 +91,6 @@ func (s userService) create(username, email, fullName string, accountKey crypto. return nil, err } - accountKeyResponse, err := s.client.createAccountKey(credentialRequest.Fingerprint, accountKey, encrypter) - if err != nil { - return nil, err - } - - user.PublicKey = accountKeyResponse.PublicKey - return user, nil } From d290c7ff476d9f7f4757a21f6e46abbeada10b0f Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Tue, 30 Jun 2020 17:21:02 +0200 Subject: [PATCH 02/46] Remove redundant account key from create service request --- pkg/secrethub/service.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/secrethub/service.go b/pkg/secrethub/service.go index b76f4c0b..150dd471 100644 --- a/pkg/secrethub/service.go +++ b/pkg/secrethub/service.go @@ -67,7 +67,6 @@ func (s serviceService) Create(path string, description string, credentialCreato in := &api.CreateServiceRequest{ Description: description, Credential: credentialRequest, - AccountKey: credentialRequest.AccountKey, RepoMember: serviceRepoMemberRequest, } From b0624182e05c3de3289fa779b6ae9344d34dc607 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 7 Jul 2020 12:54:34 +0200 Subject: [PATCH 03/46] Adjust signup test to test the new behavior --- pkg/secrethub/user_test.go | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/pkg/secrethub/user_test.go b/pkg/secrethub/user_test.go index 1898db25..ea2dd6bd 100644 --- a/pkg/secrethub/user_test.go +++ b/pkg/secrethub/user_test.go @@ -2,7 +2,6 @@ package secrethub import ( "encoding/json" - "fmt" "net/http" "testing" "time" @@ -31,6 +30,12 @@ func TestSignup(t *testing.T) { client: Must(NewClient(opts...)), } + accountKey, err := crypto.GenerateRSAPrivateKey(512) + assert.OK(t, err) + + publicAccountKey, err := accountKey.Public().Encode() + assert.OK(t, err) + expectedCreateUserRequest := api.CreateUserRequest{ Username: username, FullName: fullName, @@ -40,12 +45,17 @@ func TestSignup(t *testing.T) { Fingerprint: cred1Fingerprint, Verifier: cred1Verifier, Proof: &api.CredentialProofKey{}, + AccountKey: &api.CreateAccountKeyRequest{ + PublicKey: publicAccountKey, + }, }, } + now := time.Now().UTC() expectedResponse := &api.User{ AccountID: uuid.New(), + PublicKey: publicAccountKey, Username: username, FullName: fullName, Email: email, @@ -60,30 +70,9 @@ func TestSignup(t *testing.T) { assert.OK(t, req.Validate()) - assert.Equal(t, req, expectedCreateUserRequest) - - // Respond - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _ = json.NewEncoder(w).Encode(expectedResponse) - }) - - accountKey, err := crypto.GenerateRSAPrivateKey(512) - assert.OK(t, err) - - publicAccountKey, err := accountKey.Public().Encode() - assert.OK(t, err) - - router.Post(fmt.Sprintf("/me/credentials/%s/key", cred1Fingerprint), func(w http.ResponseWriter, r *http.Request) { - // Assert - req := new(api.CreateAccountKeyRequest) - err := json.NewDecoder(r.Body).Decode(&req) - assert.OK(t, err) - - assert.OK(t, req.Validate()) - // We cannot predict the output of the encrypted key, therefore we do not test it here. - assert.Equal(t, req.PublicKey, publicAccountKey) + req.Credential.AccountKey.EncryptedPrivateKey = nil + assert.Equal(t, req, expectedCreateUserRequest) // Respond w.Header().Set("Content-Type", "application/json") From 3fffe24a5d86961dc7da7c5f2f02fb6133e1b1e1 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 7 Jul 2020 12:55:20 +0200 Subject: [PATCH 04/46] gofmt --- pkg/secrethub/user_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/secrethub/user_test.go b/pkg/secrethub/user_test.go index ea2dd6bd..fdf07ce6 100644 --- a/pkg/secrethub/user_test.go +++ b/pkg/secrethub/user_test.go @@ -51,7 +51,6 @@ func TestSignup(t *testing.T) { }, } - now := time.Now().UTC() expectedResponse := &api.User{ AccountID: uuid.New(), From 0fd982e683b1ac2852949038cf29fb6abc95e37d Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 7 Jul 2020 18:38:07 +0200 Subject: [PATCH 05/46] Remove account key field from create service request The account key creation request is embeded in the credential creation request instead. --- internals/api/service.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internals/api/service.go b/internals/api/service.go index b985f862..e81e6500 100644 --- a/internals/api/service.go +++ b/internals/api/service.go @@ -66,7 +66,6 @@ func (a Service) ToAuditActor() *AuditActor { type CreateServiceRequest struct { Description string `json:"description"` Credential *CreateCredentialRequest `json:"credential"` - AccountKey *CreateAccountKeyRequest `json:"account_key"` RepoMember *CreateRepoMemberRequest `json:"repo_member"` } @@ -83,13 +82,6 @@ func (req CreateServiceRequest) Validate() error { return err } - if req.AccountKey == nil { - return ErrMissingField("account_key") - } - if err := req.AccountKey.Validate(); err != nil { - return err - } - if req.RepoMember == nil { return ErrMissingField("repo_member") } From 3bd2c465bfcb8a1b4a496b8dd2b34a41b782e1fc Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Mon, 13 Jul 2020 19:26:31 +0200 Subject: [PATCH 06/46] Enforce that account key is present on create credential request validation --- internals/api/credential.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internals/api/credential.go b/internals/api/credential.go index 96419658..ca4e1c8c 100644 --- a/internals/api/credential.go +++ b/internals/api/credential.go @@ -162,6 +162,13 @@ func (req *CreateCredentialRequest) Validate() error { } } + if req.AccountKey == nil { + return ErrMissingField("account_key") + } + if err := req.AccountKey.Validate(); err != nil { + return err + } + expectedMetadata, validCredentialType := credentialTypesMetadata[req.Type] if !validCredentialType { return ErrInvalidCredentialType @@ -184,12 +191,6 @@ func (req *CreateCredentialRequest) Validate() error { } } - if req.AccountKey != nil { - if err := req.AccountKey.Validate(); err != nil { - return err - } - } - if validator, ok := req.Proof.(validator); ok { if err := validator.Validate(); err != nil { return err From 05867389d39013b0e49e982115f68adec33d007b Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Mon, 13 Jul 2020 19:38:01 +0200 Subject: [PATCH 07/46] Revert "Enforce that account key is present on create credential request validation" This reverts commit 3bd2c465bfcb8a1b4a496b8dd2b34a41b782e1fc. --- internals/api/credential.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/internals/api/credential.go b/internals/api/credential.go index ca4e1c8c..96419658 100644 --- a/internals/api/credential.go +++ b/internals/api/credential.go @@ -162,13 +162,6 @@ func (req *CreateCredentialRequest) Validate() error { } } - if req.AccountKey == nil { - return ErrMissingField("account_key") - } - if err := req.AccountKey.Validate(); err != nil { - return err - } - expectedMetadata, validCredentialType := credentialTypesMetadata[req.Type] if !validCredentialType { return ErrInvalidCredentialType @@ -191,6 +184,12 @@ func (req *CreateCredentialRequest) Validate() error { } } + if req.AccountKey != nil { + if err := req.AccountKey.Validate(); err != nil { + return err + } + } + if validator, ok := req.Proof.(validator); ok { if err := validator.Validate(); err != nil { return err From aa56406beaf9cefe4c57d9e14491660ddb6476bd Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Thu, 16 Jul 2020 12:07:43 +0200 Subject: [PATCH 08/46] Enforce that account key is present in create service request --- internals/api/service.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internals/api/service.go b/internals/api/service.go index e81e6500..ea4a47b3 100644 --- a/internals/api/service.go +++ b/internals/api/service.go @@ -82,6 +82,13 @@ func (req CreateServiceRequest) Validate() error { return err } + if req.Credential.AccountKey == nil{ + return ErrMissingField("account_key") + } + if err := req.Credential.AccountKey.Validate(); err != nil { + return err + } + if req.RepoMember == nil { return ErrMissingField("repo_member") } From 659f463ac623e51d7a25c5f5e469624ea9e86f2e Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Thu, 16 Jul 2020 12:10:37 +0200 Subject: [PATCH 09/46] Fix linter errors --- internals/api/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/api/service.go b/internals/api/service.go index ea4a47b3..ac472558 100644 --- a/internals/api/service.go +++ b/internals/api/service.go @@ -82,7 +82,7 @@ func (req CreateServiceRequest) Validate() error { return err } - if req.Credential.AccountKey == nil{ + if req.Credential.AccountKey == nil { return ErrMissingField("account_key") } if err := req.Credential.AccountKey.Validate(); err != nil { From 65cbf971394c19e223139fe7145e99cd5711882f Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 12:32:32 +0100 Subject: [PATCH 10/46] Add CredentialSource interface and include credential source in errors This interface is meant to be implemented by credential readers and add more information to credential-related errors. If implemented by a credential reader, every credential parsing, decoding and decrypting error will also include the source of the credential (path to credential file or environment variable name). --- pkg/secrethub/configdir/dir.go | 5 +++++ pkg/secrethub/credentials/key.go | 10 +++++++++ pkg/secrethub/credentials/providers.go | 13 ++++++++++- pkg/secrethub/credentials/readers.go | 30 +++++++++++++++++++++----- 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/pkg/secrethub/configdir/dir.go b/pkg/secrethub/configdir/dir.go index 51b6106b..c8f83218 100644 --- a/pkg/secrethub/configdir/dir.go +++ b/pkg/secrethub/configdir/dir.go @@ -77,6 +77,11 @@ func (f *CredentialFile) Path() string { return f.path } +// Source returns the path to the credential file. +func (f *CredentialFile) Source() string { + return f.path +} + // Write writes the given bytes to the credential file. func (f *CredentialFile) Write(data []byte) error { err := os.MkdirAll(filepath.Dir(f.path), os.FileMode(0700)) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index 211c2767..53cc835e 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -2,12 +2,22 @@ package credentials import ( "errors" + "fmt" "github.com/secrethub/secrethub-go/internals/auth" "github.com/secrethub/secrethub-go/internals/crypto" "github.com/secrethub/secrethub-go/pkg/secrethub/internals/http" ) +type ErrLoadingCredential struct { + Location string + Err error +} + +func (e ErrLoadingCredential) Error() string { + return fmt.Sprintf("error loading credential loaded from '%s': %v", e.Location, e.Err) +} + // Key is a credential that uses a local key for all its operations. type Key struct { key *RSACredential diff --git a/pkg/secrethub/credentials/providers.go b/pkg/secrethub/credentials/providers.go index 858160c2..14be6918 100644 --- a/pkg/secrethub/credentials/providers.go +++ b/pkg/secrethub/credentials/providers.go @@ -39,7 +39,12 @@ func (k KeyProvider) Passphrase(passphraseReader Reader) Provider { // Provide implements the Provider interface for a KeyProvider. func (k KeyProvider) Provide(httpClient *http.Client) (auth.Authenticator, Decrypter, error) { key, err := ImportKey(k.credentialReader, k.passphraseReader) - if err != nil { + if source, ok := k.credentialReader.(CredentialSource); ok && err != nil { + return nil, nil, ErrLoadingCredential{ + Location: source.Source(), + Err: err, + } + } else if err != nil { return nil, nil, err } return key.Provide(httpClient) @@ -52,3 +57,9 @@ type providerFunc func(*http.Client) (auth.Authenticator, Decrypter, error) func (f providerFunc) Provide(httpClient *http.Client) (auth.Authenticator, Decrypter, error) { return f(httpClient) } + +// CredentialSource should be implemented by credential readers to allow returning credential reading errors +// that include the credentials source (e.g. path to credential file, environment variable etc.). +type CredentialSource interface { + Source() string +} diff --git a/pkg/secrethub/credentials/readers.go b/pkg/secrethub/credentials/readers.go index 67fe88bd..9d1fdaab 100644 --- a/pkg/secrethub/credentials/readers.go +++ b/pkg/secrethub/credentials/readers.go @@ -14,7 +14,7 @@ type Reader interface { // FromFile returns a reader that reads the contents of a file, // e.g. a credential or a passphrase. func FromFile(path string) Reader { - return readerFunc(func() ([]byte, error) { + return newReader(path, func() ([]byte, error) { return ioutil.ReadFile(path) }) } @@ -22,7 +22,7 @@ func FromFile(path string) Reader { // FromEnv returns a reader that reads the contents of an // environment variable, e.g. a credential or a passphrase. func FromEnv(key string) Reader { - return readerFunc(func() ([]byte, error) { + return newReader("$"+key, func() ([]byte, error) { return []byte(os.Getenv(key)), nil }) } @@ -43,10 +43,30 @@ func FromString(raw string) Reader { }) } -// readerFunc is a helper function to create a reader from any func() ([]byte, error). type readerFunc func() ([]byte, error) +func (r readerFunc) Read() ([]byte, error) { + return r() +} + +// newReader is a helper function to create a reader with a source from any func() ([]byte, error). +func newReader(source string, read func() ([]byte, error)) Reader { + return reader{ + source: source, + readFunc: read, + } +} + +type reader struct { + source string + readFunc func() ([]byte, error) +} + +func (r reader) Source() string { + return r.source +} + // Read implements the Reader interface. -func (f readerFunc) Read() ([]byte, error) { - return f() +func (r reader) Read() ([]byte, error) { + return r.readFunc() } From 27c7b3dec4b0ace5b76b3dd46c2c4558a25274e7 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 12:40:24 +0100 Subject: [PATCH 11/46] Use credential.FromEnv for reading environment variable --- pkg/secrethub/client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/secrethub/client.go b/pkg/secrethub/client.go index 5e0802b7..cc492b44 100644 --- a/pkg/secrethub/client.go +++ b/pkg/secrethub/client.go @@ -264,9 +264,10 @@ func (c *Client) with(options ...ClientOption) error { // sourcing it either from the SECRETHUB_CREDENTIAL environment variable or // from the configuration directory. func (c *Client) DefaultCredential() credentials.Reader { - envCredential := os.Getenv("SECRETHUB_CREDENTIAL") + const credentialEnvironmentVariable = "SECRETHUB_CREDENTIAL" + envCredential := os.Getenv(credentialEnvironmentVariable) if envCredential != "" { - return credentials.FromString(envCredential) + return credentials.FromEnv(credentialEnvironmentVariable) } return c.ConfigDir.Credential() From 2bcd52959a2a611c37ee20d8ec4aa32fb06daa6d Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 12:41:21 +0100 Subject: [PATCH 12/46] Prevent credentials.FromEnv from delaying env var read --- pkg/secrethub/credentials/readers.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/readers.go b/pkg/secrethub/credentials/readers.go index 9d1fdaab..0efcfd2a 100644 --- a/pkg/secrethub/credentials/readers.go +++ b/pkg/secrethub/credentials/readers.go @@ -22,8 +22,9 @@ func FromFile(path string) Reader { // FromEnv returns a reader that reads the contents of an // environment variable, e.g. a credential or a passphrase. func FromEnv(key string) Reader { + credential := os.Getenv(key) return newReader("$"+key, func() ([]byte, error) { - return []byte(os.Getenv(key)), nil + return []byte(credential), nil }) } From bbe89cb9951a032d6f401ae56509872b68924c17 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 12:43:36 +0100 Subject: [PATCH 13/46] Add more detailed no passphrase error --- pkg/secrethub/credentials/encoding.go | 1 + pkg/secrethub/credentials/key.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/encoding.go b/pkg/secrethub/credentials/encoding.go index 3ed72ddf..72bf6b94 100644 --- a/pkg/secrethub/credentials/encoding.go +++ b/pkg/secrethub/credentials/encoding.go @@ -25,6 +25,7 @@ var ( ErrCannotDecodeCredentialPayload = errCredentials.Code("invalid_credential_header").ErrorPref("cannot decode credential payload: %v") ErrCannotDecodeEncryptedCredential = errCredentials.Code("cannot_decode_encrypted_credential").Error("cannot decode an encrypted credential without a key") ErrCannotDecryptCredential = errCredentials.Code("cannot_decrypt_credential").Error("passphrase is incorrect") + ErrNeedPassphrase = errCredentials.Code("credential_passphrase_required").Error("default credential is password-protected. Configure the client to use service credential by provisioning one through the SECRETHUB_CREDENTIAL environment variable") ErrMalformedCredential = errCredentials.Code("malformed_credential").ErrorPref("credential is malformed: %v") ErrInvalidKey = errCredentials.Code("invalid_key").Error("the given key is not valid for the encryption algorithm") ) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index 53cc835e..8c3b535b 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -80,7 +80,7 @@ func ImportKey(credentialReader, passphraseReader Reader) (Key, error) { } if encoded.IsEncrypted() { if passphraseReader == nil { - return Key{}, errors.New("need passphrase") + return Key{}, ErrNeedPassphrase } // Try up to three times to get the correct passphrase. From 4de867f48fe19067ebce297228351b3c5dbdaa5a Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 12:54:36 +0100 Subject: [PATCH 14/46] Check SECRETHUB_CREDENTIAL_PASSPHRASE envvar if no passphrase reader is given --- pkg/secrethub/credentials/key.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index 8c3b535b..90e086ed 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -3,6 +3,7 @@ package credentials import ( "errors" "fmt" + "os" "github.com/secrethub/secrethub-go/internals/auth" "github.com/secrethub/secrethub-go/internals/crypto" @@ -79,9 +80,17 @@ func ImportKey(credentialReader, passphraseReader Reader) (Key, error) { return Key{}, err } if encoded.IsEncrypted() { - if passphraseReader == nil { + envPassphrase := os.Getenv("SECRETHUB_CREDENTIAL_PASSPHRASE") + if passphraseReader == nil && envPassphrase == "" { return Key{}, ErrNeedPassphrase } + if passphraseReader == nil { + credential, err := decryptKey([]byte(envPassphrase), encoded) + if err != nil { + return Key{}, err + } + return Key{key:credential}, nil + } // Try up to three times to get the correct passphrase. for i := 0; i < 3; i++ { From 6c73171158acf618df90ae6d9423193fb1e56e8f Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 12:55:40 +0100 Subject: [PATCH 15/46] Run goimports --- pkg/secrethub/credentials/key.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index 90e086ed..a1aa6a9f 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -89,7 +89,7 @@ func ImportKey(credentialReader, passphraseReader Reader) (Key, error) { if err != nil { return Key{}, err } - return Key{key:credential}, nil + return Key{key: credential}, nil } // Try up to three times to get the correct passphrase. From 12e9d3df0aa5de0a3aaf0f4281df5f44fb2d4c54 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 11 Nov 2020 13:04:35 +0100 Subject: [PATCH 16/46] Make SECRETHUB_CREDENTIAL_PASSPHRASE have priority over passphrase reader --- pkg/secrethub/credentials/key.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index a1aa6a9f..cd1045ed 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -81,16 +81,16 @@ func ImportKey(credentialReader, passphraseReader Reader) (Key, error) { } if encoded.IsEncrypted() { envPassphrase := os.Getenv("SECRETHUB_CREDENTIAL_PASSPHRASE") - if passphraseReader == nil && envPassphrase == "" { - return Key{}, ErrNeedPassphrase - } - if passphraseReader == nil { + if envPassphrase != "" { credential, err := decryptKey([]byte(envPassphrase), encoded) if err != nil { return Key{}, err } return Key{key: credential}, nil } + if passphraseReader == nil { + return Key{}, ErrNeedPassphrase + } // Try up to three times to get the correct passphrase. for i := 0; i < 3; i++ { From a86d7da02e9f7e13813a482f20570c9850d2fa49 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 2 Dec 2020 16:04:46 +0200 Subject: [PATCH 17/46] Update pkg/secrethub/credentials/key.go Co-authored-by: Simon Barendse --- pkg/secrethub/credentials/key.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index cd1045ed..0c2f89e2 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -16,7 +16,7 @@ type ErrLoadingCredential struct { } func (e ErrLoadingCredential) Error() string { - return fmt.Sprintf("error loading credential loaded from '%s': %v", e.Location, e.Err) + return "load credential " + e.Location + ": " + e.Err.Error() } // Key is a credential that uses a local key for all its operations. From 57d464b59bf9b62f681c3e05b84d7671b7c5f642 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 2 Dec 2020 16:08:35 +0200 Subject: [PATCH 18/46] Refactor err check Co-authored-by: Simon Barendse --- pkg/secrethub/credentials/providers.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/secrethub/credentials/providers.go b/pkg/secrethub/credentials/providers.go index 14be6918..c68d4dfd 100644 --- a/pkg/secrethub/credentials/providers.go +++ b/pkg/secrethub/credentials/providers.go @@ -39,12 +39,13 @@ func (k KeyProvider) Passphrase(passphraseReader Reader) Provider { // Provide implements the Provider interface for a KeyProvider. func (k KeyProvider) Provide(httpClient *http.Client) (auth.Authenticator, Decrypter, error) { key, err := ImportKey(k.credentialReader, k.passphraseReader) - if source, ok := k.credentialReader.(CredentialSource); ok && err != nil { - return nil, nil, ErrLoadingCredential{ - Location: source.Source(), - Err: err, + if err != nil { + if source, ok := k.credentialReader.(CredentialSource); ok { + return nil, nil, ErrLoadingCredential{ + Location: source.Source(), + Err: err, + } } - } else if err != nil { return nil, nil, err } return key.Provide(httpClient) From 53bddabda5712d03c8f55083515f39c83c57851d Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 2 Dec 2020 15:15:19 +0100 Subject: [PATCH 19/46] Remove unused import --- pkg/secrethub/credentials/key.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index 0c2f89e2..b90ac157 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -2,7 +2,6 @@ package credentials import ( "errors" - "fmt" "os" "github.com/secrethub/secrethub-go/internals/auth" From 48e536f6c714df3841c286ba7765c4a49f6ffffc Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Wed, 2 Dec 2020 16:18:49 +0200 Subject: [PATCH 20/46] Improve 'Need Passphrase' error Co-authored-by: Simon Barendse --- pkg/secrethub/credentials/encoding.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/encoding.go b/pkg/secrethub/credentials/encoding.go index 72bf6b94..1151bf33 100644 --- a/pkg/secrethub/credentials/encoding.go +++ b/pkg/secrethub/credentials/encoding.go @@ -25,7 +25,7 @@ var ( ErrCannotDecodeCredentialPayload = errCredentials.Code("invalid_credential_header").ErrorPref("cannot decode credential payload: %v") ErrCannotDecodeEncryptedCredential = errCredentials.Code("cannot_decode_encrypted_credential").Error("cannot decode an encrypted credential without a key") ErrCannotDecryptCredential = errCredentials.Code("cannot_decrypt_credential").Error("passphrase is incorrect") - ErrNeedPassphrase = errCredentials.Code("credential_passphrase_required").Error("default credential is password-protected. Configure the client to use service credential by provisioning one through the SECRETHUB_CREDENTIAL environment variable") + ErrNeedPassphrase = errCredentials.Code("credential_passphrase_required").Error("credential is password-protected. Configure the client to use service credential by provisioning one through the SECRETHUB_CREDENTIAL environment variable") ErrMalformedCredential = errCredentials.Code("malformed_credential").ErrorPref("credential is malformed: %v") ErrInvalidKey = errCredentials.Code("invalid_key").Error("the given key is not valid for the encryption algorithm") ) From fd75ad861eef600af471f696769786cefffff0a9 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Fri, 4 Dec 2020 16:57:29 +0100 Subject: [PATCH 21/46] Add option set set a default passphrase that is used for the default key credential This allows setting a passphrase without having to provide a credential itself, which can be used to make configuring the Terraform provider easier. --- pkg/secrethub/client.go | 11 +++++++---- pkg/secrethub/client_options.go | 9 +++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/secrethub/client.go b/pkg/secrethub/client.go index cc492b44..1c73c11d 100644 --- a/pkg/secrethub/client.go +++ b/pkg/secrethub/client.go @@ -82,6 +82,8 @@ type Client struct { // These are cached repoIndexKeys map[api.RepoPath]*crypto.SymmetricKey + defaultPassphraseReader credentials.Reader + appInfo []*AppInfo ConfigDir *configdir.Dir } @@ -119,9 +121,10 @@ 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{}, + httpClient: http.NewClient(), + repoIndexKeys: make(map[api.RepoPath]*crypto.SymmetricKey), + appInfo: []*AppInfo{}, + defaultPassphraseReader: credentials.FromEnv("SECRETHUB_CREDENTIAL_PASSPHRASE"), } err := client.with(with...) if err != nil { @@ -144,7 +147,7 @@ func NewClient(with ...ClientOption) (*Client, error) { var provider credentials.Provider switch strings.ToLower(identityProvider) { case "", "key": - provider = credentials.UseKey(client.DefaultCredential()) + provider = credentials.UseKey(client.DefaultCredential()).Passphrase(client.defaultPassphraseReader) case "aws": provider = credentials.UseAWS() case "gcp": diff --git a/pkg/secrethub/client_options.go b/pkg/secrethub/client_options.go index 7ad43796..2c867b8e 100644 --- a/pkg/secrethub/client_options.go +++ b/pkg/secrethub/client_options.go @@ -78,3 +78,12 @@ func WithCredentials(provider credentials.Provider) ClientOption { return nil } } + +// WithDefaultPassphraseReader sets a default passphrase reader that is used for decrypting an encrypted key credential +// if no credential is set explicitly. +func WithDefaultPassphraseReader(reader credentials.Reader) ClientOption { + return func(c *Client) error { + c.defaultPassphraseReader = reader + return nil + } +} From d81fe5a62239e24c4e1e05266cf670ee580854bd Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Tue, 15 Dec 2020 15:59:15 +0100 Subject: [PATCH 22/46] Delay reading of credential from environment variable --- pkg/secrethub/credentials/readers.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/secrethub/credentials/readers.go b/pkg/secrethub/credentials/readers.go index 0efcfd2a..9d1fdaab 100644 --- a/pkg/secrethub/credentials/readers.go +++ b/pkg/secrethub/credentials/readers.go @@ -22,9 +22,8 @@ func FromFile(path string) Reader { // FromEnv returns a reader that reads the contents of an // environment variable, e.g. a credential or a passphrase. func FromEnv(key string) Reader { - credential := os.Getenv(key) return newReader("$"+key, func() ([]byte, error) { - return []byte(credential), nil + return []byte(os.Getenv(key)), nil }) } From e5f45b9d8a0dd7bc2c10b0a77a50556d7d2b1d87 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Tue, 15 Dec 2020 16:06:59 +0100 Subject: [PATCH 23/46] Wrap errors regarding credential passphrase and specify passphrase source --- pkg/secrethub/credentials/key.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/secrethub/credentials/key.go b/pkg/secrethub/credentials/key.go index b90ac157..23e039d9 100644 --- a/pkg/secrethub/credentials/key.go +++ b/pkg/secrethub/credentials/key.go @@ -2,6 +2,7 @@ package credentials import ( "errors" + "fmt" "os" "github.com/secrethub/secrethub-go/internals/auth" @@ -79,11 +80,12 @@ func ImportKey(credentialReader, passphraseReader Reader) (Key, error) { return Key{}, err } if encoded.IsEncrypted() { - envPassphrase := os.Getenv("SECRETHUB_CREDENTIAL_PASSPHRASE") + const credentialPassphraseEnvVar = "SECRETHUB_CREDENTIAL_PASSPHRASE" + envPassphrase := os.Getenv(credentialPassphraseEnvVar) if envPassphrase != "" { credential, err := decryptKey([]byte(envPassphrase), encoded) if err != nil { - return Key{}, err + return Key{}, fmt.Errorf("decrypting credential with passphrase read from $%s: %v", credentialPassphraseEnvVar, err) } return Key{key: credential}, nil } From 38d7f3b3fecf3d3f6a594dddba1f5c249f8e97fa Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 15 Dec 2020 17:08:36 +0100 Subject: [PATCH 24/46] fix need passphrase error message Co-authored-by: Marton Soos --- pkg/secrethub/credentials/encoding.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrethub/credentials/encoding.go b/pkg/secrethub/credentials/encoding.go index 1151bf33..0c22d047 100644 --- a/pkg/secrethub/credentials/encoding.go +++ b/pkg/secrethub/credentials/encoding.go @@ -25,7 +25,7 @@ var ( ErrCannotDecodeCredentialPayload = errCredentials.Code("invalid_credential_header").ErrorPref("cannot decode credential payload: %v") ErrCannotDecodeEncryptedCredential = errCredentials.Code("cannot_decode_encrypted_credential").Error("cannot decode an encrypted credential without a key") ErrCannotDecryptCredential = errCredentials.Code("cannot_decrypt_credential").Error("passphrase is incorrect") - ErrNeedPassphrase = errCredentials.Code("credential_passphrase_required").Error("credential is password-protected. Configure the client to use service credential by provisioning one through the SECRETHUB_CREDENTIAL environment variable") + ErrNeedPassphrase = errCredentials.Code("credential_passphrase_required").Error("credential is password-protected. Configure a credential passphrase through the SECRETHUB_CREDENTIAL_PASSPHRASE environment variable or use a credential that is not password-protected") ErrMalformedCredential = errCredentials.Code("malformed_credential").ErrorPref("credential is malformed: %v") ErrInvalidKey = errCredentials.Code("invalid_key").Error("the given key is not valid for the encryption algorithm") ) From 309a63b6c3e2a0b245aef8bf0bbe853cc9e2c9e3 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Tue, 5 Jan 2021 10:01:11 +0100 Subject: [PATCH 25/46] Allow UnexpectedError to be unwrapped to orig err This allows displaying the original error without any other text, while not breaking the existing contract of throwing UnexpectedErrors. --- internals/errio/errors.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internals/errio/errors.go b/internals/errio/errors.go index ea490e0b..77b348b1 100644 --- a/internals/errio/errors.go +++ b/internals/errio/errors.go @@ -158,6 +158,7 @@ func UnexpectedError(err error) PublicError { "an unexpected error occurred: %v\n\nTry again later or contact support@secrethub.io if the problem persists", err, ), + err: err, } } @@ -190,6 +191,7 @@ type PublicError struct { Namespace Namespace `json:"namespace,omitempty"` Code string `json:"code"` Message string `json:"message"` + err error } // PublicError implements the error interface. @@ -221,6 +223,11 @@ func (e PublicError) Type() string { return fmt.Sprintf("%s.%s", e.Namespace, e.Code) } +// Unwrap returns the wrapped error if the PublicError represents an error wrapped as an UnexpectedError. +func (e PublicError) Unwrap() error { + return e.err +} + // PublicStatusError represents an http error. It contains an HTTP status // code and can be json encoded in an HTTP response. type PublicStatusError struct { From 6f9662b0e54603b7e8f59da8e7a8219521cce651 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 5 Jan 2021 16:47:20 +0100 Subject: [PATCH 26/46] Configure the remote server with SECRETHUB_API_REMOTE envvar Just like the CLI already checks the SECRETHUB_API_REMOTE envvar, the SDK now also accepts the same envvar. This also makes it possible to use this envvar in other projects, for example the Terraform provider, if they upgrade the SDK to a version including this commit. --- pkg/secrethub/client.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/secrethub/client.go b/pkg/secrethub/client.go index 1c73c11d..14bbc2f8 100644 --- a/pkg/secrethub/client.go +++ b/pkg/secrethub/client.go @@ -181,6 +181,14 @@ func NewClient(with ...ClientOption) (*Client, error) { client.httpClient.Options(http.WithUserAgent(userAgent)) + apiRemote := os.Getenv("SECRETHUB_API_REMOTE") + if apiRemote != "" { + err := client.with(WithServerURL(apiRemote)) + if err != nil { + return nil, err + } + } + return client, nil } From 1a4b0944c98439a80b41f45549ba97ff2e35d301 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 12:05:23 +0100 Subject: [PATCH 27/46] Refactor access rule creation Factored out re-encryption of directories and secrets, so that in a next commit, retry behavior can be implemented. --- pkg/secrethub/acl.go | 107 +++++++++++++++++++++++++++++++------------ 1 file changed, 77 insertions(+), 30 deletions(-) diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index bd210e3a..1473378d 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -218,50 +218,97 @@ func (s accessRuleService) create(path api.BlindNamePath, permission api.Permiss Permission: permission, } + encrypter := newReencrypter(account, s.client) + if currentAccessLevel.Permission < api.PermissionRead { - encryptedTree, err := s.client.httpClient.GetTree(blindName, -1, true) + err = encrypter.Add(blindName) if err != nil { - return nil, errio.Error(err) + return nil, err } - accountKey, err := s.client.getAccountKey() - if err != nil { - return nil, errio.Error(err) - } + in.EncryptedDirs = encrypter.Dirs() + in.EncryptedSecrets = encrypter.Secrets() + } + err = in.Validate() + if err != nil { + return nil, err + } - dirs, secrets, err := encryptedTree.DecryptContents(accountKey) - if err != nil { - return nil, errio.Error(err) - } + accessRule, err := s.client.httpClient.CreateAccessRule(blindName, accountName, in) - in.EncryptedDirs = make([]api.EncryptedNameForNodeRequest, 0, len(dirs)) - for _, dir := range dirs { - encryptedDirs, err := s.client.encryptDirFor(dir, account) - if err != nil { - return nil, errio.Error(err) - } - in.EncryptedDirs = append(in.EncryptedDirs, encryptedDirs...) - } + return accessRule, err - in.EncryptedSecrets = make([]api.SecretAccessRequest, 0, len(secrets)) - for _, secret := range secrets { - encryptedSecrets, err := s.client.encryptSecretFor(secret, account) - if err != nil { - return nil, errio.Error(err) - } - in.EncryptedSecrets = append(in.EncryptedSecrets, encryptedSecrets...) +} - } +func newReencrypter(encryptFor *api.Account, client *Client) *reencrypter { + return &reencrypter{ + dirs: make(map[uuid.UUID]api.EncryptedNameForNodeRequest), + secrets: make(map[uuid.UUID]api.SecretAccessRequest), + encryptFor: encryptFor, + client: client, } +} - err = in.Validate() +type reencrypter struct { + dirs map[uuid.UUID]api.EncryptedNameForNodeRequest + secrets map[uuid.UUID]api.SecretAccessRequest + encryptFor *api.Account + client *Client +} + +func (re *reencrypter) Add(blindName string) error { + encryptedTree, err := re.client.httpClient.GetTree(blindName, -1, true) if err != nil { - return nil, err + return err } - accessRule, err := s.client.httpClient.CreateAccessRule(blindName, accountName, in) - return accessRule, errio.Error(err) + accountKey, err := re.client.getAccountKey() + if err != nil { + return err + } + + dirs, secrets, err := encryptedTree.DecryptContents(accountKey) + if err != nil { + return err + } + + for _, dir := range dirs { + encryptedDirs, err := re.client.encryptDirFor(dir, re.encryptFor) + if err != nil { + return err + } + re.dirs[dir.DirID] = encryptedDirs[0] + } + + for _, secret := range secrets { + encryptedSecrets, err := re.client.encryptSecretFor(secret, re.encryptFor) + if err != nil { + return err + } + re.secrets[secret.SecretID] = encryptedSecrets[0] + } + return nil +} + +func (re *reencrypter) Secrets() []api.SecretAccessRequest { + res := make([]api.SecretAccessRequest, len(re.secrets)) + i := 0 + for _, secret := range re.secrets { + res[i] = secret + i++ + } + return res +} + +func (re *reencrypter) Dirs() []api.EncryptedNameForNodeRequest { + res := make([]api.EncryptedNameForNodeRequest, len(re.dirs)) + i := 0 + for _, dir := range re.dirs { + res[i] = dir + i++ + } + return res } // UpdateAccessRule updates an AccessRule for an account with a certain permission level. From cccce6bc3f1d3221e67b64933721c55088982985 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 12:12:37 +0100 Subject: [PATCH 28/46] Change reencrypter to lookup if dir is already reencrypted This allows to skip directories and secrets that were already reencrypted in a previous iteration, when we add retry behavior. --- pkg/secrethub/acl.go | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index 1473378d..ca441386 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -267,25 +267,34 @@ func (re *reencrypter) Add(blindName string) error { return err } - dirs, secrets, err := encryptedTree.DecryptContents(accountKey) - if err != nil { - return err - } - - for _, dir := range dirs { - encryptedDirs, err := re.client.encryptDirFor(dir, re.encryptFor) - if err != nil { - return err + for _, dir := range encryptedTree.Directories { + _, ok := re.dirs[dir.DirID] + if !ok { + decrypted, err := dir.Decrypt(accountKey) + if err != nil { + return err + } + encrypted, err := re.client.encryptDirFor(decrypted, re.encryptFor) + if err != nil { + return err + } + re.dirs[dir.DirID] = encrypted[0] } - re.dirs[dir.DirID] = encryptedDirs[0] } - for _, secret := range secrets { - encryptedSecrets, err := re.client.encryptSecretFor(secret, re.encryptFor) - if err != nil { - return err + for _, secret := range encryptedTree.Secrets { + _, ok := re.secrets[secret.SecretID] + if !ok { + decrypted, err := secret.Decrypt(accountKey) + if err != nil { + return err + } + encrypted, err := re.client.encryptSecretFor(decrypted, re.encryptFor) + if err != nil { + return err + } + re.secrets[secret.SecretID] = encrypted[0] } - re.secrets[secret.SecretID] = encryptedSecrets[0] } return nil From e28f38807584ff1821ba61cf358fdb9f6cfe9173 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 12:21:18 +0100 Subject: [PATCH 29/46] Add retry behavior for missing members to access rule creation --- pkg/secrethub/acl.go | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index ca441386..baeecd04 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -31,6 +31,14 @@ type AccessRuleService interface { LevelIterator(path string, _ *AccessLevelIteratorParams) AccessLevelIterator } +// missingMemberRetries is the number of times creation of access rules is retried when +// encrypted members are missing in the request. Members to encrypt are fetched again this +// number of times and a new access rule create request is made. +// When creating access rules, missing members occur when secrets, secret keys, or directories +// were added between fetching the secrets, secret keys and directories to encrypt for the +// account for which the access rule is created and the request creating the access rule. +const missingMemberRetries = 3 + func newAccessRuleService(client *Client) AccessRuleService { return accessRuleService{ client: client, @@ -220,24 +228,28 @@ func (s accessRuleService) create(path api.BlindNamePath, permission api.Permiss encrypter := newReencrypter(account, s.client) - if currentAccessLevel.Permission < api.PermissionRead { - err = encrypter.Add(blindName) + tries := 0 + for { + if currentAccessLevel.Permission < api.PermissionRead { + err = encrypter.Add(blindName) + if err != nil { + return nil, err + } + + in.EncryptedDirs = encrypter.Dirs() + in.EncryptedSecrets = encrypter.Secrets() + } + err = in.Validate() if err != nil { return nil, err } - in.EncryptedDirs = encrypter.Dirs() - in.EncryptedSecrets = encrypter.Secrets() - } - err = in.Validate() - if err != nil { - return nil, err + accessRule, err := s.client.httpClient.CreateAccessRule(blindName, accountName, in) + if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + return accessRule, err + } + tries++ } - - accessRule, err := s.client.httpClient.CreateAccessRule(blindName, accountName, in) - - return accessRule, err - } func newReencrypter(encryptFor *api.Account, client *Client) *reencrypter { From c640f85d1007287f55461aaf4d94eebf40e4cd16 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 13:30:10 +0100 Subject: [PATCH 30/46] Factor out function to encrypt name for a single account --- pkg/secrethub/crypto.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/secrethub/crypto.go b/pkg/secrethub/crypto.go index 8512656c..def5d31d 100644 --- a/pkg/secrethub/crypto.go +++ b/pkg/secrethub/crypto.go @@ -103,25 +103,33 @@ func encryptNameForNodeAccounts(nodeID uuid.UUID, name string, accounts ...*api. func encryptNameForAccounts(name string, accounts ...*api.Account) ([]api.EncryptedNameRequest, error) { encryptedNames := make([]api.EncryptedNameRequest, len(accounts)) for i, account := range accounts { - publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) - if err != nil { - return nil, errio.Error(err) - } - - ciphertext, err := publicKey.Wrap([]byte(name)) + var err error + encryptedNames[i], err = encryptNameForAccount(name, account) if err != nil { return nil, err } - - encryptedNames[i] = api.EncryptedNameRequest{ - AccountID: account.AccountID, - EncryptedName: ciphertext, - } } return encryptedNames, nil } +func encryptNameForAccount(name string, account *api.Account) (api.EncryptedNameRequest, error) { + publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) + if err != nil { + return api.EncryptedNameRequest{}, err + } + + ciphertext, err := publicKey.Wrap([]byte(name)) + if err != nil { + return api.EncryptedNameRequest{}, err + } + + return api.EncryptedNameRequest{ + AccountID: account.AccountID, + EncryptedName: ciphertext, + }, nil +} + // encryptKeyForAccounts encrypts the key for every account and returns a list of EncryptedKeyRequests func encryptKeyForAccounts(key *crypto.SymmetricKey, accounts ...*api.Account) ([]api.EncryptedKeyRequest, error) { encryptedKeys := make([]api.EncryptedKeyRequest, len(accounts)) From 5c544de1a14e7846ac5d3d6e25495db72a324466 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 13:37:06 +0100 Subject: [PATCH 31/46] Refactor access rule encryption Before this commit, the reencrypter took the first (and only) entry out of the returned lists. This was possible because the number of entries in the returned list equals the number of passed accounts to the function. With this refactor, the lookup is removed and we can rely fully on the compiler rather than this implicit agreement that the number of returned entries equals the number of accounts passed. --- pkg/secrethub/acl.go | 4 +- pkg/secrethub/crypto.go | 102 ++++++++++++++++++---------------------- 2 files changed, 47 insertions(+), 59 deletions(-) diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index baeecd04..a91fdba5 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -290,7 +290,7 @@ func (re *reencrypter) Add(blindName string) error { if err != nil { return err } - re.dirs[dir.DirID] = encrypted[0] + re.dirs[dir.DirID] = encrypted } } @@ -305,7 +305,7 @@ func (re *reencrypter) Add(blindName string) error { if err != nil { return err } - re.secrets[secret.SecretID] = encrypted[0] + re.secrets[secret.SecretID] = encrypted } } diff --git a/pkg/secrethub/crypto.go b/pkg/secrethub/crypto.go index def5d31d..3722b1b9 100644 --- a/pkg/secrethub/crypto.go +++ b/pkg/secrethub/crypto.go @@ -7,96 +7,84 @@ import ( "github.com/secrethub/secrethub-go/internals/errio" ) -func (c *Client) encryptDirFor(dir *api.Dir, accounts ...*api.Account) ([]api.EncryptedNameForNodeRequest, error) { - currentDir, err := encryptNameForNodeAccounts(dir.DirID, dir.Name, accounts...) - return currentDir, errio.Error(err) +func (c *Client) encryptDirFor(dir *api.Dir, account *api.Account) (api.EncryptedNameForNodeRequest, error) { + return encryptNameForNodeAccount(dir.DirID, dir.Name, account) } // encryptSecretFor encrypts the secret for every account. // The SecretKeys are retrieved from the API. // The keys are decrypted, then for every account this key is encrypted. -func (c *Client) encryptSecretFor(secret *api.Secret, accounts ...*api.Account) ([]api.SecretAccessRequest, error) { - results := make([]api.SecretAccessRequest, len(accounts)) - +func (c *Client) encryptSecretFor(secret *api.Secret, account *api.Account) (api.SecretAccessRequest, error) { secretKeys, err := c.httpClient.ListSecretKeys(secret.BlindName) if err != nil { - return nil, errio.Error(err) + return api.SecretAccessRequest{}, err } myKey, err := c.getAccountKey() if err != nil { - return nil, errio.Error(err) + return api.SecretAccessRequest{}, err } decryptedKeys := make([]*api.SecretKey, len(secretKeys)) for i, key := range secretKeys { decryptedKeys[i], err = key.Decrypt(myKey) if err != nil { - return nil, errio.Error(err) + return api.SecretAccessRequest{}, err } } - for i, account := range accounts { - publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) - if err != nil { - return nil, errio.Error(err) - } + publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) + if err != nil { + return api.SecretAccessRequest{}, err + } - encryptedSecretName, err := publicKey.Wrap([]byte(secret.Name)) - if err != nil { - return nil, errio.Error(err) - } + encryptedSecretName, err := publicKey.Wrap([]byte(secret.Name)) + if err != nil { + return api.SecretAccessRequest{}, err + } - encryptedName := api.EncryptedNameForNodeRequest{ - EncryptedNameRequest: api.EncryptedNameRequest{ - AccountID: account.AccountID, - EncryptedName: encryptedSecretName, - }, - NodeID: secret.SecretID, - } + encryptedName := api.EncryptedNameForNodeRequest{ + EncryptedNameRequest: api.EncryptedNameRequest{ + AccountID: account.AccountID, + EncryptedName: encryptedSecretName, + }, + NodeID: secret.SecretID, + } - encryptedKeys := make([]api.SecretKeyMemberRequest, len(decryptedKeys)) - for keyIndex, decryptedKey := range decryptedKeys { - encryptedKey, err := publicKey.Wrap(decryptedKey.Key.Export()) - if err != nil { - return nil, errio.Error(err) - } - - encryptedKeys[keyIndex] = api.SecretKeyMemberRequest{ - AccountID: account.AccountID, - SecretKeyID: decryptedKey.SecretKeyID, - EncryptedKey: encryptedKey, - } + encryptedKeys := make([]api.SecretKeyMemberRequest, len(decryptedKeys)) + for keyIndex, decryptedKey := range decryptedKeys { + encryptedKey, err := publicKey.Wrap(decryptedKey.Key.Export()) + if err != nil { + return api.SecretAccessRequest{}, err } - results[i] = api.SecretAccessRequest{ - Name: encryptedName, - Keys: encryptedKeys, + encryptedKeys[keyIndex] = api.SecretKeyMemberRequest{ + AccountID: account.AccountID, + SecretKeyID: decryptedKey.SecretKeyID, + EncryptedKey: encryptedKey, } } - return results, nil + return api.SecretAccessRequest{ + Name: encryptedName, + Keys: encryptedKeys, + }, nil } -// encryptNameForNodeAccounts encrypts the name for every account and returns a list of ExistingNameMemberRequests. -func encryptNameForNodeAccounts(nodeID uuid.UUID, name string, accounts ...*api.Account) ([]api.EncryptedNameForNodeRequest, error) { - encryptedNames, err := encryptNameForAccounts(name, accounts...) +// encryptNameForNodeAccount encrypts the name for the account and returns a EncryptedNameForNodeRequest. +func encryptNameForNodeAccount(nodeID uuid.UUID, name string, account *api.Account) (api.EncryptedNameForNodeRequest, error) { + encryptedName, err := encryptNameForAccount(name, account) if err != nil { - return nil, err - } - - encryptedExistingNames := make([]api.EncryptedNameForNodeRequest, len(encryptedNames)) - for index, encryptedName := range encryptedNames { - encryptedExistingNames[index] = api.EncryptedNameForNodeRequest{ - EncryptedNameRequest: api.EncryptedNameRequest{ - AccountID: encryptedName.AccountID, - EncryptedName: encryptedName.EncryptedName, - }, - NodeID: nodeID, - } + return api.EncryptedNameForNodeRequest{}, err } - return encryptedExistingNames, nil + return api.EncryptedNameForNodeRequest{ + EncryptedNameRequest: api.EncryptedNameRequest{ + AccountID: encryptedName.AccountID, + EncryptedName: encryptedName.EncryptedName, + }, + NodeID: nodeID, + }, nil } // encryptNameForAccounts encrypts the name for every account and returns a list of EncryptedNameRequests. From 296549a3865e75d5d3285b1989f61fcc10dcbbc3 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 13:46:23 +0100 Subject: [PATCH 32/46] Refactor dir name encryption in dir creation Using an intermediate map to store the encryption results allows for re-use of already encrypted names when implementing retry behavior in a later commit. --- pkg/secrethub/dir.go | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/pkg/secrethub/dir.go b/pkg/secrethub/dir.go index c794b5f6..14cbe04d 100644 --- a/pkg/secrethub/dir.go +++ b/pkg/secrethub/dir.go @@ -117,24 +117,39 @@ func (s dirService) Create(path string) (*api.Dir, error) { return nil, errio.Error(err) } - accounts, err := s.client.listDirAccounts(parentPath) + blindName, err := s.client.convertPathToBlindName(p) if err != nil { return nil, errio.Error(err) } - encryptedNames, err := encryptNameForAccounts(p.GetDirName(), accounts...) + parentBlindName, err := s.client.convertPathToBlindName(parentPath) if err != nil { return nil, errio.Error(err) } - blindName, err := s.client.convertPathToBlindName(p) + accounts, err := s.client.listDirAccounts(parentPath) if err != nil { return nil, errio.Error(err) } - parentBlindName, err := s.client.convertPathToBlindName(parentPath) - if err != nil { - return nil, errio.Error(err) + dirName := p.GetDirName() + encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest, len(accounts)) + for _, account := range accounts { + _, ok := encryptedNamesMap[account.AccountID] + if !ok { + encryptedName, err := encryptNameForAccount(dirName, account) + if err != nil { + return nil, err + } + encryptedNamesMap[account.AccountID] = encryptedName + } + } + + encryptedNames := make([]api.EncryptedNameRequest, len(encryptedNamesMap)) + i := 0 + for _, encryptedName := range encryptedNamesMap { + encryptedNames[i] = encryptedName + i++ } request := &api.CreateDirRequest{ From 259387ed59e4e008806db6d07be101384926bda2 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 13:52:32 +0100 Subject: [PATCH 33/46] Add retry behavior for dir creation in case of missing members When an access rule for an extra account is created inbetween fetching the accounts to encrypt for and the creation of the directory, the accounts to encrypt for are now fetched again and dir creation is retried with the directory encrypted for those fetches accounts. --- pkg/secrethub/dir.go | 76 ++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/pkg/secrethub/dir.go b/pkg/secrethub/dir.go index 14cbe04d..577f887e 100644 --- a/pkg/secrethub/dir.go +++ b/pkg/secrethub/dir.go @@ -127,50 +127,56 @@ func (s dirService) Create(path string) (*api.Dir, error) { return nil, errio.Error(err) } - accounts, err := s.client.listDirAccounts(parentPath) - if err != nil { - return nil, errio.Error(err) - } - dirName := p.GetDirName() - encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest, len(accounts)) - for _, account := range accounts { - _, ok := encryptedNamesMap[account.AccountID] - if !ok { - encryptedName, err := encryptNameForAccount(dirName, account) - if err != nil { - return nil, err + + tries := 0 + for { + accounts, err := s.client.listDirAccounts(parentPath) + if err != nil { + return nil, errio.Error(err) + } + + encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest, len(accounts)) + for _, account := range accounts { + _, ok := encryptedNamesMap[account.AccountID] + if !ok { + encryptedName, err := encryptNameForAccount(dirName, account) + if err != nil { + return nil, err + } + encryptedNamesMap[account.AccountID] = encryptedName } - encryptedNamesMap[account.AccountID] = encryptedName } - } - encryptedNames := make([]api.EncryptedNameRequest, len(encryptedNamesMap)) - i := 0 - for _, encryptedName := range encryptedNamesMap { - encryptedNames[i] = encryptedName - i++ - } + encryptedNames := make([]api.EncryptedNameRequest, len(encryptedNamesMap)) + i := 0 + for _, encryptedName := range encryptedNamesMap { + encryptedNames[i] = encryptedName + i++ + } - request := &api.CreateDirRequest{ - BlindName: blindName, - ParentBlindName: parentBlindName, + request := &api.CreateDirRequest{ + BlindName: blindName, + ParentBlindName: parentBlindName, - EncryptedNames: encryptedNames, - } + EncryptedNames: encryptedNames, + } - encryptedDir, err := s.client.httpClient.CreateDir(p.GetNamespace(), p.GetRepo(), request) - if err != nil { - return nil, errio.Error(err) - } + encryptedDir, err := s.client.httpClient.CreateDir(p.GetNamespace(), p.GetRepo(), request) + if err == nil { + accountKey, err := s.client.getAccountKey() + if err != nil { + return nil, errio.Error(err) + } - accountKey, err := s.client.getAccountKey() - if err != nil { - return nil, errio.Error(err) + dir, err := encryptedDir.Decrypt(accountKey) + return dir, errio.Error(err) + } + if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + return nil, err + } + tries++ } - - dir, err := encryptedDir.Decrypt(accountKey) - return dir, errio.Error(err) } // Exists returns whether a directory where you have access to exists at a given path. From 69f8d168b42c560342b208e5275b0897d3791d22 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 14:05:03 +0100 Subject: [PATCH 34/46] Fix: don't overwrite map of encrypted names between retries --- pkg/secrethub/dir.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrethub/dir.go b/pkg/secrethub/dir.go index 577f887e..2cdc4360 100644 --- a/pkg/secrethub/dir.go +++ b/pkg/secrethub/dir.go @@ -129,6 +129,7 @@ func (s dirService) Create(path string) (*api.Dir, error) { dirName := p.GetDirName() + encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest) tries := 0 for { accounts, err := s.client.listDirAccounts(parentPath) @@ -136,7 +137,6 @@ func (s dirService) Create(path string) (*api.Dir, error) { return nil, errio.Error(err) } - encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest, len(accounts)) for _, account := range accounts { _, ok := encryptedNamesMap[account.AccountID] if !ok { From 32dd9939f77d4b5ecc845bc7cd109c06a3c09c31 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 14:16:24 +0100 Subject: [PATCH 35/46] Refactor encryption in secret creation Using an intermediate map to store the encryption result allows for re-use of already encrypted names and keys when implementing retry behavior in a later commit. --- pkg/secrethub/crypto.go | 32 +++++++----------- pkg/secrethub/secret_version.go | 60 ++++++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 35 deletions(-) diff --git a/pkg/secrethub/crypto.go b/pkg/secrethub/crypto.go index 3722b1b9..94951076 100644 --- a/pkg/secrethub/crypto.go +++ b/pkg/secrethub/crypto.go @@ -4,7 +4,6 @@ import ( "github.com/secrethub/secrethub-go/internals/api" "github.com/secrethub/secrethub-go/internals/api/uuid" "github.com/secrethub/secrethub-go/internals/crypto" - "github.com/secrethub/secrethub-go/internals/errio" ) func (c *Client) encryptDirFor(dir *api.Dir, account *api.Account) (api.EncryptedNameForNodeRequest, error) { @@ -118,25 +117,20 @@ func encryptNameForAccount(name string, account *api.Account) (api.EncryptedName }, nil } -// encryptKeyForAccounts encrypts the key for every account and returns a list of EncryptedKeyRequests -func encryptKeyForAccounts(key *crypto.SymmetricKey, accounts ...*api.Account) ([]api.EncryptedKeyRequest, error) { - encryptedKeys := make([]api.EncryptedKeyRequest, len(accounts)) - for i, account := range accounts { - publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) - if err != nil { - return nil, errio.Error(err) - } - - encryptedSecretKey, err := publicKey.Wrap(key.Export()) - if err != nil { - return nil, errio.Error(err) - } +// encryptKeyForAccount encrypts the key for the account and returns an EncryptedKeyRequest. +func encryptKeyForAccount(key *crypto.SymmetricKey, account *api.Account) (api.EncryptedKeyRequest, error) { + publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) + if err != nil { + return api.EncryptedKeyRequest{}, err + } - encryptedKeys[i] = api.EncryptedKeyRequest{ - AccountID: account.AccountID, - EncryptedKey: encryptedSecretKey, - } + encryptedSecretKey, err := publicKey.Wrap(key.Export()) + if err != nil { + return api.EncryptedKeyRequest{}, err } - return encryptedKeys, nil + return api.EncryptedKeyRequest{ + AccountID: account.AccountID, + EncryptedKey: encryptedSecretKey, + }, nil } diff --git a/pkg/secrethub/secret_version.go b/pkg/secrethub/secret_version.go index 626da944..b764226f 100644 --- a/pkg/secrethub/secret_version.go +++ b/pkg/secrethub/secret_version.go @@ -3,12 +3,12 @@ package secrethub import ( "fmt" - "github.com/secrethub/secrethub-go/pkg/secrethub/iterator" - units "github.com/docker/go-units" "github.com/secrethub/secrethub-go/internals/api" + "github.com/secrethub/secrethub-go/internals/api/uuid" "github.com/secrethub/secrethub-go/internals/crypto" "github.com/secrethub/secrethub-go/internals/errio" + "github.com/secrethub/secrethub-go/pkg/secrethub/iterator" ) const ( @@ -229,31 +229,64 @@ func (c *Client) createSecret(secretPath api.SecretPath, data []byte) (*api.Secr if err != nil { return nil, errio.Error(err) } - - // Get all accounts that have permission to read the secret. - accounts, err := c.listDirAccounts(parentPath) + encryptedData, err := secretKey.Encrypt(data) if err != nil { return nil, errio.Error(err) } - encryptedNames, err := encryptNameForAccounts(secretPath.GetSecret(), accounts...) + blindName, err := c.convertPathToBlindName(secretPath) if err != nil { return nil, errio.Error(err) } - encryptedKeys, err := encryptKeyForAccounts(secretKey, accounts...) + parentBlindName, err := c.convertPathToBlindName(parentPath) if err != nil { return nil, errio.Error(err) } - encryptedData, err := secretKey.Encrypt(data) + secretName := secretPath.GetSecret() + + encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest) + encryptedKeysMap := make(map[uuid.UUID]api.EncryptedKeyRequest) + + // Get all accounts that have permission to read the secret. + accounts, err := c.listDirAccounts(parentPath) if err != nil { return nil, errio.Error(err) } - blindName, err := c.convertPathToBlindName(secretPath) - if err != nil { - return nil, errio.Error(err) + for _, account := range accounts { + _, ok := encryptedNamesMap[account.AccountID] + if !ok { + encryptedName, err := encryptNameForAccount(secretName, account) + if err != nil { + return nil, err + } + encryptedNamesMap[account.AccountID] = encryptedName + } + + _, ok = encryptedKeysMap[account.AccountID] + if !ok { + encryptedKey, err := encryptKeyForAccount(secretKey, account) + if err != nil { + return nil, err + } + encryptedKeysMap[account.AccountID] = encryptedKey + } + } + + encryptedNames := make([]api.EncryptedNameRequest, len(encryptedNamesMap)) + i := 0 + for _, encryptedName := range encryptedNamesMap { + encryptedNames[i] = encryptedName + i++ + } + + encryptedKeys := make([]api.EncryptedKeyRequest, len(encryptedKeysMap)) + i = 0 + for _, encryptedKey := range encryptedKeysMap { + encryptedKeys[i] = encryptedKey + i++ } in := &api.CreateSecretRequest{ @@ -264,11 +297,6 @@ func (c *Client) createSecret(secretPath api.SecretPath, data []byte) (*api.Secr EncryptedKeys: encryptedKeys, } - parentBlindName, err := c.convertPathToBlindName(parentPath) - if err != nil { - return nil, errio.Error(err) - } - resp, err := c.httpClient.CreateSecret(secretPath.GetNamespace(), secretPath.GetRepo(), parentBlindName, in) if err != nil { return nil, errio.Error(err) From 18b4ea484614aa46e88ea72564db9768cec3dc41 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 14:20:14 +0100 Subject: [PATCH 36/46] Add retry behavior for missing accounts in secret creation When an access rule is created that gives an additional account access between fetching the accounts to encrypt for and the request creating the secret, the accounts to encrypt for are now fetched again and the secret creation retried for the newly fetched accounts. --- pkg/secrethub/secret_version.go | 99 +++++++++++++++++---------------- 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/pkg/secrethub/secret_version.go b/pkg/secrethub/secret_version.go index b764226f..9dc9b46b 100644 --- a/pkg/secrethub/secret_version.go +++ b/pkg/secrethub/secret_version.go @@ -249,65 +249,70 @@ func (c *Client) createSecret(secretPath api.SecretPath, data []byte) (*api.Secr encryptedNamesMap := make(map[uuid.UUID]api.EncryptedNameRequest) encryptedKeysMap := make(map[uuid.UUID]api.EncryptedKeyRequest) - // Get all accounts that have permission to read the secret. - accounts, err := c.listDirAccounts(parentPath) - if err != nil { - return nil, errio.Error(err) - } + tries := 0 + for { + // Get all accounts that have permission to read the secret. + accounts, err := c.listDirAccounts(parentPath) + if err != nil { + return nil, errio.Error(err) + } - for _, account := range accounts { - _, ok := encryptedNamesMap[account.AccountID] - if !ok { - encryptedName, err := encryptNameForAccount(secretName, account) - if err != nil { - return nil, err + for _, account := range accounts { + _, ok := encryptedNamesMap[account.AccountID] + if !ok { + encryptedName, err := encryptNameForAccount(secretName, account) + if err != nil { + return nil, err + } + encryptedNamesMap[account.AccountID] = encryptedName } - encryptedNamesMap[account.AccountID] = encryptedName - } - _, ok = encryptedKeysMap[account.AccountID] - if !ok { - encryptedKey, err := encryptKeyForAccount(secretKey, account) - if err != nil { - return nil, err + _, ok = encryptedKeysMap[account.AccountID] + if !ok { + encryptedKey, err := encryptKeyForAccount(secretKey, account) + if err != nil { + return nil, err + } + encryptedKeysMap[account.AccountID] = encryptedKey } - encryptedKeysMap[account.AccountID] = encryptedKey } - } - encryptedNames := make([]api.EncryptedNameRequest, len(encryptedNamesMap)) - i := 0 - for _, encryptedName := range encryptedNamesMap { - encryptedNames[i] = encryptedName - i++ - } + encryptedNames := make([]api.EncryptedNameRequest, len(encryptedNamesMap)) + i := 0 + for _, encryptedName := range encryptedNamesMap { + encryptedNames[i] = encryptedName + i++ + } - encryptedKeys := make([]api.EncryptedKeyRequest, len(encryptedKeysMap)) - i = 0 - for _, encryptedKey := range encryptedKeysMap { - encryptedKeys[i] = encryptedKey - i++ - } + encryptedKeys := make([]api.EncryptedKeyRequest, len(encryptedKeysMap)) + i = 0 + for _, encryptedKey := range encryptedKeysMap { + encryptedKeys[i] = encryptedKey + i++ + } - in := &api.CreateSecretRequest{ - BlindName: blindName, - EncryptedData: encryptedData, + in := &api.CreateSecretRequest{ + BlindName: blindName, + EncryptedData: encryptedData, - EncryptedNames: encryptedNames, - EncryptedKeys: encryptedKeys, - } + EncryptedNames: encryptedNames, + EncryptedKeys: encryptedKeys, + } - resp, err := c.httpClient.CreateSecret(secretPath.GetNamespace(), secretPath.GetRepo(), parentBlindName, in) - if err != nil { - return nil, errio.Error(err) - } + resp, err := c.httpClient.CreateSecret(secretPath.GetNamespace(), secretPath.GetRepo(), parentBlindName, in) + if err == nil { + accountKey, err := c.getAccountKey() + if err != nil { + return nil, err + } - accountKey, err := c.getAccountKey() - if err != nil { - return nil, errio.Error(err) + return resp.Decrypt(accountKey) + } + if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + return nil, err + } + tries++ } - - return resp.Decrypt(accountKey) } // decryptSecretVersions decrypts EncryptedSecretVersions to a list of SecretVersions From 8d7f2982539800dd0271867cd5429ac4d406c94c Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 14:26:24 +0100 Subject: [PATCH 37/46] Refactor encryption in secret key creation Using an intermediate map to store encryption results allows for re-use of already encrypted keys when implementing retry behavior in a later commit. --- pkg/secrethub/secret_key.go | 50 +++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/pkg/secrethub/secret_key.go b/pkg/secrethub/secret_key.go index a53bbf35..497834b9 100644 --- a/pkg/secrethub/secret_key.go +++ b/pkg/secrethub/secret_key.go @@ -2,6 +2,7 @@ package secrethub import ( "github.com/secrethub/secrethub-go/internals/api" + "github.com/secrethub/secrethub-go/internals/api/uuid" "github.com/secrethub/secrethub-go/internals/crypto" "github.com/secrethub/secrethub-go/internals/errio" ) @@ -38,39 +39,50 @@ func (c *Client) createSecretKey(secretPath api.SecretPath) (*api.SecretKey, err return nil, errio.Error(err) } + blindName, err := c.convertPathToBlindName(secretPath) + if err != nil { + return nil, errio.Error(err) + } + + encryptedKeysMap := make(map[uuid.UUID]api.EncryptedKeyRequest) + // Get all accounts that have permission to read the secret. accounts, err := c.listDirAccounts(parentPath) if err != nil { return nil, errio.Error(err) } - encryptedFor := make([]api.EncryptedKeyRequest, len(accounts)) - for i, account := range accounts { - publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) - if err != nil { - return nil, errio.Error(err) - } - - encryptedSecretKey, err := publicKey.Wrap(secretKey.Export()) - if err != nil { - return nil, errio.Error(err) + for _, account := range accounts { + _, ok := encryptedKeysMap[account.AccountID] + if !ok { + publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) + if err != nil { + return nil, errio.Error(err) + } + + encryptedSecretKey, err := publicKey.Wrap(secretKey.Export()) + if err != nil { + return nil, errio.Error(err) + } + + encryptedKeysMap[account.AccountID] = api.EncryptedKeyRequest{ + AccountID: account.AccountID, + EncryptedKey: encryptedSecretKey, + } } + } - encryptedFor[i] = api.EncryptedKeyRequest{ - AccountID: account.AccountID, - EncryptedKey: encryptedSecretKey, - } + encryptedFor := make([]api.EncryptedKeyRequest, len(encryptedKeysMap)) + i := 0 + for _, encryptedKey := range encryptedKeysMap { + encryptedFor[i] = encryptedKey + i++ } in := &api.CreateSecretKeyRequest{ EncryptedFor: encryptedFor, } - blindName, err := c.convertPathToBlindName(secretPath) - if err != nil { - return nil, errio.Error(err) - } - resp, err := c.httpClient.CreateSecretKey(blindName, in) if err != nil { return nil, errio.Error(err) From 2b86d63b94cee3b1df970dfd10a108169fd5330b Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 14:32:21 +0100 Subject: [PATCH 38/46] Add retry behavior for missing accounts is secret key creation When an additional account is given access to a secret inbetween fetching the accounts to encrypt for and the creation of the key, the accounts to encrypt for are now fetched again and secret key creation is retried including the new accounts. --- pkg/secrethub/secret_key.go | 85 ++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/pkg/secrethub/secret_key.go b/pkg/secrethub/secret_key.go index 497834b9..500caf6c 100644 --- a/pkg/secrethub/secret_key.go +++ b/pkg/secrethub/secret_key.go @@ -46,52 +46,57 @@ func (c *Client) createSecretKey(secretPath api.SecretPath) (*api.SecretKey, err encryptedKeysMap := make(map[uuid.UUID]api.EncryptedKeyRequest) - // Get all accounts that have permission to read the secret. - accounts, err := c.listDirAccounts(parentPath) - if err != nil { - return nil, errio.Error(err) - } - - for _, account := range accounts { - _, ok := encryptedKeysMap[account.AccountID] - if !ok { - publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) - if err != nil { - return nil, errio.Error(err) - } - - encryptedSecretKey, err := publicKey.Wrap(secretKey.Export()) - if err != nil { - return nil, errio.Error(err) - } + tries := 0 + for { + // Get all accounts that have permission to read the secret. + accounts, err := c.listDirAccounts(parentPath) + if err != nil { + return nil, errio.Error(err) + } - encryptedKeysMap[account.AccountID] = api.EncryptedKeyRequest{ - AccountID: account.AccountID, - EncryptedKey: encryptedSecretKey, + for _, account := range accounts { + _, ok := encryptedKeysMap[account.AccountID] + if !ok { + publicKey, err := crypto.ImportRSAPublicKey(account.PublicKey) + if err != nil { + return nil, errio.Error(err) + } + + encryptedSecretKey, err := publicKey.Wrap(secretKey.Export()) + if err != nil { + return nil, errio.Error(err) + } + + encryptedKeysMap[account.AccountID] = api.EncryptedKeyRequest{ + AccountID: account.AccountID, + EncryptedKey: encryptedSecretKey, + } } } - } - encryptedFor := make([]api.EncryptedKeyRequest, len(encryptedKeysMap)) - i := 0 - for _, encryptedKey := range encryptedKeysMap { - encryptedFor[i] = encryptedKey - i++ - } + encryptedFor := make([]api.EncryptedKeyRequest, len(encryptedKeysMap)) + i := 0 + for _, encryptedKey := range encryptedKeysMap { + encryptedFor[i] = encryptedKey + i++ + } - in := &api.CreateSecretKeyRequest{ - EncryptedFor: encryptedFor, - } + in := &api.CreateSecretKeyRequest{ + EncryptedFor: encryptedFor, + } - resp, err := c.httpClient.CreateSecretKey(blindName, in) - if err != nil { - return nil, errio.Error(err) - } + resp, err := c.httpClient.CreateSecretKey(blindName, in) + if err == nil { + accountKey, err := c.getAccountKey() + if err != nil { + return nil, err + } - accountKey, err := c.getAccountKey() - if err != nil { - return nil, errio.Error(err) + return resp.Decrypt(accountKey) + } + if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + return nil, err + } + tries++ } - - return resp.Decrypt(accountKey) } From ddbad720fe9f7b1cde710d134bf20d1b8768c9a0 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 17:17:49 +0100 Subject: [PATCH 39/46] Change http status code for ErrNotEncryptedForAccounts to 409 I believe this more specifically reflects the issue of the error. The request conflicts with the stored state, for example existing access rules, secrets, secret keys and directories. --- internals/api/secret.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/api/secret.go b/internals/api/secret.go index 8fbab4e9..6ece303a 100644 --- a/internals/api/secret.go +++ b/internals/api/secret.go @@ -31,7 +31,7 @@ var ( ErrNoSecretMembers = errAPI.Code("no_secret_members").StatusError("no secret members added to write request", http.StatusBadRequest) ErrInvalidSecretKeyID = errAPI.Code("invalid_secret_key_id").StatusError("secret_key_id is invalid", http.StatusBadRequest) - ErrNotEncryptedForAccounts = errAPI.Code("not_encrypted_for_accounts").StatusError("missing data encrypted for accounts", http.StatusBadRequest) + ErrNotEncryptedForAccounts = errAPI.Code("not_encrypted_for_accounts").StatusError("missing data encrypted for accounts", http.StatusConflict) ErrNotUniquelyEncryptedForAccounts = errAPI.Code("not_uniquely_encrypted_for_accounts").StatusError("not uniquely encrypted for accounts", http.StatusBadRequest) ErrCannotDeleteLastSecretVersion = errAPI.Code("cannot_delete_last_version").StatusError("Cannot delete the last version of a secret", http.StatusForbidden) From c9281511ee9b3ae7604f807836fb99933ff721cf Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 17:34:29 +0100 Subject: [PATCH 40/46] Add more context to ErrNotEncryptedForAccounts error message --- internals/api/secret.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/api/secret.go b/internals/api/secret.go index 6ece303a..52836007 100644 --- a/internals/api/secret.go +++ b/internals/api/secret.go @@ -31,7 +31,7 @@ var ( ErrNoSecretMembers = errAPI.Code("no_secret_members").StatusError("no secret members added to write request", http.StatusBadRequest) ErrInvalidSecretKeyID = errAPI.Code("invalid_secret_key_id").StatusError("secret_key_id is invalid", http.StatusBadRequest) - ErrNotEncryptedForAccounts = errAPI.Code("not_encrypted_for_accounts").StatusError("missing data encrypted for accounts", http.StatusConflict) + ErrNotEncryptedForAccounts = errAPI.Code("not_encrypted_for_accounts").StatusError("missing data encrypted for accounts. This can occur when access rules are simultaneously created with resources controlled by the access rule. You may try again.", http.StatusConflict) ErrNotUniquelyEncryptedForAccounts = errAPI.Code("not_uniquely_encrypted_for_accounts").StatusError("not uniquely encrypted for accounts", http.StatusBadRequest) ErrCannotDeleteLastSecretVersion = errAPI.Code("cannot_delete_last_version").StatusError("Cannot delete the last version of a secret", http.StatusForbidden) From 349df293c321e20a4e828743c9e9d1b7aea72e8c Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Thu, 28 Jan 2021 17:34:57 +0100 Subject: [PATCH 41/46] Return more specific errors when members are missing --- pkg/secrethub/acl.go | 7 ++++++- pkg/secrethub/dir.go | 6 +++++- pkg/secrethub/secret_key.go | 7 ++++++- pkg/secrethub/secret_version.go | 5 ++++- 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index a91fdba5..6664669d 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -1,6 +1,8 @@ package secrethub import ( + "fmt" + "github.com/secrethub/secrethub-go/internals/api" "github.com/secrethub/secrethub-go/internals/api/uuid" "github.com/secrethub/secrethub-go/internals/errio" @@ -245,9 +247,12 @@ func (s accessRuleService) create(path api.BlindNamePath, permission api.Permiss } accessRule, err := s.client.httpClient.CreateAccessRule(blindName, accountName, in) - if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + if err != api.ErrNotEncryptedForAccounts { return accessRule, err } + if tries >= missingMemberRetries { + return nil, fmt.Errorf("cannot create access rule: resources controlled by the access rule are simultaneously being created; you may try again") + } tries++ } } diff --git a/pkg/secrethub/dir.go b/pkg/secrethub/dir.go index 2cdc4360..d4289d96 100644 --- a/pkg/secrethub/dir.go +++ b/pkg/secrethub/dir.go @@ -1,6 +1,7 @@ package secrethub import ( + "fmt" "strings" "github.com/secrethub/secrethub-go/internals/api" @@ -172,9 +173,12 @@ func (s dirService) Create(path string) (*api.Dir, error) { dir, err := encryptedDir.Decrypt(accountKey) return dir, errio.Error(err) } - if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + if err != api.ErrNotEncryptedForAccounts { return nil, err } + if tries >= missingMemberRetries { + return nil, fmt.Errorf("cannot create directory: access rules giving access to the directory are simultaneously being created; you may try again") + } tries++ } } diff --git a/pkg/secrethub/secret_key.go b/pkg/secrethub/secret_key.go index 500caf6c..2d046602 100644 --- a/pkg/secrethub/secret_key.go +++ b/pkg/secrethub/secret_key.go @@ -1,6 +1,8 @@ package secrethub import ( + "fmt" + "github.com/secrethub/secrethub-go/internals/api" "github.com/secrethub/secrethub-go/internals/api/uuid" "github.com/secrethub/secrethub-go/internals/crypto" @@ -94,9 +96,12 @@ func (c *Client) createSecretKey(secretPath api.SecretPath) (*api.SecretKey, err return resp.Decrypt(accountKey) } - if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + if err != api.ErrNotEncryptedForAccounts { return nil, err } + if tries >= missingMemberRetries { + return nil, fmt.Errorf("cannot create secret key: access rules giving access to the secret (key) are simultaneously being created; you may try again") + } tries++ } } diff --git a/pkg/secrethub/secret_version.go b/pkg/secrethub/secret_version.go index 9dc9b46b..eb15637e 100644 --- a/pkg/secrethub/secret_version.go +++ b/pkg/secrethub/secret_version.go @@ -308,9 +308,12 @@ func (c *Client) createSecret(secretPath api.SecretPath, data []byte) (*api.Secr return resp.Decrypt(accountKey) } - if err != api.ErrNotEncryptedForAccounts || tries >= missingMemberRetries { + if err != api.ErrNotEncryptedForAccounts { return nil, err } + if tries >= missingMemberRetries { + return nil, fmt.Errorf("cannot create secret version: access rules giving access to the secret are simultaneously being created; you may try again") + } tries++ } } From fc3714e65ed8650164c4a01328ce2d6a2de247ac Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 2 Feb 2021 16:34:25 +0100 Subject: [PATCH 42/46] Drop client.Users().Create Signups are not intended to be scripted. The CLI does not require this function anymore, as CLI signups are dropped in favor of web-signups in https://github.com/secrethub/secrethub-cli/pull/368 --- pkg/secrethub/fakeclient/user.go | 11 +-- pkg/secrethub/internals/http/client.go | 11 +-- pkg/secrethub/user.go | 64 -------------- pkg/secrethub/user_test.go | 117 ------------------------- 4 files changed, 3 insertions(+), 200 deletions(-) diff --git a/pkg/secrethub/fakeclient/user.go b/pkg/secrethub/fakeclient/user.go index 2952290c..06cc551f 100644 --- a/pkg/secrethub/fakeclient/user.go +++ b/pkg/secrethub/fakeclient/user.go @@ -4,14 +4,12 @@ package fakeclient import ( "github.com/secrethub/secrethub-go/internals/api" - "github.com/secrethub/secrethub-go/pkg/secrethub/credentials" ) // UserService is a mock of the UserService interface. type UserService struct { - GetFunc func(username string) (*api.User, error) - MeFunc func() (*api.User, error) - CreateFunc func(username, email, fullName string, credentialCreator credentials.Creator) (*api.User, error) + GetFunc func(username string) (*api.User, error) + MeFunc func() (*api.User, error) } // Get implements the UserService interface Get function. @@ -23,8 +21,3 @@ func (s *UserService) Get(username string) (*api.User, error) { func (s *UserService) Me() (*api.User, error) { return s.MeFunc() } - -// Create implements the UserService interface Create function. -func (s *UserService) Create(username, email, fullName string, credentialCreator credentials.CreatorProvider) (*api.User, error) { - return s.CreateFunc(username, email, fullName, credentialCreator) -} diff --git a/pkg/secrethub/internals/http/client.go b/pkg/secrethub/internals/http/client.go index c3b6256c..a93e07a6 100644 --- a/pkg/secrethub/internals/http/client.go +++ b/pkg/secrethub/internals/http/client.go @@ -48,8 +48,7 @@ const ( pathCreateAccountKey = "%s/me/credentials/%s/key" // Users - pathUsers = "%s/users" - pathUser = "%s/users/%s" + pathUser = "%s/users/%s" // Repositories pathRepos = "%s/namespaces/%s/repos" @@ -217,14 +216,6 @@ func (c *Client) GetAccount(name api.AccountName) (*api.Account, error) { // USERS -// SignupUser creates a new user at SecretHub -func (c *Client) SignupUser(in *api.CreateUserRequest) (*api.User, error) { - out := &api.User{} - rawURL := fmt.Sprintf(pathUsers, c.base.String()) - err := c.post(rawURL, false, http.StatusCreated, in, out) - return out, errio.Error(err) -} - // GetUser gets a user by its username from SecretHub func (c *Client) GetUser(username string) (*api.User, error) { out := &api.User{} diff --git a/pkg/secrethub/user.go b/pkg/secrethub/user.go index ffd28882..c9247a6a 100644 --- a/pkg/secrethub/user.go +++ b/pkg/secrethub/user.go @@ -9,8 +9,6 @@ import ( // UserService handles operations on users from SecretHub. type UserService interface { - // Create creates a new user at SecretHub. - Create(username, email, fullName string, credential credentials.CreatorProvider) (*api.User, error) // Me gets the account's user if it exists. Me() (*api.User, error) // Get a user by their username. @@ -32,68 +30,6 @@ func (s userService) Me() (*api.User, error) { return s.client.httpClient.GetMyUser() } -// Create creates a new user at SecretHub and authenticates the client as this user. -func (s userService) Create(username, email, fullName string, credentials credentials.CreatorProvider) (*api.User, error) { - err := api.ValidateUsername(username) - if err != nil { - return nil, errio.Error(err) - } - - err = api.ValidateEmail(email) - if err != nil { - return nil, errio.Error(err) - } - - err = api.ValidateFullName(fullName) - if err != nil { - return nil, errio.Error(err) - } - - err = credentials.Create() - if err != nil { - return nil, err - } - - accountKey, err := generateAccountKey() - if err != nil { - return nil, errio.Error(err) - } - - return s.create(username, email, fullName, accountKey, credentials.Verifier(), credentials.Encrypter(), credentials.Metadata(), credentials) -} - -func (s userService) create(username, email, fullName string, accountKey crypto.RSAPrivateKey, verifier credentials.Verifier, encrypter credentials.Encrypter, metadata map[string]string, credentials credentials.Provider) (*api.User, error) { - credentialRequest, err := s.client.createCredentialRequest(encrypter, accountKey, verifier, metadata) - if err != nil { - return nil, errio.Error(err) - } - - err = credentialRequest.Validate() - if err != nil { - return nil, err - } - - userRequest := &api.CreateUserRequest{ - Username: username, - Email: email, - FullName: fullName, - Credential: credentialRequest, - } - - user, err := s.client.httpClient.SignupUser(userRequest) - if err != nil { - return nil, errio.Error(err) - } - - // Authenticate the client with the new credential. - err = WithCredentials(credentials)(s.client) - if err != nil { - return nil, err - } - - return user, nil -} - // Get retrieves the user with the given username from SecretHub. func (s userService) Get(username string) (*api.User, error) { err := api.ValidateUsername(username) diff --git a/pkg/secrethub/user_test.go b/pkg/secrethub/user_test.go index fdf07ce6..b9573f93 100644 --- a/pkg/secrethub/user_test.go +++ b/pkg/secrethub/user_test.go @@ -11,7 +11,6 @@ import ( "github.com/secrethub/secrethub-go/internals/api/uuid" "github.com/secrethub/secrethub-go/internals/assert" - "github.com/secrethub/secrethub-go/internals/crypto" ) const ( @@ -20,122 +19,6 @@ const ( email = "dev1@testing.com" ) -func TestSignup(t *testing.T) { - - // Arrange - router, opts, cleanup := setup() - defer cleanup() - - userService := userService{ - client: Must(NewClient(opts...)), - } - - accountKey, err := crypto.GenerateRSAPrivateKey(512) - assert.OK(t, err) - - publicAccountKey, err := accountKey.Public().Encode() - assert.OK(t, err) - - expectedCreateUserRequest := api.CreateUserRequest{ - Username: username, - FullName: fullName, - Email: email, - Credential: &api.CreateCredentialRequest{ - Type: api.CredentialTypeKey, - Fingerprint: cred1Fingerprint, - Verifier: cred1Verifier, - Proof: &api.CredentialProofKey{}, - AccountKey: &api.CreateAccountKeyRequest{ - PublicKey: publicAccountKey, - }, - }, - } - - now := time.Now().UTC() - expectedResponse := &api.User{ - AccountID: uuid.New(), - PublicKey: publicAccountKey, - Username: username, - FullName: fullName, - Email: email, - CreatedAt: &now, - LastLoginAt: &now, - } - router.Post("/users", func(w http.ResponseWriter, r *http.Request) { - // Assert - req := new(api.CreateUserRequest) - err := json.NewDecoder(r.Body).Decode(&req) - assert.OK(t, err) - - assert.OK(t, req.Validate()) - - // We cannot predict the output of the encrypted key, therefore we do not test it here. - req.Credential.AccountKey.EncryptedPrivateKey = nil - assert.Equal(t, req, expectedCreateUserRequest) - - // Respond - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _ = json.NewEncoder(w).Encode(expectedResponse) - }) - - // Act - actual, err := userService.create(username, email, fullName, accountKey, cred1, cred1, nil, cred1) - - // Assert - assert.OK(t, err) - assert.Equal(t, actual, expectedResponse) -} - -func TestSignup_AlreadyExists(t *testing.T) { - - // Arrange - router, opts, cleanup := setup() - defer cleanup() - - userService := userService{ - client: Must(NewClient(opts...)), - } - - expected := api.ErrUserEmailAlreadyExists - - router.Post("/users", func(w http.ResponseWriter, r *http.Request) { - // Respond - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(expected.StatusCode) - _ = json.NewEncoder(w).Encode(expected) - }) - - key, err := crypto.GenerateRSAPrivateKey(512) - assert.OK(t, err) - - // Act - _, err = userService.create("dev1", "dev1@testing.com", "Developer Uno", key, cred1, cred1, nil, cred1) - - // Assert - assert.Equal(t, err, expected) -} - -func TestSignup_InvalidArgument(t *testing.T) { - - // Arrange - _, opts, cleanup := setup() - defer cleanup() - - userService := userService{ - client: Must(NewClient(opts...)), - } - - key, err := crypto.GenerateRSAPrivateKey(512) - assert.OK(t, err) - - // Act - _, err = userService.create("invalidname$#@%%", "dev1@testing.com", "Developer Uno", key, cred1, cred1, nil, cred1) - - // Assert - assert.Equal(t, err, api.ErrInvalidUsername) -} - func TestGetUser(t *testing.T) { // Arrange From 8b04bbc8688745d6fb27fe5493e6f23b6bc7f169 Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Tue, 2 Feb 2021 17:27:45 +0100 Subject: [PATCH 43/46] Remove unused CreateUserRequest structs --- internals/api/user.go | 53 -------------------------- internals/api/user_test.go | 77 -------------------------------------- 2 files changed, 130 deletions(-) diff --git a/internals/api/user.go b/internals/api/user.go index 95fcead1..5099fecc 100644 --- a/internals/api/user.go +++ b/internals/api/user.go @@ -85,56 +85,3 @@ func (u User) ToAuditActor() *AuditActor { User: u.Trim(), } } - -// CreateUserRequest contains the required fields for signing up -type CreateUserRequest struct { - Username string `json:"username"` - Email string `json:"email"` - FullName string `json:"full_name"` - Password string `json:"password,omitempty"` - Credential *CreateCredentialRequest `json:"credential,omitempty"` -} - -// Validate validates the request fields. -func (req *CreateUserRequest) Validate() error { - err := ValidateUsername(req.Username) - if err != nil { - return err - } - - if req.Credential == nil && req.Password == "" { - return ErrNoPasswordNorCredential - } - - if req.Credential != nil { - err = req.Credential.Validate() - if err != nil { - return err - } - } - - err = ValidateEmail(req.Email) - if err != nil { - return err - } - - err = ValidateFullName(req.FullName) - if err != nil { - return err - } - return nil -} - -// CreateFederatedUserRequest contains the required fields for signing up with a federated user -type CreateFederatedUserRequest struct { - Username string `json:"username"` -} - -// Validate validates the request fields. -func (req CreateFederatedUserRequest) Validate() error { - err := ValidateUsername(req.Username) - if err != nil { - return err - } - return nil -} diff --git a/internals/api/user_test.go b/internals/api/user_test.go index feef638a..043c6a2b 100644 --- a/internals/api/user_test.go +++ b/internals/api/user_test.go @@ -4,8 +4,6 @@ import ( "fmt" "strings" "testing" - - "github.com/secrethub/secrethub-go/internals/assert" ) func TestValidateUsername(t *testing.T) { @@ -168,78 +166,3 @@ func TestValidateFullName(t *testing.T) { } } } - -func TestCreateUserRequest_Validate(t *testing.T) { - cases := map[string]struct { - req CreateUserRequest - err error - }{ - "valid using password": { - req: CreateUserRequest{ - Username: "test.-_UserTestT", - Email: "test-account.dev1@secrethub.io", - FullName: "Test Tester", - Password: "hello world", - }, - err: nil, - }, - "valid using credential": { - req: CreateUserRequest{ - Username: "test.-_UserTestT", - Email: "test-account.dev1@secrethub.io", - FullName: "Test Tester", - Credential: &CreateCredentialRequest{ - Type: CredentialTypeKey, - Fingerprint: "88c9eae68eb300b2971a2bec9e5a26ff4179fd661d6b7d861e4c6557b9aaee14", - Verifier: []byte("verifier"), - }, - }, - err: nil, - }, - "invalid no password nor credential": { - req: CreateUserRequest{ - Username: "test.-_UserTestT", - Email: "test-account.dev1@secrethub.io", - FullName: "Test Tester", - }, - err: ErrNoPasswordNorCredential, - }, - "invalid username": { - req: CreateUserRequest{ - Username: "", - Email: "test-account.dev1@secrethub.io", - FullName: "Test Tester", - Password: "hello world", - }, - err: ErrInvalidUsername, - }, - "invalid email": { - req: CreateUserRequest{ - Username: "test", - Email: "notanemail", - FullName: "Test Tester", - Password: "hello world", - }, - err: ErrInvalidEmail, - }, - "invalid full name": { - req: CreateUserRequest{ - Username: "test", - Email: "test-account.dev1@secrethub.io", - FullName: "", - Password: "hello world", - }, - err: ErrInvalidFullName, - }, - } - - for name, tc := range cases { - t.Run(name, func(t *testing.T) { - // Do - err := tc.req.Validate() - - // Assert - assert.Equal(t, err, tc.err) - }) - } -} From 27ab731148fa6ba5d022394d865c026dbdb3369c Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Wed, 3 Feb 2021 11:03:21 +0100 Subject: [PATCH 44/46] Refactor: Explicitly handle err==nil case Previously, the err==nil case was handled in the err!=api.ErrNotEncryptedForAccounts block. We prefer the readability of handling both cases separately, as it's easier to see this includes the nil case. This comes at the cost of consiseness and a small performance penalty of introducing another conditional. We value the readability more. --- pkg/secrethub/acl.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index 6664669d..3ec3412c 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -247,8 +247,11 @@ func (s accessRuleService) create(path api.BlindNamePath, permission api.Permiss } accessRule, err := s.client.httpClient.CreateAccessRule(blindName, accountName, in) + if err == nil { + return accessRule, nil + } if err != api.ErrNotEncryptedForAccounts { - return accessRule, err + return nil, err } if tries >= missingMemberRetries { return nil, fmt.Errorf("cannot create access rule: resources controlled by the access rule are simultaneously being created; you may try again") From 2c0a836a90b89d31698c0c731ac1927748578bdf Mon Sep 17 00:00:00 2001 From: Simon Barendse Date: Wed, 3 Feb 2021 11:49:13 +0100 Subject: [PATCH 45/46] Make err comparisons indepedent of message and HTTP status code In an earlier commit, I've changed the message and HTTP status code of the api.ErrNotEncryptedForAccounts error. This commit makes the error checks compatible with both versions of the error and with any future changes that may be made. --- internals/errio/errors.go | 11 +++++++++++ pkg/secrethub/acl.go | 2 +- pkg/secrethub/dir.go | 2 +- pkg/secrethub/secret_key.go | 2 +- pkg/secrethub/secret_version.go | 2 +- 5 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internals/errio/errors.go b/internals/errio/errors.go index 77b348b1..e3d084b9 100644 --- a/internals/errio/errors.go +++ b/internals/errio/errors.go @@ -270,3 +270,14 @@ func Equals(a PublicError, b error) bool { } return a.Namespace == publicError.Namespace && a.Code == publicError.Code } + +// EqualsAPIError checks whether the given error has the namespace and code of the +// given API error. The HTTP status code and error message aren't checked, so this +// function is compatible with any changes to the message and HTTP status code. +func EqualsAPIError(apiErr PublicStatusError, err error) bool { + publicStatusError, ok := err.(PublicStatusError) + if !ok { + return false + } + return Equals(apiErr.PublicError, publicStatusError.PublicError) +} diff --git a/pkg/secrethub/acl.go b/pkg/secrethub/acl.go index 3ec3412c..ba69da9e 100644 --- a/pkg/secrethub/acl.go +++ b/pkg/secrethub/acl.go @@ -250,7 +250,7 @@ func (s accessRuleService) create(path api.BlindNamePath, permission api.Permiss if err == nil { return accessRule, nil } - if err != api.ErrNotEncryptedForAccounts { + if !errio.EqualsAPIError(api.ErrNotEncryptedForAccounts, err) { return nil, err } if tries >= missingMemberRetries { diff --git a/pkg/secrethub/dir.go b/pkg/secrethub/dir.go index d4289d96..f174f83b 100644 --- a/pkg/secrethub/dir.go +++ b/pkg/secrethub/dir.go @@ -173,7 +173,7 @@ func (s dirService) Create(path string) (*api.Dir, error) { dir, err := encryptedDir.Decrypt(accountKey) return dir, errio.Error(err) } - if err != api.ErrNotEncryptedForAccounts { + if !errio.EqualsAPIError(api.ErrNotEncryptedForAccounts, err) { return nil, err } if tries >= missingMemberRetries { diff --git a/pkg/secrethub/secret_key.go b/pkg/secrethub/secret_key.go index 2d046602..b4581e93 100644 --- a/pkg/secrethub/secret_key.go +++ b/pkg/secrethub/secret_key.go @@ -96,7 +96,7 @@ func (c *Client) createSecretKey(secretPath api.SecretPath) (*api.SecretKey, err return resp.Decrypt(accountKey) } - if err != api.ErrNotEncryptedForAccounts { + if !errio.EqualsAPIError(api.ErrNotEncryptedForAccounts, err) { return nil, err } if tries >= missingMemberRetries { diff --git a/pkg/secrethub/secret_version.go b/pkg/secrethub/secret_version.go index eb15637e..9ffed43d 100644 --- a/pkg/secrethub/secret_version.go +++ b/pkg/secrethub/secret_version.go @@ -308,7 +308,7 @@ func (c *Client) createSecret(secretPath api.SecretPath, data []byte) (*api.Secr return resp.Decrypt(accountKey) } - if err != api.ErrNotEncryptedForAccounts { + if !errio.EqualsAPIError(api.ErrNotEncryptedForAccounts, err) { return nil, err } if tries >= missingMemberRetries { From e3b147dfc6c35af07a07a07f4d975db58e897d28 Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Thu, 4 Feb 2021 11:20:26 +0000 Subject: [PATCH 46/46] Update version to v0.32.0 in pkg/secrethub/client_version.go --- pkg/secrethub/client_version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrethub/client_version.go b/pkg/secrethub/client_version.go index 94c736ed..fd456f5b 100644 --- a/pkg/secrethub/client_version.go +++ b/pkg/secrethub/client_version.go @@ -2,4 +2,4 @@ package secrethub // ClientVersion is the current version of the client // Do not edit this unless you know what you're doing. -const ClientVersion = "v0.31.0" +const ClientVersion = "v0.32.0"