Improvements to secret injector #13282

jim-minter · 2017-03-07T15:36:25Z

Disallow @ character in host component of URL patterns, so that people don't mistakenly try to add URL patterns of the form user@host.

Extend admission controller to reject invalid URL patterns on secrets to provide early feedback to end users when their patterns are wrong.

bparees · 2017-03-09T04:18:44Z

pkg/build/admission/secretinjector/admission.go

@@ -105,6 +116,30 @@ func (si *secretInjector) Admit(attr admission.Attributes) (err error) {
 	return nil
 }

+func (si *secretInjector) admitSecret(attr admission.Attributes, secret *api.Secret) (err error) {


can't this be part of the static validation logic for secrets, or that's not an option because it's an upstream object so we can't override/extend the validation logic?

I assumed it's not an option for the reason you mention.

bparees · 2017-03-09T04:46:23Z

pkg/util/urlpattern/urlpattern.go

@@ -12,7 +12,7 @@ var InvalidPatternError = errors.New("invalid pattern")

 var urlPatternRegex = regexp.MustCompile(`^` +
 	`(?:(\*|git|http|https|ssh)://)` +
-	`(\*|(?:\*\.)?[^/*]+)` +
+	`(\*|(?:\*\.)?[^@/*]+)` +
 	`(/.*)` +
 	`$`)


i don't think we should be disallowing this for all users of the utility, since technically urls containing @ can be valid, even though you want to disallow it in your particular usage.

We're disallowing @ in the host part of the /pattern/, not in the URL matched. A pattern of http://example.com/* would still match the URL http://user:[email protected]/ (although I'll add a test case for this). I want to disable @ in the pattern because I don't think it makes any sense for us to match user:password@ in the URL.

Test cases added.

you don't think we'd have a case where we want to ensure a user name is being supplied in the url, as for an ssh url?

we might not need it today, i'm not convinced we wouldn't want it in the future, but i guess i'm ok with adding this restriction for now.

bparees · 2017-03-16T00:00:01Z

pkg/build/admission/secretinjector/admission.go

 	}

+	if secret, ok := obj.(*api.Secret); ok {
+		return si.admitSecret(attr, secret)
+	}


i suspect @smarterclayton may have strong opinions about adding this admission control check.

bparees · 2017-03-16T00:06:26Z

lgtm as long as @smarterclayton doesn't veto the use of admission control for this (due to added overhead)

smarterclayton · 2017-03-16T01:51:34Z

Veto. We don't use admission for validation, and doing this here is wrong in lots of ways. Also, people may already have these, so you have to suck it up a bit. If you want to report broken secrets, fail the build.

smarterclayton · 2017-03-16T01:52:20Z

Can talk tomorrow and go over it.

jim-minter · 2017-03-16T13:27:36Z

@smarterclayton the motivation here is to provide rapid feedback when the annotation added to the secret does not parse, in order to make the situation less confusing to the person adding the annotation. Is there an alternative way to meet this goal?

smarterclayton · 2017-03-16T15:16:06Z

We don't block creations on annotation parsing (it's just not done). Annotations are not allowed to break core submission today, and I think it's unreasonable to expect it to. I think this is a "we'll try to improve it later, for now, do it in in the controllers" type of thing.

…

On Thu, Mar 16, 2017 at 9:27 AM, Jim Minter ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> the motivation here is to provide rapid feedback when the annotation added to the secret does not parse, in order to make the situation less confusing to the person adding the annotation. Is there an alternative way to meet this goal? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13282 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_M0ahJ7b_zC6XmXbwydz1rSEVhnks5rmTjKgaJpZM4MVovV> .

jim-minter · 2017-03-16T17:40:12Z

OK, I think that means that all we can do is log the fact that the annotation didn't parse to the master log when at some point we touch it, which is what we do today. Is that correct?

smarterclayton · 2017-03-17T00:29:09Z

Where do we touch?

jim-minter · 2017-03-17T09:16:46Z

The documentation suggests using oc annotate secret, but it could also be via oc edit secret or via the web yaml editor, for example.

jim-minter · 2017-04-20T10:38:06Z

