From d6a71ca2b45b56d28f0518d67564f973805e0b34 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 18 Aug 2017 14:15:05 -0700 Subject: [PATCH] Write secrets to tmpfs So that we may avoid writing the decrypted contents of secrets to disk, write them to a file in a tmpfs. Change-Id: I7c029b67d0fc2fa3827dc811137dd4f3a90706d8 --- tests/unit/test_bubblewrap.py | 7 ++- tests/unit/test_v3.py | 6 +-- zuul/driver/__init__.py | 5 ++- zuul/driver/bubblewrap/__init__.py | 71 ++++++++++++++++++++++-------- zuul/driver/nullwrap/__init__.py | 13 +++++- zuul/executor/server.py | 31 +++++++------ 6 files changed, 91 insertions(+), 42 deletions(-) diff --git a/tests/unit/test_bubblewrap.py b/tests/unit/test_bubblewrap.py index bb1be733c7..661d868c4f 100644 --- a/tests/unit/test_bubblewrap.py +++ b/tests/unit/test_bubblewrap.py @@ -39,8 +39,8 @@ class TestBubblewrap(testtools.TestCase): ssh_agent.start() po = context.getPopen(work_dir=work_dir, ssh_auth_sock=ssh_agent.env['SSH_AUTH_SOCK']) - self.assertTrue(po.passwd_r > 2) - self.assertTrue(po.group_r > 2) + self.assertTrue(po.fds[0] > 2) + self.assertTrue(po.fds[1] > 2) self.assertTrue(work_dir in po.command) # Now run /usr/bin/id to verify passwd/group entries made it in true_proc = po(['/usr/bin/id'], stdout=subprocess.PIPE, @@ -51,8 +51,7 @@ class TestBubblewrap(testtools.TestCase): # And that it did not print things on stderr self.assertEqual(0, len(errs.strip())) # Make sure the _r's are closed - self.assertIsNone(po.passwd_r) - self.assertIsNone(po.group_r) + self.assertEqual([], po.fds) def test_bubblewrap_leak(self): bwrap = bubblewrap.BubblewrapDriver() diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 2293ca017a..eb11edfea3 100755 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -1283,8 +1283,7 @@ class TestSecretLeaks(AnsibleZuulTestCase): ], ordered=False) matches = self.searchForContent(self.history[0].jobdir.root, b'test-password') - self.assertEqual(set(['/ansible/playbook_0/secrets.yaml', - '/work/secret-file.txt']), + self.assertEqual(set(['/work/secret-file.txt']), set(matches)) def test_secret_file(self): @@ -1319,8 +1318,7 @@ class TestSecretLeaks(AnsibleZuulTestCase): ], ordered=False) matches = self.searchForContent(self.history[0].jobdir.root, b'test-password') - self.assertEqual(set(['/ansible/playbook_0/secrets.yaml', - '/work/failure-file.txt']), + self.assertEqual(set(['/work/failure-file.txt']), set(matches)) def test_secret_file_fail(self): diff --git a/zuul/driver/__init__.py b/zuul/driver/__init__.py index 9096197f15..682b251abe 100644 --- a/zuul/driver/__init__.py +++ b/zuul/driver/__init__.py @@ -258,7 +258,7 @@ class WrapperInterface(object, metaclass=abc.ABCMeta): """ @abc.abstractmethod - def getExecutionContext(self, ro_paths=None, rw_paths=None): + def getExecutionContext(self, ro_paths=None, rw_paths=None, secrets=None): """Create and return an execution context. The execution context is meant to be used for a single @@ -268,6 +268,9 @@ class WrapperInterface(object, metaclass=abc.ABCMeta): :arg list ro_paths: read only files or directories to bind mount :arg list rw_paths: read write files or directories to bind mount + :arg dict secrets: a dictionary where the key is a file path, + and the value is the content which should be written to + that path in a secure manner. :returns: a new ExecutionContext object. :rtype: BaseExecutionContext diff --git a/zuul/driver/bubblewrap/__init__.py b/zuul/driver/bubblewrap/__init__.py index d70329ed3f..a8969cc180 100644 --- a/zuul/driver/bubblewrap/__init__.py +++ b/zuul/driver/bubblewrap/__init__.py @@ -22,6 +22,7 @@ import pwd import shlex import subprocess import sys +import threading import re from typing import Dict, List # flake8: noqa @@ -31,10 +32,9 @@ from zuul.execution_context import BaseExecutionContext class WrappedPopen(object): - def __init__(self, command, passwd_r, group_r): + def __init__(self, command, fds): self.command = command - self.passwd_r = passwd_r - self.group_r = group_r + self.fds = fds def __call__(self, args, *sub_args, **kwargs): try: @@ -46,7 +46,7 @@ class WrappedPopen(object): # versions. So until we are py3 only we can only bwrap # things that are close_fds=False pass_fds = list(kwargs.get('pass_fds', [])) - for fd in (self.passwd_r, self.group_r): + for fd in self.fds: if fd not in pass_fds: pass_fds.append(fd) kwargs['pass_fds'] = pass_fds @@ -56,26 +56,32 @@ class WrappedPopen(object): return proc def __del__(self): - if self.passwd_r: + for fd in self.fds: try: - os.close(self.passwd_r) + os.close(fd) except OSError: pass - self.passwd_r = None - if self.group_r: - try: - os.close(self.group_r) - except OSError: - pass - self.group_r = None + self.fds = [] class BubblewrapExecutionContext(BaseExecutionContext): log = logging.getLogger("zuul.BubblewrapExecutionContext") - def __init__(self, bwrap_command, ro_paths, rw_paths): + def __init__(self, bwrap_command, ro_paths, rw_paths, secrets): self.bwrap_command = bwrap_command self.mounts_map = {'ro': ro_paths, 'rw': rw_paths} + self.secrets = secrets + + def startPipeWriter(self, pipe, data): + # In case we have a large amount of data to write through a + # pipe, spawn a thread to handle the writes. + t = threading.Thread(target=self._writer, args=(pipe, data)) + t.daemon = True + t.start() + + def _writer(self, pipe, data): + os.write(pipe, data) + os.close(pipe) def getPopen(self, **kwargs): # Set zuul_dir if it was not passed in @@ -95,6 +101,9 @@ class BubblewrapExecutionContext(BaseExecutionContext): for bind in self.mounts_map[mount_type]: bwrap_command.extend([bind_arg, bind, bind]) + # A list of file descriptors which must be held open so that + # bwrap may read from them. + read_fds = [] # Need users and groups uid = os.getuid() passwd = list(pwd.getpwuid(uid)) @@ -106,6 +115,7 @@ class BubblewrapExecutionContext(BaseExecutionContext): os.write(passwd_w, passwd_bytes) os.write(passwd_w, b'\n') os.close(passwd_w) + read_fds.append(passwd_r) gid = os.getgid() group = grp.getgrgid(gid) @@ -115,6 +125,20 @@ class BubblewrapExecutionContext(BaseExecutionContext): os.write(group_w, group_bytes) os.write(group_w, b'\n') os.close(group_w) + read_fds.append(group_r) + + # Create a tmpfs for each directory which holds secrets, and + # tell bubblewrap to write the contents to a file therein. + secret_dirs = set() + for fn, content in self.secrets.items(): + secret_dir = os.path.dirname(fn) + if secret_dir not in secret_dirs: + bwrap_command.extend(['--tmpfs', secret_dir]) + secret_dirs.add(secret_dir) + secret_r, secret_w = os.pipe() + self.startPipeWriter(secret_w, content.encode('utf8')) + bwrap_command.extend(['--file', str(secret_r), fn]) + read_fds.append(secret_r) kwargs = dict(kwargs) # Don't update passed in dict kwargs['uid'] = uid @@ -126,7 +150,7 @@ class BubblewrapExecutionContext(BaseExecutionContext): self.log.debug("Bubblewrap command: %s", " ".join(shlex.quote(c) for c in command)) - wrapped_popen = WrappedPopen(command, passwd_r, group_r) + wrapped_popen = WrappedPopen(command, read_fds) return wrapped_popen @@ -186,14 +210,17 @@ class BubblewrapDriver(Driver, WrapperInterface): return bwrap_command - def getExecutionContext(self, ro_paths=None, rw_paths=None): + def getExecutionContext(self, ro_paths=None, rw_paths=None, secrets=None): if not ro_paths: ro_paths = [] if not rw_paths: rw_paths = [] + if not secrets: + secrets = {} return BubblewrapExecutionContext( self.bwrap_command, - ro_paths, rw_paths) + ro_paths, rw_paths, + secrets) def main(args=None): @@ -204,14 +231,22 @@ def main(args=None): parser = argparse.ArgumentParser() parser.add_argument('--ro-paths', nargs='+') parser.add_argument('--rw-paths', nargs='+') + parser.add_argument('--secret', nargs='+') parser.add_argument('work_dir') parser.add_argument('run_args', nargs='+') cli_args = parser.parse_args() ssh_auth_sock = os.environ.get('SSH_AUTH_SOCK') + secrets = {} + if cli_args.secret: + for secret in cli_args.secret: + fn, content = secret.split('=', 1) + secrets[fn]=content + context = driver.getExecutionContext( - cli_args.ro_paths, cli_args.rw_paths) + cli_args.ro_paths, cli_args.rw_paths, + secrets) popen = context.getPopen(work_dir=cli_args.work_dir, ssh_auth_sock=ssh_auth_sock) diff --git a/zuul/driver/nullwrap/__init__.py b/zuul/driver/nullwrap/__init__.py index d807904951..178e4c7fcc 100644 --- a/zuul/driver/nullwrap/__init__.py +++ b/zuul/driver/nullwrap/__init__.py @@ -32,5 +32,16 @@ class NullwrapDriver(Driver, WrapperInterface): name = 'nullwrap' log = logging.getLogger("zuul.NullwrapDriver") - def getExecutionContext(self, ro_paths=None, rw_paths=None): + def getExecutionContext(self, ro_paths=None, rw_paths=None, secrets=None): + # The bubblewrap driver writes secrets to a tmpfs so that they + # don't hit the disk (unless the kernel swaps the memory to + # disk, which can be mitigated with encrypted swap). We + # haven't implemented similar functionality in nullwrap, so + # for safety, raise an exception in that case. If you are + # interested in implementing this functionality, please + # contact us on the mailing list. + if secrets: + raise NotImplementedError( + "The nullwrap driver does not support the use of secrets. " + "Consider using the bubblewrap driver instead.") return NullExecutionContext() diff --git a/zuul/executor/server.py b/zuul/executor/server.py index 55e47da7bb..1a445f13f9 100644 --- a/zuul/executor/server.py +++ b/zuul/executor/server.py @@ -255,8 +255,9 @@ class JobDirPlaybook(object): self.roles_path = [] self.ansible_config = os.path.join(self.root, 'ansible.cfg') self.project_link = os.path.join(self.root, 'project') - self.secrets = os.path.join(self.root, 'secrets.yaml') - self.has_secrets = False + self.secrets_root = os.path.join(self.root, 'secrets') + self.secrets = os.path.join(self.secrets_root, 'secrets.yaml') + self.secrets_content = None def addRole(self): count = len(self.roles) @@ -1266,6 +1267,13 @@ class AnsibleJob(object): for role in playbook['roles']: self.prepareRole(jobdir_playbook, role, args) + secrets = playbook['secrets'] + if secrets: + if 'zuul' in secrets: + raise Exception("Defining secrets named 'zuul' is not allowed") + jobdir_playbook.secrets_content = yaml.safe_dump( + secrets, default_flow_style=False) + self.writeAnsibleConfig(jobdir_playbook, playbook) def checkoutTrustedProject(self, project, branch): @@ -1390,15 +1398,6 @@ class AnsibleJob(object): def writeAnsibleConfig(self, jobdir_playbook, playbook): trusted = jobdir_playbook.trusted - secrets = playbook['secrets'].copy() - if secrets: - if 'zuul' in secrets: - raise Exception("Defining secrets named 'zuul' is not allowed") - with open(jobdir_playbook.secrets, 'w') as secrets_yaml: - secrets_yaml.write( - yaml.safe_dump(secrets, default_flow_style=False)) - jobdir_playbook.has_secrets = True - # TODO(mordred) This should likely be extracted into a more generalized # mechanism for deployers being able to add callback # plugins. @@ -1441,7 +1440,7 @@ class AnsibleJob(object): # role. Otherwise, printing the args could be useful for # debugging. config.write('display_args_to_stdout = %s\n' % - str(not secrets)) + str(not playbook['secrets'])) config.write('[ssh_connection]\n') # NB: when setting pipelining = True, keep_remote_files @@ -1509,8 +1508,12 @@ class AnsibleJob(object): if self.executor_variables_file: ro_paths.append(self.executor_variables_file) + secrets = {} + if playbook.secrets_content: + secrets[playbook.secrets] = playbook.secrets_content + context = self.executor_server.execution_wrapper.getExecutionContext( - ro_paths, rw_paths) + ro_paths, rw_paths, secrets) popen = context.getPopen( work_dir=self.jobdir.work_root, @@ -1593,7 +1596,7 @@ class AnsibleJob(object): verbose = '-v' cmd = ['ansible-playbook', verbose, playbook.path] - if playbook.has_secrets: + if playbook.secrets_content: cmd.extend(['-e', '@' + playbook.secrets]) if success is not None: