From 22954f220231281360377922b709efb904559949 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 23 Oct 2018 21:15:56 +1100 Subject: [PATCH] LfsToken uses JSONWebToken::HMACToken by default LfsToken::HMACToken#token_valid?() will be examined and if false, look in redis via LfsToken::LegacyRedisDeviseToken#token_valid?(). --- lib/gitlab/auth.rb | 4 +- lib/gitlab/lfs_token.rb | 121 ++++++++++++++++---- spec/lib/gitlab/lfs_token_spec.rb | 171 ++++++++++++++++++++++++++--- spec/requests/api/internal_spec.rb | 8 +- 4 files changed, 261 insertions(+), 43 deletions(-) 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 05d3096a208..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 - TOKEN_LENGTH = 50 - EXPIRY_TIME = 1800 + def actor_name + user? ? actor.username : "lfs+deploy-key-#{actor.id}" + end + end + + include LfsTokenHelper + + 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 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 redis_shared_state_key - "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor + 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 + + 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..9842ecb0e4d 100644 --- a/spec/lib/gitlab/lfs_token_spec.rb +++ b/spec/lib/gitlab/lfs_token_spec.rb @@ -1,50 +1,187 @@ +# 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 - end - - it 'returns the correct token based on the key' do - token = handler.token - - expect(handler.token).to eq(token) + 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) } - let(:handler) { described_class.new(actor) } + 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 username' do - expect(handler.actor_name).to eq(actor.username) + expect(lfs_token.actor_name).to eq(actor.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 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(lfs_token.actor_name).to eq(user.username) + end + + it 'returns the correct token type' do + 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 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