From 27a6ad623ea9926c58e2bddb7158e9e5278a7dc4 Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Thu, 20 Apr 2023 10:26:10 -0400 Subject: [PATCH] Revert "authn: Add NewConfigKeychain to load a config from explicit path (#1603)" (#1664) This reverts commit 1cb7e133961581d6e0f5564e187b8f4dbf33f9d5. --- pkg/authn/keychain.go | 109 +++++++++---------------------------- pkg/authn/keychain_test.go | 41 ++------------ 2 files changed, 31 insertions(+), 119 deletions(-) diff --git a/pkg/authn/keychain.go b/pkg/authn/keychain.go index 2c92e0456..4e32500cd 100644 --- a/pkg/authn/keychain.go +++ b/pkg/authn/keychain.go @@ -48,27 +48,11 @@ type Keychain interface { // defaultKeychain implements Keychain with the semantics of the standard Docker // credential keychain. type defaultKeychain struct { - once sync.Once - cfg types.AuthConfig - - configFilePath string + mu sync.Mutex } var ( - // DefaultKeychain implements Keychain by interpreting the Docker config file. - // This matches the behavior of tools like `docker` and `podman`. - // - // This keychain looks for credentials configured in a few places, in order: - // - // 1. $HOME/.docker/config.json - // 2. $DOCKER_CONFIG/config.json - // 3. $XDG_RUNTIME_DIR/containers/auth.json (for compatibility with Podman) - // - // If a config file is found and can be parsed, Resolve will return credentials - // configured by the file for the given registry. - // - // If no config file is found, Resolve returns Anonymous. - // If a config file is found but can't be parsed, Resolve returns an error. + // DefaultKeychain implements Keychain by interpreting the docker config file. DefaultKeychain = RefreshingKeychain(&defaultKeychain{}, 5*time.Minute) ) @@ -78,16 +62,11 @@ const ( DefaultAuthKey = "https://" + name.DefaultRegistry + "/v1/" ) -// NewConfigKeychain implements Keychain by interpreting the Docker config file -// at the specified file path. -// -// It acts like DefaultKeychain except that the exact path of the file can be specified, -// instead of being dependent on environment variables and conventional file names. -func NewConfigKeychain(filename string) Keychain { - return &defaultKeychain{configFilePath: filename} -} +// Resolve implements Keychain. +func (dk *defaultKeychain) Resolve(target Resource) (Authenticator, error) { + dk.mu.Lock() + defer dk.mu.Unlock() -func getDefaultConfigFile() (*configfile.ConfigFile, error) { // Podman users may have their container registry auth configured in a // different location, that Docker packages aren't aware of. // If the Docker config file isn't found, we'll fallback to look where @@ -120,7 +99,7 @@ func getDefaultConfigFile() (*configfile.ConfigFile, error) { } else { f, err := os.Open(filepath.Join(os.Getenv("XDG_RUNTIME_DIR"), "containers/auth.json")) if err != nil { - return nil, nil + return Anonymous, nil } defer f.Close() cf, err = config.LoadFromReader(f) @@ -128,65 +107,31 @@ func getDefaultConfigFile() (*configfile.ConfigFile, error) { return nil, err } } - return cf, nil -} -func (dk *defaultKeychain) Resolve(target Resource) (Authenticator, error) { - var err error - var empty types.AuthConfig - dk.once.Do(func() { - var cf *configfile.ConfigFile - if dk.configFilePath == "" { - cf, err = getDefaultConfigFile() - if err != nil { - return - } - if cf == nil { - dk.cfg = empty - return - } - } else { - var f *os.File - f, err = os.Open(dk.configFilePath) - if err != nil { - return - } - defer f.Close() - cf, err = config.LoadFromReader(f) - if err != nil { - return - } + // See: + // https://github.com/google/ko/issues/90 + // https://github.com/moby/moby/blob/fc01c2b481097a6057bec3cd1ab2d7b4488c50c4/registry/config.go#L397-L404 + var cfg, empty types.AuthConfig + for _, key := range []string{ + target.String(), + target.RegistryStr(), + } { + if key == name.DefaultRegistry { + key = DefaultAuthKey } - // See: - // https://github.com/google/ko/issues/90 - // https://github.com/moby/moby/blob/fc01c2b481097a6057bec3cd1ab2d7b4488c50c4/registry/config.go#L397-L404 - for _, key := range []string{ - target.String(), - target.RegistryStr(), - } { - if key == name.DefaultRegistry { - key = DefaultAuthKey - } - - dk.cfg, err = cf.GetAuthConfig(key) - if err != nil { - return - } - // cf.GetAuthConfig automatically sets the ServerAddress attribute. Since - // we don't make use of it, clear the value for a proper "is-empty" test. - // See: https://github.com/google/go-containerregistry/issues/1510 - dk.cfg.ServerAddress = "" - if dk.cfg != empty { - break - } + cfg, err = cf.GetAuthConfig(key) + if err != nil { + return nil, err + } + // cf.GetAuthConfig automatically sets the ServerAddress attribute. Since + // we don't make use of it, clear the value for a proper "is-empty" test. + // See: https://github.com/google/go-containerregistry/issues/1510 + cfg.ServerAddress = "" + if cfg != empty { + break } - }) - if err != nil { - return nil, err } - - cfg := dk.cfg if cfg == empty { return Anonymous, nil } diff --git a/pkg/authn/keychain_test.go b/pkg/authn/keychain_test.go index ec2c3b745..f983a4dec 100644 --- a/pkg/authn/keychain_test.go +++ b/pkg/authn/keychain_test.go @@ -113,7 +113,7 @@ func TestPodmanConfig(t *testing.T) { // At first, $DOCKER_CONFIG is unset and $HOME/.docker/config.json isn't // found, but Podman auth is configured. This should return Podman's // auth. - auth, err := NewConfigKeychain("").Resolve(testRegistry) + auth, err := DefaultKeychain.Resolve(testRegistry) if err != nil { t.Fatalf("Resolve() = %v", err) } @@ -140,7 +140,7 @@ func TestPodmanConfig(t *testing.T) { t.Fatalf("write %q: %v", cfg, err) } defer func() { os.Remove(cfg) }() - auth, err = NewConfigKeychain("").Resolve(testRegistry) + auth, err = DefaultKeychain.Resolve(testRegistry) if err != nil { t.Fatalf("Resolve() = %v", err) } @@ -164,7 +164,7 @@ func TestPodmanConfig(t *testing.T) { cd := setupConfigFile(t, content) defer os.RemoveAll(filepath.Dir(cd)) - auth, err = NewConfigKeychain("").Resolve(testRegistry) + auth, err = DefaultKeychain.Resolve(testRegistry) if err != nil { t.Fatalf("Resolve() = %v", err) } @@ -181,39 +181,6 @@ func TestPodmanConfig(t *testing.T) { } } -func TestAuthConfigPath(t *testing.T) { - tmpdir := os.Getenv("TEST_TMPDIR") - if tmpdir == "" { - tmpdir = t.TempDir() - } - fresh++ - p := filepath.Join(tmpdir, fmt.Sprintf("%d", fresh)) - if err := os.MkdirAll(filepath.Join(p, "custom"), 0777); err != nil { - t.Fatalf("mkdir %s/custom: %v", p, err) - } - cfg := filepath.Join(p, "cfg.xml") - content := fmt.Sprintf(`{"auths": {"test.io": {"auth": %q}}}`, encode("foo", "bar")) - if err := os.WriteFile(cfg, []byte(content), 0600); err != nil { - t.Fatalf("write %q: %v", cfg, err) - } - - auth, err := NewConfigKeychain(cfg).Resolve(testRegistry) - if err != nil { - t.Fatalf("Resolve() = %v", err) - } - got, err := auth.Authorization() - if err != nil { - t.Fatal(err) - } - want := &AuthConfig{ - Username: "foo", - Password: "bar", - } - if !reflect.DeepEqual(got, want) { - t.Errorf("got %+v, want %+v", got, want) - } -} - func encode(user, pass string) string { delimited := fmt.Sprintf("%s:%s", user, pass) return base64.StdEncoding.EncodeToString([]byte(delimited)) @@ -310,7 +277,7 @@ func TestVariousPaths(t *testing.T) { // For some reason, these tempdirs don't get cleaned up. defer os.RemoveAll(filepath.Dir(cd)) - auth, err := NewConfigKeychain("").Resolve(test.target) + auth, err := DefaultKeychain.Resolve(test.target) if test.wantErr { if err == nil { t.Fatal("wanted err, got nil")