Project

General

Profile

Actions

Bug #15814

closed

Running a workflow from WB2 exposes secret inputs

Added by Bryan Cosca over 4 years ago. Updated 19 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Story points:
-

Description

In the container.json log of e51c5-xvhdp-ctxc9vlsbq7ae3x you can see:

                "api_key_per_sample": {
                    "$include": "/secrets/s0" 
                }

In the container.json log of e51c5-xvhdp-mkzv6lz6mnphv7b, you can see the api_key_per_sample in plaintext.

For reference, the way secrets are handled in arvados-cwl-runner:

  1. the submitter process takes the secret string and adds a "text" type mount at /secrets/s0 (s1, s2, etc) to the container request
  2. In the input object, the parameter is replaced with "$include": "/secrets/s0"
  3. The workflow runner process (inside the container) loads the input object and processes the $input directive, which reads /secrets/s0 and replaces it with the contents of the file
  4. The workflow runner internally swaps the secret for a placeholder to avoid printing it in logging (including debug logging)
  5. The command line tool uses InitialWorkDir to define the credential files
  6. It observes that the file contains the placeholder for the secret
  7. The file is moved to secret_mounts and the placeholder is replaced by the real secret
  8. secret_mounts are hidden from all API responses except when crunch-run requests the "self" container. Secrets are wiped from the database when the container is finished

Implementation

