Ensure that unencrypted fields during a encryption key rotation are not modified

app/actions/droplet_copy.rb

@@ -4,7 +4,6 @@ class InvalidCopyError < StandardError; end
 
     CLONED_ATTRIBUTES = [
       :buildpack_receipt_buildpack_guid,
-      :salt,
       :process_types,
       :buildpack_receipt_buildpack,
       :execution_metadata,

db/migrations/20230306111500_drop_encrypted_environment_variables.rb

@@ -0,0 +1,6 @@
+Sequel.migration do
+  change do
+    drop_column :droplets, :encrypted_environment_variables
+    drop_column :droplets, :salt
+  end
+end

lib/cloud_controller/encryptor.rb

@@ -161,7 +161,9 @@ def set_field_as_encrypted(field_name, options={})
           raise 'Field "encryption_key_label" does not exist' unless columns.include?(:encryption_key_label)
           raise 'Field "encryption_iterations" does not exist' unless columns.include?(:encryption_iterations)
 
-          encrypted_fields << { field_name: field_name, salt_name: salt_name }
+          fields = { field_name: field_name, salt_name: salt_name }
+          fields.merge!({ storage_column: storage_column }) if storage_column
+          encrypted_fields << fields
 
           Encryptor.encrypted_classes << self.name
 

lib/cloud_controller/errands/rotate_database_key.rb

@@ -34,6 +34,13 @@ def rotate_for_class(klass, batch_size)
                  all
           break if rows.count == 0
 
+          klass.instance_exec do
+            @allow_manual_timestamp_update = true
+          end
+          klass.descendants.select { |m| m.to_s.start_with?('VCAP::CloudController::') }.each do |model|
+            model.instance_exec { @allow_manual_timestamp_update = true }
+          end
+
           rotate_batch(klass, rows)
           logger.info("Rotated batch of #{rows.count} rows of #{klass}")
         end
@@ -45,7 +52,8 @@ def rotate_batch(klass, rows)
           row.db.transaction do
             row.lock!
             encrypt_row(encrypted_fields, row)
-            row.save(validate: false)
+            row.modified!(:updated_at)
+            row.save(validate: false, changed: true)
           rescue Sequel::NoExistingObject
             raise Sequel::Rollback
           rescue StandardError => e

spec/support/fakes/blueprints.rb

@@ -50,6 +50,11 @@ module VCAP::CloudController
     buildpack_lifecycle_data { BuildpackLifecycleDataModel.make(app: object.save) }
   end
 
+  AppModel.blueprint(:all_fields) do
+    droplet_guid { Sham.guid }
+    buildpack_cache_sha256_checksum { Sham.guid }
+  end
+
   AppModel.blueprint(:kpack) do
     name { Sham.name }
     space { Space.make }
@@ -118,6 +123,22 @@ module VCAP::CloudController
     buildpack_lifecycle_data { BuildpackLifecycleDataModel.make(droplet: object.save) }
   end
 
+  DropletModel.blueprint(:all_fields) do
+    execution_metadata { 'some-metadata' }
+    error_id { 'error-id' }
+    error_description { 'a-error' }
+    staging_memory_in_mb { 256 }
+    staging_disk_in_mb { 256 }
+    buildpack_receipt_buildpack { 'a-buildpack' }
+    buildpack_receipt_buildpack_guid { Sham.guid }
+    buildpack_receipt_detect_output { 'buildpack-output' }
+    docker_receipt_image { 'docker-iamge' }
+    package_guid { Sham.guid }
+    build_guid { Sham.guid }
+    docker_receipt_username { 'a-user' }
+    sidecars { 'a-sidecar' }
+  end
+
   DropletModel.blueprint(:docker) do
     guid { Sham.guid }
     droplet_hash { nil }
@@ -163,7 +184,9 @@ module VCAP::CloudController
     command { 'bundle exec rake' }
     state { VCAP::CloudController::TaskModel::RUNNING_STATE }
     memory_in_mb { 256 }
+    disk_in_mb {}
     sequence_id { Sham.sequence_id }
+    failure_reason {}
   end
 
   TaskModel.blueprint(:running) do
@@ -252,23 +275,13 @@ module VCAP::CloudController
 
   Route.blueprint do
     space { Space.make }
-
-    domain do
-      PrivateDomain.make(
-        owning_organization: space.organization,
-      )
-    end
-
+    domain { PrivateDomain.make(owning_organization: space.organization) }
     host { Sham.host }
   end
 
   Route.blueprint(:tcp) do
     port { Sham.port }
-    domain do
-      SharedDomain.make(
-        :tcp,
-      )
-    end
+    domain { SharedDomain.make(:tcp) }
   end
 
   Space.blueprint do
@@ -361,6 +374,15 @@ module VCAP::CloudController
     maintenance_info           {}
   end
 
+  ManagedServiceInstance.blueprint(:all_fields) do
+    gateway_data               { 'some data' }
+    dashboard_url              { Sham.url }
+    syslog_drain_url           { Sham.url }
+    tags                       { ['a-tag', 'another-tag'] }
+    route_service_url          { Sham.url }
+    maintenance_info           { 'maintenance info' }
+  end
+
   ManagedServiceInstance.blueprint(:routing) do
     service_plan { ServicePlan.make(:routing) }
   end
@@ -508,6 +530,18 @@ module VCAP::CloudController
     auth_password     { Sham.auth_password }
   end
 
+  ServiceBroker.blueprint(:space_scoped) do
+    space_id          { Space.make.id }
+  end
+
+  ServiceBrokerUpdateRequest.blueprint do
+    name { Sham.name }
+    broker_url { Sham.url }
+    authentication { '{"credentials":{"username":"new-admin","password":"welcome"}}' }
+    service_broker_id {}
+    fk_service_brokers_id {}
+  end
+
   ServiceDashboardClient.blueprint do
     uaa_id          { Sham.name }
     service_broker  { ServiceBroker.make }
@@ -587,6 +621,15 @@ module VCAP::CloudController
     stack { Stack.make.name }
   end
 
+  BuildpackLifecycleDataModel.blueprint(:all_fields) do
+    buildpacks { ['http://example.com/repo.git'] }
+    stack { Stack.make.name }
+    app_guid { Sham.guid }
+    droplet_guid { Sham.guid }
+    admin_buildpack_name { 'admin-bp' }
+    build_guid { Sham.guid }
+  end
+
   KpackLifecycleDataModel.blueprint do
     build { BuildModel.make }
     buildpacks { [] }
@@ -597,6 +640,12 @@ module VCAP::CloudController
     buildpack_url { nil }
   end
 
+  BuildpackLifecycleBuildpackModel.blueprint(:all_fields) do
+    buildpack_lifecycle_data_guid { BuildpackLifecycleDataModel.make.guid }
+    version { Sham.version }
+    buildpack_name { Sham.name }
+  end
+
   BuildpackLifecycleBuildpackModel.blueprint(:custom_buildpack) do
     admin_buildpack_name { nil }
     buildpack_url { 'http://example.com/temporary' }

spec/unit/lib/cloud_controller/errands/rotate_database_key_spec.rb

@@ -7,27 +7,47 @@ module VCAP::CloudController
         # Apps are an example of a single encrypted field
         let(:historical_app) { AppModel.make }
         let(:historical_app_with_no_environment) { AppModel.make }
-        let(:app) { AppModel.make }
+        let(:app) { AppModel.make(:all_fields) }
         let(:app_the_second) { AppModel.make }
         let(:app_new_key_label) { AppModel.make }
         let(:env_vars) { { 'environment' => 'vars', 'PORT' => 344, 'longstring' => 'x' * 4097 } } # PORT is invalid!
         let(:env_vars_2) { { 'vars' => 'environment' } }
 
         # Service bindings are an example of multiple encrypted fields
-        let(:service_binding) { ServiceBinding.make }
+        let(:service_binding) { ServiceBinding.make(syslog_drain_url: Sham.url, name: Sham.name) }
         let(:service_binding_new_key_label) { ServiceBinding.make }
         let(:credentials) { { 'secret' => 'creds' } }
         let(:credentials_2) { { 'more' => 'secrets' } }
 
         # Service instances are an example of single table inheritance
-        let(:service_instance) { ManagedServiceInstance.make }
+        let(:service_instance) { ManagedServiceInstance.make(:all_fields) }
         let(:service_instance_new_key_label) { ManagedServiceInstance.make }
         let(:instance_credentials) { { 'instance' => 'credentials' } }
         let(:instance_credentials_2) { { 'instance_credentials' => 'live here' } }
-
-        let(:task) { TaskModel.make }
+        let(:task) { TaskModel.make(disk_in_mb: 256, failure_reason: 'error') }
         let(:task_the_second) { TaskModel.make }
 
+        let(:service_broker) { ServiceBroker.make(:space_scoped) }
+        let(:service_broker_update_request) { ServiceBrokerUpdateRequest.make(service_broker_id: service_broker.id, fk_service_brokers_id: service_broker.id) }
+
+        let(:encrypted_models) {
+          {
+            'VCAP::CloudController::AppModel' => app,
+            'VCAP::CloudController::PackageModel' => PackageModel.make(:docker, package_hash: Sham.guid, error: 'a-error', docker_image: 'image', docker_username: 'user'),
+            'VCAP::CloudController::DropletModel' => DropletModel.make(:all_fields),
+            'VCAP::CloudController::BuildpackLifecycleDataModel' => BuildpackLifecycleDataModel.make(:all_fields),
+            'VCAP::CloudController::BuildpackLifecycleBuildpackModel' => BuildpackLifecycleBuildpackModel.make(:all_fields),
+            'VCAP::CloudController::TaskModel' => task,
+            'VCAP::CloudController::EnvironmentVariableGroup' => EnvironmentVariableGroup.make,
+            'VCAP::CloudController::RevisionModel' => RevisionModel.make,
+            'VCAP::CloudController::ServiceBinding' => service_binding,
+            'VCAP::CloudController::ServiceInstance' => service_instance,
+            'VCAP::CloudController::ServiceBroker' => service_broker,
+            'VCAP::CloudController::ServiceBrokerUpdateRequest' => service_broker_update_request,
+            'VCAP::CloudController::ServiceKey' => ServiceKey.make,
+          }
+        }
+
         let(:database_encryption_keys) { { old: 'old-key', new: 'new-key' } }
 
         before do
@@ -90,6 +110,28 @@ module VCAP::CloudController
           ])
         end
 
