Merge remote-tracking branch 'dev/master'

CHANGELOG-EE.md

 Please view this file on the master branch, on stable branches it's out of date.
 
+## 13.2.6 (2020-08-18)
+
+- No changes.
+
+## 13.2.5 (2020-08-17)
+
+- No changes.
+
 ## 13.2.4 (2020-08-11)
 
 ### Performance (1 change)
@@ -383,6 +391,14 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Resolve duplicate use of shorcuts-tree. !36732
 
 
+## 13.1.8 (2020-08-18)
+
+- No changes.
+
+## 13.1.7 (2020-08-17)
+
+- No changes.
+
 ## 13.1.6 (2020-08-05)
 
 - No changes.
@@ -568,6 +584,14 @@ Please view this file on the master branch, on stable branches it's out of date.
 - Relocate Go models. !34338 (Ethan Reesor (@firelizzard))
 
 
+## 13.0.14 (2020-08-18)
+
+- No changes.
+
+## 13.0.13 (2020-08-17)
+
+- No changes.
+
 ## 13.0.12 (2020-08-05)
 
 - No changes.

CHANGELOG.md

@@ -2,6 +2,18 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.2.6 (2020-08-18)
+
+- No changes.
+
+## 13.2.5 (2020-08-17)
+
+### Security (2 changes)
+
+- Stop deploy token being mis-used as user in ProjectPolicy and GroupPolicy.
+- Project access is checked during deploy token authentication.
+
+
 ## 13.2.4 (2020-08-11)
 
 ### Security (1 change)
@@ -1058,6 +1070,18 @@ entry.
 - Remove removeIssue logic from list model. (nuwe1)
 
 
+## 13.1.8 (2020-08-18)
+
+- No changes.
+
+## 13.1.7 (2020-08-17)
+
+### Security (2 changes)
+
+- Stop deploy token being mis-used as user in ProjectPolicy and GroupPolicy.
+- Project access is checked during deploy token authentication.
+
+
 ## 13.1.6 (2020-08-05)
 
 ### Security (11 changes)
@@ -1609,6 +1633,18 @@ entry.
 - Remove removeIssue logic from list model. (nuwe1)
 
 
+## 13.0.14 (2020-08-18)
+
+- No changes.
+
+## 13.0.13 (2020-08-17)
+
+### Security (2 changes)
+
+- Stop deploy token being mis-used as user in ProjectPolicy and GroupPolicy.
+- Project access is checked during deploy token authentication.
+
+
 ## 13.0.12 (2020-08-05)
 
 ### Security (10 changes)

app/policies/group_policy.rb

@@ -167,6 +167,7 @@ class GroupPolicy < BasePolicy
 
   def access_level
     return GroupMember::NO_ACCESS if @user.nil?
+    return GroupMember::NO_ACCESS unless user_is_user?
 
     @access_level ||= lookup_access_level!
   end
@@ -174,6 +175,12 @@ class GroupPolicy < BasePolicy
   def lookup_access_level!
     @subject.max_member_access_for_user(@user)
   end
+
+  private
+
+  def user_is_user?
+    user.is_a?(User)
+  end
 end
 
 GroupPolicy.prepend_if_ee('EE::GroupPolicy')

app/policies/project_policy.rb

@@ -589,8 +589,13 @@ class ProjectPolicy < BasePolicy
 
   private
 
+  def user_is_user?
+    user.is_a?(User)
+  end
+
   def team_member?
     return false if @user.nil?
+    return false unless user_is_user?
 
     greedy_load_subject = false
 
@@ -618,6 +623,7 @@ class ProjectPolicy < BasePolicy
   # rubocop: disable CodeReuse/ActiveRecord
   def project_group_member?
     return false if @user.nil?
