diff options
| -rw-r--r-- | app/controllers/concerns/requires_health_token.rb | 25 | ||||
| -rw-r--r-- | app/controllers/concerns/requires_whitelisted_monitoring_client.rb | 33 | ||||
| -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-- | changelogs/unreleased/33949-deprecate-healthcheck-access-token.yml | 4 | ||||
| -rw-r--r-- | config/gitlab.yml.example | 9 | ||||
| -rw-r--r-- | config/initializers/1_settings.rb | 7 | ||||
| -rw-r--r-- | config/initializers/8_metrics.rb | 2 | ||||
| -rw-r--r-- | doc/user/admin_area/monitoring/health_check.md | 26 | ||||
| -rw-r--r-- | spec/controllers/health_check_controller_spec.rb | 75 | ||||
| -rw-r--r-- | spec/controllers/health_controller_spec.rb | 95 | ||||
| -rw-r--r-- | spec/controllers/metrics_controller_spec.rb | 34 |
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 3dbacbbc897..98c2aaa3526 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 241c36620d1..cc389e554ad 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['cache_check']['status']).to eq('ok') expect(json_response['queues_check']['status']).to eq('ok') @@ -27,22 +31,50 @@ 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['cache_check']['status']).to eq('ok') expect(json_response['queues_check']['status']).to eq('ok') @@ -51,11 +83,40 @@ 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 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 c46dd92b454..86847c07c09 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 @@ -83,7 +83,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 |