From 0e016646a094f7194262b4aa1ccb094d1ef3f32c Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Thu, 1 Jul 2021 11:02:07 +0200 Subject: [PATCH] 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 --- docs/containers-auth.json.5.md | 2 +- pkg/docker/config/config.go | 109 +++++++++++------ pkg/docker/config/config_test.go | 193 +++++++++++++++++++++++++++++++ 3 files changed, 269 insertions(+), 35 deletions(-) diff --git a/docs/containers-auth.json.5.md b/docs/containers-auth.json.5.md index 7acc0ddf7b..48c8829932 100644 --- a/docs/containers-auth.json.5.md +++ b/docs/containers-auth.json.5.md @@ -24,7 +24,7 @@ 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 +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 diff --git a/pkg/docker/config/config.go b/pkg/docker/config/config.go index e5ef902f55..40f4aa5410 100644 --- a/pkg/docker/config/config.go +++ b/pkg/docker/config/config.go @@ -54,10 +54,18 @@ var ( // SetCredentials stores the username and password in a location // appropriate for sys and the users’ configuration. +// An error will be part of the returned multierror if the provided key is +// namespaced and a credentials helper other than the AuthenticationFileHelper +// is used. // 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 +80,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 { + 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) +func SetAuthentication(sys *types.SystemContext, key, username, password string) error { + _, err := SetCredentials(sys, key, username, password) return err } @@ -326,9 +346,14 @@ 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 { +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 +363,21 @@ 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 - return - } - if credentials.IsErrCredentialsNotFoundMessage(err.Error()) { - logrus.Debugf("Not logged in to %s with credential helper %s", registry, helper) - return + if isNamespaced { + logrus.Debugf("Not removing credentials: %v", unsupportedNamespaceErr(helper)) + } 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 + } } - 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 +386,15 @@ 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 { + delete(auths.AuthConfigs, key) + } else if _, ok := auths.AuthConfigs[normalizeRegistry(key)]; ok { isLoggedIn = true - delete(auths.AuthConfigs, normalizeRegistry(registry)) + delete(auths.AuthConfigs, normalizeRegistry(key)) } return true, multiErr }) @@ -699,18 +728,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 +748,15 @@ func normalizeRegistry(registry string) string { } return normalized } + +// validateKey verifies that the input key does not have any not allowed prefix +// 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. + stripped := stripScheme(key) + return strings.ContainsRune(stripped, '/'), nil +} diff --git a/pkg/docker/config/config_test.go b/pkg/docker/config/config_test.go index dd5f465311..da74127b61 100644 --- a/pkg/docker/config/config_test.go +++ b/pkg/docker/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "encoding/json" "fmt" "io/ioutil" "os" @@ -675,3 +676,195 @@ 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 + 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, + "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) + 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) + } +}