+    return false unless user_is_user?
 
     project.group &&
       (
@@ -629,6 +635,7 @@ class ProjectPolicy < BasePolicy
 
   def team_access_level
     return -1 if @user.nil?
+    return -1 unless user_is_user?
 
     lookup_access_level!
   end

lib/gitlab/auth.rb

@@ -220,6 +220,9 @@ module Gitlab
         return unless token && login
         return if login != token.username
 
+        # Registry access (with jwt) does not have access to project
+        return if project && !token.has_access_to?(project)
+
         scopes = abilities_for_scopes(token.scopes)
 
         if valid_scoped_token?(token, all_available_scopes)

spec/lib/gitlab/auth_spec.rb

@@ -551,7 +551,7 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
 
         it 'fails if token is not related to project' do
           another_deploy_token = create(:deploy_token)
-          expect(gl_auth.find_for_git_client(login, another_deploy_token.token, project: project, ip: 'ip'))
+          expect(gl_auth.find_for_git_client(another_deploy_token.username, another_deploy_token.token, project: project, ip: 'ip'))
             .to eq(auth_failure)
         end
 
@@ -576,6 +576,13 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
 
           expect(subject).to eq(auth_success)
         end
+
+        it 'fails if token is not related to group' do
+          another_deploy_token = create(:deploy_token, :group, read_repository: true)
+
+          expect(gl_auth.find_for_git_client(another_deploy_token.username, another_deploy_token.token, project: project_with_group, ip: 'ip'))
+            .to eq(auth_failure)
+        end
       end
 
       context 'when the deploy token has read_registry as a scope' do

spec/policies/group_policy_spec.rb

@@ -63,6 +63,24 @@ RSpec.describe GroupPolicy do
     end
   end
 
+  shared_examples 'deploy token does not get confused with user' do
+    before do
+      deploy_token.update!(id: user_id)
+    end
+
+    let(:deploy_token) { create(:deploy_token) }
+    let(:current_user) { deploy_token }
+
+    it do
+      expect_disallowed(*read_group_permissions)
+      expect_disallowed(*guest_permissions)
+      expect_disallowed(*reporter_permissions)
+      expect_disallowed(*developer_permissions)
+      expect_disallowed(*maintainer_permissions)
+      expect_disallowed(*owner_permissions)
+    end
+  end
+
   context 'guests' do
     let(:current_user) { guest }
 
@@ -74,6 +92,10 @@ RSpec.describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { guest.id }
+    end
   end
 
   context 'reporter' do
@@ -87,6 +109,10 @@ RSpec.describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { reporter.id }
+    end
   end
 
   context 'developer' do
@@ -100,6 +126,10 @@ RSpec.describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { developer.id }
+    end
   end
 
   context 'maintainer' do
@@ -136,6 +166,10 @@ RSpec.describe GroupPolicy do
         expect_disallowed(*owner_permissions)
       end
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { maintainer.id }
+    end
   end
 
   context 'owner' do
@@ -149,6 +183,10 @@ RSpec.describe GroupPolicy do
       expect_allowed(*maintainer_permissions)
       expect_allowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { owner.id }
+    end
   end
 
   context 'admin' do
@@ -166,6 +204,14 @@ RSpec.describe GroupPolicy do
     context 'with admin mode', :enable_admin_mode do
       specify { expect_allowed(*admin_permissions) }
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { admin.id }
+
+      context 'with admin mode', :enable_admin_mode do
+        it { expect_disallowed(*admin_permissions) }
+      end
+    end
   end
 
   describe 'private nested group use the highest access level from the group and inherited permissions' do

spec/requests/api/maven_packages_spec.rb

@@ -193,6 +193,24 @@ RSpec.describe API::MavenPackages do
       it_behaves_like 'downloads with a job token'
 
       it_behaves_like 'downloads with a deploy token'
+
+      it 'does not allow download by a unauthorized deploy token with same id as a user with access' do
+        unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true)
+
+        another_user = create(:user)
+        project.add_developer(another_user)
+
+        # We force the id of the deploy token and the user to be the same
+        unauthorized_deploy_token.update!(id: another_user.id)
+
+        download_file(
+          package_file.file_name,
+          {},
+          Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token
+        )
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
     end
 
     context 'project name is different from a package name' do
@@ -451,6 +469,20 @@ RSpec.describe API::MavenPackages do
       expect(response).to have_gitlab_http_status(:ok)
     end
 
+    it 'rejects requests by a unauthorized deploy token with same id as a user with access' do
+      unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true)
+
+      another_user = create(:user)
+      project.add_developer(another_user)
+
+      # We force the id of the deploy token and the user to be the same
+      unauthorized_deploy_token.update!(id: another_user.id)
+
+      authorize_upload({}, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token))
+
+      expect(response).to have_gitlab_http_status(:forbidden)
+    end
+
     def authorize_upload(params = {}, request_headers = headers)
       put api("/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/maven-metadata.xml/authorize"), params: params, headers: request_headers
     end
@@ -538,6 +570,20 @@ RSpec.describe API::MavenPackages do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
+      it 'rejects uploads by a unauthorized deploy token with same id as a user with access' do
+        unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true)
+
+        another_user = create(:user)
+        project.add_developer(another_user)
+
+        # We force the id of the deploy token and the user to be the same
+        unauthorized_deploy_token.update!(id: another_user.id)
+
+        upload_file(params, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token))
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+
       context 'version is not correct' do
         let(:version) { '$%123' }
 

spec/requests/lfs_http_spec.rb

@@ -549,12 +549,6 @@ RSpec.describe 'Git LFS API and storage' do
           project.lfs_objects << lfs_object
         end
 
