Merge branch '4193-ip-whitelisting-for-geo-enabling-functionality-in-the-primary' into 'master'

Resolve "IP whitelisting for Geo-enabling functionality in the primary"

Closes #4193

See merge request gitlab-org/gitlab-ee!10383

db/schema.rb

@@ -218,6 +218,7 @@ ActiveRecord::Schema.define(version: 20190403161806) do
     t.integer "first_day_of_week", default: 0, null: false
     t.boolean "elasticsearch_limit_indexing", default: false, null: false
     t.integer "default_project_creation", default: 2, null: false
+    t.string "geo_node_allowed_ips", default: "0.0.0.0/0, ::/0"
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id", using: :btree
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id", using: :btree
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree

doc/api/settings.md

@@ -64,7 +64,8 @@ Example response:
    "instance_statistics_visibility_private": false,
    "user_show_add_ssh_key_message": true,
    "file_template_project_id": 1,
-   "local_markdown_version": 0
+   "local_markdown_version": 0,
+   "geo_node_allowed_ips": "0.0.0.0/0, ::/0"
 }
 ```
 
@@ -127,7 +128,8 @@ Example response:
   "instance_statistics_visibility_private": false,
   "user_show_add_ssh_key_message": true,
   "file_template_project_id": 1,
-  "local_markdown_version": 0
+  "local_markdown_version": 0,
+  "geo_node_allowed_ips": "0.0.0.0/0, ::/0"
 }
 ```
 
@@ -278,3 +280,4 @@ are listed in the descriptions of the relevant settings.
 | `user_show_add_ssh_key_message`          | boolean          | no                                   | When set to `false` disable the "You won't be able to pull or push project code via SSH" warning shown to users with no uploaded SSH key. |
 | `version_check_enabled`                  | boolean          | no                                   | Let GitLab inform you when an update is available. |
 | `local_markdown_version`                 | integer          | no                                   | Increase this value when any cached markdown should be invalidated. |
+| `geo_node_allowed_ips`                 | string          | yes                                   | **(Premium)** Comma-separated list of IPs and CIDRs of allowed secondary nodes. For example, `1.1.1.1, 2.2.2.0/24`. |

ee/app/controllers/ee/projects/git_http_controller.rb

@@ -47,15 +47,16 @@ module EE
       override :authenticate_user
       def authenticate_user
         return super unless geo_request?
-        return render_bad_geo_auth('Bad token') unless decoded_authorization
-        return render_bad_geo_auth('Unauthorized scope') unless jwt_scope_valid?
+        return render_bad_geo_response('Request from this IP is not allowed') unless ip_allowed?
+        return render_bad_geo_jwt('Bad token') unless decoded_authorization
+        return render_bad_geo_jwt('Unauthorized scope') unless jwt_scope_valid?
 
         # grant access
         @authentication_result = ::Gitlab::Auth::Result.new(nil, project, :geo, [:download_code, :push_code]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
       rescue ::Gitlab::Geo::InvalidDecryptionKeyError
-        render_bad_geo_auth("Invalid decryption key")
+        render_bad_geo_jwt("Invalid decryption key")
       rescue ::Gitlab::Geo::InvalidSignatureTimeError
-        render_bad_geo_auth("Invalid signature time ")
+        render_bad_geo_jwt("Invalid signature time ")
       end
 
       def jwt_scope_valid?
@@ -72,8 +73,16 @@ module EE
         end
       end
 
-      def render_bad_geo_auth(message)
-        render plain: "Geo JWT authentication failed: #{message}", status: :unauthorized
+      def render_bad_geo_jwt(message)
+        render_bad_geo_response("Geo JWT authentication failed: #{message}")
+      end
+
+      def render_bad_geo_response(message)
+        render plain: message, status: :unauthorized
+      end
+
+      def ip_allowed?
+        ::Gitlab::Geo.allowed_ip?(request.ip)
       end
     end
   end

ee/app/helpers/ee/application_settings_helper.rb

@@ -66,6 +66,7 @@ module EE
         :elasticsearch_namespace_ids,
         :elasticsearch_project_ids,
         :geo_status_timeout,
+        :geo_node_allowed_ips,
         :help_text,
         :pseudonymizer_enabled,
         :repository_size_limit,

ee/app/models/ee/application_setting.rb

@@ -76,6 +76,10 @@ module EE
                 presence: true,
                 if: -> (setting) { setting.external_auth_client_cert.present? }
 
+      validates :geo_node_allowed_ips, length: { maximum: 255 }, presence: true
+
+      validate :check_geo_node_allowed_ips
+
       validates_with X509CertificateCredentialsValidator,
                      certificate: :external_auth_client_cert,
                      pkey: :external_auth_client_key,
@@ -119,7 +123,8 @@ module EE
           snowplow_cookie_domain: nil,
           snowplow_enabled: false,
           snowplow_site_id: nil,
-          custom_project_templates_group_id: nil
+          custom_project_templates_group_id: nil,
+          geo_node_allowed_ips: '0.0.0.0/0, ::/0'
         )
       end
     end