+        def encrypted_columns(model_klass)
+          [:encryption_key_label] + model_klass.all_encrypted_fields.map(&:values).flatten
+        end
+
+        shared_examples 'unencrypted fields' do
+          it 'do not change their values' do
+            entity = encrypted_models[klass]
+            vals = entity.reload.values.except(*encrypted_columns(entity.class))
+            expect(vals.values.all? { |x| !x.blank? }).to be_truthy, "all fields of #{entity.class} need to have values"
+
+            RotateDatabaseKey.perform(batch_size: 1)
+
+            expect(entity.reload.values.except(*encrypted_columns(entity.class))).to eq(vals)
+          end
+        end
+
+        describe 'all models with encrypted fields' do
+          Encryptor.encrypted_classes.each do |klass|
+            context("model #{klass}") { it_behaves_like('unencrypted fields') { let(:klass) { klass } } }
+          end
+        end
+
         context 'no current encryption key label is set' do
           before do
             allow(Encryptor).to receive(:current_encryption_key_label).and_return(nil)
@@ -147,6 +189,15 @@ module VCAP::CloudController
           expect(service_instance.credentials).to eq(instance_credentials)
         end
 
+        it 'does not change the updated_at field' do
+          updated_at = app.reload.values[:updated_at]
+
+          sleep(1.5) # ensure that timestamp in `updated_at` would change
+          RotateDatabaseKey.perform(batch_size: 1)
+
+          expect(app.reload.values[:updated_at]).to eq(updated_at)
+        end
+
         it 'does not re-encrypt values that are already encrypted with the new label' do
           expect(Encryptor).not_to receive(:encrypt).
             with(JSON.dump(env_vars_2), app_new_key_label.salt)