Skip to content

Commit

Permalink
Addressed comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Michal Minář <[email protected]>
  • Loading branch information
Michal Minář committed Jan 12, 2018
1 parent ead80b8 commit 03f9742
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 45 deletions.
2 changes: 1 addition & 1 deletion hack/test-cmd.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ fi
# profile the web
export OPENSHIFT_PROFILE="${WEB_PROFILE-}"

export ADDITIONAL_ALLOWED_REGISTRIES=( "172.30.30.30:5000" "myregistry.com" )
export ADDITIONAL_ALLOWED_REGISTRIES=( "172.30.30.30:5000" "myregistry.com" "registry.centos.org" )

os::start::configure_server

Expand Down
18 changes: 14 additions & 4 deletions pkg/image/apis/image/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func ValidateImageStreamWithWhitelister(
}
}
if isValid && whitelister != nil {
if err := whitelister.AdmitDockerImageReference(&ref, whitelist.GetWhitelistTransportForFlag(insecureRepository, true)); err != nil {
if err := whitelister.AdmitDockerImageReference(&ref, getWhitelistTransportForFlag(insecureRepository, true)); err != nil {
result = append(result, field.Forbidden(dockerImageRepositoryPath, err.Error()))
}
}
Expand All @@ -218,7 +218,7 @@ func ValidateImageStreamWithWhitelister(
if tr, ok := stream.Spec.Tags[tag]; ok {
insecure = tr.ImportPolicy.Insecure
}
transport := whitelist.GetWhitelistTransportForFlag(insecure || insecureRepository, true)
transport := getWhitelistTransportForFlag(insecure || insecureRepository, true)
if err := whitelister.AdmitDockerImageReference(&ref, transport); err != nil {
result = append(result, field.Forbidden(field.NewPath("status", "tags").Key(tag).Child("items").Index(i).Child("dockerImageReference"), err.Error()))
}
Expand Down Expand Up @@ -249,7 +249,7 @@ func ValidateImageStreamTagReference(
} else if len(ref.ID) > 0 && tagRef.ImportPolicy.Scheduled {
errs = append(errs, field.Invalid(fldPath.Child("from", "name"), tagRef.From.Name, "only tags can be scheduled for import"))
} else if whitelister != nil {
transport := whitelist.GetWhitelistTransportForFlag(tagRef.ImportPolicy.Insecure || insecureRepository, true)
transport := getWhitelistTransportForFlag(tagRef.ImportPolicy.Insecure || insecureRepository, true)
if err := whitelister.AdmitDockerImageReference(&ref, transport); err != nil {
errs = append(errs, field.Forbidden(fldPath.Child("from", "name"), err.Error()))
}
Expand Down Expand Up @@ -341,7 +341,7 @@ func ValidateImageStreamStatusUpdateWithWhitelister(
}
}
}
transport := whitelist.GetWhitelistTransportForFlag(insecure, true)
transport := getWhitelistTransportForFlag(insecure, true)
if err := whitelister.AdmitDockerImageReference(&ref, transport); err != nil {
// TODO: should we whitelist references imported based on whitelisted/old spec?
// report error for each tag/history item having this reference
Expand Down Expand Up @@ -569,3 +569,13 @@ func isRepositoryInsecure(obj runtime.Object) bool {
}
return accessor.GetAnnotations()[imageapi.InsecureRepositoryAnnotation] == "true"
}

func getWhitelistTransportForFlag(insecure, allowSecureFallback bool) whitelist.WhitelistTransport {
if insecure {
if allowSecureFallback {
return whitelist.WhitelistTransportAny
}
return whitelist.WhitelistTransportInsecure
}
return whitelist.WhitelistTransportSecure
}
36 changes: 9 additions & 27 deletions pkg/image/apis/image/validation/whitelist/whitelister.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ type RegistryWhitelister interface {
WhitelistRegistry(hostPortGlob string, transport WhitelistTransport) error
// WhitelistPullSpecs allows to whitelist particular pull specs. References must match exactly one of the
// given pull specs for it to be whitelisted.
// TODO: accept insecure flag
WhitelistPullSpecs(pullSpecs ...string)
// Copy returns a deep copy of the whitelister. This is useful for temporarily whitelisting additional
// registries/pullSpecs before a specific validation.
Expand Down Expand Up @@ -78,7 +77,11 @@ func NewRegistryWhitelister(
// iterate in reversed order to make the patterns appear in the same order as given (patterns are prepended)
for i := len(whitelist) - 1; i >= 0; i-- {
registry := whitelist[i]
err := rw.WhitelistRegistry(registry.DomainName, GetWhitelistTransportForFlag(registry.Insecure, false))
transport := WhitelistTransportSecure
if registry.Insecure {
transport = WhitelistTransportInsecure
}
err := rw.WhitelistRegistry(registry.DomainName, transport)
if err != nil {
errs = append(errs, err)
}
Expand Down Expand Up @@ -112,9 +115,6 @@ func (rw *registryWhitelister) AdmitPullSpec(pullSpec string, transport Whitelis
}

func (rw *registryWhitelister) AdmitDockerImageReference(ref *imageapi.DockerImageReference, transport WhitelistTransport) error {
const showMax = 5
const fullWhitelistLogLevel = 5

if rw.pullSpecs.Len() > 0 {
if rw.pullSpecs.Has(ref.Exact()) || rw.pullSpecs.Has(ref.DockerClientDefaults().Exact()) || rw.pullSpecs.Has(ref.DaemonMinimal().Exact()) {
return nil
Expand Down Expand Up @@ -188,19 +188,12 @@ func (rw *registryWhitelister) AdmitDockerImageReference(ref *imageapi.DockerIma
}
}
whitelist := []string{}
full := []string{}
for i := 0; i < len(rw.whitelist); i++ {
if i < showMax-1 || len(rw.whitelist)-showMax == 0 {
whitelist = append(whitelist, fmt.Sprintf("%q", net.JoinHostPort(rw.whitelist[i].host, rw.whitelist[i].port)))
} else if i < showMax {
whitelist = append(whitelist, fmt.Sprintf("and %d more ...", len(rw.whitelist)-showMax+1))
} else if !glog.V(fullWhitelistLogLevel) {
break
}
full = append(full, fmt.Sprintf("%q", net.JoinHostPort(rw.whitelist[i].host, rw.whitelist[i].port)))
whitelist = append(whitelist, fmt.Sprintf("%q", net.JoinHostPort(rw.whitelist[i].host, rw.whitelist[i].port)))
}
glog.V(fullWhitelistLogLevel).Infof("registry %q not allowed by whitelist { %s }", hostname, strings.Join(full, ", "))
return fmt.Errorf("registry %q not allowed by whitelist { %s }", hostname, strings.Join(whitelist, ", "))
msg := fmt.Sprintf("registry %q not allowed by whitelist { %s }", hostname, strings.Join(whitelist, ", "))
glog.V(5).Info(msg)
return fmt.Errorf("%s", msg)
}

func (rw *registryWhitelister) WhitelistRegistry(hostPortGlob string, transport WhitelistTransport) error {
Expand Down Expand Up @@ -255,14 +248,3 @@ func (rw *registryWhitelister) Copy() RegistryWhitelister {
}
return &newRW
}

// GetWhitelistTransportForFlag returns transport for the given insecure flag.
func GetWhitelistTransportForFlag(insecure, allowSecureFallback bool) WhitelistTransport {
if insecure {
if allowSecureFallback {
return WhitelistTransportAny
}
return WhitelistTransportInsecure
}
return WhitelistTransportSecure
}
26 changes: 13 additions & 13 deletions pkg/image/apis/image/validation/whitelist/whitelister_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,10 @@ func TestRegistryWhitelister(t *testing.T) {
"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 ... }`),
"foo.com": fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
"index.docker.io": nil,
"localhost:5000": nil,
"my.domain.ltd:443": fmt.Errorf(`registry "my.domain.ltd:443" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),
"my.domain.ltd:443": fmt.Errorf(`registry "my.domain.ltd:443" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
"my.domain.ltd:80": nil,
"my.domain.ltd": nil,
"mydomain.ltd": nil,
Expand All @@ -201,8 +201,8 @@ func TestRegistryWhitelister(t *testing.T) {
},
difs: map[imageapi.DockerImageReference]error{
{Registry: "docker.io", Namespace: "library", Name: "busybox"}: nil,
{Registry: "foo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),
{Registry: "ffoo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "ffoo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),
{Registry: "foo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
{Registry: "ffoo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "ffoo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
},
},

Expand All @@ -211,24 +211,24 @@ func TestRegistryWhitelister(t *testing.T) {
transport: WhitelistTransportAny,
whitelist: mkAllowed(false, "localhost:5000", "docker.io", "example.com:*", "registry.com:80", "*.foo.com", "*domain.ltd"),
hostnames: map[string]error{
"a.b.c.d.foo.com:80": fmt.Errorf(`registry "a.b.c.d.foo.com:80" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", and 2 more ... }`),
"a.b.c.d.foo.com:80": fmt.Errorf(`registry "a.b.c.d.foo.com:80" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", "*.foo.com:443", "*domain.ltd:443" }`),
"domain.ltd": nil,
"example.com": nil,
"foo.com": fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", and 2 more ... }`),
"foo.com": fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", "*.foo.com:443", "*domain.ltd:443" }`),
"index.docker.io": nil,
"localhost:5000": nil,
"my.domain.ltd:443": nil,
"my.domain.ltd:80": fmt.Errorf(`registry "my.domain.ltd:80" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", and 2 more ... }`),
"my.domain.ltd:80": fmt.Errorf(`registry "my.domain.ltd:80" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", "*.foo.com:443", "*domain.ltd:443" }`),
"my.domain.ltd": nil,
"mydomain.ltd": nil,
"registry.com:443": fmt.Errorf(`registry "registry.com:443" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", and 2 more ... }`),
"registry.com:443": fmt.Errorf(`registry "registry.com:443" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", "*.foo.com:443", "*domain.ltd:443" }`),
"registry.com": nil,
"registry.foo.com": nil,
},
difs: map[imageapi.DockerImageReference]error{
{Registry: "docker.io", Namespace: "library", Name: "busybox"}: nil,
{Registry: "foo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", and 2 more ... }`),
{Registry: "ffoo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "ffoo.com" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", and 2 more ... }`),
{Registry: "foo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", "*.foo.com:443", "*domain.ltd:443" }`),
{Registry: "ffoo.com", Namespace: "library", Name: "busybox"}: fmt.Errorf(`registry "ffoo.com" not allowed by whitelist { "localhost:5000", "docker.io:443", "example.com:*", "registry.com:80", "*.foo.com:443", "*domain.ltd:443" }`),
},
},

Expand All @@ -240,14 +240,14 @@ func TestRegistryWhitelister(t *testing.T) {
"a.b.c.d.foo.com:80/repo": nil,
"domain.ltd/a/b": nil,
"example.com/c/d": nil,
"foo.com/foo": fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),
"foo.com/foo": fmt.Errorf(`registry "foo.com" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
"index.docker.io/bar": nil,
"localhost:5000/repo": nil,
"my.domain.ltd:443/a/b": fmt.Errorf(`registry "my.domain.ltd:443" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),
"my.domain.ltd:443/a/b": fmt.Errorf(`registry "my.domain.ltd:443" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
"my.domain.ltd:80/foo:latest": nil,
"my.domain.ltd/bar:1.3.4": nil,
"mydomain.ltd/my/repo/sitory": nil,
"registry.com:443/ab:tag": fmt.Errorf(`registry "registry.com:443" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", and 2 more ... }`),
"registry.com:443/ab:tag": fmt.Errorf(`registry "registry.com:443" not allowed by whitelist { "localhost:5000", "docker.io:80", "example.com:*", "registry.com:80", "*.foo.com:80", "*domain.ltd:80" }`),
"registry.com/repo": nil,
"registry.foo.com/123": nil,
"repository:latest": nil,
Expand Down

0 comments on commit 03f9742

Please sign in to comment.