Commit 0cd50746 authored by John Jarvis's avatar John Jarvis

Merge branch 'security-11-5-secret-ci-variables-exposed' into 'security-11-5'

[11.5] Secret CI variables can exposed by creating a tag with the same name as an existing protected branch

See merge request gitlab/gitlabhq!2682
parents 017cfa2c 28696689
......@@ -10,6 +10,7 @@ module Ci
include Importable
include Gitlab::Utils::StrongMemoize
include Deployable
include HasRef
belongs_to :project, inverse_of: :builds
belongs_to :runner
......@@ -642,11 +643,11 @@ module Ci
def secret_group_variables
return [] unless project.group
project.group.ci_variables_for(ref, project)
project.group.ci_variables_for(git_ref, project)
end
def secret_project_variables(environment: persisted_environment)
project.ci_variables_for(ref: ref, environment: environment)
project.ci_variables_for(ref: git_ref, environment: environment)
end
def steps
......
......@@ -11,6 +11,7 @@ module Ci
include Gitlab::Utils::StrongMemoize
include AtomicInternalId
include EnumWithNil
include HasRef
belongs_to :project, inverse_of: :pipelines
belongs_to :user
......@@ -374,10 +375,6 @@ module Ci
@commit ||= Commit.lazy(project, sha)
end
def branch?
!tag?
end
def stuck?
pending_builds.any?(&:stuck?)
end
......@@ -577,7 +574,7 @@ module Ci
end
def protected_ref?
strong_memoize(:protected_ref) { project.protected_for?(ref) }
strong_memoize(:protected_ref) { project.protected_for?(git_ref) }
end
def legacy_trigger
......@@ -697,16 +694,6 @@ module Ci
end
end
def git_ref
if branch?
Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
elsif tag?
Gitlab::Git::TAG_REF_PREFIX + ref.to_s
else
raise ArgumentError, 'Invalid pipeline type!'
end
end
def latest_builds_status
return 'failed' unless yaml_errors.blank?
......
# frozen_string_literal: true
module HasRef
extend ActiveSupport::Concern
def branch?
!tag?
end
def git_ref
if branch?
Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
elsif tag?
Gitlab::Git::TAG_REF_PREFIX + ref.to_s
end
end
end
......@@ -1817,10 +1817,21 @@ class Project < ActiveRecord::Base
end
def protected_for?(ref)
if repository.branch_exists?(ref)
ProtectedBranch.protected?(self, ref)
elsif repository.tag_exists?(ref)
ProtectedTag.protected?(self, ref)
raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref)
resolved_ref = repository.expand_ref(ref) || ref
return false unless Gitlab::Git.tag_ref?(resolved_ref) || Gitlab::Git.branch_ref?(resolved_ref)
ref_name = if resolved_ref == ref
Gitlab::Git.ref_name(resolved_ref)
else
ref
end
if Gitlab::Git.branch_ref?(resolved_ref)
ProtectedBranch.protected?(self, ref_name)
elsif Gitlab::Git.tag_ref?(resolved_ref)
ProtectedTag.protected?(self, ref_name)
end
end
......
......@@ -26,6 +26,7 @@ class Repository
delegate :bundle_to_disk, to: :raw_repository
CreateTreeError = Class.new(StandardError)
AmbiguousRefError = Class.new(StandardError)
# Methods that cache data from the Git repository.
#
......@@ -176,6 +177,18 @@ class Repository
tags.find { |tag| tag.name == name }
end
def ambiguous_ref?(ref)
tag_exists?(ref) && branch_exists?(ref)
end
def expand_ref(ref)
if tag_exists?(ref)
Gitlab::Git::TAG_REF_PREFIX + ref
elsif branch_exists?(ref)
Gitlab::Git::BRANCH_REF_PREFIX + ref
end
end
def add_branch(user, branch_name, ref)
branch = raw_repository.add_branch(branch_name, user: user, target: ref)
......
---
title: Prevent leaking protected variables for ambiguous refs.
merge_request:
author:
type: security
......@@ -54,7 +54,13 @@ module Gitlab
def protected_ref?
strong_memoize(:protected_ref) do
project.protected_for?(ref)
project.protected_for?(origin_ref)
end
end
def ambiguous_ref?
strong_memoize(:ambiguous_ref) do
project.repository.ambiguous_ref?(origin_ref)
end
end
end
......
......@@ -16,6 +16,10 @@ module Gitlab
unless @command.sha
return error('Commit not found')
end
if @command.ambiguous_ref?
return error('Ref is ambiguous')
end
end
def break?
......
......@@ -54,11 +54,11 @@ module Gitlab
end
def tag_ref?(ref)
ref.start_with?(TAG_REF_PREFIX)
ref =~ /^#{TAG_REF_PREFIX}.+/
end
def branch_ref?(ref)
ref.start_with?(BRANCH_REF_PREFIX)
ref =~ /^#{BRANCH_REF_PREFIX}.+/
end
def blank_ref?(ref)
......
......@@ -182,4 +182,24 @@ describe Gitlab::Ci::Pipeline::Chain::Command do
it { is_expected.to eq(false) }
end
end
describe '#ambiguous_ref' do
let(:project) { create(:project, :repository) }
let(:command) { described_class.new(project: project, origin_ref: 'ref') }
subject { command.ambiguous_ref? }
context 'when ref is not ambiguous' do
it { is_expected. to eq(false) }
end
context 'when ref is ambiguous' do
before do
project.repository.add_tag(project.creator, 'ref', 'master')
project.repository.add_branch(project.creator, 'ref', 'master')
end
it { is_expected. to eq(true) }
end
end
end
......@@ -14,6 +14,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do
Gitlab::Ci::Pipeline::Chain::Command.new(
project: project,
current_user: user,
origin_ref: 'master',
seeds_block: nil)
end
......@@ -106,6 +107,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do
Gitlab::Ci::Pipeline::Chain::Command.new(
project: project,
current_user: user,
origin_ref: 'master',
seeds_block: seeds_block)
end
......
......@@ -42,6 +42,27 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
end
end
context 'when ref is ambiguous' do
let(:project) do
create(:project, :repository).tap do |proj|
proj.repository.add_tag(user, 'master', 'master')
end
end
let(:command) do
Gitlab::Ci::Pipeline::Chain::Command.new(
project: project, current_user: user, origin_ref: 'master')
end
it 'breaks the chain' do
expect(step.break?).to be true
end
it 'adds an error about missing ref' do
expect(pipeline.errors.to_a)
.to include 'Ref is ambiguous'
end
end
context 'when does not have existing SHA set' do
let(:command) do
Gitlab::Ci::Pipeline::Chain::Command.new(
......
require 'spec_helper'
describe Gitlab::Ci::Pipeline::Seed::Build do
let(:pipeline) { create(:ci_empty_pipeline) }
let(:project) { create(:project, :repository) }
let(:pipeline) { create(:ci_empty_pipeline, project: project) }
let(:attributes) do
{ name: 'rspec',
......
require 'spec_helper'
describe Gitlab::Ci::Pipeline::Seed::Stage do
let(:pipeline) { create(:ci_empty_pipeline) }
let(:project) { create(:project, :repository) }
let(:pipeline) { create(:ci_empty_pipeline, project: project) }
let(:attributes) do
{ name: 'test',
......
......@@ -2275,6 +2275,8 @@ describe Ci::Build do
end
context 'when protected variable is defined' do
let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref }
let(:protected_variable) do
{ key: 'PROTECTED_KEY', value: 'protected_value', public: false }
end
......@@ -2287,7 +2289,7 @@ describe Ci::Build do
context 'when the branch is protected' do
before do
allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
end
it { is_expected.to include(protected_variable) }
......@@ -2295,7 +2297,7 @@ describe Ci::Build do
context 'when the tag is protected' do
before do
allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
end
it { is_expected.to include(protected_variable) }
......@@ -2320,6 +2322,8 @@ describe Ci::Build do
end
context 'when group protected variable is defined' do
let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref }
let(:protected_variable) do
{ key: 'PROTECTED_KEY', value: 'protected_value', public: false }
end
......@@ -2332,7 +2336,7 @@ describe Ci::Build do
context 'when the branch is protected' do
before do
allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
end
it { is_expected.to include(protected_variable) }
......@@ -2340,7 +2344,7 @@ describe Ci::Build do
context 'when the tag is protected' do
before do
allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
end
it { is_expected.to include(protected_variable) }
......@@ -2615,7 +2619,7 @@ describe Ci::Build do
allow_any_instance_of(Project)
.to receive(:ci_variables_for)
.with(ref: 'master', environment: nil) do
.with(ref: 'refs/heads/master', environment: nil) do
[create(:ci_variable, key: 'secret', value: 'value')]
end
......
......@@ -227,6 +227,10 @@ describe Ci::Pipeline, :mailer do
end
describe '#protected_ref?' do
before do
pipeline.project = create(:project, :repository)
end
it 'delegates method to project' do
expect(pipeline).not_to be_protected_ref
end
......
# frozen_string_literal: true
require 'spec_helper'
describe HasRef do
describe '#branch?' do
let(:build) { create(:ci_build) }
subject { build.branch? }
context 'is not a tag' do
before do
build.tag = false
end
it 'return true when tag is set to false' do
is_expected.to be_truthy
end
end
context 'is not a tag' do
before do
build.tag = true
end
it 'return false when tag is set to true' do
is_expected.to be_falsey
end
end
end
describe '#git_ref' do
subject { build.git_ref }
context 'when tag is true' do
let(:build) { create(:ci_build, tag: true) }
it 'returns a tag ref' do
is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX)
end
end
context 'when tag is false' do
let(:build) { create(:ci_build, tag: false) }
it 'returns a branch ref' do
is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
end
end
context 'when tag is nil' do
let(:build) { create(:ci_build, tag: nil) }
it 'returns a branch ref' do
is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
end
end
end
end
......@@ -2482,6 +2482,10 @@ describe Project do
end
context 'when the ref is not protected' do
before do
allow(project).to receive(:protected_for?).with('ref').and_return(false)
end
it 'contains only the CI variables' do
is_expected.to contain_exactly(ci_variable)
end
......@@ -2521,42 +2525,139 @@ describe Project do
end
describe '#protected_for?' do
let(:project) { create(:project) }
let(:project) { create(:project, :repository) }
subject { project.protected_for?('ref') }
subject { project.protected_for?(ref) }
context 'when the ref is not protected' do
shared_examples 'ref is not protected' do
before do
stub_application_setting(
default_branch_protection: Gitlab::Access::PROTECTION_NONE)
end
it 'returns false' do
is_expected.to be_falsey
is_expected.to be false
end
end
context 'when the ref is a protected branch' do
shared_examples 'ref is protected branch' do
before do
allow(project).to receive(:repository).and_call_original
allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(true)
create(:protected_branch, name: 'ref', project: project)
create(:protected_branch, name: 'master', project: project)
end
it 'returns true' do
is_expected.to be_truthy
is_expected.to be true
end
end
context 'when the ref is a protected tag' do
shared_examples 'ref is protected tag' do
before do
allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(false)
allow(project).to receive_message_chain(:repository, :tag_exists?).and_return(true)
create(:protected_tag, name: 'ref', project: project)
create(:protected_tag, name: 'v1.0.0', project: project)
end
it 'returns true' do
is_expected.to be_truthy
is_expected.to be true
end
end
context 'when ref is nil' do
let(:ref) { nil }
it 'returns false' do
is_expected.to be false
end
end
context 'when ref is ref name' do
context 'when ref is ambiguous' do
let(:ref) { 'ref' }
before do
project.repository.add_branch(project.creator, 'ref', 'master')
project.repository.add_tag(project.creator, 'ref', 'master')
end
it 'raises an error' do
expect { subject }.to raise_error(Repository::AmbiguousRefError)
end
end
context 'when the ref is not protected' do
let(:ref) { 'master' }
it_behaves_like 'ref is not protected'
end
context 'when the ref is a protected branch' do
let(:ref) { 'master' }
it_behaves_like 'ref is protected branch'
end
context 'when the ref is a protected tag' do
let(:ref) { 'v1.0.0' }
it_behaves_like 'ref is protected tag'
end
context 'when ref does not exist' do
let(:ref) { 'something' }
it 'returns false' do
is_expected.to be false
end
end
end
context 'when ref is full ref' do
context 'when the ref is not protected' do
let(:ref) { 'refs/heads/master' }
it_behaves_like 'ref is not protected'
end
context 'when the ref is a protected branch' do
let(:ref) { 'refs/heads/master' }
it_behaves_like 'ref is protected branch'
end
context 'when the ref is a protected tag' do
let(:ref) { 'refs/tags/v1.0.0' }
it_behaves_like 'ref is protected tag'
end
context 'when branch ref name is a full tag ref' do
let(:ref) { 'refs/tags/something' }
before do
project.repository.add_branch(project.creator, ref, 'master')
end
context 'when ref is not protected' do
it 'returns false' do
is_expected.to be false
end
end
context 'when ref is a protected branch' do
before do
create(:protected_branch, name: 'refs/tags/something', project: project)
end
it 'returns true' do
is_expected.to be true
end
end
end
context 'when ref does not exist' do
let(:ref) { 'refs/heads/something' }
it 'returns false' do
is_expected.to be false
end
end
end
end
......@@ -2765,7 +2866,7 @@ describe Project do
it 'shows full error updating an invalid MR' do
error_message = 'Failed to replace merge_requests because one or more of the new records could not be saved.'\
' Validate fork Source project is not a fork of the target project'
' Validate fork Source project is not a fork of the target project'
expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) }
.to raise_error(ActiveRecord::RecordNotSaved, error_message)
......
......@@ -1066,6 +1066,67 @@ describe Repository do
end
end
describe '#ambiguous_ref?' do
let(:ref) { 'ref' }
subject { repository.ambiguous_ref?(ref) }
context 'when ref is ambiguous' do
before do
repository.add_tag(project.creator, ref, 'master')
repository.add_branch(project.creator, ref, 'master')
end
it 'should be true' do
is_expected.to eq(true)
end
end
context 'when ref is not ambiguous' do
before do
repository.add_tag(project.creator, ref, 'master')
end
it 'should be false' do
is_expected.to eq(false)
end
end
end
describe '#expand_ref' do
let(:ref) { 'ref' }
subject { repository.expand_ref(ref) }
context 'when ref is not tag or branch name' do
let(:ref) { 'refs/heads/master' }
it 'returns nil' do
is_expected.to eq(nil)
end
end
context 'when ref is tag name' do
before do
repository.add_tag(project.creator, ref, 'master')
end
it 'returns the tag ref' do
is_expected.to eq("refs/tags/#{ref}")
end
end
context 'when ref is branch name' do
before do
repository.add_branch(project.creator, ref, 'master')
end
it 'returns the branch ref' do
is_expected.to eq("refs/heads/#{ref}")
end
end
end
describe '#add_branch' do
let(:branch_name) { 'new_feature' }
let(:target) { 'master' }
......
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