Merge branch 'security-security/sh-deploy-token-ip-fix-14-10' into '14-10-stable-ee'

Fix IP restrictions not applying to deploy tokens

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

ee/lib/ee/gitlab/git_access.rb

@@ -25,6 +25,23 @@ module EE
         super
       end
 
+      override :check_project_accessibility!
+      def check_project_accessibility!
+        super
+
+        # Deploy keys and tokens are unique in that we don't check
+        # against the project policy, where IP restrictions normally are
+        # checked. The existence of a project's associated key or
+        # token is enough to authenticate read access. To ensure deploy keys
+        # and tokens honor the IP allow list, we need to force a check here. We
+        # don't want to do this for all Git access because GitLab admin users
+        # aren't subject to this IP restriction, but deploy keys and tokens don't
+        # necessarily have an associated user.
+        return unless deploy_key? || deploy_token?
+
+        raise ::Gitlab::GitAccess::NotFoundError, not_found_message if ip_restricted?
+      end
+
       def group?
         # Strict nil check, to avoid any surprises with Object#present?
         # which can delegate to #empty?
@@ -46,6 +63,10 @@ module EE
 
       private
 
+      def ip_restricted?
+        !::Gitlab::IpRestriction::Enforcer.new(project.group).allows_current_ip? if project.group
+      end
+
       override :check_custom_action
       def check_custom_action
         geo_custom_action || super

ee/spec/lib/gitlab/git_access_spec.rb

@@ -30,6 +30,180 @@ RSpec.describe Gitlab::GitAccess do
     end
   end
 
+  describe '#check_project_accessibility!' do
+    let_it_be_with_reload(:group) { create(:group, :public) }
+    let_it_be(:project) { create(:project, :repository, group: group) }
+    let_it_be(:deploy_key) { create(:deploy_key, user: user) }
+    let_it_be(:admin) { create(:admin) }
+
+    let(:deploy_token) { create(:deploy_token, projects: [project]) }
+
+    let(:start_sha) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
+    let(:end_sha)   { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
+    let(:changes)   { "#{start_sha} #{end_sha} refs/heads/master" }
+    let(:push_error_message) { Gitlab::GitAccess::ERROR_MESSAGES[:upload] }
+
+    before(:all) do
+      project.add_developer(user)
+      deploy_key.deploy_keys_projects.create!(project: project, can_push: true)
+    end
+
+    context 'with ip restriction' do
+      before do
+        allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
+        stub_licensed_features(group_ip_restriction: true)
+      end
+
+      context 'group with restriction' do
+        before do
+          create(:ip_restriction, group: group, range: range)
+        end
+
+        context 'address is within the range' do
+          let(:range) { '192.168.0.0/24' }
+
+          context 'when actor is a DeployKey with access to project' do
+            let(:actor) { deploy_key }
+
+            it 'allows pull, push access' do
+              aggregate_failures do
+                expect { pull_changes }.not_to raise_error
+                expect { push_changes }.not_to raise_error
+              end
+            end
+          end
+
+          context 'when actor is DeployToken with access to project' do
+            let(:actor) { deploy_token }
+
+            it 'allows pull access' do
+              aggregate_failures do
+                expect { pull_changes }.not_to raise_error
+                expect { push_changes }.to raise_forbidden(push_error_message)
+              end
+            end
+          end
+
+          context 'when actor is user with access to project' do
+            let(:actor) { user }
+
+            it 'allows push, pull access' do
+              aggregate_failures do
+                expect { pull_changes }.not_to raise_error
+                expect { push_changes }.not_to raise_error
+              end
+            end
+          end
+
+          context 'when actor is instance admin', :enable_admin_mode do
+            let(:actor) { admin }
+
+            it 'allows push, pull access' do
+              aggregate_failures do
+                expect { pull_changes }.not_to raise_error
+                expect { push_changes }.not_to raise_error
+              end
+            end
+          end
+        end
+
+        context 'address is outside the range' do
+          let(:range) { '10.0.0.0/8' }
+
+          context 'when actor is a DeployKey with access to project' do
+            let(:actor) { deploy_key }
+
+            it 'blocks pull, push with "not found"' do
+              aggregate_failures do
+                expect { pull_changes }.to raise_not_found
+                expect { push_changes }.to raise_not_found
+              end
+            end
+          end
+
+          context 'when actor is DeployToken with access to project' do
+            let(:actor) { deploy_token }
+
+            it 'blocks pull, push with "not found"' do
+              aggregate_failures do
+                expect { pull_changes }.to raise_not_found
+                expect { push_changes }.to raise_not_found
+              end
+            end
+          end
+
+          context 'when actor is user with access to project' do
+            let(:actor) { user }
+
+            it 'blocks pull, push with "not found"' do
+              aggregate_failures do
+                expect { pull_changes }.to raise_not_found
+                expect { push_changes }.to raise_not_found
+              end
+            end
+          end
+
+          context 'when actor is instance admin', :enable_admin_mode do
+            let(:actor) { admin }
+
+            it 'allows push, pull access' do
+              aggregate_failures do
+                expect { pull_changes }.not_to raise_error
+                expect { push_changes }.not_to raise_error
+              end
+            end
+          end
+        end
+      end
+
+      context 'group without restriction' do
+        context 'when actor is a DeployKey with access to project' do
+          let(:actor) { deploy_key }
+
+          it 'allows pull, push access' do
+            aggregate_failures do
+              expect { pull_changes }.not_to raise_error
+              expect { push_changes }.not_to raise_error
+            end
+          end
+        end
+
+        context 'when actor is DeployToken with access to project' do
+          let(:actor) { deploy_token }
+
+          it 'allows pull access' do
+            aggregate_failures do
+              expect { pull_changes }.not_to raise_error
+              expect { push_changes }.to raise_forbidden(push_error_message)
+            end
+          end
+        end
+
+        context 'when actor is user with access to project' do
+          let(:actor) { user }
+
+          it 'allows push, pull access' do
+            aggregate_failures do
+              expect { pull_changes }.not_to raise_error
+              expect { push_changes }.not_to raise_error
+            end
+          end
+        end
+
+        context 'when actor is instance admin', :enable_admin_mode do
+          let(:actor) { admin }
+
+          it 'allows push, pull access' do
+            aggregate_failures do
+              expect { pull_changes }.not_to raise_error
+              expect { push_changes }.not_to raise_error
+            end
+          end
+        end
+      end
+    end
+  end
+
   context "when in a read-only GitLab instance" do
     before do
       create(:protected_branch, name: 'feature', project: project)
@@ -1094,4 +1268,8 @@ RSpec.describe Gitlab::GitAccess do
   def raise_forbidden(message)
     raise_error(Gitlab::GitAccess::ForbiddenError, message)
   end
+
+  def raise_not_found
+    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
+  end
 end