[CLOUDP-349630] Don't generate scramshacreds if MongoDBUser's password is not changed (#628)

# Summary

When the reconciliation happens for the `MongoDBUser` resource (because
of controller restart or because of any other reason), for `SCRAM`
authentication mechanism, the scram sha creds are generated and then
they are set in the automation config which results into another version
getting created for Automation Config.
This workflow can be improved to generated the scram sha creds only when
the password of the user is changed. If we do that we will not be
changing the user's scram creds and the automation config will not be
updated.
To achieve this we are checking if the scram sha creds that are in
automation config for a user, match with the scram sha creds generated
with the stored salt and new password. If the scram sha creds match it
means that the password for the user is not changed and we don't have to
generate the new scram sha creds, otherwise new scram sha creds will be
generated.

## Proof of Work

Unit test:

```
~/work/opensource/mongodb-kubernetes/controllers/operator/authentication (dont-rotate-scram-creds-mdbu) » go test -timeout 30s -run ^Test_isPasswordChanged$ -v
=== RUN   Test_isPasswordChanged
--- PASS: Test_isPasswordChanged (0.01s)
PASS
ok      github.com/mongodb/mongodb-kubernetes/controllers/operator/authentication       0.472s
```

I am yet to do some more tests to actually be confident that the
automation config is not changed when reconciliation happens.

## Checklist

- [x] Have you linked a jira ticket and/or is the ticket in the title?
- [x] Have you checked whether your jira ticket required DOCSP changes?
- [x] Have you added changelog file?
    - use `skip-changelog` label if not needed
- refer to [Changelog files and Release
Notes](https://github.com/mongodb/mongodb-kubernetes/blob/master/CONTRIBUTING.md#changelog-files-and-release-notes)
section in CONTRIBUTING.md for more details

Author: viveksinghggits

controllers/om/automation_config.go

@@ -394,6 +394,13 @@ type ScramShaCreds struct {
 	StoredKey      string `json:"storedKey"`
 }
 
+func (s ScramShaCreds) Equals(i ScramShaCreds) bool {
+	return s.IterationCount == i.IterationCount &&
+		s.Salt == i.Salt &&
+		s.ServerKey == i.ServerKey &&
+		s.StoredKey == i.StoredKey
+}
+
 func (u *MongoDBUser) AddRole(role *Role) {
 	u.Roles = append(u.Roles, role)
 }

controllers/operator/authentication/scramsha_credentials.go

@@ -28,10 +28,57 @@ const (
 	rfc5802MandatedSaltSize = 4
 )
 
+func isPasswordChanged(user *om.MongoDBUser, password string, acUser *om.MongoDBUser) (bool, error) {
+	// if something unexpected happens return true to make sure that the scramShaCreds will be generated again.
+	if acUser == nil || acUser.ScramSha256Creds == nil || acUser.ScramSha1Creds == nil {
+		return true, nil
+	}
+
+	if acUser.ScramSha256Creds.Salt != "" && acUser.ScramSha1Creds.Salt != "" {
+		sha256Salt, err := base64.StdEncoding.DecodeString(acUser.ScramSha256Creds.Salt)
+		if err != nil {
+			return true, err
+		}
+		sha1Salt, err := base64.StdEncoding.DecodeString(acUser.ScramSha1Creds.Salt)
+		if err != nil {
+			return true, nil
+		}
+		// generate scramshacreds with (new) password but with old salt to verify if given password
+		// is actually changed
+		newScramSha256Creds, err := computeScramShaCreds(user.Username, password, sha256Salt, ScramSha256)
+		if err != nil {
+			return false, xerrors.Errorf("error generating scramSha256 creds to verify with already present user on automation config %w", err)
+		}
+
+		newScramSha1Creds, err := computeScramShaCreds(user.Username, password, sha1Salt, MongoDBCR)
+		if err != nil {
+			return false, xerrors.Errorf("error generating scramSha1 creds to verify with already present user on automation config %w", err)
+		}
+		return !newScramSha256Creds.Equals(*acUser.ScramSha256Creds) || !newScramSha1Creds.Equals(*acUser.ScramSha1Creds), nil
+	}
+
+	return true, nil
+}
+
 // ConfigureScramCredentials creates both SCRAM-SHA-1 and SCRAM-SHA-256 credentials. This ensures
 // that changes to the authentication settings on the MongoDB resources won't leave MongoDBUsers without
 // the correct credentials.
-func ConfigureScramCredentials(user *om.MongoDBUser, password string) error {
+func ConfigureScramCredentials(user *om.MongoDBUser, password string, ac *om.AutomationConfig) error {
+	// there are chances that the reconciliation is happening again for this user resource and we wouldn't
+	// want to generate scram creds again if the password of the user has not changed.
+	_, acUser := ac.Auth.GetUser(user.Username, user.Database)
+	changed, err := isPasswordChanged(user, password, acUser)
+	if err != nil {
+		return err
+	}
+	if !changed {
+		// since the scram creds generated using the old salt are same with the scram creds stored in automation config
+		// there is no need to generate new salt/creds.
+		user.ScramSha256Creds = acUser.ScramSha256Creds
+		user.ScramSha1Creds = acUser.ScramSha1Creds
+		return nil
+	}
+
 	scram256Salt, err := generateSalt(sha256.New)
 	if err != nil {
 		return xerrors.Errorf("error generating scramSha256 salt: %w", err)

controllers/operator/authentication/scramsha_credentials_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/mongodb/mongodb-kubernetes/controllers/om"
 )
 
 func TestScramSha1SecretsMatch(t *testing.T) {
@@ -23,3 +25,41 @@ func assertSecretsMatch(t *testing.T, hash func() hash.Hash, passwordHash string
 	assert.Equal(t, computedStoredKey, storedKey)
 	assert.Equal(t, computedServerKey, serverKey)
 }
+
+var testAutomationConfig = &om.AutomationConfig{
+	Auth: &om.Auth{
+		Users: make([]*om.MongoDBUser, 0),
+	},
+}
+
+func Test_isPasswordChanged(t *testing.T) {
+	userPassword := "secretpassword"
+	userNewPassword := "newsecretpassword"
+
+	mongoUser := om.MongoDBUser{
+		Username: "new-user",
+		Database: "admin",
+	}
+
+	// will generate scram creds for the user mongoUser and set it in its fields
+	ConfigureScramCredentials(&mongoUser, userPassword, testAutomationConfig)
+
+	testAutomationConfig.Auth.Users = append(testAutomationConfig.Auth.Users, &om.MongoDBUser{
+		Username:         mongoUser.Username,
+		Database:         mongoUser.Database,
+		ScramSha256Creds: mongoUser.ScramSha256Creds,
+		ScramSha1Creds:   mongoUser.ScramSha1Creds,
+	})
+
+	// now that the scram creds are set in the automation config, let's say the reconciliation happens again
+	// with the same user and same password, isPasswordChanged should return false
+	_, u := testAutomationConfig.Auth.GetUser(mongoUser.Username, mongoUser.Database)
+	op, err := isPasswordChanged(&mongoUser, userPassword, u)
+	assert.Nil(t, err)
+	assert.False(t, op)
+
+	// if reconciliation happens again with diff password, isPasswordChanged should return true
+	op, err = isPasswordChanged(&mongoUser, userNewPassword, u)
+	assert.Nil(t, err)
+	assert.True(t, op)
+}

controllers/operator/mongodbuser_controller.go

@@ -333,7 +333,7 @@ func AddMongoDBUserController(ctx context.Context, mgr manager.Manager, memberCl
 // toOmUser converts a MongoDBUser specification and optional password into an
 // automation config MongoDB user. If the user has no password then a blank
 // password should be provided.
-func toOmUser(spec userv1.MongoDBUserSpec, password string) (om.MongoDBUser, error) {
+func toOmUser(spec userv1.MongoDBUserSpec, password string, ac *om.AutomationConfig) (om.MongoDBUser, error) {
 	user := om.MongoDBUser{
 		Database:                   spec.Database,
 		Username:                   spec.Username,
@@ -344,7 +344,7 @@ func toOmUser(spec userv1.MongoDBUserSpec, password string) (om.MongoDBUser, err
 
 	// only specify password if we're dealing with non-x509 users
 	if spec.Database != authentication.ExternalDB {
-		if err := authentication.ConfigureScramCredentials(&user, password); err != nil {
+		if err := authentication.ConfigureScramCredentials(&user, password, ac); err != nil {
 			return om.MongoDBUser{}, xerrors.Errorf("error generating SCRAM credentials: %w", err)
 		}
 	}
@@ -385,7 +385,7 @@ func (r *MongoDBUserReconciler) handleScramShaUser(ctx context.Context, user *us
 			auth.RemoveUser(user.Status.Username, user.Status.Database)
 		}
 
-		desiredUser, err := toOmUser(user.Spec, password)
+		desiredUser, err := toOmUser(user.Spec, password, ac)
 		if err != nil {
 			return err
 		}
@@ -421,18 +421,18 @@ func (r *MongoDBUserReconciler) handleScramShaUser(ctx context.Context, user *us
 }
 
 func (r *MongoDBUserReconciler) handleExternalAuthUser(ctx context.Context, user *userv1.MongoDBUser, conn om.Connection, log *zap.SugaredLogger) (reconcile.Result, error) {
-	desiredUser, err := toOmUser(user.Spec, "")
-	if err != nil {
-		return r.updateStatus(ctx, user, workflow.Failed(xerrors.Errorf("error updating user %w", err)), log)
-	}
-
 	shouldRetry := false
 	updateFunction := func(ac *om.AutomationConfig) error {
 		if !externalAuthMechanismsAvailable(ac.Auth.DeploymentAuthMechanisms) {
 			shouldRetry = true
 			return xerrors.Errorf("no external authentication mechanisms (LDAP or x509) have been configured")
 		}
 
+		desiredUser, err := toOmUser(user.Spec, "", ac)
+		if err != nil {
+			return xerrors.Errorf("errorr updating user %w", err)
+		}
+
 		auth := ac.Auth
 		if user.ChangedIdentifier() {
 			auth.RemoveUser(user.Status.Username, user.Status.Database)
@@ -442,7 +442,7 @@ func (r *MongoDBUserReconciler) handleExternalAuthUser(ctx context.Context, user
 		return nil
 	}
 
-	err = conn.ReadUpdateAutomationConfig(updateFunction, log)
+	err := conn.ReadUpdateAutomationConfig(updateFunction, log)
 	if err != nil {
 		if shouldRetry {
 			return r.updateStatus(ctx, user, workflow.Pending("%s", err.Error()).WithRetry(10), log)

controllers/operator/mongodbuser_controller_test.go

@@ -2,6 +2,7 @@ package operator
 
 import (
 	"context"
+	"encoding/json"
 	"testing"
 	"time"
 
@@ -71,6 +72,69 @@ func TestUserIsAdded_ToAutomationConfig_OnSuccessfulReconciliation(t *testing.T)
 	assert.Equal(t, len(user.Spec.Roles), len(createdUser.Roles))
 }
 
+// Not making this (DeepCopy) a method of type AutomationConfig because I want to be
+// explicit that this method is just for test code.
+func DeepCopy(original *om.AutomationConfig) (*om.AutomationConfig, error) {
+	if original == nil {
+		return nil, nil
+	}
+
+	b, err := json.Marshal(original)
+	if err != nil {
+		return nil, err
+	}
+
+	var newAC om.AutomationConfig
+	err = json.Unmarshal(b, &newAC)
+	if err != nil {
+		return nil, err
+	}
+	return &newAC, nil
+}
+
+func TestNoChange_InAC_After_Same_User_Reconciliation(t *testing.T) {
+	ctx := context.Background()
+	user := DefaultMongoDBUserBuilder().SetMongoDBResourceName("my-rs").Build()
+	reconciler, client, omConnectionFactory := userReconcilerWithAuthMode(ctx, user, util.AutomationConfigScramSha256Option)
+
+	// initialize resources required for the tests
+	_ = client.Create(ctx, DefaultReplicaSetBuilder().EnableAuth().AgentAuthMode("SCRAM").
+		SetName("my-rs").Build())
+	createUserControllerConfigMap(ctx, client)
+	createPasswordSecret(ctx, client, user.Spec.PasswordSecretKeyRef, "password")
+
+	actual, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: kube.ObjectKey(user.Namespace, user.Name)})
+
+	okReconcileResult, _ := workflow.OK().ReconcileResult()
+
+	assert.Nil(t, err, "there should be no error on successful reconciliation")
+	assert.Equal(t, okReconcileResult, actual, "there should be a successful reconciliation if the password is a valid reference")
+
+	ac, _ := omConnectionFactory.GetConnection().ReadAutomationConfig()
+	// since underlying implmentation of ReadAutomationConfig just has reference to automation config
+	// it's better to deep copy this version of AC so that it can be used to compare later.
+	originalAC, err := DeepCopy(ac)
+	assert.Nil(t, err)
+
+	// the automation config should have been updated during reconciliation
+	assert.Len(t, ac.Auth.Users, 1, "the MongoDBUser should have been added to the AutomationConfig")
+
+	_, createdUser := ac.Auth.GetUser("my-user", "admin")
+	assert.Equal(t, user.Spec.Username, createdUser.Username)
+	assert.Equal(t, user.Spec.Database, createdUser.Database)
+	assert.Equal(t, len(user.Spec.Roles), len(createdUser.Roles))
+
+	// reconcile the same user again and make sure the automation config is not updated, if the user's password is not changed
+	// we don't generate scram creds and because of that we wouldn't see changes in automatino config
+	reconcileResult, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: kube.ObjectKey(user.Namespace, user.Name)})
+	assert.Nil(t, err)
+	assert.Equal(t, okReconcileResult, reconcileResult)
+
+	acAfterSecondReconcile, _ := omConnectionFactory.GetConnection().ReadAutomationConfig()
+	// verify that the automation cnofig has not been changed because we ran reconciliation second time with the same user
+	assert.True(t, acAfterSecondReconcile.EqualsWithoutDeployment(*originalAC), "Automation config before the second reconciliation and after the second reconciliation should be same")
+}
+
 func TestReconciliationSucceed_OnAddingUser_FromADifferentNamespace(t *testing.T) {
 	ctx := context.Background()
 	user := DefaultMongoDBUserBuilder().SetMongoDBResourceName("my-rs").Build()