Commit c3943fb2 authored by Tan Le's avatar Tan Le Committed by Nikola Milojevic

Remove IP address input when creating audit events

`current_sign_in_ip` could become stale if the user has not logged out.
It should only be used as a fallback if request IP address can not be
resolved from the request context.

To avoid issue of injecting a wrong IP address, we are no longer
accepting this information when invoking the `AuditEventService` and
`Auditor`.
parent 8bd3a911
......@@ -151,14 +151,14 @@ module AuthenticatesWithTwoFactor
def handle_two_factor_failure(user, method, message)
user.increment_failed_attempts!
log_failed_two_factor(user, method, request.remote_ip)
log_failed_two_factor(user, method)
Gitlab::AppLogger.info("Failed Login: user=#{user.username} ip=#{request.remote_ip} method=#{method}")
flash.now[:alert] = message
prompt_for_two_factor(user)
end
def log_failed_two_factor(user, method, ip_address)
def log_failed_two_factor(user, method)
# overridden in EE
end
......
......@@ -98,7 +98,7 @@ module AuthenticatesWithTwoFactorForAdminMode
def admin_handle_two_factor_failure(user, method, message)
user.increment_failed_attempts!
log_failed_two_factor(user, method, request.remote_ip)
log_failed_two_factor(user, method)
Gitlab::AppLogger.info("Failed Admin Mode Login: user=#{user.username} ip=#{request.remote_ip} method=#{method}")
flash.now[:alert] = message
......
......@@ -16,7 +16,7 @@ class AuditEventService
@author = build_author(author)
@entity = entity
@details = details
@ip_address = resolve_ip_address(@details, @author)
@ip_address = resolve_ip_address(@author)
end
# Builds the @details attribute for authentication
......@@ -64,9 +64,8 @@ class AuditEventService
end
end
def resolve_ip_address(details, author)
details[:ip_address].presence ||
Gitlab::RequestContext.instance.client_ip ||
def resolve_ip_address(author)
Gitlab::RequestContext.instance.client_ip ||
author.current_sign_in_ip
end
......
......@@ -5,11 +5,10 @@ module EE
extend ::Gitlab::Utils::Override
override :log_failed_two_factor
def log_failed_two_factor(user, method, ip_address)
def log_failed_two_factor(user, method)
::AuditEventService.new(
user,
user,
ip_address: ip_address,
with: method
).for_failed_login.unauth_security_event
end
......
......@@ -21,7 +21,6 @@ module EE
::AuditEventService.new(
author,
nil,
ip_address: request.remote_ip,
with: provider
).for_failed_login.unauth_security_event
end
......
......@@ -64,7 +64,7 @@ module EE
override :log_failed_login
def log_failed_login
login = request.filtered_parameters.dig('user', 'login')
audit_event_service = ::AuditEventService.new(login, nil, ip_address: request.remote_ip)
audit_event_service = ::AuditEventService.new(login, nil)
audit_event_service.for_failed_login.unauth_security_event
super
......
......@@ -8,13 +8,13 @@ module AuditEvents
# @raise [MissingAttributeError] when required attributes are blank
#
# @return [BuildService]
def initialize(author:, scope:, target:, ip_address:, message:)
raise MissingAttributeError if missing_attribute?(author, scope, target, ip_address, message)
def initialize(author:, scope:, target:, message:)
raise MissingAttributeError if missing_attribute?(author, scope, target, message)
@author = build_author(author)
@scope = scope
@target = target
@ip_address = ip_address
@ip_address = build_ip_address
@message = build_message(message)
end
......@@ -27,7 +27,7 @@ module AuditEvents
private
def missing_attribute?(author, scope, target, ip_address, message)
def missing_attribute?(author, scope, target, message)
author.blank? || scope.blank? || target.blank? || message.blank?
end
......@@ -35,11 +35,11 @@ module AuditEvents
if License.feature_available?(:admin_audit_log)
base_payload.merge(
details: base_details_payload.merge(
ip_address: ip_address,
ip_address: @ip_address,
entity_path: @scope.full_path,
custom_message: @message
),
ip_address: ip_address
ip_address: @ip_address
)
else
base_payload.merge(details: base_details_payload)
......@@ -78,8 +78,8 @@ module AuditEvents
end
end
def ip_address
@ip_address.presence || @author.current_sign_in_ip
def build_ip_address
Gitlab::RequestContext.instance.client_ip || @author.current_sign_in_ip
end
end
end
......@@ -33,8 +33,7 @@ module EE
target_type: token&.class&.name,
target_details: token&.user&.name,
action: :custom,
custom_message: message,
ip_address: current_user.current_sign_in_ip
custom_message: message
).security_event
end
end
......
......@@ -26,8 +26,7 @@ module EE
target_type: token.class.name,
target_details: token.user.name,
action: :custom,
custom_message: message,
ip_address: current_user.current_sign_in_ip
custom_message: message
).security_event
end
end
......
......@@ -10,7 +10,6 @@ module Gitlab
# @option context [User] :author the user who authors the change
# @option context [User, Project, Group] :scope the scope which audit event belongs to
# @option context [Object] :target the target object being audited
# @option context [Object] :ip_address the request IP address
# @option context [String] :message the message describing the action
#
# @example Using block (useful when events are emitted deep in the call stack)
......@@ -21,7 +20,6 @@ module Gitlab
# author: current_user,
# scope: project_alpha,
# target: merge_approval_rule,
# ip_address: request.remote_ip
# message: 'a user has attempted to update an approval rule'
# }
#
......@@ -58,7 +56,6 @@ module Gitlab
@author = @context.fetch(:author)
@scope = @context.fetch(:scope)
@target = @context.fetch(:target)
@ip_address = @context.fetch(:ip_address, nil)
@message = @context.fetch(:message, '')
end
......@@ -91,7 +88,6 @@ module Gitlab
author: @author,
scope: @scope,
target: @target,
ip_address: @ip_address,
message: message
).execute
end
......
......@@ -7,8 +7,7 @@ RSpec.describe Gitlab::Audit::Auditor do
let(:author) { build_stubbed(:user) }
let(:scope) { build_stubbed(:group) }
let(:target) { build_stubbed(:project) }
let(:ip_address) { '192.168.8.8' }
let(:context) { { name: name, author: author, scope: scope, target: target, ip_address: ip_address } }
let(:context) { { name: name, author: author, scope: scope, target: target } }
let(:add_message) { 'Added an interesting field from project Gotham' }
let(:remove_message) { 'Removed an interesting field from project Gotham' }
let(:operation) do
......@@ -85,7 +84,7 @@ RSpec.describe Gitlab::Audit::Auditor do
let(:audit!) { auditor.audit(context) }
let(:context) do
{
name: name, author: author, scope: scope, target: target, ip_address: ip_address,
name: name, author: author, scope: scope, target: target,
message: 'Project has been deleted'
}
end
......
......@@ -2,16 +2,20 @@
require 'spec_helper'
RSpec.describe AuditEventService do
RSpec.describe AuditEventService, :request_store do
let(:project) { build_stubbed(:project) }
let_it_be(:user) { create(:user, current_sign_in_ip: '192.168.68.104') }
let_it_be(:project_member) { create(:project_member, user: user, expires_at: 1.day.from_now) }
let(:request_ip_address) { '127.0.0.1' }
let(:details) { { action: :destroy, ip_address: request_ip_address } }
let(:details) { { action: :destroy } }
let(:service) { described_class.new(user, project, details) }
before do
allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(request_ip_address)
end
describe '#for_member' do
let(:event) { service.for_member(project_member).security_event }
let(:event_details) { event[:details] }
......@@ -103,36 +107,59 @@ RSpec.describe AuditEventService do
end
context 'for an unauthenticated user' do
let(:details) { { ip_address: '10.11.12.13' } }
let(:user) { Gitlab::Audit::UnauthenticatedAuthor.new }
it 'defaults to the IP address in the details hash' do
event = service.security_event
context 'when request IP address is present' do
it 'has the request IP address' do
event = service.security_event
expect(event.ip_address).to eq('10.11.12.13')
expect(event.details[:ip_address]).to eq('10.11.12.13')
expect(event.details[:ip_address]).to eq(request_ip_address)
expect(event.ip_address).to eq(request_ip_address)
end
end
context 'when request IP address is not present' do
let(:request_ip_address) { nil }
it 'has the user IP address' do
event = service.security_event
expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
expect(event.ip_address).to eq(user.current_sign_in_ip)
end
end
end
context 'for an authenticated user' do
let(:details) { {} }
context 'when request IP address is present' do
it 'has the request IP address' do
event = service.security_event
it 'has the user IP address' do
event = service.security_event
expect(event.ip_address).to eq(user.current_sign_in_ip)
expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
expect(event.details[:ip_address]).to eq(request_ip_address)
expect(event.ip_address).to eq(request_ip_address)
end
end
it 'tracks exceptions when the event cannot be created' do
allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
context 'when request IP address is not present' do
let(:request_ip_address) { nil }
it 'has the user IP address' do
event = service.security_event
expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
expect(event.ip_address).to eq(user.current_sign_in_ip)
end
expect(Gitlab::ErrorTracking).to(
receive(:track_exception)
.with(ActiveRecord::RecordInvalid, audit_event_type: 'AuditEvent').and_call_original
)
it 'tracks exceptions when the event cannot be created' do
allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
service.security_event
expect(Gitlab::ErrorTracking).to(
receive(:track_exception)
.with(ActiveRecord::RecordInvalid, audit_event_type: 'AuditEvent').and_call_original
)
service.security_event
end
end
end
......@@ -141,11 +168,24 @@ RSpec.describe AuditEventService do
let(:impersonator) { build(:user, name: 'Donald Duck', current_sign_in_ip: '192.168.88.88') }
let(:user) { create(:user, impersonator: impersonator) }
it 'has the impersonator IP address' do
event = service.security_event
context 'when request IP address is present' do
it 'has the request IP address' do
event = service.security_event
expect(event.details[:ip_address]).to eq('192.168.88.88')
expect(event.ip_address).to eq('192.168.88.88')
expect(event.details[:ip_address]).to eq(request_ip_address)
expect(event.ip_address).to eq(request_ip_address)
end
end
context 'when request IP address is not present' do
let(:request_ip_address) { nil }
it 'has the impersonator IP address' do
event = service.security_event
expect(event.details[:ip_address]).to eq(impersonator.current_sign_in_ip)
expect(event.ip_address).to eq(impersonator.current_sign_in_ip)
end
end
it 'has the impersonator name' do
......@@ -245,7 +285,7 @@ RSpec.describe AuditEventService do
describe '#for_failed_login' do
let(:author_name) { 'testuser' }
let(:service) { described_class.new(author_name, nil, ip_address: request_ip_address) }
let(:service) { described_class.new(author_name, nil) }
let(:event) { service.for_failed_login.unauth_security_event }
before do
......@@ -520,9 +560,7 @@ RSpec.describe AuditEventService do
expect(event.details[:entity_path]).to eq(project.full_path)
end
context 'request IP address is provided' do
let(:details) { { action: :destroy, ip_address: request_ip_address } }
context 'request IP address is present' do
it 'has the IP address in the details hash' do
expect(event.details[:ip_address]).to eq(request_ip_address)
end
......@@ -532,8 +570,8 @@ RSpec.describe AuditEventService do
end
end
context 'request IP address is not provided' do
let(:details) { { action: :destroy } }
context 'request IP address is not present' do
let(:request_ip_address) { nil }
it 'has the IP address in the details hash' do
expect(event.details[:ip_address]).to eq(user.current_sign_in_ip)
......
......@@ -14,14 +14,17 @@ RSpec.describe AuditEvents::BuildService do
author: author,
scope: scope,
target: target,
ip_address: ip_address,
message: message
)
end
describe '#execute' do
describe '#execute', :request_store do
subject(:event) { service.execute }
before do
allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(ip_address)
end
context 'when licensed' do
before do
stub_licensed_features(admin_audit_log: true)
......
......@@ -7,7 +7,7 @@ RSpec.shared_examples 'an auditable failed authentication' do
operation
expect(AuditEventService).to have_received(:new).with(user, user, ip_address: '0.0.0.0', with: method)
expect(AuditEventService).to have_received(:new).with(user, user, with: method)
expect(audit_event_service).to have_received(:for_failed_login)
expect(audit_event_service).to have_received(:unauth_security_event)
end
......
......@@ -79,15 +79,14 @@ RSpec.describe AuditEventService do
context 'with IP address', :request_store do
using RSpec::Parameterized::TableSyntax
where(:from_caller, :from_context, :from_author_sign_in, :output) do
'192.168.0.1' | '192.168.0.2' | '192.168.0.3' | '192.168.0.1'
nil | '192.168.0.2' | '192.168.0.3' | '192.168.0.2'
nil | nil | '192.168.0.3' | '192.168.0.3'
where(:from_context, :from_author_sign_in, :output) do
'192.168.0.2' | '192.168.0.3' | '192.168.0.2'
nil | '192.168.0.3' | '192.168.0.3'
end
with_them do
let(:user) { create(:user, current_sign_in_ip: from_author_sign_in) }
let(:audit_service) { described_class.new(user, user, with: 'standard', ip_address: from_caller) }
let(:audit_service) { described_class.new(user, user, with: 'standard') }
before do
allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(from_context)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment