Commit 029c0d79 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'lfs-ssh-authorization-fix' into 'master'

Do not regenerate the `lfs_token` every time `git-lfs-authenticate` is called

## What does this MR do?

 Do not regenerate the `lfs_token` every time `git-lfs-authenticate` is called, instead return the saved token if one is present.

This was causing a lot of 401s, leading to 403s, as state in #22527

As it turns out, when pushing a lot of LFS objects, the LFS client was calling `git-lfs-authenticate` in the middle of the request again. This caused the `lfs_token` to be regenerated. The problem lies in that the LFS client was not aware of this change, and was still using the old token. This caused all subsequent requests to fail with a 401 error.

Since HTTP Auth is protected by Rack Attack, this 401s where immediately flagged and resulted in the IP of the user being banned. 

With this change, GitLab returns the value stored in Redis, if one is present, thus if the LFS client calls `git-lfs-authenticate` again during the request, the auth header will remain unchanged, allowing all subsequent requests to continue without issues.

## What are the relevant issue numbers?

Fixes #22527

cc @SeanPackham @jacobvosmaer-gitlab

See merge request !6551
parents 578488ee 2772109a
...@@ -90,7 +90,7 @@ module API ...@@ -90,7 +90,7 @@ module API
{ {
username: token_handler.actor_name, username: token_handler.actor_name,
lfs_token: token_handler.generate, lfs_token: token_handler.token,
repository_http_path: project.http_url_to_repo repository_http_path: project.http_url_to_repo
} }
end end
......
...@@ -124,7 +124,7 @@ module Gitlab ...@@ -124,7 +124,7 @@ module Gitlab
read_authentication_abilities read_authentication_abilities
end end
Result.new(actor, nil, token_handler.type, authentication_abilities) if Devise.secure_compare(token_handler.value, password) Result.new(actor, nil, token_handler.type, authentication_abilities) if Devise.secure_compare(token_handler.token, password)
end end
def build_access_token_check(login, password) def build_access_token_check(login, password)
......
...@@ -17,20 +17,19 @@ module Gitlab ...@@ -17,20 +17,19 @@ module Gitlab
end end
end end
def generate def token
token = Devise.friendly_token(TOKEN_LENGTH)
Gitlab::Redis.with do |redis| Gitlab::Redis.with do |redis|
token = redis.get(redis_key)
if token
redis.expire(redis_key, EXPIRY_TIME)
else
token = Devise.friendly_token(TOKEN_LENGTH)
redis.set(redis_key, token, ex: EXPIRY_TIME) redis.set(redis_key, token, ex: EXPIRY_TIME)
end end
token token
end end
def value
Gitlab::Redis.with do |redis|
redis.get(redis_key)
end
end end
def user? def user?
......
...@@ -64,7 +64,7 @@ describe Gitlab::Auth, lib: true do ...@@ -64,7 +64,7 @@ describe Gitlab::Auth, lib: true do
it 'recognizes user lfs tokens' do it 'recognizes user lfs tokens' do
user = create(:user) user = create(:user)
ip = 'ip' ip = 'ip'
token = Gitlab::LfsToken.new(user).generate token = Gitlab::LfsToken.new(user).token
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username) expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :lfs_token, full_authentication_abilities)) expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :lfs_token, full_authentication_abilities))
...@@ -73,7 +73,7 @@ describe Gitlab::Auth, lib: true do ...@@ -73,7 +73,7 @@ describe Gitlab::Auth, lib: true do
it 'recognizes deploy key lfs tokens' do it 'recognizes deploy key lfs tokens' do
key = create(:deploy_key) key = create(:deploy_key)
ip = 'ip' ip = 'ip'
token = Gitlab::LfsToken.new(key).generate token = Gitlab::LfsToken.new(key).token
expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs+deploy-key-#{key.id}") expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs+deploy-key-#{key.id}")
expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities)) expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities))
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::LfsToken, lib: true do describe Gitlab::LfsToken, lib: true do
describe '#generate and #value' do describe '#token' do
shared_examples 'an LFS token generator' do shared_examples 'an LFS token generator' do
it 'returns a randomly generated token' do it 'returns a randomly generated token' do
token = handler.generate token = handler.token
expect(token).not_to be_nil expect(token).not_to be_nil
expect(token).to be_a String expect(token).to be_a String
...@@ -12,9 +12,9 @@ describe Gitlab::LfsToken, lib: true do ...@@ -12,9 +12,9 @@ describe Gitlab::LfsToken, lib: true do
end end
it 'returns the correct token based on the key' do it 'returns the correct token based on the key' do
token = handler.generate token = handler.token
expect(handler.value).to eq(token) expect(handler.token).to eq(token)
end end
end end
......
...@@ -111,7 +111,7 @@ describe API::API, api: true do ...@@ -111,7 +111,7 @@ describe API::API, api: true do
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username) expect(json_response['username']).to eq(user.username)
expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value) 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(json_response['repository_http_path']).to eq(project.http_url_to_repo)
end end
...@@ -131,7 +131,7 @@ describe API::API, api: true do ...@@ -131,7 +131,7 @@ describe API::API, api: true do
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}") expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value) 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(json_response['repository_http_path']).to eq(project.http_url_to_repo)
end end
end end
......
...@@ -257,6 +257,29 @@ describe 'Git LFS API and storage' do ...@@ -257,6 +257,29 @@ describe 'Git LFS API and storage' do
it_behaves_like 'responds with a file' it_behaves_like 'responds with a file'
end end
describe 'when using a user key' do
let(:authorization) { authorize_user_key }
context 'when user allowed' do
let(:update_permissions) do
project.team << [user, :master]
project.lfs_objects << lfs_object
end
it_behaves_like 'responds with a file'
end
context 'when user not allowed' do
let(:update_permissions) do
project.lfs_objects << lfs_object
end
it 'responds with status 404' do
expect(response).to have_http_status(404)
end
end
end
context 'when build is authorized as' do context 'when build is authorized as' do
let(:authorization) { authorize_ci_project } let(:authorization) { authorize_ci_project }
...@@ -1110,7 +1133,11 @@ describe 'Git LFS API and storage' do ...@@ -1110,7 +1133,11 @@ describe 'Git LFS API and storage' do
end end
def authorize_deploy_key def authorize_deploy_key
ActionController::HttpAuthentication::Basic.encode_credentials("lfs+deploy-key-#{key.id}", Gitlab::LfsToken.new(key).generate) ActionController::HttpAuthentication::Basic.encode_credentials("lfs+deploy-key-#{key.id}", Gitlab::LfsToken.new(key).token)
end
def authorize_user_key
ActionController::HttpAuthentication::Basic.encode_credentials(user.username, Gitlab::LfsToken.new(user).token)
end end
def fork_project(project, user, object = nil) def fork_project(project, user, object = nil)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment