diff --git a/doc/administration/geo/replication/security_review.md b/doc/administration/geo/replication/security_review.md index 3dcc4774f5f9b89fdb1a4477d19bbeaaa3860463..f77527ae8a7100a2064d48a06b9aedeeb6362abc 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/app/controllers/ee/projects/git_http_controller.rb b/ee/app/controllers/ee/projects/git_http_controller.rb index ba779157db35fc6837312fd2be763263c16f4e65..c9c512642cfc7d2ce0983a1678bf816a1679ca99 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/app/services/geo/base_sync_service.rb b/ee/app/services/geo/base_sync_service.rb index daf053ad578ac0de945dda3bbd13d66b2cc54a95..49bda975b929cc549d05f7a705ace821fd2b6abd 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/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 53a556bbf7adfe3020c0907178c42793edc83855..3c6add2c9b22b3bb953b0a9ffddf69692329c935 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 diff --git a/ee/lib/gitlab/geo/git_push_ssh_proxy.rb b/ee/lib/gitlab/geo/git_push_ssh_proxy.rb index dc63ddb0707ea07d3db646bdbea251d510d8ac8b..e7e1a2d6e637c5e2f0ca49b64b3c7e42b2fd2fa7 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(%r{^\/|\.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 7fcc71ee9a74b339022e41dd3237ba1cb4f9cf9a..3366bf0b9746c2d2a64ca7f38a1ff5a0cc36bd9d 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 60c4c26fe169316859cf4eb61b0baee91856ec7e..d1c2526458719b41325298d257ba6585692b12fc 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,20 @@ 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 '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 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 eb76f243e278eaaa2b3cf2995e8edb2170726ef8..fb2bb84fc746b65318f4431e7a1037092759dd52 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) diff --git a/ee/spec/requests/git_http_geo_spec.rb b/ee/spec/requests/git_http_geo_spec.rb index 5554dc7e11fda5feba1aa56726e6e79e99c61c53..cc2bad4300c26b761b01773fe6ca6070a570f7ce 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