Merge branch '338925-update-rack-attack-for-the-dependency-proxy' into 'master'

Update request auhenticator for the dependency proxy routes

See merge request gitlab-org/gitlab!71532

app/controllers/groups/dependency_proxy/application_controller.rb

@@ -21,8 +21,14 @@ module Groups
         authenticate_with_http_token do |token, _|
           @authentication_result = EMPTY_AUTH_RESULT
 
-          found_user = user_from_token(token)
-          sign_in(found_user) if found_user.is_a?(User)
+          user_or_deploy_token = ::DependencyProxy::AuthTokenService.user_or_deploy_token_from_jwt(token)
+
+          if user_or_deploy_token.is_a?(User)
+            @authentication_result = Gitlab::Auth::Result.new(user_or_deploy_token, nil, :user, [])
+            sign_in(user_or_deploy_token)
+          elsif user_or_deploy_token.is_a?(DeployToken)
+            @authentication_result = Gitlab::Auth::Result.new(user_or_deploy_token, nil, :deploy_token, [])
+          end
         end
 
         request_bearer_token! unless authenticated_user
@@ -39,28 +45,6 @@ module Groups
         response.headers['WWW-Authenticate'] = ::DependencyProxy::Registry.authenticate_header
         render plain: '', status: :unauthorized
       end
-
-      def user_from_token(token)
-        token_payload = ::DependencyProxy::AuthTokenService.decoded_token_payload(token)
-
-        if token_payload['user_id']
-          token_user = User.find(token_payload['user_id'])
-          return unless token_user
-
-          @authentication_result = Gitlab::Auth::Result.new(token_user, nil, :user, [])
-          return token_user
-        elsif token_payload['deploy_token']
-          deploy_token = DeployToken.active.find_by_token(token_payload['deploy_token'])
-          return unless deploy_token
-
-          @authentication_result = Gitlab::Auth::Result.new(deploy_token, nil, :deploy_token, [])
-          return deploy_token
-        end
-
-        nil
-      rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::ImmatureSignature
-        nil
-      end
     end
   end
 end

app/services/dependency_proxy/auth_token_service.rb

@@ -12,10 +12,16 @@ module DependencyProxy
       JSONWebToken::HMACToken.decode(token, ::Auth::DependencyProxyAuthenticationService.secret).first
     end
 
-    class << self
-      def decoded_token_payload(token)
-        self.new(token).execute
+    def self.user_or_deploy_token_from_jwt(raw_jwt)
+      token_payload = self.new(raw_jwt).execute
+
+      if token_payload['user_id']
+        User.find(token_payload['user_id'])
+      elsif token_payload['deploy_token']
+        DeployToken.active.find_by_token(token_payload['deploy_token'])
       end
+    rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::ImmatureSignature
+      nil
     end
   end
 end

lib/gitlab/auth/request_authenticator.rb

@@ -30,6 +30,7 @@ module Gitlab
       end
 
       def find_sessionless_user(request_format)
+        find_user_from_dependency_proxy_token ||
           find_user_from_web_access_token(request_format, scopes: [:api, :read_api]) ||
           find_user_from_feed_token(request_format) ||
           find_user_from_static_object_token(request_format) ||
@@ -82,6 +83,28 @@ module Gitlab
           basic_auth_personal_access_token: api_request? || git_request?
         }
       end
+
+      def find_user_from_dependency_proxy_token
+        return unless dependency_proxy_request?
+
+        token, _ = ActionController::HttpAuthentication::Token.token_and_options(current_request)
+
+        return unless token
+
+        user_or_deploy_token = ::DependencyProxy::AuthTokenService.user_or_deploy_token_from_jwt(token)
+
+        # Do not return deploy tokens
+        # See https://gitlab.com/gitlab-org/gitlab/-/issues/342481
+        return unless user_or_deploy_token.is_a?(::User)
+
+        user_or_deploy_token
+      rescue ActiveRecord::RecordNotFound
+        nil # invalid id used return no user
+      end
+
+      def dependency_proxy_request?
+        Gitlab::PathRegex.dependency_proxy_route_regex.match?(current_request.path)
+      end
     end
   end
 end

