diff --git a/pkg/image/apiserver/importer/credentials.go b/pkg/image/apiserver/importer/credentials.go index 9d37c7fe12..3efbc3fd2e 100644 --- a/pkg/image/apiserver/importer/credentials.go +++ b/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 } diff --git a/pkg/image/apiserver/importer/nodecredentials.go b/pkg/image/apiserver/importer/nodecredentials.go new file mode 100644 index 0000000000..a46bd00736 --- /dev/null +++ b/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 +} diff --git a/pkg/image/apiserver/importer/nodecredentials_test.go b/pkg/image/apiserver/importer/nodecredentials_test.go new file mode 100644 index 0000000000..2b830c4e05 --- /dev/null +++ b/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") + } + }) + } +} diff --git a/pkg/image/apiserver/importer/test/config.json b/pkg/image/apiserver/importer/test/config.json new file mode 100644 index 0000000000..06794591a2 --- /dev/null +++ b/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)" + } +} diff --git a/pkg/image/apiserver/importer/unioncredentials.go b/pkg/image/apiserver/importer/unioncredentials.go new file mode 100644 index 0000000000..2fc7a34874 --- /dev/null +++ b/pkg/image/apiserver/importer/unioncredentials.go @@ -0,0 +1,76 @@ +package importer + +import ( + "net/url" + "strings" + + "github.com/openshift/library-go/pkg/image/registryclient" +) + +// CredentialStore is an interface implemented by all credential stores. +type CredentialStore interface { + Basic(*url.URL) (string, string) + Err() error +} + +// NewUnionCredentialStore returns an UnionCredentialStore populated with +// provided credential stores. +func NewUnionCredentialStore(stores ...CredentialStore) *UnionCredentialStore { + return &UnionCredentialStore{ + stores: stores, + RefreshTokenStore: registryclient.NewRefreshTokenStore(), + } +} + +// UnionCredentialStore is a handler that holds multiple internal credential +// stores. It allows callers to aggregate multiple Docker credentials keyrings. +type UnionCredentialStore struct { + stores []CredentialStore + registryclient.RefreshTokenStore +} + +// Basic looks for BasicAuth information for a given URL using the internal +// group of credential stores. Internal stores are sequentially queried and +// this function returns as soon as the first match happens. +func (u *UnionCredentialStore) Basic(target *url.URL) (string, string) { + var user string + var pass string + + for _, s := range u.stores { + user, pass = s.Basic(target) + if len(user) > 0 || len(pass) > 0 { + return user, pass + } + } + + return user, pass +} + +// Err returns all errors reported by internal credential stores. +func (u *UnionCredentialStore) Err() error { + multierr := &MultiError{} + for _, s := range u.stores { + if err := s.Err(); err != nil { + multierr.Append(err) + } + } + if len(multierr.errors) == 0 { + return nil + } + return multierr +} + +// MultiError is a wrap for multiple errors. +type MultiError struct { + errors []string +} + +// Append adds a new error. +func (m *MultiError) Append(e error) { + m.errors = append(m.errors, e.Error()) +} + +// Error exists to comply with golang's error interface. +func (m *MultiError) Error() string { + return strings.Join(m.errors, " ") +} diff --git a/pkg/image/apiserver/importer/unioncredentials_test.go b/pkg/image/apiserver/importer/unioncredentials_test.go new file mode 100644 index 0000000000..9f5684ce32 --- /dev/null +++ b/pkg/image/apiserver/importer/unioncredentials_test.go @@ -0,0 +1,134 @@ +package importer + +import ( + "fmt" + "net/url" + "testing" +) + +type StoreMock struct { + keyring map[string][]string + err error +} + +func (s *StoreMock) Basic(target *url.URL) (string, string) { + if s.keyring == nil { + return "", "" + } + + dat, ok := s.keyring[target.Host] + if !ok { + return "", "" + } + return dat[0], dat[1] +} + +func (s *StoreMock) Err() error { + return s.err +} + +func TestUnionCredentialStoreBasic(t *testing.T) { + for _, tt := range []struct { + name string + stores []CredentialStore + url *url.URL + user string + pass string + errExpected bool + }{ + { + name: "empty stores", + stores: []CredentialStore{}, + url: nil, + }, + { + name: "not found", + stores: []CredentialStore{ + &StoreMock{}, + &StoreMock{}, + }, + url: nil, + }, + { + name: "found on first store", + stores: []CredentialStore{ + &StoreMock{ + keyring: map[string][]string{ + "registry0": { + "user0", + "pass0", + }, + }, + }, + &StoreMock{ + keyring: map[string][]string{ + "registry0": { + "user1", + "pass1", + }, + }, + }, + }, + url: &url.URL{ + Host: "registry0", + }, + user: "user0", + pass: "pass0", + }, + { + name: "found on second store", + stores: []CredentialStore{ + &StoreMock{ + keyring: map[string][]string{ + "registry0": { + "user0", + "pass0", + }, + }, + }, + &StoreMock{ + keyring: map[string][]string{ + "registry1": { + "user1", + "pass1", + }, + }, + }, + }, + url: &url.URL{ + Host: "registry1", + }, + user: "user1", + pass: "pass1", + }, + { + name: "error", + stores: []CredentialStore{ + &StoreMock{ + err: fmt.Errorf("error"), + }, + }, + url: &url.URL{ + Host: "registry1", + }, + user: "", + pass: "", + errExpected: true, + }, + } { + t.Run(tt.name, func(t *testing.T) { + store := NewUnionCredentialStore(tt.stores...) + user, pass := store.Basic(tt.url) + if user != tt.user || pass != tt.pass { + t.Error("invalid user/pass pair returned") + } + err := store.Err() + if tt.errExpected && err == nil { + t.Error("expecting error, nil received") + } + if !tt.errExpected && err != nil { + t.Error("unexpected error returned") + } + }) + } +} diff --git a/pkg/image/apiserver/registry/imagestreamimport/rest.go b/pkg/image/apiserver/registry/imagestreamimport/rest.go index f316fb21a2..6c1f61bfb8 100644 --- a/pkg/image/apiserver/registry/imagestreamimport/rest.go +++ b/pkg/image/apiserver/registry/imagestreamimport/rest.go @@ -216,14 +216,19 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation v2regConf.Registries[i].Prefix = reg.Location } - // only load secrets if we need them - credentials := importer.NewLazyCredentialsForSecrets(func() ([]corev1.Secret, error) { - secrets, err := r.isV1Client.ImageStreams(namespace).Secrets(isi.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - return secrets.Items, nil - }) + credentials := importer.NewUnionCredentialStore( + importer.NewLazyCredentialsForSecrets(func() ([]corev1.Secret, error) { + secrets, err := r.isV1Client.ImageStreams(namespace).Secrets( + isi.Name, metav1.GetOptions{}, + ) + if err != nil { + return nil, err + } + return secrets.Items, nil + }), + importer.NewNodeCredentialStore(), + ) + importCtx := registryclient.NewContext(r.transport, r.insecureTransport).WithCredentials(credentials) imports := r.importFn(importCtx, v2regConf) if err := imports.Import(ctx.(gocontext.Context), isi, stream); err != nil {