Merge "Set allowed-projects on untrusted jobs with secrets"

This commit is contained in:
Zuul 2019-01-22 23:36:49 +00:00 committed by Gerrit Code Review
commit ac66685584
6 changed files with 156 additions and 23 deletions

View File

@ -562,6 +562,13 @@ Here is an example of two job definitions:
specified in a project's pipeline, set this attribute to
``true``.
.. warning::
It is possible to circumvent the use of `final` in an
:term:`untrusted-project` by creating a change which
`Depends-On` a change which alters `final`. This limitation
does not apply to jobs in a :term:`config-project`.
.. attr:: protected
:default: false
@ -569,12 +576,28 @@ Here is an example of two job definitions:
from this job. Once this is set to ``true`` it cannot be reset to
``false``.
.. warning::
It is possible to circumvent the use of `protected` in an
:term:`untrusted-project` by creating a change which
`Depends-On` a change which alters `protected`. This
limitation does not apply to jobs in a
:term:`config-project`.
.. attr:: abstract
:default: false
To indicate a job is not intended to be run directly, but
instead must be inherited from, set this attribute to ``true``.
.. warning::
It is possible to circumvent the use of `abstract` in an
:term:`untrusted-project` by creating a change which
`Depends-On` a change which alters `abstract`. This
limitation does not apply to jobs in a
:term:`config-project`.
.. attr:: success-message
:default: SUCCESS
@ -1009,6 +1032,19 @@ Here is an example of two job definitions:
it should be able to run this job, then it must be explicitly
listed. By default, all projects may use the job.
If a :attr:`job.secrets` is used in a job definition in an
:term:`untrusted-project`, `allowed-projects` is automatically
set to the current project only, and can not be overridden.
.. warning::
It is possible to circumvent the use of `allowed-projects` in
an :term:`untrusted-project` by creating a change which
`Depends-On` a change which alters `allowed-projects`. This
limitation does not apply to jobs in a
:term:`config-project`, or jobs in an `untrusted-project`
which use a secret.
.. attr:: post-review
:default: false
@ -1022,6 +1058,15 @@ Here is an example of two job definitions:
it will remain set for all child jobs and variants (it can not be
set to ``false``).
.. warning::
It is possible to circumvent the use of `post-review` in an
:term:`untrusted-project` by creating a change which
`Depends-On` a change which alters `post-review`. This
limitation does not apply to jobs in a
:term:`config-project`, or jobs in an `untrusted-project`
which use a secret.
.. attr:: branches
A regular expression (or list of regular expressions) which
@ -1372,7 +1417,9 @@ indicate the job should only run in post-review pipelines.
If a job with secrets is unsafe to be used by other projects, the
:attr:`job.allowed-projects` attribute can be used to restrict the
projects which can invoke that job.
projects which can invoke that job. If a job with secrets is defined
in an `untrusted-project`, `allowed-projects` is automatically set to
that project only, and can not be overridden.
Secrets, like most configuration items, are unique within a tenant,
though a secret may be defined on multiple branches of the same

View File

@ -0,0 +1,23 @@
---
security:
- |
Jobs with secrets in untrusted projects now automatically set
`allowed-projects`.
It is possible to circumvent the use of `allowed-projects` in
untrusted projects by creating a change which `Depends-On` a
change which alters a project definition. This behavior may be
unexpected, so documentation has been updated with warnings to
avoid relying on it in sensitive cases.
It may have been possible to expose a secret, or use resources
protected by a secret, if a job using a secret was defined in an
untrusted project on a system with an independent pre-merge
post-review pipeline -- that is, a pipeline with `post-review` set
to true, `manager` set to `independent`, and which operated on
changes before they merged.
To prevent disclosure or use in this situation, `allowed-projects`
is now automatically set to the current project when a secret is
used in a job defined in an untrusted project, and it can not be
overridden.

View File

@ -3,6 +3,12 @@
parent: restricted-job
allowed-projects:
- org/project2
- job:
name: test-project2b
parent: restricted-job
allowed-projects:
- org/project2
- project:
name: org/project2

View File

