DEVPROD-10509 Update git.clone project command to redact for modules #8330

ZackarySantana · 2024-09-20T15:54:31Z

DEVPROD-10509

Description

This ticket cleans up some of the redacting logic and adds redaction to the generated tokens.

It also fixes a bug where modules would still generate a token even if the user passed in an access token.

Testing

Existing unit tests

This task on staging after the fix, the token for git clone ... github-merge-queue-sandbox is redacted.

ZackarySantana · 2024-09-20T17:58:55Z

agent/command/git.go


 		logger.Execution().Info("Fetching source from git...")
-		redactedCmds := fetchScript


Our redaction logic takes care of this

ZackarySantana · 2024-09-20T17:59:09Z

agent/command/git.go

-		out := stdErr.String()
-		if out != "" {
-			if opts.token != "" {
-				out = strings.Replace(out, opts.token, "[redacted github token]", -1)


Our redaction logic takes care of this

ZackarySantana · 2024-09-20T17:59:34Z

agent/command/git.go

@@ -725,17 +716,19 @@ func (c *gitFetchProject) fetchModuleSource(ctx context.Context,
 		return errors.Wrap(err, "setting location to clone from")
 	}

-	if opts.method == cloneMethodOAuth {
+	if opts.method == cloneMethodOAuth || opts.method == cloneMethodAccessToken {


This fixes the problem where modules always generate an installation token regardless if the user passed in an access token

ZackarySantana · 2024-09-20T17:59:49Z

agent/command/git.go

 		// Otherwise, create an installation token for to clone the module.
 		// Fallback to the legacy global token if the token cannot be created.
 		appToken, err := comm.CreateInstallationToken(ctx, td, opts.owner, opts.repo)
 		if err == nil {
 			opts.token = appToken
 			opts.method = cloneMethodAccessToken
+
+			// After generating, redact the token from the logs.
+			conf.NewExpansions.Redact("EVERGREEN_GENERATED_GITHUB_TOKEN", appToken)


Always after creating an installation token on the fly, we should redact it

ZackarySantana · 2024-09-20T18:00:01Z

agent/command/git.go

@@ -791,9 +784,7 @@ func (c *gitFetchProject) fetchModuleSource(ctx context.Context,

 		errOutput := stdErr.String()
 		if errOutput != "" {
-			if opts.token != "" {
-				errOutput = strings.Replace(errOutput, opts.token, "[redacted github token]", -1)


Our redaction logic should take care of this.

ZackarySantana · 2024-09-20T18:00:14Z

agent/command/git.go

-			if opts.token != "" {
-				errOutput = strings.Replace(errOutput, opts.token, "[redacted github token]", -1)
-			}
+			errOutput = strings.ReplaceAll(errOutput, "\n", fmt.Sprintf("\n%s: ", module.Name))


I added this line to append every line with the module name for easier debugging

ZackarySantana · 2024-09-20T18:00:26Z

agent/internal/client/base_client.go

@@ -403,7 +403,7 @@ func (c *baseCommunicator) makeSender(ctx context.Context, tsk *task.Task, confi
 	levelInfo := send.LevelInfo{Default: level.Info, Threshold: level.Debug}
 	var senders []send.Sender
 	if config.SendToGlobalSender {
-		senders = append(senders, grip.GetSender())
+		senders = append(senders, redactor.NewRedactingSender(grip.GetSender(), config.RedactorOpts))


This adds the redactor to a global sender, just in case

ZackarySantana · 2024-09-20T18:00:48Z

model/project.go

@@ -620,11 +620,6 @@ func (c *PluginCommandConf) unmarshalParams() error {
 	return nil
 }

-type ArtifactInstructions struct {


Random unused code

ZackarySantana · 2024-09-24T15:05:53Z

I'm waiting for a review to update the agent version because of merge conflicts

agent/util/expansions.go

agent/command/git.go

Kimchelly · 2024-09-25T14:32:35Z

agent/command/git_test.go

@@ -461,8 +462,8 @@ func (s *GitGetProjectSuite) TestBuildHTTPCloneCommand() {
 	s.Require().Len(cmds, 5)
 	s.True(utility.ContainsOrderedSubset(cmds, []string{
 		"set +o xtrace",
-		"echo \"git clone https://[redacted github token]:[email protected]/evergreen-ci/sample.git 'dir' --branch 'main'\"",
-		"git clone https://PROJECTTOKEN:[email protected]/evergreen-ci/sample.git 'dir' --branch 'main'",
+		fmt.Sprintf("echo \"git clone https://%s:[email protected]/evergreen-ci/sample.git 'dir' --branch 'main'\"", projectGitHubToken),


Since these now return the unredacted commands, are there other tests that cover the functionality that GetLoggerProducer loggers redact sensitive secrets? Reading through some of the logic, it looks like the relies on some slightly risky assumptions, like the fact that the expansions must be passed as a pointer to the redaction opts so that the memory is shared between the task config and the redacting sender.

If not, we should either add new tests or make a separate ticket to add tests that the loggers do redact correctly because it's critical functionality at this point and we shouldn't rely on manual testing to verify that.

I improved the test in this file (here) and I added another more specific test here to the base communicator.

The git test uses the mock communicator, so it isn't a full on test of how it would work in production.

The test on the base communicator is more of a test on how it will work on production.

Kimchelly

LGTM mod couple nits.

Kimchelly · 2024-09-25T17:30:12Z

agent/internal/client/client_test.go

@@ -23,6 +29,88 @@ func TestEvergreenCommunicatorConstructor(t *testing.T) {
 	assert.Equal(t, defaultTimeoutMax, c.retry.MaxDelay)
 }

+func TestLoggerProducerRedactorOptions(t *testing.T) {
+	secret := "super_soccer_ball"


Kimchelly · 2024-09-25T17:32:38Z

agent/command/git_test.go

+	findTokenInRedacted := func() bool {
+		for _, redacted := range conf.NewExpansions.GetRedacted() {
+			if redacted.Key == generatedTokenKey {
+				if redacted.Value == token {


mega nit: could combine the two ifs into one if redacted.Key == generatedTokenKey && redacted.Value == token

Thanks, I originally did two different booleans (found and then correctValue) but this is an artifact from that

Kimchelly · 2024-09-25T17:33:51Z

agent/internal/client/client_test.go

+	t.Run("LeaksWithoutRedactor", func(t *testing.T) {
+		task := createTask()
+		comm := newBaseCommunicator("whatever", map[string]string{})
+		logger, err := comm.GetLoggerProducer(context.Background(), task, nil)


nit: use a context.WithCancel if possible rather than plain context.Background().

Same with the test below.

Kimchelly · 2024-09-25T17:35:26Z

agent/internal/client/client_test.go

+
+		data := readLogs(t, task)
+		assert.NotContains(t, data, secret)
+		assert.Contains(t, data, "secret_key")


nit: might be clearer that this is checking for the redacted string by explicitly looking for <REDACTED:secret_key>.

I'll update it, I was only worried about if we changed the redacted format (which I think the test would be easy to amend then anyways)

Kimchelly · 2024-09-25T17:36:45Z

agent/internal/client/client_test.go

+		assert.Contains(t, data, "More fluff")
+		assert.Contains(t, data, "Even more fluff")
+	})
+}


The redacting test is great, love the helper functions and clear test cases. Thanks for adding it! 😎

…al redacting logic

ZackarySantana requested a review from a team September 20, 2024 15:54

ZackarySantana self-assigned this Sep 20, 2024

ZackarySantana commented Sep 20, 2024

View reviewed changes

ZackarySantana changed the title ~~DEVPROD-10509 Update global sender to redact logs as well~~ DEVPROD-10509 Update git.clone project command to redact for modules Sep 20, 2024

ZackarySantana force-pushed the DEVPROD-10509 branch from bf74c84 to 3d1a179 Compare September 23, 2024 14:36

Kimchelly requested changes Sep 25, 2024

View reviewed changes

ZackarySantana requested a review from Kimchelly September 25, 2024 17:01

Kimchelly approved these changes Sep 25, 2024

View reviewed changes

ZackarySantana added 21 commits September 25, 2024 14:22

refactor: unused struct

f2d816f

refactor: simplify return

7c616a5

fix: add redacting sender to the global sender

88da4c8

feat: add redact function to expansions

5deb26f

feat: add enchanced security for autogenerated tokens and remove manu…

ee3f099

…al redacting logic

fix: redact the token when created

bfa9a54

fix: remove more manual redacting logic

d251bc2

fix: add expansions to the task config for test

6e2e479

fix: tests

569402a

fix: escape strings again

1f55b70

fix: test

2f238b3

fix: add back check

d318c27

fix: nil pointer exception

bc62f90

fix: remove check for redacted

09e37c1

docs: add comments/refresh comments for expansions

eee48cc

fix: test if token is empty rather than error

fca0489

feat: add redactor to mock logger producer

51875dd

feat: add test for redacting token

6719853

feat: add logger producer test

edf126d

fix: mega nit

78578cc

fix: cancel context

da23163

ZackarySantana added 2 commits September 25, 2024 14:22

fix: constants for redaction and secret key

d1df660

ci: agent version

8512cdd

ZackarySantana force-pushed the DEVPROD-10509 branch from b8633cd to 8512cdd Compare September 25, 2024 18:22

ZackarySantana merged commit dc993be into evergreen-ci:main Sep 25, 2024
3 of 4 checks passed

ZackarySantana deleted the DEVPROD-10509 branch September 25, 2024 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEVPROD-10509 Update git.clone project command to redact for modules #8330

DEVPROD-10509 Update git.clone project command to redact for modules #8330

ZackarySantana commented Sep 20, 2024 •

edited

Loading

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana Sep 20, 2024

ZackarySantana commented Sep 24, 2024

Kimchelly Sep 25, 2024

ZackarySantana Sep 25, 2024

Kimchelly left a comment

Kimchelly Sep 25, 2024

Kimchelly Sep 25, 2024

ZackarySantana Sep 25, 2024

Kimchelly Sep 25, 2024

Kimchelly Sep 25, 2024

ZackarySantana Sep 25, 2024

Kimchelly Sep 25, 2024


		logger.Execution().Info("Fetching source from git...")
		redactedCmds := fetchScript

DEVPROD-10509 Update git.clone project command to redact for modules #8330

DEVPROD-10509 Update git.clone project command to redact for modules #8330

Conversation

ZackarySantana commented Sep 20, 2024 • edited Loading

Description

Testing

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

ZackarySantana commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimchelly 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

ZackarySantana commented Sep 20, 2024 •

edited

Loading