Merge branch 'ashmckenzie/8114-geo-push-ssh-lfs-http-auth-bug' into 'master'

Geo: Fix push to secondary over SSH for LFS

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

app/models/project.rb

@@ -1141,6 +1141,11 @@ class Project < ActiveRecord::Base
     "#{web_url}.git"
   end
 
+  # Is overriden in EE
+  def lfs_http_url_to_repo(_)
+    http_url_to_repo
+  end
+
   def forked?
     fork_network && fork_network.root_project != self
   end

ee/app/models/ee/project.rb

@@ -10,6 +10,9 @@ module EE
     extend ::Gitlab::Utils::Override
     extend ::Gitlab::Cache::RequestCache
     include ::Gitlab::Utils::StrongMemoize
+    include ::EE::GitlabRoutingHelper
+
+    GIT_LFS_DOWNLOAD_OPERATION = 'download'.freeze
 
     prepended do
       include Elastic::ProjectsSearch
@@ -488,6 +491,14 @@ module EE
       webide_pipelines.running_or_pending.for_user(user)
     end
 
+    override :lfs_http_url_to_repo
+    def lfs_http_url_to_repo(operation)
+      return super unless ::Gitlab::Geo.secondary_with_primary?
+      return super if operation == GIT_LFS_DOWNLOAD_OPERATION # download always comes from secondary
+
+      geo_primary_http_url_to_repo(self)
+    end
+
     private
 
     def set_override_pull_mirror_available

ee/changelogs/unreleased/ashmckenzie-8114-geo-push-ssh-lfs-http-auth-bug.yml

+---
+title: 'Geo: Fix push to secondary over SSH for LFS'
+merge_request: 8044
+author:
+type: fixed

ee/spec/models/project_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Project do
   include ExternalAuthorizationServiceHelpers
+  include ::EE::GeoHelpers
   using RSpec::Parameterized::TableSyntax
 
   describe 'associations' do
@@ -1366,6 +1367,78 @@ describe Project do
     end
   end
 
+  describe '#lfs_http_url_to_repo' do
+    let(:project) { create(:project) }
+    let(:project_path) { "#{Gitlab::Routing.url_helpers.project_path(project)}.git" }
+
+    let(:primary_base_host) { 'primary.geo' }
+    let(:primary_base_url) { "http://#{primary_base_host}" }
+    let(:primary_url) { "#{primary_base_url}#{project_path}" }
+
+    context 'with a Geo setup that is a primary' do
+      let(:primary_node) { create(:geo_node, url: primary_base_url) }
+
+      before do
+        stub_current_geo_node(primary_node)
+        stub_default_url_options(primary_base_host)
+      end
+
+      context 'for an upload operation' do
+        it 'returns the project HTTP URL for the primary' do
+          expect(project.lfs_http_url_to_repo('upload')).to eq(primary_url)
+        end
+      end
+    end
+
+    context 'with a Geo setup that is a secondary' do
+      let(:secondary_base_host) { 'secondary.geo' }
+      let(:secondary_base_url) { "http://#{secondary_base_host}" }
+      let(:secondary_node) { create(:geo_node, url: secondary_base_url) }
+      let(:secondary_url) { "#{secondary_base_url}#{project_path}"  }
+
+      before do
+        stub_current_geo_node(secondary_node)
+        stub_default_url_options(current_rails_hostname)
+      end
+
+      context 'and has a primary' do
+        let(:primary_node) { create(:geo_node, url: primary_base_url) }
+
+        context 'for an upload operation' do
+          let(:current_rails_hostname) { primary_base_host }
+
+          it 'returns the project HTTP URL for the primary' do
+            expect(project.lfs_http_url_to_repo('upload')).to eq(primary_url)
+          end
+        end
+
+        context 'for a download operation' do
+          let(:current_rails_hostname) { secondary_base_host }
+
+          it 'returns the project HTTP URL for the secondary' do
+            expect(project.lfs_http_url_to_repo('download')).to eq(secondary_url)
+          end
+        end
+      end
+
+      context 'without a primary' do
+        let(:current_rails_hostname) { secondary_base_host }
+
+        it 'returns the project HTTP URL for the secondary' do
+          expect(project.lfs_http_url_to_repo('operation_that_doesnt_matter')).to eq(secondary_url)
+        end
+      end
+    end
+
+    context 'without a Geo setup' do
+      it 'returns the project HTTP URL for the main node' do
+        project_url = "#{Gitlab::Routing.url_helpers.project_url(project)}.git"
+
+        expect(project.lfs_http_url_to_repo('operation_that_doesnt_matter')).to eq(project_url)
+      end
+    end
+  end
+
   describe '#add_import_job' do
     let(:project) { create(:project) }
 
