Allow to restrict imports to white listed registries #13313

mfojtik · 2017-03-08T20:56:53Z

This change will allow to restrict image importing to registries that are white listed in the master-config.yaml. By default (when not specified) the legacy behavior is retained and any registry is allowed for the image import.
However, for a new clusters when the master-config.yaml is created, we picked the known public registries and whitelisted them explicitely.

Q/A:

What impact will this have to existing users?
Unless you recreate the master-config.yaml, everything should work as it worked before.

What impact will this have to new users?
When the master-config.yaml is written to disk, the list of registries we allow to import from will be created. In case you want to import Docker image from third-party registry, that registry needs to be added to the list (a server restart is required after that change).

The verification is done in image import validation, so:

 # oc import-image foo.bar:5000/test/test --confirm
The ImageStreamImport "test" is invalid: spec.images[0].from.name: Invalid value: "foo.bar:5000/test/test": importing images from registry "foo.bar" is forbidden, only images from "docker.io,registry.access.redhat.com,gcr.io,quay.io,*.amazonaws.com" are allowed

In case of creating the ImageStream directly with tags, the importer will report errors if the untrusted
registry is used in the ImageStream status:

# oc describe is/ruby
Name:			ruby
Namespace:		test
Created:		8 seconds ago
Labels:			<none>
Annotations:		openshift.io/display-name=Ruby
Docker Pull Spec:	172.30.53.250:5000/test/ruby
Unique Images:		0
Tags:			5
...
...
foo.bar:5000/openshift/ruby-20-centos7:latest
  pushed image

  ! error: Import failed (Invalid value: "foo.bar:5000/openshift/ruby-20-centos7:latest": importing images from registry "foo.bar" is forbidden, only images from "docker.io,registry.access.redhat.com,gcr.io,quay.io,*.amazonaws.com" are allowed):
      7 seconds ago

In case you already have images imported from third-party registries that are not whitelisted
the images will remain but the scheduled import won't work for them anymore (you will see the error above).

mfojtik · 2017-03-08T20:56:58Z

[test]

mfojtik · 2017-03-08T20:58:32Z

pkg/cmd/server/api/v1/conversions.go

@@ -99,6 +99,16 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error {
 				// The final value of OAuthConfig.MasterCA should never be nil
 				obj.OAuthConfig.MasterCA = &s
 			}
+
+			if len(obj.ImagePolicyConfig.AllowedRegistriesForImport) == 0 {
+				obj.ImagePolicyConfig.AllowedRegistriesForImport = []string{


@smarterclayton need to double check on the registry hostnames here...

We can't default this on - that breaks existing clusters.

You need to do this when we create a config, not in API defaulting.

fixed. also I moved the default whitelist to types.go (instead of burrying it in master_args)

smarterclayton · 2017-03-08T21:08:00Z

pkg/cmd/server/api/types.go

+	// images from. By default docker.io, Red Hat registry, gcr.io, Amazon ECS, quay.io are
+	// allowed. If you want to import images from your private Docker Registry, you have to
+	// add it into the list.
+	AllowedRegistriesForImport []string


Make this a sub struct with a single field.

Specify domain name.

@smarterclayton i this we also need wildcard (*) for registries like ECS. Does it make sense?

Yes, supporting wildcard is fine. Be sure to document the wildcard syntax. A bit concerned that we shouldn't support crazy wildcards (filepath matching is crazy).

@smarterclayton i don't want to re-invent wildcard matching, so if you have better suggestion from the standard library I'm all for it :-) The filepath matching seems reasonable to me... I guess most users/admins will just use the '*' and '??' wildcards anyway.

smarterclayton · 2017-03-08T21:09:19Z

pkg/cmd/server/api/v1/types.go

+	// AllowedRegistriesForImport is a white list of registries that we allow to import the
+	// images from. By default docker.io, Red Hat registry, gcr.io, Amazon ECS, quay.io are
+	// allowed. If you want to import images from your private Docker Registry, you have to
+	// add it into the list.


This needs to only impose this behavior when this is set. If empty, old behavior applies.

smarterclayton · 2017-03-08T21:10:04Z

pkg/image/api/helper.go

+// RegistryHostname returns the registry hostname without port
+func (r DockerImageReference) RegistryHostname() string {
+	registryHost := r.AsV2().Registry
+	if strings.Contains(registryHost, ":") {


Ports matter - gcr.io on port 443 is not the same as gcr.io on port 935

i assume all registries we whitelist by default run on 443?

so I'd say if port is not specified, it can be 443 or 80 (secure or insecure). If it is specified, it must match. I.e. docker.io matches docker.io:443, but docker.io does not match docker.io:8443

@smarterclayton maybe we can add that to the config struct (insecure: true)

smarterclayton · 2017-03-08T21:21:15Z

pkg/image/importer/importer.go

@@ -129,6 +132,11 @@ func (i *ImageStreamImporter) importImages(ctx gocontext.Context, retriever Repo
 		repoName := defaultRef.RepositoryName()
 		registryURL := defaultRef.RegistryURL()

+		if !importer.isAllowedRegistry(defaultRef.RegistryHostname()) {


If empty, you have to bypass this. Can't break old clusters.

right, I will make it optional.

smarterclayton · 2017-03-08T21:22:05Z

pkg/image/importer/importer.go

@@ -129,6 +132,11 @@ func (i *ImageStreamImporter) importImages(ctx gocontext.Context, retriever Repo
 		repoName := defaultRef.RepositoryName()
 		registryURL := defaultRef.RegistryURL()

+		if !importer.isAllowedRegistry(defaultRef.RegistryHostname()) {
+			isi.Status.Images[i].Status = invalidStatus("", field.Invalid(field.NewPath("from", "name"), from.Name, fmt.Sprintf("registry %q not allowed in master configuration", defaultRef.RegistryHostname())))


This message needs to be really good - this is going to break people. You're going to have to make sure the CLI and UI do a good job of presenting this error.

Images may not be imported from %q - only images from %s are allowed

Would be good to demonstrate how this would be shown in oc import-image, oc describe is, web console, etc

with the current implementation, it will get surfaced in the events inside image stream... when I rework this in validation, I think it you will get the error when you run oc import-image, so the IS won't be created at all.

Web console will get validation error as well, so I think that will be presented to users in nice form.

smarterclayton · 2017-03-08T21:22:33Z

pkg/image/importer/importer.go

+// the master configuration.
+func (i *ImageStreamImporter) isAllowedRegistry(registry string) bool {
+	for _, allowedRegistry := range i.allowedRegistries {
+		if matched, err := filepath.Match(allowedRegistry, registry); matched && err == nil {


I don't like filepath because it is OS specific.

yeah, I will probably craft something custom and make it part of util/strings.go

smarterclayton · 2017-03-08T21:23:05Z

pkg/image/importer/importer.go

@@ -129,6 +132,11 @@ func (i *ImageStreamImporter) importImages(ctx gocontext.Context, retriever Repo
 		repoName := defaultRef.RepositoryName()
 		registryURL := defaultRef.RegistryURL()

+		if !importer.isAllowedRegistry(defaultRef.RegistryHostname()) {


Why is this here? Put this in ImageStreamImport validation, pass the allowed registries to the strategy, not the importer.

you're right... I thought we want to do this in importer, but this really is a validation... i will pass the list of allowed registries to import validator and verify it there.

mfojtik · 2017-03-09T12:48:18Z

@smarterclayton all updated, PTAL pls.

soltysh · 2017-03-09T13:33:24Z

pkg/cmd/server/api/types.go

+	DomainName string
+	// Insecure indicates whether the registry is secury (https) or insecure (http)
+	// By default (if not specified) the registry is assumed as secure.
+	Insecure bool


What about different port, how one will specify that?

If it's part of domain name (I assume from validation code) can you state that in the doc text for DomainName, it isn't obvious.

yes, good idea

soltysh · 2017-03-09T13:46:58Z

pkg/image/api/helper.go

@@ -168,6 +169,17 @@ func (r DockerImageReference) RepositoryName() string {
 	return r.Exact()
 }

+// RegistryHostname returns the registry hostname without port
+func (r DockerImageReference) RegistryHostPort() (string, string) {


Shouldn't this method accept bool Insecure? And return 80 if it's set to true?

the godoc is wrong, but yeah

actually I already have that fixed :)

soltysh · 2017-03-09T13:48:08Z

pkg/image/api/validation/validation.go

+		case 2:
+			allowedRegistryHost, allowedRegistryPort = parts[0], parts[1]
+		default:
+			// TODO: Should we panic/glog here? The format of allowed registries should be


glog for sure

glog not needed, so I will remove it from here (see Clayton comment)

soltysh · 2017-03-09T13:52:48Z

pkg/cmd/server/api/types.go

+// name. The domain name might include wildcards, like '*' or '??'.
+type RegistryLocation struct {
+	// DomainName specifies a domain name for the registry
+	DomainName string


Additionally, since we allow wildcard matches here, that should be mentioned in the doc as well.

yeah, I mention it in the comment for the registry location to make it more visible, but I can move it down.

miminar

~~Now what about oc tag? 😄~~ I realized it's not an issue because oc tag modifies the spec of ImageStream and leaves the actually image creation on importer controller.

Nevertheless I'd like to see at least an integration test proving that the oc tag case is handled.

miminar · 2017-03-09T13:44:49Z

pkg/image/api/validation/validation.go

+		default:
+			// TODO: Should we panic/glog here? The format of allowed registries should be
+			// validated during the server startup.
+			return errs


At least log an error an continue.

miminar · 2017-03-09T13:50:59Z

pkg/image/api/helper.go

@@ -168,6 +169,17 @@ func (r DockerImageReference) RepositoryName() string {
 	return r.Exact()
 }

+// RegistryHostname returns the registry hostname without port


miminar · 2017-03-09T13:52:17Z

pkg/image/api/helper.go

+		return hostname, port
+	}
+	// The default registry port is https/443
+	return registryHost, "443"


The 443 may be wrong. Empty port should be returned if not specified.

@miminar empty port mean 80 right? that might be incorrect.

we default to https in RegistryURL() anyway, so I think this is fine.

(I added a bool option to make @soltysh happy :-)

empty port mean 80 right? that might be incorrect.

Empty port means not specified in this case.

we default to https in RegistryURL() anyway, so I think this is fine.

ah, that's unfortunate...

miminar · 2017-03-09T13:57:09Z

pkg/image/api/validation/validation_test.go

+			isi:               &api.ImageStreamImport{ObjectMeta: validMeta, Spec: repoFn("unknown.registry:5000/foo/bar")},
+			expected:          field.ErrorList{},
+			allowedRegistries: &configapi.AllowedRegistries{},
+		},


Can you add one more test case with AllowedRegistries not empty and matching the registry?

sure, but that case is covered when allowedRegistry is set to defaultallowedregistry and matched docker.io

added extra test case.

miminar · 2017-03-09T13:59:34Z

pkg/cmd/server/start/master_args.go

+		// By default all registries have set to be "secure", iow. the port for them is
+		// defaulted to "443".
+		// If the registry you are adding here is insecure, you can add 'Insecure: true' which
+		// in that case it will default to port '80'.


s/which in that case it will default to port '80'/to make it default to "80"/

fixed. (moved to types anyway)

miminar · 2017-03-09T14:04:18Z

pkg/cmd/server/api/types.go

+type RegistryLocation struct {
+	// DomainName specifies a domain name for the registry
+	DomainName string
+	// Insecure indicates whether the registry is secury (https) or insecure (http)


soltysh · 2017-03-09T14:16:07Z

I will strongly argue against fixing the error part as part of this PR (the 2nd commit). I'd prefer having a proper solution to error handling inside the entire ImageStream. Currently we only have nice error handling presented per tag, but none when it comes to failing when .spec.dockerImageRepository is specified. I'd rather have a proper followup fixing the error handling once and for all and not an ad-hoc solution for this particular error.

mfojtik · 2017-03-09T14:20:19Z

@soltysh my fix is addressing the repository import as well.... in that case the repository is used as "tag", which sux and can be improved.
I need to report nice error to users that trying to import from remote, untrusted registry and this will do. I'm not against improving it, but we can get there in iterative step and the change in this PR can be the first step.

mfojtik · 2017-03-09T15:11:37Z

pkg/image/controller/controller.go

@@ -163,7 +169,50 @@ func (c *ImportController) Next(stream *api.ImageStream, notifier Notifier) erro
 		if apierrs.IsNotFound(err) && client.IsStatusErrorKind(err, "imageStream") {
 			return ErrNotImportable
 		}
-		glog.V(4).Infof("Import stream %s/%s partial=%t error: %v", stream.Namespace, stream.Name, partial, err)
+		glog.V(4).Infof("Import stream %s/%s partial=%t error: %#+v", stream.Namespace, stream.Name, partial, err)
+		if apierrs.IsInvalid(err) && client.IsStatusErrorKind(err, "ImageStreamImport") {


@smarterclayton this uglyness is needed to properly report the import errors to users who:

Create ImageStream that contains multiple tags (typically examples/imagestreams...)

The import fails because the registry used in ImageStream is invalid (for tag or for repository)

The error is then logged into glog.V(4) and burried in server log

User end up with ImageStream that tells you the tags are being imported... forever.

(in addition, if this happen, all other tags in that IS are not imported even though they are valid... but one problem at the time)...

And since the StatusInvalid will not tell us what tag failed specifically you have to guess it from the error message (which sux big time, but I can't figure out better solution).

I would said that this code is just a band-aid for having the import error reported properly (since white-listing registries needs to do this), but we should fix the error reporting in general from the image streams in 1.6.

Yes, please open a new issue and I will start working on it right after this merges.

Spawned #13319

miminar · 2017-03-09T15:31:39Z

pkg/image/api/validation/validation.go

+			glog.V(5).Infof("invalid registry domain format: %s", registry.DomainName)
+			continue
+		}
+		if util.IsWildcardMatch(registryHost, allowedRegistryHost) && registryPort == allowedRegistryPort {


So if the whitelist has a benevolent entry with insecure==true and someone wants import a from the repo securely, he won't be allowed to.

Another case this fails is an import from a registry hosted on :443 with invalid certificate - user is forced to use the insecure flag to no avail because the ports won't match here.

@miminar in that case you will have to add both combinations (secure and insecure) to the whitelist

mfojtik · 2017-03-09T15:56:46Z

[test]

mfojtik · 2017-04-04T08:09:39Z

[test]

mfojtik · 2017-04-05T11:09:57Z

[test] one more time

mfojtik · 2017-04-05T11:25:16Z

@smarterclayton @stevekuznetsov i still can't figure out what is wrong on GCE here... it seems like the image tags failed to import (presumably because something prevented them to). I wonder if we have some special master-config for GCE or we use the one that the Origin generates (iow. do we have whitelist or not).

Also I know that I'm annoying with this but it would really help to have master logs from GCE :-/

mfojtik · 2017-04-05T11:25:44Z

[test]

soltysh · 2017-04-05T11:29:19Z

@smarterclayton @soltysh support for local registry added:

This LGTM (since I wasn't explicit last time ;))

smarterclayton · 2017-04-05T11:31:01Z

You can run a deployment yourself pretty easily, or ssh in while it is running. The master config is generated by the Ansible install. On Apr 5, 2017, at 1:25 PM, Michal Fojtik <[email protected]> wrote: @smarterclayton <https://github.com/smarterclayton> @stevekuznetsov <https://github.com/stevekuznetsov> i still can't figure out what is wrong on GCE here... it seems like the image tags failed to import (presumably because something prevented them to). I wonder if we have some special master-config for GCE or we use the one that the Origin generates (iow. do we have whitelist or not). Also I know that I'm annoying with this but it would *really* help to have master logs from GCE :-/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13313 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p-iJd4nw36eU2NRwl40b7Sr5YaDEks5rs3oegaJpZM4MXTJq> .

mfojtik · 2017-04-06T09:30:23Z

gce docker flake [test]

mfojtik · 2017-04-06T11:17:26Z

[test]

…ed for import

mfojtik · 2017-04-06T12:07:07Z

@smarterclayton got it... it was actually my fault (missing nil-check)... it reflected on GCE because the master configuration we generate there for some reason does not include the whitelist (which means all images are allowed)...

mfojtik · 2017-04-06T12:10:05Z

[test] (should pass this time)

soltysh · 2017-04-06T12:14:07Z

@smarterclayton got it... it was actually my fault (missing nil-check)... it reflected on GCE because the master configuration we generate there for some reason does not include the whitelist (which means all images are allowed)...

IIRC, the configuration we have there is coming from our ansible script, doesn't that needs to be updated to reflect your changes in that case?

openshift-bot · 2017-04-06T12:21:31Z

Evaluated for origin test up to c058b4d

mfojtik · 2017-04-06T12:42:19Z

@soltysh I kind of like that it tests the "legacy" case where it is not set and the origin in that case should not enforce any whitelist and the legacy behavior is preserved... in fact it helped to discover this bug.

soltysh · 2017-04-06T12:58:34Z

@mfojtik completely agree, but that causes discrepancy between what we get when running installer manually vs what ansible does for us. Although I wonder which part of the ansible would be causing that. I though that --write-config which I assume is being called by ansible when generating configs should work ootb, but apparently it it not :/

openshift-bot · 2017-04-06T13:56:10Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/601/) (Base Commit: b7a5b0f)

soltysh · 2017-04-06T14:45:12Z

[merge]

openshift-bot · 2017-04-06T14:45:40Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/601/) (Image: devenv-rhel7_6119)

openshift-bot · 2017-04-06T14:45:41Z

Evaluated for origin merge up to c058b4d

smarterclayton · 2017-04-07T23:25:03Z

Ansible generates a config from a template, does not use write-config. Only new clusters in Ansible should get these flags, old users need to opt in (otherwise you could break working namespaces).

…

On Thu, Apr 6, 2017 at 4:47 PM, OpenShift Bot ***@***.***> wrote: Merged #13313 <#13313>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13313 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p6RBWorkUk6I01uHGvwwHhfQmsviks5rtPsCgaJpZM4MXTJq> .

smarterclayton · 2018-02-25T22:48:18Z

pkg/cmd/server/api/types.go

+		{DomainName: "gcr.io"},
+		{DomainName: "quay.io"},
+		// FIXME: Probably need to have more fine-tuned pattern defined
+		{DomainName: "*.amazonaws.com"},


We should not have added this - this effectively invalidates everything we did to secure this. This means that anyone can go and set a registry up on amazonaws and defeat this protection.

mfojtik force-pushed the registry-whitelist branch from 6123bb5 to 1521270 Compare March 8, 2017 20:57

mfojtik commented Mar 8, 2017

View reviewed changes

smarterclayton reviewed Mar 8, 2017

View reviewed changes

mfojtik force-pushed the registry-whitelist branch 4 times, most recently from 8c2c69d to 6fe3cbe Compare March 9, 2017 12:42

mfojtik changed the title ~~WIP: Restrict image imports to white-listed registries~~ Restrict image imports to white-listed registries Mar 9, 2017

mfojtik changed the title ~~Restrict image imports to white-listed registries~~ Allow to restrict imports to white listed registries Mar 9, 2017

soltysh suggested changes Mar 9, 2017

View reviewed changes

miminar suggested changes Mar 9, 2017

View reviewed changes

miminar reviewed Mar 9, 2017

View reviewed changes

mfojtik force-pushed the registry-whitelist branch from c6010e7 to e5b7594 Compare March 9, 2017 14:15

mfojtik force-pushed the registry-whitelist branch 2 times, most recently from d5257ed to c6fd96b Compare March 9, 2017 15:05

mfojtik commented Mar 9, 2017

View reviewed changes

mfojtik force-pushed the registry-whitelist branch from c6fd96b to 13b12ab Compare March 9, 2017 15:16

miminar reviewed Mar 9, 2017

View reviewed changes

mfojtik mentioned this pull request Mar 9, 2017

Improve import errors reporting to users #13319

Closed

mfojtik force-pushed the registry-whitelist branch from b5f7288 to f6da4cf Compare April 4, 2017 08:41

mfojtik force-pushed the registry-whitelist branch from b9b9f3b to 107e83d Compare April 5, 2017 11:22

mfojtik added 4 commits April 6, 2017 14:05

Add AllowedRegistriesForImport to allow whitelisting registries allow…

f16a9cf

…ed for import

Update swagger

8227ec2

Fix tags test to ignore pointers to list

da551c9

deploy: add missing failure trap

c058b4d

mfojtik force-pushed the registry-whitelist branch from 107e83d to c058b4d Compare April 6, 2017 12:05

openshift-bot merged commit 4b16943 into openshift:master Apr 6, 2017

stevekuznetsov mentioned this pull request Apr 6, 2017

oc exec hangs for six hours #13662

Closed

smarterclayton reviewed Feb 25, 2018

View reviewed changes

mfojtik deleted the registry-whitelist branch September 5, 2018 21:08

Allow to restrict imports to white listed registries #13313

Allow to restrict imports to white listed registries #13313

Conversation

mfojtik commented Mar 8, 2017 • edited Loading

mfojtik commented Mar 8, 2017

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

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

mfojtik commented Mar 9, 2017

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 left a comment • 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

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

soltysh commented Mar 9, 2017

mfojtik commented Mar 9, 2017

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

mfojtik commented Mar 9, 2017

mfojtik commented Apr 4, 2017

mfojtik commented Apr 5, 2017

mfojtik commented Apr 5, 2017

mfojtik commented Apr 5, 2017

soltysh commented Apr 5, 2017

smarterclayton commented Apr 5, 2017 via email

mfojtik commented Apr 6, 2017

mfojtik commented Apr 6, 2017

mfojtik commented Apr 6, 2017

mfojtik commented Apr 6, 2017

soltysh commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

mfojtik commented Apr 6, 2017

soltysh commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

soltysh commented Apr 6, 2017

openshift-bot commented Apr 6, 2017 • edited Loading

mfojtik commented Mar 8, 2017 •

edited

Loading

miminar left a comment •

edited

Loading

openshift-bot commented Apr 6, 2017 •

edited

Loading