Merge branch 'security-deploy-token-registry-access' into 'master'

Check for registry permissions on docker login request

Closes #43

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

app/services/auth/container_registry_authentication_service.rb

@@ -3,12 +3,24 @@
 module Auth
   class ContainerRegistryAuthenticationService < BaseService
     AUDIENCE = 'container_registry'
+    REGISTRY_LOGIN_ABILITIES = [
+      :read_container_image,
+      :create_container_image,
+      :destroy_container_image,
+      :update_container_image,
+      :admin_container_image,
+      :build_read_container_image,
+      :build_create_container_image,
+      :build_destroy_container_image
+    ].freeze
 
     def execute(authentication_abilities:)
       @authentication_abilities = authentication_abilities
 
       return error('UNAVAILABLE', status: 404, message: 'registry not enabled') unless registry.enabled
 
+      return error('DENIED', status: 403, message: 'access forbidden') unless has_registry_ability?
+
       unless scopes.any? || current_user || project
         return error('DENIED', status: 403, message: 'access forbidden')
       end
@@ -197,5 +209,11 @@ module Auth
     def has_authentication_ability?(capability)
       @authentication_abilities.to_a.include?(capability)
     end
+
+    def has_registry_ability?
+      @authentication_abilities.any? do |ability|
+        REGISTRY_LOGIN_ABILITIES.include?(ability)
+      end
+    end
   end
 end

changelogs/unreleased/security-deploy-token-registry-access.yml

+---
+title: Update container registry authentication to account for login request when
+  checking permissions
+merge_request:
+author:
+type: security

spec/services/auth/container_registry_authentication_service_spec.rb

@@ -769,6 +769,15 @@ describe Auth::ContainerRegistryAuthenticationService do
     context 'when deploy token has read_registry as a scope' do
       let(:current_user) { create(:deploy_token, projects: [project]) }
 
+      shared_examples 'able to login' do
+        context 'registry provides read_container_image authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [:read_container_image] }
+
+          it_behaves_like 'an authenticated'
+        end
+      end
+
       context 'for public project' do
         let(:project) { create(:project, :public) }
 
@@ -783,6 +792,8 @@ describe Auth::ContainerRegistryAuthenticationService do
 
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
 
       context 'for internal project' do
@@ -799,6 +810,8 @@ describe Auth::ContainerRegistryAuthenticationService do
 
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
 
       context 'for private project' do
@@ -815,18 +828,38 @@ describe Auth::ContainerRegistryAuthenticationService do
 
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'able to login'
       end
     end
 
     context 'when deploy token does not have read_registry scope' do
       let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) }
 
+      shared_examples 'unable to login' do
+        context 'registry provides no container authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [] }
+
+          it_behaves_like 'a forbidden'
+        end
+
+        context 'registry provides inapplicable container authentication_abilities' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [:download_code] }
+
+          it_behaves_like 'a forbidden'
+        end
+      end
+
       context 'for public project' do
         let(:project) { create(:project, :public) }
 
         context 'when pulling' do
           it_behaves_like 'a pullable'
         end
+
+        it_behaves_like 'unable to login'
       end
 
       context 'for internal project' do
@@ -835,6 +868,8 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'when pulling' do
           it_behaves_like 'an inaccessible'
         end
+
+        it_behaves_like 'unable to login'
       end
 
       context 'for private project' do
@@ -843,6 +878,15 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'when pulling' do
           it_behaves_like 'an inaccessible'
         end
+
+        context 'when logging in' do
+          let(:current_params) { {} }
+          let(:authentication_abilities) { [] }
+
+          it_behaves_like 'a forbidden'
+        end
+
+        it_behaves_like 'unable to login'
       end
     end