Merge branch 'security-sh-fix-container-registry-ip-restriction-14-10' into '14-10-stable-ee'

Fix group IP restrictions not enforced for container registry requests

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

app/policies/project_policy.rb

@@ -54,7 +54,13 @@ class ProjectPolicy < BasePolicy
 
   desc "Container registry is disabled"
   condition(:container_registry_disabled, scope: :subject) do
-    !access_allowed_to?(:container_registry)
+    if user.is_a?(DeployToken)
+      (!user.read_registry? && !user.write_registry?) ||
+      user.revoked? ||
+      !project.container_registry_enabled?
+    else
+      !access_allowed_to?(:container_registry)
+    end
   end
 
   desc "Container registry is enabled for everyone with access to the project"
@@ -83,6 +89,16 @@ class ProjectPolicy < BasePolicy
     user.is_a?(DeployKey) && user.can_push_to?(project)
   end
 
+  desc "Deploy token with read_container_image scope"
+  condition(:read_container_image_deploy_token) do
+    user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_registry?
+  end
+
+  desc "Deploy token with create_container_image scope"
+  condition(:create_container_image_deploy_token) do
+    user.is_a?(DeployToken) && user.has_access_to?(project) && user.write_registry?
+  end
+
   desc "Deploy token with read_package_registry scope"
   condition(:read_package_registry_deploy_token) do
     user.is_a?(DeployToken) && user.has_access_to?(project) && user.read_package_registry
@@ -685,6 +701,14 @@ class ProjectPolicy < BasePolicy
     enable :push_code
   end
 
+  rule { read_container_image_deploy_token }.policy do
+    enable :read_container_image
+  end
+
+  rule { create_container_image_deploy_token }.policy do
+    enable :create_container_image
+  end
+
   rule { read_package_registry_deploy_token }.policy do
     enable :read_package
     enable :read_project

app/services/auth/container_registry_authentication_service.rb

@@ -215,15 +215,13 @@ module Auth
     def deploy_token_can_pull?(requested_project)
       has_authentication_ability?(:read_container_image) &&
         deploy_token.present? &&
-        deploy_token.has_access_to?(requested_project) &&
-        deploy_token.read_registry?
+        can?(deploy_token, :read_container_image, requested_project)
     end
 
     def deploy_token_can_push?(requested_project)
       has_authentication_ability?(:create_container_image) &&
         deploy_token.present? &&
-        deploy_token.has_access_to?(requested_project) &&
-        deploy_token.write_registry?
+        can?(deploy_token, :create_container_image, requested_project)
     end
 
     ##

ee/app/policies/ee/project_policy.rb

@@ -356,6 +356,7 @@ module EE
         prevent :read_merge_request
         prevent :read_milestone
         prevent :read_container_image
+        prevent :create_container_image
       end
 
       rule { locked_approvers_rules }.policy do

ee/spec/policies/project_policy_spec.rb

@@ -475,6 +475,7 @@ RSpec.describe ProjectPolicy do
           it { is_expected.to be_allowed(:read_merge_request) }
           it { is_expected.to be_allowed(:read_milestone) }
           it { is_expected.to be_allowed(:read_container_image) }
+          it { is_expected.to be_allowed(:create_container_image) }
         end
 
         context 'address is outside the range' do
@@ -485,6 +486,7 @@ RSpec.describe ProjectPolicy do
           it { is_expected.to be_disallowed(:read_merge_request) }
           it { is_expected.to be_disallowed(:read_milestone) }
           it { is_expected.to be_disallowed(:read_container_image) }
+          it { is_expected.to be_disallowed(:create_container_image) }
 
           context 'with admin enabled', :enable_admin_mode do
             it { is_expected.to be_allowed(:read_project) }
@@ -492,6 +494,7 @@ RSpec.describe ProjectPolicy do
             it { is_expected.to be_allowed(:read_merge_request) }
             it { is_expected.to be_allowed(:read_milestone) }
             it { is_expected.to be_allowed(:read_container_image) }
+            it { is_expected.to be_allowed(:create_container_image) }
           end
 
           context 'with admin disabled' do
@@ -500,6 +503,7 @@ RSpec.describe ProjectPolicy do
             it { is_expected.to be_disallowed(:read_merge_request) }
             it { is_expected.to be_disallowed(:read_milestone) }
             it { is_expected.to be_disallowed(:read_container_image) }
+            it { is_expected.to be_disallowed(:create_container_image) }
           end
 
           context 'with auditor' do
