Retrieve exclusion list from environment variable #1164

0x2b3bfa0 · 2022-09-12T21:43:55Z

Suggested solution for #802, along with iterative/terraform-provider-iterative#665

dacbd · 2022-09-12T22:39:56Z

I haven't tested this yet. And I don't think this should be the permanent solution. I am in favor of setting up the CI agents as their own service that the runner monitors so that their parent is systemd and not cml, thus avoiding the env leaks.

⚠️

This should have a blog post for some migration information on how to use the --cloud-permission-set for all three providers as the leaking (in the case of AWS) is slightly displayed as a feature example:

tasdomas · 2022-09-13T08:39:26Z

src/cml.js

-    return await this.getDriver().startRunner(opts);
+    const env = {};
+    const sensitive =
+      ['CML_RUNNER_SENSITIVE_ENV'] +


Would a whitelist of environment variables to propagate be a more robust solution here?

Indeed, that's what I originally proposed:

cml-runner self-hosted issues #721 (comment)

https://iterativeai.slack.com/archives/C01900GSB4J/p1631893922227800

https://iterativeai.slack.com/archives/C01900GSB4J/p1632223956257700

Although that would require launching the runner process from a shell, to provide the bare minimum variables like e.g. PATH and any custom NVIDIA values.

Again

I am in favor of setting up the CI agents as their own service that the runner monitors so that their parent is systemd and not cml, thus avoiding the env leaks.

would also solve it, would it not?

would also solve it, would it not?

Except for Kubernetes

would also solve it, would it not?

It depends how you would like to solve this. Cloud agnostic should be the way that way CML is secure in every environment. I might want to just launch CML on my own

const sensitive = ['CML_RUNNER_SENSITIVE_ENV'] + process.env.CML_RUNNER_SENSITIVE_ENV.split(':'); for (const variable in process.env) if (!sensitive.includes(variable)) env[variable]

This does not sound to me correct. Have you tried this?

in JS + concatenates strings so I think that this is calling the toSting() incarnation of them giving you

CML_RUNNER_SENSITIVE_ENVone,two,three

for process.env.CML_RUNNER_SENSITIVE_ENV = ['one:two:three'] ?

this does indeed not concat correctly, though using string.includes negates whether it really matters

It works, but it's definitely not correct. 🤦🏼 Thanks!

JavaScript blunder fixed with e29b8aa (commit comment worth watching)

Would a whitelist of environment variables to propagate be a more robust solution here?

Better solutions are possible, but also more complicated.

dacbd

yargs collides with your naming choice CML_RUNNER_*

Sep 14 18:07:44 ip-172-31-21-33 systemd[1]: Started cml.service.
Sep 14 18:07:44 ip-172-31-21-33 cml.sh[2632]:   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
Sep 14 18:07:44 ip-172-31-21-33 cml.sh[2632]:                                  Dload  Upload   Total   Spent    Left  Speed
Sep 14 18:07:44 ip-172-31-21-33 cml.sh[2632]: [158B blob data]
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]: cml runner
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]: Manage self-hosted (cloud & on-premise) CI runners
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]: Commands:
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:   cml runner launch  Launch and register a self-hosted runner
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]: Global Options:
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:   --log     Logging verbosity
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:           [string] [choices: "error", "warn", "info", "debug"] [default: "info"]
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:   --driver  Git provider where the repository is hosted
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:     [string] [choices: "github", "gitlab", "bitbucket"] [default: infer from the
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:                                                                     environment]
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:   --repo    Repository URL or slug[string] [default: infer from the environment]
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:   --token   Personal access token [string] [default: infer from the environment]
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]:   --help    Show help                                                  [boolean]
Sep 14 18:07:45 ip-172-31-21-33 cml.sh[2632]: Unknown argument: sensitiveEnv
Sep 14 18:07:45 ip-172-31-21-33 systemd[1]: cml.service: Main process exited, code=exited, status=1/FAILURE
Sep 14 18:07:45 ip-172-31-21-33 systemd[1]: cml.service: Failed with result 'exit-code'.