lib/gitlab/path_regex.rb

@@ -262,6 +262,10 @@ module Gitlab
       @container_image_blob_sha_regex ||= %r{[\w+.-]+:?\w+}.freeze
     end
 
+    def dependency_proxy_route_regex
+      @dependency_proxy_route_regex ||= %r{\A/v2/#{full_namespace_route_regex}/dependency_proxy/containers/#{container_image_regex}/(manifests|blobs)/#{container_image_blob_sha_regex}\z}
+    end
+
     private
 
     def personal_snippet_path_regex

spec/lib/gitlab/auth/request_authenticator_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Auth::RequestAuthenticator do
+  include DependencyProxyHelpers
+
   let(:env) do
     {
       'rack.input' => '',
@@ -15,8 +17,8 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
   subject { described_class.new(request) }
 
   describe '#user' do
-    let!(:sessionless_user) { build(:user) }
-    let!(:session_user) { build(:user) }
+    let_it_be(:sessionless_user) { build(:user) }
+    let_it_be(:session_user) { build(:user) }
 
     it 'returns sessionless user first' do
       allow_any_instance_of(described_class).to receive(:find_sessionless_user).and_return(sessionless_user)
@@ -41,15 +43,25 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
   end
 
   describe '#find_sessionless_user' do
-    let!(:access_token_user) { build(:user) }
-    let!(:feed_token_user) { build(:user) }
-    let!(:static_object_token_user) { build(:user) }
-    let!(:job_token_user) { build(:user) }
-    let!(:lfs_token_user) { build(:user) }
-    let!(:basic_auth_access_token_user) { build(:user) }
-    let!(:basic_auth_password_user) { build(:user) }
-
-    it 'returns access_token user first' do
+    let_it_be(:dependency_proxy_user) { build(:user) }
+    let_it_be(:access_token_user) { build(:user) }
+    let_it_be(:feed_token_user) { build(:user) }
+    let_it_be(:static_object_token_user) { build(:user) }
+    let_it_be(:job_token_user) { build(:user) }
+    let_it_be(:lfs_token_user) { build(:user) }
+    let_it_be(:basic_auth_access_token_user) { build(:user) }
+    let_it_be(:basic_auth_password_user) { build(:user) }
+
+    it 'returns dependency_proxy user first' do
+      allow_any_instance_of(described_class).to receive(:find_user_from_dependency_proxy_token)
+                                                  .and_return(dependency_proxy_user)
+
+      allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token).and_return(access_token_user)
+
+      expect(subject.find_sessionless_user(:api)).to eq dependency_proxy_user
+    end
+
+    it 'returns access_token user if no dependency_proxy user found' do
       allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token)
                                                   .with(anything, scopes: [:api, :read_api])
                                                   .and_return(access_token_user)
@@ -154,6 +166,75 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
 
       expect(subject.find_sessionless_user(:api)).to be_blank
     end
