Error on namespaced registries for credential helpers

We now error on login if repositories or repository namespaces are used
for other credential helpers than the `AuthenticationFileHelper`. On
logout we ignore them and debug log a warning that nothing has been
modified.

The functions `SetCredentials` (for login) as well as
`RemoveAuthentication` (for logout) already feature support for path
based registries for the `AuthenticationFileHelper`. This patch adds
unit tests to ensure that the support will not break in the future.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

docs/containers-auth.json.5.md

@@ -21,14 +21,15 @@ Except the primary (read/write) file, other files are read-only, unless the user
 The auth.json file stores encrypted authentication information for the
 user to container image registries.  The file can have zero to many entries and
 is created by a `login` command from a container tool such as `podman login`,
-`buildah login` or `skopeo login`.  Each entry includes the name of the registry and then an auth
-token in the form of a base64 encoded string from the concatenation of the
-username, a colon, and the password. The registry name can additionally contain
-a path or repository name (an image name without tag or digest). The path (or
-namespace) is matched in its hierarchical order when checking for available
-authentications. For example, an image pull for
-`my-registry.local/namespace/user/image:latest` will result in a lookup in
-`auth.json` in the following order:
+`buildah login` or `skopeo login`. Each entry either contains a single
+hostname (e.g. `docker.io`) or a namespace (e.g. `quay.io/user/image`) as a key
+and an auth token in the form of a base64 encoded string as value of `auth`. The
+token is built from the concatenation of the username, a colon, and the
+password. The registry name can additionally contain a repository name (an image
+name without tag or digest) and namespaces. The path (or namespace) is matched
+in its hierarchical order when checking for available authentications. For
+example, an image pull for `my-registry.local/namespace/user/image:latest` will
+result in a lookup in `auth.json` in the following order:
 
 - `my-registry.local/namespace/user/image`
 - `my-registry.local/namespace/user`

pkg/docker/config/config.go