@@ -1469,4 +1542,13 @@ describe Project do
       subject
     end
   end
+
+  # Despite stubbing the current node as the primary or secondary, the
+  # behaviour for EE::Project#lfs_http_url_to_repo() is to call
+  # Project#lfs_http_url_to_repo() which does not have a Geo context.
+  def stub_default_url_options(host)
+    allow(Rails.application.routes)
+      .to receive(:default_url_options)
+      .and_return(host: host)
+  end
 end

lib/api/internal.rb

@@ -122,7 +122,7 @@ module API
         {
           username: token_handler.actor_name,
           lfs_token: token_handler.token,
-          repository_http_path: project.http_url_to_repo
+          repository_http_path: project.lfs_http_url_to_repo(params[:operation])
         }
       end
       # rubocop: enable CodeReuse/ActiveRecord

lib/gitlab/auth.rb

@@ -201,7 +201,7 @@ module Gitlab
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      def lfs_token_check(login, password, project)
+      def lfs_token_check(login, encoded_token, project)
         deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/)
 
         actor =
@@ -224,7 +224,7 @@ module Gitlab
             read_authentication_abilities
           end
 
-        if Devise.secure_compare(token_handler.token, password)
+        if token_handler.token_valid?(encoded_token)
           Gitlab::Auth::Result.new(actor, nil, token_handler.type, authentication_abilities)
         end
       end

lib/gitlab/lfs_token.rb

@@ -2,10 +2,21 @@
 
 module Gitlab
   class LfsToken
-    attr_accessor :actor
+    module LfsTokenHelper
+      def user?
+        actor.is_a?(User)
+      end
+
+      def actor_name
+        user? ? actor.username : "lfs+deploy-key-#{actor.id}"
+      end
+    end
+
+    include LfsTokenHelper
 
-    TOKEN_LENGTH = 50
-    EXPIRY_TIME = 1800
+    DEFAULT_EXPIRE_TIME = 1800
+
+    attr_accessor :actor
 
     def initialize(actor)
       @actor =
@@ -19,36 +30,108 @@ module Gitlab
         end
     end
 
-    def token
-      Gitlab::Redis::SharedState.with do |redis|
-        token = redis.get(redis_shared_state_key)
-        token ||= Devise.friendly_token(TOKEN_LENGTH)
-        redis.set(redis_shared_state_key, token, ex: EXPIRY_TIME)
+    def token(expire_time: DEFAULT_EXPIRE_TIME)
+      HMACToken.new(actor).token(expire_time)
+    end
 
-        token
-      end
+    def token_valid?(token_to_check)
+      HMACToken.new(actor).token_valid?(token_to_check) ||
+        LegacyRedisDeviseToken.new(actor).token_valid?(token_to_check)
     end
 
     def deploy_key_pushable?(project)
       actor.is_a?(DeployKey) && actor.can_push_to?(project)
     end
 
-    def user?
-      actor.is_a?(User)
-    end
-
     def type
-      actor.is_a?(User) ? :lfs_token : :lfs_deploy_token
+      user? ? :lfs_token : :lfs_deploy_token
     end
 
