Add error handling for invalid encryption keys with logging (#4326)

* Add error handling for invalid encryption keys with logging

- Wrapped pbkdf2_hmac in a begin-rescue block to catch encryption key errors.
- Added detailed error logging for failed key derivation.

* New log only for key=nil

* Throw appropriate error instead of UnknownError for GET /v3/service_credential_bindings/:binding_guid/details when encryption-key-label is invalid

* enhance error handling

* add test for rescue_from decryption error

* change error name

* handle cipher error in central place in applications controller

* add test for GET /v3/apps/:guid/environment_variables when the encryption_key_label is invalid

* add test for service borker update when the encryption_key_label is invalid

* rename error message, remove obsolete parts in test setups

* Update spec/request/apps_spec.rb

---------

Co-authored-by: Philipp Thun <philipp.thun@sap.com>

Author: kathap

app/controllers/v3/application_controller.rb

@@ -93,6 +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
 
   def configuration
     Config.config
@@ -219,6 +220,11 @@ def handle_db_connection_error(_)
     handle_api_error(error)
   end
 
+  def handle_key_derivation_error(_)
+    error = CloudController::Errors::V3::ApiError.new_from_details('InternalServerError', 'Error while processing encrypted data')
+    handle_api_error(error)
+  end
+
   def handle_exception(error)
     presenter = ErrorPresenter.new(error, Rails.env.test?, V3ErrorHasher.new(error))
     logger.info(presenter.log_message)

errors/v3.yml

@@ -17,3 +17,8 @@
   name: UaaRateLimited
   http_code: 429
   message: "The UAA is currently rate limited. Please try again later"
+
+10001:
+  name: InternalServerError
+  http_code: 500
+  message: "%s"

spec/request/apps_spec.rb

@@ -3398,6 +3398,21 @@
         end
       end
     end
+
+    context 'when the encryption_key_label is invalid' do
+      before do
+        allow_any_instance_of(ErrorPresenter).to receive(:raise_500?).and_return(false)
+      end
+
+      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)
+        api_call.call(admin_headers)
+
+        expect(last_response).to have_status_code(500)
+        expect(parsed_response['errors'].first['detail']).to match(/Error while processing encrypted data/i)
+      end
+    end
   end
 
   describe 'GET /v3/apps/:guid/permissions' do

spec/request/service_brokers_spec.rb

@@ -899,6 +899,34 @@ def expect_empty_list(user_headers)
         expect(response).to include('detail' => 'Service broker not found')
       end
     end
+
+    context 'when updating credentials and the encryption_key_label is invalid' do
+      let(:broker) { VCAP::CloudController::ServiceBroker.make }
+      let(:api_call) do
+        lambda { |headers|
+          patch "/v3/service_brokers/#{broker.guid}", { authentication: {
+            type: 'basic',
+            credentials: {
+              username: 'your-username',
+              password: 'your-password'
+            }
+          } }.to_json, headers
+        }
+      end
+
+      before do
+        allow_any_instance_of(ErrorPresenter).to receive(:raise_500?).and_return(false)
+      end
+
+      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)
+        api_call.call(admin_headers)
+
+        expect(last_response).to have_status_code(500)
+        expect(parsed_response['errors'].first['detail']).to match(/Error while processing encrypted data/i)
+      end
+    end
   end
 
   describe 'POST /v3/service_brokers' do

spec/request/service_credential_bindings_spec.rb

@@ -624,6 +624,21 @@ def check_filtered_bindings(*bindings)
       }
     end
 
+    context 'when the encryption_key_label is invalid' do
+      before do
+        allow_any_instance_of(ErrorPresenter).to receive(:raise_500?).and_return(false)
+      end
+
+      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)
+        api_call.call(admin_headers)
+
+        expect(last_response).to have_status_code(500)
+        expect(parsed_response['errors'].first['detail']).to match(/Error while processing encrypted data/i)
+      end
+    end
+
     context "last binding operation is in 'create succeeded' state" do
       before do
         app_binding.save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })

spec/unit/controllers/v3/application_controller_spec.rb

@@ -38,14 +38,18 @@ def not_found
       raise CloudController::Errors::NotFound.new_from_details('NotFound')
     end
 
-    def db_connection_error
-      raise Sequel::DatabaseConnectionError.new
+    def key_derivation_error
+      raise OpenSSL::Cipher::CipherError
     end
 
     def db_disconnect_error
       raise Sequel::DatabaseDisconnectError.new
     end
 
+    def db_connection_error
+      raise Sequel::DatabaseConnectionError.new
+    end
+
     def warnings_is_nil
       add_warning_headers(nil)
       render status: :ok, json: {}
@@ -320,6 +324,23 @@ def warnings_incorrect_type
     end
   end
 
+  describe '#handle_key_derivation_error' do
+    let!(:user) { set_current_user(VCAP::CloudController::User.make) }
+
+    before do
+      allow_any_instance_of(ErrorPresenter).to receive(:raise_500?).and_return(false)
+      routes.draw do
+        get 'key_derivation_error' => 'anonymous#key_derivation_error'
+      end
+    end
+
+    it 'rescues from OpenSSL::Cipher::CipherError 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/)
+    end
+  end
+
   describe '#add_warning_headers' do
     let!(:user) { set_current_user(VCAP::CloudController::User.make) }