Skip to content

types,docker: add DockerAuthConfig #113

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
mtrmac merged 1 commit into from
Oct 11, 2016
Merged

types,docker: add DockerAuthConfig #113

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: 5 additions & 2 deletions docker/docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func newDockerClient(ctx *types.SystemContext, ref dockerReference, write bool)
if registry == dockerHostname {
registry = dockerRegistry
}
username, password, err := getAuth(ref.ref.Hostname())
username, password, err := getAuth(ctx, ref.ref.Hostname())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -243,7 +243,10 @@ func (c *dockerClient) getBearerToken(realm, service, scope string) (string, err
return tokenStruct.Token, nil
}

func getAuth(registry string) (string, string, error) {
func getAuth(ctx *types.SystemContext, registry string) (string, string, error) {
if ctx != nil && ctx.DockerAuthConfig != nil {
return ctx.DockerAuthConfig.Username, ctx.DockerAuthConfig.Password, nil
Copy link
Collaborator

@mtrmac mtrmac Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why support both Auth and Username/Password here? Do we already have real users for both which would benefit? Then we need to document which overrides what, it just seems like unnecessary API complexity.

I’d like to have only the more explicit and type-safe username/password pair, if there were no other reason to prefer the Auth one or to supply both.

Copy link
Collaborator

@mtrmac mtrmac Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

(It is not completely certain that we can always ignore the hostname here, but it is likely enough to be the common case; we can always add a DockerAuthConfigMap later, so this is fine.)

Copy link
Member Author

@runcom runcom Oct 3, 2016

Choose a reason for hiding this comment

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

Ocid can provide username/password from k8s or skopeo can. K8s and OpenShift can just provide Auth.

Copy link
Collaborator

@mtrmac mtrmac Oct 3, 2016

Choose a reason for hiding this comment

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

K8s and OpenShift can just provide Auth.

AFAICT for access to the Docker endpoint for OpenShift, one would set {Username:"unused",Password:openShiftTokenString}, at least based on what ~/.kube/config is storing. Does the API really give the user the Docker base64 blob somewhere?

Copy link
Member Author

@runcom runcom Oct 3, 2016

Choose a reason for hiding this comment

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

Alright, not sure about origin then, but that struct is exactly the same in k8s so it might happen k8s sends it that way

}
// TODO(runcom): get this from *cli.Context somehow
//if username != "" && password != "" {
//return username, password, nil
Expand Down
35 changes: 28 additions & 7 deletions docker/docker_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"reflect"
"testing"

"github.com/containers/image/types"
"github.com/docker/docker/pkg/homedir"
)

Expand Down Expand Up @@ -43,6 +44,7 @@ func TestGetAuth(t *testing.T) {
expectedUsername string
expectedPassword string
expectedError error
ctx *types.SystemContext
}{
{
name: "empty hostname",
Expand Down Expand Up @@ -130,6 +132,21 @@ func TestGetAuth(t *testing.T) {
expectedUsername: "me",
expectedPassword: "mine",
},
{
name: "use system context",
hostname: "example.org",
authConfig: makeTestAuthConfig(testAuthConfigDataMap{
"example.org": testAuthConfigData{"user", "pw"},
}),
expectedUsername: "foo",
expectedPassword: "bar",
ctx: &types.SystemContext{
DockerAuthConfig: &types.DockerAuthConfig{
Username: "foo",
Password: "bar",
},
},
},
} {
contents, err := json.MarshalIndent(&tc.authConfig, "", " ")
if err != nil {
Expand All @@ -141,7 +158,11 @@ func TestGetAuth(t *testing.T) {
continue
}

username, password, err := getAuth(tc.hostname)
var ctx *types.SystemContext
if tc.ctx != nil {
ctx = tc.ctx
}
username, password, err := getAuth(ctx, tc.hostname)
if err == nil && tc.expectedError != nil {
t.Errorf("[%s] got unexpected non error and username=%q, password=%q", tc.name, username, password)
continue
Expand Down Expand Up @@ -222,7 +243,7 @@ func TestGetAuthFromLegacyFile(t *testing.T) {
continue
}

username, password, err := getAuth(tc.hostname)
username, password, err := getAuth(nil, tc.hostname)
if err == nil && tc.expectedError != nil {
t.Errorf("[%s] got unexpected non error and username=%q, password=%q", tc.name, username, password)
continue
Expand Down Expand Up @@ -293,7 +314,7 @@ func TestGetAuthPreferNewConfig(t *testing.T) {
}
}

username, password, err := getAuth("index.docker.io")
username, password, err := getAuth(nil, "index.docker.io")
if err != nil {
t.Fatalf("got unexpected error: %#+v", err)
}
Expand Down Expand Up @@ -330,7 +351,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
configPath := filepath.Join(configDir, "config.json")

// no config file present
username, password, err := getAuth("index.docker.io")
username, password, err := getAuth(nil, "index.docker.io")
if err != nil {
t.Fatalf("got unexpected error: %#+v", err)
}
Expand All @@ -341,7 +362,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
if err := ioutil.WriteFile(configPath, []byte("Json rocks! Unless it doesn't."), 0640); err != nil {
t.Fatalf("failed to write file %q: %v", configPath, err)
}
username, password, err = getAuth("index.docker.io")
username, password, err = getAuth(nil, "index.docker.io")
if err == nil {
t.Fatalf("got unexpected non-error: username=%q, password=%q", username, password)
}
Expand All @@ -352,7 +373,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
// remove the invalid config file
os.RemoveAll(configPath)
// no config file present
username, password, err = getAuth("index.docker.io")
username, password, err = getAuth(nil, "index.docker.io")
if err != nil {
t.Fatalf("got unexpected error: %#+v", err)
}
Expand All @@ -364,7 +385,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
if err := ioutil.WriteFile(configPath, []byte("I'm certainly not a json string."), 0640); err != nil {
t.Fatalf("failed to write file %q: %v", configPath, err)
}
username, password, err = getAuth("index.docker.io")
username, password, err = getAuth(nil, "index.docker.io")
if err == nil {
t.Fatalf("got unexpected non-error: username=%q, password=%q", username, password)
}
Expand Down
8 changes: 8 additions & 0 deletions types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@ type ImageInspectInfo struct {
Layers []string
}

// DockerAuthConfig contains authorization information for connecting to a registry.
type DockerAuthConfig struct {
Username string
Password string
}

// SystemContext allows parametrizing access to implicitly-accessed resources,
// like configuration files in /etc and users' login state in their home directory.
// Various components can share the same field only if their semantics is exactly
Expand All @@ -228,4 +234,6 @@ type SystemContext struct {
// === docker.Transport overrides ===
DockerCertPath string // If not "", a directory containing "cert.pem" and "key.pem" used when talking to a Docker Registry
DockerInsecureSkipTLSVerify bool // Allow contacting docker registries over HTTP, or HTTPS with failed TLS verification. Note that this does not affect other TLS connections.
// if nil, the library tries to parse ~/.docker/config.json to retrieve credentials
DockerAuthConfig *DockerAuthConfig
}