Skip to content
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

Merged

Conversation

@marcusmartins
Copy link
Contributor

commented May 24, 2017

- 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:

{
	"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.

- 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

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

cc @n4ss

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

I missed a test that I am fixing now.

@marcusmartins marcusmartins force-pushed the marcusmartins:handle_empty_registry_info branch from 8eaef41 to 2bb40dd May 25, 2017

@codecov-io

This comment has been minimized.

Copy link

commented May 25, 2017

Codecov Report

Merging #126 into master will not change coverage.
The diff coverage is n/a.

@@           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-io

This comment has been minimized.

Copy link

commented May 25, 2017

Codecov Report

Merging #126 into master will decrease coverage by 1.31%.
The diff coverage is 100%.

@@            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
@n4ss

This comment has been minimized.

Copy link
Contributor

commented May 25, 2017

Good catch for the test, LGTM!

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2017

LGTM

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@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.

@n4ss

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

still LGTM :)

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@dnephin Can you help review?

@dnephin
Copy link
Collaborator

left a comment

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.

buf := new(bytes.Buffer)
cli := test.NewFakeCli(&fakeClient{infoFunc: tc.infoFunc}, buf)
server := ElectAuthServer(context.Background(), cli)
require.Equal(t, tc.expectedAuthServer, server)

This comment has been minimized.

Copy link
@dnephin

dnephin May 26, 2017

Collaborator

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.

expectedAuthServer: "https://index.docker.io/v1/",
infoFunc: func() (types.Info, error) {
return types.Info{IndexServerAddress: ""}, nil
},

This comment has been minimized.

Copy link
@dnephin

dnephin May 26, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@marcusmartins

marcusmartins May 26, 2017

Author Contributor

Addressed in 6cd3a60

infoFunc := func() (types.Info, error) {
return types.Info{IndexServerAddress: registry.IndexServer}, nil
}
cmd := NewPullCommand(test.NewFakeCli(&fakeClient{infoFunc: infoFunc}, buf))

This comment has been minimized.

Copy link
@dnephin

dnephin May 26, 2017

Collaborator

Was this change necessary to keep the tests passing? I don't see why it would be necessary

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 26, 2017

Collaborator

Without this, the mock info endpoint returns an empty Registry field, which triggers the warning and causes the output not to match expectations.

@marcusmartins marcusmartins referenced this pull request May 26, 2017
23 of 23 tasks complete
@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@dnephin Thanks for the review.

I have addressed both your comments here (6cd3a60)

Regarding:

Since the change is to add a warning, I think it would also be good to check the warning in the test cases.

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.

@marcusmartins marcusmartins force-pushed the marcusmartins:handle_empty_registry_info branch from 6c11546 to 6cd3a60 May 26, 2017

@dnephin
Copy link
Collaborator

left a comment

Thanks, those changes look good, but I think they should be using stderr

@@ -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)

This comment has been minimized.

Copy link
@dnephin

dnephin May 26, 2017

Collaborator

Oh, I just noticed this now. These should definitely be going to cli.Err() not cli.Out().

@marcusmartins marcusmartins force-pushed the marcusmartins:handle_empty_registry_info branch from 7c5777e to cf46529 May 26, 2017

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@dnephin Thanks. Addressed in cf46529

I was also able to remove the additional mocking on the pull function now that the output is going to stderr.

Handle a Docker daemon without registry info
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>

@marcusmartins marcusmartins force-pushed the marcusmartins:handle_empty_registry_info branch from cf46529 to 8626497 May 26, 2017

@dnephin
Copy link
Collaborator

left a comment

LGTM

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@aaronlehmann I did some refactoring based on @dnephin feedback. Do you mind taking another look?

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2017

LGTM

@aaronlehmann aaronlehmann merged commit 1b8b63b into docker:master May 26, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project Absolute coverage decreased by -1.31% but relative coverage increased by +53.87% compared to 11e7d35
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.06.0 milestone May 26, 2017

@marcusmartins marcusmartins deleted the marcusmartins:handle_empty_registry_info branch May 26, 2017

@mavenugo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@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.

@marcusmartins

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2017

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.

@n4ss

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

@mavenugo this is a P1 as this leads to a security issue under certain conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.