From 86c7e074286661fde1e347791c2b423aba8f07e8 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 12 Apr 2022 21:07:37 +0300 Subject: [PATCH] FEATURE: Improve screened IPs roll up and extend for IPv6 (#15585) This commit improves the logic for rolling up IPv4 screened IP addresses and extending it for IPv6. IPv4 addresses will roll up only up to /24. IPv6 can rollup to /48 at most. The log message that is generated contains the list of original IPs and new subnet. --- app/models/screened_ip_address.rb | 117 ++++++++++------------ app/services/staff_action_logger.rb | 4 +- spec/models/screened_ip_address_spec.rb | 52 ++++++++++ spec/services/staff_action_logger_spec.rb | 10 +- 4 files changed, 112 insertions(+), 71 deletions(-) diff --git a/app/models/screened_ip_address.rb b/app/models/screened_ip_address.rb index 9d25a3c9a1a..70d51c85ec5 100644 --- a/app/models/screened_ip_address.rb +++ b/app/models/screened_ip_address.rb @@ -13,6 +13,17 @@ class ScreenedIpAddress < ActiveRecord::Base validates :ip_address, ip_address_format: true, presence: true after_validation :check_for_match + ROLLED_UP_BLOCKS = [ + # IPv4 + [4, 32, 24], + # IPv6 + [6, (65..128).to_a, 64], + [6, 64, 60], + [6, 60, 56], + [6, 56, 52], + [6, 52, 48], + ] + def self.watch(ip_address, opts = {}) match_for_ip_address(ip_address) || create(opts.slice(:action_type).merge(ip_address: ip_address)) end @@ -67,7 +78,8 @@ class ScreenedIpAddress < ActiveRecord::Base # # http://www.postgresql.org/docs/9.1/static/datatype-net-types.html # http://www.postgresql.org/docs/9.1/static/functions-net.html - order('masklen(ip_address) DESC').find_by("? <<= ip_address", ip_address.to_s) + ip_address = IPAddr === ip_address ? ip_address.to_cidr_s : ip_address.to_s + order('masklen(ip_address) DESC').find_by("? <<= ip_address", ip_address) end def self.should_block?(ip_address) @@ -94,82 +106,57 @@ class ScreenedIpAddress < ActiveRecord::Base !exists_for_ip_address_and_action?(ip_address, actions[:allow_admin], record_match: false) end - def self.star_subnets_query - @star_subnets_query ||= <<~SQL - SELECT network(inet(host(ip_address) || '/24'))::text AS ip_range + def self.subnets(family, from_masklen, to_masklen) + sql = <<~SQL + WITH ips_and_subnets AS ( + SELECT ip_address, + network(inet(host(ip_address) || '/' || :to_masklen))::text subnet FROM screened_ip_addresses - WHERE action_type = #{ScreenedIpAddress.actions[:block]} - AND family(ip_address) = 4 - AND masklen(ip_address) = 32 - GROUP BY ip_range - HAVING COUNT(*) >= :min_count - SQL - end - - def self.star_star_subnets_query - @star_star_subnets_query ||= <<~SQL - WITH weighted_subnets AS ( - SELECT network(inet(host(ip_address) || '/16'))::text AS ip_range, - CASE masklen(ip_address) - WHEN 32 THEN 1 - WHEN 24 THEN :roll_up_weight - ELSE 0 - END AS weight - FROM screened_ip_addresses - WHERE action_type = #{ScreenedIpAddress.actions[:block]} - AND family(ip_address) = 4 + WHERE family(ip_address) = :family AND + masklen(ip_address) IN (:from_masklen) AND + action_type = :blocked ) - SELECT ip_range - FROM weighted_subnets - GROUP BY ip_range - HAVING SUM(weight) >= :min_count + SELECT subnet + FROM ips_and_subnets + GROUP BY subnet + HAVING COUNT(*) >= :min_ban_entries_for_roll_up SQL - end - def self.star_subnets - min_count = SiteSetting.min_ban_entries_for_roll_up - DB.query_single(star_subnets_query, min_count: min_count) - end - - def self.star_star_subnets - weight = SiteSetting.min_ban_entries_for_roll_up - DB.query_single(star_star_subnets_query, min_count: 10, roll_up_weight: weight) + DB.query_single( + sql, + family: family, + from_masklen: from_masklen, + to_masklen: to_masklen, + blocked: ScreenedIpAddress.actions[:block], + min_ban_entries_for_roll_up: SiteSetting.min_ban_entries_for_roll_up, + ) end def self.roll_up(current_user = Discourse.system_user) - subnets = [star_subnets, star_star_subnets].flatten + ROLLED_UP_BLOCKS.each do |family, from_masklen, to_masklen| + ScreenedIpAddress.subnets(family, from_masklen, to_masklen).map do |subnet| + next if ScreenedIpAddress.where("? <<= ip_address", subnet).exists? - StaffActionLogger.new(current_user).log_roll_up(subnets) unless subnets.blank? + old_ips = ScreenedIpAddress + .where(action_type: ScreenedIpAddress.actions[:block]) + .where("ip_address << ?", subnet) + .where("family(ip_address) = ?", family) + .where("masklen(ip_address) IN (?)", from_masklen) - subnets.each do |subnet| - ScreenedIpAddress.create(ip_address: subnet) unless ScreenedIpAddress.where("? <<= ip_address", subnet).exists? + sum_match_count, max_last_match_at, min_created_at = + old_ips.pluck_first('SUM(match_count), MAX(last_match_at), MIN(created_at)') - sql = <<~SQL - UPDATE screened_ip_addresses - SET match_count = sum_match_count - , created_at = min_created_at - , last_match_at = max_last_match_at - FROM ( - SELECT SUM(match_count) AS sum_match_count - , MIN(created_at) AS min_created_at - , MAX(last_match_at) AS max_last_match_at - FROM screened_ip_addresses - WHERE action_type = #{ScreenedIpAddress.actions[:block]} - AND family(ip_address) = 4 - AND ip_address << :ip_address - ) s - WHERE ip_address = :ip_address - SQL + ScreenedIpAddress.create!( + ip_address: subnet, + match_count: sum_match_count, + last_match_at: max_last_match_at, + created_at: min_created_at, + ) - DB.exec(sql, ip_address: subnet) - - ScreenedIpAddress.where(action_type: ScreenedIpAddress.actions[:block]) - .where("family(ip_address) = 4") - .where("ip_address << ?", subnet) - .delete_all + StaffActionLogger.new(current_user).log_roll_up(subnet, old_ips.map(&:ip_address)) + old_ips.delete_all + end end - - subnets end end diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index ecb126b2cea..823a2b6a8a3 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -437,10 +437,10 @@ class StaffActionLogger )) end - def log_roll_up(subnets, opts = {}) + def log_roll_up(subnet, ips, opts = {}) UserHistory.create!(params(opts).merge( action: UserHistory.actions[:roll_up], - details: subnets.join(", ") + details: "#{subnet} from #{ips.join(", ")}" )) end diff --git a/spec/models/screened_ip_address_spec.rb b/spec/models/screened_ip_address_spec.rb index 418ebf2c5f2..d2d12eac578 100644 --- a/spec/models/screened_ip_address_spec.rb +++ b/spec/models/screened_ip_address_spec.rb @@ -320,4 +320,56 @@ describe ScreenedIpAddress do end end end + + describe '#roll_up' do + it 'rolls up IPv4 addresses' do + SiteSetting.min_ban_entries_for_roll_up = 3 + + # this should not be touched + Fabricate(:screened_ip_address, ip_address: "1.1.1.254/31") + + Fabricate(:screened_ip_address, ip_address: "1.1.1.1") + expect { ScreenedIpAddress.roll_up }.not_to change { ScreenedIpAddress.count } + + Fabricate(:screened_ip_address, ip_address: "1.1.1.2") + expect { ScreenedIpAddress.roll_up }.not_to change { ScreenedIpAddress.count } + + Fabricate(:screened_ip_address, ip_address: "1.1.1.3") + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(-2) + expect(ScreenedIpAddress.pluck(:ip_address)).to include("1.1.1.0/24", "1.1.1.254/31") + expect(ScreenedIpAddress.pluck(:ip_address)).not_to include("1.1.1.1", "1.1.1.2", "1.1.1.3") + + # expect roll up to be stable + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(0) + end + + it 'rolls up IPv6 addresses' do + SiteSetting.min_ban_entries_for_roll_up = 3 + + Fabricate(:screened_ip_address, ip_address: "2001:db8:3333:4441:5555:6666:7777:8888") + expect { ScreenedIpAddress.roll_up }.not_to change { ScreenedIpAddress.count } + + Fabricate(:screened_ip_address, ip_address: "2001:db8:3333:4441:5555:6666:7777:8889") + expect { ScreenedIpAddress.roll_up }.not_to change { ScreenedIpAddress.count } + + Fabricate(:screened_ip_address, ip_address: "2001:db8:3333:4441:5555:6666:7777:888a/96") + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(-2) + expect(ScreenedIpAddress.pluck(:ip_address)).to include("2001:db8:3333:4441::/64") + expect(ScreenedIpAddress.pluck(:ip_address)).not_to include("2001:db8:3333:4441:5555:6666:7777:8888", "2001:db8:3333:4441:5555:6666:7777:8889", "2001:db8:3333:4441:5555:6666:7777:888a") + + # expect roll up to be stable + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(0) + + Fabricate(:screened_ip_address, ip_address: "2001:db8:3333:4442::/64") + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(0) + + Fabricate(:screened_ip_address, ip_address: "2001:db8:3333:4443::/64") + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(-2) + expect(ScreenedIpAddress.pluck(:ip_address)).to include("2001:db8:3333:4440::/60") + expect(ScreenedIpAddress.pluck(:ip_address)).not_to include("2001:db8:3333:4441::/64", "2001:db8:3333:4442::/64", "2001:db8:3333:4443::/64") + + # expect roll up to be stable + expect { ScreenedIpAddress.roll_up }.to change { ScreenedIpAddress.count }.by(0) + end + end end diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index bd89342170c..d2abc33b913 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -307,13 +307,15 @@ describe StaffActionLogger do end describe 'log_roll_up' do - let(:subnets) { ["1.2.3.0/24", "42.42.42.0/24"] } - subject(:log_roll_up) { described_class.new(admin).log_roll_up(subnets) } + let(:subnet) { "1.2.3.0/24" } + let(:ips) { ["1.2.3.4", "1.2.3.100"] } + + subject(:log_roll_up) { described_class.new(admin).log_roll_up(subnet, ips) } it 'creates a new UserHistory record' do - log_record = logger.log_roll_up(subnets) + log_record = logger.log_roll_up(subnet, ips) expect(log_record).to be_valid - expect(log_record.details).to eq(subnets.join(", ")) + expect(log_record.details).to eq("#{subnet} from #{ips.join(", ")}") end end