Fix BuildConfig Environment Variable Modification #19532

adambkaplan · 2018-04-26T20:52:13Z

Ensure build config environment vars are not modified when they are safely logged.
Only redact a URL if it contains a password

adambkaplan · 2018-04-26T21:05:58Z

bparees · 2018-04-26T21:14:09Z

pkg/build/util/util_test.go

@@ -163,8 +164,7 @@ func TestSafeForLoggingS2IConfig(t *testing.T) {
 	if redactedRegex.MatchString(config.ScriptDownloadProxyConfig.HTTPSProxy.String()) {
 		t.Errorf("credentials stripped from original scripts proxy: %v", config.ScriptDownloadProxyConfig.HTTPSProxy)
 	}
-	//checkEnvList(t, config.Environment, true)
-
+	checkEnvList(t, config.Environment, true)


we should also make sure none of the other proxy fields changed in the original, e.g. ScriptDownloadProxyConfig.HTTPProxy, etc.

bparees · 2018-04-26T21:17:11Z

pkg/build/util/util_test.go

+	}
+	if redactedRegex.MatchString(*proxyBuild.Spec.Source.Git.HTTPSProxy) {
+		t.Errorf("improperly redacted credentials in https proxy value: %s", *stripped.Spec.Source.Git.HTTPSProxy)
+	}


ah i see you're covering that here

bparees · 2018-04-27T02:57:47Z

pkg/build/util/util.go

-	newUrl := *u
-	newUrl.User = url.User("redacted")
-	return &newUrl
+	newURL := *u


Even though you added a deepcopy higher in the stack, there's no guarantee every caller to SafeForLoggingURL is coming in via that path (in fact I don't think they all are, see: SafeForLoggingS2IConfig for example)

So I think it's still important to deepcopy the url arg here, we shouldn't be mutating the input to the function.

adambkaplan · 2018-04-27T13:11:56Z

pkg/build/util/util.go

-	newUrl := *u
-	newUrl.User = url.User("redacted")
-	return &newUrl
+	newURL, err := url.Parse(u.String())


url doesn't support DeepCopy(), so I re-parsed the string instead.

that seems fine

bparees · 2018-04-27T15:00:16Z

please add an explicit unit test for SafeForLoggingURL to prove it can update urls and isn't mutating the input arg. (I know the higher level tests basically cover this, but i'd feel better having a direct unit test for it)

adambkaplan · 2018-04-27T15:05:36Z

pkg/build/util/util_test.go

