Block connection related variables

There are some special variables that should be only set by nodepool
and not on job level [1]. Overriding those could make mitm attacks
possible. Fix this by blocking those variables in the job definition
and data return.

[1] https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html#connection-variables

Story: 2008672
Task: 41964
Change-Id: Ie85fe110c092df7ef816af20356a55426cbebcb2
Co-Authored-By: Tobias Henkel <tobias.henkel@bmw.de>

releasenotes/notes/connection-vars-f4638e5ffeef095d.yaml

@@ -0,0 +1,15 @@
+---
+security:
+  - |
+    The following connection-related variables are no longer allowed
+    to be set in job definitions, as they may be used to circumvent
+    security measures:
+
+      * ansible_connection
+      * ansible_host
+      * ansible_python_interpreter
+      * ansible_shell_executable
+      * ansible_user
+
+    They may still be set using the corresponding settings in
+    Nodepool.

tests/fixtures/config/connection-vars/git/common-config/playbooks/run.yaml

@@ -0,0 +1,7 @@
+- hosts: all
+  tasks:
+    - debug:
+        msg: "{{ ansible_shell_executable }}"
+    - zuul_return:
+        data:
+          ansible_shell_executable: /bin/du

tests/fixtures/config/connection-vars/git/common-config/zuul.yaml

@@ -0,0 +1,34 @@
+- pipeline:
+    name: check
+    manager: independent
+    trigger:
+      gerrit:
+        - event: patchset-created
+        - event: comment-added
+          comment: '^(Patch Set [0-9]+:\n\n)?(?i:recheck)$'
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null
+
+- job:
+    name: test-job
+    run: playbooks/run.yaml
+    nodeset:
+      nodes:
+        - name: controller
+          label: foo
+
+- job:
+    name: second-job
+    run: playbooks/run.yaml
+    nodeset:
+      nodes:
+        - name: controller
+          label: foo

tests/fixtures/config/connection-vars/git/org_project/README

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

tests/fixtures/config/connection-vars/git/org_project/zuul.yaml

@@ -0,0 +1,6 @@
+- project:
+    check:
+      jobs:
+        - test-job
+        - second-job:
+            dependencies: test-job

tests/fixtures/config/connection-vars/main.yaml

@@ -0,0 +1,8 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project

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

@@ -82,14 +82,14 @@
     run: playbooks/group-inventory.yaml
     group-vars:
       ceph-osd:
-        ansible_python_interpreter: python4
+        ceph_var: ceph
 
 - job:
     name: hostvars-inventory
     run: playbooks/hostvars-inventory.yaml
     nodeset: nodeset2
     vars:
-      ansible_python_interpreter: python1.5.2
+      all_var: all
 
 - job:
     name: jinja2-message

tests/unit/test_inventory.py

@@ -295,13 +295,20 @@ class TestInventory(TestInventoryBase):
                           inventory['all']['children']
                           ['ceph-monitor']['hosts'])
         self.assertEqual(
-            'python4',
+            'auto',
             inventory['all']['hosts']['controller']
             ['ansible_python_interpreter'])
+        self.assertEqual(
+            'ceph',
+            inventory['all']['hosts']['controller']
+            ['ceph_var'])
         self.assertEqual(
             'auto',
             inventory['all']['hosts']['compute1']
             ['ansible_python_interpreter'])
+        self.assertNotIn(
+            'ceph_var',
+            inventory['all']['hosts']['compute1'])
         self.assertIn('zuul', inventory['all']['vars'])
         z_vars = inventory['all']['vars']['zuul']
         self.assertIn('executor', z_vars)
@@ -341,9 +348,13 @@ class TestInventory(TestInventoryBase):
                     inventory['all']['hosts'][node_name]['ansible_connection'])
 
             self.assertEqual(
-                'python1.5.2',
+                'auto',
                 inventory['all']['hosts'][node_name]
                 ['ansible_python_interpreter'])
+            self.assertEqual(
+                'all',
+                inventory['all']['hosts'][node_name]
+                ['all_var'])
         self.assertNotIn(
             'ansible_python_interpreter',
             inventory['all']['vars'])

tests/unit/test_v3.py

