From 8edbb87f53aa92ba9b787dccca976f344bd615df Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 19 May 2025 15:31:17 +0200 Subject: [PATCH 1/2] Catch CipherError and TypeError in run_cipher and raise EncryptorError Wraps OpenSSL::Cipher::CipherError and TypeError in a custom EncryptorError to allow centralized handling in ApplicationController. This enables consistent 500 error responses with clearer messages like "Error while processing encrypted data". --- app/controllers/v3/application_controller.rb | 2 +- lib/cloud_controller/encryptor.rb | 4 ++++ spec/request/apps_spec.rb | 2 +- spec/request/service_brokers_spec.rb | 2 +- spec/request/service_credential_bindings_spec.rb | 2 +- spec/unit/controllers/v3/application_controller_spec.rb | 4 ++-- spec/unit/lib/cloud_controller/encryptor_spec.rb | 9 +++++++++ 7 files changed, 19 insertions(+), 6 deletions(-) diff --git a/app/controllers/v3/application_controller.rb b/app/controllers/v3/application_controller.rb index b992d0199e7..30a8f2ca69d 100644 --- a/app/controllers/v3/application_controller.rb +++ b/app/controllers/v3/application_controller.rb @@ -93,7 +93,7 @@ class ApplicationController < ActionController::Base rescue_from CloudController::Errors::CompoundError, with: :handle_compound_error rescue_from ActionDispatch::Http::Parameters::ParseError, with: :handle_invalid_request_body rescue_from Sequel::DatabaseConnectionError, Sequel::DatabaseDisconnectError, with: :handle_db_connection_error - rescue_from OpenSSL::Cipher::CipherError, with: :handle_key_derivation_error + rescue_from VCAP::CloudController::Encryptor::EncryptorError, with: :handle_key_derivation_error def configuration Config.config diff --git a/lib/cloud_controller/encryptor.rb b/lib/cloud_controller/encryptor.rb index 60a4a754f6d..05dff457c1a 100644 --- a/lib/cloud_controller/encryptor.rb +++ b/lib/cloud_controller/encryptor.rb @@ -9,6 +9,8 @@ module VCAP::CloudController module Encryptor ENCRYPTION_ITERATIONS = 2048 + class EncryptorError < StandardError; end + class << self ALGORITHM = 'AES-128-CBC'.freeze @@ -87,6 +89,8 @@ def run_cipher(cipher, input, salt, key, iterations:) cipher.iv = salt end cipher.update(input) << cipher.final + rescue OpenSSL::Cipher::CipherError, TypeError => e + raise EncryptorError.new("Encryption/Decryption failed: #{e.message}") end def deprecated_short_salt?(salt) diff --git a/spec/request/apps_spec.rb b/spec/request/apps_spec.rb index 37a5998861e..063a2452f7a 100644 --- a/spec/request/apps_spec.rb +++ b/spec/request/apps_spec.rb @@ -3406,7 +3406,7 @@ it 'fails to decrypt the environment variables and returns a 500 error' do app_model # ensure that app model is created before run_cipher is mocked to throw an error - allow(VCAP::CloudController::Encryptor).to receive(:run_cipher).and_raise(OpenSSL::Cipher::CipherError) + allow(VCAP::CloudController::Encryptor).to receive(:run_cipher).and_raise(VCAP::CloudController::Encryptor::EncryptorError) api_call.call(admin_headers) expect(last_response).to have_status_code(500) diff --git a/spec/request/service_brokers_spec.rb b/spec/request/service_brokers_spec.rb index 265cfc596bd..863bd86b988 100644 --- a/spec/request/service_brokers_spec.rb +++ b/spec/request/service_brokers_spec.rb @@ -920,7 +920,7 @@ def expect_empty_list(user_headers) it 'fails to decrypt the broker data and returns a 500 error' do broker # ensure the broker is created before run_cipher is mocked to throw an error - allow(VCAP::CloudController::Encryptor).to receive(:run_cipher).and_raise(OpenSSL::Cipher::CipherError) + allow(VCAP::CloudController::Encryptor).to receive(:run_cipher).and_raise(VCAP::CloudController::Encryptor::EncryptorError) api_call.call(admin_headers) expect(last_response).to have_status_code(500) diff --git a/spec/request/service_credential_bindings_spec.rb b/spec/request/service_credential_bindings_spec.rb index 7176af5f6a8..ae5431b7dce 100644 --- a/spec/request/service_credential_bindings_spec.rb +++ b/spec/request/service_credential_bindings_spec.rb @@ -631,7 +631,7 @@ def check_filtered_bindings(*bindings) it 'fails to decrypt the credentials and returns a 500 error' do app_binding # ensure that binding is created before run_cipher is mocked to throw an error - allow(VCAP::CloudController::Encryptor).to receive(:run_cipher).and_raise(OpenSSL::Cipher::CipherError) + allow(VCAP::CloudController::Encryptor).to receive(:run_cipher).and_raise(VCAP::CloudController::Encryptor::EncryptorError) api_call.call(admin_headers) expect(last_response).to have_status_code(500) diff --git a/spec/unit/controllers/v3/application_controller_spec.rb b/spec/unit/controllers/v3/application_controller_spec.rb index e8c15173345..1d40b81309a 100644 --- a/spec/unit/controllers/v3/application_controller_spec.rb +++ b/spec/unit/controllers/v3/application_controller_spec.rb @@ -39,7 +39,7 @@ def not_found end def key_derivation_error - raise OpenSSL::Cipher::CipherError + raise VCAP::CloudController::Encryptor::EncryptorError end def db_disconnect_error @@ -334,7 +334,7 @@ def warnings_incorrect_type end end - it 'rescues from OpenSSL::Cipher::CipherError and renders an error presenter' do + it 'rescues from EncryptorError and renders an error presenter' do get :key_derivation_error expect(response).to have_http_status(:internal_server_error) expect(response).to have_error_message(/Error while processing encrypted data/) diff --git a/spec/unit/lib/cloud_controller/encryptor_spec.rb b/spec/unit/lib/cloud_controller/encryptor_spec.rb index aa1e460a3f6..713be2318da 100644 --- a/spec/unit/lib/cloud_controller/encryptor_spec.rb +++ b/spec/unit/lib/cloud_controller/encryptor_spec.rb @@ -175,6 +175,15 @@ module VCAP::CloudController expect(result).not_to eq(unencrypted_string) end + + it 'raises an EncryptorError' do + allow(Encryptor).to receive(:current_encryption_key_label).and_return('foo') + encrypted_string = Encryptor.encrypt(unencrypted_string, salt) + + expect do + Encryptor.decrypt(encrypted_string, salt, label: 'bar', iterations: encryption_iterations) + end.to raise_error(VCAP::CloudController::Encryptor::EncryptorError, %r{Encryption/Decryption failed: }) + end end end end From 27a3fbf490262ecf3fecf66ef6c568c8bb2ed33e Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Tue, 20 May 2025 11:01:42 +0200 Subject: [PATCH 2/2] use new EncryptorError instead of CipherError --- errors/v3.yml | 2 +- lib/cloud_controller/validate_database_keys.rb | 3 ++- spec/support/shared_examples/models/encrypted_attribute.rb | 2 +- spec/unit/lib/cloud_controller/encryptor_spec.rb | 4 ++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/errors/v3.yml b/errors/v3.yml index d533a567049..35a7cc6dd35 100644 --- a/errors/v3.yml +++ b/errors/v3.yml @@ -18,7 +18,7 @@ http_code: 429 message: "The UAA is currently rate limited. Please try again later" -10001: +10081: name: InternalServerError http_code: 500 message: "%s" diff --git a/lib/cloud_controller/validate_database_keys.rb b/lib/cloud_controller/validate_database_keys.rb index 2f2ca1826c6..335f043d484 100644 --- a/lib/cloud_controller/validate_database_keys.rb +++ b/lib/cloud_controller/validate_database_keys.rb @@ -56,7 +56,8 @@ def validate_encryption_key_values_unchanged!(config) iterations: sentinel_model.encryption_iterations ) # A failed decryption occasionally results in a CipherError: bad decrypt instead of a garbled string - rescue OpenSSL::Cipher::CipherError + # This is now caught inside Encryptor and re-raised as EncryptorError + rescue VCAP::CloudController::Encryptor::EncryptorError labels_with_changed_keys << label_string next end diff --git a/spec/support/shared_examples/models/encrypted_attribute.rb b/spec/support/shared_examples/models/encrypted_attribute.rb index 17c181a2e3f..33fe6e76f54 100644 --- a/spec/support/shared_examples/models/encrypted_attribute.rb +++ b/spec/support/shared_examples/models/encrypted_attribute.rb @@ -53,7 +53,7 @@ def last_row begin decrypted_value = Encryptor.decrypt(saved_attribute, model.send(attr_salt), label: model.encryption_key_label, iterations: model.encryption_iterations) - rescue OpenSSL::Cipher::CipherError + rescue VCAP::CloudController::Encryptor::EncryptorError errored = true end diff --git a/spec/unit/lib/cloud_controller/encryptor_spec.rb b/spec/unit/lib/cloud_controller/encryptor_spec.rb index 713be2318da..6f7f2bb9122 100644 --- a/spec/unit/lib/cloud_controller/encryptor_spec.rb +++ b/spec/unit/lib/cloud_controller/encryptor_spec.rb @@ -155,7 +155,7 @@ module VCAP::CloudController result = begin Encryptor.decrypt(encrypted_string, salt, iterations: encryption_iterations) - rescue OpenSSL::Cipher::CipherError => e + rescue VCAP::CloudController::Encryptor::EncryptorError => e e.message end @@ -169,7 +169,7 @@ module VCAP::CloudController result = begin Encryptor.decrypt(encrypted_string, salt, label: 'death', iterations: encryption_iterations) - rescue OpenSSL::Cipher::CipherError => e + rescue VCAP::CloudController::Encryptor::EncryptorError => e e.message end