Additional registry whitelisting #17783

miminar · 2017-12-14T14:02:47Z

Adds validation to imagestreams and imagestreamtags that forbids new image pull specs not present on the whitelist.

Resolves bz#1505315

miminar · 2017-12-14T14:13:21Z

/test

@openshift/sig-developer-experience

I'm working on integration test now. Documentation is missing but otherwise ready for review.

dmage · 2017-12-14T16:20:01Z

issue for the image registry tests: #17786

bparees

will try to give this a more thorough review tomorrow.

bparees · 2017-12-14T15:54:36Z

pkg/image/admission/registry_whitelist.go

+
+// RegistryWhitelister decides whether given image pull specs are allowed by system's image policy.
+type RegistryWhitelister interface {
+	// AdmitHostname returns error if the given host is allowed by the whitelist.


is not allowed?

bparees · 2017-12-14T15:55:21Z

pkg/image/admission/registry_whitelist.go

+	// TODO: accept insecure flag
+	WhitelistPullSpecs(pullSpecs ...string)
+	// Copy returns a deep copy of the whitelister. This is useful for temporarily whitelisting additional
+	// registires/pullSpecs before a specific validation.


typo on registries.

smarterclayton · 2017-12-15T21:17:25Z

Why are we doing this in admission again?

miminar · 2017-12-18T07:44:03Z

Why are we doing this in admission again?

There's no admission controller. The functionality is just located in the admission package since it's closely related. All the admission happens in ImageStream(Tag)'s strategy. I'll rename the package to avoid confusion.

miminar · 2017-12-18T16:37:34Z

Rebased, moved logic from image/admission to image/apis/image/validation/whitelist. Added more unit tests. Will continue with integration test.

PTAL

miminar · 2017-12-19T10:49:27Z

Added integration test.

miminar · 2017-12-19T11:05:35Z

Flake #17769

bparees · 2018-01-03T01:55:05Z

pkg/image/apis/image/validation/validation.go

 	result := validation.ValidateObjectMeta(&stream.ObjectMeta, true, ValidateImageStreamName, field.NewPath("metadata"))

 	// Ensure we can generate a valid docker image repository from namespace/name
 	if len(stream.Namespace+"/"+stream.Name) > reference.NameTotalLengthMax {
 		result = append(result, field.Invalid(field.NewPath("metadata", "name"), stream.Name, fmt.Sprintf("'namespace/name' cannot be longer than %d characters", reference.NameTotalLengthMax)))
 	}

+	insecureRepository := isRepositoryInsecure(stream)
+
 	if len(stream.Spec.DockerImageRepository) != 0 {


we don't whitelist check stream.Spec.DockerImageRepository itself? Does something else prevent a user from importing a disallowed image via that mechanism?

There's a prevention mechanism - the individual tags get refused. But I agree it would be better to admit this spec as well to avoid needless frequent import errors. I'll fix this.

bparees

various questions on some of the port matching/denying behavior, but the fundamentals lgtm.

bparees · 2018-01-03T16:11:57Z

pkg/image/apis/image/validation/whitelist/whitelister.go

+	stringsutil "github.com/openshift/origin/pkg/util/strings"
+)
+
+// WhitelistTransport says whether the associated registry host shell be treated as secure or insecure.


s/shell/shall/

bparees · 2018-01-03T16:18:54Z

pkg/image/apis/image/validation/whitelist/whitelister.go

+	//  <host>, <host>:<port>
+	// where each component can contain wildcards like '*' or '??' to mach wide range of registries. If the
+	// port is omitted, the default will be appended based on the given transport. If the transport is "any",
+	// the given blob will match hosts with both :80 and :443 ports.


s/blob/glob/

bparees · 2018-01-03T16:21:03Z

pkg/image/apis/image/validation/whitelist/whitelister.go

+		pullSpecs:             sets.NewString(),
+		registryHostRetriever: registryHostRetriever,
+	}
+	// iterate in reversed order to make the patterns appear in the same order as given (patters are prepended)


s/patters/patterns/

bparees · 2018-01-03T19:04:54Z

