diff options
author | 🤖 GitLab Bot 🤖 <gitlab-bot@gitlab.com> | 2018-12-05 10:03:28 +0300 |
---|---|---|
committer | 🤖 GitLab Bot 🤖 <gitlab-bot@gitlab.com> | 2018-12-05 10:03:28 +0300 |
commit | 00acef434031b5dc0bf39576a9e83802c7806842 (patch) | |
tree | 2cdc969cd6ee72f65bb7e6ee32841fad246ee2d0 | |
parent | dcc395b4730eb5a1f0fc3314195dcf46a4a8e093 (diff) |
Revert "LfsToken uses JSONWebToken::HMACToken by default"
This reverts commit 22954f220231281360377922b709efb904559949
-rw-r--r-- | lib/gitlab/auth.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/lfs_token.rb | 121 | ||||
-rw-r--r-- | spec/lib/gitlab/lfs_token_spec.rb | 167 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 8 |
4 files changed, 41 insertions, 259 deletions
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 7aa02009aa0..6eb5f9e2300 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, encoded_token, project) + def lfs_token_check(login, password, project) deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/) actor = @@ -222,7 +222,7 @@ module Gitlab read_authentication_abilities end - if token_handler.token_valid?(encoded_token) + if Devise.secure_compare(token_handler.token, password) 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 c09d3ebc7be..05d3096a208 100644 --- a/lib/gitlab/lfs_token.rb +++ b/lib/gitlab/lfs_token.rb @@ -2,22 +2,11 @@ module Gitlab class LfsToken - module LfsTokenHelper - def user? - actor.is_a?(User) - end - - def actor_name - user? ? actor.username : "lfs+deploy-key-#{actor.id}" - end - end - - include LfsTokenHelper - - DEFAULT_EXPIRE_TIME = 1800 - attr_accessor :actor + TOKEN_LENGTH = 50 + EXPIRY_TIME = 1800 + def initialize(actor) @actor = case actor @@ -30,108 +19,36 @@ module Gitlab end end - def token(expire_time: DEFAULT_EXPIRE_TIME) - HMACToken.new(actor).token(expire_time) - 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_valid?(token_to_check) - HMACToken.new(actor).token_valid?(token_to_check) || - LegacyRedisDeviseToken.new(actor).token_valid?(token_to_check) + token + end 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 user? ? :lfs_token : :lfs_deploy_token end - 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 + def actor_name + actor.is_a?(User) ? actor.username : "lfs+deploy-key-#{actor.id}" end - # 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 + private - attr_reader :actor - - def state_key - "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" - end + def redis_shared_state_key + "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor end end end diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb index 1ec1ba19e39..77ee30264bf 100644 --- a/spec/lib/gitlab/lfs_token_spec.rb +++ b/spec/lib/gitlab/lfs_token_spec.rb @@ -1,187 +1,50 @@ -# frozen_string_literal: true - require 'spec_helper' -describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do +describe Gitlab::LfsToken do describe '#token' do shared_examples 'an LFS token generator' do - it 'returns a computed token' do - expect(Settings).to receive(:attr_encrypted_db_key_base).and_return('fbnbv6hdjweo53qka7kza8v8swxc413c05pb51qgtfte0bygh1p2e508468hfsn5ntmjcyiz7h1d92ashpet5pkdyejg7g8or3yryhuso4h8o5c73h429d9c3r6bjnet').twice - - token = lfs_token.token + it 'returns a randomly generated token' do + token = handler.token expect(token).not_to be_nil expect(token).to be_a String - 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') + expect(token.length).to eq 50 end - it_behaves_like 'an LFS token generator' - - it 'returns the correct username' do - expect(lfs_token.actor_name).to eq(actor.username) - end + it 'returns the correct token based on the key' do + token = handler.token - it 'returns the correct token type' do - expect(lfs_token.type).to eq(:lfs_token) + expect(handler.token).to eq(token) end end - 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 + context 'when the actor is a user' do + let(:actor) { create(:user) } + let(:handler) { described_class.new(actor) } it_behaves_like 'an LFS token generator' it 'returns the correct username' do - expect(lfs_token.actor_name).to eq(user.username) + expect(handler.actor_name).to eq(actor.username) end it 'returns the correct token type' do - expect(lfs_token.type).to eq(:lfs_token) + expect(handler.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(:project) { create(:project) } - let(:lfs_token) { described_class.new(actor) } - - before do - allow(actor).to receive(:id).and_return(actor_id) - end + let(:handler) { described_class.new(actor) } it_behaves_like 'an LFS token generator' it 'returns the correct username' do - expect(lfs_token.actor_name).to eq("lfs+deploy-key-#{actor_id}") + expect(handler.actor_name).to eq("lfs+deploy-key-#{actor.id}") end it 'returns the correct token type' do - 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 + expect(handler.type).to eq(:lfs_deploy_token) end end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 1575de78bb4..2ebcb787d06 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -156,8 +156,9 @@ 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 @@ -165,8 +166,9 @@ 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 @@ -196,8 +198,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 |