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

@@ -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

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.

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

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

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))

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)