Allow credential mapping from dockercfg for canonical ports

pkg/image/importer/credentials.go

@@ -165,7 +165,13 @@ 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 {
+		// only lookup credential for http that say they are for http
+		value = target.String()
+	}
 
 	// Lookup(...) expects an image (not a URL path).
 	// The keyring strips /v1/ and /v2/ version prefixes,
@@ -188,6 +194,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

@@ -11,7 +11,6 @@ import (
 	kapiv1 "k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/credentialprovider"
 
-	"github.com/golang/glog"
 	_ "github.com/openshift/origin/pkg/api/install"
 )
 
@@ -29,7 +28,7 @@ func TestCredentialsForSecrets(t *testing.T) {
 	for i, secret := range secrets.Items {
 		err := kapiv1.Convert_api_Secret_To_v1_Secret(&secret, &secretsv1[i], nil)
 		if err != nil {
-			glog.V(2).Infof("Unable to make the Docker keyring for %s/%s secret: %v", secret.Name, secret.Namespace, err)
+			t.Logf("Unable to make the Docker keyring for %s/%s secret: %v", secret.Name, secret.Namespace, err)
 			continue
 		}
 	}
@@ -70,3 +69,57 @@ 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, but should be
+		{name: "exact http", args: args{keyring: fn("http://localhost", def), target: &url.URL{Scheme: "http", Host: "localhost:80"}}, user: "", password: ""},
+		{name: "keyring canonical https", args: args{keyring: fn("localhost:443", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: "", password: ""},
+
+		// these should not be allowed
+		{name: "host is for port 80 only", args: args{keyring: fn("localhost:80", def), target: &url.URL{Host: "localhost"}}, user: "", password: ""},
+		{name: "host is for port 443 only", args: args{keyring: fn("localhost:443", def), target: &url.URL{Host: "localhost"}}, user: "", password: ""},
+		{name: "don't assume port 80 in keyring is https", args: args{keyring: fn("localhost:80", def), target: &url.URL{Scheme: "http", Host: "localhost"}}, user: "", password: ""},
+		{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, actual = %v", user, tt.user)
+			}
+			if password != tt.password {
+				t.Errorf("basicCredentialsFromKeyring() password = %v, actual = %v", password, tt.password)
+			}
+		})
+	}
+}