Merge branch 'ash.mckenzie/geo-git-push-ssh-proxy-primary' into 'master'

Geo: backend support to allow Secondary node to push to a Primary as push proxying

See merge request gitlab-org/gitlab-ee!6455

ee/app/controllers/ee/projects/git_http_controller.rb

@@ -6,11 +6,24 @@ module EE
       override :render_ok
       def render_ok
         set_workhorse_internal_api_content_type
+
         render json: ::Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name, show_all_refs: geo_request?)
       end
 
       private
 
+      def user
+        super || geo_push_user&.user
+      end
+
+      def geo_push_user
+        @geo_push_user ||= ::Geo::PushUser.new_from_headers(request.headers)
+      end
+
+      def geo_push_user_headers_provided?
+        ::Geo::PushUser.needed_headers_provided?(request.headers)
+      end
+
       def geo_request?
         ::Gitlab::Geo::JwtRequestDecoder.geo_auth_attempt?(request.headers['Authorization'])
       end
@@ -21,9 +34,11 @@ module EE
 
       override :access_actor
       def access_actor
-        return :geo if geo?
+        return super unless geo?
+        return :geo unless geo_push_user_headers_provided?
+        return geo_push_user.user if geo_push_user.user
 
-        super
+        raise ::Gitlab::GitAccess::UnauthorizedError, 'Geo push user is invalid.'
       end
 
       override :authenticate_user
@@ -32,7 +47,7 @@ module EE
 
         payload = ::Gitlab::Geo::JwtRequestDecoder.new(request.headers['Authorization']).decode
         if payload
