Feature #13135

[CWL] Support for secrets

Added by Peter Amstutz almost 2 years ago. Updated over 1 year 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

Task #13214: Review 13135-cwl-secretsResolvedPeter Amstutz

Associated revisions

Revision 5282d546
Added by Peter Amstutz over 1 year ago

Merge branch '13135-cwl-secrets' closes #13135

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz almost 2 years ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz almost 2 years ago

  • Description updated (diff)

#3 Updated by Peter Amstutz almost 2 years ago

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

#4 Updated by Peter Amstutz almost 2 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

#5 Updated by Lucas Di Pentima over 1 year 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/

#6 Updated by Peter Amstutz over 1 year ago

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

#7 Updated by Peter Amstutz over 1 year ago

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

#8 Updated by Lucas Di Pentima over 1 year 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.

#9 Updated by Peter Amstutz over 1 year 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.

#10 Updated by Peter Amstutz over 1 year ago

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

#11 Updated by Tom Morris over 1 year ago

  • Release set to 17

Also available in: Atom PDF