@@ -510,6 +514,7 @@ RSpec.describe ProjectPolicy do
             it { is_expected.to be_allowed(:read_merge_request) }
             it { is_expected.to be_allowed(:read_milestone) }
             it { is_expected.to be_allowed(:read_container_image) }
+            it { is_expected.to be_allowed(:create_container_image) }
           end
         end
       end

ee/spec/requests/jwt_controller_spec.rb

@@ -3,33 +3,131 @@
 require 'spec_helper'
 
 RSpec.describe JwtController do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+
+  let(:actions) { ['pull'] }
+  let(:expected_actions) { actions }
+  let(:scope) { "repository:#{project.full_path}:#{expected_actions.join(',')}" }
+  let(:service_name) { 'container_registry' }
+  let(:headers) { { authorization: credentials(user.username, user.password) } }
+  let(:parameters) { { account: user.username, client_id: 'docker', offline_token: true, service: service_name, scope: scope } }
+
+  shared_examples 'successful JWT auth' do
+    it 'allows access' do
+      get '/jwt/auth', params: parameters, headers: headers
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(token_response['access']).to be_present
+      expect(token_access['actions']).to eq expected_actions
+      expect(token_access['type']).to eq 'repository'
+      expect(token_access['name']).to eq project.full_path
+    end
+  end
+
+  shared_examples 'unsuccessful JWT auth' do
+    it 'denies access' do
+      get '/jwt/auth', params: parameters, headers: headers
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(token_response['access']).to eq []
+    end
+  end
+
+  context 'with IP restriction' do
+    let_it_be(:project) { create(:project, :private, group: group) }
+    let_it_be(:group_deploy_token) { create(:deploy_token, :group, groups: [group], read_registry: true, write_registry: true) }
+    let_it_be(:project_deploy_token) { create(:deploy_token, projects: [project], read_registry: true, write_registry: true) }
+
+    let(:actions) { %w[push pull] }
+
+    before do
+      project.add_developer(user)
+      stub_container_registry_config(enabled: true, key: 'spec/fixtures/x509_certificate_pk.key')
+      allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
+      stub_licensed_features(group_ip_restriction: true)
+    end
+
+    context 'group with restriction' do
+      using RSpec::Parameterized::TableSyntax
+
+      before do
+        create(:ip_restriction, group: group, range: range)
+      end
+
+      shared_examples 'successful JWT auth with token' do
+        let(:headers) { { authorization: credentials(token.username, token.token) } }
+
+        where(:read, :write, :expected_actions) do
+          true  | false | %w[pull]
+          false | true  | %w[push]
+          true  | true  | %w[push pull]
+        end
+
+        with_them do
+          before do
+            token.update!(read_registry: read, write_registry: write)
+          end
+
+          it_behaves_like 'successful JWT auth'
+        end
+      end
+
+      context 'address is within the range' do
+        let(:range) { '192.168.0.0/24' }
+
+        it_behaves_like 'successful JWT auth'
+
+        context 'with project deploy token' do
+          let(:token) { project_deploy_token }
+
+          it_behaves_like 'successful JWT auth with token'
+        end
+
+        context 'with group deploy token' do
+          let(:token) { group_deploy_token }
+
+          it_behaves_like 'successful JWT auth with token'
+        end
+      end
+
+      context 'address is outside the range' do
+        let(:range) { '10.0.0.0/8' }
+
+        it_behaves_like 'unsuccessful JWT auth'
+
+        context 'with deploy token credentials' do
+          let(:headers) { { authorization: credentials(token.username, token.token) } }
+
+          context 'with project deploy token' do
+            let(:token) { project_deploy_token }
+
+            it_behaves_like 'unsuccessful JWT auth'
+          end
+
+          context 'with group deploy token' do
+            let(:token) { group_deploy_token }
+
+            it_behaves_like 'unsuccessful JWT auth'
+          end
+        end
+      end
+    end
+  end
+
   context 'authenticating against container registry' do
-    let(:user) { create(:user) }
-    let(:group) { create(:group) }
-    let(:project) { create(:project, :private, group: group) }
-    let(:scope) { "repository:#{project.full_path}:pull" }
-    let(:service_name) { 'container_registry' }
-    let(:headers) { { authorization: credentials(user.username, user.password) } }
-    let(:parameters) { { account: user.username, client_id: 'docker', offline_token: true, service: service_name, scope: scope } }
+    let_it_be(:project) { create(:project, :private, group: group) }
 
     before do
-      stub_container_registry_config(enabled: true, issuer: 'gitlab-issuer', key: 'spec/fixtures/x509_certificate_pk.key')
       project.add_reporter(user)
