diff options
author | Stan Hu <stanhu@gmail.com> | 2018-12-05 09:52:00 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-12-05 09:52:00 +0300 |
commit | dcc395b4730eb5a1f0fc3314195dcf46a4a8e093 (patch) | |
tree | bc76acb43a09846d3fec2cefea6d229b340f395f | |
parent | cae69a15c4cddde274d08c6d8b6ae815da08e3b7 (diff) | |
parent | 21014c59f883bf8ad11e89725834f20861abca93 (diff) |
Merge branch 'ashmckenzie/8114-geo-push-ssh-lfs-http-auth-bug' into 'master'
Geo: Backport of EE MR Geo: Fix push to secondary over SSH for LFS
See merge request gitlab-org/gitlab-ce!22856
-rw-r--r-- | app/models/project.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/auth.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/lfs_token.rb | 123 | ||||
-rw-r--r-- | spec/lib/gitlab/lfs_token_spec.rb | 222 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 11 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 8 |
6 files changed, 331 insertions, 42 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 1c5c34111b0..6ec323b58c2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1140,6 +1140,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 diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 6eb5f9e2300..7aa02009aa0 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -199,7 +199,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 = @@ -222,7 +222,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 diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb index fa44bd842b2..c09d3ebc7be 100644 --- a/lib/gitlab/lfs_token.rb +++ b/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 diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb index 3a20dad16d0..1ec1ba19e39 100644 --- a/spec/lib/gitlab/lfs_token_spec.rb +++ b/spec/lib/gitlab/lfs_token_spec.rb @@ -1,50 +1,242 @@ +# 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3f99fd12e38..54ad3c858d5 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2710,6 +2710,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 diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 2ebcb787d06..1575de78bb4 100644 --- a/spec/requests/api/internal_spec.rb +++ b/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 |