DEVPROD-4025 project variable redaction #7453

ybrill · 2024-01-25T17:31:17Z

Description

Redact sensitive default variables, private project variables, and suspicious (non-private) project variables.

Testing

On staging the logs had a redacted value for jonathans_super_secret_variable which is private and julian_password which is not private but has a suspicious pattern.

Successful race detector patch

Documentation

Added a line to the expansions section of the configuration file doc.

ybrill · 2024-01-25T17:34:17Z

cc @bsamek since you'd mentioned you might want to look over the PR.
I ended up going with strings.Replacer because the interface seemed more comfortable and maybe it's more efficient 🤷 . I started benchmarking but then thought getting this out the door was a higher priority.

bsamek

Thanks! LGTM, but @julianedwards should also review, since I'm not familiar with the logging subsystem.

julianedwards · 2024-01-25T17:59:35Z

agent/internal/client/redacting_sender.go

+	"github.com/mongodb/grip/send"
+)
+
+const redactedVariableTemplate = "<redacted:%s>"


Nit: could we make it in caps, something like<REDACTED:%s>, so it is more obvious to users in the logs?

agent/internal/client/interface.go

julianedwards · 2024-01-25T20:14:31Z

docs/Project-Configuration/Project-Configuration-Files.md

@@ -648,6 +648,9 @@ level on the project configuration page or on a build variant level
 within the project. They can be used **as inputs to commands**,
 including shell scripts.

+Expansion values defined on the project configurations page are redacted from task logs
+and replaced with \<redacted:expansion_key\>


This needs to be updated with <REDACTED:expansion_key>

julianedwards · 2024-01-25T20:15:43Z

agent/internal/client/redacting_sender_test.go

+				"secret_key": "secret_val",
+			},
+			inputString: "secret_val secret_val",
+			expected:    "<redacted:secret_key> <redacted:secret_key>",


These tests need to be updated, maybe use fmt.Sprintf(redactedVariableTemplate, <val>) instead of hardcoding it?

ybrill · 2024-01-25T22:17:51Z

Still works with the new strategy.

julianedwards

LGTM % race and broken tests

joe-dipilato · 2024-01-25T23:55:21Z

I think our log redaction might be a bit over eager.

See: https://spruce-staging.corp.mongodb.com/task/evg_ubuntu2204_echo_variable_patch_4225ee2a49a89afcd2b3deec7bb1c68c9810262b_65b27e5c97b1d34e31fe84ae_24_01_25_15_29_54/logs?execution=0&logtype=all

Can we skip some of the evergreen default expansions that we know are not secret? e.g. num_hosts
Should we consider a minimum length for redaction if it isn't marked as a 'secret' variable?
- it might get disruptive for projects that have short variables like max_attempts=12 have the number 12 get redacted anywhere it appears in a log.

/dev/root 7.9M 5<redacted:num_hosts>6K 7.<redacted:num_hosts>M 7% /

[2024/01/25 15:30:30.067] Filesystem     Inodes IUsed IFree IUse% Mounted on
[2024/01/25 15:30:30.067] /dev/root        7.9M  5<redacted:num_hosts>6K  7.<redacted:num_hosts>M    7% /
[2024/01/25 15:30:30.067] tmpfs            2.0M     2  2.0M    1% /dev/shm
[2024/01/25 15:30:30.067] tmpfs            800K   653  800K    1% /run
[2024/01/25 15:30:30.067] tmpfs            2.0M     3  2.0M    1% /run/lock
[2024/01/25 15:30:30.067] /dev/xvda15         0     0     0     - /boot/efi
[2024/01/25 15:30:30.067] /dev/xvdb        150M    <redacted:num_hosts>6  150M    1% /data
[2024/01/25 15:30:30.129] Cleaning up task because task is starting
[2024/01/25 15:30:30.129] Cleaning up processes for task: 'evg_ubuntu220<redacted:num_hosts>_echo_variable_patch_<redacted:num_hosts>225ee2a<redacted:num_hosts>9a89afcd2b3deec7bb1c68c9810262b_65b27e5c97b1d3<redacted:num_hosts>e31fe8<redacted:num_hosts>ae_2<redacted:num_hosts>_01_25_15_29_5<redacted:num_hosts>'.
[2024/01/25 15:30:30.142] Cleaned up processes for task: 'evg_ubuntu220<redacted:num_hosts>_echo_variable_patch_<redacted:num_hosts>225ee2a<redacted:num_hosts>9a89afcd2b3deec7bb1c68c9810262b_65b27e5c97b1d3<redacted:num_hosts>e31fe8<redacted:num_hosts>ae_2<redacted:num_hosts>_01_25_15_29_5<redacted:num_hosts>'.