@smarterclayton is there anything useful that can come out of this PR, or should I just abandon it?

bparees · 2017-04-26T14:10:22Z

@smarterclayton bump

… try to add URL patterns of the form username@hostname/path.

jim-minter · 2017-09-11T18:31:19Z

@bparees removed the admission controller pieces from this and rebased - ptal

bparees · 2017-09-11T19:51:07Z

@jim-minter so what's the net effect of this change? who are the consumers that are impacted? any effect on existing objects in clusters?

bparees · 2017-09-15T15:01:17Z

@jim-minter bump

jim-minter · 2017-09-15T22:29:55Z

/retest

jim-minter · 2017-09-15T22:37:03Z

@bparees tbh, minimal effect. Whereas previously an invalid url pattern of the form *://user@host/* would have parsed but failed to match anything, now it will fail to parse causing a glog.V(2).Infof to be emitted to the master log. Wider behaviour remains the same - BC object passes through and is created. No effect on existing objects.

bparees · 2017-09-16T00:05:28Z

thanks @jim-minter
/lgtm

openshift-merge-robot · 2017-09-16T00:05:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, jim-minter

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/util/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-09-16T14:33:14Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-09-16T14:38:38Z

Automatic merge from submit-queue (batch tested with PRs 16269, 13282, 16386)

openshift-ci-robot · 2017-09-16T15:18:18Z

@jim-minter: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/end_to_end	`ae22665`	link	`/test end_to_end`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jim-minter assigned bparees Mar 7, 2017

jim-minter mentioned this pull request Mar 7, 2017

Properly support git uris of the form user@host:path/to/file.git in build secret injector #13223

Closed

bparees reviewed Mar 9, 2017

View reviewed changes

jim-minter force-pushed the secret-injector-improvements branch from 85a5ea3 to f5df357 Compare March 9, 2017 10:32

bparees reviewed Mar 16, 2017

View reviewed changes

jim-minter force-pushed the secret-injector-improvements branch from f5df357 to 77d631c Compare April 27, 2017 18:26

jim-minter force-pushed the secret-injector-improvements branch from 77d631c to 280217a Compare May 8, 2017 09:54

Disallow @ character in URL patterns, so that people don't mistakenly…

ae22665

… try to add URL patterns of the form username@hostname/path.

jim-minter force-pushed the secret-injector-improvements branch from 280217a to ae22665 Compare September 11, 2017 18:30

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2017

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2017

openshift-merge-robot merged commit a4e5e53 into openshift:master Sep 16, 2017

Improvements to secret injector #13282

Improvements to secret injector #13282

Uh oh!

Conversation

jim-minter commented Mar 7, 2017

Uh oh!

bparees Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

jim-minter Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

bparees Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

jim-minter Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

jim-minter Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

bparees Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

bparees Mar 16, 2017

Choose a reason for hiding this comment

Uh oh!

bparees commented Mar 16, 2017

Uh oh!

smarterclayton commented Mar 16, 2017

Uh oh!

smarterclayton commented Mar 16, 2017

Uh oh!

jim-minter commented Mar 16, 2017

Uh oh!

smarterclayton commented Mar 16, 2017 via email

Uh oh!

jim-minter commented Mar 16, 2017

Uh oh!

smarterclayton commented Mar 17, 2017 via email

Uh oh!

jim-minter commented Mar 17, 2017

Uh oh!

jim-minter commented Apr 20, 2017

Uh oh!

bparees commented Apr 26, 2017

Uh oh!

jim-minter commented Sep 11, 2017

Uh oh!

bparees commented Sep 11, 2017

Uh oh!

bparees commented Sep 15, 2017

Uh oh!

jim-minter commented Sep 15, 2017

Uh oh!

jim-minter commented Sep 15, 2017

Uh oh!

bparees commented Sep 16, 2017

Uh oh!

openshift-merge-robot commented Sep 16, 2017

Uh oh!

openshift-merge-robot commented Sep 16, 2017

Uh oh!

openshift-merge-robot commented Sep 16, 2017

Uh oh!

openshift-ci-robot commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 16, 2017 •

edited

Loading