DavidGOrtega · 2022-09-21T13:34:44Z

Would be awesome if in the description PR you could explain how this is set or used.

Maybe I would prefer ; as a separator

DavidGOrtega · 2022-09-21T14:46:14Z

src/cml.js

-    return await this.getDriver().startRunner(opts);
+    const env = {};
+    const sensitive =
+      ['CML_RUNNER_SENSITIVE_ENV'] +


const sensitive = ['CML_RUNNER_SENSITIVE_ENV'] + process.env.CML_RUNNER_SENSITIVE_ENV.split(':'); for (const variable in process.env) if (!sensitive.includes(variable)) env[variable]

This does not sound to me correct. Have you tried this?

in JS + concatenates strings so I think that this is calling the toSting() incarnation of them giving you

CML_RUNNER_SENSITIVE_ENVone,two,three

for process.env.CML_RUNNER_SENSITIVE_ENV = ['one:two:three'] ?

dacbd · 2022-09-21T15:13:20Z

Would be awesome if in the description PR you could explain how this is set or used.

Maybe I would prefer ; as a separator

it is set in the linked PR for tpi: iterative/terraform-provider-iterative#665

DavidGOrtega · 2022-09-21T15:14:50Z

it is set in the linked PR for tpi: iterative/terraform-provider-iterative#665

This PR assumes :

dacbd · 2022-09-21T15:15:52Z

it is set in the linked PR for tpi: iterative/terraform-provider-iterative#665

This PR assumes :

sorry for only for the first part of your question, how it is set/used

0x2b3bfa0 · 2022-09-21T15:30:58Z

Maybe I would prefer ; as a separator
This PR assumes :

I'd rather prefer the ⚔️ emoji as a separator, but there is a clear reason to choose : over ; to separate items: VALUES=ONE;TWO would produce sh: command not found: TWO because ; is used by the shell to separate command lists. Of course, quoting is always an option. 🤷🏼‍♂️

src/cml.js

src/drivers/gitlab.js

src/cml.js

https://www.youtube.com/watch?v=vcFBwt1nu2U

0x2b3bfa0 · 2022-10-10T05:25:25Z

Reply to #1164 (comment)

I don't think this should be the permanent solution

Neither do I 👍🏼

I am in favor of setting up the CI agents as their own service that the runner monitors so that their parent is systemd and not cml, thus avoiding the env leaks.

Sounds worth considering, although would effectively separate cml runner --cloud from cml runner 👍🏼

This should have a blog post for some migration information on how to use the --cloud-permission-set for all three providers as the leaking (in the case of AWS) is slightly displayed as a feature

Looks like the HCL snippet at iterative/terraform-provider-iterative#602 (comment) could be a good starting point

0x2b3bfa0 · 2022-10-11T23:11:51Z

@iterative/cml, cascading to #802

casperdcl · 2022-10-24T22:52:04Z

as the leaking (in the case of AWS) is slightly displayed as a feature example:

That's poor wording on the cml.dev/doc pages. The intention was to say "users only need to go through the nightmare of obtaining cloud auth tokens once for both cloud storage AND provisioning." It was NOT meant to imply "we inject your provisioning-auth-tokens into the provisioned machine just in case you want to use them for something else."

Retrieve exclusion list from environment variable

36da045

0x2b3bfa0 temporarily deployed to internal September 12, 2022 21:44 Inactive

This was referenced Sep 12, 2022

Mark sensitive environment variables for runners iterative/terraform-provider-iterative#665

Open

Add exclusion list for environment variables #802

Merged

tasdomas reviewed Sep 13, 2022

View reviewed changes

dacbd self-requested a review September 14, 2022 18:29

dacbd suggested changes Sep 14, 2022

View reviewed changes

dacbd assigned 0x2b3bfa0 Sep 14, 2022

DavidGOrtega suggested changes Sep 21, 2022

