Skip to content

support legacy registries in exernal stores #23100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aaronlehmann merged 1 commit into from
Sep 7, 2016
Merged

support legacy registries in exernal stores #23100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion api/client/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ func (cli *DockerCli) RegistryAuthenticationPrivilegedFunc(index *registrytypes.
return func() (string, error) {
fmt.Fprintf(cli.out, "\nPlease login prior to %s:\n", cmdName)
indexServer := registry.GetAuthConfigKey(index)
authConfig, err := cli.ConfigureAuth("", "", indexServer, false)
isDefaultRegistry := indexServer == cli.ElectAuthServer(context.Background())
authConfig, err := cli.ConfigureAuth("", "", indexServer, isDefaultRegistry)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -91,6 +92,10 @@ func (cli *DockerCli) ConfigureAuth(flUser, flPassword, serverAddress string, is
cli.in = os.Stdin
}

if !isDefaultRegistry {
Copy link
Copy Markdown

@aaronlehmann aaronlehmann Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isDefaultRegistry flag is a bit weird and I'm not sure whether it can be trusted here.

I see that RegistryAuthenticationPrivilegedFunc has the following code:

                fmt.Fprintf(cli.out, "\nPlease login prior to %s:\n", cmdName)
                indexServer := registry.GetAuthConfigKey(index)
                authConfig, err := cli.ConfigureAuth("", "", indexServer, false)

It hardcodes isDefaultRegistry = false, but GetAuthConfig may return the default registry, including the scheme:

// GetAuthConfigKey special-cases using the full index address of the official
// index as the AuthConfig key, and uses the (host)name[:port] for private indexes.
func GetAuthConfigKey(index *registrytypes.IndexInfo) string {
        if index.Official {
                return IndexServer
        }
        return index.Name
}

IndexServer includes the scheme.

So I think that this code path might wrongly strip the scheme from the default registry. Would it make sense to remove the isDefaultRegistry flag (which doesn't seem to be set correctly), and instead have ConfigureAuth check if serverAddress == ElectAuthServer(ctx) instead?

Copy link
Copy Markdown
Member Author

@runcom runcom Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to remove the isDefaultRegistry flag (which doesn't seem to be set correctly), and instead have ConfigureAuth check if serverAddress == ElectAuthServer(ctx) instead?

yup, works for me, so that IsDefaultRegistry becomes a local variable and is set to := serverAddress == ElectAuthServer(ctx) - am I understanding correctly?

Copy link
Copy Markdown

@aaronlehmann aaronlehmann Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I was thinking. I'm not 100% certain about this so I wonder if I'm missing something, but it seems like that would be more correct than relying on the isDefaultRegistry flag.

Copy link
Copy Markdown
Member Author

@runcom runcom Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll edit it and see if tests catch anything..

Copy link
Copy Markdown
Member Author

@runcom runcom Sep 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronlehmann BTW, RegistryAuthenticationPrivilegedFunc hardcodes false just because it needs not to pop up:

Login with your Docker ID to push and pull images from Docker Hub. If you don' t have a Docker ID, head over to https://hub.docker.com to create one.

When prompting interactively... which seems wrong so fixing..

Copy link
Copy Markdown
Member Author

@runcom runcom Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronlehmann push forced to take care of this, PTAL

serverAddress = registry.ConvertToHostname(serverAddress)
}

authconfig, err := GetCredentials(cli.configFile, serverAddress)
if err != nil {
return authconfig, err
Expand Down
12 changes: 8 additions & 4 deletions api/client/registry/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,18 @@ func runLogin(dockerCli *client.DockerCli, opts loginOptions) error {
ctx := context.Background()
clnt := dockerCli.Client()

var serverAddress string
var isDefaultRegistry bool
var (
serverAddress string
authServer = dockerCli.ElectAuthServer(ctx)
)
if opts.serverAddress != "" {
serverAddress = opts.serverAddress
} else {
serverAddress = dockerCli.ElectAuthServer(ctx)
isDefaultRegistry = true
serverAddress = authServer
}

isDefaultRegistry := serverAddress == authServer

authConfig, err := dockerCli.ConfigureAuth(opts.user, opts.password, serverAddress, isDefaultRegistry)
if err != nil {
return err
Expand Down
35 changes: 30 additions & 5 deletions api/client/registry/logout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/docker/docker/api/client"
"github.com/docker/docker/cli"
"github.com/docker/docker/registry"
"github.com/spf13/cobra"
)

Expand All @@ -31,21 +32,45 @@ func NewLogoutCommand(dockerCli *client.DockerCli) *cobra.Command {

func runLogout(dockerCli *client.DockerCli, serverAddress string) error {
ctx := context.Background()
var isDefaultRegistry bool

if serverAddress == "" {
serverAddress = dockerCli.ElectAuthServer(ctx)
isDefaultRegistry = true
}

var (
loggedIn bool
regsToLogout []string
hostnameAddress = serverAddress
regsToTry = []string{serverAddress}
)
if !isDefaultRegistry {
hostnameAddress = registry.ConvertToHostname(serverAddress)
// the tries below are kept for backward compatibily where a user could have
// saved the registry in one of the following format.
regsToTry = append(regsToTry, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress)
}

// check if we're logged in based on the records in the config file
// which means it couldn't have user/pass cause they may be in the creds store
if _, ok := dockerCli.ConfigFile().AuthConfigs[serverAddress]; !ok {
fmt.Fprintf(dockerCli.Out(), "Not logged in to %s\n", serverAddress)
for _, s := range regsToTry {
if _, ok := dockerCli.ConfigFile().AuthConfigs[s]; ok {
loggedIn = true
regsToLogout = append(regsToLogout, s)
}
}

if !loggedIn {
fmt.Fprintf(dockerCli.Out(), "Not logged in to %s\n", hostnameAddress)
return nil
}

fmt.Fprintf(dockerCli.Out(), "Removing login credentials for %s\n", serverAddress)
if err := client.EraseCredentials(dockerCli.ConfigFile(), serverAddress); err != nil {
fmt.Fprintf(dockerCli.Err(), "WARNING: could not erase credentials: %v\n", err)
fmt.Fprintf(dockerCli.Out(), "Removing login credentials for %s\n", hostnameAddress)
for _, r := range regsToLogout {
if err := client.EraseCredentials(dockerCli.ConfigFile(), r); err != nil {
fmt.Fprintf(dockerCli.Err(), "WARNING: could not erase credentials: %v\n", err)
}
}

return nil
Expand Down
20 changes: 3 additions & 17 deletions cliconfig/credentials/file_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package credentials

import (
"strings"

"github.com/docker/docker/cliconfig/configfile"
"github.com/docker/docker/registry"
"github.com/docker/engine-api/types"
)

Expand Down Expand Up @@ -32,8 +31,8 @@ func (c *fileStore) Get(serverAddress string) (types.AuthConfig, error) {
if !ok {
// Maybe they have a legacy config file, we will iterate the keys converting
// them to the new format and testing
for registry, ac := range c.file.AuthConfigs {
if serverAddress == convertToHostname(registry) {
for r, ac := range c.file.AuthConfigs {
if serverAddress == registry.ConvertToHostname(r) {
return ac, nil
}
}
Expand All @@ -52,16 +51,3 @@ func (c *fileStore) Store(authConfig types.AuthConfig) error {
c.file.AuthConfigs[authConfig.ServerAddress] = authConfig
return c.file.Save()
}

func convertToHostname(url string) string {
stripped := url
if strings.HasPrefix(url, "http://") {
stripped = strings.Replace(url, "http://", "", 1)
} else if strings.HasPrefix(url, "https://") {
stripped = strings.Replace(url, "https://", "", 1)
}

nameParts := strings.SplitN(stripped, "/", 2)

return nameParts[0]
}
44 changes: 44 additions & 0 deletions integration-cli/docker_cli_logout_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package main

import (
"bytes"
"fmt"
"io/ioutil"
"os"
"os/exec"
"path/filepath"

"github.com/docker/docker/pkg/integration/checker"
Expand Down Expand Up @@ -54,3 +56,45 @@ func (s *DockerRegistryAuthHtpasswdSuite) TestLogoutWithExternalAuth(c *check.C)
c.Assert(err, check.NotNil, check.Commentf(out))
c.Assert(out, checker.Contains, "Error: image dockercli/busybox:authtest not found")
}

// #23100
func (s *DockerRegistryAuthHtpasswdSuite) TestLogoutWithWrongHostnamesStored(c *check.C) {
osPath := os.Getenv("PATH")
defer os.Setenv("PATH", osPath)

workingDir, err := os.Getwd()
c.Assert(err, checker.IsNil)
absolute, err := filepath.Abs(filepath.Join(workingDir, "fixtures", "auth"))
c.Assert(err, checker.IsNil)
testPath := fmt.Sprintf("%s%c%s", osPath, filepath.ListSeparator, absolute)

os.Setenv("PATH", testPath)

cmd := exec.Command("docker-credential-shell-test", "store")
stdin := bytes.NewReader([]byte(fmt.Sprintf(`{"ServerURL": "https://%s", "Username": "%s", "Secret": "%s"}`, privateRegistryURL, s.reg.username, s.reg.password)))
cmd.Stdin = stdin
c.Assert(cmd.Run(), checker.IsNil)

tmp, err := ioutil.TempDir("", "integration-cli-")
c.Assert(err, checker.IsNil)

externalAuthConfig := fmt.Sprintf(`{ "auths": {"https://%s": {}}, "credsStore": "shell-test" }`, privateRegistryURL)

configPath := filepath.Join(tmp, "config.json")
err = ioutil.WriteFile(configPath, []byte(externalAuthConfig), 0644)
c.Assert(err, checker.IsNil)

dockerCmd(c, "--config", tmp, "login", "-u", s.reg.username, "-p", s.reg.password, privateRegistryURL)

b, err := ioutil.ReadFile(configPath)
c.Assert(err, checker.IsNil)
c.Assert(string(b), checker.Contains, fmt.Sprintf("\"https://%s\": {}", privateRegistryURL))
c.Assert(string(b), checker.Contains, fmt.Sprintf("\"%s\": {}", privateRegistryURL))

dockerCmd(c, "--config", tmp, "logout", privateRegistryURL)

b, err = ioutil.ReadFile(configPath)
c.Assert(err, checker.IsNil)
c.Assert(string(b), checker.Not(checker.Contains), fmt.Sprintf("\"https://%s\": {}", privateRegistryURL))
c.Assert(string(b), checker.Not(checker.Contains), fmt.Sprintf("\"%s\": {}", privateRegistryURL))
}
46 changes: 46 additions & 0 deletions integration-cli/docker_cli_pull_local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,52 @@ func (s *DockerRegistrySuite) TestPullManifestList(c *check.C) {
dockerCmd(c, "rmi", repoName)
}

// #23100
func (s *DockerRegistryAuthHtpasswdSuite) TestPullWithExternalAuthLoginWithScheme(c *check.C) {
osPath := os.Getenv("PATH")
defer os.Setenv("PATH", osPath)

workingDir, err := os.Getwd()
c.Assert(err, checker.IsNil)
absolute, err := filepath.Abs(filepath.Join(workingDir, "fixtures", "auth"))
c.Assert(err, checker.IsNil)
testPath := fmt.Sprintf("%s%c%s", osPath, filepath.ListSeparator, absolute)

os.Setenv("PATH", testPath)

repoName := fmt.Sprintf("%v/dockercli/busybox:authtest", privateRegistryURL)

tmp, err := ioutil.TempDir("", "integration-cli-")
c.Assert(err, checker.IsNil)

externalAuthConfig := `{ "credsStore": "shell-test" }`

configPath := filepath.Join(tmp, "config.json")
err = ioutil.WriteFile(configPath, []byte(externalAuthConfig), 0644)
c.Assert(err, checker.IsNil)

dockerCmd(c, "--config", tmp, "login", "-u", s.reg.username, "-p", s.reg.password, privateRegistryURL)

b, err := ioutil.ReadFile(configPath)
c.Assert(err, checker.IsNil)
c.Assert(string(b), checker.Not(checker.Contains), "\"auth\":")

dockerCmd(c, "--config", tmp, "tag", "busybox", repoName)
dockerCmd(c, "--config", tmp, "push", repoName)

dockerCmd(c, "--config", tmp, "logout", privateRegistryURL)
dockerCmd(c, "--config", tmp, "login", "-u", s.reg.username, "-p", s.reg.password, "https://"+privateRegistryURL)
dockerCmd(c, "--config", tmp, "pull", repoName)

// likewise push should work
repoName2 := fmt.Sprintf("%v/dockercli/busybox:nocreds", privateRegistryURL)
dockerCmd(c, "tag", repoName, repoName2)
dockerCmd(c, "--config", tmp, "push", repoName2)

// logout should work w scheme also because it will be stripped
dockerCmd(c, "--config", tmp, "logout", "https://"+privateRegistryURL)
}

func (s *DockerRegistryAuthHtpasswdSuite) TestPullWithExternalAuth(c *check.C) {
osPath := os.Getenv("PATH")
defer os.Setenv("PATH", osPath)
Expand Down
30 changes: 16 additions & 14 deletions registry/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,21 @@ func v2AuthHTTPClient(endpoint *url.URL, authTransport http.RoundTripper, modifi

}

// ConvertToHostname converts a registry url which has http|https prepended
// to just an hostname.
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://")
}

nameParts := strings.SplitN(stripped, "/", 2)

return nameParts[0]
}

// ResolveAuthConfig matches an auth configuration to a server address or a URL
func ResolveAuthConfig(authConfigs map[string]types.AuthConfig, index *registrytypes.IndexInfo) types.AuthConfig {
configKey := GetAuthConfigKey(index)
Expand All @@ -214,23 +229,10 @@ func ResolveAuthConfig(authConfigs map[string]types.AuthConfig, index *registryt
return c
}

convertToHostname := func(url string) string {
stripped := url
if strings.HasPrefix(url, "http://") {
stripped = strings.Replace(url, "http://", "", 1)
} else if strings.HasPrefix(url, "https://") {
stripped = strings.Replace(url, "https://", "", 1)
}

nameParts := strings.SplitN(stripped, "/", 2)

return nameParts[0]
}

// Maybe they have a legacy config file, we will iterate the keys converting
// them to the new format and testing
for registry, ac := range authConfigs {
if configKey == convertToHostname(registry) {
if configKey == ConvertToHostname(registry) {
return ac
}
}
Expand Down