Handle secrets in branches

There were two problems with secrets related to branches.  First,
a secret defined in one branch could not be used in another.  This
is because the isSameProject method was a bit overzealous and also
ensured the secrets were on the same branch.  Relaxing that allows
secrets to be used by jobs defined in multiple branches of the same
project.

Second, because secrets are required to be globally unique, the
expected workflow of branching a project would immediately produce
a configuration error since the secret would already be defined.
To handle this case, allow multiple definitions of a secret, but
only if they are in multiple branches of the same project, and only
if they have the same data.  This should facilitate this workflow,
as well as the ability to age-out secrets on old branches.

We do not support different values for the same secret name on
different branches.

Story: 2001443
Task: 6154
Story: 2001442
Task: 6153
Change-Id: Ia9d5b77d1ce46e6461b370e951301ede4045bbb9

doc/source/user/config.rst

@@ -1137,8 +1137,12 @@ In order to maintain the security of the data, the values are usually
 encrypted, however, data which are not sensitive may be provided
 unencrypted as well for convenience.
 
-A Secret may only be used by jobs defined within the same project.  To
-use a secret, a :ref:`job` must specify the secret in
+A Secret may only be used by jobs defined within the same project.
+Note that they can be used by any branch of that project, so if a
+project's branches have different access controls, consider whether
+all branches of that project are equally trusted before using secrets.
+
+To use a secret, a :ref:`job` must specify the secret in
 :attr:`job.secrets`.  Secrets are bound to the playbooks associated
 with the specific job definition where they were declared.  Additional
 pre or post playbooks which appear in child jobs will not have access
@@ -1175,6 +1179,12 @@ If a job with secrets is unsafe to be used by other projects, the
 `allowed-projects` job attribute can be used to restrict the projects
 which can invoke that job.
 
+Secrets, like most configuration items, are globally unique, though a
+secret may be defined on multiple branches of the same project as long
+as the contents are the same.  This is to aid in branch maintenance,
+so that creating a new branch based on an existing branch will not
+immediately produce a configuration error.
+
 .. attr:: secret
 
    The following attributes must appear on a secret:

tests/fixtures/config/secrets/git/common-config/zuul.yaml

@@ -0,0 +1,38 @@
+- pipeline:
+    name: check
+    manager: independent
+    post-review: true
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- pipeline:
+    name: gate
+    manager: dependent
+    success-message: Build succeeded (gate).
+    trigger:
+      gerrit:
+        - event: comment-added
+          approval:
+            - Approved: 1
+    success:
+      gerrit:
+        Verified: 2
+        submit: true
+    failure:
+      gerrit:
+        Verified: -2
+    start:
+      gerrit:
+        Verified: 0
+    precedence: high
+
+- job:
+    name: base
+    parent: null

tests/fixtures/config/secrets/git/org_project1/README

@@ -0,0 +1 @@
+test

tests/fixtures/config/secrets/git/org_project1/playbooks/secret.yaml

@@ -0,0 +1,2 @@
+- hosts: all
+  tasks: []

tests/fixtures/config/secrets/git/org_project1/zuul.yaml

@@ -0,0 +1,26 @@
+- secret:
+    name: project1_secret
+    data:
+      username: test-username
+      password: !encrypted/pkcs1-oaep |
+        BFhtdnm8uXx7kn79RFL/zJywmzLkT1GY78P3bOtp4WghUFWobkifSu7ZpaV4NeO0s71YUsi1wGZZ
+        L0LveZjUN0t6OU1VZKSG8R5Ly7urjaSo1pPVIq5Rtt/H7W14Lecd+cUeKb4joeusC9drN3AA8a4o
+        ykcVpt1wVqUnTbMGC9ARMCQP6eopcs1l7tzMseprW4RDNhIuz3CRgd0QBMPl6VDoFgBPB8vxtJw+
+        3m0rqBYZCLZgCXekqlny8s2s92nJMuUABbJOEcDRarzibDsSXsfJt1y+5n7yOURsC7lovMg4GF/v
+        Cl/0YMKjBO5bpv9EM5fToeKYyPGSKQoHOnCYceb3cAVcv5UawcCic8XjhEhp4K7WPdYf2HVAC/qt
+        xhbpjTxG4U5Q/SoppOJ60WqEkQvbXs6n5Dvy7xmph6GWmU/bAv3eUK3pdD3xa2Ue1lHWz3U+rsYr
+        aI+AKYsMYx3RBlfAmCeC1ve2BXPrqnOo7G8tnUvfdYPbK4Aakk0ds/AVqFHEZN+S6hRBmBjLaRFW
+        Z3QSO1NjbBxWnaHKZYT7nkrJm8AMCgZU0ZArFLpaufKCeiK5ECSsDxic4FIsY1OkWT42qEUfL0Wd
+        +150AKGNZpPJnnP3QYY4W/MWcKH/zdO400+zWN52WevbSqZy90tqKDJrBkMl1ydqbuw1E4ZHvIs=
+
+- job:
+    parent: base
+    name: project1-secret
+    run: playbooks/secret.yaml
+    secrets:
+      - project1_secret
+
+- project:
+    check:
+      jobs:
+        - project1-secret

tests/fixtures/config/secrets/git/org_project2/README

@@ -0,0 +1 @@
+test

tests/fixtures/config/secrets/git/org_project2/playbooks/secret.yaml

@@ -0,0 +1,2 @@
+- hosts: all
+  tasks: []

tests/fixtures/config/secrets/git/org_project2/zuul-secret.yaml

@@ -0,0 +1,29 @@
+- secret:
+    name: project2_secret
+    data:
+      username: test-username
+      password: !encrypted/pkcs1-oaep |
+        BFhtdnm8uXx7kn79RFL/zJywmzLkT1GY78P3bOtp4WghUFWobkifSu7ZpaV4NeO0s71YUsi1wGZZ
+        L0LveZjUN0t6OU1VZKSG8R5Ly7urjaSo1pPVIq5Rtt/H7W14Lecd+cUeKb4joeusC9drN3AA8a4o
+        ykcVpt1wVqUnTbMGC9ARMCQP6eopcs1l7tzMseprW4RDNhIuz3CRgd0QBMPl6VDoFgBPB8vxtJw+
+        3m0rqBYZCLZgCXekqlny8s2s92nJMuUABbJOEcDRarzibDsSXsfJt1y+5n7yOURsC7lovMg4GF/v
+        Cl/0YMKjBO5bpv9EM5fToeKYyPGSKQoHOnCYceb3cAVcv5UawcCic8XjhEhp4K7WPdYf2HVAC/qt
+        xhbpjTxG4U5Q/SoppOJ60WqEkQvbXs6n5Dvy7xmph6GWmU/bAv3eUK3pdD3xa2Ue1lHWz3U+rsYr
+        aI+AKYsMYx3RBlfAmCeC1ve2BXPrqnOo7G8tnUvfdYPbK4Aakk0ds/AVqFHEZN+S6hRBmBjLaRFW
+        Z3QSO1NjbBxWnaHKZYT7nkrJm8AMCgZU0ZArFLpaufKCeiK5ECSsDxic4FIsY1OkWT42qEUfL0Wd
+        +150AKGNZpPJnnP3QYY4W/MWcKH/zdO400+zWN52WevbSqZy90tqKDJrBkMl1ydqbuw1E4ZHvIs=
+
+- job:
+    parent: base
+    name: project2-secret
+    run: playbooks/secret.yaml
+    secrets:
+      - project2_secret
+
+- project:
+    check:
+      jobs:
+        - project2-secret
+    gate:
+      jobs:
+        - noop

tests/fixtures/config/secrets/git/org_project2/zuul.yaml

@@ -0,0 +1,12 @@
+- job:
+    parent: base
+    name: project2-secret
+    run: playbooks/secret.yaml
+
+- project:
+    check:
+      jobs:
+        - project2-secret
+    gate:
+      jobs:
+        - noop

tests/fixtures/config/secrets/main.yaml

@@ -0,0 +1,9 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project1
+          - org/project2

tests/unit/test_v3.py

@@ -2528,6 +2528,157 @@ class TestBaseJobs(ZuulTestCase):
         self.assertHistory([])
 
 
