From 8299713a8f0658ef20461bd726c8520231deab4d Mon Sep 17 00:00:00 2001 From: Johannes Haass Date: Fri, 20 Oct 2023 15:46:20 +0200 Subject: [PATCH] Implement additional audit events Some API endpoints which return credentials do not write audit events. This change adds the following audit events: - `audit.service_instance.show` & `audit.user_provided_service_instance.show` Endpoints: - GET /v2/service_instances/:guid - GET /v3/service_instances/:guid/credentials - `audit.service_binding.show` Endpoints: - GET /v3/service_credential_bindings/:guid/details - GET /v2/service_bindings/:guid - `audit.service_key.show` Endpoints: - GET /v3/service_credential_bindings/:guid/details - GET /v2/service_keys/:guid Co-authored-by: Wei Quan --- .../services/service_bindings_controller.rb | 13 ++++-- .../services/service_instances_controller.rb | 7 ++++ .../services/service_keys_controller.rb | 2 + .../service_credential_bindings_controller.rb | 12 +++++- .../v3/service_instances_controller.rb | 2 + ...ervice_generic_binding_event_repository.rb | 17 ++++++++ .../service_credential_bindings_spec.rb | 40 +++++++++++++++++++ spec/request/service_instances_spec.rb | 23 +++++++++++ spec/request/v2/service_bindings_spec.rb | 17 ++++++++ spec/request/v2/service_keys_spec.rb | 17 ++++++++ .../service_instances_controller_spec.rb | 31 ++++++++++++++ 11 files changed, 176 insertions(+), 5 deletions(-) diff --git a/app/controllers/services/service_bindings_controller.rb b/app/controllers/services/service_bindings_controller.rb index 62d76c48684..9c831db9036 100644 --- a/app/controllers/services/service_bindings_controller.rb +++ b/app/controllers/services/service_bindings_controller.rb @@ -22,11 +22,16 @@ class ServiceBindingsController < RestController::ModelController get path_guid, :read def read(guid) - obj = find_guid(guid) - raise CloudController::Errors::ApiError.new_from_details('ServiceBindingNotFound', guid) if obj.v2_app.blank? + service_binding = find_guid(guid) + raise CloudController::Errors::ApiError.new_from_details('ServiceBindingNotFound', guid) if service_binding.v2_app.blank? - validate_access(:read, obj) - object_renderer.render_json(self.class, obj, @opts) + validate_access(:read, service_binding) + + Repositories::ServiceGenericBindingEventRepository.new( + Repositories::ServiceGenericBindingEventRepository::SERVICE_APP_CREDENTIAL_BINDING + ).record_show(service_binding, UserAuditInfo.from_context(SecurityContext)) + + object_renderer.render_json(self.class, service_binding, @opts) end post path, :create diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 6c303eda7ce..e7167148860 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/app/controllers/services/service_instances_controller.rb @@ -149,6 +149,13 @@ def update(guid) def read(guid) service_instance = find_guid_and_validate_access(:read, guid, ServiceInstance) + + if service_instance.instance_of?(UserProvidedServiceInstance) + @services_event_repository.record_user_provided_service_instance_event(:show, service_instance) + else + @services_event_repository.record_service_instance_event(:show, service_instance) + end + object_renderer.render_json(self.class, service_instance, @opts) end diff --git a/app/controllers/services/service_keys_controller.rb b/app/controllers/services/service_keys_controller.rb index dad61b02189..5d073d52cd2 100644 --- a/app/controllers/services/service_keys_controller.rb +++ b/app/controllers/services/service_keys_controller.rb @@ -73,6 +73,8 @@ def read(guid) e.name == 'NotAuthorized' ? raise(CloudController::Errors::ApiError.new_from_details('ServiceKeyNotFound', guid)) : raise(e) end + @services_event_repository.record_service_key_event(:show, service_key) + [HTTP::OK, { 'Location' => "#{self.class.path}/#{service_key.guid}" }, object_renderer.render_json(self.class, service_key, @opts)] diff --git a/app/controllers/v3/service_credential_bindings_controller.rb b/app/controllers/v3/service_credential_bindings_controller.rb index cdffe911334..addd9ff02af 100644 --- a/app/controllers/v3/service_credential_bindings_controller.rb +++ b/app/controllers/v3/service_credential_bindings_controller.rb @@ -131,7 +131,7 @@ def details not_found! unless can_read_secrets_in_the_binding_space? not_found_with_message!(service_credential_binding) unless service_credential_binding.create_succeeded? - credentials = if service_credential_binding[:type] == 'key' && service_credential_binding.credhub_reference? + credentials = if service_credential_binding.is_a?(ServiceKey) && service_credential_binding.credhub_reference? fetch_credentials_value(service_credential_binding.credhub_reference) else service_credential_binding.credentials @@ -142,6 +142,16 @@ def details credentials: credentials ).to_hash + type = if service_credential_binding.is_a?(ServiceKey) + Repositories::ServiceGenericBindingEventRepository::SERVICE_KEY_CREDENTIAL_BINDING + else + Repositories::ServiceGenericBindingEventRepository::SERVICE_APP_CREDENTIAL_BINDING + end + Repositories::ServiceGenericBindingEventRepository.new(type).record_show( + service_credential_binding, + user_audit_info + ) + render status: :ok, json: details end diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 3b44e3319c1..26f792f88f3 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -199,6 +199,8 @@ def credentials service_instance_not_found! unless service_instance && can_read_service_instance?(service_instance) unauthorized! unless permission_queryer.can_read_secrets_in_space?(service_instance.space_id, service_instance.space.organization_id) + service_event_repository.record_user_provided_service_instance_event(:show, service_instance) + render status: :ok, json: (service_instance.credentials || {}) end diff --git a/app/repositories/service_generic_binding_event_repository.rb b/app/repositories/service_generic_binding_event_repository.rb index 1fbe1eeb75a..af4020e1a9f 100644 --- a/app/repositories/service_generic_binding_event_repository.rb +++ b/app/repositories/service_generic_binding_event_repository.rb @@ -76,6 +76,23 @@ def record_delete(service_binding, user_audit_info) ) end + def record_show(service_binding, user_audit_info) + metadata = { + request: { + service_instance_guid: service_binding.service_instance_guid + } + } + + metadata[:request].merge!({ app_guid: service_binding.app_guid }) if service_binding.try(:app_guid) + + record_event( + type: "audit.#{@actee_name}.show", + service_binding: service_binding, + user_audit_info: user_audit_info, + metadata: metadata + ) + end + private def censor_request_attributes(request) diff --git a/spec/request/service_credential_bindings_spec.rb b/spec/request/service_credential_bindings_spec.rb index 24e6e2107e5..7bdf089f5ee 100644 --- a/spec/request/service_credential_bindings_spec.rb +++ b/spec/request/service_credential_bindings_spec.rb @@ -617,6 +617,26 @@ def check_filtered_bindings(*bindings) api_call.call(admin_headers) expect(last_response).to have_status_code(200) end + + it 'logs an audit event' do + api_call.call(admin_headers) + event = VCAP::CloudController::Event.find(type: 'audit.service_binding.show') + expect(event).to be + + expect(event.type).to eq('audit.service_binding.show') + expect(event.actee_type).to eq('service_binding') + + expect(event.metadata).to match( + { + 'request' => { + 'app_guid' => app_binding.app_guid, + 'service_instance_guid' => app_binding.service_instance_guid + } + } + ) + + expect(event.actee).to eq(app_binding.guid) + end end context "last binding operation is in 'create in progress' state" do @@ -792,6 +812,26 @@ def check_filtered_bindings(*bindings) expect(credhub_server_stub).not_to have_been_requested expect(parsed_response['credentials']).to eq(key_binding.credentials) end + + it 'logs an audit event' do + api_call.call(admin_headers) + event = VCAP::CloudController::Event.find(type: 'audit.service_key.show') + expect(event).to be + + expect(event.actee).to eq(key_binding.guid) + expect(event.type).to eq('audit.service_key.show') + expect(event.actee_type).to eq('service_key') + + expect(event.metadata).to match( + { + 'request' => { + 'service_instance_guid' => key_binding.service_instance_guid + } + } + ) + + expect(event.actee).to eq(key_binding.guid) + end end context 'for app bindings with credhub references' do diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index f1b0249fd17..e07f34c5b99 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -491,6 +491,12 @@ def check_filtered_instances(*instances) end describe 'GET /v3/service_instances/:guid/credentials' do + let(:space_dev_headers) do + org.add_user(user) + space.add_developer(user) + headers_for(user) + end + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS do let(:api_call) { ->(user_headers) { get "/v3/service_instances/#{guid}/credentials", nil, user_headers } } let(:credentials) { { 'fake-key' => 'fake-value' } } @@ -526,6 +532,23 @@ def check_filtered_instances(*instances) get "/v3/service_instances/#{msi.guid}/credentials", nil, admin_headers expect(last_response).to have_status_code(404) end + + it 'records an audit event' do + upsi = VCAP::CloudController::UserProvidedServiceInstance.make(space: space, credentials: {}) + get "/v3/service_instances/#{upsi.guid}/credentials", nil, space_dev_headers + expect(last_response).to have_status_code(200) + + event = VCAP::CloudController::Event.last + expect(event.values).to include({ + type: 'audit.user_provided_service_instance.show', + actor: user.guid, + actee: upsi.guid, + actee_type: 'user_provided_service_instance', + actee_name: upsi.name, + space_guid: space.guid, + organization_guid: space.organization.guid + }) + end end describe 'GET /v3/service_instances/:guid/parameters' do diff --git a/spec/request/v2/service_bindings_spec.rb b/spec/request/v2/service_bindings_spec.rb index f86f3e64472..d470ac6f271 100644 --- a/spec/request/v2/service_bindings_spec.rb +++ b/spec/request/v2/service_bindings_spec.rb @@ -311,6 +311,23 @@ get "/v2/service_bindings/#{non_displayed_binding.guid}", nil, headers_for(user) expect(last_response.status).to eq(404) end + + it 'creates an audit event' do + get "/v2/service_bindings/#{service_binding1.guid}", nil, headers_for(user) + + event = VCAP::CloudController::Event.last + expect(event.type).to eq('audit.service_binding.show') + expect(event.actee).to eq(service_binding1.guid) + expect(event.actee_type).to eq('service_binding') + expect(event.metadata).to match( + { + 'request' => { + 'app_guid' => service_binding1.app_guid, + 'service_instance_guid' => service_binding1.service_instance_guid + } + } + ) + end end describe 'POST /v2/service_bindings' do diff --git a/spec/request/v2/service_keys_spec.rb b/spec/request/v2/service_keys_spec.rb index f2abcae0848..32aa929c339 100644 --- a/spec/request/v2/service_keys_spec.rb +++ b/spec/request/v2/service_keys_spec.rb @@ -89,6 +89,23 @@ } ) end + + it 'creates an audit event' do + get "/v2/service_keys/#{service_key1.guid}", nil, headers_for(user) + + event = VCAP::CloudController::Event.last + expect(event.type).to eq('audit.service_key.show') + expect(event.actee).to eq(service_key1.guid) + expect(event.actee_type).to eq('service_key') + expect(event.metadata).to match( + { + 'request' => { + 'name' => service_key1.name, + 'service_instance_guid' => service_key1.service_instance_guid + } + } + ) + end end describe 'GET /v2/service_keys/:guid/parameters' do diff --git a/spec/unit/controllers/services/service_instances_controller_spec.rb b/spec/unit/controllers/services/service_instances_controller_spec.rb index 7392f892438..779e65b1439 100644 --- a/spec/unit/controllers/services/service_instances_controller_spec.rb +++ b/spec/unit/controllers/services/service_instances_controller_spec.rb @@ -1563,6 +1563,22 @@ def stub_delete_and_return(status, body) expect(last_response.status).to eq(200) expect(decoded_response.fetch('metadata').fetch('guid')).to eq(service_instance.guid) end + + it 'creates a service audit event for showing the service instance' do + set_current_user(developer) + get "v2/service_instances/#{service_instance.guid}" + + event = VCAP::CloudController::Event.first(type: 'audit.service_instance.show') + expect(event.type).to eq('audit.service_instance.show') + expect(event.actor_type).to eq('user') + expect(event.actor).to eq(developer.guid) + expect(event.timestamp).to be + expect(event.actee).to eq(service_instance.guid) + expect(event.actee_type).to eq('service_instance') + expect(event.actee_name).to eq(service_instance.name) + expect(event.space_guid).to eq(service_instance.space.guid) + expect(event.organization_guid).to eq(service_instance.space.organization.guid) + end end context 'with a user provided service instance' do @@ -1581,6 +1597,21 @@ def stub_delete_and_return(status, body) expect(last_response.status).to eq(200) expect(decoded_response.fetch('entity').fetch('service_bindings_url')).to include('user_provided_service_instances') end + + it 'creates a service audit event for showing the service instance' do + get "v2/service_instances/#{service_instance.guid}" + + event = VCAP::CloudController::Event.first(type: 'audit.user_provided_service_instance.show') + expect(event.type).to eq('audit.user_provided_service_instance.show') + expect(event.actor_type).to eq('user') + expect(event.actor).to eq(developer.guid) + expect(event.timestamp).to be + expect(event.actee).to eq(service_instance.guid) + expect(event.actee_type).to eq('user_provided_service_instance') + expect(event.actee_name).to eq(service_instance.name) + expect(event.space_guid).to eq(service_instance.space.guid) + expect(event.organization_guid).to eq(service_instance.space.organization.guid) + end end end