@@ -7356,3 +7356,48 @@ class TestUnsafeVars(AnsibleZuulTestCase):
 
         # This is marked unsafe
         self.assertIn("TESTJOB SECRET: {{ subtext }}", job_output)
+
+
+class TestConnectionVars(AnsibleZuulTestCase):
+    tenant_config_file = 'config/connection-vars/main.yaml'
+
+    def _get_file(self, build, path):
+        p = os.path.join(build.jobdir.root, path)
+        with open(p) as f:
+            return f.read()
+
+    def test_ansible_connection(self):
+        in_repo_conf = textwrap.dedent(
+            """
+            - project:
+                check:
+                  jobs:
+                    - test-job:
+                        vars:
+                          ansible_shell_executable: /bin/du
+            """)
+
+        file_dict = {'zuul.yaml': in_repo_conf}
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
+                                           files=file_dict)
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertIn("Variable name 'ansible_shell_executable' "
+                      "is not allowed", A.messages[0])
+        self.assertHistory([])
+
+    def test_return_data(self):
+        self.executor_server.keep_jobdir = True
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.assertHistory([
+            dict(name='test-job', result='SUCCESS', changes='1,1'),
+        ], ordered=False)
+
+        # Currently, second-job errors; if it ever runs, add these assertions:
+        # job = self.getJobFromHistory('second-job')
+        # job_output = self._get_file(job, 'work/logs/job-output.txt')
+        # self.log.debug(job_output)
+        # self.assertNotIn("/bin/du", job_output)

zuul/configloader.py

@@ -32,6 +32,7 @@ import zuul.manager.supercedent
 import zuul.manager.serial
 from zuul.lib.logutil import get_annotated_logger
 from zuul.lib.re2util import filter_allowed_disallowed
+from zuul.lib.varnames import check_varnames
 from zuul.zk.semaphore import SemaphoreHandler
 
 
@@ -873,31 +874,22 @@ class JobParser(object):
                 setattr(job, k, v)
 
         variables = conf.get('vars', None)
-        forbidden = {'zuul', 'nodepool', 'unsafe_vars'}
         if variables:
-            if set(variables.keys()).intersection(forbidden):
-                raise Exception("Variables named 'zuul', 'nodepool', "
-                                "or 'unsafe_vars' are not allowed.")
+            check_varnames(variables)
             job.variables = variables
         extra_variables = conf.get('extra-vars', None)
         if extra_variables:
-            if set(extra_variables.keys()).intersection(forbidden):
-                raise Exception("Variables named 'zuul', 'nodepool', "
-                                "or 'unsafe_vars' are not allowed.")
+            check_varnames(extra_variables)
             job.extra_variables = extra_variables
         host_variables = conf.get('host-vars', None)
         if host_variables:
             for host, hvars in host_variables.items():
-                if set(hvars.keys()).intersection(forbidden):
-                    raise Exception("Variables named 'zuul', 'nodepool', "
-                                    "or 'unsafe_vars'are not allowed.")
+                check_varnames(hvars)
             job.host_variables = host_variables
         group_variables = conf.get('group-vars', None)
         if group_variables:
             for group, gvars in group_variables.items():
-                if set(gvars.keys()).intersection(forbidden):
-                    raise Exception("Variables named 'zuul', 'nodepool', "
-                                    "or 'unsafe_vars'are not allowed.")
+                check_varnames(gvars)
             job.group_variables = group_variables
 
         allowed_projects = conf.get('allowed-projects', None)

zuul/executor/server.py

@@ -44,6 +44,7 @@ from zuul.lib.logutil import get_annotated_logger
 from zuul.lib.statsd import get_statsd
 from zuul.lib import filecomments
 from zuul.lib.keystorage import ZooKeeperKeyStorage
+from zuul.lib.varnames import check_varnames
 
 import gear
 
@@ -742,25 +743,6 @@ class DeduplicateQueue(object):
             self.condition.release()
 
 