@@ -290,5 +295,11 @@ module EE
     def email_additional_text_column_exists?
       ::Gitlab::Database.cached_column_exists?(:application_settings, :email_additional_text)
     end
+
+    def check_geo_node_allowed_ips
+      ::Gitlab::CIDR.new(geo_node_allowed_ips)
+    rescue ::Gitlab::CIDR::ValidationError => e
+      errors.add(:geo_node_allowed_ips, e.message)
+    end
   end
 end

ee/app/views/admin/application_settings/_geo.html.haml

@@ -13,15 +13,17 @@
         = form_errors(@application_setting)
 
         %fieldset
-          %p
-            These settings will only take effect if Geo is enabled and require a restart to take effect.
           .form-group
             = f.label :geo_status_timeout, 'Connection timeout', class: 'label-bold'
             = f.number_field :geo_status_timeout, class: 'form-control'
             .form-text.text-muted
-              The amount of seconds after which a request to get a secondary node
-              status will time out.
+              = _('The amount of seconds after which a request to get a secondary node status will time out.')
+          .form-group
+            = f.label :geo_node_allowed_ips, 'Allowed Geo IP', class: 'label-bold'
+            = f.text_field :geo_node_allowed_ips, class: 'form-control'
+            .form-text.text-muted
+              = _('List of IPs and CIDRs of allowed secondary nodes. Comma-separated, e.g. "1.1.1.1, 2.2.2.0/24"')
 
-        = f.submit 'Save changes', class: "btn btn-success"
+        = f.submit _('Save changes'), class: "btn btn-success"
 - else
   = render 'shared/empty_states/geo'

ee/changelogs/unreleased/4193-ip-whitelisting-for-geo-enabling-functionality-in-the-primary.yml

+---
+title: IP whitelisting for Geo-enabling functionality in the primary
+merge_request: 10383
+author:
+type: added

ee/db/migrate/20190322145954_add_allowed_geo_i_ps_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddAllowedGeoIPsToApplicationSettings < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :geo_node_allowed_ips, :string, default: '0.0.0.0/0, ::/0'
+  end
+end

ee/lib/api/geo.rb

@@ -26,6 +26,8 @@ module API
         requires :id, type: Integer, desc: 'The DB ID of the file'
       end
       get 'transfers/:type/:id' do
+        check_gitlab_geo_request_ip!
+
         service = ::Geo::FileUploadService.new(params, headers['Authorization'])
         response = service.execute
 
@@ -44,6 +46,7 @@ module API
       # Example request:
       #   POST /geo/status
       post 'status' do
+        check_gitlab_geo_request_ip!
         authenticate_by_gitlab_geo_node_token!
 
         db_status = GeoNode.find(params[:geo_node_id]).find_or_build_status

ee/lib/ee/api/helpers.rb

@@ -20,6 +20,10 @@ module EE
         render_api_error!(e.to_s, 401)
       end
 
+      def check_gitlab_geo_request_ip!
+        unauthorized! unless ::Gitlab::Geo.allowed_ip?(request.ip)
+      end
+
       override :current_user
       def current_user
         strong_memoize(:current_user) do

ee/lib/gitlab/cidr.rb

+# rubocop:disable Naming/FileName
+# frozen_string_literal: true
+require 'ipaddr'
+
+module Gitlab
+  class CIDR
+    ValidationError = Class.new(StandardError)
+
+    attr_reader :cidrs
+
+    def initialize(values)
+      @cidrs = parse_cidrs(values)
+    end
+
+    def match?(ip)
+      cidrs.find { |cidr| cidr.include?(ip) }.present?
+    end
+
+    private
+
+    def parse_cidrs(values)
+      values.to_s.split(',').map do |value|
+        ::IPAddr.new(value.strip)
+      end
+    rescue => e
+      raise ValidationError, e.message
+    end
+  end
+end

ee/lib/gitlab/geo.rb

@@ -132,5 +132,11 @@ module Gitlab
         true
       end
     end
