Commit ad6a369d authored by Valery Sizov's avatar Valery Sizov

Geo-GL-ID should be passed in JWT token

it will protect the parameter from tampering
parent 0530cc86
...@@ -35,11 +35,17 @@ module EE ...@@ -35,11 +35,17 @@ module EE
end end
def geo_push_user def geo_push_user
@geo_push_user ||= ::Geo::PushUser.new_from_headers(request.headers) return unless geo_gl_id
@geo_push_user ||= ::Geo::PushUser.new(geo_gl_id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
end
def geo_gl_id
decoded_authorization&.dig(:gl_id)
end end
def geo_push_user_headers_provided? def geo_push_proxy_request?
::Geo::PushUser.needed_headers_provided?(request.headers) geo_gl_id
end end
def geo_request? def geo_request?
...@@ -53,8 +59,8 @@ module EE ...@@ -53,8 +59,8 @@ module EE
override :access_actor override :access_actor
def access_actor def access_actor
return super unless geo? return super unless geo?
return :geo unless geo_push_user_headers_provided? return :geo unless geo_push_proxy_request?
return geo_push_user.user if geo_push_user.user return geo_push_user.user if geo_push_user&.user
raise ::Gitlab::GitAccess::ForbiddenError, 'Geo push user is invalid.' raise ::Gitlab::GitAccess::ForbiddenError, 'Geo push user is invalid.'
end end
......
...@@ -7,16 +7,6 @@ class Geo::PushUser ...@@ -7,16 +7,6 @@ class Geo::PushUser
@gl_id = gl_id @gl_id = gl_id
end end
def self.needed_headers_provided?(headers)
headers['Geo-GL-Id'].present?
end
def self.new_from_headers(headers)
return unless needed_headers_provided?(headers)
new(headers['Geo-GL-Id'])
end
def user def user
@user ||= identify_using_ssh_key(gl_id) @user ||= identify_using_ssh_key(gl_id)
end end
......
---
title: 'Geo: Pass GL-ID in a JWT token when proxy-push from secondary'
author:
type: security
...@@ -134,8 +134,7 @@ module Gitlab ...@@ -134,8 +134,7 @@ module Gitlab
def base_headers def base_headers
@base_headers ||= { @base_headers ||= {
'Geo-GL-Id' => gl_id, 'Authorization' => Gitlab::Geo::BaseRequest.new(scope: auth_scope, gl_id: gl_id).authorization
'Authorization' => Gitlab::Geo::BaseRequest.new(scope: auth_scope).authorization
} }
end end
......
...@@ -14,6 +14,7 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do ...@@ -14,6 +14,7 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { project.creator } let(:user) { project.creator }
let(:key) { create(:key, user: user) } let(:key) { create(:key, user: user) }
let(:key_identifier) { "key-#{key.id}" }
let(:base_request) { double(Gitlab::Geo::BaseRequest.new.authorization) } let(:base_request) { double(Gitlab::Geo::BaseRequest.new.authorization) }
let(:info_refs_body_short) do let(:info_refs_body_short) do
...@@ -22,7 +23,6 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do ...@@ -22,7 +23,6 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
let(:base_headers) do let(:base_headers) do
{ {
'Geo-GL-Id' => "key-#{key.id}",
'Authorization' => 'secret' 'Authorization' => 'secret'
} }
end end
...@@ -72,13 +72,13 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do ...@@ -72,13 +72,13 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
context 'authorization header is scoped' do context 'authorization header is scoped' do
it 'passes the scope when .info_refs_upload_pack is called' do it 'passes the scope when .info_refs_upload_pack is called' do
expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier)
subject.info_refs_upload_pack subject.info_refs_upload_pack
end end
it 'passes the scope when .receive_pack is called' do it 'passes the scope when .receive_pack is called' do
expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier)
subject.receive_pack(info_refs_body_short) subject.receive_pack(info_refs_body_short)
end end
...@@ -299,13 +299,13 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do ...@@ -299,13 +299,13 @@ RSpec.describe Gitlab::Geo::GitSSHProxy, :geo do
context 'authorization header is scoped' do context 'authorization header is scoped' do
it 'passes the scope when .info_refs_receive_pack is called' do it 'passes the scope when .info_refs_receive_pack is called' do
expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier )
subject.info_refs_receive_pack subject.info_refs_receive_pack
end end
it 'passes the scope when .receive_pack is called' do it 'passes the scope when .receive_pack is called' do
expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path, gl_id: key_identifier)
subject.receive_pack(info_refs_body_short) subject.receive_pack(info_refs_body_short)
end end
......
...@@ -10,58 +10,6 @@ RSpec.describe Geo::PushUser do ...@@ -10,58 +10,6 @@ RSpec.describe Geo::PushUser do
subject { described_class.new(gl_id) } subject { described_class.new(gl_id) }
describe '.needed_headers_provided?' do
where(:headers) do
[
{},
{ 'Geo-GL-Id' => nil },
{ 'Geo-GL-Id' => '' }
]
end
with_them do
it 'returns false' do
expect(described_class.needed_headers_provided?(headers)).to be(false)
end
end
context 'where gl_id is not nil' do
let(:headers) do
{ 'Geo-GL-Id' => gl_id }
end
it 'returns true' do
expect(described_class.needed_headers_provided?(headers)).to be(true)
end
end
end
describe '.new_from_headers' do
where(:headers) do
[
{},
{ 'Geo-GL-Id' => nil },
{ 'Geo-GL-Id' => '' }
]
end
with_them do
it 'returns false' do
expect(described_class.new_from_headers(headers)).to be_nil
end
end
context 'where gl_id is not nil' do
let(:headers) do
{ 'Geo-GL-Id' => gl_id }
end
it 'returns an instance of Geo::PushUser' do
expect(described_class.new_from_headers(headers)).to be_a(described_class)
end
end
end
describe '#user' do describe '#user' do
context 'with a junk gl_id' do context 'with a junk gl_id' do
let(:gl_id) { "test" } let(:gl_id) { "test" }
......
...@@ -17,8 +17,6 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do ...@@ -17,8 +17,6 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
let_it_be(:primary) { create(:geo_node, :primary, url: primary_url) } let_it_be(:primary) { create(:geo_node, :primary, url: primary_url) }
let_it_be(:secondary) { create(:geo_node, url: secondary_url) } let_it_be(:secondary) { create(:geo_node, url: secondary_url) }
# Ensure the token always comes from the real time of the request
let(:auth_token) { Gitlab::Geo::BaseRequest.new(scope: project.full_path).authorization }
let!(:user) { create(:user) } let!(:user) { create(:user) }
let!(:user_without_any_access) { create(:user) } let!(:user_without_any_access) { create(:user) }
let!(:user_without_push_access) { create(:user) } let!(:user_without_push_access) { create(:user) }
...@@ -131,6 +129,8 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do ...@@ -131,6 +129,8 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
context 'when current node is a secondary' do context 'when current node is a secondary' do
let(:current_node) { secondary } let(:current_node) { secondary }
let(:auth_token) { Gitlab::Geo::BaseRequest.new(scope: project.full_path).authorization }
describe 'GET info_refs' do describe 'GET info_refs' do
context 'git pull' do context 'git pull' do
def make_request def make_request
...@@ -453,6 +453,10 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do ...@@ -453,6 +453,10 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
end end
context 'when current node is the primary', :use_clean_rails_memory_store_caching do context 'when current node is the primary', :use_clean_rails_memory_store_caching do
let!(:geo_gl_id) { "key-#{key.id}" }
# Ensure the token always comes from the real time of the request
let(:auth_token) { Gitlab::Geo::BaseRequest.new(scope: project.full_path, gl_id: geo_gl_id).authorization }
let(:current_node) { primary } let(:current_node) { primary }
describe 'POST git_receive_pack' do describe 'POST git_receive_pack' do
...@@ -503,27 +507,7 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do ...@@ -503,27 +507,7 @@ RSpec.describe "Git HTTP requests (Geo)", :geo do
let_it_be(:project) { project_with_repo } let_it_be(:project) { project_with_repo }
let(:endpoint_path) { "/#{project.full_path}.git/git-receive-pack" } let(:endpoint_path) { "/#{project.full_path}.git/git-receive-pack" }
before do context 'when gl_id is provided in JWT token' do
env['Geo-GL-Id'] = geo_gl_id
end
context 'when gl_id is incorrectly provided via HTTP headers' do
where(:geo_gl_id) do
[
nil,
''
]
end
with_them do
it 'returns a 403' do
is_expected.to have_gitlab_http_status(:forbidden)
expect(response.body).to eql('You are not allowed to upload code for this project.')
end
end
end
context 'when gl_id is provided via HTTP headers' do
context 'but is invalid' do context 'but is invalid' do
where(:geo_gl_id) do where(:geo_gl_id) do
%w[ %w[
......
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