-          @authentication_result = ::Gitlab::Auth::Result.new(nil, project, :geo, [:download_code]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          @authentication_result = ::Gitlab::Auth::Result.new(nil, project, :geo, [:download_code, :push_code]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
           return # grant access
         end
 

ee/app/models/geo/push_user.rb

+# frozen_string_literal: true
+
+class Geo::PushUser
+  include ::Gitlab::Identifier
+
+  def initialize(gl_id)
+    @gl_id = gl_id
+  end
+
+  def self.needed_headers_provided?(headers)
+    headers['Geo-GL-Id'].present?
+  end
+
+  def self.new_from_headers(headers)
+    return nil unless needed_headers_provided?(headers)
+
+    new(headers['Geo-GL-Id'])
+  end
+
+  def user
+    @user ||= identify_using_ssh_key(gl_id)
+  end
+
+  private
+
+  attr_reader :gl_id
+end

ee/changelogs/unreleased/ash-mckenzie-geo-git-push-ssh-proxy-primary.yml

+---
+title: Allow push_code when auth'd via Geo JWT
+merge_request: 6455
+author:
+type: changed

ee/spec/lib/gitlab/git_access_spec.rb

@@ -248,12 +248,23 @@ describe Gitlab::GitAccess do
     end
   end
 
+  describe 'Geo system permissions' do
+    let(:actor) { :geo }
+
+    it { expect { pull_changes }.not_to raise_error }
+    it { expect { push_changes }.to raise_unauthorized(Gitlab::GitAccess::ERROR_MESSAGES[:push_code]) }
+  end
+
   private
 
   def push_changes(changes = '_any')
     access.check('git-receive-pack', changes)
   end
 
+  def pull_changes(changes = '_any')
+    access.check('git-upload-pack', changes)
+  end
+
   def raise_unauthorized(message)
     raise_error(Gitlab::GitAccess::UnauthorizedError, message)
   end

ee/spec/models/geo/push_user_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Geo::PushUser do
+  let!(:user) { create(:user) }
+  let!(:key) { create(:key, user: user) }
+
+  let(:gl_id) { "key-#{key.id}" }
+
+  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" }
+
+      it 'returns nil' do
+        expect(subject.user).to be_nil
+      end
+    end
+
+    context 'with an unsupported gl_id type' do
+      let(:gl_id) { "user-#{user.id}" }
+
+      it 'returns nil' do
+        expect(subject.user).to be_nil
+      end
+    end
+
+    context 'when the User associated to gl_id matches the User associated to gl_username' do
+      it 'returns a User' do
+        expect(subject.user).to be_a(User)
+      end
+    end
+  end
+end

ee/spec/requests/git_http_geo_spec.rb

@@ -13,15 +13,24 @@ describe "Git HTTP requests (Geo)" do
 
   # Ensure the token always comes from the real time of the request
   let!(:auth_token) { Gitlab::Geo::BaseRequest.new.authorization }
+  let!(:user) { create(:user) }
+  let!(:user_without_any_access) { create(:user) }
+  let!(:user_without_push_access) { create(:user) }
+  let!(:key) { create(:key, user: user) }
+  let!(:key_for_user_without_any_access) { create(:key, user: user_without_any_access) }
+  let!(:key_for_user_without_push_access) { create(:key, user: user_without_push_access) }
 
   let(:env) { valid_geo_env }
 
   before do
+    project.add_maintainer(user)
+    project.add_guest(user_without_push_access)
+
     stub_licensed_features(geo: true)
-    stub_current_geo_node(secondary)
+    stub_current_geo_node(current_node)
   end
 
-  shared_examples_for 'Geo sync request' do
+  shared_examples_for 'Geo request' do
     subject do
       make_request
       response
@@ -64,20 +73,25 @@ describe "Git HTTP requests (Geo)" do
     end
   end
 
+  context 'when current node is a secondary' do
+    let(:current_node) { secondary }
+
+    set(:project) { create(:project, :repository, :private) }
+
     describe 'GET info_refs' do
       context 'git pull' do
         def make_request
           get "/#{project.full_path}.git/info/refs", { service: 'git-upload-pack' }, env
         end
 
-      it_behaves_like 'Geo sync request'
+        it_behaves_like 'Geo request'
 
         context 'when terms are enforced' do
           before do
             enforce_terms
           end
 
-        it_behaves_like 'Geo sync request'
+          it_behaves_like 'Geo request'
         end
       end
 
@@ -101,19 +115,19 @@ describe "Git HTTP requests (Geo)" do
       end
     end
 
-  describe 'POST upload_pack' do
+    describe 'POST git_upload_pack' do
       def make_request
         post "/#{project.full_path}.git/git-upload-pack", {}, env
       end
 
-    it_behaves_like 'Geo sync request'
+      it_behaves_like 'Geo request'
 
       context 'when terms are enforced' do
         before do
           enforce_terms
         end
 
-      it_behaves_like 'Geo sync request'
+        it_behaves_like 'Geo request'
       end
     end
 
@@ -243,6 +257,96 @@ describe "Git HTTP requests (Geo)" do
         end
       end
     end
+  end
+
+  context 'when current node is the primary' do
+    let(:current_node) { primary }
+
+    describe 'POST git_receive_pack' do
+      def make_request
+        post url, {}, env
+      end
+
+      let(:url) { "/#{project.full_path}.git/git-receive-pack" }
+
+      before do
+        env['Geo-GL-Id'] = geo_gl_id
+      end
+
+      subject do
+        make_request
+        response
+      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 'but is invalid' do
+          where(:geo_gl_id) do
+            [
+              'key-999',
+              'key-1',
+              'key-999',
+              'junk',
+              'junk-1',
+              'kkey-1'
+            ]
+          end
+
+          with_them do
+            it 'returns a 403' do
+              is_expected.to have_gitlab_http_status(:forbidden)
+              expect(response.body).to eql('Geo push user is invalid.')
+            end
+          end
+        end
+
+        context 'and is valid' do
+          context 'but the user has no access' do
+            let(:geo_gl_id) { "key-#{key_for_user_without_any_access.id}" }
+
+            it 'returns a 404' do
+              is_expected.to have_gitlab_http_status(:not_found)
+              expect(response.body).to eql('The project you were looking for could not be found.')
+            end
+          end
+
+          context 'but the user does not have push access' do
+            let(:geo_gl_id) { "key-#{key_for_user_without_push_access.id}" }
+
+            it 'returns a 403' do
+              is_expected.to have_gitlab_http_status(:forbidden)
+              expect(response.body).to eql('You are not allowed to push code to this project.')
+            end
+          end
+
+          context 'and the user has push access' do
+            let(:geo_gl_id) { "key-#{key.id}" }
+
+            it 'returns a 200' do
+              is_expected.to have_gitlab_http_status(:ok)
+              expect(json_response['GL_ID']).to match("user-#{user.id}")
+              expect(json_response['GL_REPOSITORY']).to match(Gitlab::GlRepository.gl_repository(project, false))
+            end
+          end
+        end
+      end
+    end
+  end
 
   def valid_geo_env
     geo_env(auth_token)

lib/gitlab/checks/change_access.rb

@@ -50,6 +50,7 @@ module Gitlab
 
       def push_checks
         unless can_push?
+          # You are not allowed to push code to this project.
           raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
         end
       end

lib/gitlab/git_access.rb

@@ -236,11 +236,13 @@ module Gitlab
     # TODO: please clean this up
     def check_push_access!
       if project.repository_read_only?
+        # The repository is temporarily read-only. Please try again later.
         raise UnauthorizedError, ERROR_MESSAGES[:read_only]
       end
 
       if deploy_key?
         unless deploy_key.can_push_to?(project)
+          # This deploy key does not have write access to this project.
           raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
         end
       elsif user
@@ -248,6 +250,7 @@ module Gitlab
       elsif authed_via_jwt?
         # Authenticated via JWT
       else
+        # You are not allowed to upload code for this project.
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
 

spec/lib/gitlab/git_access_spec.rb

@@ -1165,13 +1165,6 @@ describe Gitlab::GitAccess do
     end
   end
 
-  describe 'Geo system permissions' do
-    let(:actor) { :geo }
-
-    it { expect { pull_access_check }.not_to raise_error }
-    it { expect { push_access_check }.to raise_unauthorized(Gitlab::GitAccess::ERROR_MESSAGES[:upload]) }
-  end
-
   context 'terms are enforced' do
     before do
       enforce_terms