@ -62,11 +62,6 @@
- trusted-secrets
- trusted-secrets-trusted-child
- trusted-secrets-untrusted-child
gate:
jobs:
- untrusted-secrets
- untrusted-secrets-trusted-child
- untrusted-secrets-untrusted-child
- secret:
name: trusted-secret

View File

@ -728,6 +728,77 @@ class TestAllowedProjects(ZuulTestCase):
dict(name='restricted-job', result='SUCCESS', changes='1,1'),
], ordered=False)
def test_allowed_projects_dynamic_config(self):
# It is possible to circumvent allowed-projects with a
# depends-on.
in_repo_conf2 = textwrap.dedent(
"""
- job:
name: test-project2b
parent: restricted-job
allowed-projects:
- org/project1
""")
in_repo_conf1 = textwrap.dedent(
"""
- project:
check:
jobs:
- test-project2b
""")
file_dict = {'zuul.yaml': in_repo_conf2}
A = self.fake_gerrit.addFakeChange('org/project2', 'master', 'A',
files=file_dict)
file_dict = {'zuul.yaml': in_repo_conf1}
B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B',
files=file_dict)
B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
B.subject, A.data['id'])
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([
dict(name='test-project2b', result='SUCCESS', changes='1,1 2,1'),
], ordered=False)
def test_allowed_projects_dynamic_config_secret(self):
# It is not possible to circumvent allowed-projects with a
# depends-on if there is a secret involved.
in_repo_conf2 = textwrap.dedent(
"""
- secret:
name: project2_secret
data: {}
- job:
name: test-project2b
parent: restricted-job
secrets: project2_secret
allowed-projects:
- org/project1
""")
in_repo_conf1 = textwrap.dedent(
"""
- project:
check:
jobs:
- test-project2b
""")
file_dict = {'zuul.yaml': in_repo_conf2}
A = self.fake_gerrit.addFakeChange('org/project2', 'master', 'A',
files=file_dict)
file_dict = {'zuul.yaml': in_repo_conf1}
B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B',
files=file_dict)
B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
B.subject, A.data['id'])
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([])
self.assertEqual(B.reported, 1)
self.assertIn('Project org/project1 is not allowed '
'to run job test-project2b', B.messages[0])
class TestCentralJobs(ZuulTestCase):
tenant_config_file = 'config/central-jobs/main.yaml'
@ -3764,21 +3835,6 @@ class TestSecretInheritance(ZuulTestCase):
self._checkTrustedSecrets()
def test_untrusted_secret_inheritance_gate(self):
A = self.fake_gerrit.addFakeChange('common-config', 'master', 'A')
A.addApproval('Code-Review', 2)
self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
self.waitUntilSettled()
self.assertHistory([
dict(name='untrusted-secrets', result='SUCCESS', changes='1,1'),
dict(name='untrusted-secrets-trusted-child',
result='SUCCESS', changes='1,1'),
dict(name='untrusted-secrets-untrusted-child',
result='SUCCESS', changes='1,1'),
], ordered=False)
self._checkUntrustedSecrets()
def test_untrusted_secret_inheritance_check(self):
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))

View File

@ -662,9 +662,14 @@ class JobParser(object):
# A job in an untrusted repo that uses secrets requires
# special care. We must note this, and carry this flag
# through inheritance to ensure that we don't run this job in
# an unsafe check pipeline.
# an unsafe check pipeline. We must also set allowed-projects
# to only the current project, as otherwise, other projects
# might be able to cause something to happen with the secret
# by using a depends-on header.
if secrets and not conf['_source_context'].trusted:
job.post_review = True
job.allowed_projects = frozenset((
conf['_source_context'].project.name,))
if (conf.get('timeout') and
self.pcontext.tenant.max_job_timeout != -1 and
@ -798,7 +803,8 @@ class JobParser(object):
job.group_variables = group_variables
allowed_projects = conf.get('allowed-projects', None)
if allowed_projects:
# See note above at "post-review".
if allowed_projects and not job.allowed_projects:
allowed = []
for p in as_list(allowed_projects):
(trusted, project) = self.pcontext.tenant.getProject(p)