ROX-22354: autorotate DP secrets if data has changed

.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"
@@ -824,14 +826,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) {
+		secrets, secretSum, 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 secretSum != remoteCentral.Metadata.SecretDataSum { // pragma: allowlist secret
+			status.Secrets = secrets         // pragma: allowlist secret
+			status.SecretDataSum = secretSum // pragma: allowlist secret
+		}
 	}
 
 	return status, nil
@@ -875,18 +881,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) (map[string]string, string, error) {
 	secrets, err := r.collectSecrets(ctx, remoteCentral)
 	if err != nil {
-		return nil, err
+		return nil, "", err
 	}
 
-	encryptedSecrets, err := r.encryptSecrets(secrets)
+	encryptedSecrets, secretDataSha, err := r.encryptSecrets(secrets)
 	if err != nil {
-		return nil, fmt.Errorf("encrypting secrets for namespace: %s: %w", remoteCentral.Metadata.Namespace, err)
+		return nil, "", fmt.Errorf("encrypting secrets for namespace: %s: %w", remoteCentral.Metadata.Namespace, err)
 	}
 
-	return encryptedSecrets, nil
+	return encryptedSecrets, secretDataSha, nil
 }
 
 func (r *CentralReconciler) decryptSecrets(secrets map[string]string) (map[string]*corev1.Secret, error) {
@@ -914,24 +920,43 @@ 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) {
+// 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) (map[string]string, string, error) {
 	encryptedSecrets := 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 nil, "", 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 nil, "", fmt.Errorf("encrypting secret: %s: %w", key, err)
 		}
 
 		encryptedSecrets[key] = base64.StdEncoding.EncodeToString(encryptedBytes)
 	}
 
-	return encryptedSecrets, nil
+	secretSum := sha256.Sum256(allSecretData)
+	secretShaStr := base64.StdEncoding.EncodeToString(secretSum[:])
+
+	return encryptedSecrets, secretShaStr, nil
 
 }
 

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"),
+			},
+		},
+	}
+
+	_, shaSumFirst, err := reconciler.encryptSecrets(testSecrets)
+	require.NoError(t, err)
+	_, shaSumSecond, err := reconciler.encryptSecrets(testSecrets)
+	require.NoError(t, err)
+
+	testSecrets["testsecret1"].Data["test3"] = []byte("test3")
+	_, shaSumChanged, err := reconciler.encryptSecrets(testSecrets)
+	require.NoError(t, err)
+
+	require.NoError(t, err)
+	require.Equal(t, shaSumFirst, shaSumSecond, "hash of equal secrets was not equal")
+	require.NotEqual(t, shaSumFirst, shaSumChanged, "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++ {
+		_, sum, err := reconciler.encryptSecrets(testSecrets)
+		require.NoError(t, err)
+		sums[i] = sum
+	}
+
+	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"`
+	// SecretDataSum 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
+	SecretDataSum string `json:"secret_data_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
+	SecretDataSum          string
 	CentralVersion         string
 	CentralOperatorVersion string
 }

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

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

internal/dinosaur/pkg/migrations/20240703120000_add_secret_data_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 addSecretDataSumToCentralRequest() *gormigrate.Migration {
+	type CentralRequest struct {
+		db.Model
+		SecretDataSum string `json:"secret_data_sum"`
+	}
+	migrationID := "20240703120000"
+
+	return &gormigrate.Migration{
+		ID: migrationID,
+		Migrate: func(tx *gorm.DB) error {
+			return addColumnIfNotExists(tx, &CentralRequest{}, "secret_data_sum")
+		},
+		Rollback: func(tx *gorm.DB) error {
+			return errors.Wrap(
+				tx.Migrator().DropColumn(&CentralRequest{}, "secret_data_sum"),
+				"failed to drop secret_data_sum column",
+			)
+		},
+	}
+}

internal/dinosaur/pkg/migrations/migrations.go

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

internal/dinosaur/pkg/presenters/data_plane_dinosaur_status.go

@@ -34,7 +34,8 @@ func ConvertDataPlaneDinosaurStatus(status map[string]private.DataPlaneCentralSt
 			CentralClusterID: k,
 			Conditions:       c,
 			Routes:           routes,
-			Secrets:          v.Secrets, // pragma: allowlist secret
+			Secrets:          v.Secrets,       // pragma: allowlist secret
+			SecretDataSum:    v.SecretDataSum, // pragma: allowlist secret
 		})
 	}
 

internal/dinosaur/pkg/presenters/managedcentral.go

@@ -120,6 +120,7 @@ func (c *ManagedCentralPresenter) presentManagedCentral(gitopsConfig gitops.Conf
 			},
 			Internal:      from.Internal,
 			SecretsStored: getSecretNames(from), // pragma: allowlist secret
+			SecretDataSum: from.SecretDataSum,   // pragma: allowlist secret
 			ExpiredAt:     dbapi.NullTimeToTimePtr(from.ExpiredAt),
 		},
 		Spec: private.ManagedCentralAllOfSpec{

internal/dinosaur/pkg/services/data_plane_dinosaur.go

@@ -280,11 +280,18 @@ func (d *dataPlaneCentralService) addSecretsToRequest(centralRequest *dbapi.Cent
 		logger.Logger.V(10).Infof("skip persisting secrets for Central %s, report is empty or nil", centralRequest.ID)
 		return nil
 	}
+
+	if centralStatus.SecretDataSum == "" {
+		// TODO: change this to send a bad request later, once we are sure no FS version without SecretDataSum feature is running
+		logger.Logger.V(10).Warningf("persisting secret but no secret data sum. this might be a request of a outdated fleetshard version")
+	}
+
 	logger.Logger.Infof("store secret information for central %s", centralRequest.ID)
 
 	if err := centralRequest.SetSecrets(centralStatus.Secrets); err != nil {
 		return serviceError.NewWithCause(serviceError.ErrorGeneral, err, "failed to set secrets for central %s", centralRequest.ID)
 	}
+	centralRequest.SecretDataSum = centralStatus.SecretDataSum // pragma: allowlist secret
 
 	return nil
 }

internal/dinosaur/pkg/services/dinosaur.go

@@ -183,11 +183,11 @@ func (k *dinosaurService) RotateCentralRHSSOClient(ctx context.Context, centralR
 
 func (k *dinosaurService) ResetCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError {
 	centralRequest.Secrets = nil // pragma: allowlist secret
-
+	centralRequest.SecretDataSum = ""
 	logStateChange("reset secrets", centralRequest.ID, nil)
 
 	dbConn := k.connectionFactory.New()
-	if err := dbConn.Model(centralRequest).Select("secrets").Updates(centralRequest).Error; err != nil {
+	if err := dbConn.Model(centralRequest).Select("secrets", "secret_data_sum").Updates(centralRequest).Error; err != nil {
 		return errors.NewWithCause(errors.ErrorGeneral, err, "Unable to reset secrets for central request")
 	}
 

openapi/fleet-manager-private.yaml

@@ -269,6 +269,8 @@ components:
                   type: object
                   additionalProperties:
                     type: string
+                secretDataSum:
+                  type: string
                 expired-at:
                   type: string
                   format: date-time
@@ -459,6 +461,9 @@ components:
           type: object
           additionalProperties:
             type: string
+        secretDataSum:
+          description: "Hash of plain text secret data used for equality check"
+          type: string
 
       example:
         $ref: "#/components/examples/DataPlaneCentralStatusRequestExample"