Skip to content

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

Merged
merged 4 commits into from
Apr 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions pkg/api/install/tags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ func checkInternalJsonTags(objType reflect.Type, seen *map[reflect.Type]bool, t
if internalTypesWithAllowedJsonTags.Has(objType.Name()) {
return
}
if objType.Kind() != reflect.Struct {
Copy link
Contributor

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

Copy link
Contributor Author

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.

return
}

for i := 0; i < objType.NumField(); i++ {
structField := objType.FieldByIndex([]int{i})
Expand Down Expand Up @@ -143,6 +146,10 @@ func checkExternalJsonTags(objType reflect.Type, seen *map[reflect.Type]bool, t
return
}

if objType.Kind() != reflect.Struct {
return
}

for i := 0; i < objType.NumField(); i++ {
structField := objType.FieldByIndex([]int{i})

Expand Down
7 changes: 7 additions & 0 deletions pkg/cmd/server/api/serialization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ func fuzzInternalObject(t *testing.T, forVersion unversioned.GroupVersion, item
if len(obj.APILevels) == 0 {
obj.APILevels = configapi.DefaultOpenShiftAPILevels
}
if obj.ImagePolicyConfig.AllowedRegistriesForImport == nil {
obj.ImagePolicyConfig.AllowedRegistriesForImport = &configapi.AllowedRegistries{}
}
if len(obj.Controllers) == 0 {
obj.Controllers = configapi.ControllersAll
}
Expand Down Expand Up @@ -259,6 +262,10 @@ func fuzzInternalObject(t *testing.T, forVersion unversioned.GroupVersion, item
if obj.ScheduledImageImportMinimumIntervalSeconds == 0 {
obj.ScheduledImageImportMinimumIntervalSeconds = 15 * 60
}
obj.AllowedRegistriesForImport = &configapi.AllowedRegistries{
{DomainName: "docker.io"},
{DomainName: "gcr.io"},
}
},
func(obj *configapi.DNSConfig, c fuzz.Continue) {
c.FuzzNoCustom(obj)
Expand Down
39 changes: 39 additions & 0 deletions pkg/cmd/server/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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 {
Expand Down
11 changes: 11 additions & 0 deletions pkg/cmd/server/api/v1/swagger_doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ var map_ImagePolicyConfig = map[string]string{
"disableScheduledImport": "DisableScheduledImport allows scheduled background import of images to be disabled.",
"scheduledImageImportMinimumIntervalSeconds": "ScheduledImageImportMinimumIntervalSeconds is the minimum number of seconds that can elapse between when image streams scheduled for background import are checked against the upstream repository. The default value is 15 minutes.",
"maxScheduledImageImportsPerMinute": "MaxScheduledImageImportsPerMinute is the maximum number of scheduled image streams that will be imported in the background per minute. The default value is 60. Set to -1 for unlimited.",
"allowedRegistriesForImport": "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.",
}

func (ImagePolicyConfig) SwaggerDoc() map[string]string {
Expand Down Expand Up @@ -702,6 +703,16 @@ func (RFC2307Config) SwaggerDoc() map[string]string {
return map_RFC2307Config
}

var map_RegistryLocation = map[string]string{
"": "RegistryLocation contains a location of the registry specified by the registry domain name. The domain name might include wildcards, like '*' or '??'.",
"domainName": "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.",
"insecure": "Insecure indicates whether the registry is secure (https) or insecure (http) By default (if not specified) the registry is assumed as secure.",
}

func (RegistryLocation) SwaggerDoc() map[string]string {
return map_RegistryLocation
}

var map_RemoteConnectionInfo = map[string]string{
"": "RemoteConnectionInfo holds information necessary for establishing a remote connection",
"url": "URL is the remote URL to connect to",
Expand Down
22 changes: 22 additions & 0 deletions pkg/cmd/server/api/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,28 @@ type ImagePolicyConfig struct {
// MaxScheduledImageImportsPerMinute is the maximum number of scheduled image streams that will be imported in the
// background per minute. The default value is 60. Set to -1 for unlimited.
MaxScheduledImageImportsPerMinute int `json:"maxScheduledImageImportsPerMinute"`
// 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 `json:"allowedRegistriesForImport,omitempty"`
}

// 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 `json:"domainName"`
// Insecure indicates whether the registry is secure (https) or insecure (http)
// By default (if not specified) the registry is assumed as secure.
Insecure bool `json:"insecure,omitempty"`
}

// holds the necessary configuration options for
Expand Down
23 changes: 23 additions & 0 deletions pkg/cmd/server/api/validation/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/url"
"reflect"
"regexp"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -473,6 +474,28 @@ func ValidateImagePolicyConfig(config api.ImagePolicyConfig, fldPath *field.Path
if config.MaxScheduledImageImportsPerMinute == 0 || config.MaxScheduledImageImportsPerMinute < -1 {
errs = append(errs, field.Invalid(fldPath.Child("maxScheduledImageImportsPerMinute"), config.MaxScheduledImageImportsPerMinute, "must be a positive integer or -1"))
}
if config.AllowedRegistriesForImport != nil {
for i, registry := range *config.AllowedRegistriesForImport {
if len(registry.DomainName) == 0 {
errs = append(errs, field.Invalid(fldPath.Index(i).Child("allowedRegistriesForImport", "domainName"), registry.DomainName, "cannot be an empty string"))
}
parts := strings.Split(registry.DomainName, ":")
// Check for ':8080'
if len(parts) == 0 || len(parts[0]) == 0 {
errs = append(errs, field.Invalid(fldPath.Index(i).Child("allowedRegistriesForImport", "domainName"), registry.DomainName, "invalid domain specified, must be registry.url.local[:port]"))
}
// Check for 'foo:bar:1234'
if len(parts) > 2 {
errs = append(errs, field.Invalid(fldPath.Index(i).Child("allowedRegistriesForImport", "domainName"), registry.DomainName, "invalid format, must be registry.url.local[:port]"))
}
// Check for 'foo:bar'
if len(parts) == 2 {
if _, err := strconv.Atoi(parts[1]); err != nil {
errs = append(errs, field.Invalid(fldPath.Index(i).Child("allowedRegistriesForImport", "domainName"), registry.DomainName, "invalid port format, must be a number"))
}
}
}
}
return errs
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/server/origin/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ func (c *MasterConfig) GetRestStorage() map[unversioned.GroupVersion]map[string]
importerDockerClientFn := func() dockerregistry.Client {
return dockerregistry.NewClient(20*time.Second, false)
}
imageStreamImportStorage := imagestreamimport.NewREST(importerFn, imageStreamRegistry, internalImageStreamStorage, imageStorage, c.ImageStreamImportSecretClient(), importTransport, insecureImportTransport, importerDockerClientFn)
imageStreamImportStorage := imagestreamimport.NewREST(importerFn, imageStreamRegistry, internalImageStreamStorage, imageStorage, c.ImageStreamImportSecretClient(), importTransport, insecureImportTransport, importerDockerClientFn, c.Options.ImagePolicyConfig.AllowedRegistriesForImport, c.RegistryNameFn, c.ImageStreamImportSARClient().SubjectAccessReviews())
imageStreamImageStorage := imagestreamimage.NewREST(imageRegistry, imageStreamRegistry)
imageStreamImageRegistry := imagestreamimage.NewRegistry(imageStreamImageStorage)

Expand Down
5 changes: 5 additions & 0 deletions pkg/cmd/server/origin/master_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,11 @@ func (c *MasterConfig) ImageStreamImportSecretClient() *osclient.Client {
return c.PrivilegedLoopbackOpenShiftClient
}

// ImageStreamImportSARClient returns the client capable of performing self-SAR requests
func (c *MasterConfig) ImageStreamImportSARClient() *osclient.Client {
return c.PrivilegedLoopbackOpenShiftClient
}

// ResourceQuotaManagerClients returns the client capable of retrieving resources needed for resource quota
// evaluation
func (c *MasterConfig) ResourceQuotaManagerClients() (*osclient.Client, *kclientset.Clientset) {
Expand Down
11 changes: 11 additions & 0 deletions pkg/cmd/server/start/master_args.go
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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{
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 oc cluster up we can add an option (--add-trusted-registries or something?)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
whitelisting mechanism reject that? Or it's smart enough to know it's the internal registry and will allow that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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: "",
Expand Down
20 changes: 20 additions & 0 deletions pkg/image/api/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"net"
"net/url"
"regexp"
"sort"
Expand Down Expand Up @@ -34,6 +35,10 @@ const (

// TagReferenceAnnotationTagHidden indicates that a given TagReference is hidden from search results
TagReferenceAnnotationTagHidden = "hidden"

// ImportRegistryNotAllowed indicates that the image tag was not imported due to
// untrusted registry.
ImportRegistryNotAllowed = "registry is not allowed for import"
)

// DefaultRegistry returns the default Docker registry (host or host:port), or false if it is not available.
Expand Down Expand Up @@ -168,6 +173,21 @@ func (r DockerImageReference) RepositoryName() string {
return r.Exact()
}

// RegistryHostPort returns the registry hostname and the port.
// If the port is not specified in the registry hostname we default to 443.
// This will also default to Docker client defaults if the registry hostname is empty.
func (r DockerImageReference) RegistryHostPort(insecure bool) (string, string) {
registryHost := r.AsV2().DockerClientDefaults().Registry
if strings.Contains(registryHost, ":") {
hostname, port, _ := net.SplitHostPort(registryHost)
return hostname, port
}
if insecure {
return registryHost, "80"
}
return registryHost, "443"
}

// RepositoryName returns the registry relative name
func (r DockerImageReference) RegistryURL() *url.URL {
return &url.URL{
Expand Down
33 changes: 33 additions & 0 deletions pkg/image/api/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ import (
"k8s.io/kubernetes/pkg/util/diff"
"k8s.io/kubernetes/pkg/util/validation/field"

serverapi "github.com/openshift/origin/pkg/cmd/server/api"
"github.com/openshift/origin/pkg/image/api"
stringsutil "github.com/openshift/origin/pkg/util/strings"
)

// RepositoryNameComponentRegexp restricts registry path component names to
Expand Down Expand Up @@ -297,6 +299,37 @@ func ValidateImageStreamTagUpdate(newIST, oldIST *api.ImageStreamTag) field.Erro
return result
}

func ValidateRegistryAllowedForImport(path *field.Path, name, registryHost, registryPort string, allowedRegistries *serverapi.AllowedRegistries) field.ErrorList {
errs := field.ErrorList{}
if allowedRegistries == nil {
return errs
}
allowedRegistriesForHumans := []string{}
for _, registry := range *allowedRegistries {
allowedRegistryHost, allowedRegistryPort := "", ""
parts := strings.Split(registry.DomainName, ":")
switch len(parts) {
case 1:
allowedRegistryHost = parts[0]
if registry.Insecure {
allowedRegistryPort = "80"
} else {
allowedRegistryPort = "443"
}
case 2:
allowedRegistryHost, allowedRegistryPort = parts[0], parts[1]
default:
continue
}
if stringsutil.IsWildcardMatch(registryHost, allowedRegistryHost) && stringsutil.IsWildcardMatch(registryPort, allowedRegistryPort) {
return errs
}
allowedRegistriesForHumans = append(allowedRegistriesForHumans, registry.DomainName)
}
return append(errs, field.Invalid(path, name,
fmt.Sprintf("importing images from registry %q is forbidden, only images from %q are allowed", registryHost+":"+registryPort, strings.Join(allowedRegistriesForHumans, ","))))
}

func ValidateImageStreamImport(isi *api.ImageStreamImport) field.ErrorList {
specPath := field.NewPath("spec")
imagesPath := specPath.Child("images")
Expand Down
Loading