diff options
author | Pawel Chojnacki <pawel@chojnacki.ws> | 2017-07-03 18:09:34 +0300 |
---|---|---|
committer | Pawel Chojnacki <pawel@chojnacki.ws> | 2017-07-05 01:46:11 +0300 |
commit | 18521584bd6cfc8de9511722696e87aef59795c5 (patch) | |
tree | fa5b83fca15ff3d6f7a70fd9b87bc31ad575a08a | |
parent | 5af1fcd6f329858d757bab0d67cb50af6c820160 (diff) |
Remove the need to use health check token
in favor of whitelist that will be used to
control the access to monitoring resources
-rw-r--r-- | app/controllers/concerns/requires_health_token.rb | 25 | ||||
-rw-r--r-- | app/controllers/concerns/requires_whitelisted_monitoring_client.rb | 20 | ||||
-rw-r--r-- | app/controllers/health_check_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/health_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/metrics_controller.rb | 4 | ||||
-rw-r--r-- | config/gitlab.yml.example | 6 | ||||
-rw-r--r-- | config/initializers/1_settings.rb | 7 | ||||
-rw-r--r-- | spec/controllers/health_check_controller_spec.rb | 48 | ||||
-rw-r--r-- | spec/controllers/health_controller_spec.rb | 24 | ||||
-rw-r--r-- | spec/controllers/metrics_controller_spec.rb | 14 |
10 files changed, 87 insertions, 65 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..92ed559ba8a --- /dev/null +++ b/app/controllers/concerns/requires_whitelisted_monitoring_client.rb @@ -0,0 +1,20 @@ +module RequiresWhitelistedMonitoringClient + extend ActiveSupport::Concern + included do + before_action :validate_ip_whitelisted! + end + + private + + def validate_ip_whitelisted! + render_404 unless client_ip_whitelisted? + end + + def client_ip_whitelisted? + Settings.monitoring.ip_whitelist.any? {|e| e.include?(Gitlab::RequestContext.client_ip) } + 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/config/gitlab.yml.example b/config/gitlab.yml.example index 4b81fd90f59..a49929a05b2 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -548,6 +548,12 @@ production: &base # unicorn_sampler_interval: 10 + ## Monitoring + # Built in monitoring settings + monitoring: + # IP whitelist to access monitoring endpoints + access_whitelist: 127.0.0.0/8 + # # 5. Extra customization # ========================== diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index cb11d2c34f4..0c0dcd8413e 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -495,6 +495,13 @@ Settings.webpack.dev_server['host'] ||= 'localhost' Settings.webpack.dev_server['port'] ||= 3808 # +# Monitoring settings +# +Settings['monitoring'] ||= Settingslogic.new({}) +Settings.monitoring['ip_whitelist'] ||= %w{127.0.0.1/8} +Settings.monitoring.ip_whitelist.map!(&IPAddr.method(:new)) + +# # Prometheus metrics settings # Settings['prometheus'] ||= Settingslogic.new({}) diff --git a/spec/controllers/health_check_controller_spec.rb b/spec/controllers/health_check_controller_spec.rb index 58c16cc57e6..15b3cacf623 100644 --- a/spec/controllers/health_check_controller_spec.rb +++ b/spec/controllers/health_check_controller_spec.rb @@ -3,52 +3,57 @@ 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(: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([IPAddr.new(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 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 + let(:ip) { '127.0.0.1' } + + before do + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) end it 'supports successful plaintest response' do - get :index, token: token + 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 @@ -62,29 +67,22 @@ describe HealthCheckController do 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') - end - - it 'supports passing the token in the header' do - request.headers['TOKEN'] = token - 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') + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) end it 'supports failure plaintest response' do - get :index, token: token + 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 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 +90,7 @@ 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 +98,7 @@ 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..3e4370652d0 100644 --- a/spec/controllers/health_controller_spec.rb +++ b/spec/controllers/health_controller_spec.rb @@ -3,17 +3,19 @@ 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(: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([IPAddr.new(whitelisted_ip)]) stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') end describe '#readiness' do - context 'authorization token provided' do + context 'accessed from whitelisted ip' do before do - request.headers['TOKEN'] = token + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) end it 'returns proper response' do @@ -25,7 +27,11 @@ describe HealthController do end end - context 'without authorization token' do + 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 :readiness expect(response.status).to eq(404) @@ -34,9 +40,9 @@ describe HealthController do end describe '#liveness' do - context 'authorization token provided' do + context 'accessed from whitelisted ip' do before do - request.headers['TOKEN'] = token + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) end it 'returns proper response' do @@ -47,7 +53,11 @@ describe HealthController do end end - context 'without authorization token' do + 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 :liveness expect(response.status).to eq(404) diff --git a/spec/controllers/metrics_controller_spec.rb b/spec/controllers/metrics_controller_spec.rb index 044c9f179ed..5bcdc6bd872 100644 --- a/spec/controllers/metrics_controller_spec.rb +++ b/spec/controllers/metrics_controller_spec.rb @@ -3,20 +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(:not_whitelisted_ip) { '127.0.0.2' } 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([IPAddr.new(whitelisted_ip)]) end describe '#index' do - context 'authorization token provided' do + context 'accessed from whitelisted ip' do before do - request.headers['TOKEN'] = token + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip) end it 'returns DB ping metrics' do @@ -59,7 +61,11 @@ describe MetricsController do end end - context 'without authorization token' do + 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 |