-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Handle a Docker daemon without registry info #126
Handle a Docker daemon without registry info #126
Conversation
cc @n4ss |
I missed a test that I am fixing now. |
8eaef41
to
2bb40dd
Compare
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
=======================================
Coverage 46.12% 46.12%
=======================================
Files 161 161
Lines 11006 11006
=======================================
Hits 5077 5077
Misses 5639 5639
Partials 290 290 |
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
==========================================
- Coverage 46.12% 44.81% -1.32%
==========================================
Files 161 169 +8
Lines 11006 11360 +354
==========================================
+ Hits 5077 5091 +14
- Misses 5639 5979 +340
Partials 290 290 |
Good catch for the test, LGTM! |
LGTM |
@aaronlehmann @n4ss I added an additional commit (ddeca13) to test https://github.com/docker/cli/pull/126/files#diff-edfeae638bfd9c8f34d563582ac1ecd4L30) Sorry about making changes after your review. I can squash the commits after that. |
still LGTM :) |
@dnephin Can you help review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding some tests. I think this is looking good.
Since the change is to add a warning, I think it would also be good to check the warning in the test cases.
cli/command/registry_test.go
Outdated
buf := new(bytes.Buffer) | ||
cli := test.NewFakeCli(&fakeClient{infoFunc: tc.infoFunc}, buf) | ||
server := ElectAuthServer(context.Background(), cli) | ||
require.Equal(t, tc.expectedAuthServer, server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good place to use assert.Equal()
instead of require
so that it will run all the test cases, instead of ending after the first failure.
cli/command/registry_test.go
Outdated
expectedAuthServer: "https://index.docker.io/v1/", | ||
infoFunc: func() (types.Info, error) { | ||
return types.Info{IndexServerAddress: ""}, nil | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test the warning on this case you can add fakeClient.SetErr(errorBuffer)
, and then check that the buffer has the string you expect (or at least part of the message).
I guess there is another case below where a warning is expected as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 6cd3a60
cli/command/image/pull_test.go
Outdated
infoFunc := func() (types.Info, error) { | ||
return types.Info{IndexServerAddress: registry.IndexServer}, nil | ||
} | ||
cmd := NewPullCommand(test.NewFakeCli(&fakeClient{infoFunc: infoFunc}, buf)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change necessary to keep the tests passing? I don't see why it would be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, the mock info
endpoint returns an empty Registry
field, which triggers the warning and causes the output not to match expectations.
@dnephin Thanks for the review. I have addressed both your comments here (6cd3a60) Regarding:
Actually the main change is what auth server should be returned if we don't get an registry back from the server. I thought about not emitting any message in that case, but I decided to do so we could more easily uncover misbehaving daemons. |
6c11546
to
6cd3a60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, those changes look good, but I think they should be using stderr
cli/command/registry.go
Outdated
@@ -29,6 +29,8 @@ func ElectAuthServer(ctx context.Context, cli Cli) string { | |||
serverAddress := registry.IndexServer | |||
if info, err := cli.Client().Info(ctx); err != nil { | |||
fmt.Fprintf(cli.Out(), "Warning: failed to get default registry endpoint from daemon (%v). Using system default: %s\n", err, serverAddress) | |||
} else if info.IndexServerAddress == "" { | |||
fmt.Fprintf(cli.Out(), "Warning: Empty registry endpoint from daemon. Using system default: %s\n", serverAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just noticed this now. These should definitely be going to cli.Err()
not cli.Out()
.
7c5777e
to
cf46529
Compare
The current implementation of the ElectAuthServer doesn't handle well when the default Registry server is not included in the response from the daemon Info endpoint. That leads to the storage and usage of the credentials for the default registry (`https://index.docker.io/v1/`) under an empty string on the client config file. Sample config file after a login via a Docker Daemon without Registry information: ```json { "auths": { "": { "auth": "***" } } } ``` That can lead to duplication of the password for the default registry and authentication failures against the default registry if a pull/push is performed without first authenticating via the misbehaving daemon. Also, changes the output of the warning message from stdout to sdterr as per dnephin suggestion. Signed-off-by: Marcus Martins <marcus@docker.com>
cf46529
to
8626497
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@aaronlehmann I did some refactoring based on @dnephin feedback. Do you mind taking another look? |
LGTM |
@marcusmartins @dnephin @aaronlehmann can you pls mark the priority tag in the PR ? It will help us determine if this should go into RC2 or not. |
I am not a maintainer so I will not set a priority but I believe it should be in RC2, so whatever priority reflects that. |
@mavenugo this is a P1 as this leads to a security issue under certain conditions. |
- What I did
The current implementation of the ElectAuthServer doesn't handle well when the
default Registry server is not included in the response from the daemon Info
endpoint.
That leads to the storage and usage of the credentials for the default registry
(
https://index.docker.io/v1/
) under an empty string on the client config file.Sample config file after a login via a Docker Daemon without Registry
information:
That can lead to duplication of the password for the default registry and
authentication failures against the default registry if a pull/push is performed
without first authenticating via the misbehaving daemon.
- How I did it
Added an additional check to the ElectAuthServer function to validate if the daemon returned a default Registry as part of the Info API call.
- How to verify it
Run against a Docker host that doesn't return the default registry on the info api. Modern Docker hosts included that information.
- Description for the changelog
Better handling of registry operations against Daemon that don't return default registry information.
Signed-off-by: Marcus Martins marcus@docker.com