Commit d542329e authored by Marius Bobin's avatar Marius Bobin

Allow tokens only from running jobs for API auth

Adds a finder class used for authentication with CI job tokens.
It checks that the job's status is `running` and that the project
is not removed.
parent a2f340d4
# frozen_string_literal: true
module Ci
class AuthJobFinder
AuthError = Class.new(StandardError)
NotRunningJobError = Class.new(AuthError)
ErasedJobError = Class.new(AuthError)
DeletedProjectError = Class.new(AuthError)
def initialize(token:)
@token = token
end
def execute!
find_job_by_token.tap do |job|
next unless job
validate_job!(job)
end
end
def execute
execute!
rescue AuthError
end
private
attr_reader :token, :require_running, :raise_on_missing
def find_job_by_token
::Ci::Build.find_by_token(token)
end
def validate_job!(job)
validate_running_job!(job)
validate_job_not_erased!(job)
validate_project_presence!(job)
true
end
def validate_running_job!(job)
raise NotRunningJobError, 'Job is not running' unless job.running?
end
def validate_job_not_erased!(job)
raise ErasedJobError, 'Job has been erased!' if job.erased?
end
def validate_project_presence!(job)
if job.project.nil? || job.project.pending_delete?
raise DeletedProjectError, 'Project has been deleted!'
end
end
end
end
......@@ -10,6 +10,9 @@ module Ci
elsif job_from_token
create_pipeline_from_job(job_from_token)
end
rescue Ci::AuthJobFinder::AuthError => e
error(e.message, 401)
end
private
......@@ -41,8 +44,6 @@ module Ci
# this check is to not leak the presence of the project if user cannot read it
return unless can?(job.user, :read_project, project)
return error("400 Job has to be running", 400) unless job.running?
pipeline = Ci::CreatePipelineService.new(project, job.user, ref: params[:ref])
.execute(:pipeline, ignore_skip_ci: true) do |pipeline|
source = job.sourced_pipelines.build(
......@@ -64,7 +65,7 @@ module Ci
def job_from_token
strong_memoize(:job) do
Ci::Build.find_by_token(params[:token].to_s)
Ci::AuthJobFinder.new(token: params[:token].to_s).execute!
end
end
......
---
title: Allow only running job tokens for API authentication
merge_request:
author:
type: security
......@@ -6,7 +6,7 @@ RSpec.describe API::Helpers do
include API::APIGuard::HelperMethods
include described_class
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let(:route_authentication_setting) { {} }
let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) }
let(:env) do
......@@ -36,7 +36,9 @@ RSpec.describe API::Helpers do
describe ".current_user" do
describe "when authenticating using a job token" do
let(:job) { create(:ci_build, user: user) }
let_it_be(:job, reload: true) do
create(:ci_build, user: user, status: :running)
end
context 'when route is allowed to be authenticated' do
let(:route_authentication_setting) { { job_token_allowed: true } }
......@@ -47,6 +49,13 @@ RSpec.describe API::Helpers do
expect { current_user }.to raise_error /401/
end
it "returns a 401 response for a job that's not running" do
job.update!(status: :success)
env[Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER] = job.token
expect { current_user }.to raise_error /401/
end
it "returns a 403 response for a user without access" do
env[Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER] = job.token
allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
......
......@@ -26,7 +26,7 @@ RSpec.describe API::Jobs do
end
describe 'GET /projects/:id/jobs/:job_id/artifacts' do
let(:job) { create(:ci_build, :artifacts, pipeline: pipeline, user: api_user) }
let(:job) { create(:ci_build, :artifacts, pipeline: pipeline, user: api_user, status: :running) }
context 'when using job_token to authenticate' do
subject do
......@@ -71,15 +71,30 @@ RSpec.describe API::Jobs do
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when the job is not running' do
let(:api_user) { developer }
before do
job.success!
end
it 'disallows access to the artifacts' do
subject
expect(response).to have_gitlab_http_status(:unauthorized)
end
end
end
end
describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
let(:running_job) { create(:ci_build, :artifacts, pipeline: pipeline, user: api_user, status: :running) }
let(:job) { create(:ci_build, :artifacts, pipeline: pipeline, user: api_user) }
context 'when using job_token to authenticate' do
subject do
get api("/projects/#{project.id}/jobs/artifacts/#{pipeline.ref}/download"), params: { job: job.name, job_token: job.token }
get api("/projects/#{project.id}/jobs/artifacts/#{pipeline.ref}/download"), params: { job: job.name, job_token: running_job.token }
end
context 'when cross-project pipelines are enabled' do
......
......@@ -3,8 +3,8 @@
require 'spec_helper'
RSpec.describe API::Triggers do
let(:user) { create(:user) }
let(:project) { create(:project, :repository, :auto_devops, creator: user) }
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project, :repository, :auto_devops, creator: user) }
describe 'POST /projects/:project_id/trigger/pipeline' do
context 'when triggering a pipeline from a job token' do
......@@ -78,8 +78,8 @@ RSpec.describe API::Triggers do
it 'does not create a pipeline' do
subject
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['message']).to eq('400 Job has to be running')
expect(response).to have_gitlab_http_status(:unauthorized)
expect(json_response['message']).to eq('Job is not running')
end
end
......
......@@ -222,7 +222,7 @@ module API
return unless token
::Ci::Build.find_by_token(token.access_token_id.to_s)
::Ci::AuthJobFinder.new(token: token.access_token_id.to_s).execute
end
def decode_oauth_token_from_jwt
......
......@@ -23,7 +23,7 @@ module API
return unless token
::Ci::Build.find_by_token(token)
::Ci::AuthJobFinder.new(token: token).execute
end
def find_deploy_token_from_http_basic_auth
......
......@@ -69,9 +69,7 @@ module Gitlab
current_request.env[JOB_TOKEN_HEADER].presence
return unless token
job = ::Ci::Build.find_by_token(token)
raise UnauthorizedError unless job
job = find_valid_running_job_by_token!(token)
@current_authenticated_job = job # rubocop:disable Gitlab/ModuleWithInstanceVariables
job.user
......@@ -84,9 +82,7 @@ module Gitlab
return unless login.present? && password.present?
return unless ::Gitlab::Auth::CI_JOB_USER == login
job = ::Ci::Build.find_by_token(password)
raise UnauthorizedError unless job
job = find_valid_running_job_by_token!(password)
job.user
end
......@@ -179,7 +175,7 @@ module Gitlab
token = parsed_oauth_token
return unless token
job = ::Ci::Build.find_by_token(token)
job = ::Ci::AuthJobFinder.new(token: token).execute
return unless job
@current_authenticated_job = job # rubocop:disable Gitlab/ModuleWithInstanceVariables
......@@ -304,6 +300,12 @@ module Gitlab
def blob_request?
current_request.path.include?('/raw/')
end
def find_valid_running_job_by_token!(token)
::Ci::AuthJobFinder.new(token: token).execute.tap do |job|
raise UnauthorizedError unless job
end
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::AuthJobFinder do
let_it_be(:job, reload: true) { create(:ci_build, status: :running) }
let(:token) { job.token }
subject(:finder) do
described_class.new(token: token)
end
describe '#execute!' do
subject(:execute) { finder.execute! }
it { is_expected.to eq(job) }
it 'raises error if the job is not running' do
job.success!
expect { execute }.to raise_error described_class::NotRunningJobError, 'Job is not running'
end
it 'raises error if the job is erased' do
expect(::Ci::Build).to receive(:find_by_token).with(job.token).and_return(job)
expect(job).to receive(:erased?).and_return(true)
expect { execute }.to raise_error described_class::ErasedJobError, 'Job has been erased!'
end
it 'raises error if the the project is missing' do
expect(::Ci::Build).to receive(:find_by_token).with(job.token).and_return(job)
expect(job).to receive(:project).and_return(nil)
expect { execute }.to raise_error described_class::DeletedProjectError, 'Project has been deleted!'
end
it 'raises error if the the project is being removed' do
project = double(Project)
expect(::Ci::Build).to receive(:find_by_token).with(job.token).and_return(job)
expect(job).to receive(:project).twice.and_return(project)
expect(project).to receive(:pending_delete?).and_return(true)
expect { execute }.to raise_error described_class::DeletedProjectError, 'Project has been deleted!'
end
context 'with wrong job token' do
let(:token) { 'missing' }
it { is_expected.to be_nil }
end
end
describe '#execute' do
subject(:execute) { finder.execute }
before do
job.success!
end
it { is_expected.to be_nil }
end
end
......@@ -50,7 +50,7 @@ RSpec.describe API::Helpers::PackagesManagerClientsHelpers do
describe '#find_job_from_http_basic_auth' do
let_it_be(:user) { personal_access_token.user }
let(:job) { create(:ci_build, user: user) }
let(:job) { create(:ci_build, user: user, status: :running) }
let(:password) { job.token }
subject { helper.find_job_from_http_basic_auth }
......@@ -60,6 +60,16 @@ RSpec.describe API::Helpers::PackagesManagerClientsHelpers do
end
it_behaves_like 'invalid auth header'
context 'when the job is not running' do
before do
job.update!(status: :failed)
end
it_behaves_like 'valid auth header' do
let(:expected_result) { nil }
end
end
end
describe '#find_deploy_token_from_http_basic_auth' do
......
......@@ -37,11 +37,29 @@ RSpec.describe Gitlab::Auth::AuthFinders do
expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError)
end
it "return user if token is valid" do
set_token(job.token)
context 'with a running job' do
before do
job.update!(status: :running)
end
it 'return user if token is valid' do
set_token(job.token)
expect(subject).to eq(user)
expect(@current_authenticated_job).to eq job
end
end
expect(subject).to eq(user)
expect(@current_authenticated_job).to eq job
context 'with a job that is not running' do
before do
job.update!(status: :failed)
end
it 'returns an Unauthorized exception' do
set_token(job.token)
expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError)
end
end
end
end
......@@ -557,7 +575,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
context 'with CI username' do
let(:username) { ::Gitlab::Auth::CI_JOB_USER }
let(:user) { create(:user) }
let(:build) { create(:ci_build, user: user) }
let(:build) { create(:ci_build, user: user, status: :running) }
it 'returns nil without password' do
set_basic_auth_header(username, nil)
......@@ -576,6 +594,13 @@ RSpec.describe Gitlab::Auth::AuthFinders do
expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError)
end
it 'returns exception if the job is not running' do
set_basic_auth_header(username, build.token)
build.success!
expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError)
end
end
end
......@@ -586,7 +611,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
context 'with a job token' do
let(:route_authentication_setting) { { job_token_allowed: true } }
let(:job) { create(:ci_build, user: user) }
let(:job) { create(:ci_build, user: user, status: :running) }
before do
env['HTTP_AUTHORIZATION'] = "Bearer #{job.token}"
......@@ -641,7 +666,7 @@ RSpec.describe Gitlab::Auth::AuthFinders do
end
describe '#find_user_from_job_token' do
let(:job) { create(:ci_build, user: user) }
let(:job) { create(:ci_build, user: user, status: :running) }
let(:route_authentication_setting) { { job_token_allowed: true } }
subject { find_user_from_job_token }
......@@ -666,6 +691,13 @@ RSpec.describe Gitlab::Auth::AuthFinders do
expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError)
end
it 'returns exception if the job is not running' do
set_header(described_class::JOB_TOKEN_HEADER, job.token)
job.success!
expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError)
end
context 'when route is not allowed to be authenticated' do
let(:route_authentication_setting) { { job_token_allowed: false } }
......
......@@ -88,7 +88,7 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
describe '#find_user_from_job_token' do
let!(:user) { build(:user) }
let!(:job) { build(:ci_build, user: user) }
let!(:job) { build(:ci_build, user: user, status: :running) }
before do
env[Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER] = 'token'
......@@ -97,13 +97,18 @@ RSpec.describe Gitlab::Auth::RequestAuthenticator do
context 'with API requests' do
before do
env['SCRIPT_NAME'] = '/api/endpoint'
expect(::Ci::Build).to receive(:find_by_token).with('token').and_return(job)
end
it 'tries to find the user' do
expect(::Ci::Build).to receive(:find_by_token).and_return(job)
expect(subject.find_sessionless_user([:api])).to eq user
end
it 'returns nil if the job is not running' do
job.status = :success
expect(subject.find_sessionless_user([:api])).to be_blank
end
end
context 'without API requests' do
......
......@@ -13,7 +13,7 @@ RSpec.describe API::ConanPackages do
let(:base_secret) { SecureRandom.base64(64) }
let(:auth_token) { personal_access_token.token }
let(:job) { create(:ci_build, user: user) }
let(:job) { create(:ci_build, user: user, status: :running) }
let(:job_token) { job.token }
let(:deploy_token) { create(:deploy_token, read_package_registry: true, write_package_registry: true) }
let(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) }
......@@ -93,6 +93,14 @@ RSpec.describe API::ConanPackages do
expect(response).to have_gitlab_http_status(:unauthorized)
end
it 'responds with 401 Unauthorized when the job is not running' do
job.update!(status: :failed)
jwt = build_jwt_from_job(job)
get api('/packages/conan/v1/ping'), headers: build_token_auth_header(jwt.encoded)
expect(response).to have_gitlab_http_status(:unauthorized)
end
context 'packages feature disabled' do
it 'responds with 404 Not Found' do
stub_packages_setting(enabled: false)
......
......@@ -11,7 +11,7 @@ RSpec.describe API::GoProxy do
let_it_be(:base) { "#{Settings.build_gitlab_go_url}/#{project.full_path}" }
let_it_be(:oauth) { create :oauth_access_token, scopes: 'api', resource_owner: user }
let_it_be(:job) { create :ci_build, user: user }
let_it_be(:job) { create :ci_build, user: user, status: :running }
let_it_be(:pa_token) { create :personal_access_token, user: user }
let_it_be(:modules) do
......@@ -393,6 +393,13 @@ RSpec.describe API::GoProxy do
expect(response).to have_gitlab_http_status(:ok)
end
it 'returns unauthorized with a failed job token' do
job.update!(status: :failed)
get_resource(oauth_access_token: job)
expect(response).to have_gitlab_http_status(:unauthorized)
end
it 'returns unauthorized with no authentication' do
get_resource
......
......@@ -12,7 +12,7 @@ RSpec.describe API::MavenPackages do
let_it_be(:package_file) { package.package_files.with_file_name_like('%.xml').first }
let_it_be(:jar_file) { package.package_files.with_file_name_like('%.jar').first }
let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
let_it_be(:job) { create(:ci_build, user: user) }
let_it_be(:job, reload: true) { create(:ci_build, user: user, status: :running) }
let_it_be(:deploy_token) { create(:deploy_token, read_package_registry: true, write_package_registry: true) }
let_it_be(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) }
......@@ -102,11 +102,25 @@ RSpec.describe API::MavenPackages do
end
shared_examples 'downloads with a job token' do
it 'allows download with job token' do
download_file(package_file.file_name, job_token: job.token)
context 'with a running job' do
it 'allows download with job token' do
download_file(package_file.file_name, job_token: job.token)
expect(response).to have_gitlab_http_status(:ok)
expect(response.media_type).to eq('application/octet-stream')
expect(response).to have_gitlab_http_status(:ok)
expect(response.media_type).to eq('application/octet-stream')
end
end
context 'with a finished job' do
before do
job.update!(status: :failed)
end
it 'returns unauthorized error' do
download_file(package_file.file_name, job_token: job.token)
expect(response).to have_gitlab_http_status(:unauthorized)
end
end
end
......@@ -569,13 +583,20 @@ RSpec.describe API::MavenPackages do
expect(jar_file.file_name).to eq(file_upload.original_filename)
end
it 'allows upload with job token' do
it 'allows upload with running job token' do
upload_file(params.merge(job_token: job.token))
expect(response).to have_gitlab_http_status(:ok)
expect(project.reload.packages.last.build_info.pipeline).to eq job.pipeline
end
it 'rejects upload without running job token' do
job.update!(status: :failed)
upload_file(params.merge(job_token: job.token))
expect(response).to have_gitlab_http_status(:unauthorized)
end
it 'allows upload with deploy token' do
upload_file(params, headers_with_deploy_token)
......
......@@ -12,7 +12,7 @@ RSpec.describe API::NpmPackages do
let_it_be(:package, reload: true) { create(:npm_package, project: project) }
let_it_be(:token) { create(:oauth_access_token, scopes: 'api', resource_owner: user) }
let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
let_it_be(:job) { create(:ci_build, user: user) }
let_it_be(:job, reload: true) { create(:ci_build, user: user, status: :running) }
let_it_be(:deploy_token) { create(:deploy_token, read_package_registry: true, write_package_registry: true) }
let_it_be(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) }
......@@ -27,12 +27,19 @@ RSpec.describe API::NpmPackages do
expect_a_valid_package_response
end
it 'returns the package info with job token' do
it 'returns the package info with running job token' do
get_package_with_job_token(package)
expect_a_valid_package_response
end
it 'denies request without running job token' do
job.update!(status: :success)
get_package_with_job_token(package)
expect(response).to have_gitlab_http_status(:unauthorized)
end
it 'denies request without oauth token' do
get_package(package)
......
......@@ -671,12 +671,20 @@ RSpec.describe API::Releases do
end
context 'when a valid token is provided' do
it 'creates the release' do
it 'creates the release for a running job' do
job.update!(status: :running)
post api("/projects/#{project.id}/releases"), params: params.merge(job_token: job.token)
expect(response).to have_gitlab_http_status(:created)
expect(project.releases.last.description).to eq('Another nice release')
end
it 'returns an :unauthorized error for a completed job' do
job.success!
post api("/projects/#{project.id}/releases"), params: params.merge(job_token: job.token)
expect(response).to have_gitlab_http_status(:unauthorized)
end
end
end
......
......@@ -72,7 +72,7 @@ RSpec.describe API::Terraform::State do
let(:auth_header) { job_basic_auth_header(job) }
context 'with maintainer permissions' do
let(:job) { create(:ci_build, project: project, user: maintainer) }
let(:job) { create(:ci_build, status: :running, project: project, user: maintainer) }
it 'returns terraform state belonging to a project of given state name' do
request
......@@ -81,6 +81,13 @@ RSpec.describe API::Terraform::State do
expect(response.body).to eq(state.file.read)
end
it 'returns unauthorized if the the job is not running' do
job.update!(status: :failed)
request
expect(response).to have_gitlab_http_status(:unauthorized)
end
context 'for a project that does not exist' do
let(:project_id) { '0000' }
......@@ -93,7 +100,7 @@ RSpec.describe API::Terraform::State do
end
context 'with developer permissions' do
let(:job) { create(:ci_build, project: project, user: developer) }
let(:job) { create(:ci_build, status: :running, project: project, user: developer) }
it 'returns terraform state belonging to a project of given state name' do
request
......
......@@ -106,9 +106,23 @@ RSpec.describe Ci::PipelineTriggerService do
let(:params) { { token: job.token, ref: 'master', variables: nil } }
let(:job) { create(:ci_build, :success, pipeline: pipeline, user: user) }
it 'does nothing' do
it 'does nothing', :aggregate_failures do
expect { result }.not_to change { Ci::Pipeline.count }
expect(result[:message]).to eq('Job is not running')
expect(result[:http_status]).to eq(401)
end
end
context 'when job does not have a project' do
let(:params) { { token: job.token, ref: 'master', variables: nil } }
let(:job) { create(:ci_build, status: :running, pipeline: pipeline, user: user) }
it 'does nothing', :aggregate_failures do
job.update!(project: nil)
expect { result }.not_to change { Ci::Pipeline.count }
expect(result[:message]).to eq('400 Job has to be running')
expect(result[:message]).to eq('Project has been deleted!')
expect(result[:http_status]).to eq(401)
end
end
......
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