From db7e4e0ba2d48867c4d4bef5eed0615a055f2ef6 Mon Sep 17 00:00:00 2001 From: Johannes Malsam <60240743+johannes94@users.noreply.github.com> Date: Mon, 8 Jul 2024 09:17:15 +0200 Subject: [PATCH] ROX-22354: autorotate DP secrets if data has changed (#1933) * autorotate DP secrets if data has changed * add DB migration --- .secrets.baseline | 6 +- .../pkg/central/reconciler/reconciler.go | 64 ++++++++++---- .../pkg/central/reconciler/reconciler_test.go | 87 +++++++++++++++++++ .../pkg/api/dbapi/central_request_types.go | 10 ++- .../api/dbapi/data_plane_central_status.go | 1 + .../dinosaur/pkg/api/private/api/openapi.yaml | 5 ++ .../model_data_plane_central_status.go | 2 + .../model_managed_central_all_of_metadata.go | 17 ++-- ...40122180000_add_alternate_user_id copy.go} | 0 ...cret_data_sha256_sum_to_central_request.go | 34 ++++++++ .../dinosaur/pkg/migrations/migrations.go | 1 + .../presenters/data_plane_dinosaur_status.go | 9 +- .../dinosaur/pkg/presenters/managedcentral.go | 7 +- .../pkg/services/data_plane_dinosaur.go | 7 ++ internal/dinosaur/pkg/services/dinosaur.go | 4 +- openapi/fleet-manager-private.yaml | 5 ++ 16 files changed, 219 insertions(+), 40 deletions(-) rename internal/dinosaur/pkg/migrations/{20240122180000_add_alternate_user_id.go => 20240122180000_add_alternate_user_id copy.go} (100%) create mode 100644 internal/dinosaur/pkg/migrations/20240703120000_add_secret_data_sha256_sum_to_central_request.go diff --git a/.secrets.baseline b/.secrets.baseline index 692288f829..cf809fecf2 100644 --- a/.secrets.baseline +++ b/.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" } diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index 7bc256bc2b..f40814a25a 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/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 diff --git a/fleetshard/pkg/central/reconciler/reconciler_test.go b/fleetshard/pkg/central/reconciler/reconciler_test.go index 18008326bd..c4069a7b70 100644 --- a/fleetshard/pkg/central/reconciler/reconciler_test.go +++ b/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") + } +} diff --git a/internal/dinosaur/pkg/api/dbapi/central_request_types.go b/internal/dinosaur/pkg/api/dbapi/central_request_types.go index d05a5eb709..2c71d3aab1 100644 --- a/internal/dinosaur/pkg/api/dbapi/central_request_types.go +++ b/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-" 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"` diff --git a/internal/dinosaur/pkg/api/dbapi/data_plane_central_status.go b/internal/dinosaur/pkg/api/dbapi/data_plane_central_status.go index 6e7b6b4b5f..0e210747f8 100644 --- a/internal/dinosaur/pkg/api/dbapi/data_plane_central_status.go +++ b/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 } diff --git a/internal/dinosaur/pkg/api/private/api/openapi.yaml b/internal/dinosaur/pkg/api/private/api/openapi.yaml index 1b87a0371b..ba85ccc411 100644 --- a/internal/dinosaur/pkg/api/private/api/openapi.yaml +++ b/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 diff --git a/internal/dinosaur/pkg/api/private/model_data_plane_central_status.go b/internal/dinosaur/pkg/api/private/model_data_plane_central_status.go index f8519590f8..32df630aba 100644 --- a/internal/dinosaur/pkg/api/private/model_data_plane_central_status.go +++ b/internal/dinosaur/pkg/api/private/model_data_plane_central_status.go @@ -18,4 +18,6 @@ type DataPlaneCentralStatus struct { Routes []DataPlaneCentralStatusRoutes `json:"routes,omitempty"` // Map of Secrets created for a Central Secrets map[string]string `json:"secrets,omitempty"` + // Hash of plain text secret data used for equality check + SecretDataSha256Sum string `json:"secretDataSha256Sum,omitempty"` } diff --git a/internal/dinosaur/pkg/api/private/model_managed_central_all_of_metadata.go b/internal/dinosaur/pkg/api/private/model_managed_central_all_of_metadata.go index 8abc1c1245..b65c3dde80 100644 --- a/internal/dinosaur/pkg/api/private/model_managed_central_all_of_metadata.go +++ b/internal/dinosaur/pkg/api/private/model_managed_central_all_of_metadata.go @@ -16,12 +16,13 @@ import ( // ManagedCentralAllOfMetadata struct for ManagedCentralAllOfMetadata type ManagedCentralAllOfMetadata struct { - Name string `json:"name,omitempty"` - Namespace string `json:"namespace,omitempty"` - Internal bool `json:"internal,omitempty"` - Annotations ManagedCentralAllOfMetadataAnnotations `json:"annotations,omitempty"` - DeletionTimestamp string `json:"deletionTimestamp,omitempty"` - SecretsStored []string `json:"secretsStored,omitempty"` - Secrets map[string]string `json:"secrets,omitempty"` - ExpiredAt *time.Time `json:"expired-at,omitempty"` + Name string `json:"name,omitempty"` + Namespace string `json:"namespace,omitempty"` + Internal bool `json:"internal,omitempty"` + Annotations ManagedCentralAllOfMetadataAnnotations `json:"annotations,omitempty"` + DeletionTimestamp string `json:"deletionTimestamp,omitempty"` + SecretsStored []string `json:"secretsStored,omitempty"` + Secrets map[string]string `json:"secrets,omitempty"` + SecretDataSha256Sum string `json:"secretDataSha256Sum,omitempty"` + ExpiredAt *time.Time `json:"expired-at,omitempty"` } diff --git a/internal/dinosaur/pkg/migrations/20240122180000_add_alternate_user_id.go b/internal/dinosaur/pkg/migrations/20240122180000_add_alternate_user_id copy.go similarity index 100% rename from internal/dinosaur/pkg/migrations/20240122180000_add_alternate_user_id.go rename to internal/dinosaur/pkg/migrations/20240122180000_add_alternate_user_id copy.go diff --git a/internal/dinosaur/pkg/migrations/20240703120000_add_secret_data_sha256_sum_to_central_request.go b/internal/dinosaur/pkg/migrations/20240703120000_add_secret_data_sha256_sum_to_central_request.go new file mode 100644 index 0000000000..143b162e98 --- /dev/null +++ b/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", + ) + }, + } +} diff --git a/internal/dinosaur/pkg/migrations/migrations.go b/internal/dinosaur/pkg/migrations/migrations.go index d96036202c..3b3eab1b5f 100644 --- a/internal/dinosaur/pkg/migrations/migrations.go +++ b/internal/dinosaur/pkg/migrations/migrations.go @@ -54,6 +54,7 @@ func getMigrations() []*gormigrate.Migration { addClusterAddons(), addAlternateUserIDFieldToCentralRequests(), addTraitsFieldToCentralRequests(), + addSecretDataSha256SumToCentralRequest(), } } diff --git a/internal/dinosaur/pkg/presenters/data_plane_dinosaur_status.go b/internal/dinosaur/pkg/presenters/data_plane_dinosaur_status.go index e1b0789f5d..391db04dfa 100644 --- a/internal/dinosaur/pkg/presenters/data_plane_dinosaur_status.go +++ b/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 }) } diff --git a/internal/dinosaur/pkg/presenters/managedcentral.go b/internal/dinosaur/pkg/presenters/managedcentral.go index 8561a0cab1..0a458cce7c 100644 --- a/internal/dinosaur/pkg/presenters/managedcentral.go +++ b/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{ diff --git a/internal/dinosaur/pkg/services/data_plane_dinosaur.go b/internal/dinosaur/pkg/services/data_plane_dinosaur.go index 58288a69b5..8f4a28784b 100644 --- a/internal/dinosaur/pkg/services/data_plane_dinosaur.go +++ b/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.SecretDataSha256Sum == "" { + // 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.SecretDataSha256Sum = centralStatus.SecretDataSha256Sum // pragma: allowlist secret return nil } diff --git a/internal/dinosaur/pkg/services/dinosaur.go b/internal/dinosaur/pkg/services/dinosaur.go index 533f3e5fd3..1116328d40 100644 --- a/internal/dinosaur/pkg/services/dinosaur.go +++ b/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.SecretDataSha256Sum = "" 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_sha256_sum").Updates(centralRequest).Error; err != nil { return errors.NewWithCause(errors.ErrorGeneral, err, "Unable to reset secrets for central request") } diff --git a/openapi/fleet-manager-private.yaml b/openapi/fleet-manager-private.yaml index 558487fc87..2663ea3440 100644 --- a/openapi/fleet-manager-private.yaml +++ b/openapi/fleet-manager-private.yaml @@ -269,6 +269,8 @@ components: type: object additionalProperties: type: string + secretDataSha256Sum: + type: string expired-at: type: string format: date-time @@ -639,6 +641,9 @@ components: type: object additionalProperties: type: string + secretDataSha256Sum: + description: "Hash of plain text secret data used for equality check" + type: string example: $ref: "#/components/examples/DataPlaneCentralStatusRequestExample"