Improve basic auth handling for canonical ports

pkg/image/importer/client.go

@@ -47,19 +47,54 @@ func NewContext(transport, insecureTransport http.RoundTripper) Context {
 		Transport:         transport,
 		InsecureTransport: insecureTransport,
 		Challenges:        challenge.NewSimpleManager(),
+		Actions:           []string{"pull"},
 	}
 }
 
 type Context struct {
 	Transport         http.RoundTripper
 	InsecureTransport http.RoundTripper
 	Challenges        challenge.Manager
+	Scopes            []auth.Scope
+	Actions           []string
+}
+
+func (c Context) WithScopes(scopes ...auth.Scope) Context {
+	c.Scopes = scopes
+	return c
+}
+
+func (c Context) WithActions(actions ...string) Context {
+	c.Actions = actions
+	return c
 }
 
 func (c Context) WithCredentials(credentials auth.CredentialStore) RepositoryRetriever {
+	return c.WithAuthHandlers(func(rt http.RoundTripper, _ *url.URL, repoName string) []auth.AuthenticationHandler {
+		scopes := make([]auth.Scope, 0, 1+len(c.Scopes))
+		scopes = append(scopes, c.Scopes...)
+		if len(c.Actions) == 0 {
+			scopes = append(scopes, auth.RepositoryScope{Repository: repoName, Actions: []string{"pull"}})
+		} else {
+			scopes = append(scopes, auth.RepositoryScope{Repository: repoName, Actions: c.Actions})
+		}
+		return []auth.AuthenticationHandler{
+			auth.NewTokenHandlerWithOptions(auth.TokenHandlerOptions{
+				Transport:   rt,
+				Credentials: credentials,
+				Scopes:      scopes,
+			}),
+			auth.NewBasicHandler(credentials),
+		}
+	})
+}
+
+type AuthHandlersFunc func(transport http.RoundTripper, registry *url.URL, repoName string) []auth.AuthenticationHandler
+
+func (c Context) WithAuthHandlers(fn AuthHandlersFunc) RepositoryRetriever {
 	return &repositoryRetriever{
 		context:     c,
-		credentials: credentials,
+		credentials: fn,
 
 		pings:    make(map[url.URL]error),
 		redirect: make(map[url.URL]*url.URL),
@@ -68,7 +103,7 @@ func (c Context) WithCredentials(credentials auth.CredentialStore) RepositoryRet
 
 type repositoryRetriever struct {
 	context     Context
-	credentials auth.CredentialStore
+	credentials AuthHandlersFunc
 
 	pings    map[url.URL]error
 	redirect map[url.URL]*url.URL
@@ -111,8 +146,7 @@ func (r *repositoryRetriever) Repository(ctx gocontext.Context, registry *url.UR
 		// TODO: make multiple attempts if the first credential fails
 		auth.NewAuthorizer(
 			r.context.Challenges,
-			auth.NewTokenHandler(t, r.credentials, repoName, "pull"),
-			auth.NewBasicHandler(r.credentials),
+			r.credentials(t, registry, repoName)...,
 		),
 	)
 

pkg/image/importer/credentials.go

@@ -19,6 +19,31 @@ var (
 	emptyKeyring = &credentialprovider.BasicDockerKeyring{}
 )
 
+type refreshTokenKey struct {
+	url     string
+	service string
+}
+
+type refreshTokenStore struct {
+	lock  sync.Mutex
+	store map[refreshTokenKey]string
+}
+
+func (s *refreshTokenStore) RefreshToken(url *url.URL, service string) string {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+	return s.store[refreshTokenKey{url: url.String(), service: service}]
+}
+
+func (s *refreshTokenStore) SetRefreshToken(url *url.URL, service string, token string) {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+	if s.store == nil {
+		s.store = make(map[refreshTokenKey]string)
+	}
+	s.store[refreshTokenKey{url: url.String(), service: service}] = token
+}
+
 type noopCredentialStore struct{}
 
 func (s *noopCredentialStore) Basic(url *url.URL) (string, string) {
@@ -36,7 +61,7 @@ func (s *noopCredentialStore) SetRefreshToken(url *url.URL, service string, toke
 }
 
 func NewBasicCredentials() *BasicCredentials {
-	return &BasicCredentials{}
+	return &BasicCredentials{refreshTokenStore: &refreshTokenStore{}}
 }
 
 type basicForURL struct {
@@ -46,6 +71,7 @@ type basicForURL struct {
 
 type BasicCredentials struct {
 	creds []basicForURL
+	*refreshTokenStore
 }
 
 func (c *BasicCredentials) Add(url *url.URL, username, password string) {
@@ -65,38 +91,34 @@ func (c *BasicCredentials) Basic(url *url.URL) (string, string) {
 	return "", ""
 }
 
-func (c *BasicCredentials) RefreshToken(url *url.URL, service string) string {
-	return ""
-}
-
-func (c *BasicCredentials) SetRefreshToken(url *url.URL, service string, token string) {
-}
-
 func NewLocalCredentials() auth.CredentialStore {
-	return &keyringCredentialStore{credentialprovider.NewDockerKeyring()}
+	return &keyringCredentialStore{
+		DockerKeyring:     credentialprovider.NewDockerKeyring(),
+		refreshTokenStore: &refreshTokenStore{},
+	}
 }
 
 type keyringCredentialStore struct {
 	credentialprovider.DockerKeyring
+	*refreshTokenStore
 }
 
 func (s *keyringCredentialStore) Basic(url *url.URL) (string, string) {
 	return basicCredentialsFromKeyring(s.DockerKeyring, url)
 }
 
-func (s *keyringCredentialStore) RefreshToken(url *url.URL, service string) string {
-	return ""
-}
-
-func (s *keyringCredentialStore) SetRefreshToken(url *url.URL, service string, token string) {
-}
-
 func NewCredentialsForSecrets(secrets []kapiv1.Secret) *SecretCredentialStore {
-	return &SecretCredentialStore{secrets: secrets}
+	return &SecretCredentialStore{
+		secrets:           secrets,
+		refreshTokenStore: &refreshTokenStore{},
+	}
 }
 
 func NewLazyCredentialsForSecrets(secretsFn func() ([]kapiv1.Secret, error)) *SecretCredentialStore {
-	return &SecretCredentialStore{secretsFn: secretsFn}
+	return &SecretCredentialStore{
+		secretsFn:         secretsFn,
+		refreshTokenStore: &refreshTokenStore{},
+	}
 }
 
 type SecretCredentialStore struct {
@@ -105,19 +127,14 @@ type SecretCredentialStore struct {
 	secretsFn func() ([]kapiv1.Secret, error)
 	err       error
 	keyring   credentialprovider.DockerKeyring
+
+	*refreshTokenStore
 }
 
 func (s *SecretCredentialStore) Basic(url *url.URL) (string, string) {
 	return basicCredentialsFromKeyring(s.init(), url)
 }
 
-func (s *SecretCredentialStore) RefreshToken(url *url.URL, service string) string {
-	return ""
-}
-
-func (s *SecretCredentialStore) SetRefreshToken(url *url.URL, service string, token string) {
-}
-
 func (s *SecretCredentialStore) Err() error {
 	s.lock.Lock()
 	defer s.lock.Unlock()
@@ -151,7 +168,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,
@@ -174,6 +197,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)
+			}
+		})
+	}
+}