Add a way to pass secrets to processes in a controlled fashion by kostko · Pull Request #406 · genialis/resolwe

kostko · 2017-12-05T13:55:06Z

DEPLOYMENT CONSIDERATIONS:

Secrets are encrypted using SECRET_KEY, keep it secure and backed up as it should already be ;-)
Setting FLOW_DOCKER_MAPPINGS has been removed in favor of new FLOW_DOCKER_VOLUME_EXTRA_OPTIONS and FLOW_DOCKER_EXTRA_VOLUMES.

jberci

LGTM

codecov-io · 2017-12-05T18:24:31Z

Codecov Report

Merging #406 into master will decrease coverage by 0.54%.
The diff coverage is 46.08%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
- Coverage   80.09%   79.55%   -0.55%     
==========================================
  Files         181      186       +5     
  Lines        9899    10065     +166     
  Branches     1059     1069      +10     
==========================================
+ Hits         7929     8007      +78     
- Misses       1789     1875      +86     
- Partials      181      183       +2

Impacted Files	Coverage Δ
resolwe/test_helpers/test_runner.py	`43.83% <ø> (-0.55%)`	⬇️
resolwe/flow/execution_engines/bash/__init__.py	`90.32% <ø> (ø)`	⬆️
resolwe/test/testcases/setting_overrides.py	`100% <ø> (ø)`	⬆️
resolwe/flow/executors/run.py	`0% <0%> (ø)`	⬆️
resolwe/flow/executors/docker/run.py	`0% <0%> (ø)`	⬆️
resolwe/flow/migrations/0010_add_secret.py	`100% <100%> (ø)`
resolwe/flow/routing.py	`100% <100%> (ø)`	⬆️
resolwe/flow/models/__init__.py	`100% <100%> (ø)`	⬆️
resolwe/flow/tests/test_secrets.py	`100% <100%> (ø)`
resolwe/flow/managers/__init__.py	`54.16% <100%> (ø)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07f46c1...5c35716. Read the comment docs.

dblenkus · 2017-12-05T14:07:16Z

resolwe/flow/static/flow/typeSchema.json

+                    "type": "string",
+                    "pattern": "^basic:secret:$"
+                },
+                "value": {


Enforce the secret format, i.e. "pattern": "^[0-9a-fA-F]{24}$". Otherwise some users may try to save secret directly in to the field.

dblenkus · 2017-12-05T21:30:30Z

resolwe/flow/executors/prepare.py

+            file content that should be created. Keys are filenames, values
+            are the strings that should be written into those files.
        """
+        data = Data.objects.get(pk=data_id)


Prefetch process.

As we are getting just a single object this doesn't make any difference in performance or the number of queries as even prefetching requires an additional query. We could use select_related here instead of prefetch to combine everything into a single join.

dblenkus · 2017-12-05T21:36:49Z

resolwe/flow/executors/docker/run.py

+        mappings.append({
+            'src': os.path.join(SETTINGS['FLOW_EXECUTOR']['RUNTIME_DIR'], str(self.data_id), 'secrets'),
+            'dest': '/secrets',
+            'mode': 'ro,Z'


It is not ok to set SELinux mode on the systems that don't support it. We had such problems mounting data dir from Amazon EBS. Runtime dir will be often on shared storage, so you have to take care of that.

Hm, but we are using this also above for mounting passwd/group?

Yes, those files are normally on local disk so it is ok. The difference here is that runtime dir can be on shared storage.

Ok, I will move this into configuration as we have for data/upload dirs.

But this means that you have to support different mounting points in the container.

What if we add a setting to enable/disable SELinux config, e.g. FLOW_ENABLE_SELINUX_CONF, and the you can construct mount here?

cc @tjanez

But then we must change this everywhere as this is very strange to have some modes in settings and others are derived from this?

But this means that you have to support different mounting points in the container.

I don't think we should support this even with existing data/upload etc. endpoints. They should be fixed.

I've pushed a version which moves this into settings (I don't like this though). We can move it back if this makes sense.

But then we must change this everywhere

Yes.

Actually we support this for data/upload dir:

resolwe/resolwe/flow/executors/docker/run.py

Line 158 in ada2c06

script = script.replace(map_['src'], map_['dest'])

I know, but this (doing search and replace in arbitrary scripts) is a very ugly hack that should be removed.

dblenkus · 2017-12-05T21:37:13Z

resolwe/flow/executors/prepare.py

-            pk=data_id
-        ).process)
+        files[ExecutorFiles.DATA] = model_to_dict(data)
+        files[ExecutorFiles.PROCESS] = model_to_dict(data.process)


dblenkus · 2017-12-05T21:44:31Z

resolwe/flow/managers/base.py


        # extend the settings with whatever the executor wants
-        self.executor.extend_settings(data_id, files)
+        self.executor.extend_settings(data_id, files, extra)


I don't like name extra as it doesn't apply that it contains sensitive data. Maybe secrets or protected_files?

On the other hand, the docstring for extend_settings specifies this plainly as a field for general raw files (as opposed to JSON serializations in the files parameter). Secrets are just one kind of opaque blob, so usage agrees with the docstring's specification.

But we also prevent listing of such dirs and restrict masks of files. So it may have unwanted consequences if you put such file directly into executor dir.
I would prefer to narrow use of this, make it secure (it already is) and clearly state that.

dblenkus · 2017-12-05T21:51:26Z

resolwe/flow/models/data.py

+                    contributor=self.contributor
+                )
+            except Secret.DoesNotExist:
+                raise PermissionDenied("Access to secret not allowed")


Or does not exist.

dblenkus · 2017-12-05T22:01:44Z

resolwe/flow/models/secret.py

+        return secret.value
+
+
+class Secret(models.Model):


I think we will need an extra to describe where the secret comes from. E.g. if you get user's API token first time, you don't have to ask for it in following requests.

We can add an arbitrary JSON metadata column for that?

dblenkus · 2017-12-05T22:04:12Z

resolwe/flow/tests/processes/secrets.yml

+  run:
+    program: |
+      # Echo secret to output. NEVER do this in an actual process!
+      re-save secret "$(cat /secrets/{{token.handle}})"


It would be useful to add Jinja filter for this, i.e. {{ token | get_secret }}

For which part? The whole command is specific for bash. Or did you mean just the {{token.handle}}?

I mean that you write {{ token | get_secret }} in the process and it is replaced with $(cat /secrets/{{token.handle}}) by Jinja filter.

That would make it much more user friendly.

dblenkus · 2017-12-05T22:22:50Z

resolwe/flow/models/secret.py

+        secret = self.create(value=value, contributor=contributor)
+        return str(secret.handle)
+
+    def get_secret(self, handle, contributor=None):


Is there a use-case where contributor is not defined? If not i would prefer to make it mandatory.

kostko · 2017-12-06T11:40:55Z

Now also pushed removal of FLOW_DOCKER_MAPPINGS.

dblenkus · 2017-12-06T14:36:32Z

resolwe/flow/executors/docker/run.py

-        mappings = SETTINGS.get('FLOW_DOCKER_MAPPINGS', {})
-        for map_ in mappings:
-            script = script.replace(map_['src'], map_['dest'])
+        # XXX: This is a huge hack and should be removed.


Can we mount everything on the same path as it is outside?

I don't think this is the correct way to go as it is much cleaner to have fixed volumes inside the container, regardless of what is outside. In this way you can always rely on specific paths when you run inside Docker.

It would be much better to always defer path translation to the executor (actually to the prepare part of the executor) as it is executor specific (e.g., the local executor does nothing, the Docker executor uses fixed volumes, etc.). Not just blindly using *_DIR from settings.

This hack has been removed in the latest commit.

hadalin · 2017-12-06T14:45:34Z

resolwe/flow/models/secret.py

+        :param value: Secret value to store
+        :param contributor: User owning the secret
+        :param metadata: Optional metadata dictionary (must be JSON serializable)
+        :param expires: Optional expiry time


Maybe state which units are used for expires?

Also maybe state it defaults to None.

kostko · 2017-12-06T18:28:11Z

@jberci please review my last commit, which fixes some issues with incorrect singleton use in Channels workers, which got exposed when removing hacky path translation.

jberci · 2017-12-07T12:17:40Z

LGTM so far. The global manager instance patching is still a bit hacky on the whole, but the Channel handler proxy will probably take care of that, since the manager will then be constructed once, as it was pre-channelization.

kostko · 2017-12-07T15:41:25Z

@jberci I've now pushed an updated version of the "Remove hacky path translation in Docker executor" commit, which separates the Channels consumer from the manager and uses the global manager instance instead.

hadalin · 2017-12-08T08:43:55Z

resolwe/flow/models/secret.py

+class SecretManager(models.Manager):
+    """Manager for Secret objects."""
+
+    def create_secret(self, value, contributor, metadata=None, expires=None):


Should this method always be used when creating secrets? If yes, then update docs and maybe also raise error when using the usual .create(. In this case it would also be nicer to return handle, secret to remove the need to query the secret by its handle if you intend to use the secret directly.

Alternatively if .create is allowed, then override it and call it in create_secret.

Both ways are ok I guess, but prevent confusion.

hadalin · 2017-12-08T09:22:21Z

resolwe/flow/models/secret.py

+    value = EncryptedTextField()
+
+    #: secret metadata (not encrypted)
+    metadata = JSONField(default=dict)


Since this is arbitrary data, should a naming convention be followed to prevent conflicts?

dblenkus · 2017-12-08T10:26:15Z

resolwe/flow/managers/base.py

        ))
+
+        # Ensure we have the correct executor loaded.
+        self.executor = self.load_executor(executor)  # pylint: disable=attribute-defined-outside-init


Is this still needed now when only the singleton manager is used?

This has already been changed (discover_engines is now called instead), but yes we need re-discovery as the executor may change with each message.

dblenkus · 2017-12-08T10:31:45Z

resolwe/flow/executors/run.py

+            logger.exception("Unhandled exception in executor")
+
+            # Send error report.
+            self.update_data_status(process_error=[str(error)], status=DATA_META['STATUS_ERROR'])


A quick question: are process_* fields extended when updated or are they overwritten? @jberci?

@jberci, ping

dblenkus · 2017-12-08T12:21:40Z

resolwe/flow/executors/docker/run.py

-                    for template in mappings_template]
+        # Setup Docker volumes.
+        def add_volume(volumes, kind, base_dir_name, volume, path=None, read_only=True):
+            """Add a new volume to the volumes list."""


Add description of arguments.

This is an internal helper, but ok, if you think that it should be added ;-)

You can add just a quick description of arguments. There are just a lot of them, so it takes some time to understand them :)

dblenkus · 2017-12-08T12:27:25Z

resolwe/flow/executors/docker/run.py


-        mappings.append({'src': passwd_file.name, 'dest': '/etc/passwd', 'mode': 'ro,Z'})
-        mappings.append({'src': group_file.name, 'dest': '/etc/group', 'mode': 'ro,Z'})
+        volumes.append({'src': passwd_file.name, 'dest': '/etc/passwd', 'options': 'ro,Z'})


It would be nice to make these options configurable.

self.volumes? Or move self.volumes = volumes.copy() to the end of the function.

self.volumes is no longer needed now that we got rid of hacky path translation, I forgot to remove it.

dblenkus · 2017-12-08T12:27:36Z

resolwe/flow/executors/docker/run.py

        # NOTE: Since the tools are shared among all containers they must use the shared SELinux
        # label (z option)
-        self.mappings_tools = [{'src': tool, 'dest': '/usr/local/bin/resolwe/{}'.format(i), 'mode': 'ro,z'}
+        self.mappings_tools = [{'src': tool, 'dest': '/usr/local/bin/resolwe/{}'.format(i), 'options': 'ro,z'}


This SELinux label will also be a problem on a shared storage.

I left this as it was before? So this was already a problem before?

Yes, it was already a problem before, I just pointed it out as the part for determining modes is being refactored.

Good point, I'll add another extra option for tools and one for passwd/group.

dblenkus · 2017-12-08T12:36:35Z

resolwe/test/testcases/setting_overrides.py


-
-# update FLOW_DOCKER_MAPPINGS setting if necessary
-def _get_updated_docker_mappings(previous_settings=getattr(settings, 'FLOW_EXECUTOR', {})):


dblenkus · 2017-12-08T12:38:45Z

resolwe/flow/execution_engines/bash/__init__.py

            inputs['proc'] = {
                'data_id': data.id,
-                'data_dir': settings.FLOW_EXECUTOR['DATA_DIR'],
+                'data_dir': self.manager.get_executor().resolve_data_path(data, '..'),


Maybe add dedicated function for this instead of passing .. as filename?

@kostko make the filename argument optional and return root data dir if it is not defined.

EDIT: Actually both parameters should be optional in this case.

dblenkus · 2017-12-08T12:40:25Z

resolwe/flow/managers/base.py



-class BaseManager(BaseConsumer):
+class BaseManager(object):


This should go into a separate commit.

dblenkus

Nice work!

tjanez

@kostko, very nice job!

Thanks for improving so many things in Resolwe.

Most of the comments are style improvements, so they should be easy to fix.

tjanez · 2017-12-12T08:24:08Z

resolwe/flow/executors/run.py

+            logger.exception("Unhandled exception in executor")
+
+            # Send error report.
+            self.update_data_status(process_error=[str(error)], status=DATA_META['STATUS_ERROR'])


@jberci, ping

tjanez · 2017-12-12T08:25:45Z

resolwe/flow/executors/run.py

+
+        if finish_fields is not None:
+            self._send_manager_command(ExecutorProtocol.FINISH, extra_fields=finish_fields)
+        # the feedback key deletes itself once the list is drained


Hmm... I don't understand this comment.

I know it's not yours but since you touched it, can you make it more elaborate?
Also, make it a sentence.

I found it confusing as well. It seems to be talking about the Redis list used for communication, which is deleted when all elements are popped? @jberci can you confirm this is the case?

It is the case, the comment is about Redis deleting the list once all items have been popped. This behaviour was not obvious to me when I was doing this. If memory serves, the code was also structured so that one would expect explicit key deletion at the end, so I put that comment there. Feel free to get rid of it.

Thanks for the explanation. I like @kostko's rewording and I think we can leave it in.

tjanez · 2017-12-12T08:44:42Z

resolwe/flow/managers/base.py

            with open(file_path, 'wt') as json_file:
                json.dump(files[file_name], json_file, cls=SettingsJSONifier)

+        # Save the secrets in the runtime dir, with permissions to prevent listing the given directory.


Could you wrap this line at 100 characters?

tjanez · 2017-12-12T08:46:09Z

resolwe/flow/managers/base.py


+        # Save the secrets in the runtime dir, with permissions to prevent listing the given directory.
+        secrets_dir = os.path.join(runtime_dir, ExecutorFiles.SECRETS_DIR)
+        os.makedirs(secrets_dir, mode=stat.S_IWUSR | stat.S_IXUSR)


To me, mode=0o300 is actually more readable than mode=stat.S_IWUSR | stat.S_IXUSR.

If you agree, change it.

tjanez · 2017-12-12T08:47:42Z

resolwe/flow/managers/base.py

+        self.executor.extend_settings(data_id, files, secrets)

-        # save the settings into the various files in the data dir
+        # save the settings into the various files in the runtime dir


Please, convert this to a sentence since you touched it 🙂 .

It should have been a sentence in the first place. I'll fix it.

tjanez · 2017-12-14T14:08:16Z

resolwe/flow/managers/consumer.py

@@ -0,0 +1,24 @@
+"""Manager Channels consumer."""


tjanez · 2017-12-14T14:20:23Z

resolwe/flow/executors/docker/prepare.py

+
+        :param data: Data object instance
+        :param filename: Filename to resolve
+        :return: Resolved filename, which can be used to access the given data


Please, wrap the docstring at 72 characters.

Should we have linters for these things? I never look at where I wrap things :-)

It's not yet implemented in pycodestyle, but the PR @ PyCQA/pycodestyle#674 seems on right track and that it will got approval from pycodestyle's maintainer.

tjanez · 2017-12-14T14:20:30Z

resolwe/flow/executors/docker/prepare.py

+        """Resolve upload path for use with the executor.
+
+        :param filename: Filename to resolve
+        :return: Resolved filename, which can be used to access the given uploaded


Please, wrap the docstring at 72 characters.

tjanez · 2017-12-14T14:21:13Z

resolwe/flow/executors/prepare.py

+
+        :param data: Data object instance
+        :param filename: Filename to resolve
+        :return: Resolved filename, which can be used to access the given data


Please, wrap the docstring at 72 characters.

tjanez · 2017-12-14T14:21:21Z

resolwe/flow/executors/prepare.py

+        """Resolve upload path for use with the executor.
+
+        :param filename: Filename to resolve
+        :return: Resolved filename, which can be used to access the given uploaded


Please, wrap the docstring at 72 characters.

dblenkus · 2017-12-17T17:29:28Z

resolwe/flow/executors/docker/run.py

+        volumes += SETTINGS.get('FLOW_DOCKER_EXTRA_VOLUMES', [])
+
+        # Create Docker --volume parameters from volumes.
+        command_args['volumes'] = ' '.join(['--volume="{src}":"{dest}":{options}'.format(**volume)


@kostko Will this work if options is empty? Or do you have to omit the colon?

Yes, it works if the options are empty (no need to omit the colon).

tjanez

Looks good!

kostko requested review from dblenkus, jberci and tjanez December 5, 2017 13:55

jberci approved these changes Dec 5, 2017

View reviewed changes

kostko force-pushed the feature/secrets branch from 4003ec2 to b226d82 Compare December 5, 2017 18:24

dblenkus reviewed Dec 5, 2017

View reviewed changes

kostko force-pushed the feature/secrets branch 3 times, most recently from 0e5addb to 991af17 Compare December 6, 2017 11:39

kostko force-pushed the feature/secrets branch from 991af17 to eec8bb6 Compare December 6, 2017 11:48

kostko changed the title ~~WIP: Add a way to pass secrets to processes in a controlled fashion~~ Add a way to pass secrets to processes in a controlled fashion Dec 6, 2017

kostko force-pushed the feature/secrets branch from eec8bb6 to b863e76 Compare December 6, 2017 12:26

dblenkus reviewed Dec 6, 2017

View reviewed changes

hadalin reviewed Dec 6, 2017

View reviewed changes

kostko force-pushed the feature/secrets branch from b863e76 to 2b4c789 Compare December 6, 2017 15:49

kostko force-pushed the feature/secrets branch 2 times, most recently from d37e16b to c525cf4 Compare December 6, 2017 19:05

kostko force-pushed the feature/secrets branch 2 times, most recently from 95c31dc to 5aa051e Compare December 7, 2017 15:40

kostko force-pushed the feature/secrets branch 2 times, most recently from e818f13 to 3d74236 Compare December 7, 2017 18:13

hadalin reviewed Dec 8, 2017

View reviewed changes

dblenkus reviewed Dec 8, 2017

View reviewed changes

kostko force-pushed the feature/secrets branch 2 times, most recently from 9f5a974 to b770740 Compare December 11, 2017 11:36

dblenkus approved these changes Dec 11, 2017

View reviewed changes

Set correct executor in flow manager

1967308

kostko force-pushed the feature/secrets branch from b770740 to 9cccf35 Compare December 13, 2017 00:09

tjanez suggested changes Dec 14, 2017

View reviewed changes

Make executors more robust to unhandled failures

ea9d425

kostko force-pushed the feature/secrets branch 2 times, most recently from cfa388e to 0b5b928 Compare December 15, 2017 01:35

dblenkus reviewed Dec 17, 2017

View reviewed changes

kostko force-pushed the feature/secrets branch from 0b5b928 to 3236bb1 Compare December 18, 2017 17:15

tjanez approved these changes Dec 19, 2017

View reviewed changes

kostko added 5 commits December 19, 2017 10:37

Add support for passing secrets to processes

e5e298c

Get rid of FLOW_DOCKER_MAPPINGS

d8afa63

Split manager and manager Channels consumer

d9f9b81

Remove hacky path translation in Docker executor

eb403e7

Ignore files created during tests

5c35716

kostko force-pushed the feature/secrets branch from 3236bb1 to 5c35716 Compare December 19, 2017 09:38

kostko merged commit 5c35716 into genialis:master Dec 19, 2017

kostko deleted the feature/secrets branch December 19, 2017 11:33



		# update FLOW_DOCKER_MAPPINGS setting if necessary
		def _get_updated_docker_mappings(previous_settings=getattr(settings, 'FLOW_EXECUTOR', {})):



		class BaseManager(BaseConsumer):
		class BaseManager(object):

Conversation

kostko commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jberci left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostko Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jberci Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblenkus Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblenkus Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostko commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kostko commented Dec 5, 2017 •

edited

Loading

codecov-io commented Dec 5, 2017 •

edited

Loading

kostko Dec 6, 2017 •

edited

Loading

jberci Dec 5, 2017 •

edited

Loading

dblenkus Dec 5, 2017 •

edited

Loading

dblenkus Dec 5, 2017 •

edited

Loading

hadalin Dec 6, 2017 •

edited

Loading

dblenkus Dec 8, 2017 •

edited

Loading