-    def actor_name
-      actor.is_a?(User) ? actor.username : "lfs+deploy-key-#{actor.id}"
+    private  # rubocop:disable Lint/UselessAccessModifier
+
+    class HMACToken
+      include LfsTokenHelper
+
+      def initialize(actor)
+        @actor = actor
+      end
+
+      def token(expire_time)
+        hmac_token = JSONWebToken::HMACToken.new(secret)
+        hmac_token.expire_time = Time.now + expire_time
+        hmac_token[:data] = { actor: actor_name }
+        hmac_token.encoded
+      end
+
+      def token_valid?(token_to_check)
+        decoded_token = JSONWebToken::HMACToken.decode(token_to_check, secret).first
+        decoded_token.dig('data', 'actor') == actor_name
+      rescue JWT::DecodeError
+        false
+      end
+
+      private
+
+      attr_reader :actor
+
+      def secret
+        salt + key
+      end
+
+      def salt
+        case actor
+        when DeployKey, Key
+          actor.fingerprint.delete(':').first(16)
+        when User
+          # Take the last 16 characters as they're more unique than the first 16
+          actor.id.to_s + actor.encrypted_password.last(16)
+        end
+      end
+
+      def key
+        # Take 16 characters of attr_encrypted_db_key_base, as that's what the
+        # cipher needs exactly
+        Settings.attr_encrypted_db_key_base.first(16)
+      end
     end
 
-    private
+    # TODO: LegacyRedisDeviseToken and references need to be removed after
+    # next released milestone
+    #
+    class LegacyRedisDeviseToken
+      TOKEN_LENGTH = 50
+      DEFAULT_EXPIRY_TIME = 1800 * 1000  # 30 mins
+
+      def initialize(actor)
+        @actor = actor
+      end
+
+      def token_valid?(token_to_check)
+        Devise.secure_compare(stored_token, token_to_check)
+      end
+
+      def stored_token
+        Gitlab::Redis::SharedState.with { |redis| redis.get(state_key) }
+      end
+
+      # This method exists purely to facilitate legacy testing to ensure the
+      # same redis key is used.
+      #
+      def store_new_token(expiry_time_in_ms = DEFAULT_EXPIRY_TIME)
+        Gitlab::Redis::SharedState.with do |redis|
+          new_token = Devise.friendly_token(TOKEN_LENGTH)
+          redis.set(state_key, new_token, px: expiry_time_in_ms)
+          new_token
+        end
+      end
+
+      private
 
-    def redis_shared_state_key
-      "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
+      attr_reader :actor
+
+      def state_key
+        "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}"
+      end
     end
   end
 end

spec/lib/gitlab/lfs_token_spec.rb

+# frozen_string_literal: true
+
 require 'spec_helper'
 
-describe Gitlab::LfsToken do
+describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
   describe '#token' do
     shared_examples 'an LFS token generator' do
-      it 'returns a randomly generated token' do
-        token = handler.token
+      it 'returns a computed token' do
+        expect(Settings).to receive(:attr_encrypted_db_key_base).and_return('fbnbv6hdjweo53qka7kza8v8swxc413c05pb51qgtfte0bygh1p2e508468hfsn5ntmjcyiz7h1d92ashpet5pkdyejg7g8or3yryhuso4h8o5c73h429d9c3r6bjnet').twice
+
+        token = lfs_token.token
 
         expect(token).not_to be_nil
         expect(token).to be_a String
-        expect(token.length).to eq 50
+        expect(described_class.new(actor).token_valid?(token)).to be_truthy
       end
+    end
+
+    context 'when the actor is a user' do
+      let(:actor) { create(:user, username: 'test_user_lfs_1') }
+      let(:lfs_token) { described_class.new(actor) }
+
+      before do
+        allow(actor).to receive(:encrypted_password).and_return('$2a$04$ETfzVS5spE9Hexn9wh6NUenCHG1LyZ2YdciOYxieV1WLSa8DHqOFO')
+      end
+
+      it_behaves_like 'an LFS token generator'
 
-      it 'returns the correct token based on the key' do
-        token = handler.token
+      it 'returns the correct username' do
+        expect(lfs_token.actor_name).to eq(actor.username)
+      end
 
-        expect(handler.token).to eq(token)
+      it 'returns the correct token type' do
+        expect(lfs_token.type).to eq(:lfs_token)
       end
     end
 
-    context 'when the actor is a user' do
-      let(:actor) { create(:user) }
-      let(:handler) { described_class.new(actor) }
+    context 'when the actor is a key' do
+      let(:user) { create(:user, username: 'test_user_lfs_2') }
+      let(:actor) { create(:key, user: user) }
+      let(:lfs_token) { described_class.new(actor) }
+
+      before do
+        allow(user).to receive(:encrypted_password).and_return('$2a$04$C1GAMKsOKouEbhKy2JQoe./3LwOfQAZc.hC8zW2u/wt8xgukvnlV.')
+      end
 
       it_behaves_like 'an LFS token generator'
 
       it 'returns the correct username' do
