From cee6281530100c4b1a58321a2c28286a6698964a Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Thu, 5 Sep 2019 14:12:44 -0700 Subject: [PATCH 1/4] Update Keychain interface to accept repo Fixes #495 --- pkg/authn/helper.go | 4 +-- pkg/authn/helper_test.go | 4 +-- pkg/authn/k8schain/k8schain.go | 15 +++++--- pkg/authn/k8schain/k8schain_test.go | 53 ++++++++++++++++++++--------- pkg/authn/keychain.go | 18 ++++++---- pkg/authn/keychain_test.go | 4 +-- pkg/authn/multikeychain.go | 8 ++--- pkg/authn/multikeychain_test.go | 6 ++-- pkg/v1/google/keychain.go | 5 ++- pkg/v1/remote/delete.go | 2 +- pkg/v1/remote/descriptor.go | 2 +- pkg/v1/remote/layer.go | 2 +- pkg/v1/remote/list.go | 2 +- pkg/v1/remote/options.go | 5 ++- pkg/v1/remote/write.go | 6 ++-- 15 files changed, 78 insertions(+), 58 deletions(-) diff --git a/pkg/authn/helper.go b/pkg/authn/helper.go index c8ba4e6e2..2000a163b 100644 --- a/pkg/authn/helper.go +++ b/pkg/authn/helper.go @@ -20,8 +20,6 @@ import ( "fmt" "os/exec" "strings" - - "github.com/google/go-containerregistry/pkg/name" ) // magicNotFoundMessage is the string that the CLI special cases to mean @@ -46,7 +44,7 @@ func (dr *defaultRunner) Run(cmd *exec.Cmd) error { // helper executes the named credential helper against the given domain. type helper struct { name string - domain name.Registry + domain string // We add this layer of indirection to facilitate unit testing. r runner diff --git a/pkg/authn/helper_test.go b/pkg/authn/helper_test.go index 56b58a9f7..74dc57246 100644 --- a/pkg/authn/helper_test.go +++ b/pkg/authn/helper_test.go @@ -18,12 +18,10 @@ import ( "errors" "os/exec" "testing" - - "github.com/google/go-containerregistry/pkg/name" ) var ( - testDomain, _ = name.NewRegistry("foo.dev", name.WeakValidation) + testDomain = "foo.dev" ) // errorRunner implements runner to always return an execution error. diff --git a/pkg/authn/k8schain/k8schain.go b/pkg/authn/k8schain/k8schain.go index d90ac4d1d..b34f7ad5f 100644 --- a/pkg/authn/k8schain/k8schain.go +++ b/pkg/authn/k8schain/k8schain.go @@ -153,10 +153,17 @@ type keychain struct { } // Resolve implements authn.Keychain -func (kc *keychain) Resolve(reg name.Registry) (authn.Authenticator, error) { - // TODO(mattmoor): Lookup expects an image reference and we only have a registry, - // find something better than this. - creds, found := kc.keyring.Lookup(reg.String() + "/foo/bar") +func (kc *keychain) Resolve(target authn.Target) (authn.Authenticator, error) { + var ( + creds []credentialprovider.LazyAuthConfiguration + found bool + ) + if repo, ok := target.(name.Repository); ok { + creds, found = kc.keyring.Lookup(repo.String()) + } else { + // Lookup expects an image reference and we only have a registry. + creds, found = kc.keyring.Lookup(target.RegistryStr() + "/foo/bar") + } if !found || len(creds) < 1 { return authn.Anonymous, nil } diff --git a/pkg/authn/k8schain/k8schain_test.go b/pkg/authn/k8schain/k8schain_test.go index ce0d810cd..cf321c8bc 100644 --- a/pkg/authn/k8schain/k8schain_test.go +++ b/pkg/authn/k8schain/k8schain_test.go @@ -109,6 +109,7 @@ func TestAttachedServiceAccount(t *testing.T) { func TestImagePullSecrets(t *testing.T) { username, password := "foo", "bar" + specificUser, specificPass := "very", "specific" client := fakeclient.NewSimpleClientset(&corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -122,8 +123,9 @@ func TestImagePullSecrets(t *testing.T) { Type: corev1.SecretTypeDockercfg, Data: map[string][]byte{ corev1.DockerConfigKey: []byte( - fmt.Sprintf(`{"fake.registry.io": {"auth": "%s"}}`, - base64.StdEncoding.EncodeToString([]byte(username+":"+password))), + fmt.Sprintf(`{"fake.registry.io": {"auth": "%s"}, "fake.registry.io/more/specific": {"auth": "%s"}}`, + base64.StdEncoding.EncodeToString([]byte(username+":"+password)), + base64.StdEncoding.EncodeToString([]byte(specificUser+":"+specificPass))), ), }, }) @@ -136,24 +138,41 @@ func TestImagePullSecrets(t *testing.T) { t.Fatalf("New() = %v", err) } - reg, err := name.NewRegistry("fake.registry.io", name.WeakValidation) + repo, err := name.NewRepository("fake.registry.io/more/specific", name.WeakValidation) if err != nil { t.Errorf("NewRegistry() = %v", err) } - auth, err := kc.Resolve(reg) - if err != nil { - t.Errorf("Resolve(%v) = %v", reg, err) - } - got, err := auth.Authorization() - if err != nil { - t.Errorf("Authorization() = %v", err) - } - want, err := (&authn.Basic{Username: username, Password: password}).Authorization() - if err != nil { - t.Errorf("Authorization() = %v", err) - } - if got != want { - t.Errorf("Resolve() = %v, want %v", got, want) + for _, tc := range []struct { + name string + auth authn.Authenticator + target authn.Target + }{{ + name: "registry", + auth: &authn.Basic{Username: username, Password: password}, + target: repo.Registry, + }, { + name: "repo", + auth: &authn.Basic{Username: specificUser, Password: specificPass}, + target: repo, + }} { + t.Run(tc.name, func(t *testing.T) { + tc := tc + auth, err := kc.Resolve(tc.target) + if err != nil { + t.Errorf("Resolve(%v) = %v", tc.target, err) + } + got, err := auth.Authorization() + if err != nil { + t.Errorf("Authorization() = %v", err) + } + want, err := tc.auth.Authorization() + if err != nil { + t.Errorf("Authorization() = %v", err) + } + if got != want { + t.Errorf("Resolve() = %v, want %v", got, want) + } + }) } } diff --git a/pkg/authn/keychain.go b/pkg/authn/keychain.go index 8b2ead5db..6eaef96ed 100644 --- a/pkg/authn/keychain.go +++ b/pkg/authn/keychain.go @@ -24,13 +24,17 @@ import ( "runtime" "github.com/google/go-containerregistry/pkg/logs" - "github.com/google/go-containerregistry/pkg/name" ) +type Target interface { + String() string + RegistryStr() string +} + // Keychain is an interface for resolving an image reference to a credential. type Keychain interface { // Resolve looks up the most appropriate credential for the specified registry. - Resolve(name.Registry) (Authenticator, error) + Resolve(Target) (Authenticator, error) } // defaultKeychain implements Keychain with the semantics of the standard Docker @@ -97,7 +101,7 @@ var ( ) // Resolve implements Keychain. -func (dk *defaultKeychain) Resolve(reg name.Registry) (Authenticator, error) { +func (dk *defaultKeychain) Resolve(target Target) (Authenticator, error) { dir, err := configDir() if err != nil { logs.Warn.Printf("Unable to determine config dir: %v", err) @@ -119,21 +123,21 @@ func (dk *defaultKeychain) Resolve(reg name.Registry) (Authenticator, error) { // Per-registry credential helpers take precedence. if cf.CredHelper != nil { for _, form := range domainForms { - if entry, ok := cf.CredHelper[fmt.Sprintf(form, reg.Name())]; ok { - return &helper{name: entry, domain: reg, r: &defaultRunner{}}, nil + if entry, ok := cf.CredHelper[fmt.Sprintf(form, target.RegistryStr())]; ok { + return &helper{name: entry, domain: target.RegistryStr(), r: &defaultRunner{}}, nil } } } // A global credential helper is next in precedence. if cf.CredStore != "" { - return &helper{name: cf.CredStore, domain: reg, r: &defaultRunner{}}, nil + return &helper{name: cf.CredStore, domain: target.RegistryStr(), r: &defaultRunner{}}, nil } // Lastly, the 'auths' section directly contains basic auth entries. if cf.Auths != nil { for _, form := range domainForms { - if entry, ok := cf.Auths[fmt.Sprintf(form, reg.Name())]; ok { + if entry, ok := cf.Auths[fmt.Sprintf(form, target.RegistryStr())]; ok { if entry.Auth != "" { return &auth{entry.Auth}, nil } else if entry.Username != "" { diff --git a/pkg/authn/keychain_test.go b/pkg/authn/keychain_test.go index d0841b816..09f56e45c 100644 --- a/pkg/authn/keychain_test.go +++ b/pkg/authn/keychain_test.go @@ -156,8 +156,8 @@ func checkHelper(t *testing.T) { if help.name != "test" { t.Errorf("Resolve().name; got %v, want \"test\"", help.name) } - if help.domain != testRegistry { - t.Errorf("Resolve().domain; got %v, want %v", help.domain, testRegistry) + if help.domain != testRegistry.RegistryStr() { + t.Errorf("Resolve().domain; got %v, want %v", help.domain, testRegistry.RegistryStr()) } } diff --git a/pkg/authn/multikeychain.go b/pkg/authn/multikeychain.go index 9d7fb3145..a518345c5 100644 --- a/pkg/authn/multikeychain.go +++ b/pkg/authn/multikeychain.go @@ -14,10 +14,6 @@ package authn -import ( - "github.com/google/go-containerregistry/pkg/name" -) - type multiKeychain struct { keychains []Keychain } @@ -31,9 +27,9 @@ func NewMultiKeychain(kcs ...Keychain) Keychain { } // Resolve implements Keychain. -func (mk *multiKeychain) Resolve(reg name.Registry) (Authenticator, error) { +func (mk *multiKeychain) Resolve(target Target) (Authenticator, error) { for _, kc := range mk.keychains { - auth, err := kc.Resolve(reg) + auth, err := kc.Resolve(target) if err != nil { return nil, err } diff --git a/pkg/authn/multikeychain_test.go b/pkg/authn/multikeychain_test.go index 26c5a98f5..d40f695d6 100644 --- a/pkg/authn/multikeychain_test.go +++ b/pkg/authn/multikeychain_test.go @@ -85,13 +85,13 @@ func TestMultiKeychain(t *testing.T) { } } -type fixedKeychain map[name.Registry]Authenticator +type fixedKeychain map[Target]Authenticator var _ Keychain = (fixedKeychain)(nil) // Resolve implements Keychain. -func (fk fixedKeychain) Resolve(reg name.Registry) (Authenticator, error) { - if auth, ok := fk[reg]; ok { +func (fk fixedKeychain) Resolve(target Target) (Authenticator, error) { + if auth, ok := fk[target]; ok { return auth, nil } return Anonymous, nil diff --git a/pkg/v1/google/keychain.go b/pkg/v1/google/keychain.go index d21cfd796..d34b0db99 100644 --- a/pkg/v1/google/keychain.go +++ b/pkg/v1/google/keychain.go @@ -19,7 +19,6 @@ import ( "strings" "github.com/google/go-containerregistry/pkg/authn" - "github.com/google/go-containerregistry/pkg/name" ) // Keychain exports an instance of the google Keychain. @@ -48,9 +47,9 @@ type googleKeychain struct{} // // In general, we don't worry about that here because we expect to use the same // gcloud configuration in the scope of this one process. -func (gk *googleKeychain) Resolve(reg name.Registry) (authn.Authenticator, error) { +func (gk *googleKeychain) Resolve(target authn.Target) (authn.Authenticator, error) { // Only authenticate GCR so it works with authn.NewMultiKeychain to fallback. - if !strings.HasSuffix(reg.String(), "gcr.io") { + if !strings.HasSuffix(target.RegistryStr(), "gcr.io") { return authn.Anonymous, nil } diff --git a/pkg/v1/remote/delete.go b/pkg/v1/remote/delete.go index c034435f9..8e4e99abd 100644 --- a/pkg/v1/remote/delete.go +++ b/pkg/v1/remote/delete.go @@ -26,7 +26,7 @@ import ( // Delete removes the specified image reference from the remote registry. func Delete(ref name.Reference, options ...Option) error { - o, err := makeOptions(ref.Context().Registry, options...) + o, err := makeOptions(ref.Context(), options...) if err != nil { return err } diff --git a/pkg/v1/remote/descriptor.go b/pkg/v1/remote/descriptor.go index 1c8c1214c..0f5a56873 100644 --- a/pkg/v1/remote/descriptor.go +++ b/pkg/v1/remote/descriptor.go @@ -84,7 +84,7 @@ func Get(ref name.Reference, options ...Option) (*Descriptor, error) { // Handle options and fetch the manifest with the acceptable MediaTypes in the // Accept header. func get(ref name.Reference, acceptable []types.MediaType, options ...Option) (*Descriptor, error) { - o, err := makeOptions(ref.Context().Registry, options...) + o, err := makeOptions(ref.Context(), options...) if err != nil { return nil, err } diff --git a/pkg/v1/remote/layer.go b/pkg/v1/remote/layer.go index 915fc5cd4..1e1e331cc 100644 --- a/pkg/v1/remote/layer.go +++ b/pkg/v1/remote/layer.go @@ -58,7 +58,7 @@ func (rl *remoteLayer) MediaType() (types.MediaType, error) { // digest of the blob to be read and the repository portion is the repo where // that blob lives. func Layer(ref name.Digest, options ...Option) (v1.Layer, error) { - o, err := makeOptions(ref.Context().Registry, options...) + o, err := makeOptions(ref.Context(), options...) if err != nil { return nil, err } diff --git a/pkg/v1/remote/list.go b/pkg/v1/remote/list.go index 4cbdc4f19..bf50a797f 100644 --- a/pkg/v1/remote/list.go +++ b/pkg/v1/remote/list.go @@ -32,7 +32,7 @@ type tags struct { // List calls /tags/list for the given repository, returning the list of tags // in the "tags" property. func List(repo name.Repository, options ...Option) ([]string, error) { - o, err := makeOptions(repo.Registry, options...) + o, err := makeOptions(repo, options...) if err != nil { return nil, err } diff --git a/pkg/v1/remote/options.go b/pkg/v1/remote/options.go index c60344840..de42a6533 100644 --- a/pkg/v1/remote/options.go +++ b/pkg/v1/remote/options.go @@ -19,7 +19,6 @@ import ( "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/logs" - "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote/transport" ) @@ -34,7 +33,7 @@ type options struct { platform v1.Platform } -func makeOptions(reg name.Registry, opts ...Option) (*options, error) { +func makeOptions(target authn.Target, opts ...Option) (*options, error) { o := &options{ auth: authn.Anonymous, transport: http.DefaultTransport, @@ -48,7 +47,7 @@ func makeOptions(reg name.Registry, opts ...Option) (*options, error) { } if o.keychain != nil { - auth, err := o.keychain.Resolve(reg) + auth, err := o.keychain.Resolve(target) if err != nil { return nil, err } diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go index 9c2d65293..a64165658 100644 --- a/pkg/v1/remote/write.go +++ b/pkg/v1/remote/write.go @@ -46,7 +46,7 @@ func Write(ref name.Reference, img v1.Image, options ...Option) error { return err } - o, err := makeOptions(ref.Context().Registry, options...) + o, err := makeOptions(ref.Context(), options...) if err != nil { return err } @@ -428,7 +428,7 @@ func WriteIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) error { return err } - o, err := makeOptions(ref.Context().Registry, options...) + o, err := makeOptions(ref.Context(), options...) if err != nil { return err } @@ -484,7 +484,7 @@ func WriteIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) error { // WriteLayer uploads the provided Layer to the specified name.Digest. func WriteLayer(ref name.Digest, layer v1.Layer, options ...Option) error { - o, err := makeOptions(ref.Context().Registry, options...) + o, err := makeOptions(ref.Context(), options...) if err != nil { return err } From 4d213bd4d07a2c02975318f97171e817ed3d60c4 Mon Sep 17 00:00:00 2001 From: jonjohnsonjr Date: Thu, 5 Sep 2019 19:45:36 -0700 Subject: [PATCH 2/4] Update pkg/authn/k8schain/k8schain_test.go Co-Authored-By: Matt Moore --- pkg/authn/k8schain/k8schain_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/authn/k8schain/k8schain_test.go b/pkg/authn/k8schain/k8schain_test.go index cf321c8bc..4cc8862be 100644 --- a/pkg/authn/k8schain/k8schain_test.go +++ b/pkg/authn/k8schain/k8schain_test.go @@ -123,7 +123,7 @@ func TestImagePullSecrets(t *testing.T) { Type: corev1.SecretTypeDockercfg, Data: map[string][]byte{ corev1.DockerConfigKey: []byte( - fmt.Sprintf(`{"fake.registry.io": {"auth": "%s"}, "fake.registry.io/more/specific": {"auth": "%s"}}`, + fmt.Sprintf(`{"fake.registry.io": {"auth": %q}, "fake.registry.io/more/specific": {"auth": %q}}`, base64.StdEncoding.EncodeToString([]byte(username+":"+password)), base64.StdEncoding.EncodeToString([]byte(specificUser+":"+specificPass))), ), From 0fcab34ea1e0306fa7f0385c1313a536d5a34b46 Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Thu, 5 Sep 2019 19:48:47 -0700 Subject: [PATCH 3/4] Address feedback --- pkg/authn/keychain.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/authn/keychain.go b/pkg/authn/keychain.go index 6eaef96ed..1c5ab389d 100644 --- a/pkg/authn/keychain.go +++ b/pkg/authn/keychain.go @@ -26,14 +26,21 @@ import ( "github.com/google/go-containerregistry/pkg/logs" ) +// Target represents a registry or repository that can be authenticated against. type Target interface { + // String returns the full string representation of the target, e.g. + // gcr.io/my-project or just gcr.io. String() string + + // RegistryStr returns just the registry portion of the target, e.g. for + // gcr.io/my-project, this should just return gcr.io. This is needed to + // pull out an appropriate hostname. RegistryStr() string } // Keychain is an interface for resolving an image reference to a credential. type Keychain interface { - // Resolve looks up the most appropriate credential for the specified registry. + // Resolve looks up the most appropriate credential for the specified target. Resolve(Target) (Authenticator, error) } From 8a60c535cfd0e24bdc6c0f6a1f454251990720cd Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Mon, 9 Sep 2019 09:16:36 -0700 Subject: [PATCH 4/4] Target -> Resource --- pkg/authn/k8schain/k8schain.go | 2 +- pkg/authn/k8schain/k8schain_test.go | 2 +- pkg/authn/keychain.go | 8 ++++---- pkg/authn/multikeychain.go | 2 +- pkg/authn/multikeychain_test.go | 4 ++-- pkg/v1/google/keychain.go | 2 +- pkg/v1/remote/options.go | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/authn/k8schain/k8schain.go b/pkg/authn/k8schain/k8schain.go index b34f7ad5f..3409edaf4 100644 --- a/pkg/authn/k8schain/k8schain.go +++ b/pkg/authn/k8schain/k8schain.go @@ -153,7 +153,7 @@ type keychain struct { } // Resolve implements authn.Keychain -func (kc *keychain) Resolve(target authn.Target) (authn.Authenticator, error) { +func (kc *keychain) Resolve(target authn.Resource) (authn.Authenticator, error) { var ( creds []credentialprovider.LazyAuthConfiguration found bool diff --git a/pkg/authn/k8schain/k8schain_test.go b/pkg/authn/k8schain/k8schain_test.go index 4cc8862be..bd6a4627d 100644 --- a/pkg/authn/k8schain/k8schain_test.go +++ b/pkg/authn/k8schain/k8schain_test.go @@ -146,7 +146,7 @@ func TestImagePullSecrets(t *testing.T) { for _, tc := range []struct { name string auth authn.Authenticator - target authn.Target + target authn.Resource }{{ name: "registry", auth: &authn.Basic{Username: username, Password: password}, diff --git a/pkg/authn/keychain.go b/pkg/authn/keychain.go index 1c5ab389d..8c735915b 100644 --- a/pkg/authn/keychain.go +++ b/pkg/authn/keychain.go @@ -26,8 +26,8 @@ import ( "github.com/google/go-containerregistry/pkg/logs" ) -// Target represents a registry or repository that can be authenticated against. -type Target interface { +// Resource represents a registry or repository that can be authenticated against. +type Resource interface { // String returns the full string representation of the target, e.g. // gcr.io/my-project or just gcr.io. String() string @@ -41,7 +41,7 @@ type Target interface { // Keychain is an interface for resolving an image reference to a credential. type Keychain interface { // Resolve looks up the most appropriate credential for the specified target. - Resolve(Target) (Authenticator, error) + Resolve(Resource) (Authenticator, error) } // defaultKeychain implements Keychain with the semantics of the standard Docker @@ -108,7 +108,7 @@ var ( ) // Resolve implements Keychain. -func (dk *defaultKeychain) Resolve(target Target) (Authenticator, error) { +func (dk *defaultKeychain) Resolve(target Resource) (Authenticator, error) { dir, err := configDir() if err != nil { logs.Warn.Printf("Unable to determine config dir: %v", err) diff --git a/pkg/authn/multikeychain.go b/pkg/authn/multikeychain.go index a518345c5..3b1804f5d 100644 --- a/pkg/authn/multikeychain.go +++ b/pkg/authn/multikeychain.go @@ -27,7 +27,7 @@ func NewMultiKeychain(kcs ...Keychain) Keychain { } // Resolve implements Keychain. -func (mk *multiKeychain) Resolve(target Target) (Authenticator, error) { +func (mk *multiKeychain) Resolve(target Resource) (Authenticator, error) { for _, kc := range mk.keychains { auth, err := kc.Resolve(target) if err != nil { diff --git a/pkg/authn/multikeychain_test.go b/pkg/authn/multikeychain_test.go index d40f695d6..3dff0c9db 100644 --- a/pkg/authn/multikeychain_test.go +++ b/pkg/authn/multikeychain_test.go @@ -85,12 +85,12 @@ func TestMultiKeychain(t *testing.T) { } } -type fixedKeychain map[Target]Authenticator +type fixedKeychain map[Resource]Authenticator var _ Keychain = (fixedKeychain)(nil) // Resolve implements Keychain. -func (fk fixedKeychain) Resolve(target Target) (Authenticator, error) { +func (fk fixedKeychain) Resolve(target Resource) (Authenticator, error) { if auth, ok := fk[target]; ok { return auth, nil } diff --git a/pkg/v1/google/keychain.go b/pkg/v1/google/keychain.go index d34b0db99..74a6ab9d7 100644 --- a/pkg/v1/google/keychain.go +++ b/pkg/v1/google/keychain.go @@ -47,7 +47,7 @@ type googleKeychain struct{} // // In general, we don't worry about that here because we expect to use the same // gcloud configuration in the scope of this one process. -func (gk *googleKeychain) Resolve(target authn.Target) (authn.Authenticator, error) { +func (gk *googleKeychain) Resolve(target authn.Resource) (authn.Authenticator, error) { // Only authenticate GCR so it works with authn.NewMultiKeychain to fallback. if !strings.HasSuffix(target.RegistryStr(), "gcr.io") { return authn.Anonymous, nil diff --git a/pkg/v1/remote/options.go b/pkg/v1/remote/options.go index de42a6533..6840b2f6d 100644 --- a/pkg/v1/remote/options.go +++ b/pkg/v1/remote/options.go @@ -33,7 +33,7 @@ type options struct { platform v1.Platform } -func makeOptions(target authn.Target, opts ...Option) (*options, error) { +func makeOptions(target authn.Resource, opts ...Option) (*options, error) { o := &options{ auth: authn.Anonymous, transport: http.DefaultTransport,