+
+    context 'dependency proxy' do
+      let_it_be(:dependency_proxy_user) { create(:user) }
+
+      let(:token) { build_jwt(dependency_proxy_user).encoded }
+      let(:authenticator) { described_class.new(request) }
+
+      subject { authenticator.find_sessionless_user(:api) }
+
+      before do
+        env['SCRIPT_NAME'] = accessed_path
+        env['HTTP_AUTHORIZATION'] = "Bearer #{token}"
+      end
+
+      shared_examples 'identifying dependency proxy urls properly with' do |user_type|
+        context 'with pulling a manifest' do
+          let(:accessed_path) { '/v2/group1/dependency_proxy/containers/alpine/manifests/latest' }
+
+          it { is_expected.to eq(dependency_proxy_user) } if user_type == :user
+          it { is_expected.to eq(nil) } if user_type == :no_user
+        end
+
+        context 'with pulling a blob' do
+          let(:accessed_path) { '/v2/group1/dependency_proxy/containers/alpine/blobs/sha256:a0d0a0d46f8b52473982a3c466318f479767577551a53ffc9074c9fa7035982e' }
+
+          it { is_expected.to eq(dependency_proxy_user) } if user_type == :user
+          it { is_expected.to eq(nil) } if user_type == :no_user
+        end
+
+        context 'with any other path' do
+          let(:accessed_path) { '/foo/bar' }
+
+          it { is_expected.to eq(nil) }
+        end
+      end
+
+      context 'with a user' do
+        it_behaves_like 'identifying dependency proxy urls properly with', :user
+
+        context 'with an invalid id' do
+          let(:token) { build_jwt { |jwt| jwt['user_id'] = 'this_is_not_a_user' } }
+
+          it_behaves_like 'identifying dependency proxy urls properly with', :no_user
+        end
+      end
+
+      context 'with a deploy token' do
+        let_it_be(:dependency_proxy_user) { create(:deploy_token) }
+
+        it_behaves_like 'identifying dependency proxy urls properly with', :no_user
+      end
+
+      context 'with no jwt token' do
+        let(:token) { nil }
+
+        it_behaves_like 'identifying dependency proxy urls properly with', :no_user
+      end
+
+      context 'with an expired jwt token' do
+        let(:token) { build_jwt(dependency_proxy_user).encoded }
+        let(:accessed_path) { '/v2/group1/dependency_proxy/containers/alpine/manifests/latest' }
+
+        it 'returns nil' do
+          travel_to(Time.zone.now + Auth::DependencyProxyAuthenticationService.token_expire_at + 1.minute) do
+            expect(subject).to eq(nil)
+          end
+        end
+      end
+    end
   end
 
   describe '#find_personal_access_token_from_http_basic_auth' do
@@ -201,8 +282,8 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
   end
 
   describe '#find_user_from_job_token' do
-    let!(:user) { build(:user) }
-    let!(:job) { build(:ci_build, user: user, status: :running) }
+    let_it_be(:user) { build(:user) }
+    let_it_be(:job) { build(:ci_build, user: user, status: :running) }
 
     before do
       env[Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER] = 'token'
@@ -239,7 +320,7 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
   end
 
   describe '#runner' do
-    let!(:runner) { build(:ci_runner) }
+    let_it_be(:runner) { build(:ci_runner) }
 
     it 'returns the runner using #find_runner_from_token' do
       expect_any_instance_of(described_class)

spec/lib/gitlab/path_regex_spec.rb

@@ -561,4 +561,25 @@ RSpec.describe Gitlab::PathRegex do
       expect(subject.match('sha256:asdf1234%2f')[0]).to eq('sha256:asdf1234')
     end
   end
