ROX-22354: autorotate DP secrets if data has changed (#1933)

* autorotate DP secrets if data has changed

* add DB migration

.secrets.baseline

@@ -304,14 +304,14 @@
         "filename": "internal/dinosaur/pkg/presenters/managedcentral.go",
         "hashed_secret": "f4ac636d63edfd5477df8f25e4f4794c73e91d51",
         "is_verified": false,
-        "line_number": 208
+        "line_number": 209
       },
       {
         "type": "Secret Keyword",
         "filename": "internal/dinosaur/pkg/presenters/managedcentral.go",
         "hashed_secret": "e26735ec1cbf8ad15cb7d1eea4893035f61297aa",
         "is_verified": false,
-        "line_number": 214
+        "line_number": 215
       }
     ],
     "internal/dinosaur/pkg/services/dinosaurservice_moq.go": [
@@ -473,5 +473,5 @@
       }
     ]
   },
-  "generated_at": "2024-06-17T20:09:11Z"
+  "generated_at": "2024-07-02T18:42:50Z"
 }

fleetshard/pkg/central/reconciler/reconciler.go

@@ -4,6 +4,7 @@ package reconciler
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -30,6 +31,7 @@ import (
 	centralNotifierUtils "github.com/stackrox/rox/central/notifiers/utils"
 	"github.com/stackrox/rox/operator/apis/platform/v1alpha1"
 	"github.com/stackrox/rox/pkg/declarativeconfig"
+	"github.com/stackrox/rox/pkg/maputil"
 	"github.com/stackrox/rox/pkg/random"
 	"gopkg.in/yaml.v2"
 	"helm.sh/helm/v3/pkg/chart"
@@ -103,6 +105,11 @@ type needsReconcileFunc func(changed bool, central *v1alpha1.Central, storedSecr
 type restoreCentralSecretsFunc func(ctx context.Context, remoteCentral private.ManagedCentral) error
 type areSecretsStoredFunc func(secretsStored []string) bool
 
+type encryptedSecrets struct {
+	secrets   map[string]string
+	sha256Sum string
+}
+
 // CentralReconcilerOptions are the static options for creating a reconciler.
 type CentralReconcilerOptions struct {
 	UseRoutes             bool
@@ -824,14 +831,18 @@ func (r *CentralReconciler) collectReconciliationStatus(ctx context.Context, rem
 	}
 
 	// Only report secrets if Central is ready, to ensure we're not trying to get secrets before they are created.
-	// Only report secrets if not all secrets are already stored to ensure we don't overwrite initial secrets with corrupted secrets
-	// from the cluster state.
-	if isRemoteCentralReady(remoteCentral) && !r.areSecretsStored(remoteCentral.Metadata.SecretsStored) {
-		secrets, err := r.collectSecretsEncrypted(ctx, remoteCentral)
+	if isRemoteCentralReady(remoteCentral) {
+		encSecrets, err := r.collectSecretsEncrypted(ctx, remoteCentral)
 		if err != nil {
 			return nil, err
 		}
-		status.Secrets = secrets // pragma: allowlist secret
+
+		// Only report secrets if data hash differs to make sure we don't produce huge amount of data
+		// if no update is required on the fleet-manager DB
+		if encSecrets.sha256Sum != remoteCentral.Metadata.SecretDataSha256Sum { // pragma: allowlist secret
+			status.Secrets = encSecrets.secrets               // pragma: allowlist secret
+			status.SecretDataSha256Sum = encSecrets.sha256Sum // pragma: allowlist secret
+		}
 	}
 
 	return status, nil
@@ -875,18 +886,18 @@ func (r *CentralReconciler) collectSecrets(ctx context.Context, remoteCentral *p
 	return secrets, nil
 }
 
-func (r *CentralReconciler) collectSecretsEncrypted(ctx context.Context, remoteCentral *private.ManagedCentral) (map[string]string, error) {
+func (r *CentralReconciler) collectSecretsEncrypted(ctx context.Context, remoteCentral *private.ManagedCentral) (encryptedSecrets, error) {
 	secrets, err := r.collectSecrets(ctx, remoteCentral)
 	if err != nil {
-		return nil, err
+		return encryptedSecrets{}, err
 	}
 
-	encryptedSecrets, err := r.encryptSecrets(secrets)
+	encSecrets, err := r.encryptSecrets(secrets)
 	if err != nil {
-		return nil, fmt.Errorf("encrypting secrets for namespace: %s: %w", remoteCentral.Metadata.Namespace, err)
+		return encSecrets, fmt.Errorf("encrypting secrets for namespace: %s: %w", remoteCentral.Metadata.Namespace, err)
 	}
 
-	return encryptedSecrets, nil
+	return encSecrets, nil
 }
 
 func (r *CentralReconciler) decryptSecrets(secrets map[string]string) (map[string]*corev1.Secret, error) {
@@ -914,25 +925,44 @@ func (r *CentralReconciler) decryptSecrets(secrets map[string]string) (map[strin
 	return decryptedSecrets, nil
 }
 
-func (r *CentralReconciler) encryptSecrets(secrets map[string]*corev1.Secret) (map[string]string, error) {
-	encryptedSecrets := map[string]string{}
+// encryptSecrets return the encrypted secrets and a sha256 sum of secret data to check if secrets
+// need update later on
+func (r *CentralReconciler) encryptSecrets(secrets map[string]*corev1.Secret) (encryptedSecrets, error) {
+	encSecrets := encryptedSecrets{secrets: map[string]string{}}
 
-	for key, secret := range secrets { // pragma: allowlist secret
+	allSecretData := []byte{}
+	// sort to ensure the loop always executed in the same order
+	// otherwise the sha sum can differ across multiple invocations
+	keys := maputil.Keys(secrets)
+	sort.Strings(keys)
+	for _, key := range keys { // pragma: allowlist secret
+		secret := secrets[key]
 		secretBytes, err := json.Marshal(secret)
 		if err != nil {
-			return nil, fmt.Errorf("error marshaling secret for encryption: %s: %w", key, err)
+			return encSecrets, fmt.Errorf("error marshaling secret for encryption: %s: %w", key, err)
+		}
+
+		// sort to ensure the loop always executed in the same order
+		// otherwise the sha sum can differ across multiple invocations
+		dataKeys := maputil.Keys(secret.Data)
+		sort.Strings(dataKeys)
+		for _, dataKey := range dataKeys {
+			allSecretData = append(allSecretData, secret.Data[dataKey]...)
 		}
 
 		encryptedBytes, err := r.secretCipher.Encrypt(secretBytes)
 		if err != nil {
-			return nil, fmt.Errorf("encrypting secret: %s: %w", key, err)
+			return encSecrets, fmt.Errorf("encrypting secret: %s: %w", key, err)
 		}
 
-		encryptedSecrets[key] = base64.StdEncoding.EncodeToString(encryptedBytes)
+		encSecrets.secrets[key] = base64.StdEncoding.EncodeToString(encryptedBytes)
 	}
 
-	return encryptedSecrets, nil
+	secretSum := sha256.Sum256(allSecretData)
+	secretShaStr := base64.StdEncoding.EncodeToString(secretSum[:])
+	encSecrets.sha256Sum = secretShaStr
 
+	return encSecrets, nil
 }
 
 // ensureSecretHasOwnerReference is used to make sure the central-tls secret has it's

fleetshard/pkg/central/reconciler/reconciler_test.go

@@ -2607,3 +2607,90 @@ func TestChartValues(t *testing.T) {
 	}
 
 }
+
+func TestEncryptionShaSum(t *testing.T) {
+	reconciler := &CentralReconciler{
+		secretCipher: cipher.LocalBase64Cipher{}, // pragma: allowlist secret
+	}
+
+	testSecrets := map[string]*v1.Secret{
+		"testsecret1": {
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testsecret1",
+				Namespace: centralNamespace,
+			},
+			Data: map[string][]byte{
+				"test1": []byte("test1-secretdata1"),
+				"test2": []byte("test1-secretdata2"),
+			},
+		},
+		"testsecret2": {
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testsecret2",
+				Namespace: centralNamespace,
+			},
+			Data: map[string][]byte{
+				"test1": []byte("test2-secretdata1"),
+				"test2": []byte("test2-secretdata2"),
+			},
+		},
+	}
+
+	enc1, err := reconciler.encryptSecrets(testSecrets)
+	require.NoError(t, err)
+	enc2, err := reconciler.encryptSecrets(testSecrets)
+	require.NoError(t, err)
+
+	testSecrets["testsecret1"].Data["test3"] = []byte("test3")
+	encChanged, err := reconciler.encryptSecrets(testSecrets)
+	require.NoError(t, err)
+
+	require.NoError(t, err)
+	require.Equal(t, enc1.sha256Sum, enc2.sha256Sum, "hash of equal secrets was not equal")
+	require.NotEqual(t, enc1.sha256Sum, encChanged.sha256Sum, "hash of unequal secrets was equal")
+}
+
+func TestEncyrptionSHASumSameObject(t *testing.T) {
+	// This test is important, since it helped catch a bug discovered during e2e testing
+	// of this feature that would cause the calculated hash to be not equal for the same secrets
+	// because the function was looping over keys of Go maps, which is not guaranteed to loop in the
+	// same order on every invokation
+	reconciler := &CentralReconciler{
+		secretCipher: cipher.LocalBase64Cipher{}, // pragma: allowlist secret
+	}
+
+	testSecrets := map[string]*v1.Secret{
+		"testsecret1": {
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testsecret1",
+				Namespace: centralNamespace,
+			},
+			Data: map[string][]byte{
+				"test1": []byte("test1-secretdata1"),
+				"test2": []byte("test1-secretdata2"),
+			},
+		},
+		"testsecret2": {
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testsecret2",
+				Namespace: centralNamespace,
+			},
+			Data: map[string][]byte{
+				"test1": []byte("test2-secretdata1"),
+				"test2": []byte("test2-secretdata2"),
+			},
+		},
+	}
+
+	amount := 1000
+	sums := make([]string, 1000)
+	for i := 0; i < amount; i++ {
+		enc, err := reconciler.encryptSecrets(testSecrets)
+		require.NoError(t, err)
+		sums[i] = enc.sha256Sum
+	}
+
+	for i := 1; i < amount; i++ {
+		require.Equal(t, sums[i-1], sums[i], "hash of the same object should always be equal but was not")
+	}
+}

internal/dinosaur/pkg/api/dbapi/central_request_types.go

@@ -74,9 +74,13 @@ type CentralRequest struct {
 	// We store this in the database to ensure that old centrals whose namespace contained "owner-<central-id>" information will continue to work.
 
 	// Secrets stores the encrypted secrets reported for a central tenant
-	Secrets          api.JSON `json:"secrets"`
-	Namespace        string   `json:"namespace"`
-	RoutesCreationID string   `json:"routes_creation_id"`
+	Secrets api.JSON `json:"secrets"`
+	// SecretDataSha256Sum is the b64 encoded hash of plain text data of the stored secrets.
+	// It used used for equality checks of secrets in the dataplane cluster with the secrets stored in DB
+	SecretDataSha256Sum string `json:"secret_data_sha256_sum"`
+
+	Namespace        string `json:"namespace"`
+	RoutesCreationID string `json:"routes_creation_id"`
 	// DeletionTimestamp stores the timestamp of the DELETE api call for the resource.
 	DeletionTimestamp sql.NullTime `json:"deletionTimestamp"`
 

internal/dinosaur/pkg/api/dbapi/data_plane_central_status.go

@@ -11,6 +11,7 @@ type DataPlaneCentralStatus struct {
 	// Going to ignore the rest of fields (like capacity and versions) for now, until when they are needed
 	Routes                 []DataPlaneCentralRoute
 	Secrets                map[string]string
+	SecretDataSha256Sum    string
 	CentralVersion         string
 	CentralOperatorVersion string
 }

internal/dinosaur/pkg/api/private/api/openapi.yaml

@@ -560,6 +560,9 @@ components:
             type: string
           description: Map of Secrets created for a Central
           type: object
+        secretDataSha256Sum:
+          description: Hash of plain text secret data used for equality check
+          type: string
       type: object
     DataPlaneCentralStatusUpdateRequest:
       additionalProperties:
@@ -622,6 +625,8 @@ components:
           additionalProperties:
             type: string
           type: object
+        secretDataSha256Sum:
+          type: string
         expired-at:
           format: date-time
           nullable: true

internal/dinosaur/pkg/migrations/20240703120000_add_secret_data_sha256_sum_to_central_request.go

@@ -0,0 +1,34 @@
+package migrations
+
+// Migrations should NEVER use types from other packages. Types can change
+// and then migrations run on a _new_ database will fail or behave unexpectedly.
+// Instead of importing types, always re-create the type in the migration, as
+// is done here, even though the same type is defined in pkg/api
+
+import (
+	"github.com/go-gormigrate/gormigrate/v2"
+	"github.com/pkg/errors"
+	"github.com/stackrox/acs-fleet-manager/pkg/db"
+	"gorm.io/gorm"
+)
+
+func addSecretDataSha256SumToCentralRequest() *gormigrate.Migration {
+	type CentralRequest struct {
+		db.Model
+		SecretDataSha256Sum string `json:"secret_data_sha256_sum"`
+	}
+	migrationID := "20240703120000"
+
+	return &gormigrate.Migration{
+		ID: migrationID,
+		Migrate: func(tx *gorm.DB) error {
+			return addColumnIfNotExists(tx, &CentralRequest{}, "secret_data_sha256_sum")
+		},
+		Rollback: func(tx *gorm.DB) error {
+			return errors.Wrap(
+				tx.Migrator().DropColumn(&CentralRequest{}, "secret_data_sha256_sum"),
+				"failed to drop secret_data_sha256_sum column",
+			)
+		},
+	}
+}

internal/dinosaur/pkg/migrations/migrations.go

@@ -54,6 +54,7 @@ func getMigrations() []*gormigrate.Migration {
 		addClusterAddons(),
 		addAlternateUserIDFieldToCentralRequests(),
 		addTraitsFieldToCentralRequests(),
+		addSecretDataSha256SumToCentralRequest(),
 	}
 }
 

internal/dinosaur/pkg/presenters/data_plane_dinosaur_status.go

@@ -31,10 +31,11 @@ func ConvertDataPlaneDinosaurStatus(status map[string]private.DataPlaneCentralSt
 		}
 
 		res = append(res, &dbapi.DataPlaneCentralStatus{
-			CentralClusterID: k,
-			Conditions:       c,
-			Routes:           routes,
-			Secrets:          v.Secrets, // pragma: allowlist secret
+			CentralClusterID:    k,
+			Conditions:          c,
+			Routes:              routes,
+			Secrets:             v.Secrets,             // pragma: allowlist secret
+			SecretDataSha256Sum: v.SecretDataSha256Sum, // pragma: allowlist secret
 		})
 	}
 

internal/dinosaur/pkg/presenters/managedcentral.go

@@ -118,9 +118,10 @@ func (c *ManagedCentralPresenter) presentManagedCentral(gitopsConfig gitops.Conf
 				MasId:          from.ID,
 				MasPlacementId: from.PlacementID,
 			},
-			Internal:      from.Internal,
-			SecretsStored: getSecretNames(from), // pragma: allowlist secret
-			ExpiredAt:     dbapi.NullTimeToTimePtr(from.ExpiredAt),
+			Internal:            from.Internal,
+			SecretsStored:       getSecretNames(from),     // pragma: allowlist secret
+			SecretDataSha256Sum: from.SecretDataSha256Sum, // pragma: allowlist secret
+			ExpiredAt:           dbapi.NullTimeToTimePtr(from.ExpiredAt),
 		},
 		Spec: private.ManagedCentralAllOfSpec{
 			Owners: []string{