fix(credentials): use the host's credential helper for a namespaced key (#1457)

* fix(credentials): use the host's credential helper for a namespaced key

DynamicStore picked the backing store by an exact credHelpers lookup, so a
namespaced key such as example.com/team/app never matched the helper
configured for example.com. Put then failed with plaintext puts disabled,
or wrote the secret in cleartext to config.json with them enabled. Walk up
the key's path to the nearest configured helper instead.

Fixes #1453

Signed-off-by: Eljees <3.14hell@gmail.com>

* docs(credentials): describe helper selection for namespaced keys

Co-authored-by: Terry Howe <terrylhowe@gmail.com>

Signed-off-by: Eljees <3.14hell@gmail.com>

* fix(credentials): treat an inherited helper's Get error as a miss

A helper found by walking up to a parent namespace was never configured for the key it is probed with. A helper that reports an unknown server URL with its own message would otherwise fail the first iteration of the namespace walk instead of letting it reach the host.

Co-authored-by: Terry Howe <terrylhowe@gmail.com>

Signed-off-by: Eljees <3.14hell@gmail.com>

---------

Signed-off-by: Eljees <3.14hell@gmail.com>
Co-authored-by: Terry Howe <terrylhowe@gmail.com>

Author: Eljees

registry/remote/credentials/store.go

@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/oras-project/oras-go/v3/registry/remote/internal/configfile"
 )