pkg/image/apis/image/validation/whitelist/whitelister_test.go

+				"localhost:443":  nil,
+				"localhost:5000": fmt.Errorf(`registry "localhost:5000" not allowed by whitelist { "*:443" }`),
+				"localhost:80":   fmt.Errorf(`registry "localhost:80" not allowed by whitelist { "*:443" }`),
+				"localhost":      nil,


why are "localhost", "docker.io", and "example.com" allowed here, when they are not on port 443?

That's because * wildcard in the whitelist defaults to a secure port :443. All the given hostnames also default to :443 because of the WhitelistTransportSecure. Therefor, their ports will match.

bparees · 2018-01-03T19:07:49Z

pkg/image/apis/image/validation/whitelist/whitelister_test.go

+			hostnames: map[string]error{
+				"docker.io":      nil,
+				"example.com":    nil,
+				"localhost:443":  fmt.Errorf(`registry "localhost:443" not allowed by whitelist { "*:80" }`),


why isn't 443 not allowed for "any transport"? especially when :80 is allowed?

mkAllowed creates AllowedRegistries with Insecure flag set. Therefor the whitelist looks like the error suggests: { "*:80" }. Thus any given hostname must either have :80 port specified or be passed with one of {any, insecure} transport.

ok i think i understand this now as well.

bparees · 2018-01-03T19:10:25Z

pkg/image/apis/image/validation/whitelist/whitelister_test.go

+				"a.b.c.d.foo.com:80": nil,
+				"domain.ltd":         nil,
+				"example.com":        nil,
+				"foo.com":            fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),


on what basis is foo.com denied? the whitelist has *.foo.com and allow insecure.

The whitelisted registry has . as a prefix. Therefore it matches any sub-domain of foo.com but not the domain itself. I'd like to avoid additional logic on special treating *.. If the admin wants to match the domain as well, he can add the second entry without the leading *..

i'm still unclear on this one. I would expect *.foo.com to match foo.com.

discussed on irc

miminar · 2018-01-04T14:01:13Z

pkg/image/apis/image/validation/whitelist/whitelister.go

