Feature #13135
closed
[CWL] Support for secrets
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.
Updated by Peter Amstutz almost 7 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz almost 7 years ago
- Description updated (diff)
- Status changed from In Progress to New
Updated by Peter Amstutz almost 7 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
Updated by Lucas Di Pentima almost 7 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/
Updated by Peter Amstutz almost 7 years ago
Fixed test, new test run in the queue: https://ci.curoverse.com/job/developer-run-tests/651
Updated by Peter Amstutz almost 7 years ago
Rebased one more time after merging #13134: 13135-cwl-secrets @ 16feb1a87c3e2e1955ad45caad0bd4b531f1cb26
Updated by Lucas Di Pentima almost 7 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.
Updated by Peter Amstutz almost 7 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.
Updated by Peter Amstutz almost 7 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|5282d5462d86721c73849c2e5f3d057657c74b51.
Updated by Tom Morris