Merge branch 'security-workhorse-package-bypass' into 'master'

Verify workhorse request for file uploads

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

changelogs/unreleased/security-workhorse-package-bypass.yml

+---
+title: Add workhorse request verification to package upload endpoints
+merge_request:
+author:
+type: security

ee/lib/api/maven_packages.rb

@@ -179,10 +179,7 @@ module API
       end
       route_setting :authentication, job_token_allowed: true
       put ':id/packages/maven/*path/:file_name/authorize', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
-        authorize_create_package!(user_project)
-
-        require_gitlab_workhorse!
-        Gitlab::Workhorse.verify_api_request!(headers)
+        authorize_upload!
 
         status 200
         content_type Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
@@ -205,8 +202,7 @@ module API
       end
       route_setting :authentication, job_token_allowed: true
       put ':id/packages/maven/*path/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
-        authorize_create_package!(user_project)
-        require_gitlab_workhorse!
+        authorize_upload!
 
         file_name, format = extract_format(params[:file_name])
 

ee/spec/requests/api/conan_packages_spec.rb

@@ -586,6 +586,8 @@ describe API::ConanPackages do
         context 'without a file from workhorse' do
           let(:params) { { file: nil } }
 
+          it_behaves_like 'package workhorse uploads'
+
           it 'rejects the request' do
             subject
 
@@ -710,7 +712,7 @@ describe API::ConanPackages do
 
         subject
 
-        expect(response).to have_gitlab_http_status(:error)
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
 
       context 'when using remote storage' do

ee/spec/requests/api/nuget_packages_spec.rb

@@ -172,8 +172,8 @@ describe API::NugetPackages do
   end
 
   describe 'PUT /api/v4/projects/:id/packages/nuget' do
-    let_it_be(:workhorse_token) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
-    let_it_be(:workhorse_header) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => workhorse_token } }
+    let(:workhorse_token) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
+    let(:workhorse_header) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => workhorse_token } }
     let_it_be(:file_name) { 'package.nupkg' }
     let(:url) { "/projects/#{project.id}/packages/nuget" }
     let(:headers) { {} }

ee/spec/support/shared_examples/requests/api/nuget_packages_shared_examples.rb

@@ -72,7 +72,7 @@ RSpec.shared_examples 'process nuget workhorse authorization' do |user_type, sta
         project.add_maintainer(user)
       end
 
-      it_behaves_like 'returning response status', :error
+      it_behaves_like 'returning response status', :forbidden
     end
   end
 end
@@ -104,6 +104,7 @@ RSpec.shared_examples 'process nuget upload' do |user_type, status, add_member =
       end
 
       context 'with correct params' do
+        it_behaves_like 'package workhorse uploads'
         it_behaves_like 'creates nuget package files'
         it_behaves_like 'a gitlab tracking event', described_class.name, 'push_package'
       end

ee/spec/support/shared_examples/services/packages_shared_examples.rb

@@ -115,3 +115,17 @@ RSpec.shared_examples 'background upload schedules a file migration' do
     end
   end
 end
+
+shared_examples 'package workhorse uploads' do
+  context 'without a workhorse header' do
+    let(:workhorse_token) { JWT.encode({ 'iss' => 'invalid header' }, Gitlab::Workhorse.secret, 'HS256') }
+
+    it_behaves_like 'returning response status', :forbidden
+
+    it 'logs an error' do
+      expect(Gitlab::ErrorTracking).to receive(:track_exception).once
+
+      subject
+    end
+  end
+end

lib/api/helpers.rb

@@ -258,11 +258,21 @@ module API
     end
 
     def require_gitlab_workhorse!
+      verify_workhorse_api!
+
       unless env['HTTP_GITLAB_WORKHORSE'].present?
         forbidden!('Request should be executed via GitLab Workhorse')
       end
     end
 
+    def verify_workhorse_api!
+      Gitlab::Workhorse.verify_api_request!(request.headers)
+    rescue => e
+      Gitlab::ErrorTracking.track_exception(e)
+
+      forbidden!
+    end
+
     def require_pages_enabled!
       not_found! unless user_project.pages_available?
     end

spec/requests/api/runner_spec.rb

@@ -1545,7 +1545,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
 
           authorize_artifacts
 
-          expect(response).to have_gitlab_http_status(500)
+          expect(response).to have_gitlab_http_status(:forbidden)
         end
 
         context 'authorization token is invalid' do
@@ -1675,6 +1675,18 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             end
           end
 
+          context 'Is missing GitLab Workhorse token headers' do
+            let(:jwt_token) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') }
+
+            it 'fails to post artifacts without GitLab-Workhorse' do
+              expect(Gitlab::ErrorTracking).to receive(:track_exception).once
+
+              upload_artifacts(file_upload, headers_with_token)
+
+              expect(response).to have_gitlab_http_status(:forbidden)
+            end
+          end
+
           context 'when setting an expire date' do
             let(:default_artifacts_expire_in) {}
             let(:post_data) do