Check validity of prometheus_service before query

Check validity before querying so that if the dns entry for the api_url
has been changed to something invalid after the model was saved and
checked for validity, it will not query. This is to solve a toctou
(time of check to time of use) issue.

3 files changed, 53 insertions, 19 deletions

diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb
index 60cb2d380d5..c68a9d923c8 100644
--- a/app/models/project_services/prometheus_service.rb
+++ b/app/models/project_services/prometheus_service.rb
@@ -71,7 +71,7 @@ class PrometheusService < MonitoringService
   end
 
   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
 
   def prometheus_available?
@@ -83,6 +83,10 @@ class PrometheusService < MonitoringService
 
   private
 
+  def should_return_client?
+    api_url && manual_configuration? && active? && valid?
+  end
+
   def synchronize_service_state
     self.active = prometheus_available? || manual_configuration?
 
diff --git a/changelogs/unreleased/security-55468-check-validity-before-querying.yml b/changelogs/unreleased/security-55468-check-validity-before-querying.yml
new file mode 100644
index 00000000000..8bb11a97f52
--- /dev/null
+++ b/changelogs/unreleased/security-55468-check-validity-before-querying.yml
@@ -0,0 +1,5 @@
+---
+title: Fix blind SSRF in Prometheus integration by checking URL before querying
+merge_request:
+author:
+type: security
diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb
index b6cf4c72450..e9c7c94ad70 100644
--- a/spec/models/project_services/prometheus_service_spec.rb
+++ b/spec/models/project_services/prometheus_service_spec.rb
@@ -33,18 +33,38 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do
   describe 'Validations' do
     context 'when manual_configuration is enabled' do
       before do
-        subject.manual_configuration = true
+        service.manual_configuration = true
       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
 
     context 'when manual configuration is disabled' do
       before do
-        subject.manual_configuration = false
+        service.manual_configuration = false
       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
 
@@ -74,30 +94,35 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do
   end
 
   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
-      let(:api_url) { 'http://some_url' }
+      let(:manual_configuration) { true }
 
-      before do
-        subject.active = true
-        subject.manual_configuration = true
-        subject.api_url = api_url
+      it 'returns rest client from api_url' do
+        expect(service.prometheus_client.url).to eq(api_url)
       end
 
-      it 'returns rest client from api_url' do
-        expect(subject.prometheus_client.url).to eq(api_url)
+      it 'calls valid?' do
+        allow(service).to receive(:valid?).and_call_original
+
+        expect(service.prometheus_client).not_to be_nil
+
+        expect(service).to have_received(:valid?)
       end
     end
 
     context 'manual configuration is disabled' do
-      let(:api_url) { 'http://some_url' }
-
-      before do
-        subject.manual_configuration = false
-        subject.api_url = api_url
-      end
+      let(:manual_configuration) { false }
 
       it 'no client provided' do
-        expect(subject.prometheus_client).to be_nil
+        expect(service.prometheus_client).to be_nil
       end
     end
   end