Project

General

Profile

Actions

Feature #13135

closed

[CWL] Support for secrets

Added by Peter Amstutz over 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
03/13/2018
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

string type inputs to a workflow can be marked "secret: true"

arvados-cwl-runner ensures that secrets are obscured using "secret_mounts" in container requests.

Secrets are entered into a "secrets" list inside workflow runner.

When submitting a job, any mount or environment variable that contains any string in the "secrets" list is placed in "secret_mounts" or "secret_environment".

In addition, any command line argument that contains a secret could go into a "secret_command". (In container request, this is merged with the regular command line. Something like a list of null values or strings, null values are skipped, strings replace the corresponding position in the command line.)

Assumption: workflows don't modify the contents of secrets. This seems reasonable.

a-c-r logger has a filter that checks if any strings in the "secrets" list appears in output and obscures it.

When submitting workflow runner, any secrets are placed in file literals in "secret_mounts", the secret parameters appear in input.json file as an $include which reads the secret file contents when the runner executes.


Subtasks 1 (0 open1 closed)

Task #13214: Review 13135-cwl-secretsResolvedPeter Amstutz03/13/2018

Actions
Actions #1

Updated by Peter Amstutz over 4 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)
  • Status changed from In Progress to New
Actions #4

Updated by Peter Amstutz over 4 years ago

  • Status changed from New to In Progress
  • Assigned To set to Peter Amstutz
  • Target version changed from Arvados Future Sprints to 2018-03-14 Sprint
Actions #5

Updated by Lucas Di Pentima over 4 years ago

While I continue reviewing, I ran the test suite on jenkins and got an error: https://ci.curoverse.com/job/developer-run-tests/649/

Actions #6

Updated by Peter Amstutz over 4 years ago

Fixed test, new test run in the queue: https://ci.curoverse.com/job/developer-run-tests/651

Actions #7

Updated by Peter Amstutz over 4 years ago

Rebased one more time after merging #13134: 13135-cwl-secrets @ 16feb1a87c3e2e1955ad45caad0bd4b531f1cb26

Actions #8

Updated by Lucas Di Pentima over 4 years ago

Reviewing 16feb1a87

Don't want to make you wait more than you already did, so here are some questions:

  • File sdk/cwl/arvados_cwl/__init__.py
    • Line 359: Is this assignment needed? If so, can it be used on line 458 to improve readability?
  • File sdk/cwl/arvados_cwl/arvcontainer.py
    • Lines 57, 60: Can you explain where do self.command_line & self.environment come from?
  • File sdk/cwl/tests/wf/secret_job.cwl
    • Line 6 & 7: Documentation says that secrets must be defined at the toplevel workflow, so can these lines be deleted?

Apart from these, LGTM.

Actions #9

Updated by Peter Amstutz over 4 years ago

Lucas Di Pentima wrote:

Reviewing 16feb1a87

Don't want to make you wait more than you already did, so here are some questions:

  • File sdk/cwl/arvados_cwl/__init__.py
    • Line 359: Is this assignment needed? If so, can it be used on line 458 to improve readability?

Yes, because ArvadosContainer accesses arvrunner.secret_store. Fixed named parameter on line 458.

  • File sdk/cwl/arvados_cwl/arvcontainer.py
    • Lines 57, 60: Can you explain where do self.command_line & self.environment come from?

Added the following comment:

        # ArvadosCommandTool subclasses from cwltool.CommandLineTool,
        # which calls makeJobRunner() to get a new ArvadosContainer
        # object.  The fields that define execution such as
        # command_line, environment, etc are set on the
        # ArvadosContainer object by CommandLineTool.job() before
        # run() is called.
  • File sdk/cwl/tests/wf/secret_job.cwl
    • Line 6 & 7: Documentation says that secrets must be defined at the toplevel workflow, so can these lines be deleted?

Technically with the current implementation it isn't necessary, but secrets really should be regarded secret the whole way through, and in the future the static checker might check that you don't accidentally pass a secret to a non-secret.

Apart from these, LGTM.

Thanks, I'll merge.

Actions #10

Updated by Peter Amstutz over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100
Actions #11

Updated by Tom Morris about 4 years ago

  • Release set to 17
Actions

Also available in: Atom PDF