Commit fa2f136f authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-55468-check-validity-before-querying' into 'master'

Fix blind SSRF in Prometheus Integration

See merge request gitlab/gitlabhq!2907
parents 219075fc 50591efc
...@@ -71,7 +71,7 @@ class PrometheusService < MonitoringService ...@@ -71,7 +71,7 @@ class PrometheusService < MonitoringService
end end
def prometheus_client def prometheus_client
RestClient::Resource.new(api_url, max_redirects: 0) if api_url && manual_configuration? && active? RestClient::Resource.new(api_url, max_redirects: 0) if should_return_client?
end end
def prometheus_available? def prometheus_available?
...@@ -83,6 +83,10 @@ class PrometheusService < MonitoringService ...@@ -83,6 +83,10 @@ class PrometheusService < MonitoringService
private private
def should_return_client?
api_url && manual_configuration? && active? && valid?
end
def synchronize_service_state def synchronize_service_state
self.active = prometheus_available? || manual_configuration? self.active = prometheus_available? || manual_configuration?
......
---
title: Fix blind SSRF in Prometheus integration by checking URL before querying
merge_request:
author:
type: security
...@@ -33,18 +33,38 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do ...@@ -33,18 +33,38 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do
describe 'Validations' do describe 'Validations' do
context 'when manual_configuration is enabled' do context 'when manual_configuration is enabled' do
before do before do
subject.manual_configuration = true service.manual_configuration = true
end end
it { is_expected.to validate_presence_of(:api_url) } it 'validates presence of api_url' do
expect(service).to validate_presence_of(:api_url)
end
end end
context 'when manual configuration is disabled' do context 'when manual configuration is disabled' do
before do before do
subject.manual_configuration = false service.manual_configuration = false
end end
it { is_expected.not_to validate_presence_of(:api_url) } it 'does not validate presence of api_url' do
expect(service).not_to validate_presence_of(:api_url)
end
end
context 'when the api_url domain points to localhost or local network' do
let(:domain) { Addressable::URI.parse(service.api_url).hostname }
it 'cannot query' do
expect(service.can_query?).to be true
aggregate_failures do
['127.0.0.1', '192.168.2.3'].each do |url|
allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([Addrinfo.tcp(url, 80)])
expect(service.can_query?).to be false
end
end
end
end end
end end
...@@ -74,30 +94,35 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do ...@@ -74,30 +94,35 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do
end end
describe '#prometheus_client' do describe '#prometheus_client' do
let(:api_url) { 'http://some_url' }
before do
service.active = true
service.api_url = api_url
service.manual_configuration = manual_configuration
end
context 'manual configuration is enabled' do context 'manual configuration is enabled' do
let(:api_url) { 'http://some_url' } let(:manual_configuration) { true }
before do it 'returns rest client from api_url' do
subject.active = true expect(service.prometheus_client.url).to eq(api_url)
subject.manual_configuration = true
subject.api_url = api_url
end end
it 'returns rest client from api_url' do it 'calls valid?' do
expect(subject.prometheus_client.url).to eq(api_url) allow(service).to receive(:valid?).and_call_original
expect(service.prometheus_client).not_to be_nil
expect(service).to have_received(:valid?)
end end
end end
context 'manual configuration is disabled' do context 'manual configuration is disabled' do
let(:api_url) { 'http://some_url' } let(:manual_configuration) { false }
before do
subject.manual_configuration = false
subject.api_url = api_url
end
it 'no client provided' do it 'no client provided' do
expect(subject.prometheus_client).to be_nil expect(service.prometheus_client).to be_nil
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment