From ed7f9da75eae24506996cf82487f1343304f9b84 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 22 Jan 2019 12:26:20 -0800 Subject: [PATCH] Set allowed-projects on untrusted jobs with secrets 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. The test_trusted_secret_inheritance_gate test is removed because it only tested that jobs with secrets in an untrusted repo were able to run in a trusted repo. That is no longer possible. Change-Id: I77f6a011bca08a2433137dc29597b7cc2757adb1 Story: 2004837 Task: 29037 --- doc/source/user/config.rst | 49 ++++++++++- .../allowed-projects-8f6f0cb42ffd0a88.yaml | 23 +++++ .../git/org_project2/zuul.yaml | 6 ++ .../git/common-config/zuul.yaml | 5 -- tests/unit/test_v3.py | 86 +++++++++++++++---- zuul/configloader.py | 10 ++- 6 files changed, 156 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/allowed-projects-8f6f0cb42ffd0a88.yaml diff --git a/doc/source/user/config.rst b/doc/source/user/config.rst index 1fb921619b..04de33e014 100644 --- a/doc/source/user/config.rst +++ b/doc/source/user/config.rst @@ -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 diff --git a/releasenotes/notes/allowed-projects-8f6f0cb42ffd0a88.yaml b/releasenotes/notes/allowed-projects-8f6f0cb42ffd0a88.yaml new file mode 100644 index 0000000000..4dfd5606a5 --- /dev/null +++ b/releasenotes/notes/allowed-projects-8f6f0cb42ffd0a88.yaml @@ -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. diff --git a/tests/fixtures/config/allowed-projects/git/org_project2/zuul.yaml b/tests/fixtures/config/allowed-projects/git/org_project2/zuul.yaml index bf0f07a693..d2e2f692a0 100644 --- a/tests/fixtures/config/allowed-projects/git/org_project2/zuul.yaml +++ b/tests/fixtures/config/allowed-projects/git/org_project2/zuul.yaml @@ -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 diff --git a/tests/fixtures/config/secret-inheritance/git/common-config/zuul.yaml b/tests/fixtures/config/secret-inheritance/git/common-config/zuul.yaml index e6162f1100..eef41334e1 100644 --- a/tests/fixtures/config/secret-inheritance/git/common-config/zuul.yaml +++ b/tests/fixtures/config/secret-inheritance/git/common-config/zuul.yaml @@ -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 diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index b9955d2c2b..d6554a08a7 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -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)) diff --git a/zuul/configloader.py b/zuul/configloader.py index 9bc7aca26f..64318e2658 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -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)