Honor independent pipeline requirements for non-live changes

Independent pipelines ignore requirements for non-live changes
because they are not actually executed.  However, a user might
configure an independent pipeline that requires code review and
expect a positive code-review pipeline requirement to be enforced.
To ignore it risks executing unreviewed code via dependencies.

To correct this, we now enforce pipeline requirements in independent
pipelines in the same way as dependent ones.

This also adds a new "allow-other-connections" pipeline configuration
option which permits users to specify exhaustive pipeline requirements.

Change-Id: I6c006f9e63a888f83494e575455395bd534b955f
Story: 2010515

15 files changed, 167 insertions, 12 deletions

diff --git a/doc/source/config/pipeline.rst b/doc/source/config/pipeline.rst
index f4d7cce69..41bedfbf2 100644
--- a/doc/source/config/pipeline.rst
+++ b/doc/source/config/pipeline.rst
@@ -266,6 +266,17 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
       type of the connection will dictate which options are available.
       See :ref:`drivers`.
 
+   .. attr:: allow-other-connections
+      :default: true
+
+      If this is set to `false` then any change enqueued into the
+      pipeline (whether it is enqueued to run jobs or merely as a
+      dependency) must be from one of the connections specified in the
+      pipeline configuration (this includes any trigger, reporter, or
+      source requirement).  When used in conjuctions with
+      :attr:`pipeline.require`, this can ensure that pipeline
+      requirements are exhaustive.
+
    .. attr:: supercedes
 
       The name of a pipeline, or a list of names, that this pipeline
diff --git a/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml b/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml
new file mode 100644
index 000000000..052d5b255
--- /dev/null
+++ b/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml
@@ -0,0 +1,29 @@
+---
+features:
+  - |
+    A new pipeline attribute,
+    :attr:`pipeline.allow-other-connections`, has been added
+    to ensure that only changes from connections which
+    are mentioned in the pipeline configuration (such as triggers,
+    reporters, or pipeline requirements) are enqueued.
+security:
+  - |
+    Non-live items are now subject to pipeline requirements for
+    independent pipelines.
+
+    Previously, an optimization for independent pipelines skipped
+    checking that a change met the pipeline requirements.  If an
+    independent pipeline is intended only to run reviewed code, this
+    could allow running unreviewed code by updating dependent changes.
+
+    Now both non-live and live items are subject to pipeline
+    requirements in all pipeline managers.
+
+  - |
+    The new `allow-other-connections` pipeline configuration option
+    may now be used to ensure that only changes from connections which
+    are mentioned in the pipeline configuration (such as triggers,
+    reporters, or pipeline requirements) are enqueued.  This allows
+    the construction of a pipeline where, for example, code review
+    requirements are strictly enforced, even for dependencies which
+    are not normally directly enqueued.
diff --git a/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml b/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml
new file mode 100644
index 000000000..f679dceae
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml
@@ -0,0 +1,2 @@
+- hosts: all
+  tasks: []
diff --git a/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml b/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml
new file mode 100644
index 000000000..494cd3cc7
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml
@@ -0,0 +1,41 @@
+- pipeline:
+    name: trusted-check
+    manager: independent
+    allow-other-connections: false
+    require:
+      gerrit:
+        approval:
+          - Code-Review: 2
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null
+    run: playbooks/base.yaml
+    nodeset:
+      nodes:
+        - label: ubuntu-xenial
+          name: controller
+
+- job:
+    name: check-job
+
+- project:
+    name: org/project
+    trusted-check:
+      jobs:
+        - check-job
+
+- project:
+    name: gh/project
+    trusted-check:
+      jobs:
+        - check-job
diff --git a/tests/fixtures/config/requirements/trusted-check/git/gh_project/README b/tests/fixtures/config/requirements/trusted-check/git/gh_project/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/gh_project/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/requirements/trusted-check/git/org_project/README b/tests/fixtures/config/requirements/trusted-check/git/org_project/README
new file mode 100644
index 000000000..9daeafb98
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/git/org_project/README
@@ -0,0 +1 @@
+test
diff --git a/tests/fixtures/config/requirements/trusted-check/main.yaml b/tests/fixtures/config/requirements/trusted-check/main.yaml
new file mode 100644
index 000000000..c8deb36c1
--- /dev/null
+++ b/tests/fixtures/config/requirements/trusted-check/main.yaml
@@ -0,0 +1,11 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project
+      github:
+        untrusted-projects:
+          - gh/project
diff --git a/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml b/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml
index 3fcc43ce6..975b9713e 100644
--- a/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml
+++ b/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml
@@ -4,7 +4,7 @@
     require:
       gerrit:
         approval:
-          - Verified: -1
+          - email: for-check@example.com
     trigger:
       gerrit:
         - event: patchset-created
@@ -24,7 +24,7 @@
     require:
       gerrit:
         approval:
-          - Verified: 1
+          - email: for-gate@example.com
     trigger:
       gerrit:
         - event: comment-added
diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py
index 9a32e4b21..9f3b87187 100644
--- a/tests/unit/test_requirements.py
+++ b/tests/unit/test_requirements.py
@@ -453,3 +453,40 @@ class TestRequirementsReject(ZuulTestCase):
         self.fake_gerrit.addEvent(comment)
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 3)
+
+
+class TestRequirementsTrustedCheck(ZuulTestCase):
+    config_file = "zuul-gerrit-github.conf"
+    tenant_config_file = "config/requirements/trusted-check/main.yaml"
+
+    def test_non_live_requirements(self):
+        # Test that pipeline requirements are applied to non-live
+        # changes.
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.setDependsOn(A, 1)
+        B.addApproval('Code-Review', 2)
+
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([])
+
+        self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2))
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='check-job', result='SUCCESS', changes='1,1 2,1')],
+            ordered=False)
+
+    def test_other_connections(self):
+        # Test allow-other-connections: False
+        A = self.fake_github.openFakePullRequest("gh/project", "master", "A")
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.url,
+        )
+        B.addApproval('Code-Review', 2)
+
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([])
diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index 6a0da1279..60c92cdd0 100644
--- a/tests/unit/test_scheduler.py
+++ b/tests/unit/test_scheduler.py
@@ -2807,6 +2807,7 @@ class TestScheduler(ZuulTestCase):
         B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
         B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
             B.subject, A.data['url'])
+        A.addApproval('Code-Review', 2)
         B.addApproval('Code-Review', 2)
 
         self.executor_server.hold_jobs_in_build = True
diff --git a/tests/unit/test_zuultrigger.py b/tests/unit/test_zuultrigger.py
index 25bfd4c30..f649f4723 100644
--- a/tests/unit/test_zuultrigger.py
+++ b/tests/unit/test_zuultrigger.py
@@ -35,9 +35,10 @@ class TestZuulTriggerParentChangeEnqueued(ZuulTestCase):
         A.addApproval('Code-Review', 2)
         B1.addApproval('Code-Review', 2)
         B2.addApproval('Code-Review', 2)
-        A.addApproval('Verified', 1)    # required by gate
-        B1.addApproval('Verified', -1)  # should go to check
-        B2.addApproval('Verified', 1)   # should go to gate
+        A.addApproval('Verified', 1, username="for-check")   # reqd by check
+        A.addApproval('Verified', 1, username="for-gate")    # reqd by gate
+        B1.addApproval('Verified', 1, username="for-check")  # go to check
+        B2.addApproval('Verified', 1, username="for-gate")   # go to gate
         B1.addApproval('Approved', 1)
         B2.addApproval('Approved', 1)
         B1.setDependsOn(A, 1)
@@ -75,9 +76,9 @@ class TestZuulTriggerParentChangeEnqueued(ZuulTestCase):
             self.scheds.first.sched, "addTriggerEvent", addTriggerEvent
         ):
             C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C')
-            C.addApproval('Verified', -1)
+            C.addApproval('Verified', 1, username="for-check")
             D = self.fake_gerrit.addFakeChange('org/project', 'master', 'D')
-            D.addApproval('Verified', -1)
+            D.addApproval('Verified', 1, username="for-check")
             D.setDependsOn(C, 1)
             self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
 
@@ -108,6 +109,7 @@ class TestZuulTriggerParentChangeEnqueuedGithub(ZuulGithubAppTestCase):
         B1.addReview('derp', 'APPROVED')
         B2.addReview('derp', 'APPROVED')
         A.addLabel('for-gate')    # required by gate
+        A.addLabel('for-check')   # required by check
         B1.addLabel('for-check')  # should go to check
         B2.addLabel('for-gate')   # should go to gate
 
diff --git a/zuul/configloader.py b/zuul/configloader.py
index 08b517618..7f8346382 100644
--- a/zuul/configloader.py
+++ b/zuul/configloader.py
@@ -1292,6 +1292,7 @@ class PipelineParser(object):
 
         pipeline = {vs.Required('name'): str,
                     vs.Required('manager'): manager,
+                    'allow-other-connections': bool,
                     'precedence': precedence,
                     'supercedes': to_list(str),
                     'description': str,
@@ -1331,6 +1332,8 @@ class PipelineParser(object):
         pipeline = model.Pipeline(conf['name'], self.pcontext.tenant)
         pipeline.source_context = conf['_source_context']
         pipeline.start_mark = conf['_start_mark']
+        pipeline.allow_other_connections = conf.get(
+            'allow-other-connections', True)
         pipeline.description = conf.get('description')
         pipeline.supercedes = as_list(conf.get('supercedes', []))
 
@@ -1366,6 +1369,7 @@ class PipelineParser(object):
         # Make a copy to manipulate for backwards compat.
         conf_copy = conf.copy()
 
+        seen_connections = set()
         for conf_key, action in self.reporter_actions.items():
             reporter_set = []
             allowed_reporters = self.pcontext.tenant.allowed_reporters
@@ -1379,6 +1383,7 @@ class PipelineParser(object):
                         reporter_name, pipeline, params)
                     reporter.setAction(conf_key)
                     reporter_set.append(reporter)
+                    seen_connections.add(reporter_name)
             setattr(pipeline, action, reporter_set)
 
         # If merge-conflict actions aren't explicit, use the failure actions
@@ -1423,11 +1428,13 @@ class PipelineParser(object):
             source = self.pcontext.connections.getSource(source_name)
             manager.ref_filters.extend(
                 source.getRequireFilters(require_config))
+            seen_connections.add(source_name)
 
         for source_name, reject_config in conf.get('reject', {}).items():
             source = self.pcontext.connections.getSource(source_name)
             manager.ref_filters.extend(
                 source.getRejectFilters(reject_config))
+            seen_connections.add(source_name)
 
         for connection_name, trigger_config in conf.get('trigger').items():
             if self.pcontext.tenant.allowed_triggers is not None and \
@@ -1439,7 +1446,9 @@ class PipelineParser(object):
             manager.event_filters.extend(
                 trigger.getEventFilters(connection_name,
                                         conf['trigger'][connection_name]))
+            seen_connections.add(connection_name)
 
+        pipeline.connections = list(seen_connections)
         # Pipelines don't get frozen
         return pipeline
 
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index dc3290451..d0fb10f7b 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -508,6 +508,12 @@ class PipelineManager(metaclass=ABCMeta):
             log.debug("Change %s is already in pipeline, ignoring" % change)
             return True
 
+        if ((not self.pipeline.allow_other_connections) and
+            (change.project.connection_name not in self.pipeline.connections)):
+            log.debug("Change %s is not from a connection known to %s ",
+                      change, self.pipeline)
+            return False
+
         if not ignore_requirements:
             for f in self.ref_filters:
                 if f.connection_name != change.project.connection_name:
diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py
index aeeff5adc..ca52b37fe 100644
--- a/zuul/manager/independent.py
+++ b/zuul/manager/independent.py
@@ -61,13 +61,15 @@ class IndependentPipelineManager(PipelineManager):
         for needed_change in needed_changes:
             # This differs from the dependent pipeline by enqueuing
             # changes ahead as "not live", that is, not intended to
-            # have jobs run.  Also, pipeline requirements are always
-            # ignored (which is safe because the changes are not
-            # live).
+            # have jobs run.  Pipeline requirements are still in place
+            # in order to avoid unreviewed code being executed in
+            # pipelines that require review.
             if needed_change not in history:
                 r = self.addChange(needed_change, event, quiet=True,
-                                   ignore_requirements=True, live=False,
-                                   change_queue=change_queue, history=history,
+                                   ignore_requirements=ignore_requirements,
+                                   live=False,
+                                   change_queue=change_queue,
+                                   history=history,
                                    dependency_graph=dependency_graph)
                 if not r:
                     return False
diff --git a/zuul/model.py b/zuul/model.py
index ac38c3705..0d789f976 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -438,6 +438,8 @@ class Pipeline(object):
         # reconfigured).  A pipeline requires a tenant in order to
         # reach the currently active layout for that tenant.
         self.tenant = tenant
+        self.allow_other_connections = True
+        self.connections = []
         self.source_context = None
         self.start_mark = None
         self.description = None