+		hp.host = parts[0]
+		switch transport {
+		case WhitelistTransportAny:
+			hp.port = "*"


I've just realized this is way too benevolent. any should mean either secure (:443) or insecure (:80) not "any transport protocol". I'll fix this.

miminar · 2018-01-09T17:09:13Z

Addressed comments.

bparees · 2018-01-09T20:19:33Z

@miminar lgtm, squash and i'll merge.

openshift-bot · 2018-01-11T22:34:44Z

/retest

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

bparees · 2018-01-11T22:51:03Z

/hold
(for @smarterclayton's comments).

Will this break integrations people have built when you turn this on?

Possibly, if someone has an integration that relied on working around the configured imagepolicy rules that weren't being enforced properly. Otherwise it's not obvious to me what this would break, do you have something in mind?

What backwards compatibility implications are there?

The backwards compatibility logic is that on update, @miminar has dynamically whitelisted existing values, so this logic only prevents the creation of new imagestreams w/ disallowed values, or the updating of an existing one that adds a new disallowed value.

It needs to handle registry qualification.

Can you elaborate on this? This is image import, so someone specifies an image they want to import. Whether they fully qualify it or not doesn't matter to us, we're just going to see if it matches the allowed registries/imagespecs (fully qualified or otherwise) the admin specified.

Assuming you mean that we should be automatically qualifying unqualified specifications, would you expect us to do that on the whitelist side, or the value being checked side? both? What would we default to? How does the existing image-import logic handle defaulting an unqualified import request? It looks to me like it just assumes docker.io, because this fails:

$ oc import-image rhscl/ruby-24-rhel7 --confirm
The import completed with errors.

while this succeeds:

$ oc import-image registry.access.redhat.com/rhscl/ruby-24-rhel7 --confirm
The import completed successfully.

and this succeeds (via docker.io):

$ oc import-image centos/ruby-24-centos7 --confirm
The import completed successfully.

so it is not respecting the docker daemon search list, because this succeeds on my host:

$ docker pull rhscl/ruby-24-rhel7
Using default tag: latest
Trying to pull repository docker.io/rhscl/ruby-24-rhel7 ... 
Trying to pull repository registry.fedoraproject.org/rhscl/ruby-24-rhel7 ... 
Trying to pull repository registry.access.redhat.com/rhscl/ruby-24-rhel7 ... 
sha256:a2b8fe97c63db4cd4ea5f46cf9ebb8ec92ce859cdef2dcaaf21e2b282bbd5450: Pulling from registry.access.redhat.com/rhscl/ruby-24-rhel7

miminar · 2018-01-12T12:27:36Z

Addressed the comments except for those answered by Ben since I agree with him.

miminar · 2018-01-12T14:53:58Z

/retest

Signed-off-by: Michal Minář <[email protected]>

bparees · 2018-01-15T18:20:54Z

@miminar I talked with @smarterclayton, this is good to go but we'll need a follow up that updates the whitelisting logic in the following way:

if the image the user wants to import/reference does not have a registry qualification on it, we need to prepend "docker.io" as the registry before checking it against the whitelist, because ultimately that is the registry we import from for unqualified image values.

(or if you just want to make that change in this PR, that's fine too)

miminar · 2018-01-16T07:33:42Z

I don't understand why a whitelisting/validation logic should do any changes to the input data at all. If desired, I can do a follow up.

bparees · 2018-01-16T13:27:48Z

I don't understand why a whitelisting/validation logic should do any changes to the input data at all.

it's something the image import policy should have always been doing, because ultimately the image import logic is qualifying image values with "docker.io" if they are unqualified, so the whitelisting should be checking them as if they had that prefix.

miminar · 2018-01-16T17:57:41Z

so the whitelisting should be checking them as if they had that prefix.

That's already the case for docker.io .

If I understand #18020 correctly, the qualification will be done for us in a level below (closer to etcd). If this PR merges as is before #18020, the whitelisting will be broken once #18020 merges (without touching the whitelister) because whitelister will treat unqualified images differently.

So either

this needs to wait for Admission plugin: openshift.io/ImageQualify #18020 and process the same rule set to do the qualification, or
Admission plugin: openshift.io/ImageQualify #18020 needs to be updated for the whitelisting when this merges

I'm fine with both variants. @bparees @frobware what do you think?

bparees · 2018-01-16T18:04:21Z

@miminar my impression is they are unrelated.

#18020 is due to a change in kubernetes behavior w/ respect to what image will get run when a pod which references an unqalified image name is created (prior to the latest kube release, the image would get qualified by the docker daemon. Now, unqualified images are always qualified as being from docker.io. Thus #18020 introduces a higher level qualifier that can qualify images differently before kube sees them and prepends docker.io)

None of that has any bearing on how image importing is done. Image import, near as I can tell, always treated unqualified image references as coming from docker.io. And the image import whitelisting behavior should do the same (if someone tries to import an unqualified image name, we should prepend docker.io and then check it against the whitelist). It sounds like you're saying that already happens, so we're fine.

Can you elaborate on how you think #18020 will break the image import whitelister?

miminar · 2018-01-18T16:13:23Z

It won't actually break the whitelister. But there will be a mismatch between the whitelister always defaulting to docker.io and whitelisting such am image and a pod showing image coming from not whitelisted registry which was prepended by the admission plugin.

But this "mismatching" could be fixed in a follow-up as well.

bparees · 2018-01-18T16:17:03Z

@miminar i'm still unclear on one thing.

if a user attempts to define an imagestream with a reference to "openshift/jenkins-2-centos7" and the whitelister only includes "docker.io" as an allowed registry, will this imagestream be allowed or not?

in my opinion it should be, since we are going to import from docker.io in this case.

miminar · 2018-01-18T17:09:50Z

... will this imagestream be allowed or not?

@bparees in the current state of things, yes.

However, with a follow-up, where the qualification rules get applied before/during whitelisting, it may no longer be the case.

bparees · 2018-01-18T17:11:57Z

/hold cancel
/lgtm
/retest

openshift-ci-robot · 2018-01-18T17:12:03Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

~~hack/OWNERS~~ [bparees]
~~pkg/image/OWNERS~~ [bparees]
~~test/common/OWNERS~~ [bparees]
~~test/integration/OWNERS~~ [bparees]
~~test/testdata/OWNERS~~ [bparees]
~~test/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-bot · 2018-01-19T05:27:02Z

/retest

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

miminar · 2018-01-19T07:37:28Z

Flake #17694

openshift-bot · 2018-01-19T21:34:09Z

/retest

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

openshift-merge-robot · 2018-01-20T01:12:54Z

Automatic merge from submit-queue.

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017

openshift-ci-robot requested review from liggitt and smarterclayton December 14, 2017 14:02

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 14, 2017

miminar force-pushed the enforce-registry-whitelist branch from 7c8b944 to 842eba4 Compare December 14, 2017 14:06

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

miminar changed the title ~~Registry whitelisting functional globally~~ [WIP] Additional registry whitelisting Dec 14, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2017

bparees reviewed Dec 14, 2017

View reviewed changes

miminar force-pushed the enforce-registry-whitelist branch from bf1d062 to e8a26ae Compare December 15, 2017 15:05

bparees self-assigned this Dec 15, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2017

miminar force-pushed the enforce-registry-whitelist branch from e00604b to 46fd247 Compare December 18, 2017 16:35

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

miminar force-pushed the enforce-registry-whitelist branch from 46fd247 to befd2b5 Compare December 19, 2017 10:48

miminar changed the title ~~[WIP] Additional registry whitelisting~~ Additional registry whitelisting Dec 19, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2017

bparees reviewed Jan 3, 2018

View reviewed changes

bparees requested changes Jan 3, 2018

View reviewed changes

miminar commented Jan 4, 2018

View reviewed changes

miminar force-pushed the enforce-registry-whitelist branch from 1a81d12 to 945f73d Compare January 9, 2018 19:22

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2018

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2018

miminar force-pushed the enforce-registry-whitelist branch from 63e2525 to 03f9742 Compare January 12, 2018 11:52

miminar force-pushed the enforce-registry-whitelist branch from 03f9742 to a78e5eb Compare January 12, 2018 12:33

Addressed comments

dd4851b

Signed-off-by: Michal Minář <[email protected]>

miminar force-pushed the enforce-registry-whitelist branch from a78e5eb to dd4851b Compare January 12, 2018 20:11

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 18, 2018

openshift-merge-robot merged commit 8b8c798 into openshift:master Jan 20, 2018

This was referenced Feb 14, 2018

admin-guide:image-policy: allowed registries for import openshift/openshift-docs#7788

Merged

configure imagePolicyConfig:allowedRegistriesForImport openshift/openshift-ansible#7197

Merged

miminar deleted the enforce-registry-whitelist branch February 22, 2018 15:48

Additional registry whitelisting #17783

Additional registry whitelisting #17783

Conversation

miminar commented Dec 14, 2017 • edited Loading

miminar commented Dec 14, 2017

dmage commented Dec 14, 2017

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Dec 15, 2017

miminar commented Dec 18, 2017

miminar commented Dec 18, 2017

miminar commented Dec 19, 2017

miminar commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees 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

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

miminar commented Jan 9, 2018

bparees commented Jan 9, 2018

openshift-bot commented Jan 11, 2018

bparees commented Jan 11, 2018

miminar commented Jan 12, 2018

miminar commented Jan 12, 2018

bparees commented Jan 15, 2018

miminar commented Jan 16, 2018

bparees commented Jan 16, 2018

miminar commented Jan 16, 2018

bparees commented Jan 16, 2018

miminar commented Jan 18, 2018

bparees commented Jan 18, 2018

miminar commented Jan 18, 2018

bparees commented Jan 18, 2018

openshift-ci-robot commented Jan 18, 2018

openshift-bot commented Jan 19, 2018

miminar commented Jan 19, 2018

openshift-bot commented Jan 19, 2018

openshift-merge-robot commented Jan 20, 2018

miminar commented Dec 14, 2017 •

edited

Loading