Block LFS writes in read-only mode

ee/changelogs/unreleased/ag-block-lfs-writes-in-read-only-mode.yml

+---
+title: Block LFS writes when database is read-only but allow on Geo secondaries
+merge_request: 47684
+author:
+type: changed

ee/lib/ee/gitlab/middleware/read_only/controller.rb

@@ -20,11 +20,15 @@ module EE
             'repositories/git_http' => %w{git_receive_pack}
           }.freeze
 
+          ALLOWLISTED_GIT_LFS_LOCKS_ROUTES = {
+            'repositories/lfs_locks_api' => %w{verify create unlock}
+          }.freeze
+
           private
 
           override :allowlisted_routes
           def allowlisted_routes
-            super || geo_node_update_route? || geo_proxy_git_ssh_route? || geo_api_route? || geo_proxy_git_http_route?
+            super || geo_node_update_route? || geo_proxy_git_ssh_route? || geo_api_route? || geo_proxy_git_http_route? || lfs_locks_route?
           end
 
           def geo_node_update_route?
@@ -58,6 +62,18 @@ module EE
               request.path.include?("/api/v#{version}/geo_replication")
             end
           end
+
+          def lfs_locks_route?
+            # Calling route_hash may be expensive. Only do it if we think there's a possible match
+            return unless ::Gitlab::Geo.secondary?
+
+            unless request.path.end_with?('/info/lfs/locks', '/info/lfs/locks/verify') ||
+                %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path)
+              return false
+            end
+
+            ALLOWLISTED_GIT_LFS_LOCKS_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
+          end
         end
       end
     end

ee/spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_ee_instance_shared_examples.rb

@@ -35,13 +35,50 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance' do
       it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/force_redownload'
 
       it_behaves_like 'allowlisted request', :delete, '/admin/geo/replication/uploads/1'
-    end
 
-    it 'expects geo replication node api requests to be allowed' do
-      response = request.post("/api/#{API::API.version}/geo_replication/designs/resync")
+      context 'on Geo secondary' do
+        before do
+          allow(::Gitlab::Geo).to receive(:secondary?).and_return(true)
+        end
+
+        where(:description, :path) do
+          'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
+          'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+          'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+          'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+          'to geo replication node api' | "/api/#{API::API.version}/geo_replication/designs/resync"
+        end
+
+        with_them do
+          it "expects a POST #{description} URL to be allowed" do
+            response = request.post(path)
+
+            expect(response).not_to be_redirect
+            expect(subject).not_to disallow_request
+          end
+        end
+      end
 
-      expect(response).not_to be_redirect
-      expect(subject).not_to disallow_request
+      context 'when not on Geo secondary' do
+        before do
+          allow(::Gitlab::Geo).to receive(:secondary?).and_return(false)
+        end
+
+        where(:description, :path) do
+          'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+          'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+          'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+        end
+
+        with_them do
+          it "expects a POST #{description} URL not to be allowed" do
+            response = request.post(path)
+
+            expect(response).to be_redirect
+            expect(subject).to disallow_request
+          end
+        end
+      end
     end
 
     it 'expects a POST request to git-receive-pack URL to be allowed' do

lib/gitlab/middleware/read_only/controller.rb

@@ -13,9 +13,8 @@ module Gitlab
           'repositories/git_http' => %w{git_upload_pack}
         }.freeze
 
-        ALLOWLISTED_GIT_LFS_ROUTES = {
-          'repositories/lfs_api' => %w{batch},
-          'repositories/lfs_locks_api' => %w{verify create unlock}
+        ALLOWLISTED_GIT_LFS_BATCH_ROUTES = {
+          'repositories/lfs_api' => %w{batch}
         }.freeze
 
         ALLOWLISTED_GIT_REVISION_ROUTES = {
@@ -88,7 +87,7 @@ module Gitlab
 
         # Overridden in EE module
         def allowlisted_routes
-          workhorse_passthrough_route? || internal_route? || lfs_route? || compare_git_revisions_route? || sidekiq_route? || session_route? || graphql_query?
+          workhorse_passthrough_route? || internal_route? || lfs_batch_route? || compare_git_revisions_route? || sidekiq_route? || session_route? || graphql_query?
         end
 
         # URL for requests passed through gitlab-workhorse to rails-web
@@ -112,15 +111,13 @@ module Gitlab
           ALLOWLISTED_GIT_REVISION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
 
-        def lfs_route?
+        # Batch upload requests are blocked in:
+        # https://gitlab.com/gitlab-org/gitlab/blob/master/app/controllers/repositories/lfs_api_controller.rb#L106
+        def lfs_batch_route?
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          unless request.path.end_with?('/info/lfs/objects/batch',
-            '/info/lfs/locks', '/info/lfs/locks/verify') ||
-              %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path)
-            return false
-          end
+          return unless request.path.end_with?('/info/lfs/objects/batch')
 
-          ALLOWLISTED_GIT_LFS_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
+          ALLOWLISTED_GIT_LFS_BATCH_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
 
         def session_route?

spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb

@@ -124,9 +124,6 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
 
       where(:description, :path) do
         'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
-        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
-        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
-        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
         'request to git-upload-pack'  | '/root/rouge.git/git-upload-pack'
       end
 
@@ -139,6 +136,21 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
           expect(subject).not_to disallow_request
         end
       end
+
+      where(:description, :path) do
+        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+      end
+
+      with_them do
+        it "expects a POST #{description} URL not to be allowed" do
+          response = request.post(path)
+
+          expect(response).to be_redirect
+          expect(subject).to disallow_request
+        end
+      end
     end
   end