-        expect(handler.actor_name).to eq(actor.username)
+        expect(lfs_token.actor_name).to eq(user.username)
       end
 
       it 'returns the correct token type' do
-        expect(handler.type).to eq(:lfs_token)
+        expect(lfs_token.type).to eq(:lfs_token)
       end
     end
 
     context 'when the actor is a deploy key' do
+      let(:actor_id) { 1 }
       let(:actor) { create(:deploy_key) }
-      let(:handler) { described_class.new(actor) }
+      let(:project) { create(:project) }
+      let(:lfs_token) { described_class.new(actor) }
+
+      before do
+        allow(actor).to receive(:id).and_return(actor_id)
+      end
 
       it_behaves_like 'an LFS token generator'
 
       it 'returns the correct username' do
-        expect(handler.actor_name).to eq("lfs+deploy-key-#{actor.id}")
+        expect(lfs_token.actor_name).to eq("lfs+deploy-key-#{actor_id}")
       end
 
       it 'returns the correct token type' do
-        expect(handler.type).to eq(:lfs_deploy_token)
+        expect(lfs_token.type).to eq(:lfs_deploy_token)
+      end
+    end
+
+    context 'when the actor is invalid' do
+      it 'raises an exception' do
+        expect { described_class.new('invalid') }.to raise_error('Bad Actor')
+      end
+    end
+  end
+
+  describe '#token_valid?' do
+    let(:actor) { create(:user, username: 'test_user_lfs_1') }
+    let(:lfs_token) { described_class.new(actor) }
+
+    before do
+      allow(actor).to receive(:encrypted_password).and_return('$2a$04$ETfzVS5spE9Hexn9wh6NUenCHG1LyZ2YdciOYxieV1WLSa8DHqOFO')
+    end
+
+    context 'for an HMAC token' do
+      before do
+        # We're not interested in testing LegacyRedisDeviseToken here
+        allow(Gitlab::LfsToken::LegacyRedisDeviseToken).to receive_message_chain(:new, :token_valid?).and_return(false)
+      end
+
+      context 'where the token is invalid' do
+        context "because it's junk" do
+          it 'returns false' do
+            expect(lfs_token.token_valid?('junk')).to be_falsey
+          end
+        end
+
+        context "because it's been fiddled with" do
+          it 'returns false' do
+            fiddled_token = lfs_token.token.tap { |token| token[0] = 'E' }
+            expect(lfs_token.token_valid?(fiddled_token)).to be_falsey
+          end
+        end
+
+        context "because it was generated with a different secret" do
+          it 'returns false' do
+            different_actor = create(:user, username: 'test_user_lfs_2')
+            different_secret_token = described_class.new(different_actor).token
+            expect(lfs_token.token_valid?(different_secret_token)).to be_falsey
+          end
+        end
+
+        context "because it's expired" do
+          it 'returns false' do
+            expired_token = lfs_token.token
+            # Needs to be at least 1860 seconds, because the default expiry is
+            # 1800 seconds with an additional 60 second leeway.
+            Timecop.freeze(Time.now + 1865) do
+              expect(lfs_token.token_valid?(expired_token)).to be_falsey
+            end
+          end
+        end
+      end
+
+      context 'where the token is valid' do
+        it 'returns true' do
+          expect(lfs_token.token_valid?(lfs_token.token)).to be_truthy
+        end
+      end
+    end
+
+    context 'for a LegacyRedisDevise token' do
+      before do
+        # We're not interested in testing HMACToken here
+        allow_any_instance_of(Gitlab::LfsToken::HMACToken).to receive(:token_valid?).and_return(false)
+      end
+
+      context 'where the token is invalid' do
+        context "because it's junk" do
+          it 'returns false' do
+            expect(lfs_token.token_valid?('junk')).to be_falsey
+          end
+        end
+
+        context "because it's been fiddled with" do
+          it 'returns false' do
+            generated_token = Gitlab::LfsToken::LegacyRedisDeviseToken.new(actor).store_new_token
+            fiddled_token = generated_token.tap { |token| token[0] = 'E' }
+            expect(lfs_token.token_valid?(fiddled_token)).to be_falsey
+          end
+        end
+
+        context "because it was generated with a different secret" do
+          it 'returns false' do
+            different_actor = create(:user, username: 'test_user_lfs_2')
+            different_secret_token = described_class.new(different_actor).token
+            expect(lfs_token.token_valid?(different_secret_token)).to be_falsey
+          end
+        end
+
+        context "because it's expired" do
+          it 'returns false' do
+            generated_token = Gitlab::LfsToken::LegacyRedisDeviseToken.new(actor).store_new_token(1)
+            # We need a real sleep here because we need to wait for redis to expire the key.
+            sleep(0.01)
+            expect(lfs_token.token_valid?(generated_token)).to be_falsey
+          end
+        end
+      end
+
+      context 'where the token is valid' do
+        it 'returns true' do
+          generated_token = Gitlab::LfsToken::LegacyRedisDeviseToken.new(actor).store_new_token
+          expect(lfs_token.token_valid?(generated_token)).to be_truthy
+        end
+      end
+    end
+  end
+
+  describe '#deploy_key_pushable?' do
+    let(:lfs_token) { described_class.new(actor) }
+
+    context 'when actor is not a DeployKey' do
+      let(:actor) { create(:user) }
+      let(:project) { create(:project) }
+
+      it 'returns false' do
+        expect(lfs_token.deploy_key_pushable?(project)).to be_falsey
+      end
+    end
+
+    context 'when actor is a DeployKey' do
+      let(:deploy_keys_project) { create(:deploy_keys_project, can_push: can_push) }
+      let(:project) { deploy_keys_project.project }
+      let(:actor) { deploy_keys_project.deploy_key }
+
+      context 'but the DeployKey cannot push to the project' do
+        let(:can_push) { false }
+
+        it 'returns false' do
+          expect(lfs_token.deploy_key_pushable?(project)).to be_falsey
+        end
+      end
+
+      context 'and the DeployKey can push to the project' do
+        let(:can_push) { true }
+
+        it 'returns true' do
+          expect(lfs_token.deploy_key_pushable?(project)).to be_truthy
+        end
+      end
+    end
+  end
+
+  describe '#type' do
+    let(:lfs_token) { described_class.new(actor) }
+
+    context 'when actor is not a User' do
+      let(:actor) { create(:deploy_key) }
+
+      it 'returns false' do
+        expect(lfs_token.type).to eq(:lfs_deploy_token)
+      end
+    end
+
+    context 'when actor is a User' do
+      let(:actor) { create(:user) }
+
+      it 'returns false' do
+        expect(lfs_token.type).to eq(:lfs_token)
       end
     end
   end

