Merge branch '32113-extend-group-ip-restriction-to-git-activity' into 'master'

Extend group IP restriction to Git activity

Closes #32113

See merge request gitlab-org/gitlab!16980

GITLAB_SHELL_VERSION

-10.0.0
+10.1.0

doc/development/internal_api.md

@@ -47,6 +47,7 @@ POST /internal/allowed
 | `protocol` | string | yes     | SSH when called from GitLab-shell, HTTP or SSH when called from Gitaly |
 | `action`   | string | yes     | Git command being run (`git-upload-pack`, `git-receive-pack`, `git-upload-archive`) |
 | `changes`  | string | yes     | `<oldrev> <newrev> <refname>` when called from Gitaly, The magic string `_any` when called from GitLab Shell |
+| `check_ip` | string | no     | Ip adress from which call to GitLab Shell was made |
 
 Example request:
 

doc/user/group/index.md

@@ -351,7 +351,7 @@ Add one or more whitelisted IP subnets using CIDR notation in comma separated fo
 coming from a different IP address won't be able to access the restricted
 content.
 
-Restriction currently applies to UI and API access, Git actions via SSH are not restricted.
+Restriction currently applies to UI, API access and Git actions via SSH.
 To avoid accidental lock-out, admins and group owners are are able to access
 the group regardless of the IP restriction.
 

ee/changelogs/unreleased/32113-extend-group-ip-restriction-to-git-activity.yml

+---
+title: Extend group IP restriction to Git activity
+merge_request: 16980
+author:
+type: added

ee/lib/ee/api/internal/base.rb

@@ -34,6 +34,14 @@ module EE
             def fetch_geo_node_referrer
               ::Gitlab::Geo::GitPushHttp.new(params[:identifier], params[:gl_repository]).fetch_referrer_node
             end
+
+            override :check_allowed
+            def check_allowed(params)
+              ip = params.fetch(:check_ip, nil)
+              ::Gitlab::IpAddressState.with(ip) do # rubocop: disable CodeReuse/ActiveRecord
+                super
+              end
+            end
           end
         end
       end

ee/spec/lib/gitlab/middleware/ip_restrictor_spec.rb

@@ -38,7 +38,7 @@ describe Gitlab::Middleware::IpRestrictor do
         allow(env).to receive(:[]).with('PATH_INFO').and_return('/api/v4/internal/allowed')
       end
 
-      it 'calls ip address state to set the address' do
+      it 'does not call ip address state to set the address' do
         expect(::Gitlab::IpAddressState).not_to receive(:with)
         expect(app).to receive(:call)
 

ee/spec/requests/api/internal/base_spec.rb

@@ -252,6 +252,54 @@ describe API::Internal::Base do
         end
       end
     end
+
+    context 'ip restriction' do
+      let_it_be(:group) { create(:group)}
+      let_it_be(:project) { create(:project, :repository, namespace: group) }
+      let(:params) do
+        {
+          key_id: key.id,
+          project: project.full_path,
+          gl_repository: "project-#{project.id}",
+          action: 'git-upload-pack',
+          secret_token: secret_token,
+          protocol: 'ssh'
+        }
+      end
+      let(:allowed_ip) { '150.168.0.1' }
+
+      before do
+        create(:ip_restriction, group: group, range: allowed_ip)
+        stub_licensed_features(group_ip_restriction: true)
+
+        project.add_developer(user)
+      end
+
+      context 'with or without check_ip parameter' do
+        using RSpec::Parameterized::TableSyntax
+
+        where(:check_ip_present, :ip, :status) do
+          false | nil           | 200
+          true  | '150.168.0.1' | 200
+          true  | '150.168.0.2' | 404
+        end
+
+        with_them do
+          subject do
+            post(
+              api('/internal/allowed'),
+              params: check_ip_present ? params.merge(check_ip: ip) : params
+            )
+          end
+
+          it 'modifies access' do
+            subject
+
+            expect(response).to have_gitlab_http_status(status)
+          end
+        end
+      end
+    end
   end
 
   describe "POST /internal/lfs_authenticate", :geo do

lib/api/internal/base.rb

@@ -26,20 +26,11 @@ module API
         def ee_post_receive_response_hook(response)
           # Hook for EE to add messages
         end
-      end
 
-      namespace 'internal' do
-        # Check if git command is allowed for project
-        #
-        # Params:
-        #   key_id - ssh key id for Git over SSH
-        #   user_id - user id for Git over HTTP or over SSH in keyless SSH CERT mode
-        #   username - user name for Git over SSH in keyless SSH cert mode
-        #   protocol - Git access protocol being used, e.g. HTTP or SSH
-        #   project - project full_path (not path on disk)
-        #   action - git action (git-upload-pack or git-receive-pack)
-        #   changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
-        post "/allowed" do
+        def check_allowed(params)
+          # This is a separate method so that EE can alter its behaviour more
+          # easily.
+
           # Stores some Git-specific env thread-safely
           env = parse_env
           Gitlab::Git::HookEnv.set(gl_repository, env) if project
@@ -53,11 +44,11 @@ module API
                            @project ||= access_checker.project
                            result
                          rescue Gitlab::GitAccess::UnauthorizedError => e
-                           break response_with_status(code: 401, success: false, message: e.message)
+                           return response_with_status(code: 401, success: false, message: e.message)
                          rescue Gitlab::GitAccess::TimeoutError => e
-                           break response_with_status(code: 503, success: false, message: e.message)
+                           return response_with_status(code: 503, success: false, message: e.message)
                          rescue Gitlab::GitAccess::NotFoundError => e
-                           break response_with_status(code: 404, success: false, message: e.message)
+                           return response_with_status(code: 404, success: false, message: e.message)
                          end
 
           log_user_activity(actor.user)
@@ -91,6 +82,26 @@ module API
             response_with_status(code: 500, success: false, message: UNKNOWN_CHECK_RESULT_ERROR)
           end
         end
+      end
+
+      namespace 'internal' do
+        # Check if git command is allowed for project
+        #
+        # Params:
+        #   key_id - ssh key id for Git over SSH
+        #   user_id - user id for Git over HTTP or over SSH in keyless SSH CERT mode
+        #   username - user name for Git over SSH in keyless SSH cert mode
+        #   protocol - Git access protocol being used, e.g. HTTP or SSH
+        #   project - project full_path (not path on disk)
+        #   action - git action (git-upload-pack or git-receive-pack)
+        #   changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
+        #   check_ip - optional, only in EE version, may limit access to
+        #     group resources based on its IP restrictions
+        post "/allowed" do
+          # It was moved to a separate method so that EE can alter its behaviour more
+          # easily.
+          check_allowed(params)
+        end
 
         # rubocop: disable CodeReuse/ActiveRecord
         post "/lfs_authenticate" do