+
+  describe '.dependency_proxy_route_regex' do
+    subject { described_class.dependency_proxy_route_regex }
+
+    it { is_expected.to match('/v2/group1/dependency_proxy/containers/alpine/manifests/latest') }
+    it { is_expected.to match('/v2/group1/dependency_proxy/containers/alpine/blobs/sha256:14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab') }
+
+    it { is_expected.not_to match('') }
+    it { is_expected.not_to match('/v3/group1/dependency_proxy/containers/alpine/manifests/latest') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/container/alpine/manifests/latest') }
+    it { is_expected.not_to match('/v2/group1/dependency_prox/containers/alpine/manifests/latest') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/containers/alpine/manifest/latest') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/containers/alpine/manifest/la%2Ftest') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/containers/alpine/manifest/latest/../one') }
+    it { is_expected.not_to match('/v3/group1/dependency_proxy/containers/alpine/blobs/sha256:14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/container/alpine/blobs/sha256:14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab') }
+    it { is_expected.not_to match('/v2/group1/dependency_prox/containers/alpine/blobs/sha256:14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/containers/alpine/blob/sha256:14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/containers/alpine/blob/sha256:F14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab/../latest') }
+    it { is_expected.not_to match('/v2/group1/dependency_proxy/containers/alpine/blob/sha256:F14119a10abf4669e8cdbdff324a9f9605d99697215a0d21c360fe8dfa8471bab/latest') }
+  end
 end

spec/requests/rack_attack_global_spec.rb

@@ -483,6 +483,67 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
     end
   end
 
+  describe 'dependency proxy' do
+    include DependencyProxyHelpers
+
+    let_it_be_with_reload(:group) { create(:group) }
+    let_it_be_with_reload(:other_group) { create(:group) }
+    let_it_be(:user) { create(:user) }
+    let_it_be(:other_user) { create(:user) }
+
+    let(:throttle_setting_prefix) { 'throttle_authenticated_web' }
+    let(:jwt_token) { build_jwt(user) }
+    let(:other_jwt_token) { build_jwt(other_user) }
+    let(:request_args) { [path, headers: jwt_token_authorization_headers(jwt_token)] }
+    let(:other_user_request_args) { [other_path, headers: jwt_token_authorization_headers(other_jwt_token)] }
+
+    before do
+      group.add_owner(user)
+      group.create_dependency_proxy_setting!(enabled: true)
+      other_group.add_owner(other_user)
+      other_group.create_dependency_proxy_setting!(enabled: true)
+
+      allow(Gitlab.config.dependency_proxy)
+        .to receive(:enabled).and_return(true)
+      token_response = { status: :success, token: 'abcd1234' }
+      allow_next_instance_of(DependencyProxy::RequestTokenService) do |instance|
+        allow(instance).to receive(:execute).and_return(token_response)
+      end
+    end
+
+    context 'getting a manifest' do
+      let_it_be(:manifest) { create(:dependency_proxy_manifest) }
+
+      let(:path) { "/v2/#{group.path}/dependency_proxy/containers/alpine/manifests/latest" }
+      let(:other_path) { "/v2/#{other_group.path}/dependency_proxy/containers/alpine/manifests/latest" }
+      let(:pull_response) { { status: :success, manifest: manifest, from_cache: false } }
+
+      before do
+        allow_next_instance_of(DependencyProxy::FindOrCreateManifestService) do |instance|
+          allow(instance).to receive(:execute).and_return(pull_response)
+        end
+      end
+
+      it_behaves_like 'rate-limited token-authenticated requests'
+    end
+
+    context 'getting a blob' do
+      let_it_be(:blob) { create(:dependency_proxy_blob) }
+
+      let(:path) { "/v2/#{group.path}/dependency_proxy/containers/alpine/blobs/sha256:a0d0a0d46f8b52473982a3c466318f479767577551a53ffc9074c9fa7035982e" }
+      let(:other_path) { "/v2/#{other_group.path}/dependency_proxy/containers/alpine/blobs/sha256:a0d0a0d46f8b52473982a3c466318f479767577551a53ffc9074c9fa7035982e" }
+      let(:blob_response) { { status: :success, blob: blob, from_cache: false } }
+
+      before do
+        allow_next_instance_of(DependencyProxy::FindOrCreateBlobService) do |instance|
+          allow(instance).to receive(:execute).and_return(blob_response)
+        end
+      end
+
+      it_behaves_like 'rate-limited token-authenticated requests'
+    end
+  end
+
   describe 'authenticated git lfs requests', :api do
     let_it_be(:project) { create(:project, :internal) }
     let_it_be(:user) { create(:user) }

spec/services/dependency_proxy/auth_token_service_spec.rb

@@ -4,47 +4,72 @@ require 'spec_helper'
 RSpec.describe DependencyProxy::AuthTokenService do
   include DependencyProxyHelpers
 
-  describe '.decoded_token_payload' do
   let_it_be(:user) { create(:user) }
-    let_it_be(:token) { build_jwt(user) }
+  let_it_be(:deploy_token) { create(:deploy_token) }
 
-    subject { described_class.decoded_token_payload(token.encoded) }
+  describe '.user_or_deploy_token_from_jwt' do
+    subject { described_class.user_or_deploy_token_from_jwt(token.encoded) }
 
-    it 'returns the user' do
-      result = subject
+    shared_examples 'handling token errors' do
+      context 'with a decoding error' do
+        before do
+          allow(JWT).to receive(:decode).and_raise(JWT::DecodeError)
+        end
 
-      expect(result['user_id']).to eq(user.id)
-      expect(result['deploy_token']).to be_nil
+        it { is_expected.to eq(nil) }
       end
 
-    context 'with a deploy token' do
-      let_it_be(:deploy_token) { create(:deploy_token) }
-      let_it_be(:token) { build_jwt(deploy_token) }
+      context 'with an immature signature error' do
+        before do
+          allow(JWT).to receive(:decode).and_raise(JWT::ImmatureSignature)
+        end
 
-      it 'returns the deploy token' do
-        result = subject
+        it { is_expected.to eq(nil) }
+      end
 
-        expect(result['deploy_token']).to eq(deploy_token.token)
-        expect(result['user_id']).to be_nil
+      context 'with an expired signature error' do
+        it 'returns nil' do
+          travel_to(Time.zone.now + Auth::DependencyProxyAuthenticationService.token_expire_at + 1.minute) do
+            expect(subject).to eq(nil)
+          end
+        end
       end
     end
 
-    it 'raises an error if the token is expired' do
-      travel_to(Time.zone.now + Auth::DependencyProxyAuthenticationService.token_expire_at + 1.minute) do
-        expect { subject }.to raise_error(JWT::ExpiredSignature)
+    context 'with a user' do
+      let_it_be(:token) { build_jwt(user) }
+
+      it { is_expected.to eq(user) }
+
+      context 'with an invalid user id' do
+        let_it_be(:token) { build_jwt { |jwt| jwt['user_id'] = 'this_is_not_a_user_id' } }
+
+        it 'raises an not found error' do
+          expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
         end
       end
 
-    it 'raises an error if decoding fails' do
-      allow(JWT).to receive(:decode).and_raise(JWT::DecodeError)
+      it_behaves_like 'handling token errors'
+    end
+
+    context 'with a deploy token' do
+      let_it_be(:token) { build_jwt(deploy_token) }
+
+      it { is_expected.to eq(deploy_token) }
+
+      context 'with an invalid token' do
+        let_it_be(:token) { build_jwt { |jwt| jwt['deploy_token'] = 'this_is_not_a_token' } }
 
-      expect { subject }.to raise_error(JWT::DecodeError)
+        it { is_expected.to eq(nil) }
       end
 
-    it 'raises an error if signature is immature' do
-      allow(JWT).to receive(:decode).and_raise(JWT::ImmatureSignature)
+      it_behaves_like 'handling token errors'
+    end
+
+    context 'with an empty token payload' do
+      let_it_be(:token) { build_jwt(nil) }
 
-      expect { subject }.to raise_error(JWT::ImmatureSignature)
+      it { is_expected.to eq(nil) }
     end
   end
 end

spec/support/helpers/dependency_proxy_helpers.rb

@@ -34,11 +34,19 @@ module DependencyProxyHelpers
 
   def build_jwt(user = nil, expire_time: nil)
     JSONWebToken::HMACToken.new(::Auth::DependencyProxyAuthenticationService.secret).tap do |jwt|
+      if block_given?
+        yield(jwt)
+      else
         jwt['user_id'] = user.id if user.is_a?(User)
         jwt['deploy_token'] = user.token if user.is_a?(DeployToken)
         jwt.expire_time = expire_time || jwt.issued_at + 1.minute
       end
     end
+  end
+
+  def jwt_token_authorization_headers(jwt)
+    { 'AUTHORIZATION' => "Bearer #{jwt.encoded}" }
+  end
 
   private