Unauthorized push is not detected

Issue description

If no credentials have been provided during a push operation on a secured registry (like the official docker repository), docker-py call is successful (no docker.errors.APIError is thrown) but the error is reported in output messages.

Environment

This issue has been detected on Ubuntu 17.04 with the latest version of docker-py.

docker==2.5.1
docker-pycreds==0.2.1
Python 3.6.2 :: Anaconda, Inc.
Client:
 Version:      1.12.6
 API version:  1.24
 Go version:   go1.7.4
 Git commit:   78d1802
 Built:        Tue Mar 14 09:47:15 2017
 OS/Arch:      linux/amd64

Server:
 Version:      1.12.6
 API version:  1.24
 Go version:   go1.7.4
 Git commit:   78d1802
 Built:        Tue Mar 14 09:47:15 2017
 OS/Arch:      linux/amd64

Code snippet

import docker
cli = docker.from_env(version='auto')
res = cli.images.push('centos')  # No error thrown here...
print(res)
# {"status":"The push refers to a repository [docker.io/library/centos]"}
# {"status":"Preparing","progressDetail":{},"id":"cf516324493c"}
# {"errorDetail":{"message":"unauthorized: authentication required"},"error":"unauthorized: authentication required"}

[shin-]

Thank you for the report! As far as I can tell, this is working as intended. Since the error only occurs inside the response after the HTTP status has been set, there is no way for the library to pre-emptively detect it.

Well, I'll try to convince you then :)

• This is not an explicit behavior (no exception has been raised, so everything is all right isn't it?). In consequence, if the developer is not a Docker Engine API expert (like me :) ), it's clearly unexpected.
• This is not very well documented, it only indicates that the function returns The output from the server. It might be good idea to precise errors can be embedded in the output message.
• It's not the behavior of docker-cli. If a docker push fails due to unauthorized access, the command line returns an error exit code (1). It's therefore safer to use pythonsubprocess.check_call(['docker', 'push', 'centos']) than docker-py API.
• If it's not the job of docker-py, it's the job of the caller. But in the future Docker Engine responses can change, who knows.. Callers shall in consequence be sure to support all Docker Engine versions. I thought it was the goal of docker-py to solve this kind of problems.

For your information, this error is extracted from the JSON message by the HTTP client of docker-cli:

docker/cli/cli/command/plugin/push.go:

func newPushCommand(dockerCli command.Cli) *cobra.Command {
	...

	return jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), nil)
}

moby/moby/client/request.go:

func (jm *JSONMessage) Display(out io.Writer, termInfo termInfo) error {
	if jm.Error != nil {
		if jm.Error.Code == 401 {
			return fmt.Errorf("authentication is required")
		}
		return jm.Error
}

I think it might be a good improvement to implement this in docker-py.

[erezool]

+1

[feliperuhland]

Hi @remy-phelipot @erezool.

@shin- is right. Unfortunately, the Docker Engine API has this behavior. The method returns before knowing if there was an error due to authorization problem. It may be possible to use some alternatives:

• Verify that credentials are present (Does not guarantee successful login) docker login status command?  moby/moby#15466;
• Log in before sending the image;
• Use the subprocess module;
• Download a private image (very small) before sending the image to ensure successful access.

Verify that credentials are present

As you said, there is no guarantee

Log in before sending the image

What about unsecured registries? Does it work?

Use the subprocess module

It should work, but it is overkill in my point of view.

Download a private image (very small) before sending the image to ensure successful access.

If you are off-line with a private registry, this operation fails.

Why not implement the same behaviour as docker-cli? We have to parse the response message, detect if an error message has been given by the API and raise it if necessary (and we have to do it for every REST requests, not only the push of an image). Something like:

response_msg = xxxx  # REST API request (Push or something else)
response_dict = json.loads(response_msg)
if 'error' in response_dict and response_dict['error']:
     raise docker.errors.APIError(response_dict['error'])

It's simple and shall cover all cases,and one more time, it is the actual docker-cli behaviour, so it's not an ugly fix ;-)

[feliperuhland]

Hi @remy-phelipot

I understand your point of view, but I do not know if the purpose of the library is to act this way. Anyway, I made a quick change (without testing) of the possible check_for_errors flag. Can you test? https://github.com/feliperuhland/docker-py/tree/issue-1772-docker-push-unauthorized

You should pass the argument check_for_errors=True.

And just to make it clear, I do not know if this idea will be included in the code.