+
+    def self.allowed_ip?(ip)
+      allowed_ips = ::Gitlab::CurrentSettings.current_application_settings.geo_node_allowed_ips
+
+      Gitlab::CIDR.new(allowed_ips).match?(ip)
+    end
   end
 end

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -34,7 +34,8 @@ describe Admin::ApplicationSettingsController do
           slack_app_id: 'slack_app_id',
           slack_app_secret: 'slack_app_secret',
           slack_app_verification_token: 'slack_app_verification_token',
-          allow_group_owners_to_manage_ldap: false
+          allow_group_owners_to_manage_ldap: false,
+          geo_node_allowed_ips: '0.0.0.0/0, ::/0'
       }
 
       put :update, params: { application_setting: settings }
@@ -48,7 +49,7 @@ describe Admin::ApplicationSettingsController do
     end
 
     shared_examples 'settings for licensed features' do
-      it 'does not update settings when licesed feature is not available' do
+      it 'does not update settings when licensed feature is not available' do
         stub_licensed_features(feature => false)
         attribute_names = settings.keys.map(&:to_s)
 

ee/spec/features/admin/admin_settings_spec.rb

@@ -15,10 +15,12 @@ describe 'Admin updates EE-only settings' do
         visit geo_admin_application_settings_path
         page.within('.as-geo') do
           fill_in 'Connection timeout', with: 15
+          fill_in 'Allowed Geo IP', with: '192.34.34.34'
           click_button 'Save changes'
         end
 
         expect(Gitlab::CurrentSettings.geo_status_timeout).to eq(15)
+        expect(Gitlab::CurrentSettings.geo_node_allowed_ips).to eq('192.34.34.34')
         expect(page).to have_content "Application settings saved successfully"
       end
     end

ee/spec/lib/gitlab/cidr_spec.rb

+require 'spec_helper'
+
+describe Gitlab::CIDR do
+  using RSpec::Parameterized::TableSyntax
+
+  context 'validation' do
+    it 'raises an exception when an octet is invalid' do
+      expect { described_class.new("192.1.1.257") }.to raise_error(described_class::ValidationError)
+    end
+
+    it 'raises an exception when a bitmask is invalid' do
+      expect { described_class.new("192.1.1.0/34") }.to raise_error(described_class::ValidationError)
+    end
+
+    it 'raises an exception when one IP from a set is invalid' do
+      expect { described_class.new("192.1.1.257, 192.1.1.1") }.to raise_error(described_class::ValidationError)
+    end
+  end
+
+  context 'matching' do
+    where(:values, :ip, :match) do
+      "192.1.1.1"                  | "192.1.1.1"     | true
+      "192.1.1.1, 192.1.2.1"       | "192.1.2.1"     | true
+      "192.1.1.0/24"               | "192.1.1.223"   | true
+      "192.1.0.0/16"               | "192.1.223.223" | true
+      "192.1.0.0/16, 192.1.2.0/24" | "192.1.2.223"   | true
+      "192.1.0.0/16"               | "192.2.1.1"     | false
+      "192.1.0.1"                  | "192.2.1.1"     | false
+    end
+
+    with_them do
+      it do
+        expect(described_class.new(values).match?(ip)).to eq(match)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Geo, :geo, :request_store do
+  using RSpec::Parameterized::TableSyntax
   include ::EE::GeoHelpers
 
   set(:primary_node)   { create(:geo_node, :primary) }
@@ -242,4 +243,24 @@ describe Gitlab::Geo, :geo, :request_store do
       end
     end
   end
+
+  context '.allowed_ip?' do
+    where(:allowed_ips, :ip, :allowed) do
+      "192.1.1.1"                  | "192.1.1.1"     | true
+      "192.1.1.1, 192.1.2.1"       | "192.1.2.1"     | true
+      "192.1.1.0/24"               | "192.1.1.223"   | true
+      "192.1.0.0/16"               | "192.1.223.223" | true
+      "192.1.0.0/16, 192.1.2.0/24" | "192.1.2.223"   | true
+      "192.1.0.0/16"               | "192.2.1.1"     | false
+      "192.1.0.1"                  | "192.2.1.1"     | false
+    end
+
+    with_them do
+      it do
+        stub_application_setting(geo_node_allowed_ips: allowed_ips)
+
+        expect(described_class.allowed_ip?(ip)).to eq(allowed)
+      end
+    end
+  end
 end

ee/spec/models/application_setting_spec.rb

 require 'spec_helper'
 
 describe ApplicationSetting do
