PR#417: Enable grabbing secrets from Vault server - taskotron/libtaskotron

taskotron / libtaskotron

#417 Enable grabbing secrets from Vault server

Closed 5 years ago by jskladan. Opened 5 years ago by jskladan.

feature/secrets into develop

Josef Skladanka • 5 years ago

8df7221

Fixed to reflect review

Josef Skladanka • 5 years ago

8036ea3

Enable grabbing secrets from Vault server

Josef Skladanka • 5 years ago

a76bdfa

conf/taskotron.yaml.example

file modified

+13

		`@@ -88,6 +88,19 @@`
		`## Please make sure the URL doesn't have a trailing slash.`
		`#execdb_server: http://localhost:5003`

		`+ ## URL of VAULT server API interface, which stores secrets.`
		`+ ## Please make sure the URL doesn't have a trailing slash.`
		`+ #vault_server: http://localhost:4999/api/v1`
		`+`
		`+ ## Use vault to grab secrets`
		`+ #vault_enabled: False`
		`+`
		`+ ## Username for vault server`
		`+ #vault_username: taskotron`
		`+`
		`+ ## Password for vault server`
		`+ #vault_password: taskotron`
		`+`
		`## URL of taskotron buildmaster, to construct log URLs from.`
		`## Please make sure the URL doesn't have a trailing slash.`
		`#taskotron_master: http://localhost/taskmaster`

data/ansible/runner.yml

file modified

+15

		`@@ -81,6 +81,12 @@`
		`register: result`
		`until: result.rc == 0`

		`+ - name: Upload secrets`
		`+ copy:`
		`+ src: '{{taskotron_secrets_file}}'`
		`+ dest: '{{taskotron_secrets_file}}'`
		`+ when: not local`
		`+`
		`- name: Start heartbeat`
		`command: >`
		`./heartbeat.sh start {{ heartbeat_file }} {{ heartbeat_interval }}`
		`@@ -95,6 +101,15 @@`
		`static: yes`
		`# variable 'task' is registered here`

		`+ - name: Delete secrets`
		`+ file:`
		`+ state: absent`
		`+ path: '{{taskotron_secrets_file}}'`
		`+ tags:`
		`+ - failsafe`
		`+ when: not local`
		`+`
		`+`
		`- name: Kill heartbeat`
		`command: ./heartbeat.sh stop {{ heartbeat_file }}`
		`when: taskotron_keepalive_minutes\|int > 0`

docs/source/writingtasks.rst

file modified

		`@@ -151,6 +151,14 @@`
		`tasks might want to ignore this (for example, if they're capable of`
		`checking all architectures in a single run).`

		+ ``taskotron_secrets_file``
		`+ (string) Path to the file containing relevant secrets grabbed from the`
		`+ Vault server. If a bucket owned by the taskotron user has a string like`
		+ ``taskotron_enable(XYZ)`` in the ``description`` field, and the ``XYZ``
		`+ is the same as the current task's git source repo, the bucket's secrets`
		`+ are included in the file. This file contains a json-encoded dictionary,`
		`+ where bucket's uuids are keys, and a dict of secrets is the respective value.`
		`+`
		``taskotron_supported_arches``
		(list of strings) A list of all base architectures (i.e. ``x86_64``,
		``i386``, ``armhfp``) that are officially supported by this Taskotron

libtaskotron/config_defaults.py

file modified

		`@@ -74,6 +74,10 @@`
		`taskotron_master = 'http://localhost/taskmaster' #:`
		`artifacts_baseurl = 'http://localhost/artifacts' #:`
		`download_cache_enabled = True #:`
		`+ vault_enabled = False #:`
		`+ vault_server = 'http://localhost:4999/api/v1' #:`
		`+ vault_username = 'taskotron' #:`
		`+ vault_password = 'taskotron' #:`
kparal commented 5 years ago This is supposed to be user-configurable, so this also needs to be placed in `conf/taskotron.yaml.example`, with a short description.
jskladan commented 5 years ago I actually added it, but forgot to `git add the file` thx for the catch.

		`tmpdir = '/var/tmp/taskotron' #:`
		`logdir = '/var/log/taskotron' #:`

libtaskotron/directives/resultsdb_directive.py

file modified