-VARNAME_RE = re.compile(r'^[A-Za-z0-9_]+$')
-
-
-def check_varnames(var):
-    # We block these in configloader, but block it here too to make
-    # sure that a job doesn't pass variables named zuul or nodepool.
-    if 'zuul' in var:
-        raise Exception("Defining variables named 'zuul' is not allowed")
-    if 'nodepool' in var:
-        raise Exception("Defining variables named 'nodepool' is not allowed")
-    if 'unsafe_vars' in var:
-        raise Exception("Defining variables named 'unsafe_vars' "
-                        "is not allowed")
-    for varname in var.keys():
-        if not VARNAME_RE.match(varname):
-            raise Exception("Variable names may only contain letters, "
-                            "numbers, and underscores")
-
-
 def squash_variables(nodes, groups, jobvars, groupvars,
                      extravars):
     """Combine the Zuul job variable parameters into a hostvars dictionary.
@@ -1333,6 +1315,13 @@ class AnsibleJob(object):
                     file_data = json.loads(file_data)
                     data = file_data.get('data', {})
                     secret_data = file_data.get('secret_data', {})
+            # Check the variable names for safety, but zuul is allowed.
+            data_copy = data.copy()
+            data_copy.pop('zuul', None)
+            check_varnames(data_copy)
+            secret_data_copy = data.copy()
+            secret_data_copy.pop('zuul', None)
+            check_varnames(secret_data_copy)
         except Exception:
             self.log.exception("Unable to load result data:")
         return data, secret_data

zuul/lib/varnames.py

@@ -0,0 +1,45 @@
+# Copyright 2021 Acme Gating, LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+import re
+
+
+VARNAME_RE = re.compile(r'^[A-Za-z0-9_]+$')
+
+
+def check_varnames(var):
+    # We block these in configloader, but block it here too to make
+    # sure that a job doesn't pass variables named zuul or nodepool.
+    if 'zuul' in var:
+        raise Exception("Defining variables named 'zuul' is not allowed")
+    if 'nodepool' in var:
+        raise Exception("Defining variables named 'nodepool' is not allowed")
+    if 'unsafe_vars' in var:
+        raise Exception("Defining variables named 'unsafe_vars' "
+                        "is not allowed")
+    for varname in var.keys():
+        if not VARNAME_RE.match(varname):
+            raise Exception("Variable names may only contain letters, "
+                            "numbers, and underscores")
+    # Block some connection related variables so they cannot be
+    # overridden by jobs to bypass security mechanisms.
+    connection_vars = [
+        'ansible_connection',
+        'ansible_host',
+        'ansible_python_interpreter',
+        'ansible_shell_executable',
+        'ansible_user',
+    ]
+    for conn_var in connection_vars:
+        if conn_var in var:
+            raise Exception(f"Variable name '{conn_var}' is not allowed.")

zuul/model.py

@@ -21,7 +21,6 @@ import logging
 import os
 from itertools import chain
 
-import re
 import re2
 import struct
 import time
@@ -31,6 +30,7 @@ import textwrap
 import types
 import itertools
 from zuul.lib import yamlutil as yaml
+from zuul.lib.varnames import check_varnames
 
 import jsonpath_rw
 
@@ -98,8 +98,6 @@ SCHEME_GOLANG = 'golang'
 SCHEME_FLAT = 'flat'
 SCHEME_UNIQUE = 'unique'  # Internal use only
 
-VARNAME_RE = re.compile(r'^[A-Za-z0-9_]+$')
-
 
 class ConfigurationErrorKey(object):
     """A class which attempts to uniquely identify configuration errors
@@ -1106,12 +1104,7 @@ class PlaybookContext(ConfigObject):
                 raise Exception(
                     'The secret "{name}" was not found.'.format(
                         name=secret_use.name))
-            if secret_use.alias in ('zuul', 'nodepool', 'unsafe_vars'):
-                raise Exception("Secrets named 'zuul', 'nodepool', "
-                                "or 'unsafe_vars' are not allowed.")
-            if not VARNAME_RE.match(secret_use.alias):
-                raise Exception("Variable names may only contain letters, "
-                                "numbers, and underscores")
+            check_varnames({secret_use.alias: ''})
             if not secret.source_context.isSameProject(self.source_context):
                 raise Exception(
                     "Unable to use secret {name}.  Secrets must be "