Catch CipherError and TypeError in run_cipher and raise EncryptorError (#4365)

* 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".

Author: kathap

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

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"

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)

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

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)

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)

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)

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
 

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/)

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,12 +169,21 @@ 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
 
               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