From d3be081238e884416ca73f2ada48ab0ed7236f27 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 20 Feb 2019 13:17:46 +0200 Subject: [PATCH 1/4] Revert JWT scope inforcement --- .../ee/projects/git_http_controller.rb | 9 ---- ee/spec/requests/git_http_geo_spec.rb | 45 ------------------- 2 files changed, 54 deletions(-) diff --git a/ee/app/controllers/ee/projects/git_http_controller.rb b/ee/app/controllers/ee/projects/git_http_controller.rb index ba779157db35fc..c9c512642cfc7d 100644 --- a/ee/app/controllers/ee/projects/git_http_controller.rb +++ b/ee/app/controllers/ee/projects/git_http_controller.rb @@ -48,7 +48,6 @@ def access_actor def authenticate_user return super unless geo_request? return render_bad_geo_auth('Bad token') unless decoded_authorization - return render_bad_geo_auth('Unauthorized scope') unless jwt_scope_valid? # grant access @authentication_result = ::Gitlab::Auth::Result.new(nil, project, :geo, [:download_code, :push_code]) # rubocop:disable Gitlab/ModuleWithInstanceVariables @@ -58,14 +57,6 @@ def authenticate_user render_bad_geo_auth("Invalid signature time ") end - def jwt_scope_valid? - decoded_authorization[:scope] == ::Gitlab::Geo::JwtRequestDecoder.build_repository_scope(repository_type, project.id) - end - - def repository_type - wiki? ? 'wiki' : 'repository' - end - def decoded_authorization strong_memoize(:decoded_authorization) do ::Gitlab::Geo::JwtRequestDecoder.new(request.headers['Authorization']).decode diff --git a/ee/spec/requests/git_http_geo_spec.rb b/ee/spec/requests/git_http_geo_spec.rb index 5554dc7e11fda5..cc2bad4300c26b 100644 --- a/ee/spec/requests/git_http_geo_spec.rb +++ b/ee/spec/requests/git_http_geo_spec.rb @@ -21,7 +21,6 @@ let!(:key_for_user_without_push_access) { create(:key, user: user_without_push_access) } let(:env) { valid_geo_env } - let(:auth_token_with_invalid_scope) { Gitlab::Geo::BaseRequest.new(scope: "invalid-#{project.id}").authorization } before do project.add_maintainer(user) @@ -347,50 +346,6 @@ def make_request end end end - - context 'invalid scope' do - let(:repository_path) { project.full_path } - - subject do - make_request - response - end - - def make_request - get "/#{repository_path}.git/info/refs", params: { service: 'git-upload-pack' }, headers: env - end - - shared_examples_for 'unauthorized because of invalid scope' do - it { is_expected.to have_gitlab_http_status(:unauthorized) } - - it 'returns correct error' do - expect(subject.parsed_body).to eq('Geo JWT authentication failed: Unauthorized scope') - end - end - - context 'invalid scope of Geo JWT token' do - let(:env) { geo_env(auth_token_with_invalid_scope) } - - include_examples 'unauthorized because of invalid scope' - end - - context 'Geo JWT token scopes for wiki and repository are not interchangeable' do - context 'wiki scope' do - let(:auth_token_with_valid_wiki_scope) { Gitlab::Geo::BaseRequest.new(scope: "wiki-#{project.id}").authorization } - let(:env) { geo_env(auth_token_with_valid_wiki_scope) } - - include_examples 'unauthorized because of invalid scope' - end - - context 'respository scope' do - let(:repository_path) { project.wiki.full_path } - let(:auth_token_with_valid_repository_scope) { Gitlab::Geo::BaseRequest.new(scope: "repository-#{project.id}").authorization } - let(:env) { geo_env(auth_token_with_valid_repository_scope) } - - include_examples 'unauthorized because of invalid scope' - end - end - end end def valid_geo_env -- GitLab From 322bba86b06f501012f10d483645e9ef71542b35 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 20 Feb 2019 13:18:19 +0200 Subject: [PATCH 2/4] Change format of the scope of JWT tokens for repositories --- ee/app/services/geo/base_sync_service.rb | 2 +- ee/lib/gitlab/geo/git_push_ssh_proxy.rb | 6 +++++- ee/lib/gitlab/geo/jwt_request_decoder.rb | 4 ---- ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb | 8 ++++++++ ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb | 6 ------ 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/ee/app/services/geo/base_sync_service.rb b/ee/app/services/geo/base_sync_service.rb index daf053ad578ac0..49bda975b929cc 100644 --- a/ee/app/services/geo/base_sync_service.rb +++ b/ee/app/services/geo/base_sync_service.rb @@ -104,7 +104,7 @@ def fetch_geo_mirror(repository) # Build a JWT header for authentication def jwt_authentication_header authorization = ::Gitlab::Geo::RepoSyncRequest.new( - scope: ::Gitlab::Geo::JwtRequestDecoder.build_repository_scope(type, project.id) + scope: project.repository.full_path ).authorization { "http.#{remote_url}.extraHeader" => "Authorization: #{authorization}" } diff --git a/ee/lib/gitlab/geo/git_push_ssh_proxy.rb b/ee/lib/gitlab/geo/git_push_ssh_proxy.rb index dc63ddb0707ea0..0f93f362099d69 100644 --- a/ee/lib/gitlab/geo/git_push_ssh_proxy.rb +++ b/ee/lib/gitlab/geo/git_push_ssh_proxy.rb @@ -105,10 +105,14 @@ def gl_id def base_headers @base_headers ||= { 'Geo-GL-Id' => gl_id, - 'Authorization' => Gitlab::Geo::BaseRequest.new.authorization + 'Authorization' => Gitlab::Geo::BaseRequest.new(scope: auth_scope).authorization } end + def auth_scope + URI.parse(primary_repo).path.gsub(/(^\/|\.git$)/, '') + end + def get(url, headers) request(url, Net::HTTP::Get, headers) end diff --git a/ee/lib/gitlab/geo/jwt_request_decoder.rb b/ee/lib/gitlab/geo/jwt_request_decoder.rb index 7fcc71ee9a74b3..3366bf0b9746c2 100644 --- a/ee/lib/gitlab/geo/jwt_request_decoder.rb +++ b/ee/lib/gitlab/geo/jwt_request_decoder.rb @@ -12,10 +12,6 @@ def self.geo_auth_attempt?(header) token_type == ::Gitlab::Geo::BaseRequest::GITLAB_GEO_AUTH_TOKEN_TYPE end - def self.build_repository_scope(repository_type, project_id) - [repository_type, project_id].join('-') - end - attr_reader :auth_header def initialize(auth_header) diff --git a/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb b/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb index 60c4c26fe16931..529b44334f76c3 100644 --- a/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb +++ b/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb @@ -69,6 +69,14 @@ let(:info_refs_headers) { base_headers.merge('Content-Type' => 'application/x-git-upload-pack-request') } let(:info_refs_http_body_full) { "001f# service=git-receive-pack\n0000#{info_refs_body_short}" } + context 'authorization header is scoped' do + it 'returns a Gitlab::Geo::GitPushSSHProxy::APIResponse' do + expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) + + subject.info_refs + end + end + context 'with a failed response' do let(:error_msg) { 'execution expired' } diff --git a/ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb b/ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb index eb76f243e278ea..fb2bb84fc746b6 100644 --- a/ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb +++ b/ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb @@ -7,12 +7,6 @@ subject { described_class.new(request.headers['Authorization']) } - describe ".build_repository_scope" do - it 'returns a scope that consolidates repository type and project id' do - expect(described_class.build_repository_scope('wiki', 5)).to eq('wiki-5') - end - end - describe '#decode' do it 'decodes correct data' do expect(subject.decode).to eq(data) -- GitLab From 77faae4924221044aa92725d6a7d88e4f35a69b1 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 20 Feb 2019 13:21:11 +0200 Subject: [PATCH 3/4] Update a documentation note about JWT scope enforcement --- doc/administration/geo/replication/security_review.md | 1 + .../unreleased/9310-reduce-the-scope-of-geo-jwt-step-2.yml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/administration/geo/replication/security_review.md b/doc/administration/geo/replication/security_review.md index 3dcc4774f5f9b8..f77527ae8a7100 100644 --- a/doc/administration/geo/replication/security_review.md +++ b/doc/administration/geo/replication/security_review.md @@ -271,6 +271,7 @@ questions from [owasp.org](https://www.owasp.org). - LFS and File ID. - Upload and File ID. - Job Artifact and File ID. +- Geo JWTs scopes are not enforced for Git Access yet, but will be in a future version (currently scheduled for GitLab 11.10). ### What access requirements have been defined for URI and Service calls? diff --git a/ee/changelogs/unreleased/9310-reduce-the-scope-of-geo-jwt-step-2.yml b/ee/changelogs/unreleased/9310-reduce-the-scope-of-geo-jwt-step-2.yml index 53a556bbf7adfe..3c6add2c9b22b3 100644 --- a/ee/changelogs/unreleased/9310-reduce-the-scope-of-geo-jwt-step-2.yml +++ b/ee/changelogs/unreleased/9310-reduce-the-scope-of-geo-jwt-step-2.yml @@ -1,5 +1,5 @@ --- -title: Enforce Geo JWT tokens scope +title: Enforce Geo JWT tokens scope for file uploads and Geo API merge_request: 9502 author: type: changed -- GitLab From 085fce8dd8a31faf433f65232f65ff5dd1af2e84 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 20 Feb 2019 15:49:58 +0200 Subject: [PATCH 4/4] Addressing review comments. Added tests and some style changes --- ee/lib/gitlab/geo/git_push_ssh_proxy.rb | 2 +- ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ee/lib/gitlab/geo/git_push_ssh_proxy.rb b/ee/lib/gitlab/geo/git_push_ssh_proxy.rb index 0f93f362099d69..e7e1a2d6e637c5 100644 --- a/ee/lib/gitlab/geo/git_push_ssh_proxy.rb +++ b/ee/lib/gitlab/geo/git_push_ssh_proxy.rb @@ -110,7 +110,7 @@ def base_headers end def auth_scope - URI.parse(primary_repo).path.gsub(/(^\/|\.git$)/, '') + URI.parse(primary_repo).path.gsub(%r{^\/|\.git$}, '') end def get(url, headers) diff --git a/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb b/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb index 529b44334f76c3..d1c2526458719b 100644 --- a/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb +++ b/ee/spec/lib/gitlab/geo/git_push_ssh_proxy_spec.rb @@ -70,11 +70,17 @@ let(:info_refs_http_body_full) { "001f# service=git-receive-pack\n0000#{info_refs_body_short}" } context 'authorization header is scoped' do - it 'returns a Gitlab::Geo::GitPushSSHProxy::APIResponse' do + it 'passes the scope when .info_refs is called' do expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) subject.info_refs end + + it 'passes the scope when .push is called' do + expect(Gitlab::Geo::BaseRequest).to receive(:new).with(scope: project.repository.full_path) + + subject.push(info_refs_body_short) + end end context 'with a failed response' do -- GitLab