DEVEXP-424: Leveraging node pull secrets during image stream import

pkg/image/apiserver/importer/credentials.go

@@ -5,126 +5,190 @@ import (
 	"strings"
 	"sync"
 
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/klog"
-
-	kapiv1 "k8s.io/api/core/v1"
 	"k8s.io/kubernetes/pkg/credentialprovider"
-	credentialprovidersecrets "k8s.io/kubernetes/pkg/credentialprovider/secrets"
-
-	"github.com/openshift/library-go/pkg/image/registryclient"
+	"k8s.io/kubernetes/pkg/credentialprovider/secrets"
 )
 
 var (
 	emptyKeyring = &credentialprovider.BasicDockerKeyring{}
 )
 
-func NewCredentialsForSecrets(secrets []kapiv1.Secret) *SecretCredentialStore {
+// secretsRetriever is a function that returns a list of kubernetes secrets.
+type secretsRetriever func() ([]corev1.Secret, error)
+
+// NewCredentialsForSecrets returns a credential store populated with a list
+// of kubernetes secrets. Secrets are filtered as SecretCredentialStore uses
+// only the ones containing docker credentials.
+func NewCredentialsForSecrets(secrets []corev1.Secret) *SecretCredentialStore {
 	return &SecretCredentialStore{
-		secrets:           secrets,
-		RefreshTokenStore: registryclient.NewRefreshTokenStore(),
+		secrets: secrets,
 	}
 }
 
-func NewLazyCredentialsForSecrets(secretsFn func() ([]kapiv1.Secret, error)) *SecretCredentialStore {
+// NewLazyCredentialsForSecrets returns a credential store populated with the
+// return of fn(). The return of fn() is filtered as SecretCredentialStore uses
+// only secrets that contain docker credentials.
+func NewLazyCredentialsForSecrets(fn secretsRetriever) *SecretCredentialStore {
 	return &SecretCredentialStore{
-		secretsFn:         secretsFn,
-		RefreshTokenStore: registryclient.NewRefreshTokenStore(),
+		secretsFn: fn,
 	}
 }
 
+// SecretCredentialStore holds docker credentials. It uses a list of secrets
+// from where it extracts docker credentials, allowing callers to retrieve
+// BasicAuth information by URL.
 type SecretCredentialStore struct {
 	lock      sync.Mutex
-	secrets   []kapiv1.Secret
-	secretsFn func() ([]kapiv1.Secret, error)
+	secrets   []corev1.Secret
+	secretsFn secretsRetriever
 	err       error
 	keyring   credentialprovider.DockerKeyring
-
-	registryclient.RefreshTokenStore
 }
 
+// Basic returns BasicAuth information for the given url (user and password).
+// If url does not exist on SecretCredentialStore's internal keyring empty
+// strings are returned.
 func (s *SecretCredentialStore) Basic(url *url.URL) (string, string) {
-	return basicCredentialsFromKeyring(s.init(), url)
+	s.init()
+	return basicCredentialsFromKeyring(s.keyring, url)
 }
 
+// Err returns SecretCredentialStore's internal error.
 func (s *SecretCredentialStore) Err() error {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 	return s.err
 }
 
-func (s *SecretCredentialStore) init() credentialprovider.DockerKeyring {
+// init runs only once and is reponsible for loading the internal keyring with
+// Secrets data (if a secretsRetriever function was specified). This function
+// initializes the internal keyring. In case of errors, internal err is set.
+func (s *SecretCredentialStore) init() {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 	if s.keyring != nil {
-		return s.keyring
+		return
 	}
 
 	// lazily load the secrets
-	if s.secrets == nil {
-		if s.secretsFn != nil {
-			s.secrets, s.err = s.secretsFn()
-		}
+	if s.secrets == nil && s.secretsFn != nil {
+		s.secrets, s.err = s.secretsFn()
 	}
 
-	// TODO: need a version of this that is best effort secret - otherwise one error blocks all secrets
-	keyring, err := credentialprovidersecrets.MakeDockerKeyring(s.secrets, emptyKeyring)
+	// TODO: need a version of this that is best effort secret - otherwise
+	// one error blocks all secrets
+	keyring, err := secrets.MakeDockerKeyring(s.secrets, emptyKeyring)
 	if err != nil {
 		klog.V(5).Infof("Loading keyring failed for credential store: %v", err)
 		s.err = err
 		keyring = emptyKeyring
 	}
 	s.keyring = keyring
-	return keyring
 }
 
+// basicCredentialsFromKeyring extract basicAuth information from provided
+// keyring. If keyring does not contain information for the provided URL, empty
+// strings are returned instead.
 func basicCredentialsFromKeyring(keyring credentialprovider.DockerKeyring, target *url.URL) (string, string) {
-	// TODO: compare this logic to Docker authConfig in v2 configuration
-	var value string
+	regURL := getURLForLookup(target)
+	if configs, found := keyring.Lookup(regURL); found {
+		klog.V(5).Infof(
+			"Found secret to match %s (%s): %s",
+			target, regURL, configs[0].ServerAddress,
+		)
+		return configs[0].Username, configs[0].Password
+	}
+
+	// do a special case check for docker.io to match historical lookups
+	// when we respond to a challenge
+	if regURL == "auth.docker.io/token" {
+		klog.V(5).Infof(
+			"Being asked for %s (%s), trying %s, legacy behavior",
+			target, regURL, "index.docker.io/v1",
+		)
+		return basicCredentialsFromKeyring(
+			keyring, &url.URL{Host: "index.docker.io", Path: "/v1"},
+		)
+	}
+
+	// docker 1.9 saves 'docker.io' in config in f23, see
+	// https://bugzilla.redhat.com/show_bug.cgi?id=1309739
+	if regURL == "index.docker.io" {
+		klog.V(5).Infof(
+			"Being asked for %s (%s), trying %s, legacy behavior",
+			target, regURL, "docker.io",
+		)
+		return basicCredentialsFromKeyring(
+			keyring, &url.URL{Host: "docker.io"},
+		)
+	}
+
+	// try removing the canonical ports.
+	if hasCanonicalPort(target) {
+		host := strings.SplitN(target.Host, ":", 2)[0]
+		klog.V(5).Infof(
+			"Being asked for %s (%s), trying %s without port",
+			target, regURL, host,
+		)
+		return basicCredentialsFromKeyring(
+			keyring,
+			&url.URL{
+				Scheme: target.Scheme,
+				Host:   host,
+				Path:   target.Path,
+			},
+		)
+	}
+
+	klog.V(5).Infof("Unable to find a secret to match %s (%s)",
+		target, regURL,
+	)
+	return "", ""
+}
+
+// getURLForLookup returns the URL we should use when looking for credentials
+// on a keyring.
+func getURLForLookup(target *url.URL) string {
+	var res string
+	if target == nil {
+		return res
+	}
+
 	if len(target.Scheme) == 0 || target.Scheme == "https" {
-		value = target.Host + target.Path
+		res = target.Host + target.Path
 	} else {
 		// always require an explicit port to look up HTTP credentials
-		if !strings.Contains(target.Host, ":") {
-			value = target.Host + ":80" + target.Path
+		if strings.Contains(target.Host, ":") {
+			res = target.Host + target.Path
 		} else {
-			value = target.Host + target.Path
+			res = target.Host + ":80" + target.Path
 		}
 	}
 
-	// Lookup(...) expects an image (not a URL path).
-	// The keyring strips /v1/ and /v2/ version prefixes,
-	// so we should also when selecting a valid auth for a URL.
+	// Lookup(...) expects an image (not a URL path). The keyring strips
+	// /v1/ and /v2/ version prefixes so we should do the same when
+	// selecting a valid auth for a URL.
 	pathWithSlash := target.Path + "/"
 	if strings.HasPrefix(pathWithSlash, "/v1/") || strings.HasPrefix(pathWithSlash, "/v2/") {
-		value = target.Host + target.Path[3:]
+		res = target.Host + target.Path[3:]
 	}
 
-	configs, found := keyring.Lookup(value)
-
-	if !found || len(configs) == 0 {
-		// do a special case check for docker.io to match historical lookups when we respond to a challenge
-		if value == "auth.docker.io/token" {
-			klog.V(5).Infof("Being asked for %s (%s), trying %s for legacy behavior", target, value, "index.docker.io/v1")
-			return basicCredentialsFromKeyring(keyring, &url.URL{Host: "index.docker.io", Path: "/v1"})
-		}
-		// docker 1.9 saves 'docker.io' in config in f23, see https://bugzilla.redhat.com/show_bug.cgi?id=1309739
-		if value == "index.docker.io" {
-			klog.V(5).Infof("Being asked for %s (%s), trying %s for legacy behavior", target, value, "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]
-			klog.V(5).Infof("Being asked for %s (%s), trying %s without port", target, value, host)
-
-			return basicCredentialsFromKeyring(keyring, &url.URL{Scheme: target.Scheme, Host: host, Path: target.Path})
-		}
+	return res
+}
 
-		klog.V(5).Infof("Unable to find a secret to match %s (%s)", target, value)
-		return "", ""
+// hasCanonicalPort returns if port is specified on the url and is the default
+// port for the protocol.
+func hasCanonicalPort(target *url.URL) bool {
+	switch {
+	case target == nil:
+		return false
+	case strings.HasSuffix(target.Host, ":443") && target.Scheme == "https":
+		return true
+	case strings.HasSuffix(target.Host, ":80") && target.Scheme == "http":
+		return true
+	default:
+		return false
 	}
-	klog.V(5).Infof("Found secret to match %s (%s): %s", target, value, configs[0].ServerAddress)
-	return configs[0].Username, configs[0].Password
 }

pkg/image/apiserver/importer/nodecredentials.go

@@ -0,0 +1,51 @@
+package importer
+
+import (
+	"net/url"
+
+	"k8s.io/kubernetes/pkg/credentialprovider"
+)
+
+var (
+	// NodeCredentialsDir points to the directory from where to read node
+	// Docker credentials.
+	NodeCredentialsDir = "/var/lib/kubelet/"
+)
+
+// NewNodeCredentialStore returns a credential store holding the content of
+// node's Docker pull secrets. If something wrong happens during the object
+// initialization an internal error is set.
+func NewNodeCredentialStore() *NodeCredentialStore {
+	keyring := &credentialprovider.BasicDockerKeyring{}
+
+	config, err := credentialprovider.ReadDockerConfigJSONFile(
+		[]string{NodeCredentialsDir},
+	)
+	if err == nil {
+		keyring.Add(config)
+	}
+
+	return &NodeCredentialStore{
+		err:     err,
+		keyring: keyring,
+	}
+}
+
+// NodeCredentialStore holds node's Docker pull secrets in an internal
+// keyring. It allows callers to query for BasicAuth information by registry
+// URL.
+type NodeCredentialStore struct {
+	keyring credentialprovider.DockerKeyring
+	err     error
+}
+
+// Basic returns BasicAuth information for the given url. If keyring does not
+// have credentials for the url, empty strings are returned.
+func (n *NodeCredentialStore) Basic(url *url.URL) (string, string) {
+	return basicCredentialsFromKeyring(n.keyring, url)
+}
+
+// Err returns NodeCredentialStore's internal error.
+func (n *NodeCredentialStore) Err() error {
+	return n.err
+}

pkg/image/apiserver/importer/nodecredentials_test.go

@@ -0,0 +1,62 @@
+package importer
+
+import (
+	"fmt"
+	"net/url"
+	"os"
+	"testing"
+)
+
+func TestNewNodeCredentialStore(t *testing.T) {
+	store := NewNodeCredentialStore()
+	if store.Err() == nil {
+		t.Error("able to create with invalid docker credentials path")
+	}
+}
+
+func TestBasic(t *testing.T) {
+	dir, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	oldDir := NodeCredentialsDir
+	NodeCredentialsDir = fmt.Sprintf("%s/test/", dir)
+	store := NewNodeCredentialStore()
+	NodeCredentialsDir = oldDir
+
+	if store.Err() != nil {
+		t.Fatalf("unexpected credentials store error: %v", err)
+	}
+
+	for _, tt := range []struct {
+		name string
+		url  *url.URL
+		user string
+		pass string
+	}{
+		{
+			name: "valid registry",
+			url: &url.URL{
+				Host: "registry0.redhat.io",
+			},
+			user: "registry0",
+			pass: "registry0",
+		},
+		{
+			name: "invalid registry",
+			url: &url.URL{
+				Host: "invalidregistry.redhat.io",
+			},
+		},
+		{
+			name: "nil url",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			user, pass := store.Basic(tt.url)
+			if user != tt.user || pass != tt.pass {
+				t.Error("invalid user/pass pair")
+			}
+		})
+	}
+}

pkg/image/apiserver/importer/test/config.json

@@ -0,0 +1,13 @@
+{
+	"auths": {
+		"registry0.redhat.io": {
+			"auth": "cmVnaXN0cnkwOnJlZ2lzdHJ5MA=="
+		},
+		"registry1.redhat.io": {
+			"auth": "cmVnaXN0cnkxOnJlZ2lzdHJ5MQ=="
+		}
+	},
+	"HttpHeaders": {
+		"User-Agent": "Docker-Client/19.03.5 (linux)"
+	}
+}