-        context 'when Deploy Token is valid' do
-          let(:deploy_token) { create(:deploy_token, projects: [project]) }
-
-          it_behaves_like 'an authorized request', renew_authorization: false
-        end
-
         context 'when Deploy Token is not valid' do
           let(:deploy_token) { create(:deploy_token, projects: [project], read_repository: false) }
 
@@ -564,7 +558,14 @@ RSpec.describe 'Git LFS API and storage' do
         context 'when Deploy Token is not related to the project' do
           let(:deploy_token) { create(:deploy_token, projects: [other_project]) }
 
-          it_behaves_like 'LFS http 404 response'
+          it_behaves_like 'LFS http 401 response'
+        end
+
+        # TODO: We should fix this test case that causes flakyness by alternating the result of the above test cases.
+        context 'when Deploy Token is valid' do
+          let(:deploy_token) { create(:deploy_token, projects: [project]) }
+
+          it_behaves_like 'an authorized request', renew_authorization: false
         end
       end
 

spec/support/shared_examples/policies/project_policy_shared_examples.rb

@@ -86,6 +86,28 @@ RSpec.shared_examples 'project policies as anonymous' do
   end
 end
 
+RSpec.shared_examples 'deploy token does not get confused with user' do
+  before do
+    deploy_token.update!(id: user_id)
+
+    # Project with public builds are available to all
+    project.update!(public_builds: false)
+  end
+
+  let(:deploy_token) { create(:deploy_token) }
+
+  subject { described_class.new(deploy_token, project) }
+
+  it do
+    expect_disallowed(*guest_permissions)
+    expect_disallowed(*reporter_permissions)
+    expect_disallowed(*team_member_reporter_permissions)
+    expect_disallowed(*developer_permissions)
+    expect_disallowed(*maintainer_permissions)
+    expect_disallowed(*owner_permissions)
+  end
+end
+
 RSpec.shared_examples 'project policies as guest' do
   subject { described_class.new(guest, project) }
 
@@ -104,6 +126,10 @@ RSpec.shared_examples 'project policies as guest' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { guest.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { guest_permissions }
     end
@@ -117,7 +143,7 @@ RSpec.shared_examples 'project policies as guest' do
 
     context 'when public builds disabled' do
       before do
-        project.update(public_builds: false)
+        project.update!(public_builds: false)
       end
 
       it do
@@ -128,7 +154,7 @@ RSpec.shared_examples 'project policies as guest' do
 
     context 'when builds are disabled' do
       before do
-        project.project_feature.update(builds_access_level: ProjectFeature::DISABLED)
+        project.project_feature.update!(builds_access_level: ProjectFeature::DISABLED)
       end
 
       it do
@@ -154,6 +180,10 @@ RSpec.shared_examples 'project policies as reporter' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { reporter.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { reporter_permissions }
     end
@@ -175,6 +205,10 @@ RSpec.shared_examples 'project policies as developer' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { developer.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { developer_permissions }
     end
@@ -196,6 +230,10 @@ RSpec.shared_examples 'project policies as maintainer' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { maintainer.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { maintainer_permissions }
     end
@@ -217,6 +255,10 @@ RSpec.shared_examples 'project policies as owner' do
       expect_allowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { owner.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { owner_permissions }
     end
@@ -238,6 +280,28 @@ RSpec.shared_examples 'project policies as admin with admin mode' do
       expect_allowed(*owner_permissions)
     end
 
+    context 'deploy token does not get confused with user' do
+      before do
+        allow(deploy_token).to receive(:id).and_return(admin.id)
+
+        # Project with public builds are available to all
+        project.update!(public_builds: false)
+      end
+
+      let(:deploy_token) { create(:deploy_token) }
+
+      subject { described_class.new(deploy_token, project) }
+
+      it do
+        expect_disallowed(*guest_permissions)
+        expect_disallowed(*reporter_permissions)
+        expect_disallowed(*team_member_reporter_permissions)
+        expect_disallowed(*developer_permissions)
+        expect_disallowed(*maintainer_permissions)
+        expect_disallowed(*owner_permissions)
+      end
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { owner_permissions }
     end
@@ -257,5 +321,20 @@ RSpec.shared_examples 'project policies as admin without admin mode' do
     subject { described_class.new(admin, project) }
 
     it { is_expected.to be_banned }
+
+    context 'deploy token does not get confused with user' do
+      before do
+        allow(deploy_token).to receive(:id).and_return(admin.id)
+
+        # Project with public builds are available to all
+        project.update!(public_builds: false)
+      end
+
+      let(:deploy_token) { create(:deploy_token) }
+
+      subject { described_class.new(deploy_token, project) }
+
+      it { is_expected.to be_banned }
+    end
   end
 end