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 a78e5eb
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 82 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
}
34 changes: 17 additions & 17 deletions pkg/image/apis/image/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ func TestValidateImageStreamWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("spec", "tags").Key("fail").Child("from", "name"),
`registry "registry.ltd" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "registry.ltd" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
},
},

Expand Down Expand Up @@ -673,11 +673,11 @@ func TestValidateImageStreamWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("status", "tags").Key("secure").Child("items").Index(0).Child("dockerImageReference"),
`registry "docker.io" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "docker.io" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
field.Forbidden(field.NewPath("status", "tags").Key("secure").Child("items").Index(2).Child("dockerImageReference"),
`registry "example.com:80" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "example.com:80" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
field.Forbidden(field.NewPath("status", "tags").Key("secure").Child("items").Index(3).Child("dockerImageReference"),
`registry "dev.null.io" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "dev.null.io" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
},
},

Expand Down Expand Up @@ -720,9 +720,9 @@ func TestValidateImageStreamWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("status", "tags").Key("insecure").Child("items").Index(1).Child("dockerImageReference"),
`registry "example.com:80" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "example.com:80" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
field.Forbidden(field.NewPath("status", "tags").Key("insecure").Child("items").Index(2).Child("dockerImageReference"),
`registry "registry.ltd" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "registry.ltd" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
},
},

Expand Down Expand Up @@ -763,7 +763,7 @@ func TestValidateImageStreamWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("status", "tags").Key("securebydefault").Child("items").Index(0).Child("dockerImageReference"),
`registry "localhost" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "localhost" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
},
},

Expand Down Expand Up @@ -804,7 +804,7 @@ func TestValidateImageStreamWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("status", "tags").Key("securebydefault").Child("items").Index(0).Child("dockerImageReference"),
`registry "localhost" not allowed by whitelist { "example.com:443", "localhost:5000", "dev.null.io:80" }`),
`registry "localhost" not allowed by whitelist: "example.com:443", "localhost:5000", "dev.null.io:80"`),
},
},

Expand All @@ -823,7 +823,7 @@ func TestValidateImageStreamWithWhitelister(t *testing.T) {
dockerImageRepository: "example.com/openshift/origin",
expected: field.ErrorList{
field.Forbidden(field.NewPath("spec", "dockerImageRepository"),
`registry "example.com" not allowed by whitelist { "*.example.com:443" }`),
`registry "example.com" not allowed by whitelist: "*.example.com:443"`),
},
},
} {
Expand Down Expand Up @@ -916,9 +916,9 @@ func TestValidateImageStreamUpdateWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("spec", "tags").Key("fail").Child("from", "name"),
`registry "example.com" not allowed by whitelist { "docker.io:443" }`),
`registry "example.com" not allowed by whitelist: "docker.io:443"`),
field.Forbidden(field.NewPath("status", "tags").Key("fail").Child("items").Index(0).Child("dockerImageReference"),
`registry "example.com" not allowed by whitelist { "docker.io:443" }`),
`registry "example.com" not allowed by whitelist: "docker.io:443"`),
},
},

Expand Down Expand Up @@ -994,7 +994,7 @@ func TestValidateImageStreamUpdateWithWhitelister(t *testing.T) {
newDockerImageRepository: "example.com/my/newapp",
expected: field.ErrorList{
field.Forbidden(field.NewPath("spec", "dockerImageRepository"),
`registry "example.com" not allowed by whitelist { "docker.io:443" }`)},
`registry "example.com" not allowed by whitelist: "docker.io:443"`)},
},

{
Expand Down Expand Up @@ -1148,7 +1148,7 @@ func TestValidateISTUpdateWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("tag", "from", "name"),
`registry "docker.io:443" not allowed by whitelist { "example.com:*" }`),
`registry "docker.io:443" not allowed by whitelist: "example.com:*"`),
},
},

Expand Down Expand Up @@ -1178,7 +1178,7 @@ func TestValidateISTUpdateWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("tag", "from", "name"),
`registry "docker.io:443" not allowed by whitelist { "example.com:*" }`),
`registry "docker.io:443" not allowed by whitelist: "example.com:*"`),
},
},

Expand All @@ -1191,7 +1191,7 @@ func TestValidateISTUpdateWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("tag", "from", "name"),
`registry "example.com" not allowed by whitelist { "example.com:80" }`),
`registry "example.com" not allowed by whitelist: "example.com:80"`),
},
},

Expand Down Expand Up @@ -1233,7 +1233,7 @@ func TestValidateISTUpdateWithWhitelister(t *testing.T) {
},
expected: field.ErrorList{
field.Forbidden(field.NewPath("tag", "from", "name"),
`registry "docker.io" not allowed by whitelist { "example.com:443" }`),
`registry "docker.io" not allowed by whitelist: "example.com:443"`),
},
},
} {
Expand Down Expand Up @@ -1288,7 +1288,7 @@ func TestValidateRegistryAllowedForImport(t *testing.T) {
hostname: "example.com:443",
whitelist: *mkAllowed(false, "foo.bar"),
expected: field.ErrorList{field.Forbidden(nil,
`importing images from registry "example.com:443" is forbidden: registry "example.com:443" not allowed by whitelist { "foo.bar:443" }`)},
`importing images from registry "example.com:443" is forbidden: registry "example.com:443" not allowed by whitelist: "foo.bar:443"`)},
},
} {
t.Run(tc.name, func(t *testing.T) {
Expand Down
48 changes: 19 additions & 29 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 @@ -179,28 +179,29 @@ func (rw *registryWhitelister) AdmitDockerImageReference(ref *imageapi.DockerIma
}
}

hostname := ref.Registry
var (
hostname = ref.Registry
whitelist = []string{}
msg string
)
if len(ref.Registry) == 0 {
if len(port) > 0 {
hostname = net.JoinHostPort(host, port)
} else {
hostname = host
}
}
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, ", "))
if len(rw.whitelist) > 0 {
msg = fmt.Sprintf("registry %q not allowed by whitelist: %s", hostname, strings.Join(whitelist, ", "))
} else {
msg = fmt.Sprintf("registry %q not allowed by empty whitelist", hostname)
}
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 +256,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
}
Loading

0 comments on commit a78e5eb

Please sign in to comment.