+16 -10

		`@@ -109,6 +109,18 @@`

		`directive_class = 'ResultsdbDirective'`

		`+ def git_origin_url(taskdir):`
		`+ try:`
		`+ gitconfig_filename = os.path.join(taskdir, '.git/config')`
		`+ gitconfig = configparser.ConfigParser()`
		`+ gitconfig.read(gitconfig_filename)`
		`+ task_repo_url = gitconfig['remote "origin"']['url']`
		`+ except TypeError as e:`
		`+ log.exception(e)`
		`+ task_repo_url = None`
		`+ return task_repo_url`
		`+`
		`+`

		`class ResultsdbDirective(BaseDirective):`

		`@@ -217,17 +229,11 @@`
		`else:`
		`raise TaskotronDirectiveError('No namespace for task %s exists.' % checkname)`

		`- try:`
		`- taskdir = os.path.dirname(os.path.abspath(arg_data['task']))`
		`- gitconfig_filename = os.path.join(taskdir, '.git/config')`
		`-`
		`- gitconfig = configparser.ConfigParser()`
		`- gitconfig.read(gitconfig_filename)`
		`- task_repo_url = gitconfig['remote "origin"']['url']`
		`- except TypeError as e:`
		`- log.exception(e)`
		`+ taskdir = os.path.dirname(os.path.abspath(arg_data['task']))`
		`+ task_repo_url = git_origin_url(taskdir)`
		`+ if not task_repo_url:`
		`raise TaskotronDirectiveError("Could not find task's git remote 'origin' url"`
		`- "in %s" % gitconfig_filename)`
		`+ "in %s" % os.path.join(taskdir, '.git/config'))`

		`try:`
		`if not [ns_repo for ns_repo in ns_repos if task_repo_url.strip().startswith(ns_repo)]:`

libtaskotron/executor.py

file modified

+62 -1

		`@@ -12,6 +12,10 @@`
		`import fnmatch`
		`import signal`
		`import json`
		`+ import tempfile`
		`+ import configparser`
		`+ import requests`
		`+ import re`

		`from libtaskotron import config`
		`from libtaskotron import image_utils`
		`@@ -57,6 +61,7 @@`
		`'taskotron_arch',`
		`'taskotron_item_type',`
		`'taskotron_item',`
		`+ 'taskotron_secrets_file',`
		`'taskotron_supported_arches',`
		`'taskotron_supported_binary_arches',`
		`]`
		`@@ -181,6 +186,53 @@`
		`vars_[var] = val`
		`return vars_`

		`+ def _get_vault_secrets(self, taskdir):`
		`+ '''Load secrets from the Vault server and store them in a file`
		`+`
		`+ :param str taskdir: path to the directory with test suite (on overlord)`
		`+ :return: a filename with decrypted secrets`
		`+ '''`
		`+ cfg = config.get_config()`
		`+ secrets = {}`
		`+ if cfg.vault_enabled:`
		`+ task_repo_url = resultsdb_directive.git_origin_url(taskdir)`
		`+ if task_repo_url:`
		`+ try:`
		`+ session = file_utils._get_session()`
		`+ r = session.get(`
		`+ "%s/buckets" % cfg.vault_server,`
		`+ auth=(cfg.vault_username, cfg.vault_password),`
		`+ )`
		`+ except requests.exceptions.RequestException as e:`
		`+ log.error("Connection to Vault server failed. %s", e)`
		`+ r = None`
		`+`
		`+ if r and r.ok:`
		`+ data = r.json()['data']`
		`+ valid_buckets = []`
		`+ re_enabler = re.compile(r'taskotron_enable\((.*?)\)')`
		`+ for b in data:`
		`+ desc = b['description']`
		`+ if not desc:`
		`+ continue`
		`+ enabled_for = ', '.join(re_enabler.findall(desc))`
		`+ if not task_repo_url in enabled_for:`
		`+ continue`
		`+ valid_buckets.append(b)`
		`+ for b in valid_buckets:`
		`+ secrets[b['uuid']] = b['secrets']`
		`+ elif r and not r.ok:`
		`+ log.error("Could not get data from vault. %r, %r", r.status_code, r.reason)`
		`+`
		`+ if config.get_config().profile == config.ProfileName.TESTING:`
		`+ return secrets`
		`+`
		`+ fd, fname = tempfile.mkstemp(prefix='taskotron_secrets')`
		`+ os.close(fd)`
		`+ with open(fname, 'w') as fd:`
		`+ fd.write(json.dumps(secrets, indent=2, sort_keys=True))`
		`+ return fname`
		`+`
		`def _create_playbook_vars(self, test_playbook):`
		`'''Create and return dictionary containing all variables to be used`
		`with our ansible playbook.`
		`@@ -230,6 +282,8 @@`
		`whether VM guest distro has to match taskotron_item`
		`taskotron_match_host_release`
		`whether VM guest release has to match taskotron_item`
		`+ taskotron_secrets_file`
		`+ path to the file with secrets appropriate for the task`
		`taskotron_supported_arches`
		`list of base architectures supported by Taskotron (e.g.`
		`'armhfp')`
		`@@ -274,6 +328,7 @@`
		`vars_['taskotron_arch'] = self.arg_data['arch']`
		`vars_['taskotron_item'] = self.arg_data['item']`
		`vars_['taskotron_item_type'] = self.arg_data['type']`
		`+ vars_['taskotron_secrets_file'] = self._get_vault_secrets(taskdir=vars_['taskdir'])`
		`vars_['taskotron_supported_arches'] = cfg.supported_arches`
		`vars_['taskotron_supported_binary_arches'] = [binarch for arch in`
		`cfg.supported_arches for binarch in arch_utils.Arches.binary[arch]]`
		`@@ -446,6 +501,7 @@`

		`failed = []`
		`for test_playbook in test_playbooks:`
		`+ playbook_vars = None`
		`try:`
		`# syntax check`
		`self._check_playbook_syntax(os.path.join(`
		`@@ -479,6 +535,12 @@`
		`test_playbook, e)`
		`failed.append(test_playbook)`
		`finally:`
		`+ try:`
		`+ if playbook_vars and config.get_config().profile != config.ProfileName.TESTING:`
		`+ os.remove(playbook_vars['taskotron_secrets_file'])`
		`+ except OSError as e:`
		`+ log.warning("Could not delete the secrets file at %r. %s",`
		`+ playbook_vars['taskotron_secrets_file'], e)`
		`if self.task_vm is not None:`
		`if self.arg_data['no_destroy']:`
		`log.info('Not destroying disposable client as '`
		`@@ -487,7 +549,6 @@`
		`break`
		`else:`
		`self.task_vm.teardown()`
		`-`
		`log.info('Playbook execution finished: %s', test_playbook)`

		`if failed:`

testing/functest_executor.py

file modified

+43

		`@@ -89,3 +89,46 @@`
		`assert len(matched) == 1`
		`warn_task_line = lines.index(matched[0])`
		`assert 'skipping' in lines[warn_task_line + 1]`
		`+`
		`+ def test_get_vault_secrets(self, monkeypatch):`
		`+ class MockRequests(object):`
		`+ def __init__(self, retval):`
		`+ self.ok = True`
		`+ self.retval = retval`
		`+ self.exceptions = mock.Mock()`
		`+ def get(self, args, *kwargs):`
		`+ return self`
		`+ def json(self, args, *kwargs):`
		`+ return self.retval`
		`+`
		`+ self.conf.vault_enabled = True`
		`+`
		`+ mock_git_origin_url = mock.Mock(return_value='fake://repo.git')`
		`+ monkeypatch.setattr(executor.resultsdb_directive, 'git_origin_url', mock_git_origin_url)`
		`+`
		`+ mock_requests = MockRequests({'data': [`
		`+ {`
		`+ 'description': 'dat description',`
		`+ 'secrets': 'dem secrets',`
		`+ 'uuid': 'dat uuid',`
		`+ }`
		`+ ]})`
		`+ mock_session = mock.Mock(return_value=mock_requests)`
		`+ monkeypatch.setattr(executor.file_utils, '_get_session', mock_session)`
		`+ secrets = self.executor._get_vault_secrets(taskdir=None)`
		`+ assert secrets == {}`
		`+`
		`+`
		`+ mock_requests = MockRequests({'data': [`
		`+ {`
		`+ 'description': 'dat_description, taskotron_enable(fake://repo.git)',`
		`+ 'secrets': 'dem_secrets',`
		`+ 'uuid': 'dat uuid',`
		`+ }`
		`+ ]})`
		`+ mock_session = mock.Mock(return_value=mock_requests)`
		`+ monkeypatch.setattr(executor.file_utils, '_get_session', mock_session)`
		`+ secrets = self.executor._get_vault_secrets(taskdir=None)`
		`+ assert secrets == {'dat uuid': 'dem_secrets'}`
		`+`
		`+`

jskladan commented 5 years ago

The overlord now can get secrets from the Vault secret store, and put those to a known location on minion.
Usefull for tasks-to-be like a one that creates docker images from our repos per commit.

lbrabec commented 5 years ago

lgtm

kparal commented on line 8 of libtaskotron/config_defaults.py 5 years ago

This is supposed to be user-configurable, so this also needs to be placed in conf/taskotron.yaml.example, with a short description.

kparal commented on line 66 of libtaskotron/executor.py 5 years ago

I'm confused. This only exposes it in our runner.yml. But how does a task know which file to load the secrets from? The variable needs to get forwarded to the task, right? In that case it needs to be added to FORWARDED_VARS (and also it needs to get documented in writingtasks.rst, see docs). Or what am I missing?

kparal commented on line 54 of libtaskotron/executor.py 5 years ago

Why do you close the file descriptor and then open it again?

kparal commented on line 29 of libtaskotron/executor.py 5 years ago

You can use file_utils._get_session() to get a session that automatically retries failed requests for 5xx errors and connection timeouts. Would it make sense to use it here as well? I've spent some time lately trying to get to code more robust against connection issues, and this is another connection we'll be making for each task.

jskladan commented on line 54 of libtaskotron/executor.py 5 years ago

Long story short - python 3. The file descriptor provided by mkstemp only accepts bytes, json.dumps returns str. I'm not willing to go into encode hell here, when "regular opened file" can work just fine.
I'm closing the 'rogue' fd just because I think it's polite, would you rather have it hanging in there?

kparal commented on line 34 of libtaskotron/executor.py 5 years ago

If there's a connection problem, we'll not know about it, just the task will fail with missing credentials or something. Is there a reason for at least not logging the error?

jskladan commented on line 66 of libtaskotron/executor.py 5 years ago

mea culpa, for some reason I though that all the vars_ get forwarded

jskladan commented on line 8 of libtaskotron/config_defaults.py 5 years ago

I actually added it, but forgot to git add the file thx for the catch.

kparal commented on line 20 of libtaskotron/executor.py 5 years ago

I see no "decryption" happening, it's just storing retrieved data, right? I have to add I haven't read through vault code yet, so I might be missing something.

jskladan commented on line 29 of libtaskotron/executor.py 5 years ago

cool, why not

jskladan commented on line 20 of libtaskotron/executor.py 5 years ago

I don't really understand the point of the question..

kparal commented on line 27 of libtaskotron/executor.py 5 years ago

This took me a minute to read :) Could you perhaps move this line into the if r and r.ok block, so that's it's together with the rest of the code parsing the vault data, and then add a short and simple example of how the input data looks? It would make understanding the code much easier. You could also show a short example of what the transformed output (written into a file) looks like - totally optional! Thanks!

kparal commented on line 93 of libtaskotron/executor.py 5 years ago

This will not be executed if the break command a few lines above is hit. Is that expected?

kparal commented on line 96 of libtaskotron/executor.py 5 years ago

If I understood the code correctly, the secrets file is always created, and therefore is should always be possible to delete it. Right? In that case, let's put a warning here, because something has clearly gone wrong if we can't delete that file (and add the reason, so include the OSError instance in the error message). Thanks.

jskladan commented on line 27 of libtaskotron/executor.py 5 years ago

Learn regexps, I guess? There are far more complicated regexps written by you in the code, without "explanations" :)

Moving it in to the next block makes sense, why not.

jskladan commented on line 34 of libtaskotron/executor.py 5 years ago

you have some good points!

jskladan commented on line 93 of libtaskotron/executor.py 5 years ago

Yes, this honestly only is a best-effort. I'm open to better solutions, though.

jskladan commented on line 96 of libtaskotron/executor.py 5 years ago

kparal commented on line 54 of libtaskotron/executor.py 5 years ago

OK, a comment might prevent future editors from "optimizing" this the wrong way :)

kparal commented on line 27 of libtaskotron/executor.py 5 years ago

Well, if I see what an input should look like, then it's much easier to read it and verify it's actually correct. I looked at vault project and there's no documentation how the return data should look like. Finally I found a sample in the unit tests. Since it's about 3 lines, I believe it would be of great help if it got included in a comment here as well.

kparal commented on line 93 of libtaskotron/executor.py 5 years ago

Simply put this block above the if self.task_vm is not None block and we should be good.

jskladan commented on line 27 of libtaskotron/executor.py 5 years ago

On the other hand, the regexp can be simplified, so I give you that. Have a look at the new version, if that's still hard to understand, we can have a discussion about it :)

1 new commit added