+class TestSecrets(ZuulTestCase):
+    tenant_config_file = 'config/secrets/main.yaml'
+    secret = {'password': 'test-password',
+              'username': 'test-username'}
+
+    def _getSecrets(self, job, pbtype):
+        secrets = []
+        build = self.getJobFromHistory(job)
+        for pb in build.parameters[pbtype]:
+            secrets.append(pb['secrets'])
+        return secrets
+
+    def test_secret_branch(self):
+        # Test that we can use a secret defined in another branch of
+        # the same project.
+        self.create_branch('org/project2', 'stable')
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project2', 'stable'))
+        self.waitUntilSettled()
+
+        with open(os.path.join(FIXTURE_DIR,
+                               'config/secrets/git/',
+                               'org_project2/zuul-secret.yaml')) as f:
+            config = f.read()
+
+        file_dict = {'zuul.yaml': config}
+        A = self.fake_gerrit.addFakeChange('org/project2', 'master', 'A',
+                                           files=file_dict)
+        A.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
+        self.waitUntilSettled()
+        self.assertEqual(A.data['status'], 'MERGED')
+        self.fake_gerrit.addEvent(A.getChangeMergedEvent())
+        self.waitUntilSettled()
+
+        in_repo_conf = textwrap.dedent(
+            """
+            - job:
+                parent: base
+                name: project2-secret
+                run: playbooks/secret.yaml
+                secrets: [project2_secret]
+
+            - project:
+                check:
+                  jobs:
+                    - project2-secret
+                gate:
+                  jobs:
+                    - noop
+            """)
+        file_dict = {'zuul.yaml': in_repo_conf}
+        B = self.fake_gerrit.addFakeChange('org/project2', 'stable', 'B',
+                                           files=file_dict)
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertEqual(B.reported, 1, "B should report success")
+        self.assertHistory([
+            dict(name='project2-secret', result='SUCCESS', changes='2,1'),
+        ])
+        self.assertEqual(
+            self._getSecrets('project2-secret', 'playbooks'),
+            [{'project2_secret': self.secret}])
+
+    def test_secret_branch_duplicate(self):
+        # Test that we can create a duplicate secret on a different
+        # branch of the same project -- i.e., that when we branch
+        # master to stable on a project with a secret, nothing
+        # changes.
+        self.create_branch('org/project1', 'stable')
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project1', 'stable'))
+        self.waitUntilSettled()
+
+        A = self.fake_gerrit.addFakeChange('org/project1', 'stable', 'A')
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertEqual(A.reported, 1,
+                         "A should report success")
+        self.assertHistory([
+            dict(name='project1-secret', result='SUCCESS', changes='1,1'),
+        ])
+        self.assertEqual(
+            self._getSecrets('project1-secret', 'playbooks'),
+            [{'project1_secret': self.secret}])
+
+    def test_secret_branch_error_same_branch(self):
+        # Test that we are unable to define a secret twice on the same
+        # project-branch.
+        in_repo_conf = textwrap.dedent(
+            """
+            - secret:
+                name: project1_secret
+                data: {}
+            - secret:
+                name: project1_secret
+                data: {}
+            """)
+        file_dict = {'zuul.yaml': in_repo_conf}
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A',
+                                           files=file_dict)
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertIn('already defined', A.messages[0])
+
+    def test_secret_branch_error_same_project(self):
+        # Test that we are unable to create a secret which differs
+        # from another with the same name -- i.e., that if we have a
+        # duplicate secret on multiple branches of the same project,
+        # they must be identical.
+        self.create_branch('org/project1', 'stable')
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project1', 'stable'))
+        self.waitUntilSettled()
+
+        in_repo_conf = textwrap.dedent(
+            """
+            - secret:
+                name: project1_secret
+                data: {}
+            """)
+        file_dict = {'zuul.yaml': in_repo_conf}
+        A = self.fake_gerrit.addFakeChange('org/project1', 'stable', 'A',
+                                           files=file_dict)
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertIn('does not match existing definition in branch master',
+                      A.messages[0])
+
+    def test_secret_branch_error_other_project(self):
+        # Test that we are unable to create a secret with the same
+        # name as another.  We're never allowed to have a secret with
+        # the same name outside of a project.
+        in_repo_conf = textwrap.dedent(
+            """
+            - secret:
+                name: project1_secret
+                data: {}
+            """)
+        file_dict = {'zuul.yaml': in_repo_conf}
+        A = self.fake_gerrit.addFakeChange('org/project2', 'master', 'A',
+                                           files=file_dict)
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertIn('already defined in project org/project1',
+                      A.messages[0])
+
+
 class TestSecretInheritance(ZuulTestCase):
     tenant_config_file = 'config/secret-inheritance/main.yaml'
 

zuul/configloader.py

@@ -1597,7 +1597,8 @@ class TenantParser(object):
             classes = TenantParser._getLoadClasses(tenant, config_secret)
             if 'secret' not in classes:
                 continue
-            layout.addSecret(SecretParser.fromYaml(layout, config_secret))
+            with configuration_exceptions('secret', config_secret):
+                layout.addSecret(SecretParser.fromYaml(layout, config_secret))
 
         for config_job in data.jobs:
             classes = TenantParser._getLoadClasses(tenant, config_job)

zuul/model.py

@@ -612,6 +612,9 @@ class Secret(object):
                 self.source_context == other.source_context and
                 self.secret_data == other.secret_data)
 
+    def areDataEqual(self, other):
+        return (self.secret_data == other.secret_data)
+
     def __repr__(self):
         return '<Secret %s>' % (self.name,)
 
@@ -662,7 +665,6 @@ class SourceContext(object):
         if not isinstance(other, SourceContext):
             return False
         return (self.project == other.project and
-                self.branch == other.branch and
                 self.trusted == other.trusted)
 
     def __ne__(self, other):
@@ -2589,8 +2591,23 @@ class Layout(object):
         self.nodesets[nodeset.name] = nodeset
 
     def addSecret(self, secret):
-        if secret.name in self.secrets:
-            raise Exception("Secret %s already defined" % (secret.name,))
+        # It's ok to have a duplicate secret definition, but only if
+        # they are in different branches of the same repo, and have
+        # the same values.
+        other = self.secrets.get(secret.name)
+        if other:
+            if not secret.source_context.isSameProject(other.source_context):
+                raise Exception("Secret %s already defined in project %s" %
+                                (secret.name, other.source_context.project))
+            if secret.source_context.branch == other.source_context.branch:
+                raise Exception("Secret %s already defined" % (secret.name,))
+            if not secret.areDataEqual(other):
+                raise Exception("Secret %s does not match existing definition"
+                                " in branch %s" %
+                                (secret.name, other.source_context.branch))
+            # Identical data in a different branch of the same project;
+            # ignore the duplicate definition
+            return
         self.secrets[secret.name] = secret
 
     def addSemaphore(self, semaphore):