+      stub_container_registry_config(enabled: true, issuer: 'gitlab-issuer', key: 'spec/fixtures/x509_certificate_pk.key')
     end
 
     context 'when Group SSO is enforced' do
       let!(:saml_provider) { create(:saml_provider, enforced_sso: true, group: group) }
       let!(:identity) { create(:group_saml_identity, saml_provider: saml_provider, user: user) }
 
-      it 'allows access' do
-        get '/jwt/auth', params: parameters, headers: headers
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(token_response['access']).to be_present
-        expect(token_access['actions']).to eq ['pull']
-        expect(token_access['type']).to eq 'repository'
-        expect(token_access['name']).to eq project.full_path
-      end
+      it_behaves_like 'successful JWT auth'
     end
   end
 

ee/spec/services/ee/auth/container_registry_authentication_service_spec.rb

@@ -5,6 +5,48 @@ require 'spec_helper'
 RSpec.describe Auth::ContainerRegistryAuthenticationService do
   include AdminModeHelper
 
+  describe 'with deploy keys' do
+    include_context 'container registry auth service context'
+
+    let_it_be_with_reload(:group) { create(:group, :public) }
+    let_it_be(:current_user) { nil }
+    let_it_be(:project) { create(:project, group: group) }
+
+    let(:deploy_token) { create(:deploy_token, projects: [project], read_registry: true, write_registry: true) }
+
+    context 'with IP restriction' do
+      before do
+        allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
+        stub_licensed_features(group_ip_restriction: true)
+      end
+
+      context 'group with restriction' do
+        before do
+          create(:ip_restriction, group: group, range: range)
+        end
+
+        context 'address is within the range' do
+          let(:range) { '192.168.0.0/24' }
+
+          it_behaves_like 'a container registry auth service'
+        end
+
+        context 'address is outside the range' do
+          let(:range) { '10.0.0.0/8' }
+          let(:current_params) do
+            { scopes: ["repository:#{project.full_path}:push,pull"], deploy_token: deploy_token }
+          end
+
+          context 'when actor is a deploy token with read access' do
+            it_behaves_like 'an inaccessible'
+            it_behaves_like 'not a container repository factory'
+            it_behaves_like 'logs an auth warning', %w(push pull)
+          end
+        end
+      end
+    end
+  end
+
   context 'in maintenance mode' do
     include_context 'container registry auth service context'
 

spec/policies/project_policy_spec.rb

@@ -1023,25 +1023,117 @@ RSpec.describe ProjectPolicy do
 
     subject { described_class.new(deploy_token, project) }
 
-    context 'a deploy token with read_package_registry scope' do
-      let(:deploy_token) { create(:deploy_token, read_package_registry: true) }
+    context 'private project' do
+      let(:project) { private_project }
 
-      it { is_expected.to be_allowed(:read_package) }
-      it { is_expected.to be_allowed(:read_project) }
-      it { is_expected.to be_disallowed(:create_package) }
+      context 'a deploy token with read_registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_registry: true, write_registry: false) }
 
-      it_behaves_like 'package access with repository disabled'
+        it { is_expected.to be_allowed(:read_container_image) }
+        it { is_expected.to be_disallowed(:create_container_image) }
+
+        context 'with registry disabled' do
+          include_context 'registry disabled via project features'
+
+          it { is_expected.to be_disallowed(:read_container_image) }
+          it { is_expected.to be_disallowed(:create_container_image) }
+        end
+      end
+
+      context 'a deploy token with write_registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: true) }
+
+        it { is_expected.to be_disallowed(:read_container_image) }
+        it { is_expected.to be_allowed(:create_container_image) }
+
+        context 'with registry disabled' do
+          include_context 'registry disabled via project features'
+
+          it { is_expected.to be_disallowed(:read_container_image) }
+          it { is_expected.to be_disallowed(:create_container_image) }
+        end
+      end
+
+      context 'a deploy token with no registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: false) }
+
+        it { is_expected.to be_disallowed(:read_container_image) }
+        it { is_expected.to be_disallowed(:create_container_image) }
+      end
+
+      context 'a deploy token with read_package_registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_repository: false, read_registry: false, read_package_registry: true) }
+
+        it { is_expected.to be_allowed(:read_project) }
+        it { is_expected.to be_allowed(:read_package) }
+        it { is_expected.to be_disallowed(:create_package) }
+
+        it_behaves_like 'package access with repository disabled'
+      end
+
+      context 'a deploy token with write_package_registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_repository: false, read_registry: false, write_package_registry: true) }
+
+        it { is_expected.to be_allowed(:create_package) }
+        it { is_expected.to be_allowed(:read_package) }
+        it { is_expected.to be_allowed(:read_project) }
+        it { is_expected.to be_disallowed(:destroy_package) }
+
+        it_behaves_like 'package access with repository disabled'
+      end
     end
 