julianedwards · 2024-01-26T01:53:33Z

I think our log redaction might be a bit over eager.

You are right. If someone has a project variable like max_attempts=12 any string 12 in the log output will be replaced with <REDACTED:max_attempts>. I think we should just redact private project variables and tell users they need to mark secrets as private.

…ynamic vars

ybrill · 2024-01-29T20:55:27Z

agent/logging.go

+			// Skip since the key is already in the redacted list.
+			continue


could it work to not continue here and to omit L200-202?

There is no guarantee that a redacted project variable will have a suspicious pattern so we definitely want the full list of those and then any additional project vars that are not private but have a suspicious pattern.

ybrill · 2024-01-29T21:00:05Z

agent/command/assume_ec2_role.go

-	conf.Expansions.Put(AWSSecretAccessKey, credValues.SecretAccessKey)
-	conf.Expansions.Put(AWSSessionToken, credValues.SessionToken)
-	conf.Expansions.Put(AWSRoleExpiration, expTime.String())
+	conf.NewExpansions.Put(AWSAccessKeyId, credValues.AccessKeyID)


do we need to Put into both?

No because it is wraps the same underlying map so doing it once is ok. I think eventually we would want to move everything in the agent to just use the the new DynamicExpansions.

malikchaya2

Would it be possible to add a test to check if old values are maintained in case they get logged at some later time after the expansion was updated?

For example: When token : apple gets updated to token : banana, do both apple and banana get redacted?

malikchaya2 · 2024-01-29T23:34:57Z

docs/Project-Configuration/Project-Configuration-Files.md

@@ -647,6 +647,11 @@ level on the project configuration page or on a build variant level
 within the project. They can be used **as inputs to commands**,
 including shell scripts.

+Expansion values defined on the project configurations page that
+    (1) are marked as private OR
+    (2) with key names containing any of the following case-insensitive patterns: `key`, `pass`, `pw`, `token`, `auth`, `private`


Should we add a blurb about how this shouldn't be relied on and it should be thought of as a last line of defense?

Good idea! Done.

malikchaya2 · 2024-01-29T23:43:20Z

agent/logging.go