View reviewed changes

0x2b3bfa0 commented Oct 10, 2022

View reviewed changes

src/cml.js Outdated Show resolved Hide resolved

src/cml.js Outdated Show resolved Hide resolved

Apply suggestions from code review

aca6c5b

0x2b3bfa0 temporarily deployed to internal October 10, 2022 04:00 Inactive

0x2b3bfa0 requested review from DavidGOrtega and dacbd October 10, 2022 04:00

github-advanced-security bot found potential problems Oct 10, 2022

View reviewed changes

src/drivers/gitlab.js Show resolved Hide resolved

0x2b3bfa0 commented Oct 10, 2022

View reviewed changes

src/cml.js Outdated Show resolved Hide resolved

Fix “the blunder of the century”

e29b8aa

https://www.youtube.com/watch?v=vcFBwt1nu2U

0x2b3bfa0 temporarily deployed to internal October 10, 2022 04:05 Inactive

0x2b3bfa0 requested review from a team, tasdomas and dacbd and removed request for DavidGOrtega and dacbd October 11, 2022 23:11

0x2b3bfa0 requested a review from DavidGOrtega October 11, 2022 23:11

0x2b3bfa0 merged commit daa7cde into no-envars Oct 11, 2022

0x2b3bfa0 deleted the suggestion/no-envvars branch October 11, 2022 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve exclusion list from environment variable #1164

Retrieve exclusion list from environment variable #1164

0x2b3bfa0 commented Sep 12, 2022 •

edited

Loading

dacbd commented Sep 12, 2022

tasdomas Sep 13, 2022

0x2b3bfa0 Sep 13, 2022

dacbd Sep 14, 2022

0x2b3bfa0 Sep 14, 2022

DavidGOrtega Sep 21, 2022

DavidGOrtega Sep 21, 2022 •

edited

Loading

dacbd Sep 21, 2022

0x2b3bfa0 Sep 21, 2022

0x2b3bfa0 Oct 10, 2022

0x2b3bfa0 Oct 10, 2022 •

edited

Loading

dacbd left a comment

DavidGOrtega commented Sep 21, 2022 •

edited

Loading

DavidGOrtega Sep 21, 2022 •

edited

Loading

dacbd commented Sep 21, 2022

DavidGOrtega commented Sep 21, 2022

dacbd commented Sep 21, 2022

0x2b3bfa0 commented Sep 21, 2022

0x2b3bfa0 commented Oct 10, 2022 •

edited

Loading

0x2b3bfa0 commented Oct 11, 2022 •

edited

Loading

casperdcl commented Oct 24, 2022

Retrieve exclusion list from environment variable #1164

Retrieve exclusion list from environment variable #1164

Conversation

0x2b3bfa0 commented Sep 12, 2022 • edited Loading

dacbd commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGOrtega Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x2b3bfa0 Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

dacbd left a comment

Choose a reason for hiding this comment

DavidGOrtega commented Sep 21, 2022 • edited Loading

DavidGOrtega Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

dacbd commented Sep 21, 2022

DavidGOrtega commented Sep 21, 2022

dacbd commented Sep 21, 2022

0x2b3bfa0 commented Sep 21, 2022

0x2b3bfa0 commented Oct 10, 2022 • edited Loading

Reply to #1164 (comment)

0x2b3bfa0 commented Oct 11, 2022 • edited Loading

casperdcl commented Oct 24, 2022

0x2b3bfa0 commented Sep 12, 2022 •

edited

Loading

DavidGOrtega Sep 21, 2022 •

edited

Loading

0x2b3bfa0 Oct 10, 2022 •

edited

Loading

DavidGOrtega commented Sep 21, 2022 •

edited

Loading

DavidGOrtega Sep 21, 2022 •

edited

Loading

0x2b3bfa0 commented Oct 10, 2022 •

edited

Loading

0x2b3bfa0 commented Oct 11, 2022 •

edited

Loading