Merge branch '9310-reduce-the-scope-of-geo-jwt-step-2' into 'master'

Resolve "Reduce the scope of Geo JWT. Step 2"

Closes #9310

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

doc/administration/geo/replication/security_review.md

@@ -271,7 +271,6 @@ questions from [owasp.org](https://www.owasp.org).
   - LFS and File ID.
   - Upload and File ID.
   - Job Artifact and File ID.
-- Geo JWTs scopes are not enforced, but will be in a future version (currently scheduled for GitLab 11.9).
 
 ### What access requirements have been defined for URI and Service calls?
 

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

@@ -4,6 +4,7 @@ module EE
   module Projects
     module GitHttpController
       extend ::Gitlab::Utils::Override
+      include ::Gitlab::Utils::StrongMemoize
 
       override :render_ok
       def render_ok
@@ -46,20 +47,31 @@ module EE
       override :authenticate_user
       def authenticate_user
         return super unless geo_request?
+        return render_bad_geo_auth('Bad token') unless decoded_authorization
+        return render_bad_geo_auth('Unauthorized scope') unless jwt_scope_valid?
 
-        payload = ::Gitlab::Geo::JwtRequestDecoder.new(request.headers['Authorization']).decode
-        if payload
-          @authentication_result = ::Gitlab::Auth::Result.new(nil, project, :geo, [:download_code, :push_code]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-          return # grant access
-        end
-
-        render_bad_geo_auth('Bad token')
+        # grant access
+        @authentication_result = ::Gitlab::Auth::Result.new(nil, project, :geo, [:download_code, :push_code]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
       rescue ::Gitlab::Geo::InvalidDecryptionKeyError
         render_bad_geo_auth("Invalid decryption key")
       rescue ::Gitlab::Geo::InvalidSignatureTimeError
         render_bad_geo_auth("Invalid signature time ")
       end
 
+      def jwt_scope_valid?
+        decoded_authorization[:scope] == ::Gitlab::Geo::JwtRequestDecoder.build_repository_scope(repository_type, project.id)
+      end
+
+      def repository_type
+        wiki? ? 'wiki' : 'repository'
+      end
+
+      def decoded_authorization
+        strong_memoize(:decoded_authorization) do
+          ::Gitlab::Geo::JwtRequestDecoder.new(request.headers['Authorization']).decode
+        end
+      end
+
       def render_bad_geo_auth(message)
         render plain: "Geo JWT authentication failed: #{message}", status: :unauthorized
       end

ee/app/services/geo/base_sync_service.rb

@@ -103,12 +103,11 @@ module Geo
 
     # Build a JWT header for authentication
     def jwt_authentication_header
-      authorization = ::Gitlab::Geo::RepoSyncRequest.new(scope: gl_repository).authorization
-      { "http.#{remote_url}.extraHeader" => "Authorization: #{authorization}" }
-    end
+      authorization = ::Gitlab::Geo::RepoSyncRequest.new(
+        scope: ::Gitlab::Geo::JwtRequestDecoder.build_repository_scope(type, project.id)
+      ).authorization
 
-    def gl_repository
-      "#{type}-#{project.id}"
+      { "http.#{remote_url}.extraHeader" => "Authorization: #{authorization}" }
     end
 
     def remote_url

ee/app/services/geo/file_upload_service.rb

@@ -6,22 +6,32 @@ module Geo
   #   * Returning the necessary response data to send the file back
   class FileUploadService < FileService
     attr_reader :auth_header
+    include ::Gitlab::Utils::StrongMemoize
 
     def initialize(params, auth_header)
       super(params[:type], params[:id])
       @auth_header = auth_header
     end
 
+    # Returns { code: :ok, file: CarrierWave File object } upon success
     def execute
-      # Returns { code: :ok, file: CarrierWave File object } upon success
-      data = ::Gitlab::Geo::JwtRequestDecoder.new(auth_header).decode
-      return unless data.present?
+      return unless decoded_authorization.present? && jwt_scope_valid?
 
-      uploader_klass.new(object_db_id, data).execute
+      uploader_klass.new(object_db_id, decoded_authorization).execute
     end
 
     private
 
+    def jwt_scope_valid?
+      (decoded_authorization[:file_type] == object_type.to_s) && (decoded_authorization[:file_id] == object_db_id)
+    end
+
+    def decoded_authorization
+      strong_memoize(:decoded_authorization) do
+        ::Gitlab::Geo::JwtRequestDecoder.new(auth_header).decode
+      end
+    end
+
     def uploader_klass
       "Gitlab::Geo::#{service_klass_name}Uploader".constantize
     rescue NameError => e

ee/changelogs/unreleased/9310-reduce-the-scope-of-geo-jwt-step-2.yml

+---
+title: Enforce Geo JWT tokens scope
+merge_request: 9502
+author:
+type: changed

ee/lib/ee/api/helpers.rb

@@ -4,6 +4,7 @@ module EE
   module API
     module Helpers
       extend ::Gitlab::Utils::Override
+      include ::Gitlab::Utils::StrongMemoize
 
       def require_node_to_be_enabled!
         forbidden! 'Geo node is disabled.' unless ::Gitlab::Geo.current_node&.enabled?
@@ -14,15 +15,9 @@ module EE
       end
 
       def authenticate_by_gitlab_geo_node_token!
-        auth_header = headers['Authorization']
-
-        begin
-          unless auth_header && ::Gitlab::Geo::JwtRequestDecoder.new(auth_header).decode
-            unauthorized!
-          end
-        rescue ::Gitlab::Geo::InvalidDecryptionKeyError, ::Gitlab::Geo::InvalidSignatureTimeError => e
-          render_api_error!(e.to_s, 401)
-        end
+        unauthorized! unless authorization_header_valid?
+      rescue ::Gitlab::Geo::InvalidDecryptionKeyError, ::Gitlab::Geo::InvalidSignatureTimeError => e
+        render_api_error!(e.to_s, 401)
       end
 
       override :current_user
@@ -39,6 +34,14 @@ module EE
         end
       end
 
+      def authorization_header_valid?
+        auth_header = headers['Authorization']
+        return unless auth_header
+
+        scope = ::Gitlab::Geo::JwtRequestDecoder.new(auth_header).decode.try { |x| x[:scope] }
+        scope == ::Gitlab::Geo::API_SCOPE
+      end
+
       def check_project_feature_available!(feature)
         not_found! unless user_project.feature_available?(feature)
       end

ee/lib/gitlab/geo/jwt_request_decoder.rb

@@ -12,6 +12,10 @@ module Gitlab
         token_type == ::Gitlab::Geo::BaseRequest::GITLAB_GEO_AUTH_TOKEN_TYPE
       end
 
+      def self.build_repository_scope(repository_type, project_id)
+        [repository_type, project_id].join('-')
+      end
+
       attr_reader :auth_header
 
       def initialize(auth_header)

ee/spec/lib/ee/api/helpers_spec.rb

@@ -80,5 +80,14 @@ describe EE::API::Helpers do
 
       expect(JSON.parse(last_response.body)).to eq({ 'message' => 'Gitlab::Geo::InvalidSignatureTimeError' })
     end
+
+    it 'returns unauthorized response when scope is not valid' do
+      allow_any_instance_of(::Gitlab::Geo::JwtRequestDecoder).to receive(:decode).and_return(scope: 'invalid_scope')
+
+      header 'Authorization', 'test'
+      get 'protected', params: { current_user: 'test' }
+
+      expect(JSON.parse(last_response.body)).to eq({ 'message' => '401 Unauthorized' })
+    end
   end
 end

ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb

@@ -7,6 +7,12 @@ describe Gitlab::Geo::JwtRequestDecoder do
 
   subject { described_class.new(request.headers['Authorization']) }
 
+  describe ".build_repository_scope" do
+    it 'returns a scope that consolidates repository type and project id' do
+      expect(described_class.build_repository_scope('wiki', 5)).to eq('wiki-5')
+    end
+  end
+
   describe '#decode' do
     it 'decodes correct data' do
       expect(subject.decode).to eq(data)

ee/spec/requests/git_http_geo_spec.rb

@@ -21,6 +21,7 @@ describe "Git HTTP requests (Geo)" do
   let!(:key_for_user_without_push_access) { create(:key, user: user_without_push_access) }
 
   let(:env) { valid_geo_env }
+  let(:auth_token_with_invalid_scope) { Gitlab::Geo::BaseRequest.new(scope: "invalid-#{project.id}").authorization }
 
   before do
     project.add_maintainer(user)
@@ -346,6 +347,50 @@ describe "Git HTTP requests (Geo)" do
         end
       end
     end
+
+    context 'invalid scope' do
+      let(:repository_path) { project.full_path }
+
+      subject do
+        make_request
+        response
+      end
+
+      def make_request
+        get "/#{repository_path}.git/info/refs", params: { service: 'git-upload-pack' }, headers: env
+      end
+
+      shared_examples_for 'unauthorized because of invalid scope' do
+        it { is_expected.to have_gitlab_http_status(:unauthorized) }
+
+        it 'returns correct error' do
+          expect(subject.parsed_body).to eq('Geo JWT authentication failed: Unauthorized scope')
+        end
+      end
+
+      context 'invalid scope of Geo JWT token' do
+        let(:env) { geo_env(auth_token_with_invalid_scope) }
+
+        include_examples 'unauthorized because of invalid scope'
+      end
+
+      context 'Geo JWT token scopes for wiki and repository are not interchangeable' do
+        context 'wiki scope' do
+          let(:auth_token_with_valid_wiki_scope) { Gitlab::Geo::BaseRequest.new(scope: "wiki-#{project.id}").authorization }
+          let(:env) { geo_env(auth_token_with_valid_wiki_scope) }
+
+          include_examples 'unauthorized because of invalid scope'
+        end
+
+        context 'respository scope' do
+          let(:repository_path) { project.wiki.full_path }
+          let(:auth_token_with_valid_repository_scope) { Gitlab::Geo::BaseRequest.new(scope: "repository-#{project.id}").authorization }
+          let(:env) { geo_env(auth_token_with_valid_repository_scope) }
+
+          include_examples 'unauthorized because of invalid scope'
+        end
+      end
+    end
   end
 
   def valid_geo_env

ee/spec/services/geo/file_upload_service_spec.rb

@@ -3,14 +3,35 @@ require 'spec_helper'
 describe Geo::FileUploadService do
   set(:node) { create(:geo_node, :primary) }
 
+  let(:transfer_request) { Gitlab::Geo::TransferRequest.new(request_data) }
+  let(:req_header) { transfer_request.headers['Authorization'] }
+
+  shared_examples 'no authorization header' do
+    it 'returns nil' do
+      service = described_class.new(params, nil)
+
+      expect(service.execute).to be_nil
+    end
+  end
+
+  shared_examples 'wrong scope' do
+    context 'at least one scope parameter is wrong' do
+      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(request_data.merge(file_type: 'wrong')) }
+
+      it 'returns nil' do
+        service = described_class.new(params, req_header)
+
+        expect(service.execute).to be_nil
+      end
+    end
+  end
+
   describe '#execute' do
     context 'user avatar' do
       let(:user) { create(:user, avatar: fixture_file_upload('spec/fixtures/dk.png', 'image/png')) }
       let(:upload) { Upload.find_by(model: user, uploader: 'AvatarUploader') }
       let(:params) { { id: upload.id, type: 'avatar' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:avatar, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:avatar, upload).request_data }
 
       it 'sends avatar file' do
         service = described_class.new(params, req_header)
@@ -21,20 +42,15 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(user.avatar.path)
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'group avatar' do
       let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png', 'image/png')) }
       let(:upload) { Upload.find_by(model: group, uploader: 'AvatarUploader') }
       let(:params) { { id: upload.id, type: 'avatar' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:avatar, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:avatar, upload).request_data }
 
       it 'sends avatar file' do
         service = described_class.new(params, req_header)
@@ -45,20 +61,15 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(group.avatar.path)
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'project avatar' do
       let(:project) { create(:project, avatar: fixture_file_upload('spec/fixtures/dk.png', 'image/png')) }
       let(:upload) { Upload.find_by(model: project, uploader: 'AvatarUploader') }
       let(:params) { { id: upload.id, type: 'avatar' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:avatar, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:avatar, upload).request_data }
 
       it 'sends avatar file' do
         service = described_class.new(params, req_header)
@@ -69,20 +80,15 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(project.avatar.path)
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'attachment' do
       let(:note) { create(:note, :with_attachment) }
       let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
       let(:params) { { id: upload.id, type: 'attachment' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:attachment, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:attachment, upload).request_data }
 
       it 'sends attachment file' do
         service = described_class.new(params, req_header)
@@ -93,20 +99,15 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(note.attachment.path)
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'file upload' do
       let(:project) { create(:project) }
       let(:upload) { Upload.find_by(model: project, uploader: 'FileUploader') }
       let(:params) { { id: upload.id, type: 'file' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:file, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:file, upload).request_data }
       let(:file) { fixture_file_upload('spec/fixtures/dk.png', 'image/png') }
 
       before do
@@ -122,20 +123,15 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to end_with('dk.png')
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'namespace file upload' do
       let(:group) { create(:group) }
       let(:upload) { Upload.find_by(model: group, uploader: 'NamespaceFileUploader') }
       let(:params) { { id: upload.id, type: 'file' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:file, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:file, upload).request_data }
       let(:file) { fixture_file_upload('spec/fixtures/dk.png', 'image/png') }
 
       before do
@@ -151,19 +147,14 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to end_with('dk.png')
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'LFS Object' do
       let(:lfs_object) { create(:lfs_object, :with_file) }
       let(:params) { { id: lfs_object.id, type: 'lfs' } }
-      let(:lfs_transfer) { Gitlab::Geo::LfsTransfer.new(lfs_object) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(lfs_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::LfsTransfer.new(lfs_object).request_data }
 
       it 'sends LFS file' do
         service = described_class.new(params, req_header)
@@ -174,19 +165,14 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(lfs_object.file.path)
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'job artifact' do
       let(:job_artifact) { create(:ci_job_artifact, :with_file) }
       let(:params) { { id: job_artifact.id, type: 'job_artifact' } }
-      let(:job_artifact_transfer) { Gitlab::Geo::JobArtifactTransfer.new(job_artifact) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(job_artifact_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::JobArtifactTransfer.new(job_artifact).request_data }
 
       it 'sends job artifact file' do
         service = described_class.new(params, req_header)
@@ -197,20 +183,15 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to eq(job_artifact.file.path)
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
 
     context 'import export archive' do
       let(:project) { create(:project) }
       let(:upload) { Upload.find_by(model: project, uploader: 'ImportExportUploader') }
       let(:params) { { id: upload.id, type: 'import_export' } }
-      let(:file_transfer) { Gitlab::Geo::FileTransfer.new(:import_export, upload) }
-      let(:transfer_request) { Gitlab::Geo::TransferRequest.new(file_transfer.request_data) }
-      let(:req_header) { transfer_request.headers['Authorization'] }
+      let(:request_data) { Gitlab::Geo::FileTransfer.new(:import_export, upload).request_data }
       let(:file) { fixture_file_upload('spec/fixtures/project_export.tar.gz') }
 
       before do
@@ -226,11 +207,8 @@ describe Geo::FileUploadService do
         expect(response[:file].path).to end_with('tar.gz')
       end
 
-      it 'returns nil if no authorization' do
-        service = described_class.new(params, nil)
-
-        expect(service.execute).to be_nil
-      end
+      include_examples 'no authorization header'
+      include_examples 'wrong scope'
     end
   end
 end