+  using RSpec::Parameterized::TableSyntax
+
   subject(:setting) { described_class.create_from_defaults }
 
   describe 'validations' do
@@ -80,6 +82,27 @@ describe ApplicationSetting do
         end
       end
     end
+
+    context 'when validating allowed_ips' do
+      where(:allowed_ips, :is_valid) do
+        "192.1.1.1"                   | true
+        "192.1.1.0/24"                | true
+        "192.1.1.0/24, 192.1.20.23"   | true
+        "192.1.1.0/24, 192.23.0.0/16" | true
+        "192.1.1.0/34"                | false
+        "192.1.1.257"                 | false
+        "192.1.1.257, 192.1.1.1"      | false
+        "300.1.1.0/34"                | false
+      end
+
+      with_them do
+        it do
+          setting.update_column(:geo_node_allowed_ips, allowed_ips)
+
+          expect(setting.reload.valid?).to eq(is_valid)
+        end
+      end
+    end
   end
 
   describe '#should_check_namespace_plan?' do

ee/spec/requests/api/geo_spec.rb

@@ -34,6 +34,29 @@ describe API::Geo do
       stub_current_geo_node(secondary_node)
     end
 
+    describe 'allowed IPs' do
+      let(:note) { create(:note, :with_attachment) }
+      let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
+      let(:transfer) { Gitlab::Geo::FileTransfer.new(:attachment, upload) }
+      let(:req_header) { Gitlab::Geo::TransferRequest.new(transfer.request_data).headers }
+
+      it 'responds with 401 when IP is not allowed' do
+        stub_application_setting(geo_node_allowed_ips: '192.34.34.34')
+
+        get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header
+
+        expect(response).to have_gitlab_http_status(401)
+      end
+
+      it 'responds with 200 when IP is allowed' do
+        stub_application_setting(geo_node_allowed_ips: '127.0.0.1')
+
+        get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
     describe 'GET /geo/transfers/attachment/1' do
       let(:note) { create(:note, :with_attachment) }
       let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
@@ -272,6 +295,24 @@ describe API::Geo do
       expect(response).to have_gitlab_http_status(401)
     end
 
+    describe 'allowed IPs' do
+      it 'responds with 401 when IP is not allowed' do
+        stub_application_setting(geo_node_allowed_ips: '192.34.34.34')
+
+        request
+
+        expect(response).to have_gitlab_http_status(401)
+      end
+
+      it 'responds with 201 when IP is allowed' do
+        stub_application_setting(geo_node_allowed_ips: '127.0.0.1')
+
+        request
+
+        expect(response).to have_gitlab_http_status(201)
+      end
+    end
+
     context 'when requesting primary node with valid auth header' do
       before do
         stub_current_geo_node(primary_node)

ee/spec/requests/git_http_geo_spec.rb

@@ -394,6 +394,32 @@ describe "Git HTTP requests (Geo)", :geo do
         end
       end
     end
+
+    context 'IP allowed settings' do
+      subject do
+        make_request
+        response
+      end
+
+      def make_request
+        get "/#{repository_path}.git/info/refs", params: { service: 'git-upload-pack' }, headers: env
+      end
+
+      let(:repository_path) { project.full_path }
+
+      it 'returns unauthorized error' do
+        stub_application_setting(geo_node_allowed_ips: '192.34.34.34')
+
+        is_expected.to have_gitlab_http_status(:unauthorized)
+        expect(subject.parsed_body).to eq('Request from this IP is not allowed')
+      end
+
+      it 'returns success response' do
+        stub_application_setting(geo_node_allowed_ips: '127.0.0.1')
+
+        is_expected.to have_gitlab_http_status(:success)
+      end
+    end
   end
 
   def valid_geo_env

locale/gitlab.pot

@@ -6449,6 +6449,9 @@ msgstr ""
 msgid "List available repositories"
 msgstr ""
 
+msgid "List of IPs and CIDRs of allowed secondary nodes. Comma-separated, e.g. \"1.1.1.1, 2.2.2.0/24\""
+msgstr ""
+
 msgid "List view"
 msgstr ""
 
@@ -10527,6 +10530,9 @@ msgstr ""
 msgid "The X509 Certificate to use when mutual TLS is required to communicate with the external authorization service. If left blank, the server certificate is still validated when accessing over HTTPS."
 msgstr ""
 
+msgid "The amount of seconds after which a request to get a secondary node status will time out."
+msgstr ""
+
 msgid "The branch for this project has no active pipeline configuration."
 msgstr ""