Merge branch 'security-300273-safe-geo-gl-id' into 'master'

Geo-GL-ID should be passed in JWT token so it's protected properly

See merge request gitlab-org/security/gitlab!1209

ee/app/controllers/ee/repositories/git_http_controller.rb

@@ -35,11 +35,17 @@ module EE
       end
 
       def geo_push_user
-        @geo_push_user ||= ::Geo::PushUser.new_from_headers(request.headers)
+        return unless geo_gl_id
+
+        @geo_push_user ||= ::Geo::PushUser.new(geo_gl_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      end
+
+      def geo_gl_id
+        decoded_authorization&.dig(:gl_id)
       end
 
-      def geo_push_user_headers_provided?
-        ::Geo::PushUser.needed_headers_provided?(request.headers)
+      def geo_push_proxy_request?
+        geo_gl_id
       end
 
       def geo_request?
@@ -53,8 +59,8 @@ module EE
       override :access_actor
       def access_actor
         return super unless geo?
-        return :geo unless geo_push_user_headers_provided?
-        return geo_push_user.user if geo_push_user.user
+        return :geo unless geo_push_proxy_request?
+        return geo_push_user.user if geo_push_user&.user
 
         raise ::Gitlab::GitAccess::ForbiddenError, 'Geo push user is invalid.'
       end

ee/app/models/geo/push_user.rb

@@ -7,16 +7,6 @@ class Geo::PushUser
     @gl_id = gl_id
   end
 
-  def self.needed_headers_provided?(headers)
-    headers['Geo-GL-Id'].present?
-  end
-
-  def self.new_from_headers(headers)
-    return unless needed_headers_provided?(headers)
-
-    new(headers['Geo-GL-Id'])
-  end
-
   def user
     @user ||= identify_using_ssh_key(gl_id)
   end

ee/changelogs/unreleased/288799-confidential-issue.yml

+---
+title: 'Geo: Pass GL-ID in a JWT token when proxy-push from secondary'
+author:
+type: security

ee/lib/gitlab/geo/git_ssh_proxy.rb

@@ -134,8 +134,7 @@ module Gitlab
 
       def base_headers
         @base_headers ||= {
-          'Geo-GL-Id' => gl_id,
-          'Authorization' => Gitlab::Geo::BaseRequest.new(scope: auth_scope).authorization
+          'Authorization' => Gitlab::Geo::BaseRequest.new(scope: auth_scope, gl_id: gl_id).authorization
         }
       end
 

ee/spec/lib/gitlab/geo/git_ssh_proxy_spec.rb

@@ -14,6 +14,7 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
   let(:project) { create(:project, :repository) }
   let(:user) { project.creator }
   let(:key) { create(:key, user: user) }
+  let(:key_identifier) { "key-#{key.id}" }
   let(:base_request) { double(Gitlab::Geo::BaseRequest.new.authorization) }
 
   let(:info_refs_body_short) do
@@ -22,7 +23,6 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
 
   let(:base_headers) do
     {
-      'Geo-GL-Id' => "key-#{key.id}",
       'Authorization' => 'secret'
     }
   end
@@ -72,13 +72,13 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
 
         context 'authorization header is scoped' do
           it 'passes the scope when .info_refs_upload_pack is called' do
-            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path)
+            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier)
 
             subject.info_refs_upload_pack
           end
 
           it 'passes the scope when .receive_pack is called' do
-            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path)
+            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier)
 
             subject.receive_pack(info_refs_body_short)
           end
@@ -299,13 +299,13 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
 
         context 'authorization header is scoped' do
           it 'passes the scope when .info_refs_receive_pack is called' do
-            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path)
+            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier )
 
             subject.info_refs_receive_pack
           end
 
           it 'passes the scope when .receive_pack is called' do
-            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path)
+            expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier)
 
             subject.receive_pack(info_refs_body_short)
           end

ee/spec/models/geo/push_user_spec.rb

@@ -10,58 +10,6 @@ RSpec.describe Geo::PushUser do
 
   subject { described_class.new(gl_id) }
 
-  describe '.needed_headers_provided?' do
-    where(:headers) do
-      [
-        {},
-        { 'Geo-GL-Id' => nil },
-        { 'Geo-GL-Id' => '' }
-      ]
-    end
-
-    with_them do
-      it 'returns false' do
-        expect(described_class.needed_headers_provided?(headers)).to be(false)
-      end
-    end
-
-    context 'where gl_id is not nil' do
-      let(:headers) do
-        { 'Geo-GL-Id' => gl_id }
-      end
-
-      it 'returns true' do
-        expect(described_class.needed_headers_provided?(headers)).to be(true)
-      end
-    end
-  end
-
-  describe '.new_from_headers' do
-    where(:headers) do
-      [
-        {},
-        { 'Geo-GL-Id' => nil },
-        { 'Geo-GL-Id' => '' }
-      ]
-    end
-
-    with_them do
-      it 'returns false' do
-        expect(described_class.new_from_headers(headers)).to be_nil
-      end
-    end
-
-    context 'where gl_id is not nil' do
-      let(:headers) do
-        { 'Geo-GL-Id' => gl_id }
-      end
-
-      it 'returns an instance of Geo::PushUser' do
-        expect(described_class.new_from_headers(headers)).to be_a(described_class)
-      end
-    end
-  end
-
   describe '#user' do
     context 'with a junk gl_id' do
       let(:gl_id) { "test" }

ee/spec/requests/git_http_geo_spec.rb

@@ -17,8 +17,6 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
   let_it_be(:primary) { create(:geo_node, :primary, url: primary_url) }
   let_it_be(:secondary) { create(:geo_node, url: secondary_url) }
 
-  # Ensure the token always comes from the real time of the request
-  let(:auth_token) { Gitlab::Geo::BaseRequest.new(scope: project.full_path).authorization }
   let!(:user) { create(:user) }
   let!(:user_without_any_access) { create(:user) }
   let!(:user_without_push_access) { create(:user) }
@@ -131,6 +129,8 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
   context 'when current node is a secondary' do
     let(:current_node) { secondary }
 
+    let(:auth_token) { Gitlab::Geo::BaseRequest.new(scope: project.full_path).authorization }
+
     describe 'GET info_refs' do
       context 'git pull' do
         def make_request
@@ -453,6 +453,10 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
   end
 
   context 'when current node is the primary', :use_clean_rails_memory_store_caching do
+    let!(:geo_gl_id) { "key-#{key.id}" }
+
+    # Ensure the token always comes from the real time of the request
+    let(:auth_token) { Gitlab::Geo::BaseRequest.new(scope: project.full_path, gl_id: geo_gl_id).authorization }
     let(:current_node) { primary }
 
     describe 'POST git_receive_pack' do
@@ -503,27 +507,7 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
         let_it_be(:project) { project_with_repo }
         let(:endpoint_path) { "/#{project.full_path}.git/git-receive-pack" }
 
-        before do
-          env['Geo-GL-Id'] = geo_gl_id
-        end
-
-        context 'when gl_id is incorrectly provided via HTTP headers' do
-          where(:geo_gl_id) do
-            [
-              nil,
-              ''
-            ]
-          end
-
-          with_them do
-            it 'returns a 403' do
-              is_expected.to have_gitlab_http_status(:forbidden)
-              expect(response.body).to eql('You are not allowed to upload code for this project.')
-            end
-          end
-        end
-
-        context 'when gl_id is provided via HTTP headers' do
+        context 'when gl_id is provided in JWT token' do
           context 'but is invalid' do
             where(:geo_gl_id) do
               %w[