Revert "authn: Add NewConfigKeychain to load a config from explicit p…

…ath (#1603)" (#1664) This reverts commit 1cb7e13.
google · Apr 20, 2023 · 27a6ad6 · 27a6ad6
1 parent 217318c
commit 27a6ad6
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 119 deletions.
diff --git 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,73 +99,39 @@ 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)
 		if err != nil {
 			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
@@ -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")