summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJames E. Blair <jeblair@redhat.com>2019-01-22 12:26:20 -0800
committerJames E. Blair <jeblair@redhat.com>2019-01-22 14:01:10 -0800
commited7f9da75eae24506996cf82487f1343304f9b84 (patch)
tree496927cfacaeb9b68fa263edabd7002bd7a9e968
parent6fccffe49bd9265721c9d091a69f9e08e986e938 (diff)
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
Notes
Notes (review): Code-Review+2: Monty Taylor <mordred@inaugust.com> Code-Review+2: Tobias Henkel <tobias.henkel@bmw.de> Workflow+1: Tobias Henkel <tobias.henkel@bmw.de> Verified+2: Zuul Submitted-by: Zuul Submitted-at: Tue, 22 Jan 2019 23:36:49 +0000 Reviewed-on: https://review.openstack.org/632566 Project: openstack-infra/zuul Branch: refs/heads/master
-rw-r--r--doc/source/user/config.rst49
-rw-r--r--releasenotes/notes/allowed-projects-8f6f0cb42ffd0a88.yaml23
-rw-r--r--tests/fixtures/config/allowed-projects/git/org_project2/zuul.yaml6
-rw-r--r--tests/fixtures/config/secret-inheritance/git/common-config/zuul.yaml5
-rw-r--r--tests/unit/test_v3.py86
-rw-r--r--zuul/configloader.py10
6 files changed, 156 insertions, 23 deletions
diff --git a/doc/source/user/config.rst b/doc/source/user/config.rst
index 1fb9216..04de33e 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:
562 specified in a project's pipeline, set this attribute to 562 specified in a project's pipeline, set this attribute to
563 ``true``. 563 ``true``.
564 564
565 .. warning::
566
567 It is possible to circumvent the use of `final` in an
568 :term:`untrusted-project` by creating a change which
569 `Depends-On` a change which alters `final`. This limitation
570 does not apply to jobs in a :term:`config-project`.
571
565 .. attr:: protected 572 .. attr:: protected
566 :default: false 573 :default: false
567 574
@@ -569,12 +576,28 @@ Here is an example of two job definitions:
569 from this job. Once this is set to ``true`` it cannot be reset to 576 from this job. Once this is set to ``true`` it cannot be reset to
570 ``false``. 577 ``false``.
571 578
579 .. warning::
580
581 It is possible to circumvent the use of `protected` in an
582 :term:`untrusted-project` by creating a change which
583 `Depends-On` a change which alters `protected`. This
584 limitation does not apply to jobs in a
585 :term:`config-project`.
586
572 .. attr:: abstract 587 .. attr:: abstract
573 :default: false 588 :default: false
574 589
575 To indicate a job is not intended to be run directly, but 590 To indicate a job is not intended to be run directly, but
576 instead must be inherited from, set this attribute to ``true``. 591 instead must be inherited from, set this attribute to ``true``.
577 592
593 .. warning::
594
595 It is possible to circumvent the use of `abstract` in an
596 :term:`untrusted-project` by creating a change which
597 `Depends-On` a change which alters `abstract`. This
598 limitation does not apply to jobs in a
599 :term:`config-project`.
600
578 .. attr:: success-message 601 .. attr:: success-message
579 :default: SUCCESS 602 :default: SUCCESS
580 603
@@ -1009,6 +1032,19 @@ Here is an example of two job definitions:
1009 it should be able to run this job, then it must be explicitly 1032 it should be able to run this job, then it must be explicitly
1010 listed. By default, all projects may use the job. 1033 listed. By default, all projects may use the job.
1011 1034
1035 If a :attr:`job.secrets` is used in a job definition in an
1036 :term:`untrusted-project`, `allowed-projects` is automatically
1037 set to the current project only, and can not be overridden.
1038
1039 .. warning::
1040
1041 It is possible to circumvent the use of `allowed-projects` in
1042 an :term:`untrusted-project` by creating a change which
1043 `Depends-On` a change which alters `allowed-projects`. This
1044 limitation does not apply to jobs in a
1045 :term:`config-project`, or jobs in an `untrusted-project`
1046 which use a secret.
1047
1012 .. attr:: post-review 1048 .. attr:: post-review
1013 :default: false 1049 :default: false
1014 1050
@@ -1022,6 +1058,15 @@ Here is an example of two job definitions:
1022 it will remain set for all child jobs and variants (it can not be 1058 it will remain set for all child jobs and variants (it can not be
1023 set to ``false``). 1059 set to ``false``).
1024 1060
1061 .. warning::
1062
1063 It is possible to circumvent the use of `post-review` in an
1064 :term:`untrusted-project` by creating a change which
1065 `Depends-On` a change which alters `post-review`. This
1066 limitation does not apply to jobs in a
1067 :term:`config-project`, or jobs in an `untrusted-project`
1068 which use a secret.
1069
1025 .. attr:: branches 1070 .. attr:: branches
1026 1071
1027 A regular expression (or list of regular expressions) which 1072 A regular expression (or list of regular expressions) which
@@ -1372,7 +1417,9 @@ indicate the job should only run in post-review pipelines.
1372 1417
1373If a job with secrets is unsafe to be used by other projects, the 1418If a job with secrets is unsafe to be used by other projects, the
1374:attr:`job.allowed-projects` attribute can be used to restrict the 1419:attr:`job.allowed-projects` attribute can be used to restrict the
1375projects which can invoke that job. 1420projects which can invoke that job. If a job with secrets is defined
1421in an `untrusted-project`, `allowed-projects` is automatically set to
1422that project only, and can not be overridden.
1376 1423
1377Secrets, like most configuration items, are unique within a tenant, 1424Secrets, like most configuration items, are unique within a tenant,
1378though a secret may be defined on multiple branches of the same 1425though 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 0000000..4dfd560
--- /dev/null
+++ b/releasenotes/notes/allowed-projects-8f6f0cb42ffd0a88.yaml
@@ -0,0 +1,23 @@
1---
2security:
3 - |
4 Jobs with secrets in untrusted projects now automatically set
5 `allowed-projects`.
6
7 It is possible to circumvent the use of `allowed-projects` in
8 untrusted projects by creating a change which `Depends-On` a
9 change which alters a project definition. This behavior may be
10 unexpected, so documentation has been updated with warnings to
11 avoid relying on it in sensitive cases.
12
13 It may have been possible to expose a secret, or use resources
14 protected by a secret, if a job using a secret was defined in an
15 untrusted project on a system with an independent pre-merge
16 post-review pipeline -- that is, a pipeline with `post-review` set
17 to true, `manager` set to `independent`, and which operated on
18 changes before they merged.
19
20 To prevent disclosure or use in this situation, `allowed-projects`
21 is now automatically set to the current project when a secret is
22 used in a job defined in an untrusted project, and it can not be
23 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 bf0f07a..d2e2f69 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 @@
3 parent: restricted-job 3 parent: restricted-job
4 allowed-projects: 4 allowed-projects:
5 - org/project2 5 - org/project2
6
7- job:
8 name: test-project2b
9 parent: restricted-job
10 allowed-projects:
11 - org/project2
6 12
7- project: 13- project:
8 name: org/project2 14 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 e6162f1..eef4133 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 @@
62 - trusted-secrets 62 - trusted-secrets
63 - trusted-secrets-trusted-child 63 - trusted-secrets-trusted-child
64 - trusted-secrets-untrusted-child 64 - trusted-secrets-untrusted-child
65 gate:
66 jobs:
67 - untrusted-secrets
68 - untrusted-secrets-trusted-child
69 - untrusted-secrets-untrusted-child
70 65
71- secret: 66- secret:
72 name: trusted-secret 67 name: trusted-secret
diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py
index b9955d2..d6554a0 100644
--- a/tests/unit/test_v3.py
+++ b/tests/unit/test_v3.py
@@ -728,6 +728,77 @@ class TestAllowedProjects(ZuulTestCase):
728 dict(name='restricted-job', result='SUCCESS', changes='1,1'), 728 dict(name='restricted-job', result='SUCCESS', changes='1,1'),
729 ], ordered=False) 729 ], ordered=False)
730 730
731 def test_allowed_projects_dynamic_config(self):
732 # It is possible to circumvent allowed-projects with a
733 # depends-on.
734 in_repo_conf2 = textwrap.dedent(
735 """
736 - job:
737 name: test-project2b
738 parent: restricted-job
739 allowed-projects:
740 - org/project1
741 """)
742 in_repo_conf1 = textwrap.dedent(
743 """
744 - project:
745 check:
746 jobs:
747 - test-project2b
748 """)
749
750 file_dict = {'zuul.yaml': in_repo_conf2}
751 A = self.fake_gerrit.addFakeChange('org/project2', 'master', 'A',
752 files=file_dict)
753 file_dict = {'zuul.yaml': in_repo_conf1}
754 B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B',
755 files=file_dict)
756 B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
757 B.subject, A.data['id'])
758 self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
759 self.waitUntilSettled()
760 self.assertHistory([
761 dict(name='test-project2b', result='SUCCESS', changes='1,1 2,1'),
762 ], ordered=False)
763
764 def test_allowed_projects_dynamic_config_secret(self):
765 # It is not possible to circumvent allowed-projects with a
766 # depends-on if there is a secret involved.
767 in_repo_conf2 = textwrap.dedent(
768 """
769 - secret:
770 name: project2_secret
771 data: {}
772 - job:
773 name: test-project2b
774 parent: restricted-job
775 secrets: project2_secret
776 allowed-projects:
777 - org/project1
778 """)
779 in_repo_conf1 = textwrap.dedent(
780 """
781 - project:
782 check:
783 jobs:
784 - test-project2b
785 """)
786
787 file_dict = {'zuul.yaml': in_repo_conf2}
788 A = self.fake_gerrit.addFakeChange('org/project2', 'master', 'A',
789 files=file_dict)
790 file_dict = {'zuul.yaml': in_repo_conf1}
791 B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B',
792 files=file_dict)
793 B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
794 B.subject, A.data['id'])
795 self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
796 self.waitUntilSettled()
797 self.assertHistory([])
798 self.assertEqual(B.reported, 1)
799 self.assertIn('Project org/project1 is not allowed '
800 'to run job test-project2b', B.messages[0])
801
731 802
732class TestCentralJobs(ZuulTestCase): 803class TestCentralJobs(ZuulTestCase):
733 tenant_config_file = 'config/central-jobs/main.yaml' 804 tenant_config_file = 'config/central-jobs/main.yaml'
@@ -3764,21 +3835,6 @@ class TestSecretInheritance(ZuulTestCase):
3764 3835
3765 self._checkTrustedSecrets() 3836 self._checkTrustedSecrets()
3766 3837
3767 def test_untrusted_secret_inheritance_gate(self):
3768 A = self.fake_gerrit.addFakeChange('common-config', 'master', 'A')
3769 A.addApproval('Code-Review', 2)
3770 self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
3771 self.waitUntilSettled()
3772 self.assertHistory([
3773 dict(name='untrusted-secrets', result='SUCCESS', changes='1,1'),
3774 dict(name='untrusted-secrets-trusted-child',
3775 result='SUCCESS', changes='1,1'),
3776 dict(name='untrusted-secrets-untrusted-child',
3777 result='SUCCESS', changes='1,1'),
3778 ], ordered=False)
3779
3780 self._checkUntrustedSecrets()
3781
3782 def test_untrusted_secret_inheritance_check(self): 3838 def test_untrusted_secret_inheritance_check(self):
3783 A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') 3839 A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
3784 self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) 3840 self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
diff --git a/zuul/configloader.py b/zuul/configloader.py
index 9bc7aca..64318e2 100644
--- a/zuul/configloader.py
+++ b/zuul/configloader.py
@@ -662,9 +662,14 @@ class JobParser(object):
662 # A job in an untrusted repo that uses secrets requires 662 # A job in an untrusted repo that uses secrets requires
663 # special care. We must note this, and carry this flag 663 # special care. We must note this, and carry this flag
664 # through inheritance to ensure that we don't run this job in 664 # through inheritance to ensure that we don't run this job in
665 # an unsafe check pipeline. 665 # an unsafe check pipeline. We must also set allowed-projects
666 # to only the current project, as otherwise, other projects
667 # might be able to cause something to happen with the secret
668 # by using a depends-on header.
666 if secrets and not conf['_source_context'].trusted: 669 if secrets and not conf['_source_context'].trusted:
667 job.post_review = True 670 job.post_review = True
671 job.allowed_projects = frozenset((
672 conf['_source_context'].project.name,))
668 673
669 if (conf.get('timeout') and 674 if (conf.get('timeout') and
670 self.pcontext.tenant.max_job_timeout != -1 and 675 self.pcontext.tenant.max_job_timeout != -1 and
@@ -798,7 +803,8 @@ class JobParser(object):
798 job.group_variables = group_variables 803 job.group_variables = group_variables
799 804
800 allowed_projects = conf.get('allowed-projects', None) 805 allowed_projects = conf.get('allowed-projects', None)
801 if allowed_projects: 806 # See note above at "post-review".
807 if allowed_projects and not job.allowed_projects:
802 allowed = [] 808 allowed = []
803 for p in as_list(allowed_projects): 809 for p in as_list(allowed_projects):
804 (trusted, project) = self.pcontext.tenant.getProject(p) 810 (trusted, project) = self.pcontext.tenant.getProject(p)