@@ -116,8 +117,10 @@ type StoreOptions struct {
 	// containers-auth.json (Podman/Buildah). When false (default), exact
 	// hostname matching is used, as in Docker config.json.
 	//
-	// This only affects the plaintext file store; credential helpers and
-	// native stores are always keyed by the exact server address.
+	// This only affects credential lookup in the plaintext file store. A
+	// credential helper is always selected by nearest configured parent, and
+	// the selected helper and native stores are always keyed by the exact
+	// server address, regardless of this option.
 	//
 	// It also makes the store authoritative for namespace matching, reported
 	// through [NamespaceMatcher]: a key that is present but holds no
@@ -179,6 +182,9 @@ func NewStoreFromDocker(opt StoreOptions) (*DynamicStore, error) {
 }
 
 // Get retrieves credentials from the store for the given server address.
+//
+// Callers that walk namespaces call this once per path segment, so a helper
+// inherited from a parent namespace may be executed several times per lookup.
 func (ds *DynamicStore) Get(ctx context.Context, serverAddress string) (Credential, error) {
 	return ds.getStore(serverAddress).Get(ctx, serverAddress)
 }
@@ -231,23 +237,43 @@ func (ds *DynamicStore) ConfigPath() string {
 }
 
 // getHelperSuffix returns the credential helper suffix for the given server
-// address.
-func (ds *DynamicStore) getHelperSuffix(serverAddress string) string {
-	// 1. Look for a server-specific credential helper first
-	if helper := ds.config.GetCredentialHelper(serverAddress); helper != "" {
-		return helper
+// address, and whether it was configured for that address itself rather than
+// inherited from a parent namespace.
+//
+// "credHelpers" is keyed by registry host, so a namespaced address
+// ("host/path") resolves to the helper configured for its nearest parent,
+// down to the bare host. The resolved helper is queried with the full address,
+// path included. Note that containers/image queries the helper with the host
+// alone, so a credential stored here under a namespaced key is not visible to
+// Podman or the Docker CLI.
+func (ds *DynamicStore) getHelperSuffix(serverAddress string) (suffix string, exact bool) {
+	// 1. Look for a server-specific credential helper, then for the helper of
+	// each parent namespace.
+	key := serverAddress
+	for {
+		if helper := ds.config.GetCredentialHelper(key); helper != "" {
+			return helper, key == serverAddress
+		}
+		i := strings.LastIndex(key, "/")
+		if i <= 0 {
+			break
+		}
+		key = key[:i]
 	}
 	// 2. Then look for the configured native store
 	if credsStore := ds.config.CredentialsStore(); credsStore != "" {
-		return credsStore
+		return credsStore, true
 	}
 	// 3. Use the detected default store
-	return ds.detectedCredsStore
+	return ds.detectedCredsStore, true
 }
 
 // getStore returns a store for the given server address.
 func (ds *DynamicStore) getStore(serverAddress string) Store {
-	if helper := ds.getHelperSuffix(serverAddress); helper != "" {
+	if helper, exact := ds.getHelperSuffix(serverAddress); helper != "" {
+		if !exact {
+			return inheritedHelperStore{NewNativeStore(helper)}
+		}
 		return NewNativeStore(helper)
 	}
 
@@ -257,6 +283,23 @@ func (ds *DynamicStore) getStore(serverAddress string) Store {
 	return fs
 }
 
+// inheritedHelperStore is a credential helper configured for a parent
+// namespace. Get probes it with a key it was never configured for, so an error
+// there is a miss rather than a failure.
+type inheritedHelperStore struct {
+	Store
+}
+
+// Get retrieves credentials from the helper, reporting a failed probe as a miss
+// so that the caller falls back to the parent namespace.
+func (s inheritedHelperStore) Get(ctx context.Context, serverAddress string) (Credential, error) {
+	cred, err := s.Store.Get(ctx, serverAddress)
+	if err != nil {
+		return EmptyCredential, nil
+	}
+	return cred, nil
+}
+
 // getDockerConfigPath returns the path to the default docker config file.
 func getDockerConfigPath() (string, error) {
 	// first try the environment variable

registry/remote/credentials/store_test.go

@@ -629,14 +629,44 @@ func Test_DynamicStore_getHelperSuffix(t *testing.T) {
 			serverAddress: "whatever.example.com",
 			want:          "teststore",
 		},
+		{
+			name:          "Namespaced address uses the cred helper of its host",
+			configPath:    "testdata/credHelpers_config.json",
+			serverAddress: "registry1.example.com/team/app",
+			want:          "registry1-helper",
+		},
+		{
+			name:          "Namespaced address with an empty cred helper for its host",
+			configPath:    "testdata/credHelpers_config.json",
+			serverAddress: "registry3.example.com/team/app",
+			want:          "",
+		},
+		{
+			name:          "Namespaced address without a cred helper falls back to creds store",
+			configPath:    "testdata/credsStore_config.json",
+			serverAddress: "whatever.example.com/team/app",
+			want:          "teststore",
+		},
+		{
+			name:          "Namespaced address prefers its host's cred helper over creds store",
+			configPath:    "testdata/credsStore_config.json",
+			serverAddress: "test.example.com/team/app",
+			want:          "test-helper",
+		},
+		{
+			name:          "Host is not matched by prefix",
+			configPath:    "testdata/credHelpers_config.json",
+			serverAddress: "registry1.example.com.evil/team",
+			want:          "",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ds, err := NewStore(tt.configPath, StoreOptions{})
 			if err != nil {
 				t.Fatal("NewStore() error =", err)
 			}
-			if got := ds.getHelperSuffix(tt.serverAddress); got != tt.want {
+			if got, _ := ds.getHelperSuffix(tt.serverAddress); got != tt.want {
 				t.Errorf("DynamicStore.getHelperSuffix() = %v, want %v", got, tt.want)
 			}
 		})
@@ -682,6 +712,11 @@ func Test_DynamicStore_getStore_nativeStore(t *testing.T) {
 			configPath:    "testdata/credsStore_config.json",
 			serverAddress: "whaterver.example.com",
 		},
+		{
+			name:          "Cred helper configured for the host of a namespaced address",
+			configPath:    "testdata/credHelpers_config.json",
+			serverAddress: "registry1.example.com/team/app",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -690,6 +725,9 @@ func Test_DynamicStore_getStore_nativeStore(t *testing.T) {
 				t.Fatal("NewStore() error =", err)
 			}
 			gotStore := ds.getStore(tt.serverAddress)
+			if inherited, ok := gotStore.(inheritedHelperStore); ok {
+				gotStore = inherited.Store
+			}
 			if _, ok := gotStore.(*nativeStore); !ok {
 				t.Errorf("gotStore is not a native store")
 			}
@@ -1129,3 +1167,21 @@ func TestStoreWithFallbacks_MatchesNamespace(t *testing.T) {
 		})
 	}
 }
+
+func Test_inheritedHelperStore_Get_helperError(t *testing.T) {
+	store := inheritedHelperStore{&nativeStore{&testExecuter{}}}
+	got, err := store.Get(context.Background(), exeErrorHost)
+	if err != nil {
+		t.Fatal("inheritedHelperStore.Get() error =", err)
+	}
+	if got != EmptyCredential {
+		t.Errorf("inheritedHelperStore.Get() = %v, want %v", got, EmptyCredential)
+	}
+}
+
+func Test_inheritedHelperStore_Put_helperError(t *testing.T) {
+	store := inheritedHelperStore{&nativeStore{&testExecuter{}}}
+	if err := store.Put(context.Background(), "localhost:500/unknown", Credential{}); err == nil {
+		t.Error("inheritedHelperStore.Put() error = nil, want error")
+	}
+}