spec/models/project_spec.rb

@@ -2991,6 +2991,17 @@ describe Project do
     end
   end
 
+  describe '#lfs_http_url_to_repo' do
+    let(:project) { create(:project) }
+
+    it 'returns the url to the repo without a username' do
+      lfs_http_url_to_repo = project.lfs_http_url_to_repo('operation_that_doesnt_matter')
+
+      expect(lfs_http_url_to_repo).to eq("#{project.web_url}.git")
+      expect(lfs_http_url_to_repo).not_to include('@')
+    end
+  end
+
   describe '#pipeline_status' do
     let(:project) { create(:project, :repository) }
     it 'builds a pipeline status' do

spec/requests/api/internal_spec.rb

@@ -156,9 +156,8 @@ describe API::Internal do
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['username']).to eq(user.username)
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).token)
-
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
+        expect(Gitlab::LfsToken.new(key).token_valid?(json_response['lfs_token'])).to be_truthy
       end
 
       it 'returns the correct information about the user' do
@@ -166,9 +165,8 @@ describe API::Internal do
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['username']).to eq(user.username)
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).token)
-
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
+        expect(Gitlab::LfsToken.new(user).token_valid?(json_response['lfs_token'])).to be_truthy
       end
 
       it 'returns a 404 when no key or user is provided' do
@@ -198,8 +196,8 @@ describe API::Internal do
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).token)
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
+        expect(Gitlab::LfsToken.new(key).token_valid?(json_response['lfs_token'])).to be_truthy
       end
     end
   end