Skip to content

Commit

Permalink
Don't change non empty fields to nil during a encryption key roation
Browse files Browse the repository at this point in the history 
There have been cases where the field `syslog_drain_url` of
`ManagedServiceInstance` was changed from valid URL to nil during
a ccdb encryption key rotation.

This change ensures that only modified columns/rows will be saved during
a rotation.
Tests for all models with encrypted fields check that unencrypted fields
will not be modified.

Co-authored-by: Philipp Thun <[email protected]>
  • Loading branch information
@johha @philippthun
johha and philippthun committed Mar 9, 2023
1 parent d0c7fd4 commit 57a54c6
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 19 deletions.
4 changes: 3 additions & 1 deletion lib/cloud_controller/encryptor.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion lib/cloud_controller/errands/rotate_database_key.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def rotate_batch(klass, rows)
row.lock!
encrypt_row(encrypted_fields, row)
row.modified!(:updated_at)
row.save(validate: false)
row.save(validate: false, changed: true)
rescue Sequel::NoExistingObject
raise Sequel::Rollback
rescue StandardError => e
Expand Down
73 changes: 61 additions & 12 deletions spec/support/fakes/blueprints.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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 { [] }
Expand All @@ -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' }
Expand Down
52 changes: 47 additions & 5 deletions spec/unit/lib/cloud_controller/errands/rotate_database_key_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 57a54c6

Please sign in to comment.