From 8499207620890c6fd6ac9c4cdc16ef2eb26509a4 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Thu, 3 Oct 2019 22:12:51 +0200 Subject: [PATCH 01/12] Change password in a database for a user --- pkg/deployment/bootstrap.go | 13 +++------ pkg/deployment/informers.go | 7 +++-- pkg/deployment/user.go | 56 +++++++++++++++++++++++++++++++++++++ pkg/util/k8sutil/names.go | 6 ++-- pkg/util/k8sutil/secrets.go | 14 ++++++---- 5 files changed, 76 insertions(+), 20 deletions(-) create mode 100644 pkg/deployment/user.go diff --git a/pkg/deployment/bootstrap.go b/pkg/deployment/bootstrap.go index 03839fe18..9716548b2 100644 --- a/pkg/deployment/bootstrap.go +++ b/pkg/deployment/bootstrap.go @@ -32,9 +32,7 @@ import ( "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - driver "github.com/arangodb/go-driver" - - "github.com/arangodb/kube-arangodb/pkg/util/constants" + "github.com/arangodb/go-driver" ) const ( @@ -91,12 +89,9 @@ func (d *Deployment) ensureUserPasswordSecret(secrets k8sutil.SecretInterface, u return token, nil } else if err == nil { - user, ok := auth.Data[constants.SecretUsername] - if ok && string(user) == username { - pass, ok := auth.Data[constants.SecretPassword] - if ok { - return string(pass), nil - } + user, pass, err := k8sutil.GetSecretAuthCredentials(auth) + if err == nil && user == username { + return pass, nil } return "", fmt.Errorf("invalid secret format in secret %s", secretName) } else { diff --git a/pkg/deployment/informers.go b/pkg/deployment/informers.go index 0a7fc172e..d2d26c5f8 100644 --- a/pkg/deployment/informers.go +++ b/pkg/deployment/informers.go @@ -24,7 +24,7 @@ package deployment import ( "k8s.io/api/core/v1" - v1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/client-go/tools/cache" "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" @@ -143,7 +143,10 @@ func (d *Deployment) listenForSecretEvents(stopCh <-chan struct{}) { } }, UpdateFunc: func(oldObj, newObj interface{}) { - if _, ok := getSecret(newObj); ok { + if newSecret, ok := getSecret(newObj); ok { + if oldSecret, ok := oldObj.(*v1.Secret); ok { + d.ChangeUserPassword(oldSecret, newSecret) + } d.triggerInspection() } }, diff --git a/pkg/deployment/user.go b/pkg/deployment/user.go new file mode 100644 index 000000000..c4159c313 --- /dev/null +++ b/pkg/deployment/user.go @@ -0,0 +1,56 @@ +package deployment + +import ( + "context" + + "github.com/arangodb/go-driver" + "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" + "k8s.io/api/core/v1" +) + +// ChangeUserPassword changes password in the database for a user according to the new secret +func (d *Deployment) ChangeUserPassword(old *v1.Secret, new *v1.Secret) error { + + oldUser, oldPass, err := k8sutil.GetSecretAuthCredentials(old) + if err != nil { + return nil + } + + newUser, newPass, err := k8sutil.GetSecretAuthCredentials(new) + if err != nil { + return nil + } + + if oldUser != newUser { + // TODO Is it not possible to change username?. What we should do here? + return nil + } + + if oldPass == newPass { + // Password has not been changed + return nil + } + + // TODO Below when error occurs then passwords in secret and database are different + // so maybe we should restore old password in the secret? + ctx := context.Background() + client, err := d.clientCache.GetDatabase(ctx) + if err != nil { + return maskAny(err) + } + + user, err := client.User(ctx, oldUser) + if err != nil { + if driver.IsNotFound(err) { + // TODO should we delete secret if there is no user in the database? + return nil + } + return err + } + + err = user.Update(ctx, driver.UserOptions{ + Password: newPass, + }) + + return maskAny(err) +} diff --git a/pkg/util/k8sutil/names.go b/pkg/util/k8sutil/names.go index b026667b9..694653062 100644 --- a/pkg/util/k8sutil/names.go +++ b/pkg/util/k8sutil/names.go @@ -74,11 +74,9 @@ func stripArangodPrefix(id string) string { // complies with kubernetes name requirements. // If the name is to long or contains invalid characters, // if will be adjusted and a hash with be added. -func FixupResourceName(name string, maxLength ...int) string { +func FixupResourceName(name string) string { maxLen := 63 - if len(maxLength) > 0 { - maxLen = maxLength[0] - } + sb := strings.Builder{} needHash := len(name) > maxLen for _, ch := range name { diff --git a/pkg/util/k8sutil/secrets.go b/pkg/util/k8sutil/secrets.go index 0a510dd61..e82c6d93a 100644 --- a/pkg/util/k8sutil/secrets.go +++ b/pkg/util/k8sutil/secrets.go @@ -302,14 +302,18 @@ func GetBasicAuthSecret(secrets SecretInterface, secretName string) (string, str if err != nil { return "", "", maskAny(err) } - // Load `ca.crt` field - username, found := s.Data[constants.SecretUsername] + return GetSecretAuthCredentials(s) +} + +// GetSecretAuthCredentials returns username and password from the secret +func GetSecretAuthCredentials(secret *v1.Secret) (string, string, error) { + username, found := secret.Data[constants.SecretUsername] if !found { - return "", "", maskAny(fmt.Errorf("No '%s' found in secret '%s'", constants.SecretUsername, secretName)) + return "", "", maskAny(fmt.Errorf("No '%s' found in secret '%s'", constants.SecretUsername, secret.Name)) } - password, found := s.Data[constants.SecretPassword] + password, found := secret.Data[constants.SecretPassword] if !found { - return "", "", maskAny(fmt.Errorf("No '%s' found in secret '%s'", constants.SecretPassword, secretName)) + return "", "", maskAny(fmt.Errorf("No '%s' found in secret '%s'", constants.SecretPassword, secret.Name)) } return string(username), string(password), nil } From d605d58a003fca80233b9719ae27a5500fdf584c Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Fri, 4 Oct 2019 08:45:56 +0200 Subject: [PATCH 02/12] Add unit tests --- pkg/deployment/user.go | 14 +++---- pkg/deployment/user_test.go | 80 +++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 7 deletions(-) create mode 100644 pkg/deployment/user_test.go diff --git a/pkg/deployment/user.go b/pkg/deployment/user.go index c4159c313..4aefa44ff 100644 --- a/pkg/deployment/user.go +++ b/pkg/deployment/user.go @@ -5,28 +5,28 @@ import ( "github.com/arangodb/go-driver" "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" ) // ChangeUserPassword changes password in the database for a user according to the new secret func (d *Deployment) ChangeUserPassword(old *v1.Secret, new *v1.Secret) error { - oldUser, oldPass, err := k8sutil.GetSecretAuthCredentials(old) + oldUsername, oldPassword, err := k8sutil.GetSecretAuthCredentials(old) if err != nil { return nil } - newUser, newPass, err := k8sutil.GetSecretAuthCredentials(new) + username, password, err := k8sutil.GetSecretAuthCredentials(new) if err != nil { return nil } - if oldUser != newUser { + if oldUsername != username { // TODO Is it not possible to change username?. What we should do here? return nil } - if oldPass == newPass { + if oldPassword == password { // Password has not been changed return nil } @@ -39,7 +39,7 @@ func (d *Deployment) ChangeUserPassword(old *v1.Secret, new *v1.Secret) error { return maskAny(err) } - user, err := client.User(ctx, oldUser) + user, err := client.User(ctx, username) if err != nil { if driver.IsNotFound(err) { // TODO should we delete secret if there is no user in the database? @@ -49,7 +49,7 @@ func (d *Deployment) ChangeUserPassword(old *v1.Secret, new *v1.Secret) error { } err = user.Update(ctx, driver.UserOptions{ - Password: newPass, + Password: password, }) return maskAny(err) diff --git a/pkg/deployment/user_test.go b/pkg/deployment/user_test.go new file mode 100644 index 000000000..1d289237d --- /dev/null +++ b/pkg/deployment/user_test.go @@ -0,0 +1,80 @@ +package deployment + +import ( + "testing" + + "github.com/arangodb/kube-arangodb/pkg/util/constants" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + v1 "k8s.io/api/core/v1" +) + +func TestDeployment_ChangeUserPassword(t *testing.T) { + // Arrange + testCases := []struct { + Name string + OldUsername string + OldPassword string + NewUsername string + NewPassword string + ExpectedErr error + }{ + { + Name: "Old secret without credentials", + }, + { + Name: "New secret without credentials", + OldUsername: "root", + OldPassword: "test", + }, + { + Name: "Username has been changed", + OldUsername: "root", + OldPassword: "test", + NewUsername: "user", + NewPassword: "test", + }, + { + Name: "Old and new passwords are the same", + OldUsername: "root", + OldPassword: "test", + NewUsername: "root", + NewPassword: "test", + }, + } + + createSecret := func(username, password string) *v1.Secret { + secret := &v1.Secret{} + secret.Data = make(map[string][]byte) + if len(username) > 0 { + secret.Data[constants.SecretUsername] = []byte(username) + } + + if len(password) > 0 { + secret.Data[constants.SecretPassword] = []byte(password) + } + return secret + } + + for _, testCase := range testCases { + //nolint:scopelint + t.Run(testCase.Name, func(t *testing.T) { + // Arrange + d := Deployment{} + oldSecret := createSecret(testCase.OldUsername, testCase.OldPassword) + newSecret := createSecret(testCase.NewUsername, testCase.NewPassword) + + // Act + err := d.ChangeUserPassword(oldSecret, newSecret) + + // Assert + if testCase.ExpectedErr != nil { + assert.Error(t, testCase.ExpectedErr, err) + return + } + + require.NoError(t, err) + }) + } +} From c5bf0d76614a76db9baa8f7491b8a2ebc4159c24 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Sat, 5 Oct 2019 18:03:05 +0200 Subject: [PATCH 03/12] Change password for user when hash secret has changed --- pkg/apis/deployment/v1alpha/secret_hashes.go | 41 ++++- .../deployment/v1alpha/secret_hashes_test.go | 142 ++++++++++++++++++ .../v1alpha/zz_generated.deepcopy.go | 9 +- pkg/deployment/informers.go | 5 +- pkg/deployment/resources/secret_hashes.go | 95 ++++++++++-- pkg/deployment/user.go | 56 ------- pkg/deployment/user_test.go | 80 ---------- 7 files changed, 274 insertions(+), 154 deletions(-) create mode 100644 pkg/apis/deployment/v1alpha/secret_hashes_test.go delete mode 100644 pkg/deployment/user.go delete mode 100644 pkg/deployment/user_test.go diff --git a/pkg/apis/deployment/v1alpha/secret_hashes.go b/pkg/apis/deployment/v1alpha/secret_hashes.go index f495d4d05..7eeb982ae 100644 --- a/pkg/apis/deployment/v1alpha/secret_hashes.go +++ b/pkg/apis/deployment/v1alpha/secret_hashes.go @@ -34,11 +34,13 @@ type SecretHashes struct { TLSCA string `json:"tls-ca,omitempty"` // SyncTLSCA contains the hash of the sync.tls.caSecretName secret SyncTLSCA string `json:"sync-tls-ca,omitempty"` + // User's map contains hashes for each user + Users map[string]string `json:"users,omitempty"` } // Equal compares two SecretHashes func (sh *SecretHashes) Equal(other *SecretHashes) bool { - if sh == nil || other == nil { + if other == nil { return false } else if sh == other { return true @@ -47,5 +49,40 @@ func (sh *SecretHashes) Equal(other *SecretHashes) bool { return sh.AuthJWT == other.AuthJWT && sh.RocksDBEncryptionKey == other.RocksDBEncryptionKey && sh.TLSCA == other.TLSCA && - sh.SyncTLSCA == other.SyncTLSCA + sh.SyncTLSCA == other.SyncTLSCA && + isStringMapEqual(sh.Users, other.Users) +} + +// NewEmptySecretHashes creates new empty structure +func NewEmptySecretHashes() *SecretHashes { + sh := &SecretHashes{} + sh.Users = make(map[string]string) + return sh +} + +func isStringMapEqual(first map[string]string, second map[string]string) bool { + if first == nil && second == nil { + return true + } + + if first == nil || second == nil { + return false + } + + if len(first) != len(second) { + return false + } + + for key, valueF := range first { + valueS, ok := second[key] + if !ok { + return false + } + + if valueF != valueS { + return false + } + } + + return true } diff --git a/pkg/apis/deployment/v1alpha/secret_hashes_test.go b/pkg/apis/deployment/v1alpha/secret_hashes_test.go new file mode 100644 index 000000000..809e36557 --- /dev/null +++ b/pkg/apis/deployment/v1alpha/secret_hashes_test.go @@ -0,0 +1,142 @@ +package v1alpha + +import ( + "github.com/magiconair/properties/assert" + + "testing" +) + +func TestSecretHashes_Equal(t *testing.T) { + // Arrange + sh := SecretHashes{} + testCases := []struct { + Name string + CompareFrom *SecretHashes + CompareTo *SecretHashes + Expected bool + }{ + { + Name: "Parameter can not be nil", + CompareFrom: &SecretHashes{}, + Expected: false, + }, + { + Name: "The addresses are the same", + CompareFrom: &sh, + CompareTo: &sh, + Expected: true, + }, + { + Name: "JWT token is different", + CompareFrom: &SecretHashes{ + AuthJWT: "1", + }, + CompareTo: &SecretHashes{ + AuthJWT: "2", + }, + Expected: false, + }, + { + Name: "Users are different", + CompareFrom: &SecretHashes{ + Users: map[string]string{ + "root": "", + }, + }, + CompareTo: &SecretHashes{}, + Expected: false, + }, + { + Name: "User's table size is different", + CompareFrom: &SecretHashes{ + Users: map[string]string{ + "root": "", + }, + }, + CompareTo: &SecretHashes{ + Users: map[string]string{ + "root": "", + "user": "", + }, + }, + Expected: false, + }, + { + Name: "User's table has got different users", + CompareFrom: &SecretHashes{ + Users: map[string]string{ + "root": "", + }, + }, + CompareTo: &SecretHashes{ + Users: map[string]string{ + "user": "", + }, + }, + Expected: false, + }, + { + Name: "User's table has got different hashes for users", + CompareFrom: &SecretHashes{ + Users: map[string]string{ + "root": "123", + }, + }, + CompareTo: &SecretHashes{ + Users: map[string]string{ + "root": "1234", + }, + }, + Expected: false, + }, + { + Name: "Secret hashes are the same", + CompareFrom: &SecretHashes{ + AuthJWT: "1", + RocksDBEncryptionKey: "2", + TLSCA: "3", + SyncTLSCA: "4", + Users: map[string]string{ + "root": "123", + }, + }, + CompareTo: &SecretHashes{ + AuthJWT: "1", + RocksDBEncryptionKey: "2", + TLSCA: "3", + SyncTLSCA: "4", + Users: map[string]string{ + "root": "123", + }, + }, + Expected: true, + }, + { + Name: "Secret hashes are the same without users", + CompareFrom: &SecretHashes{ + AuthJWT: "1", + RocksDBEncryptionKey: "2", + TLSCA: "3", + SyncTLSCA: "4", + }, + CompareTo: &SecretHashes{ + AuthJWT: "1", + RocksDBEncryptionKey: "2", + TLSCA: "3", + SyncTLSCA: "4", + }, + Expected: true, + }, + } + + for _, testCase := range testCases { + //nolint:scopelint + t.Run(testCase.Name, func(t *testing.T) { + // Act + expected := testCase.CompareFrom.Equal(testCase.CompareTo) + + // Assert + assert.Equal(t, testCase.Expected, expected) + }) + } +} diff --git a/pkg/apis/deployment/v1alpha/zz_generated.deepcopy.go b/pkg/apis/deployment/v1alpha/zz_generated.deepcopy.go index beaa8cff7..f8c593311 100644 --- a/pkg/apis/deployment/v1alpha/zz_generated.deepcopy.go +++ b/pkg/apis/deployment/v1alpha/zz_generated.deepcopy.go @@ -367,7 +367,7 @@ func (in *DeploymentStatus) DeepCopyInto(out *DeploymentStatus) { if in.SecretHashes != nil { in, out := &in.SecretHashes, &out.SecretHashes *out = new(SecretHashes) - **out = **in + (*in).DeepCopyInto(*out) } if in.ForceStatusReload != nil { in, out := &in.ForceStatusReload, &out.ForceStatusReload @@ -757,6 +757,13 @@ func (in *RocksDBSpec) DeepCopy() *RocksDBSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SecretHashes) DeepCopyInto(out *SecretHashes) { *out = *in + if in.Users != nil { + in, out := &in.Users, &out.Users + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/deployment/informers.go b/pkg/deployment/informers.go index d2d26c5f8..966a4511f 100644 --- a/pkg/deployment/informers.go +++ b/pkg/deployment/informers.go @@ -143,10 +143,7 @@ func (d *Deployment) listenForSecretEvents(stopCh <-chan struct{}) { } }, UpdateFunc: func(oldObj, newObj interface{}) { - if newSecret, ok := getSecret(newObj); ok { - if oldSecret, ok := oldObj.(*v1.Secret); ok { - d.ChangeUserPassword(oldSecret, newSecret) - } + if _, ok := getSecret(newObj); ok { d.triggerInspection() } }, diff --git a/pkg/deployment/resources/secret_hashes.go b/pkg/deployment/resources/secret_hashes.go index fd78ebe98..1ec10eaed 100644 --- a/pkg/deployment/resources/secret_hashes.go +++ b/pkg/deployment/resources/secret_hashes.go @@ -23,12 +23,17 @@ package resources import ( + "context" "crypto/sha256" "encoding/hex" "fmt" "sort" "strings" + "github.com/arangodb/go-driver" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1alpha" @@ -44,10 +49,14 @@ func (r *Resources) ValidateSecretHashes() error { // validate performs a secret hash comparison for a single secret. // Return true if all is good, false when the SecretChanged condition // must be set. - validate := func(secretName string, getExpectedHash func() string, setExpectedHash func(string) error) (bool, error) { + validate := func(secretName string, + getExpectedHash func() string, + setExpectedHash func(string) error, + actionHashChanged func(Context, *v1.Secret) error) (bool, error) { + log := r.log.With().Str("secret-name", secretName).Logger() expectedHash := getExpectedHash() - hash, err := r.getSecretHash(secretName) + secret, hash, err := r.getSecretHash(secretName) if expectedHash == "" { // No hash set yet, try to fill it if k8sutil.IsNotFound(err) { @@ -78,6 +87,18 @@ func (r *Resources) ValidateSecretHashes() error { Str("expected-hash", expectedHash). Str("new-hash", hash). Msg("Secret has changed.") + if actionHashChanged != nil { + if err := actionHashChanged(r.context, secret); err != nil { + log.Debug().Msg("Failed to change secret hash") + return false, nil + } + + if err := setExpectedHash(hash); err != nil { + log.Debug().Msg("Failed to change secret hash") + return true, maskAny(err) + } + return true, nil + } // This is not good, return false so SecretsChanged condition will be set. return false, nil } @@ -91,13 +112,13 @@ func (r *Resources) ValidateSecretHashes() error { status, lastVersion := r.context.GetStatus() getHashes := func() *api.SecretHashes { if status.SecretHashes == nil { - status.SecretHashes = &api.SecretHashes{} + status.SecretHashes = api.NewEmptySecretHashes() } return status.SecretHashes } updateHashes := func(updater func(*api.SecretHashes)) error { if status.SecretHashes == nil { - status.SecretHashes = &api.SecretHashes{} + status.SecretHashes = api.NewEmptySecretHashes() } updater(status.SecretHashes) if err := r.context.UpdateStatus(status, lastVersion); err != nil { @@ -114,7 +135,7 @@ func (r *Resources) ValidateSecretHashes() error { setExpectedHash := func(h string) error { return maskAny(updateHashes(func(dst *api.SecretHashes) { dst.AuthJWT = h })) } - if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash); err != nil { + if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash, nil); err != nil { return maskAny(err) } else if !hashOK { badSecretNames = append(badSecretNames, secretName) @@ -126,7 +147,7 @@ func (r *Resources) ValidateSecretHashes() error { setExpectedHash := func(h string) error { return maskAny(updateHashes(func(dst *api.SecretHashes) { dst.RocksDBEncryptionKey = h })) } - if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash); err != nil { + if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash, nil); err != nil { return maskAny(err) } else if !hashOK { badSecretNames = append(badSecretNames, secretName) @@ -138,7 +159,7 @@ func (r *Resources) ValidateSecretHashes() error { setExpectedHash := func(h string) error { return maskAny(updateHashes(func(dst *api.SecretHashes) { dst.TLSCA = h })) } - if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash); err != nil { + if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash, nil); err != nil { return maskAny(err) } else if !hashOK { badSecretNames = append(badSecretNames, secretName) @@ -150,13 +171,36 @@ func (r *Resources) ValidateSecretHashes() error { setExpectedHash := func(h string) error { return maskAny(updateHashes(func(dst *api.SecretHashes) { dst.SyncTLSCA = h })) } - if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash); err != nil { + if hashOK, err := validate(secretName, getExpectedHash, setExpectedHash, nil); err != nil { return maskAny(err) } else if !hashOK { badSecretNames = append(badSecretNames, secretName) } } + for username, secretName := range spec.Bootstrap.PasswordSecretNames { + if secretName.IsNone() || secretName.IsAuto() { + continue + } + getExpectedHash := func() string { + if v, ok := getHashes().Users[username]; ok { + return v + } + return "" + } + setExpectedHash := func(h string) error { + return maskAny(updateHashes(func(dst *api.SecretHashes) { + dst.Users[username] = h + })) + } + hashOK, err := validate(string(secretName), getExpectedHash, setExpectedHash, changeUserPassword) + if err != nil { + return maskAny(err) + } else if !hashOK { + badSecretNames = append(badSecretNames, string(secretName)) + } + } + if len(badSecretNames) > 0 { // We have invalid hashes, set the SecretsChanged condition if status.Conditions.Update(api.ConditionTypeSecretsChanged, true, @@ -185,13 +229,42 @@ func (r *Resources) ValidateSecretHashes() error { return nil } +func changeUserPassword(c Context, secret *v1.Secret) error { + username, password, err := k8sutil.GetSecretAuthCredentials(secret) + if err != nil { + return nil + } + + ctx := context.Background() + client, err := c.GetDatabaseClient(ctx) + if err != nil { + return maskAny(err) + } + + user, err := client.User(ctx, username) + if err != nil { + if driver.IsNotFound(err) { + // TODO if there is no user in the database? + // should we delete secret and secret hash in status.secretHashes.users.? + return nil + } + return err + } + + err = user.Update(ctx, driver.UserOptions{ + Password: password, + }) + + return maskAny(err) +} + // getSecretHash fetches a secret with given name and returns a hash over its value. -func (r *Resources) getSecretHash(secretName string) (string, error) { +func (r *Resources) getSecretHash(secretName string) (*v1.Secret, string, error) { kubecli := r.context.GetKubeCli() ns := r.context.GetNamespace() s, err := kubecli.CoreV1().Secrets(ns).Get(secretName, metav1.GetOptions{}) if err != nil { - return "", maskAny(err) + return nil, "", maskAny(err) } // Create hash of value rows := make([]string, 0, len(s.Data)) @@ -203,5 +276,5 @@ func (r *Resources) getSecretHash(secretName string) (string, error) { data := strings.Join(rows, "\n") rawHash := sha256.Sum256([]byte(data)) hash := fmt.Sprintf("%0x", rawHash) - return hash, nil + return s, hash, nil } diff --git a/pkg/deployment/user.go b/pkg/deployment/user.go deleted file mode 100644 index 4aefa44ff..000000000 --- a/pkg/deployment/user.go +++ /dev/null @@ -1,56 +0,0 @@ -package deployment - -import ( - "context" - - "github.com/arangodb/go-driver" - "github.com/arangodb/kube-arangodb/pkg/util/k8sutil" - v1 "k8s.io/api/core/v1" -) - -// ChangeUserPassword changes password in the database for a user according to the new secret -func (d *Deployment) ChangeUserPassword(old *v1.Secret, new *v1.Secret) error { - - oldUsername, oldPassword, err := k8sutil.GetSecretAuthCredentials(old) - if err != nil { - return nil - } - - username, password, err := k8sutil.GetSecretAuthCredentials(new) - if err != nil { - return nil - } - - if oldUsername != username { - // TODO Is it not possible to change username?. What we should do here? - return nil - } - - if oldPassword == password { - // Password has not been changed - return nil - } - - // TODO Below when error occurs then passwords in secret and database are different - // so maybe we should restore old password in the secret? - ctx := context.Background() - client, err := d.clientCache.GetDatabase(ctx) - if err != nil { - return maskAny(err) - } - - user, err := client.User(ctx, username) - if err != nil { - if driver.IsNotFound(err) { - // TODO should we delete secret if there is no user in the database? - return nil - } - return err - } - - err = user.Update(ctx, driver.UserOptions{ - Password: password, - }) - - return maskAny(err) -} diff --git a/pkg/deployment/user_test.go b/pkg/deployment/user_test.go deleted file mode 100644 index 1d289237d..000000000 --- a/pkg/deployment/user_test.go +++ /dev/null @@ -1,80 +0,0 @@ -package deployment - -import ( - "testing" - - "github.com/arangodb/kube-arangodb/pkg/util/constants" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - v1 "k8s.io/api/core/v1" -) - -func TestDeployment_ChangeUserPassword(t *testing.T) { - // Arrange - testCases := []struct { - Name string - OldUsername string - OldPassword string - NewUsername string - NewPassword string - ExpectedErr error - }{ - { - Name: "Old secret without credentials", - }, - { - Name: "New secret without credentials", - OldUsername: "root", - OldPassword: "test", - }, - { - Name: "Username has been changed", - OldUsername: "root", - OldPassword: "test", - NewUsername: "user", - NewPassword: "test", - }, - { - Name: "Old and new passwords are the same", - OldUsername: "root", - OldPassword: "test", - NewUsername: "root", - NewPassword: "test", - }, - } - - createSecret := func(username, password string) *v1.Secret { - secret := &v1.Secret{} - secret.Data = make(map[string][]byte) - if len(username) > 0 { - secret.Data[constants.SecretUsername] = []byte(username) - } - - if len(password) > 0 { - secret.Data[constants.SecretPassword] = []byte(password) - } - return secret - } - - for _, testCase := range testCases { - //nolint:scopelint - t.Run(testCase.Name, func(t *testing.T) { - // Arrange - d := Deployment{} - oldSecret := createSecret(testCase.OldUsername, testCase.OldPassword) - newSecret := createSecret(testCase.NewUsername, testCase.NewPassword) - - // Act - err := d.ChangeUserPassword(oldSecret, newSecret) - - // Assert - if testCase.ExpectedErr != nil { - assert.Error(t, testCase.ExpectedErr, err) - return - } - - require.NoError(t, err) - }) - } -} From 491a94eb518115da6633f3b136814eb8b256ca21 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Mon, 7 Oct 2019 20:16:27 +0200 Subject: [PATCH 04/12] Add integration test for changing root password --- pkg/deployment/resources/secret_hashes.go | 13 ++- tests/secret_hashes_test.go | 108 ++++++++++++++++++++++ 2 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 tests/secret_hashes_test.go diff --git a/pkg/deployment/resources/secret_hashes.go b/pkg/deployment/resources/secret_hashes.go index 1ec10eaed..ec3153f53 100644 --- a/pkg/deployment/resources/secret_hashes.go +++ b/pkg/deployment/resources/secret_hashes.go @@ -89,7 +89,7 @@ func (r *Resources) ValidateSecretHashes() error { Msg("Secret has changed.") if actionHashChanged != nil { if err := actionHashChanged(r.context, secret); err != nil { - log.Debug().Msg("Failed to change secret hash") + log.Debug().Msgf("failed to change secret. hash-changed-action returned error: %v", err) return false, nil } @@ -244,9 +244,14 @@ func changeUserPassword(c Context, secret *v1.Secret) error { user, err := client.User(ctx, username) if err != nil { if driver.IsNotFound(err) { - // TODO if there is no user in the database? - // should we delete secret and secret hash in status.secretHashes.users.? - return nil + options := &driver.UserOptions{ + Password: password, + Active: new(bool), + } + *options.Active = true + + _, err = client.CreateUser(ctx, username, options) + return maskAny(err) } return err } diff --git a/tests/secret_hashes_test.go b/tests/secret_hashes_test.go new file mode 100644 index 000000000..d279bb9da --- /dev/null +++ b/tests/secret_hashes_test.go @@ -0,0 +1,108 @@ +package tests + +import ( + "context" + "fmt" + "testing" + "time" + + api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1alpha" + "github.com/arangodb/kube-arangodb/pkg/client" + "github.com/arangodb/kube-arangodb/pkg/util/arangod" + "github.com/arangodb/kube-arangodb/pkg/util/constants" + "github.com/arangodb/kube-arangodb/pkg/util/retry" + "github.com/dchest/uniuri" + "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestSecretHashesRootUser(t *testing.T) { + longOrSkip(t) + c := client.MustNewInCluster() + kubecli := mustNewKubeClient(t) + ns := getNamespace(t) + + // Prepare deployment config + depl := newDeployment("test-auth-sng-def-" + uniuri.NewLen(4)) + depl.Spec.Mode = api.NewMode(api.DeploymentModeSingle) + depl.Spec.SetDefaults(depl.GetName()) + depl.Spec.Bootstrap.PasswordSecretNames[api.UserNameRoot] = api.PasswordSecretNameAuto + + // Create deployment + apiObject, err := c.DatabaseV1alpha().ArangoDeployments(ns).Create(depl) + if err != nil { + t.Fatalf("Create deployment failed: %v", err) + } + defer deferedCleanupDeployment(c, depl.GetName(), ns) + + // Wait for deployment to be ready + depl, err = waitUntilDeployment(c, depl.GetName(), ns, deploymentIsReady()) + if err != nil { + t.Fatalf("Deployment not running in time: %v", err) + } + + // Create a database client + ctx := arangod.WithRequireAuthentication(context.Background()) + client := mustNewArangodDatabaseClient(ctx, kubecli, apiObject, t, nil) + + // Wait for single server available + if err := waitUntilVersionUp(client, nil); err != nil { + t.Fatalf("Single server not running returning version in time: %v", err) + } + + depl, err = waitUntilDeployment(c, depl.GetName(), ns, func(obj *api.ArangoDeployment) error { + // check if root secret password is set + secretHashes := obj.Status.SecretHashes + if secretHashes == nil { + return fmt.Errorf("field Status.SecretHashes is not set") + } + + if secretHashes.Users == nil { + return fmt.Errorf("field Status.SecretHashes.Users is not set") + } + + if hash, ok := secretHashes.Users[api.UserNameRoot]; !ok { + return fmt.Errorf("field Status.SecretHashes.Users[root] is not set") + } else if len(hash) == 0 { + return fmt.Errorf("field Status.SecretHashes.Users[root] is empty") + } + + return nil + }) + + if err != nil { + t.Fatalf("Deployment is not set properly: %v", err) + } + rootHashSecret := depl.Status.SecretHashes.Users[api.UserNameRoot] + + secretRootName := string(depl.Spec.Bootstrap.PasswordSecretNames[api.UserNameRoot]) + secretRoot, err := waitUntilSecret(kubecli, secretRootName, ns, nil, time.Second) + if err != nil { + t.Fatalf("Root secret '%s' not found: %v", secretRootName, err) + } + + secretRoot.Data[constants.SecretPassword] = []byte("1") + _, err = kubecli.CoreV1().Secrets(ns).Update(secretRoot) + if err != nil { + t.Fatalf("Root secret '%s' has not been changed: %v", secretRootName, err) + } + + err = retry.Retry(func() error { + // check if root secret hash has changed + depl, err = c.DatabaseV1alpha().ArangoDeployments(ns).Get(depl.GetName(), metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get deployment: %v", err) + } + + if rootHashSecret == depl.Status.SecretHashes.Users[api.UserNameRoot] { + return maskAny(errors.New("field Status.SecretHashes.Users[root] has not been changed yet")) + } + return nil + }, deploymentReadyTimeout) + if err != nil { + t.Fatalf("%v", err) + } + + //Cleanup + removeDeployment(c, depl.GetName(), ns) +} From f6844e0dd707e5066bda8fec3d7db7fa7a00f0bd Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Mon, 7 Oct 2019 22:19:54 +0200 Subject: [PATCH 05/12] Add disclaimer --- .../deployment/v1alpha/secret_hashes_test.go | 22 +++++++++++++++++++ tests/secret_hashes_test.go | 22 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/pkg/apis/deployment/v1alpha/secret_hashes_test.go b/pkg/apis/deployment/v1alpha/secret_hashes_test.go index 809e36557..6ea89f010 100644 --- a/pkg/apis/deployment/v1alpha/secret_hashes_test.go +++ b/pkg/apis/deployment/v1alpha/secret_hashes_test.go @@ -1,3 +1,25 @@ +// +// DISCLAIMER +// +// Copyright 2019 ArangoDB GmbH, Cologne, Germany +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Copyright holder is ArangoDB GmbH, Cologne, Germany +// +// Author tomasz@arangodb.con +// + package v1alpha import ( diff --git a/tests/secret_hashes_test.go b/tests/secret_hashes_test.go index d279bb9da..52cd6b2c2 100644 --- a/tests/secret_hashes_test.go +++ b/tests/secret_hashes_test.go @@ -1,3 +1,25 @@ +// +// DISCLAIMER +// +// Copyright 2019 ArangoDB GmbH, Cologne, Germany +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Copyright holder is ArangoDB GmbH, Cologne, Germany +// +// Author tomasz@arangodb.con +// + package tests import ( From fbf2aac158bcbaf066da2453685130ab778ad881 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 8 Oct 2019 06:25:02 +0200 Subject: [PATCH 06/12] Change comment --- tests/secret_hashes_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/secret_hashes_test.go b/tests/secret_hashes_test.go index 52cd6b2c2..14900875f 100644 --- a/tests/secret_hashes_test.go +++ b/tests/secret_hashes_test.go @@ -38,6 +38,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// TestSecretHashesRootUser checks if Status.SecretHashes.Users[root] changed after request for it func TestSecretHashesRootUser(t *testing.T) { longOrSkip(t) c := client.MustNewInCluster() From 023b5484c69c7bf262f88bf2d37dd64a70f4f8f9 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 8 Oct 2019 08:36:29 +0200 Subject: [PATCH 07/12] Run unit test for whole pkg/deployment/... --- Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 912902f9b..1bdb76b9b 100644 --- a/Makefile +++ b/Makefile @@ -308,8 +308,7 @@ run-unit-tests: $(SOURCES) $(REPOPATH)/pkg/apis/deployment/v1alpha \ $(REPOPATH)/pkg/apis/replication/v1alpha \ $(REPOPATH)/pkg/apis/storage/v1alpha \ - $(REPOPATH)/pkg/deployment/reconcile \ - $(REPOPATH)/pkg/deployment/resources \ + $(REPOPATH)/pkg/deployment/... $(REPOPATH)/pkg/storage \ $(REPOPATH)/pkg/util/k8sutil \ $(REPOPATH)/pkg/util/k8sutil/test \ From 892bdb555e0d8b2d6efff3d96d48fd6202d08790 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 8 Oct 2019 13:40:30 +0200 Subject: [PATCH 08/12] Check password in the database --- pkg/deployment/resources/secret_hashes.go | 2 +- tests/secret_hashes_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/deployment/resources/secret_hashes.go b/pkg/deployment/resources/secret_hashes.go index ec3153f53..5f7c15b7f 100644 --- a/pkg/deployment/resources/secret_hashes.go +++ b/pkg/deployment/resources/secret_hashes.go @@ -90,7 +90,7 @@ func (r *Resources) ValidateSecretHashes() error { if actionHashChanged != nil { if err := actionHashChanged(r.context, secret); err != nil { log.Debug().Msgf("failed to change secret. hash-changed-action returned error: %v", err) - return false, nil + return true, nil } if err := setExpectedHash(hash); err != nil { diff --git a/tests/secret_hashes_test.go b/tests/secret_hashes_test.go index 14900875f..b8958f55b 100644 --- a/tests/secret_hashes_test.go +++ b/tests/secret_hashes_test.go @@ -28,6 +28,8 @@ import ( "testing" "time" + "github.com/arangodb/go-driver" + api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1alpha" "github.com/arangodb/kube-arangodb/pkg/client" "github.com/arangodb/kube-arangodb/pkg/util/arangod" @@ -126,6 +128,17 @@ func TestSecretHashesRootUser(t *testing.T) { t.Fatalf("%v", err) } + // Check if password changed + auth := driver.BasicAuthentication(api.UserNameRoot, "1") + _, err = client.Connection().SetAuthentication(auth) + if err != nil { + t.Fatalf("The password for user '%s' has not been changed: %v", api.UserNameRoot, err) + } + _, err = client.Version(context.Background()) + if err != nil { + t.Fatalf("can not get version after the password has been changed") + } + //Cleanup removeDeployment(c, depl.GetName(), ns) } From efcff3675680741a6da4855139921825c5978d8b Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Tue, 8 Oct 2019 15:23:53 +0200 Subject: [PATCH 09/12] Fix nil receiver --- pkg/apis/deployment/v1alpha/secret_hashes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/deployment/v1alpha/secret_hashes.go b/pkg/apis/deployment/v1alpha/secret_hashes.go index 7eeb982ae..8f0143246 100644 --- a/pkg/apis/deployment/v1alpha/secret_hashes.go +++ b/pkg/apis/deployment/v1alpha/secret_hashes.go @@ -40,7 +40,7 @@ type SecretHashes struct { // Equal compares two SecretHashes func (sh *SecretHashes) Equal(other *SecretHashes) bool { - if other == nil { + if sh == nil || other == nil { return false } else if sh == other { return true From fa0e4d8cf845edc7db783ed6821f140f2eb0b657 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Wed, 9 Oct 2019 10:33:31 +0200 Subject: [PATCH 10/12] Fix removing root secret password --- pkg/deployment/bootstrap.go | 8 +++----- pkg/deployment/resources/secret_hashes.go | 9 +++------ 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/deployment/bootstrap.go b/pkg/deployment/bootstrap.go index 9716548b2..efe5325c5 100644 --- a/pkg/deployment/bootstrap.go +++ b/pkg/deployment/bootstrap.go @@ -35,10 +35,6 @@ import ( "github.com/arangodb/go-driver" ) -const ( - rootUserName = "root" -) - // EnsureBootstrap executes the bootstrap once as soon as the deployment becomes ready func (d *Deployment) EnsureBootstrap() error { @@ -79,7 +75,9 @@ func (d *Deployment) ensureUserPasswordSecret(secrets k8sutil.SecretInterface, u if auth, err := secrets.Get(secretName, metav1.GetOptions{}); k8sutil.IsNotFound(err) { // Create new one tokenData := make([]byte, 32) - rand.Read(tokenData) + if _, err = rand.Read(tokenData); err != nil { + return "", err + } token := hex.EncodeToString(tokenData) owner := d.GetAPIObject().AsOwner() diff --git a/pkg/deployment/resources/secret_hashes.go b/pkg/deployment/resources/secret_hashes.go index 5f7c15b7f..a5193e9c3 100644 --- a/pkg/deployment/resources/secret_hashes.go +++ b/pkg/deployment/resources/secret_hashes.go @@ -193,12 +193,9 @@ func (r *Resources) ValidateSecretHashes() error { dst.Users[username] = h })) } - hashOK, err := validate(string(secretName), getExpectedHash, setExpectedHash, changeUserPassword) - if err != nil { - return maskAny(err) - } else if !hashOK { - badSecretNames = append(badSecretNames, string(secretName)) - } + + // If password changes it should not be set that deployment in 'SecretsChanged' state + validate(string(secretName), getExpectedHash, setExpectedHash, changeUserPassword) } if len(badSecretNames) > 0 { From 5436870b18c814edcf526c8d2523bc38eb49e45e Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Wed, 9 Oct 2019 10:34:38 +0200 Subject: [PATCH 11/12] Fix unit test definition in Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 1bdb76b9b..ff9e8ad74 100644 --- a/Makefile +++ b/Makefile @@ -308,7 +308,7 @@ run-unit-tests: $(SOURCES) $(REPOPATH)/pkg/apis/deployment/v1alpha \ $(REPOPATH)/pkg/apis/replication/v1alpha \ $(REPOPATH)/pkg/apis/storage/v1alpha \ - $(REPOPATH)/pkg/deployment/... + $(REPOPATH)/pkg/deployment/... \ $(REPOPATH)/pkg/storage \ $(REPOPATH)/pkg/util/k8sutil \ $(REPOPATH)/pkg/util/k8sutil/test \ From 92c9bd10a9c52dd3e7d0886c1c7a6bd80269e255 Mon Sep 17 00:00:00 2001 From: Tomasz Mielech Date: Thu, 10 Oct 2019 08:55:22 +0200 Subject: [PATCH 12/12] Don't validate non-existing users' secrets --- pkg/deployment/resources/secret_hashes.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/deployment/resources/secret_hashes.go b/pkg/deployment/resources/secret_hashes.go index a5193e9c3..dcc5d43cb 100644 --- a/pkg/deployment/resources/secret_hashes.go +++ b/pkg/deployment/resources/secret_hashes.go @@ -182,6 +182,13 @@ func (r *Resources) ValidateSecretHashes() error { if secretName.IsNone() || secretName.IsAuto() { continue } + + _, err := r.context.GetKubeCli().CoreV1().Secrets(r.context.GetNamespace()).Get(string(secretName), metav1.GetOptions{}) + if k8sutil.IsNotFound(err) { + // do nothing when secret was deleted + continue + } + getExpectedHash := func() string { if v, ok := getHashes().Users[username]; ok { return v