Allow credential mapping from dockercfg for canonical ports

Author: smarterclayton

pkg/image/importer/credentials.go

@@ -161,7 +161,12 @@ func (s *SecretCredentialStore) init() credentialprovider.DockerKeyring {
 
 func basicCredentialsFromKeyring(keyring credentialprovider.DockerKeyring, target *url.URL) (string, string) {
 	// TODO: compare this logic to Docker authConfig in v2 configuration
-	value := target.Host + target.Path
+	var value string
+	if len(target.Scheme) == 0 || target.Scheme == "https" {
+		value = target.Host + target.Path
+	} else {
+		value = target.String()
+	}
 
 	// Lookup(...) expects an image (not a URL path).
 	// The keyring strips /v1/ and /v2/ version prefixes,
@@ -184,6 +189,16 @@ func basicCredentialsFromKeyring(keyring credentialprovider.DockerKeyring, targe
 			glog.V(5).Infof("Being asked for %s, trying %s for legacy behavior", target, "docker.io")
 			return basicCredentialsFromKeyring(keyring, &url.URL{Host: "docker.io"})
 		}
+
+		// try removing the canonical ports for the given requests
+		if (strings.HasSuffix(target.Host, ":443") && target.Scheme == "https") ||
+			(strings.HasSuffix(target.Host, ":80") && target.Scheme == "http") {
+			host := strings.SplitN(target.Host, ":", 2)[0]
+			glog.V(5).Infof("Being asked for %s, trying %s without port", target, host)
+
+			return basicCredentialsFromKeyring(keyring, &url.URL{Scheme: target.Scheme, Host: host, Path: target.Path})
+		}
+
 		glog.V(5).Infof("Unable to find a secret to match %s (%s)", target, value)
 		return "", ""
 	}

pkg/image/importer/credentials_test.go

@@ -6,11 +6,10 @@ import (
 	"reflect"
 	"testing"
 
+	_ "github.com/openshift/origin/pkg/api/install"
 	"k8s.io/apimachinery/pkg/runtime"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/credentialprovider"
-
-	_ "github.com/openshift/origin/pkg/api/install"
 )
 
 func TestCredentialsForSecrets(t *testing.T) {
@@ -59,3 +58,53 @@ func TestBasicCredentials(t *testing.T) {
 		t.Fatalf("unexpected response: %s %s", u, p)
 	}
 }
+
+func Test_basicCredentialsFromKeyring(t *testing.T) {
+	fn := func(host string, entry credentialprovider.DockerConfigEntry) credentialprovider.DockerKeyring {
+		k := &credentialprovider.BasicDockerKeyring{}
+		k.Add(map[string]credentialprovider.DockerConfigEntry{host: entry})
+		return k
+	}
+	def := credentialprovider.DockerConfigEntry{
+		Username: "local_user",
+		Password: "local_pass",
+	}
+	type args struct {
+		keyring credentialprovider.DockerKeyring
+		target  *url.URL
+	}
+	tests := []struct {
+		name     string
+		args     args
+		user     string
+		password string
+	}{
+		{name: "exact", args: args{keyring: fn("localhost", def), target: &url.URL{Host: "localhost"}}, user: def.Username, password: def.Password},
+		{name: "https scheme", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password},
+		{name: "canonical https", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "https", Host: "localhost:443"}}, user: def.Username, password: def.Password},
+		{name: "only https", args: args{keyring: fn("https://localhost", def), target: &url.URL{Host: "localhost"}}, user: def.Username, password: def.Password},
+		{name: "only https scheme", args: args{keyring: fn("https://localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password},
+		{name: "mismatched scheme - http", args: args{keyring: fn("http://localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password},
+
+		// this is not allowed by the credential keyring, possibly because of insufficient testing
+		{name: "exact http", args: args{keyring: fn("http://localhost", def), target: &url.URL{Scheme: "http", Host: "localhost:80"}}, user: "", password: ""},
+
+		// these should not be allowed
+		{name: "canonical http", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "http", Host: "localhost:80"}}, user: "", password: ""},
+		{name: "http scheme", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "http", Host: "localhost"}}, user: "", password: ""},
+		{name: "https not canonical", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "https", Host: "localhost:80"}}, user: "", password: ""},
+		{name: "http not canonical", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "http", Host: "localhost:443"}}, user: "", password: ""},
+		{name: "mismatched scheme", args: args{keyring: fn("https://localhost", def), target: &url.URL{Scheme: "http", Host: "localhost"}}, user: "", password: ""},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			user, password := basicCredentialsFromKeyring(tt.args.keyring, tt.args.target)
+			if user != tt.user {
+				t.Errorf("basicCredentialsFromKeyring() user = %v, user = %v", user, tt.user)
+			}
+			if password != tt.password {
+				t.Errorf("basicCredentialsFromKeyring() password = %v, password = %v", password, tt.password)
+			}
+		})
+	}
+}