-    context 'a deploy token with write_package_registry scope' do
-      let(:deploy_token) { create(:deploy_token, write_package_registry: true) }
+    context 'public project' do
+      let(:project) { public_project }
+
+      context 'a deploy token with read_registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_registry: true, write_registry: false) }
 
-      it { is_expected.to be_allowed(:create_package) }
-      it { is_expected.to be_allowed(:read_package) }
-      it { is_expected.to be_allowed(:read_project) }
-      it { is_expected.to be_disallowed(:destroy_package) }
+        it { is_expected.to be_allowed(:read_container_image) }
+        it { is_expected.to be_disallowed(:create_container_image) }
 
-      it_behaves_like 'package access with repository disabled'
+        context 'with registry disabled' do
+          include_context 'registry disabled via project features'
+
+          it { is_expected.to be_disallowed(:read_container_image) }
+          it { is_expected.to be_disallowed(:create_container_image) }
+        end
+
+        context 'with registry private' do
+          include_context 'registry set to private via project features'
+
+          it { is_expected.to be_allowed(:read_container_image) }
+          it { is_expected.to be_disallowed(:create_container_image) }
+        end
+      end
+
+      context 'a deploy token with write_registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: true) }
+
+        it { is_expected.to be_allowed(:read_container_image) }
+        it { is_expected.to be_allowed(:create_container_image) }
+
+        context 'with registry disabled' do
+          include_context 'registry disabled via project features'
+
+          it { is_expected.to be_disallowed(:read_container_image) }
+          it { is_expected.to be_disallowed(:create_container_image) }
+        end
+
+        context 'with registry private' do
+          include_context 'registry set to private via project features'
+
+          it { is_expected.to be_allowed(:read_container_image) }
+          it { is_expected.to be_allowed(:create_container_image) }
+        end
+      end
+
+      context 'a deploy token with no registry scope' do
+        let(:deploy_token) { create(:deploy_token, read_registry: false, write_registry: false) }
+
+        it { is_expected.to be_disallowed(:read_container_image) }
+        it { is_expected.to be_disallowed(:create_container_image) }
+      end
     end
   end
 

spec/support/shared_contexts/project_features_shared_context.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'repository disabled via project features' do
+  before do
+    project.project_feature.update_columns(
+      # Disable merge_requests and builds as well, since merge_requests and
+      # builds cannot have higher visibility than repository.
+      merge_requests_access_level: ProjectFeature::DISABLED,
+      builds_access_level: ProjectFeature::DISABLED,
+      repository_access_level: ProjectFeature::DISABLED)
+  end
+end
+
+RSpec.shared_context 'registry disabled via project features' do
+  before do
+    project.project_feature.update_columns(
+      container_registry_access_level: ProjectFeature::DISABLED
+    )
+  end
+end
+
+RSpec.shared_context 'registry set to private via project features' do
+  before do
+    project.project_feature.update_columns(
+      container_registry_access_level: ProjectFeature::PRIVATE
+    )
+  end
+end

spec/support/shared_examples/policies/project_policy_shared_examples.rb

@@ -345,16 +345,7 @@ RSpec.shared_examples 'project policies as admin without admin mode' do
 end
 
 RSpec.shared_examples 'package access with repository disabled' do
-  context 'when repository is disabled' do
-    before do
-      project.project_feature.update!(
-        # Disable merge_requests and builds as well, since merge_requests and
-        # builds cannot have higher visibility than repository.
-        merge_requests_access_level: ProjectFeature::DISABLED,
-        builds_access_level: ProjectFeature::DISABLED,
-        repository_access_level: ProjectFeature::DISABLED)
-    end
+  include_context 'repository disabled via project features'
 
-    it { is_expected.to be_allowed(:read_package) }
-  end
+  it { is_expected.to be_allowed(:read_package) }
 end

spec/support/shared_examples/services/container_registry_auth_service_shared_examples.rb

@@ -142,9 +142,9 @@ RSpec.shared_examples 'logs an auth warning' do |requested_actions|
       requested_project_path: project.full_path,
       requested_actions: requested_actions,
       authorized_actions: [],
-      user_id: current_user.id,
-      username: current_user.username
-    }
+      user_id: current_user&.id,
+      username: current_user&.username
+    }.compact
   end
 
   it do