@@ -54,10 +54,17 @@ var (
 
 // SetCredentials stores the username and password in a location
 // appropriate for sys and the users’ configuration.
+// A valid key can be either a registry hostname or additionally a namespace if
+// the AuthenticationFileHelper is being unsed.
 // Returns a human-redable description of the location that was updated.
 // NOTE: The return value is only intended to be read by humans; its form is not an API,
 // it may change (or new forms can be added) any time.
-func SetCredentials(sys *types.SystemContext, registry, username, password string) (string, error) {
+func SetCredentials(sys *types.SystemContext, key, username, password string) (string, error) {
+	isNamespaced, err := validateKey(key)
+	if err != nil {
+		return "", err
+	}
+
 	helpers, err := sysregistriesv2.CredentialHelpers(sys)
 	if err != nil {
 		return "", err
@@ -72,33 +79,45 @@ func SetCredentials(sys *types.SystemContext, registry, username, password strin
 		// Special-case the built-in helpers for auth files.
 		case sysregistriesv2.AuthenticationFileHelper:
 			desc, err = modifyJSON(sys, func(auths *dockerConfigFile) (bool, error) {
-				if ch, exists := auths.CredHelpers[registry]; exists {
-					return false, setAuthToCredHelper(ch, registry, username, password)
+				if ch, exists := auths.CredHelpers[key]; exists {
+					if isNamespaced {
+						return false, unsupportedNamespaceErr(ch)
+					}
+					return false, setAuthToCredHelper(ch, key, username, password)
 				}
 				creds := base64.StdEncoding.EncodeToString([]byte(username + ":" + password))
 				newCreds := dockerAuthConfig{Auth: creds}
-				auths.AuthConfigs[registry] = newCreds
+				auths.AuthConfigs[key] = newCreds
 				return true, nil
 			})
 		// External helpers.
 		default:
-			desc = fmt.Sprintf("credential helper: %s", helper)
-			err = setAuthToCredHelper(helper, registry, username, password)
+			if isNamespaced {
+				err = unsupportedNamespaceErr(helper)
+			} else {
+				desc = fmt.Sprintf("credential helper: %s", helper)
+				err = setAuthToCredHelper(helper, key, username, password)
+			}
 		}
 		if err != nil {
 			multiErr = multierror.Append(multiErr, err)
-			logrus.Debugf("Error storing credentials for %s in credential helper %s: %v", registry, helper, err)
+			logrus.Debugf("Error storing credentials for %s in credential helper %s: %v", key, helper, err)
 			continue
 		}
-		logrus.Debugf("Stored credentials for %s in credential helper %s", registry, helper)
+		logrus.Debugf("Stored credentials for %s in credential helper %s", key, helper)
 		return desc, nil
 	}
 	return "", multiErr
 }
 
+func unsupportedNamespaceErr(helper string) error {
+	return errors.Errorf("namespaced key is not supported for credential helper %s", helper)
+}
+
 // SetAuthentication stores the username and password in the credential helper or file
-func SetAuthentication(sys *types.SystemContext, registry, username, password string) error {
-	_, err := SetCredentials(sys, registry, username, password)
+// See the documentation of SetCredentials for format of "key"
+func SetAuthentication(sys *types.SystemContext, key, username, password string) error {
+	_, err := SetCredentials(sys, key, username, password)
 	return err
 }
 
@@ -326,9 +345,16 @@ func getAuthenticationWithHomeDir(sys *types.SystemContext, registry, homeDir st
 	return auth.Username, auth.Password, nil
 }
 
-// RemoveAuthentication removes credentials for `registry` from all possible
+// RemoveAuthentication removes credentials for `key` from all possible
 // sources such as credential helpers and auth files.
-func RemoveAuthentication(sys *types.SystemContext, registry string) error {
+// A valid key can be either a registry hostname or additionally a namespace if
+// the AuthenticationFileHelper is being unsed.
+func RemoveAuthentication(sys *types.SystemContext, key string) error {
+	isNamespaced, err := validateKey(key)
+	if err != nil {
+		return err
+	}
+
 	helpers, err := sysregistriesv2.CredentialHelpers(sys)
 	if err != nil {
 		return err
@@ -338,17 +364,22 @@ func RemoveAuthentication(sys *types.SystemContext, registry string) error {
 	isLoggedIn := false
 
 	removeFromCredHelper := func(helper string) {
-		err := deleteAuthFromCredHelper(helper, registry)
-		if err == nil {
-			logrus.Debugf("Credentials for %q were deleted from credential helper %s", registry, helper)
-			isLoggedIn = true
+		if isNamespaced {
+			logrus.Debugf("Not removing credentials because namespaced keys are not supported for the credential helper: %s", helper)
 			return
+		} else {
+			err := deleteAuthFromCredHelper(helper, key)
+			if err == nil {
+				logrus.Debugf("Credentials for %q were deleted from credential helper %s", key, helper)
+				isLoggedIn = true
+				return
+			}
+			if credentials.IsErrCredentialsNotFoundMessage(err.Error()) {
+				logrus.Debugf("Not logged in to %s with credential helper %s", key, helper)
+				return
+			}
 		}
-		if credentials.IsErrCredentialsNotFoundMessage(err.Error()) {
-			logrus.Debugf("Not logged in to %s with credential helper %s", registry, helper)
-			return
-		}
-		multiErr = multierror.Append(multiErr, errors.Wrapf(err, "removing credentials for %s from credential helper %s", registry, helper))
+		multiErr = multierror.Append(multiErr, errors.Wrapf(err, "removing credentials for %s from credential helper %s", key, helper))
 	}
 
 	for _, helper := range helpers {
@@ -357,15 +388,12 @@ func RemoveAuthentication(sys *types.SystemContext, registry string) error {
 		// Special-case the built-in helper for auth files.
 		case sysregistriesv2.AuthenticationFileHelper:
 			_, err = modifyJSON(sys, func(auths *dockerConfigFile) (bool, error) {
-				if innerHelper, exists := auths.CredHelpers[registry]; exists {
+				if innerHelper, exists := auths.CredHelpers[key]; exists {
 					removeFromCredHelper(innerHelper)
 				}
-				if _, ok := auths.AuthConfigs[registry]; ok {
+				if _, ok := auths.AuthConfigs[key]; ok {
 					isLoggedIn = true
-					delete(auths.AuthConfigs, registry)
-				} else if _, ok := auths.AuthConfigs[normalizeRegistry(registry)]; ok {
-					isLoggedIn = true
-					delete(auths.AuthConfigs, normalizeRegistry(registry))
+					delete(auths.AuthConfigs, key)
 				}
 				return true, multiErr
 			})
@@ -699,18 +727,18 @@ func decodeDockerAuth(conf dockerAuthConfig) (types.DockerAuthConfig, error) {
 // to just an hostname.
 // Copied from github.com/docker/docker/registry/auth.go
 func convertToHostname(url string) string {
-	stripped := url
-	if strings.HasPrefix(url, "http://") {
-		stripped = strings.TrimPrefix(url, "http://")
-	} else if strings.HasPrefix(url, "https://") {
-		stripped = strings.TrimPrefix(url, "https://")
-	}
-
+	stripped := stripScheme(url)
 	nameParts := strings.SplitN(stripped, "/", 2)
-
 	return nameParts[0]
 }
 
+// stripScheme striped the http|https scheme from the provided URL.
+func stripScheme(url string) string {
+	stripped := strings.TrimPrefix(url, "http://")
+	stripped = strings.TrimPrefix(stripped, "https://")
+	return stripped
+}
+
 func normalizeRegistry(registry string) string {
 	normalized := convertToHostname(registry)
 	switch normalized {
@@ -719,3 +747,14 @@ func normalizeRegistry(registry string) string {
 	}
 	return normalized
 }
+
+// validateKey verifies that the input key does not have a prefix that is not
+// allowed and returns an indicator if the key is namespaced.
+func validateKey(key string) (isNamespaced bool, err error) {
+	if strings.HasPrefix(key, "http://") || strings.HasPrefix(key, "https://") {
+		return isNamespaced, errors.Errorf("key %s contains http[s]:// prefix", key)
+	}
+
+	// check if the provided key contains one or more subpaths.
+	return strings.ContainsRune(key, '/'), nil
+}

pkg/docker/config/config_test.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -675,3 +676,213 @@ func TestAuthKeysForRef(t *testing.T) {
 		require.Equal(t, tc.expected, result, tc.name)
 	}
 }
+
+func TestSetCredentials(t *testing.T) {
+	const (
+		usernamePrefix = "username-"
+		passwordPrefix = "password-"
+	)
+	getAuth := func(sys *types.SystemContext, input string) types.DockerAuthConfig {
+		ref, err := reference.ParseNamed(input)
+		require.NoError(t, err)
+		auth, err := GetCredentialsForRef(sys, ref)
+		require.NoError(t, err)
+		return auth
+	}
+
+	for _, tc := range []struct {
+		input  []string
+		assert func(*types.SystemContext, dockerConfigFile)
+	}{
+		{
+			input: []string{"quay.io"},
+			assert: func(sys *types.SystemContext, auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 1)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io"].Auth)
+			},
+		},
+		{
+			input: []string{"quay.io/a/b/c/d/image"},
+			assert: func(sys *types.SystemContext, auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 1)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io/a/b/c/d/image"].Auth)
+
+				ta := getAuth(sys, "quay.io/a/b/c/d/image")
+				assert.Equal(t, usernamePrefix+"0", ta.Username)
+				assert.Equal(t, passwordPrefix+"0", ta.Password)
+			},
+		},
+		{
+			input: []string{
+				"quay.io/a/b/c",
+				"quay.io/a/b",
+				"quay.io/a",
+				"quay.io",
+				"my-registry.local",
+				"my-registry.local",
+			},
+			assert: func(sys *types.SystemContext, auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 5)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io/a/b/c"].Auth)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io/a/b"].Auth)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io/a"].Auth)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io"].Auth)
+				assert.NotEmpty(t, auth.AuthConfigs["my-registry.local"].Auth)
+
+				ta0 := getAuth(sys, "quay.io/a/b/c")
+				assert.Equal(t, usernamePrefix+"0", ta0.Username)
+				assert.Equal(t, passwordPrefix+"0", ta0.Password)
+
+				ta1 := getAuth(sys, "quay.io/a/b")
+				assert.Equal(t, usernamePrefix+"1", ta1.Username)
+				assert.Equal(t, passwordPrefix+"1", ta1.Password)
+
+				ta2 := getAuth(sys, "quay.io/a")
+				assert.Equal(t, usernamePrefix+"2", ta2.Username)
+				assert.Equal(t, passwordPrefix+"2", ta2.Password)
+
+			},
+		},
+	} {
+		tmpFile, err := ioutil.TempFile("", "auth.json.set")
+		require.NoError(t, err)
+		defer os.RemoveAll(tmpFile.Name())
+
+		_, err = tmpFile.WriteString("{}")
+		require.NoError(t, err)
+		sys := &types.SystemContext{AuthFilePath: tmpFile.Name()}
+
+		for i, input := range tc.input {
+			_, err := SetCredentials(
+				sys,
+				input,
+				usernamePrefix+fmt.Sprint(i),
+				passwordPrefix+fmt.Sprint(i),
+			)
+			assert.NoError(t, err)
+		}
+
+		auth, err := readJSONFile(tmpFile.Name(), false)
+		require.NoError(t, err)
+
+		tc.assert(sys, auth)
+	}
+}
+
+func TestRemoveAuthentication(t *testing.T) {
+	testAuth := dockerAuthConfig{Auth: "ZXhhbXBsZTpvcmc="}
+	for _, tc := range []struct {
+		config      dockerConfigFile
+		inputs      []string
+		shouldError bool
+		assert      func(dockerConfigFile)
+	}{
+		{
+			config: dockerConfigFile{
+				AuthConfigs: map[string]dockerAuthConfig{
+					"quay.io": testAuth,
+				},
+			},
+			inputs: []string{"quay.io"},
+			assert: func(auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 0)
+			},
+		},
+		{
+			config: dockerConfigFile{
+				AuthConfigs: map[string]dockerAuthConfig{
+					"quay.io": testAuth,
+				},
+			},
+			inputs:      []string{"quay.io/user/image"},
+			shouldError: true, // not logged in
+			assert: func(auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 1)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io"].Auth)
+			},
+		},
+		{
+			config: dockerConfigFile{
+				AuthConfigs: map[string]dockerAuthConfig{
+					"quay.io":           testAuth,
+					"my-registry.local": testAuth,
+				},
+			},
+			inputs: []string{"my-registry.local"},
+			assert: func(auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 1)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io"].Auth)
+			},
+		},
+		{
+			config: dockerConfigFile{
+				AuthConfigs: map[string]dockerAuthConfig{
+					"quay.io/a/b/c":     testAuth,
+					"quay.io/a/b":       testAuth,
+					"quay.io/a":         testAuth,
+					"quay.io":           testAuth,
+					"my-registry.local": testAuth,
+				},
+			},
+			inputs: []string{
+				"quay.io/a/b",
+				"quay.io",
+				"my-registry.local",
+			},
+			assert: func(auth dockerConfigFile) {
+				assert.Len(t, auth.AuthConfigs, 2)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io/a/b/c"].Auth)
+				assert.NotEmpty(t, auth.AuthConfigs["quay.io/a"].Auth)
+			},
+		},
+	} {
+
+		content, err := json.Marshal(&tc.config)
+		require.NoError(t, err)
+
+		tmpFile, err := ioutil.TempFile("", "auth.json")
+		require.NoError(t, err)
+		defer os.RemoveAll(tmpFile.Name())
+
+		_, err = tmpFile.Write(content)
+		require.NoError(t, err)
+
+		sys := &types.SystemContext{AuthFilePath: tmpFile.Name()}
+
+		for _, input := range tc.inputs {
+			err := RemoveAuthentication(sys, input)
+			if tc.shouldError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		}
+
+		auth, err := readJSONFile(tmpFile.Name(), false)
+		require.NoError(t, err)
+
+		tc.assert(auth)
+	}
+}
+
+func TestValidateKey(t *testing.T) {
+	for _, tc := range []struct {
+		key          string
+		shouldError  bool
+		isNamespaced bool
+	}{
+		{"my-registry.local", false, false},
+		{"https://my-registry.local", true, false},
+		{"my-registry.local/path", false, true},
+		{"quay.io/a/b/c/d", false, true},
+	} {
+
+		isNamespaced, err := validateKey(tc.key)
+		if tc.shouldError {
+			assert.Error(t, err)
+		} else {
+			assert.NoError(t, err)
+		}
+		assert.Equal(t, tc.isNamespaced, isNamespaced)
+	}
+}