feat(file) implement env var substitution for state files #286

hbagdi · 2021-03-07T19:59:33Z

Introducing support for envirment variable substitution in state files.
This is primarily intended for injecting sensitive data at runtime.

Fix #191

mflendrich

I think this can be a security vulnerability (used to exfiltrate environment variables if the deck file is controlled by an actor interested in host's environment variables).

Let's discuss how to mitigate this risk. My first guess is that restricting available env vars e.g. to those sharing a predefined prefix (e.g. DECK_ENV_) might solve it.

stale · 2021-03-22T18:02:33Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

hbagdi · 2021-04-01T11:05:27Z

There are two ways to achieve this:

Ask user to include DECK_ prefix to all env vars in the state file.
Implicitly append (and document accordingly) DECK_ prefix to any invocation, eg: ${{ env "FOO" }} will be substituted with the value of DECK_FOO.

Which one do you prefer?

mflendrich · 2021-04-06T12:17:48Z

@hbagdi

Which one do you prefer?

I find the former less "magical" and less confusing. The latter approach seems acceptable too, if it gets documented well enough.

rainest · 2021-04-07T23:08:01Z

I'm also in favor of the first option, where users must include the DECK_ prefix on variables in the state file. Using the same variable name in both the state file and environment feels simplest, and users need to be aware of the requirement either way to set the properly-named variables in the environment.

hbagdi · 2021-04-08T13:53:43Z

@mflendrich @rainest Perfect. I get an odd sense of happiness we all agree AND use the exact same argument, however small the issue maybe.
I've updated the patch to match our discussion.
I'm not sure what is up with dependabot.

mflendrich · 2021-04-08T21:19:16Z

file/readfile.go

+		"env": func(key string) (string, error) {
+			if !strings.HasPrefix(key, envVarPrefix) {
+				return "", fmt.Errorf("environment variables in the state file must "+
+					"be prefixed with 'DECK_', found: '%s'", key)
+			}
+			return os.Getenv(key), nil
+		},


For readability and maintainability, please extract the lambda function into a named function, for example func GetPrefixedEnvVar

mflendrich · 2021-04-08T21:23:15Z

file/readfile.go

@@ -132,3 +140,26 @@ func configFilesInDir(dir string) ([]string, error) {
 	}
 	return res, nil
 }
+
+func processEnvs(content string) (string, error) {


This function is in fact a gotemplate rendering mapper. It happens to have env as its defined function, but may include other ones in the future.

I recommend that we redefine the responsibility of this function from "fill in env vars" to "render the deck file template" and rename it accordingly.

mflendrich · 2021-04-08T21:26:27Z

file/readfile.go

+	if err != nil {
+		return nil, fmt.Errorf("parsing file: %w", err)
+	}
+	bytes = []byte(processedContent)


Please don't reuse the same []byte for input and output of template rendering. From a strongly typed perspective, these two values represent different data types (unrendered template versus rendered document)

I get the point somewhat but not enough.
Why does it really matter? Or maybe when?

I do agree that this way of thinking is correct, I just don't completely grasp it so please help me improve my understanding here.

And since I agree, I have already updated the patch,

mflendrich · 2021-04-08T21:28:51Z

file/readfile.go

@@ -2,6 +2,9 @@ package file

 import (
 	"bufio"
+	"bytes"
+	"fmt"
+	"html/template"


You probably meant text/template. html/template will perform additional escaping which I think is undesirable (will break valid YAML) here.

Please add a test ensuring that the template renderer doesn't escape yaml characters like & or <.

hbagdi · 2021-04-12T17:31:43Z

@mflendrich @rainest Friendly ping to review this when you get a chance.

rainest

Using < in tags to check that we're not garbling YAML seems a bit off since it's not actually a valid tag, but ultimately that doesn't cause issues in unit tests.

I found one UX gotcha during practical testing: unset variables in the template will (probably, depending on the field type) result in an invalid state, and the resulting error doesn't make the issue cause clear. For example, attempting to apply file/testdata/file.yaml without setting DECK_SVC2_HOST results in:

Error: reading file: validating file content: 1 errors occurred:
	services.0.host: Invalid type. Expected: string, given: null

With the attached change:

Error: reading file: parsing file: template: state:3:12: executing "state" at <env "DECK_SVC2_HOST">: error calling env: environment variable 'DECK_SVC2_HOST' present in state file but not set

While the complete error unwrap looks a bit silly, it's at least clearer than the type error that you'll hit later.

Originally attempted to use PR suggestions, and then realized that works horribly when you need to include tabs, so git apply /tmp/lookup.diff.txt: lookup.diff.txt

hbagdi · 2021-04-14T18:04:14Z

I found one UX gotcha during practical testing: unset variables in the template will (probably, depending on the field type) result in an invalid state, and the resulting error doesn't make the issue cause clear.

Fair. What if we error out early whenever we encounter an unset environment variable?

Introducing support for envirment variable substitution in state files. This is primarily intended for injecting sensitive data at runtime. Fix #191

hbagdi · 2021-04-14T19:40:14Z

@rainest I updated the code to include your patch and also added a test case for it. PTAL.

ping @mflendrich for another review.

file/testdata/file.yaml

Co-authored-by: Michał Flendrich <michal@flendrich.pro>

Introducing support for environment variable substitution in state files. This is primarily intended for injecting sensitive data at runtime. Fix #191 Co-authored-by: Michał Flendrich <michal@flendrich.pro>

hbagdi requested a review from a team as a code owner March 7, 2021 19:59

hbagdi requested a review from cwurm March 7, 2021 19:59

hbagdi mentioned this pull request Mar 7, 2021

Deck support placeholder replace #191

Closed

mflendrich suggested changes Mar 8, 2021

View reviewed changes

stale bot added the stale Will be closed unless advocated for within 7 days label Mar 22, 2021

stale bot closed this Mar 29, 2021

hbagdi reopened this Mar 29, 2021

stale bot removed the stale Will be closed unless advocated for within 7 days label Mar 29, 2021

hbagdi force-pushed the feat/read-env branch from ffbbc92 to 73c5a33 Compare April 8, 2021 13:49

mflendrich reviewed Apr 8, 2021

View reviewed changes

hbagdi force-pushed the feat/read-env branch from 73c5a33 to 569d2b1 Compare April 8, 2021 22:09

hbagdi force-pushed the feat/read-env branch from 569d2b1 to 9ec30d3 Compare April 13, 2021 21:26

rainest suggested changes Apr 13, 2021

View reviewed changes

feat(file) implement env var substitution for state files

6548e4d

Introducing support for envirment variable substitution in state files. This is primarily intended for injecting sensitive data at runtime. Fix #191

hbagdi force-pushed the feat/read-env branch from 9ec30d3 to 6548e4d Compare April 14, 2021 19:39

rainest approved these changes Apr 14, 2021

View reviewed changes

mflendrich approved these changes Apr 15, 2021

View reviewed changes

file/testdata/file.yaml Outdated Show resolved Hide resolved

Update file/testdata/file.yaml

f1655f3

Co-authored-by: Michał Flendrich <michal@flendrich.pro>

hbagdi merged commit 6ede1a8 into main Apr 15, 2021

hbagdi deleted the feat/read-env branch April 15, 2021 15:15

mflendrich mentioned this pull request Apr 21, 2021

Ensure cobra command isolation #334

Open

mflendrich mentioned this pull request May 19, 2021

feat: implement file template function #394

Closed

carnei-ro mentioned this pull request Jul 14, 2021

Redact value from placeholders #462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(file) implement env var substitution for state files #286

feat(file) implement env var substitution for state files #286

hbagdi commented Mar 7, 2021

mflendrich left a comment

stale bot commented Mar 22, 2021

hbagdi commented Apr 1, 2021 •

edited

Loading

mflendrich commented Apr 6, 2021

rainest commented Apr 7, 2021

hbagdi commented Apr 8, 2021

mflendrich Apr 8, 2021

hbagdi Apr 8, 2021

mflendrich Apr 8, 2021

hbagdi Apr 8, 2021

mflendrich Apr 8, 2021

hbagdi Apr 8, 2021

mflendrich Apr 8, 2021

hbagdi Apr 8, 2021

hbagdi commented Apr 12, 2021

rainest left a comment •

edited

Loading

hbagdi commented Apr 14, 2021

hbagdi commented Apr 14, 2021 •

edited

Loading

feat(file) implement env var substitution for state files #286

feat(file) implement env var substitution for state files #286

Conversation

hbagdi commented Mar 7, 2021

mflendrich left a comment

Choose a reason for hiding this comment

stale bot commented Mar 22, 2021

hbagdi commented Apr 1, 2021 • edited Loading

mflendrich commented Apr 6, 2021

rainest commented Apr 7, 2021

hbagdi commented Apr 8, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Apr 12, 2021

rainest left a comment • edited Loading

Choose a reason for hiding this comment

hbagdi commented Apr 14, 2021

hbagdi commented Apr 14, 2021 • edited Loading

hbagdi commented Apr 1, 2021 •

edited

Loading

rainest left a comment •

edited

Loading

hbagdi commented Apr 14, 2021 •

edited

Loading