Merge branch '33949-remove-healthcheck-access-token' into 'master'9-4-stable-prepare-RC1

Remove the need to use health check token by adding ability to whitelist hosts

Closes #33949

See merge request !12612

13 files changed, 228 insertions, 90 deletions

diff --git a/app/controllers/concerns/requires_health_token.rb b/app/controllers/concerns/requires_health_token.rb
deleted file mode 100644
index 34ab1a97649..00000000000
--- a/app/controllers/concerns/requires_health_token.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-module RequiresHealthToken
-  extend ActiveSupport::Concern
-  included do
-    before_action :validate_health_check_access!
-  end
-
-  private
-
-  def validate_health_check_access!
-    render_404 unless token_valid?
-  end
-
-  def token_valid?
-    token = params[:token].presence || request.headers['TOKEN']
-    token.present? &&
-      ActiveSupport::SecurityUtils.variable_size_secure_compare(
-        token,
-        current_application_settings.health_check_access_token
-      )
-  end
-
-  def render_404
-    render file: Rails.root.join('public', '404'), layout: false, status: '404'
-  end
-end
diff --git a/app/controllers/concerns/requires_whitelisted_monitoring_client.rb b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb
new file mode 100644
index 00000000000..ad2f4bbc486
--- /dev/null
+++ b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb
@@ -0,0 +1,33 @@
+module RequiresWhitelistedMonitoringClient
+  extend ActiveSupport::Concern
+  included do
+    before_action :validate_ip_whitelisted_or_valid_token!
+  end
+
+  private
+
+  def validate_ip_whitelisted_or_valid_token!
+    render_404 unless client_ip_whitelisted? || valid_token?
+  end
+
+  def client_ip_whitelisted?
+    ip_whitelist.any? { |e| e.include?(Gitlab::RequestContext.client_ip) }
+  end
+
+  def ip_whitelist
+    @ip_whitelist ||= Settings.monitoring.ip_whitelist.map(&IPAddr.method(:new))
+  end
+
+  def valid_token?
+    token = params[:token].presence || request.headers['TOKEN']
+    token.present? &&
+      ActiveSupport::SecurityUtils.variable_size_secure_compare(
+        token,
+        current_application_settings.health_check_access_token
+      )
+  end
+
+  def render_404
+    render file: Rails.root.join('public', '404'), layout: false, status: '404'
+  end
+end
diff --git a/app/controllers/health_check_controller.rb b/app/controllers/health_check_controller.rb
index 5d3109b7187..c3d18991fd4 100644
--- a/app/controllers/health_check_controller.rb
+++ b/app/controllers/health_check_controller.rb
@@ -1,3 +1,3 @@
 class HealthCheckController < HealthCheck::HealthCheckController
-  include RequiresHealthToken
+  include RequiresWhitelistedMonitoringClient
 end
diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb
index abc832e6ddc..b140092eef2 100644
--- a/app/controllers/health_controller.rb
+++ b/app/controllers/health_controller.rb
@@ -1,6 +1,6 @@
 class HealthController < ActionController::Base
   protect_from_forgery with: :exception
-  include RequiresHealthToken
+  include RequiresWhitelistedMonitoringClient
 
   CHECKS = [
     Gitlab::HealthChecks::DbCheck,
diff --git a/app/controllers/metrics_controller.rb b/app/controllers/metrics_controller.rb
index 0e9a19c0b6f..37587a52eaf 100644
--- a/app/controllers/metrics_controller.rb
+++ b/app/controllers/metrics_controller.rb
@@ -1,12 +1,12 @@
 class MetricsController < ActionController::Base
-  include RequiresHealthToken
+  include RequiresWhitelistedMonitoringClient
 
   protect_from_forgery with: :exception
 
   before_action :validate_prometheus_metrics
 
   def index
-    render text: metrics_service.metrics_text, content_type: 'text/plain; verssion=0.0.4'
+    render text: metrics_service.metrics_text, content_type: 'text/plain; version=0.0.4'
   end
 
   private
diff --git a/changelogs/unreleased/33949-deprecate-healthcheck-access-token.yml b/changelogs/unreleased/33949-deprecate-healthcheck-access-token.yml
new file mode 100644
index 00000000000..a08795e1a26
--- /dev/null
+++ b/changelogs/unreleased/33949-deprecate-healthcheck-access-token.yml
@@ -0,0 +1,4 @@
+---
+title: Deprecate Healthcheck Access Token in favor of IP whitelist
+merge_request:
+author:
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 221e3d6e03b..d0ab2dab0af 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -539,10 +539,15 @@ production: &base
     #   enabled: true
     #   host: localhost
     #   port: 3808
-  prometheus:
+
+  ## Monitoring
+  # Built in monitoring settings
+  monitoring:
     # Time between sampling of unicorn socket metrics, in seconds
     # unicorn_sampler_interval: 10
-
+    # IP whitelist to access monitoring endpoints
+    ip_whitelist:
+      - 127.0.0.0/8
 
   #
   # 5. Extra customization
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index fa33e602e93..eb4a1e390a9 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -494,10 +494,11 @@ Settings.webpack.dev_server['host']    ||= 'localhost'
 Settings.webpack.dev_server['port']    ||= 3808
 
 #
-# Prometheus metrics settings
+# Monitoring settings
 #
-Settings['prometheus'] ||= Settingslogic.new({})
-Settings.prometheus['unicorn_sampler_interval'] ||= 10
+Settings['monitoring'] ||= Settingslogic.new({})
+Settings.monitoring['ip_whitelist'] ||= ['127.0.0.1/8']
+Settings.monitoring['unicorn_sampler_interval'] ||= 10
 
 #
 # Testing settings
diff --git a/config/initializers/8_metrics.rb b/config/initializers/8_metrics.rb
index d56fd7a6cfa..5e34aac6527 100644
--- a/config/initializers/8_metrics.rb
+++ b/config/initializers/8_metrics.rb
@@ -119,7 +119,7 @@ def instrument_classes(instrumentation)
 end
 # rubocop:enable Metrics/AbcSize
 
-Gitlab::Metrics::UnicornSampler.initialize_instance(Settings.prometheus.unicorn_sampler_interval).start
+Gitlab::Metrics::UnicornSampler.initialize_instance(Settings.monitoring.unicorn_sampler_interval).start
 
 Gitlab::Application.configure do |config|
   # 0 should be Sentry to catch errors in this middleware
diff --git a/doc/user/admin_area/monitoring/health_check.md b/doc/user/admin_area/monitoring/health_check.md
index a954840b8a6..69a9dfc3500 100644
--- a/doc/user/admin_area/monitoring/health_check.md
+++ b/doc/user/admin_area/monitoring/health_check.md
@@ -5,6 +5,8 @@
   - The `health_check` endpoint was [introduced][ce-3888] in GitLab 8.8 and will
     be deprecated in GitLab 9.1. Read more in the [old behavior](#old-behavior)
     section.
+  - [Access token](#access-token) has been deprecated in GitLab 9.4
+    in favor of [IP Whitelist](#ip-whitelist)
 
 GitLab provides liveness and readiness probes to indicate service health and
 reachability to required services. These probes report on the status of the
@@ -12,7 +14,19 @@ database connection, Redis connection, and access to the filesystem. These
 endpoints [can be provided to schedulers like Kubernetes][kubernetes] to hold
 traffic until the system is ready or restart the container as needed.
 
-## Access Token
+## IP Whitelist
+
+To access monitoring resources the client IP needs to be included in the whitelist.
+To add or remove hosts or IP ranges from the list you can edit `gitlab.rb` or `gitlab.yml`.
+
+Example whitelist configuration:
+```yaml
+monitoring:
+  ip_whitelist:
+    - 127.0.0.0/8 # by default only local IPs are allowed to access monitoring resources
+```
+
+## Access Token (Deprecated)
 
 An access token needs to be provided while accessing the probe endpoints. The current
 accepted token can be found under the **Admin area ➔ Monitoring ➔ Health check**
@@ -47,10 +61,10 @@ which will then provide a report of system health in JSON format:
 
 ## Using the Endpoint
 
-Once you have the access token, the probes can be accessed:
+With default whitelist settings, the probes can be accessed from localhost:
 
-- `https://gitlab.example.com/-/readiness?token=ACCESS_TOKEN`
-- `https://gitlab.example.com/-/liveness?token=ACCESS_TOKEN`
+- `http://localhost/-/readiness`
+- `http://localhost/-/liveness`
 
 ## Status
 
@@ -71,8 +85,8 @@ the database connection, the state of the database migrations, and the ability t
 and access the cache. This endpoint can be provided to uptime monitoring services like
 [Pingdom][pingdom], [Nagios][nagios-health], and [NewRelic][newrelic-health].
 
-Once you have the [access token](#access-token), health information can be
-retrieved as plain text, JSON, or XML using the `health_check` endpoint:
+Once you have the [access token](#access-token) or your client IP is [whitelisted](#ip-whitelist),
+health information can be retrieved as plain text, JSON, or XML using the `health_check` endpoint:
 
 - `https://gitlab.example.com/health_check?token=ACCESS_TOKEN`
 - `https://gitlab.example.com/health_check.json?token=ACCESS_TOKEN`
diff --git a/spec/controllers/health_check_controller_spec.rb b/spec/controllers/health_check_controller_spec.rb
index 58c16cc57e6..03da6287774 100644
--- a/spec/controllers/health_check_controller_spec.rb
+++ b/spec/controllers/health_check_controller_spec.rb
@@ -3,52 +3,79 @@ require 'spec_helper'
 describe HealthCheckController do
   include StubENV
 
-  let(:token) { current_application_settings.health_check_access_token }
   let(:json_response) { JSON.parse(response.body) }
   let(:xml_response) { Hash.from_xml(response.body)['hash'] }
+  let(:token) { current_application_settings.health_check_access_token }
+  let(:whitelisted_ip) { '127.0.0.1' }
+  let(:not_whitelisted_ip) { '127.0.0.2' }
 
   before do
+    allow(Settings.monitoring).to receive(:ip_whitelist).and_return([whitelisted_ip])
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
   end
 
   describe 'GET #index' do
-    context 'when services are up but NO access token' do
+    context 'when services are up but accessed from outside whitelisted ips' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip)
+      end
+
       it 'returns a not found page' do
         get :index
+
         expect(response).to be_not_found
       end
+
+      context 'when services are accessed with token' do
+        it 'supports passing the token in the header' do
+          request.headers['TOKEN'] = token
+
+          get :index
+
+          expect(response).to be_success
+          expect(response.content_type).to eq 'text/plain'
+        end
+
+        it 'supports passing the token in query params' do
+          get :index, token: token
+
+          expect(response).to be_success
+          expect(response.content_type).to eq 'text/plain'
+        end
+      end
     end
 
-    context 'when services are up and an access token is provided' do
-      it 'supports passing the token in the header' do
-        request.headers['TOKEN'] = token
-        get :index
-        expect(response).to be_success
-        expect(response.content_type).to eq 'text/plain'
+    context 'when services are up and accessed from whitelisted ips' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
       end
 
-      it 'supports successful plaintest response' do
-        get :index, token: token
+      it 'supports successful plaintext response' do
+        get :index
+
         expect(response).to be_success
         expect(response.content_type).to eq 'text/plain'
       end
 
       it 'supports successful json response' do
-        get :index, token: token, format: :json
+        get :index, format: :json
+
         expect(response).to be_success
         expect(response.content_type).to eq 'application/json'
         expect(json_response['healthy']).to be true
       end
 
       it 'supports successful xml response' do
-        get :index, token: token, format: :xml
+        get :index, format: :xml
+
         expect(response).to be_success
         expect(response.content_type).to eq 'application/xml'
         expect(xml_response['healthy']).to be true
       end
 
       it 'supports successful responses for specific checks' do
-        get :index, token: token, checks: 'email', format: :json
+        get :index, checks: 'email', format: :json
+
         expect(response).to be_success
         expect(response.content_type).to eq 'application/json'
         expect(json_response['healthy']).to be true
@@ -58,33 +85,29 @@ describe HealthCheckController do
     context 'when a service is down but NO access token' do
       it 'returns a not found page' do
         get :index
+
         expect(response).to be_not_found
       end
     end
 
-    context 'when a service is down and an access token is provided' do
+    context 'when a service is down and an endpoint is accessed from whitelisted ip' do
       before do
         allow(HealthCheck::Utils).to receive(:process_checks).with(['standard']).and_return('The server is on fire')
         allow(HealthCheck::Utils).to receive(:process_checks).with(['email']).and_return('Email is on fire')
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
       end
 
-      it 'supports passing the token in the header' do
-        request.headers['TOKEN'] = token
+      it 'supports failure plaintext response' do
         get :index
-        expect(response).to have_http_status(500)
-        expect(response.content_type).to eq 'text/plain'
-        expect(response.body).to include('The server is on fire')
-      end
 
-      it 'supports failure plaintest response' do
-        get :index, token: token
         expect(response).to have_http_status(500)
         expect(response.content_type).to eq 'text/plain'
         expect(response.body).to include('The server is on fire')
       end
 
       it 'supports failure json response' do
-        get :index, token: token, format: :json
+        get :index, format: :json
+
         expect(response).to have_http_status(500)
         expect(response.content_type).to eq 'application/json'
         expect(json_response['healthy']).to be false
@@ -92,7 +115,8 @@ describe HealthCheckController do
       end
 
       it 'supports failure xml response' do
-        get :index, token: token, format: :xml
+        get :index, format: :xml
+
         expect(response).to have_http_status(500)
         expect(response.content_type).to eq 'application/xml'
         expect(xml_response['healthy']).to be false
@@ -100,7 +124,8 @@ describe HealthCheckController do
       end
 
       it 'supports failure responses for specific checks' do
-        get :index, token: token, checks: 'email', format: :json
+        get :index, checks: 'email', format: :json
+
         expect(response).to have_http_status(500)
         expect(response.content_type).to eq 'application/json'
         expect(json_response['healthy']).to be false
diff --git a/spec/controllers/health_controller_spec.rb b/spec/controllers/health_controller_spec.rb
index e7c19b47a6a..94d2b420a4e 100644
--- a/spec/controllers/health_controller_spec.rb
+++ b/spec/controllers/health_controller_spec.rb
@@ -3,21 +3,25 @@ require 'spec_helper'
 describe HealthController do
   include StubENV
 
-  let(:token) { current_application_settings.health_check_access_token }
   let(:json_response) { JSON.parse(response.body) }
+  let(:token) { current_application_settings.health_check_access_token }
+  let(:whitelisted_ip) { '127.0.0.1' }
+  let(:not_whitelisted_ip) { '127.0.0.2' }
 
   before do
+    allow(Settings.monitoring).to receive(:ip_whitelist).and_return([whitelisted_ip])
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
   end
 
   describe '#readiness' do
-    context 'authorization token provided' do
-      before do
-        request.headers['TOKEN'] = token
-      end
+    shared_context 'endpoint responding with readiness data' do
+      let(:request_params) { {} }
+
+      subject { get :readiness, request_params }
+
+      it 'responds with readiness checks data' do
+        subject
 
-      it 'returns proper response' do
-        get :readiness
         expect(json_response['db_check']['status']).to eq('ok')
         expect(json_response['redis_check']['status']).to eq('ok')
         expect(json_response['fs_shards_check']['status']).to eq('ok')
@@ -25,33 +29,90 @@ describe HealthController do
       end
     end
 
-    context 'without authorization token' do
-      it 'returns proper response' do
+    context 'accessed from whitelisted ip' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint responding with readiness data'
+    end
+
+    context 'accessed from not whitelisted ip' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip)
+      end
+
+      it 'responds with resource not found' do
         get :readiness
+
         expect(response.status).to eq(404)
       end
+
+      context 'accessed with valid token' do
+        context 'token passed in request header' do
+          before do
+            request.headers['TOKEN'] = token
+          end
+
+          it_behaves_like 'endpoint responding with readiness data'
+        end
+      end
+
+      context 'token passed as URL param' do
+        it_behaves_like 'endpoint responding with readiness data' do
+          let(:request_params) { { token: token } }
+        end
+      end
     end
   end
 
   describe '#liveness' do
-    context 'authorization token provided' do
-      before do
-        request.headers['TOKEN'] = token
-      end
+    shared_context 'endpoint responding with liveness data' do
+      subject { get :liveness }
+
+      it 'responds with liveness checks data' do
+        subject
 
-      it 'returns proper response' do
-        get :liveness
         expect(json_response['db_check']['status']).to eq('ok')
         expect(json_response['redis_check']['status']).to eq('ok')
         expect(json_response['fs_shards_check']['status']).to eq('ok')
       end
     end
 
-    context 'without authorization token' do
-      it 'returns proper response' do
+    context 'accessed from whitelisted ip' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint responding with liveness data'
+    end
+
+    context 'accessed from not whitelisted ip' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip)
+      end
+
+      it 'responds with resource not found' do
         get :liveness
+
         expect(response.status).to eq(404)
       end
+
+      context 'accessed with valid token' do
+        context 'token passed in request header' do
+          before do
+            request.headers['TOKEN'] = token
+          end
+
+          it_behaves_like 'endpoint responding with liveness data'
+        end
+
+        context 'token passed as URL param' do
+          it_behaves_like 'endpoint responding with liveness data' do
+            subject { get :liveness, token: token }
+          end
+        end
+      end
     end
   end
 end
diff --git a/spec/controllers/metrics_controller_spec.rb b/spec/controllers/metrics_controller_spec.rb
index 044c9f179ed..692bc9a87b4 100644
--- a/spec/controllers/metrics_controller_spec.rb
+++ b/spec/controllers/metrics_controller_spec.rb
@@ -3,22 +3,22 @@ require 'spec_helper'
 describe MetricsController do
   include StubENV
 
-  let(:token) { current_application_settings.health_check_access_token }
   let(:json_response) { JSON.parse(response.body) }
   let(:metrics_multiproc_dir) { Dir.mktmpdir }
+  let(:whitelisted_ip) { '127.0.0.1' }
+  let(:whitelisted_ip_range) { '10.0.0.0/24' }
+  let(:ip_in_whitelisted_range) { '10.0.0.1' }
+  let(:not_whitelisted_ip) { '10.0.1.1' }
 
   before do
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
     stub_env('prometheus_multiproc_dir', metrics_multiproc_dir)
     allow(Gitlab::Metrics).to receive(:prometheus_metrics_enabled?).and_return(true)
+    allow(Settings.monitoring).to receive(:ip_whitelist).and_return([whitelisted_ip, whitelisted_ip_range])
   end
 
   describe '#index' do
-    context 'authorization token provided' do
-      before do
-        request.headers['TOKEN'] = token
-      end
-
+    shared_examples_for 'endpoint providing metrics' do
       it 'returns DB ping metrics' do
         get :index
 
@@ -59,7 +59,27 @@ describe MetricsController do
       end
     end
 
-    context 'without authorization token' do
+    context 'accessed from whitelisted ip' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
+      end
+
+      it_behaves_like 'endpoint providing metrics'
+    end
+
+    context 'accessed from ip in whitelisted range' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(ip_in_whitelisted_range)
+      end
+
+      it_behaves_like 'endpoint providing metrics'
+    end
+
+    context 'accessed from not whitelisted ip' do
+      before do
+        allow(Gitlab::RequestContext).to receive(:client_ip).and_return(not_whitelisted_ip)
+      end
+
       it 'returns proper response' do
         get :index