From ca05054ea2a8aff81822f310c5dafb68ae26e10e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 28 Jan 2016 16:28:19 -0500 Subject: Partially revert "Add IP check against DNSBLs at account sign-up" This partially reverts 6a5cd3ca - we keep the migration and add a new migration that reverts it in order to keep migration history intact. --- CHANGELOG | 1 + .../admin/application_settings_controller.rb | 2 - app/controllers/registrations_controller.rb | 5 - app/models/application_setting.rb | 2 - .../admin/application_settings/_form.html.haml | 16 ---- ..._blocking_settings_from_application_settings.rb | 6 ++ db/schema.rb | 4 +- lib/dnsxl_check.rb | 105 --------------------- lib/gitlab/ip_check.rb | 34 ------- spec/lib/dnsxl_check_spec.rb | 68 ------------- 10 files changed, 8 insertions(+), 235 deletions(-) create mode 100644 db/migrate/20160128212447_remove_ip_blocking_settings_from_application_settings.rb delete mode 100644 lib/dnsxl_check.rb delete mode 100644 lib/gitlab/ip_check.rb delete mode 100644 spec/lib/dnsxl_check_spec.rb diff --git a/CHANGELOG b/CHANGELOG index fe0504ec996..9dec6f9809e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,7 @@ v 8.5.0 (unreleased) - Track project import failure - Fix visibility level text in admin area (Zeger-Jan van de Weg) - Update the ExternalIssue regex pattern (Blake Hitchcock) + - Revert "Add IP check against DNSBLs at account sign-up" - Deprecate API "merge_request/:merge_request_id/comments". Use "merge_requests/:merge_request_id/notes" instead - Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." instead diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 094eef28a43..9943745208e 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -74,8 +74,6 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :metrics_timeout, :metrics_method_call_threshold, :metrics_sample_interval, - :ip_blocking_enabled, - :dnsbl_servers_list, :recaptcha_enabled, :recaptcha_site_key, :recaptcha_private_key, diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 5efdd613e79..c48175a4c5a 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -8,11 +8,6 @@ class RegistrationsController < Devise::RegistrationsController def create if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha - if Gitlab::IpCheck.new(request.remote_ip).spam? - flash[:alert] = 'Could not create an account. This IP is listed for spam.' - return render action: 'new' - end - super else flash[:alert] = "There was an error with the reCAPTCHA code below. Please re-enter the code." diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 2f3487b53ac..59563b8823c 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -43,8 +43,6 @@ # metrics_port :integer default(8089) # sentry_enabled :boolean default(FALSE) # sentry_dsn :string -# ip_blocking_enabled :boolean default(FALSE) -# dns_blacklist_threshold :float default(0.33) # class ApplicationSetting < ActiveRecord::Base diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index baadca09518..c4fb2accdd0 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -212,22 +212,6 @@ %fieldset %legend Spam and Anti-bot Protection - .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = f.label :ip_blocking_enabled do - = f.check_box :ip_blocking_enabled - Enable IP check against blacklist at sign-up - .help-block Helps preventing accounts creation from 'known spam sources' - - .form-group - = f.label :dnsbl_servers_list, class: 'control-label col-sm-2' do - DNSBL servers list - .col-sm-10 - = f.text_field :dnsbl_servers_list, class: 'form-control' - .help-block - Please enter DNSBL servers separated with comma - .form-group .col-sm-offset-2.col-sm-10 .checkbox diff --git a/db/migrate/20160128212447_remove_ip_blocking_settings_from_application_settings.rb b/db/migrate/20160128212447_remove_ip_blocking_settings_from_application_settings.rb new file mode 100644 index 00000000000..41821cdcc42 --- /dev/null +++ b/db/migrate/20160128212447_remove_ip_blocking_settings_from_application_settings.rb @@ -0,0 +1,6 @@ +class RemoveIpBlockingSettingsFromApplicationSettings < ActiveRecord::Migration + def change + remove_column :application_settings, :ip_blocking_enabled, :boolean, default: false + remove_column :application_settings, :dnsbl_servers_list, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 97594011a02..2a2911bfbc7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160120172143) do +ActiveRecord::Schema.define(version: 20160128212447) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -64,8 +64,6 @@ ActiveRecord::Schema.define(version: 20160120172143) do t.integer "metrics_sample_interval", default: 15 t.boolean "sentry_enabled", default: false t.string "sentry_dsn" - t.boolean "ip_blocking_enabled", default: false - t.text "dnsbl_servers_list" end create_table "audit_events", force: :cascade do |t| diff --git a/lib/dnsxl_check.rb b/lib/dnsxl_check.rb deleted file mode 100644 index 1e506b2d9cb..00000000000 --- a/lib/dnsxl_check.rb +++ /dev/null @@ -1,105 +0,0 @@ -require 'resolv' - -class DNSXLCheck - - class Resolver - def self.search(query) - begin - Resolv.getaddress(query) - true - rescue Resolv::ResolvError - false - end - end - end - - IP_REGEXP = /\A(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\z/ - DEFAULT_THRESHOLD = 0.33 - - def self.create_from_list(list) - dnsxl_check = DNSXLCheck.new - - list.each do |entry| - dnsxl_check.add_list(entry.domain, entry.weight) - end - - dnsxl_check - end - - def test(ip) - if use_threshold? - test_with_threshold(ip) - else - test_strict(ip) - end - end - - def test_with_threshold(ip) - return false if lists.empty? - - search(ip) - final_score >= threshold - end - - def test_strict(ip) - return false if lists.empty? - - search(ip) - @score > 0 - end - - def use_threshold=(value) - @use_threshold = value == true - end - - def use_threshold? - @use_threshold &&= true - end - - def threshold=(threshold) - raise ArgumentError, "'threshold' value must be grather than 0 and less than or equal to 1" unless threshold > 0 && threshold <= 1 - @threshold = threshold - end - - def threshold - @threshold ||= DEFAULT_THRESHOLD - end - - def add_list(domain, weight) - @lists ||= [] - @lists << { domain: domain, weight: weight } - end - - def lists - @lists ||= [] - end - - private - - def search(ip) - raise ArgumentError, "'ip' value must be in #{IP_REGEXP} format" unless ip.match(IP_REGEXP) - - @score = 0 - - reversed = reverse_ip(ip) - search_in_rbls(reversed) - end - - def reverse_ip(ip) - ip.split('.').reverse.join('.') - end - - def search_in_rbls(reversed_ip) - lists.each do |rbl| - query = "#{reversed_ip}.#{rbl[:domain]}" - @score += rbl[:weight] if Resolver.search(query) - end - end - - def final_score - weights = lists.map{ |rbl| rbl[:weight] }.reduce(:+).to_i - return 0 if weights == 0 - - (@score.to_f / weights.to_f).round(2) - end -end diff --git a/lib/gitlab/ip_check.rb b/lib/gitlab/ip_check.rb deleted file mode 100644 index f2e9b50d225..00000000000 --- a/lib/gitlab/ip_check.rb +++ /dev/null @@ -1,34 +0,0 @@ -module Gitlab - class IpCheck - - def initialize(ip) - @ip = ip - - application_settings = ApplicationSetting.current - @ip_blocking_enabled = application_settings.ip_blocking_enabled - @dnsbl_servers_list = application_settings.dnsbl_servers_list - end - - def spam? - @ip_blocking_enabled && blacklisted? - end - - private - - def blacklisted? - on_dns_blacklist? - end - - def on_dns_blacklist? - dnsbl_check = DNSXLCheck.new - prepare_dnsbl_list(dnsbl_check) - dnsbl_check.test(@ip) - end - - def prepare_dnsbl_list(dnsbl_check) - @dnsbl_servers_list.split(',').map(&:strip).reject(&:empty?).each do |domain| - dnsbl_check.add_list(domain, 1) - end - end - end -end diff --git a/spec/lib/dnsxl_check_spec.rb b/spec/lib/dnsxl_check_spec.rb deleted file mode 100644 index a35a1be0c90..00000000000 --- a/spec/lib/dnsxl_check_spec.rb +++ /dev/null @@ -1,68 +0,0 @@ -require 'spec_helper' -require 'ostruct' - -describe 'DNSXLCheck', lib: true, no_db: true do - let(:spam_ip) { '127.0.0.2' } - let(:no_spam_ip) { '127.0.0.3' } - let(:invalid_ip) { 'a.b.c.d' } - let!(:dnsxl_check) { DNSXLCheck.create_from_list([OpenStruct.new({ domain: 'test', weight: 1 })]) } - - before(:context) do - class DNSXLCheck::Resolver - class << self - alias_method :old_search, :search - def search(query) - return false if query.match(/always\.failing\.domain\z/) - return true if query.match(/\A2\.0\.0\.127\./) - return false if query.match(/\A3\.0\.0\.127\./) - end - end - end - end - - describe '#test' do - before do - dnsxl_check.threshold = 0.75 - dnsxl_check.add_list('always.failing.domain', 1) - end - - context 'when threshold is used' do - before { dnsxl_check.use_threshold= true } - - it { expect(dnsxl_check.test(spam_ip)).to be_falsey } - end - - context 'when threshold is not used' do - before { dnsxl_check.use_threshold= false } - - it { expect(dnsxl_check.test(spam_ip)).to be_truthy } - end - end - - describe '#test_with_threshold' do - it { expect{ dnsxl_check.test_with_threshold(invalid_ip) }.to raise_error(ArgumentError) } - - it { expect(dnsxl_check.test_with_threshold(spam_ip)).to be_truthy } - it { expect(dnsxl_check.test_with_threshold(no_spam_ip)).to be_falsey } - end - - describe '#test_strict' do - before do - dnsxl_check.threshold = 1 - dnsxl_check.add_list('always.failing.domain', 1) - end - - it { expect{ dnsxl_check.test_strict(invalid_ip) }.to raise_error(ArgumentError) } - - it { expect(dnsxl_check.test_with_threshold(spam_ip)).to be_falsey } - it { expect(dnsxl_check.test_with_threshold(no_spam_ip)).to be_falsey } - it { expect(dnsxl_check.test_strict(spam_ip)).to be_truthy } - it { expect(dnsxl_check.test_strict(no_spam_ip)).to be_falsey } - end - - describe '#threshold=' do - it { expect{ dnsxl_check.threshold = 0 }.to raise_error(ArgumentError) } - it { expect{ dnsxl_check.threshold = 1.1 }.to raise_error(ArgumentError) } - it { expect{ dnsxl_check.threshold = 0.5 }.not_to raise_error } - end -end -- cgit v1.2.1