+		}
+
+		for _, pattern := range suspiciousPatterns {
+			if strings.Contains(strings.ToLower(key), pattern) {


Why don't we also check the values for suspicious patterns? For example, we can look for things that are often at the start of certain keys like ghs_ for github.

To get this out and deployed I think we should just start with this set of patterns and expand as we see fit.

My suggestion is not just to expand the set of patterns, but to scan both keys and values. If getting this out quickly is the priority, would it maybe make sense to do this in a separate ticket?

Yep, that can be done here as well. We can add more scanning but the hope is that users will also rotate their secrets and mark their sensitive vars private.

malikchaya2 · 2024-01-29T23:47:20Z

agent/logging.go

@@ -116,6 +118,8 @@ func (a *Agent) prepLogger(tc *taskContext, c *model.LoggerConfig, commandName s
 		SendToGlobalSender: a.opts.SendTaskLogsToGlobalSender,
 		AWSCredentials:     pail.CreateAWSCredentials(tc.taskConfig.TaskSync.Key, tc.taskConfig.TaskSync.Secret, ""),
 	}
+	config.Expansions = tc.taskConfig.NewExpansions
+	config.ExpansionsToRedact = redactList(tc.taskConfig.ProjectVars, tc.taskConfig.Redacted)


We should probably also check during expansions.write and expansions.update if the new expansion needs to be redacted?

Expansions to redact are already checked in expansions.write.

I left the work to redact new private expansions created in expansions.update for DEVPROD-4107; I imagine that it will be much cleaner to implement that functionality together.

julianedwards · 2024-01-30T02:28:00Z

Would it be possible to add a test to check if old values are maintained in case they get logged at some later time after the expansion was updated?

I will leave that up to the app team to implement since it will require some thought around how to further extend the expansions class. This is really a first pass to introduce redaction since some is better than none.

malikchaya2 · 2024-01-30T15:38:46Z

Would it be possible to add a test to check if old values are maintained in case they get logged at some later time after the expansion was updated?

I will leave that up to the app team to implement since it will require some thought around how to further extend the expansions class. This is really a first pass to introduce redaction since some is better than none.

That makes sense. Can you please create a ticket for the follow up work?

malikchaya2

Approved after the two comments are addressed.

julianedwards · 2024-01-30T16:13:05Z

That makes sense. Can you please create a ticket for the follow up work?

DEVPROD-4025

ybrill added 4 commits January 24, 2024 17:20

redact project vars

e4196b8

redactor

ac5252f

test

f777a2d

add to expansions doc

d523117

ybrill requested a review from julianedwards January 25, 2024 17:31

bsamek approved these changes Jan 25, 2024

View reviewed changes

julianedwards requested changes Jan 25, 2024

View reviewed changes

caps

5d739df

julianedwards requested changes Jan 25, 2024

View reviewed changes

ybrill added 2 commits January 25, 2024 15:53

REDACTED

8b90056

different strategy

7323410

Merge branch 'main' into DEVPROD-4025_redaction

3c63800

ybrill requested a review from julianedwards January 25, 2024 22:19

julianedwards approved these changes Jan 25, 2024

View reviewed changes

julianedwards and others added 3 commits January 29, 2024 14:52

redact only private vars and suspicious non-private vars and handle d…

7419894

…ynamic vars

Merge branch 'main' into DEVPROD-4025_redaction

73dd8d6

update docs

d98ebd5

ybrill commented Jan 29, 2024

View reviewed changes

julianedwards added 2 commits January 29, 2024 16:09

use new expansions

563a15c

fix test

b583421

julianedwards requested a review from a team January 29, 2024 21:19

julianedwards added 3 commits January 29, 2024 16:20

fix more tests

41aa348

more fixes

bd66356

last one

aba7356

malikchaya2 requested changes Jan 29, 2024

View reviewed changes

julianedwards added 4 commits January 29, 2024 21:46

update docs

6ac68d2

fix docs

e8f029e

alphabetical order

27aa646

fix escape chars

3bbcdb7

julianedwards requested a review from malikchaya2 January 30, 2024 02:53

malikchaya2 approved these changes Jan 30, 2024

View reviewed changes

julianedwards merged commit 26c086e into evergreen-ci:main Jan 30, 2024
9 checks passed

ybrill mentioned this pull request Jan 31, 2024

DEVPROD-4025 fix redacting sender panic #7474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEVPROD-4025 project variable redaction #7453

DEVPROD-4025 project variable redaction #7453

ybrill commented Jan 25, 2024 •

edited by julianedwards

Loading

ybrill commented Jan 25, 2024

bsamek left a comment

julianedwards Jan 25, 2024

ybrill Jan 25, 2024

julianedwards Jan 25, 2024

ybrill Jan 25, 2024

julianedwards Jan 25, 2024

ybrill Jan 25, 2024

ybrill commented Jan 25, 2024

julianedwards left a comment

joe-dipilato commented Jan 25, 2024 •

edited

Loading

julianedwards commented Jan 26, 2024 •

edited

Loading

ybrill Jan 29, 2024

julianedwards Jan 29, 2024

ybrill Jan 29, 2024

julianedwards Jan 29, 2024

malikchaya2 left a comment

malikchaya2 Jan 29, 2024

julianedwards Jan 30, 2024

malikchaya2 Jan 29, 2024

julianedwards Jan 30, 2024

malikchaya2 Jan 30, 2024

julianedwards Jan 30, 2024

malikchaya2 Jan 29, 2024

julianedwards Jan 30, 2024 •

edited

Loading

julianedwards commented Jan 30, 2024

malikchaya2 commented Jan 30, 2024

malikchaya2 left a comment

julianedwards commented Jan 30, 2024

		// Skip since the key is already in the redacted list.
		continue

DEVPROD-4025 project variable redaction #7453

DEVPROD-4025 project variable redaction #7453

Conversation

ybrill commented Jan 25, 2024 • edited by julianedwards Loading

Description

Testing

Documentation

ybrill commented Jan 25, 2024

bsamek left a 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

ybrill commented Jan 25, 2024

julianedwards left a comment

Choose a reason for hiding this comment

joe-dipilato commented Jan 25, 2024 • edited Loading

julianedwards commented Jan 26, 2024 • 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

malikchaya2 left a 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

Choose a reason for hiding this comment

julianedwards Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

julianedwards commented Jan 30, 2024

malikchaya2 commented Jan 30, 2024

malikchaya2 left a comment

Choose a reason for hiding this comment

julianedwards commented Jan 30, 2024

ybrill commented Jan 25, 2024 •

edited by julianedwards

Loading

joe-dipilato commented Jan 25, 2024 •

edited

Loading

julianedwards commented Jan 26, 2024 •

edited

Loading

julianedwards Jan 30, 2024 •

edited

Loading