+func TestSafeForLoggingURL(t *testing.T) {
+	cases := []string{
+		"https://user:[email protected]",
+		"https://www.redhat.com",


Explicit unit test for SafeForLoggingURL - other cases to add?

user w/ no password?

(sorry if these were here before and i missed them)

adambkaplan · 2018-04-27T18:18:25Z

/retest

adambkaplan · 2018-04-27T20:27:12Z

@bparees done & tests passing

bparees · 2018-04-27T20:39:29Z

pkg/build/util/util_test.go

@@ -188,7 +189,7 @@ func checkEnvList(t *testing.T, envs s2iapi.EnvironmentList, orig bool) {
 				if credsRegex.MatchString(env.Value) {
 					t.Errorf("credentials not stripped from env value %v", env)
 				}
-				if !redactedRegex.MatchString(env.Value) {
+				if isURL(env.Value) && urlStringHasPassword(env.Value) && !redactedRegex.MatchString(env.Value) {


isn't urlStringHasPassword always going to be false if the redaction worked, so the redactedRegex check will never happen?

good catch. The redacted regex won't run in this case, but we are assured that the url string doesn't contain a password (may have a username)

just removing the redactedRegex check is probably fine. I dno't want to leave it there to potentially confuse someone in the future though.

or you can go for bonus points and fix the logic to actually invoke that check.

bparees · 2018-04-27T20:59:20Z

one last question

bparees · 2018-04-27T20:59:38Z

and go ahead and squash your commits

adambkaplan

@bparees squashed & rebased. Please see my comment re: source-to-image.

adambkaplan · 2018-04-30T15:16:13Z

pkg/build/util/util_test.go

+			if reflect.DeepEqual(url, outURL) {
+				t.Errorf("url %v was not properly redacted", outURL)
+			}
+			if !redactedRegex.MatchString(outURL.String()) {


Moved explicit check for "redacted" string here.

adambkaplan · 2018-04-30T15:22:45Z

pkg/build/util/util.go

@@ -238,17 +239,23 @@ func SafeForLoggingURL(u *url.URL) *url.URL {
 	if u == nil {
 		return nil
 	}
-	newUrl := *u
-	newUrl.User = url.User("redacted")


source-to-image has a similar SafeForLoggingURL method, which parses an input string and always adds the "redacted" user (even if there is no user info on the URL). Worth adding this logic to s2i as well?

the logic that only manipulates the value if a password is set? I suppose, not critical, vendoring the change definitely doesn't need to be part of this PR.

bparees · 2018-04-30T16:42:52Z

/lgtm

bparees · 2018-04-30T16:42:59Z

/approve

openshift-ci-robot · 2018-04-30T16:43:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, bparees

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/build/OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adambkaplan · 2018-05-01T17:57:44Z

/retest

adambkaplan · 2018-05-01T19:16:46Z

Encountered flakes:
#19058
#17731

adambkaplan · 2018-05-01T19:17:20Z

/retest

adambkaplan · 2018-05-02T13:22:37Z

/retest

New test flakes:
#19588
#19589

adambkaplan · 2018-05-02T17:48:45Z

/retest because of known HAProxy flake

Bug 1571349

adambkaplan · 2018-05-03T02:36:55Z

/retest

adambkaplan · 2018-05-03T10:45:10Z

@bparees needs lgtm - I rebased to bring in @gabemontero fix with test runs.

bparees · 2018-05-03T11:29:13Z

@adambkaplan fyi you shouldn't need to rebase for that, your PR is always merged/rebased on top of master before being tested.

/lgtm

adambkaplan · 2018-05-03T15:59:33Z

/retest

openshift-bot · 2018-05-03T19:00:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

adambkaplan · 2018-05-03T21:00:16Z

/retest
New flake downloading ruby gems during build process

openshift-ci-robot requested review from csrwng and gabemontero April 26, 2018 20:52

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 26, 2018

openshift-ci-robot assigned bparees Apr 26, 2018

bparees requested changes Apr 26, 2018

View reviewed changes

bparees reviewed Apr 26, 2018

View reviewed changes

bparees reviewed Apr 27, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 27, 2018

adambkaplan commented Apr 27, 2018

View reviewed changes

adambkaplan force-pushed the bugfix/logging-modified-url branch 2 times, most recently from 9cfa978 to 6f37562 Compare April 27, 2018 14:47

adambkaplan commented Apr 27, 2018

View reviewed changes

bparees reviewed Apr 27, 2018

View reviewed changes

adambkaplan force-pushed the bugfix/logging-modified-url branch from 07b514f to a8f4226 Compare April 30, 2018 15:15

adambkaplan commented Apr 30, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2018

Ensure environment variables and URLs are not modified for safe logging.

51b3047

Bug 1571349

adambkaplan force-pushed the bugfix/logging-modified-url branch from a8f4226 to 51b3047 Compare May 3, 2018 00:50

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 3, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2018

openshift-merge-robot merged commit 601e070 into openshift:master May 4, 2018

adambkaplan deleted the bugfix/logging-modified-url branch May 4, 2018 12:30

Fix BuildConfig Environment Variable Modification #19532

Fix BuildConfig Environment Variable Modification #19532

Conversation

adambkaplan commented Apr 26, 2018 • edited Loading

adambkaplan commented Apr 26, 2018

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

bparees commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambkaplan commented Apr 27, 2018

adambkaplan commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Apr 27, 2018

bparees commented Apr 27, 2018

adambkaplan 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

bparees commented Apr 30, 2018

bparees commented Apr 30, 2018

openshift-ci-robot commented Apr 30, 2018

adambkaplan commented May 1, 2018

adambkaplan commented May 1, 2018

adambkaplan commented May 1, 2018

adambkaplan commented May 2, 2018

adambkaplan commented May 2, 2018

adambkaplan commented May 3, 2018

adambkaplan commented May 3, 2018

bparees commented May 3, 2018

adambkaplan commented May 3, 2018

openshift-bot commented May 3, 2018

adambkaplan commented May 3, 2018

adambkaplan commented Apr 26, 2018 •

edited

Loading