-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Allow to restrict imports to white listed registries #13313
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,23 @@ var ( | |
} | ||
KnownOpenShiftFeatures = []string{FeatureBuilder, FeatureS2I, FeatureWebConsole} | ||
AtomicDisabledFeatures = []string{FeatureBuilder, FeatureS2I, FeatureWebConsole} | ||
|
||
// 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: "registry.access.redhat.com"}, | ||
{DomainName: "gcr.io"}, | ||
{DomainName: "quay.io"}, | ||
// FIXME: Probably need to have more fine-tuned pattern defined | ||
{DomainName: "*.amazonaws.com"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
) | ||
|
||
type ExtendedArguments map[string][]string | ||
|
@@ -455,6 +472,28 @@ type ImagePolicyConfig struct { | |
// MaxScheduledImageImportsPerMinute is the maximum number of image streams that will be imported in the background per minute. | ||
// The default value is 60. Set to -1 for unlimited. | ||
MaxScheduledImageImportsPerMinute int | ||
// AllowedRegistriesForImport limits the docker registries that normal users may import | ||
// images from. Set this list to the registries that you trust to contain valid Docker | ||
// images and that you want applications to be able to import from. Users with | ||
// permission to create Images or ImageStreamMappings via the API are not affected by | ||
// this policy - typically only administrators or system integrations will have those | ||
// permissions. | ||
AllowedRegistriesForImport *AllowedRegistries | ||
} | ||
|
||
// AllowedRegistries represents a list of registries allowed for the image import. | ||
type AllowedRegistries []RegistryLocation | ||
|
||
// RegistryLocation contains a location of the registry specified by the registry domain | ||
// name. The domain name might include wildcards, like '*' or '??'. | ||
type RegistryLocation struct { | ||
// DomainName specifies a domain name for the registry | ||
// In case the registry use non-standard (80 or 443) port, the port should be included | ||
// in the domain name as well. | ||
DomainName string | ||
// Insecure indicates whether the registry is secure (https) or insecure (http) | ||
// By default (if not specified) the registry is assumed as secure. | ||
Insecure bool | ||
} | ||
|
||
type ProjectConfig struct { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,6 +242,17 @@ 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'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. (moved to types anyway) |
||
// If the registry you are adding use custom port, you have to specify the port as | ||
// part of the domain name. | ||
ImagePolicyConfig: configapi.ImagePolicyConfig{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I'm not even sure we should do this as a default. It breaks random users, and oc cluster up. It's probably the right thing to do, but not sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you review how many users and default behaviors this will break when we set this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @smarterclayton this only breaks you if you use non-standard third-party registry when importing the images. So I don't think it will break a lot of users. For those who will be affected, we crafted a nice error message. Maybe for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm really worried this is going to break everyone who tries it, including oc cluster up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to ask that, what happens if I try to import image from openshift's internal registry, will your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really hesistate to whitelist the integrated registry for importing as I still can't think of any reasonable use-case somebody will want to do that (except user error). This might also get complicated by having DNS for the registry vs. IP address vs. external route and by the fact the we don't have a single place that give us all these information. |
||
AllowedRegistriesForImport: configapi.DefaultAllowedRegistriesForImport, | ||
}, | ||
|
||
ProjectConfig: configapi.ProjectConfig{ | ||
DefaultNodeSelector: "", | ||
ProjectRequestMessage: "", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I had to do this in another PR as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton was just going to ask if this is a 'safe' change... having types that are lists seems reasonable to me.