The part that workbench 2 needs to handle is:

  1. Recognizing which inputs are secrets (requires looking for cwltool:Secrets in the workflow's hints or requirements sections).
  2. Obscuring the secret with a "password" type text box
  3. When constructing the container request, moving secrets into the "secret_mounts" part, and replacing them in the input object with the $include reference.

Subtasks 1 (0 open1 closed)

Task #18709: Review 15814-wb2-secretsResolvedPeter Amstutz07/01/2024Actions

Related issues

Related to Arvados - Bug #20977: a-c-r crashes with "Secret store only accepts strings" if you try to register a workflow with secretsResolvedPeter AmstutzActions
Related to Arvados Epics - Idea #19132: Registered workflow improvementsIn Progress09/01/202309/30/2024Actions
Actions #1

Updated by Tom Morris over 4 years ago

  • Target version set to 2020-01-02 Sprint
Actions #2

Updated by Tom Morris over 4 years ago

  • Target version changed from 2020-01-02 Sprint to 2020-01-15 Sprint
Actions #3

Updated by Peter Amstutz over 4 years ago

The broader problem is that specifying secrets requires some extra work when creating the container request, and this code is essentially duplicated between arvados-cwl-runner and workbench. As a result, a-c-r knows how to do it, and workbench(1|2) doesn't. This almost certainly isn't the only example of inconsistent behavior between them.

I recommend exploring the option of submitting the workflow using a high level API, specifically the GA4GH Workflow Execution Service (WES). The implementation of the high level API can invoke arvados-cwl-runner which already has the logic to construct the low level container request. Submitted for consideration: #15918 #15917

Actions #4

Updated by Peter Amstutz over 4 years ago

  • Target version changed from 2020-01-15 Sprint to Arvados Future Sprints
Actions #5

Updated by Peter Amstutz over 4 years ago

  • Release set to 20
  • Target version deleted (Arvados Future Sprints)
Actions #6

Updated by Peter Amstutz over 2 years ago

  • Release changed from 20 to 46
  • Target version set to 2022-02-16 sprint
Actions #7

Updated by Peter Amstutz over 2 years ago

  • Assigned To set to Peter Amstutz
Actions #8

Updated by Peter Amstutz over 2 years ago

  • Subject changed from Running a workflow from WB1 & WB2 exposes secret inputs to Running a workflow from WB2 exposes secret inputs
Actions #9

Updated by Peter Amstutz over 2 years ago

  • Description updated (diff)
Actions #10

Updated by Peter Amstutz over 2 years ago

  • Description updated (diff)
Actions #11

Updated by Peter Amstutz over 2 years ago

  • Target version changed from 2022-02-16 sprint to 2022-03-02 sprint
Actions #12

Updated by Peter Amstutz over 2 years ago

  • Target version changed from 2022-03-02 sprint to 2022-03-16 sprint
Actions #13

Updated by Peter Amstutz over 2 years ago

  • Description updated (diff)
Actions #14

Updated by Peter Amstutz over 2 years ago

  • Project changed from 35 to Arvados
Actions #15

Updated by Peter Amstutz over 2 years ago

  • Assigned To changed from Peter Amstutz to Daniel Kutyła
Actions #16

Updated by Peter Amstutz over 2 years ago

  • Target version changed from 2022-03-16 sprint to 2022-04-13 Sprint
Actions #17

Updated by Peter Amstutz over 2 years ago

  • Release changed from 46 to 51
Actions #18

Updated by Peter Amstutz over 2 years ago

  • Target version changed from 2022-04-13 Sprint to 2022-04-27 Sprint
Actions #19

Updated by Peter Amstutz over 2 years ago

  • Target version changed from 2022-04-27 Sprint to 2022-05-11 sprint
Actions #20

Updated by Peter Amstutz over 2 years ago

  • Release deleted (51)
Actions #21

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-05-11 sprint to 2022-05-25 sprint
Actions #22

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-05-25 sprint to 2022-06-08 sprint
Actions #23

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-06-08 sprint to 2022-06-22 Sprint
Actions #24

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-06-22 Sprint to 2022-07-06
Actions #25

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-07-06 to 2022-07-20
Actions #26

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-07-20 to 2022-08-03 Sprint
Actions #27

Updated by Peter Amstutz about 2 years ago

  • Category set to Workbench2
Actions #28

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-08-03 Sprint to 2022-08-17 sprint
Actions #29

Updated by Peter Amstutz about 2 years ago

  • Target version changed from 2022-08-17 sprint to 2022-08-31 sprint
Actions #30

Updated by Peter Amstutz almost 2 years ago

  • Target version changed from 2022-08-31 sprint to 2022-09-28 sprint
Actions #31

Updated by Peter Amstutz almost 2 years ago

  • Target version changed from 2022-09-28 sprint to 2022-10-12 sprint
Actions #32

Updated by Peter Amstutz almost 2 years ago

  • Target version changed from 2022-10-12 sprint to 2022-10-26 sprint
Actions #33

Updated by Peter Amstutz almost 2 years ago

  • Target version changed from 2022-10-26 sprint to 2022-11-09 sprint
Actions #34

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2022-11-09 sprint to 2022-11-23 sprint
Actions #35

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2022-11-23 sprint to 2022-12-21 Sprint
Actions #36

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2022-12-21 Sprint to 2023-01-18 sprint
Actions #37

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2023-01-18 sprint to 2023-02-01 sprint
Actions #38

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2023-02-01 sprint to 2023-02-15 sprint
Actions #39

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2023-02-15 sprint to Future
Actions #40

Updated by Peter Amstutz 10 months ago

  • Assigned To deleted (Daniel Kutyła)
Actions #42

Updated by Brett Smith 10 months ago

  • Related to Bug #20977: a-c-r crashes with "Secret store only accepts strings" if you try to register a workflow with secrets added
Actions #43

Updated by Peter Amstutz 7 months ago

  • Target version changed from Future to Development 2024-01-17 sprint
Actions #44

Updated by Peter Amstutz 7 months ago

  • Target version changed from Development 2024-01-17 sprint to Development 2024-02-28 sprint
Actions #45

Updated by Peter Amstutz 7 months ago

  • Target version changed from Development 2024-02-28 sprint to Development 2024-01-31 sprint
Actions #46

Updated by Peter Amstutz 7 months ago

  • Target version changed from Development 2024-01-31 sprint to Development 2024-02-28 sprint
Actions #47

Updated by Peter Amstutz 5 months ago

  • Target version changed from Development 2024-02-28 sprint to Development 2024-03-13 sprint
Actions #48

Updated by Peter Amstutz 5 months ago

  • Target version changed from Development 2024-03-13 sprint to Development 2024-03-27 sprint
Actions #49

Updated by Peter Amstutz 5 months ago

  • Target version changed from Development 2024-03-27 sprint to Development 2024-04-10 sprint
Actions #50

Updated by Peter Amstutz 5 months ago

  • Target version changed from Development 2024-04-10 sprint to Development 2024-04-24 sprint
Actions #51

Updated by Peter Amstutz 3 months ago

  • Target version changed from Development 2024-04-24 sprint to Development 2024-05-08 sprint
Actions #52

Updated by Peter Amstutz 3 months ago

  • Target version changed from Development 2024-05-08 sprint to Development 2024-06-05 sprint
Actions #53

Updated by Peter Amstutz 3 months ago

  • Related to Idea #19132: Registered workflow improvements added
Actions #55

Updated by Peter Amstutz 3 months ago

  • Target version changed from Development 2024-06-05 sprint to Development 2024-05-22 sprint
Actions #56

Updated by Peter Amstutz 2 months ago

  • Target version changed from Development 2024-05-22 sprint to 439
Actions #57

Updated by Peter Amstutz about 2 months ago

  • Target version changed from 439 to Development 2024-06-19 sprint
Actions #58

Updated by Peter Amstutz about 2 months ago

  • Target version changed from Development 2024-06-19 sprint to Development 2024-07-03 sprint
Actions #59

Updated by Peter Amstutz about 1 month ago

  • Target version changed from Development 2024-07-03 sprint to Development 2024-06-19 sprint
Actions #60

Updated by Peter Amstutz about 1 month ago

  • Assigned To set to Peter Amstutz
Actions #62

Updated by Peter Amstutz about 1 month ago

  • Status changed from New to In Progress
Actions #63

Updated by Peter Amstutz about 1 month ago

  • Target version changed from Development 2024-06-19 sprint to Development 2024-07-03 sprint
Actions #64

Updated by Peter Amstutz about 1 month ago

15814-wb2-secrets @ 2ed8ccba7164836f69d15ae6d2cb1cbd2cd03b7c

developer-run-tests: #4301

  • All agreed upon points are implemented / addressed.
    • can now use arvados-cwl-runner --create-workflow on workflows with secrets
    • secret inputs are treated as password input and use 'password' type input
    • container request is constructed the same way that arvados-cwl-runner does it to avoid exposing passwords
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • n/a
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • added a new workbench integration test which goes through the process of creating a container request with a secret input.
  • Documentation has been updated.
    • n/a
  • Behaves appropriately at the intended scale (describe intended scale).
    • no changes in scale
  • Considered backwards and forwards compatibility issues between client and server.
    • the secrets handling feature has been around for many releases, this does not rely on any new API features
  • Follows our coding standards and GUI style guidelines.
    • yes

In the process of writing the test, I struggled a bit with Cypress. I made a few changes:

  • If run-tests.sh is run in "interactive" mode, then it will launch the cypress tests in "interactive" mode as well.
  • I added a "fail fast" plugin which will cause cypress to stop running tests in a particular spec file on the first failure. However, I set it up so it is disabled by default except when launched in interactive mode (previous bullet).
  • I ended up making some changes to a couple of "process" tests that were giving me trouble. I observed that some of the flaky tests are flaky because of race conditions where a cached value is displayed while it is waiting to get a new value from the server, but the check fails on the old cached value when it was expecting the new value. These situations require the test to be a bit more aggressive on enforcing sequencing and/or triggering a browser reload to prevent stale values.
Actions #66

Updated by Lucas Di Pentima 25 days ago

Review comments:

  • There's some missing indentation in createWorkflowSecretMounts() definition.
  • I believe that instead of writing conditionals like "if (something && something.other) {...}" you can simplify it with "if (something?.other) {...}"
  • There's also some funny indentations at
    • services/workbench2/src/store/run-process-panel/run-process-panel-actions.ts lines 160-200.
    • arvados/services/workbench2/src/models/workflow.ts lines 164-171
    • services/workbench2/cypress/e2e/process.cy.js lines 113-128, 212-218, 1335-1337, 1470-1472
    • services/workbench2/cypress.config.ts lines 16-25
    • arvados/services/workbench2/cypress/e2e/create-workflow.cy.js lines 287-335
  • When viewing the runner container, the corresponding input value for the secret shows as "Cannot display value" and while this is desirable, that input is not marked as a special one, so if its name doesn't hint the user that the value is a secret, using a better message could be helpful. Something like "Sensitive data hidden" or similar?
Actions #67

Updated by Peter Amstutz 23 days ago

Lucas Di Pentima wrote in #note-66:

  • I believe that instead of writing conditionals like "if (something && something.other) {...}" you can simplify it with "if (something?.other) {...}"

Done.

  • When viewing the runner container, the corresponding input value for the secret shows as "Cannot display value" and while this is desirable, that input is not marked as a special one, so if its name doesn't hint the user that the value is a secret, using a better message could be helpful. Something like "Sensitive data hidden" or similar?

Detected explicitly and says "Cannot display secret" now.

Fixed all the wonky indentation, it was using tabs, I need to fix my untabify-on-save hook.

15814-wb2-secrets @ d8307bae7ebd60c20ecaf445f29bf4e229941e55

developer-run-tests: #4320

Actions #69

Updated by Lucas Di Pentima 19 days ago

This LGTM, thanks!

Actions #70

Updated by Peter Amstutz 19 days ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF