Skip to content

Default config no longer sets a whitelist #19829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
May 25, 2018
Merged

Default config no longer sets a whitelist #19829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions pkg/cmd/server/apis/config/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,25 +135,6 @@ var (
}
KnownKubeAPIGroups = sets.StringKeySet(KubeAPIGroupsToAllowedVersions)
KnownOriginAPIGroups = sets.StringKeySet(OriginAPIGroupsToAllowedVersions)

// List public registries that we are allowing to import images from by default.
// 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' to
// make it default to port '80'.
// If the registry you are adding use custom port, you have to specify the port as
// part of the domain name.
DefaultAllowedRegistriesForImport = &AllowedRegistries{
{DomainName: "docker.io"},
{DomainName: "*.docker.io"}, // registry-1.docker.io
{DomainName: "*.redhat.com"}, // registry.connect.redhat.com and registry.access.redhat.com
{DomainName: "gcr.io"},
{DomainName: "quay.io"},
{DomainName: "registry.centos.org"},
{DomainName: "registry.redhat.io"},
// FIXME: Probably need to have more fine-tuned pattern defined
{DomainName: "*.amazonaws.com"},
}
)

type ExtendedArguments map[string][]string
Expand Down
11 changes: 1 addition & 10 deletions pkg/cmd/server/start/master_args.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,16 +241,7 @@ func (args MasterArgs) BuildSerializeableMasterConfig() (*configapi.MasterConfig
Latest: args.ImageFormatArgs.ImageTemplate.Latest,
},

// List public registries that we are allowing to import images from by default.
// 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'.
// If the registry you are adding use custom port, you have to specify the port as
// part of the domain name.
ImagePolicyConfig: configapi.ImagePolicyConfig{
AllowedRegistriesForImport: configapi.DefaultAllowedRegistriesForImport,
},
ImagePolicyConfig: configapi.ImagePolicyConfig{},
Copy link
Contributor

@bparees bparees May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the constant even used anywhere anymore then? can we get rid of it?

Copy link
Contributor Author

@smarterclayton smarterclayton May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the e2e tests

Copy link
Contributor Author

@smarterclayton smarterclayton May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and in the integration tests

Copy link
Contributor

@bparees bparees May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so should we move the constant into the test package? seems misleading/confusing to call if defaultallowedregistriesforimport in our core packages when it's not actually that.

Copy link
Contributor Author

@smarterclayton smarterclayton May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could call it recommended.


ProjectConfig: configapi.ProjectConfig{
DefaultNodeSelector: "",
Expand Down
37 changes: 36 additions & 1 deletion test/integration/imageimporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ func TestImageStreamImport(t *testing.T) {
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
err = testutil.CreateNamespace(clusterAdminKubeConfig, testutil.Namespace())
_, userConfig, err := testserver.CreateNewProject(clusterAdminConfig, testutil.Namespace(), "unprivileged-image-stream-importer")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
clusterAdminImageClient := imageclient.NewForConfigOrDie(clusterAdminConfig).Image()
userImageClient := imageclient.NewForConfigOrDie(userConfig).Image()

// can't give invalid image specs, should be invalid
isi, err := clusterAdminImageClient.ImageStreamImports(testutil.Namespace()).Create(&imageapi.ImageStreamImport{
Expand All @@ -69,6 +70,40 @@ func TestImageStreamImport(t *testing.T) {
t.Fatal(err)
}

// can't create on non-whitelisted images
isi, err = userImageClient.ImageStreamImports(testutil.Namespace()).Create(&imageapi.ImageStreamImport{
ObjectMeta: metav1.ObjectMeta{
Name: "doesnotexist",
},
Spec: imageapi.ImageStreamImportSpec{
Images: []imageapi.ImageImportSpec{
{From: kapi.ObjectReference{Kind: "DockerImage", Name: "mycompany.com/test/forbidden-image"}, To: &kapi.LocalObjectReference{Name: "tag"}},
},
},
})
if err == nil || !errors.IsInvalid(err) {
t.Fatalf("unexpected responses: %#v %#v %#v", err, isi, isi.Status.Import)
}
// does not create stream
if _, err := userImageClient.ImageStreams(testutil.Namespace()).Get("doesnotexist", metav1.GetOptions{}); err == nil || !errors.IsNotFound(err) {
t.Fatal(err)
}
// verify we can't create a tag outside the whitelist either
if _, err := userImageClient.ImageStreams(testutil.Namespace()).Create(&imageapi.ImageStream{
ObjectMeta: metav1.ObjectMeta{
Name: "import-whitelist",
},
Spec: imageapi.ImageStreamSpec{
Tags: map[string]imageapi.TagReference{
"invalid": {
From: &kapi.ObjectReference{Name: "mycompany.com/test/forbidden-image", Kind: "DockerImage"},
},
},
},
}); err == nil || !errors.IsInvalid(err) || !strings.Contains(err.Error(), "Forbidden: registry \"mycompany.com\" not allowed by whitelist") {
t.Fatal(err)
}

// import without committing
isi, err = clusterAdminImageClient.ImageStreamImports(testutil.Namespace()).Create(&imageapi.ImageStreamImport{
ObjectMeta: metav1.ObjectMeta{
Expand Down
19 changes: 18 additions & 1 deletion test/util/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,26 @@ func DefaultMasterOptionsWithTweaks(useDefaultPort bool) (*configapi.MasterConfi
masterConfig.EtcdClientInfo.URLs = []string{"https://" + masterConfig.EtcdConfig.Address}
}

// List public registries that make sense to allow importing images from by default.
// 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' to
// make it default to port '80'.
// If the registry you are adding use custom port, you have to specify the port as
// part of the domain name.
recommendedAllowedRegistriesForImport := configapi.AllowedRegistries{
{DomainName: "docker.io"},
{DomainName: "*.docker.io"}, // registry-1.docker.io
{DomainName: "*.redhat.com"}, // registry.connect.redhat.com and registry.access.redhat.com
{DomainName: "gcr.io"},
{DomainName: "quay.io"},
{DomainName: "registry.centos.org"},
{DomainName: "registry.redhat.io"},
}

masterConfig.ImagePolicyConfig.ScheduledImageImportMinimumIntervalSeconds = 1
allowedRegistries := append(
*configapi.DefaultAllowedRegistriesForImport,
recommendedAllowedRegistriesForImport,
configapi.RegistryLocation{DomainName: "127.0.0.1:*"},
)
for r := range util.GetAdditionalAllowedRegistries() {
Expand Down