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

500 response code for commands denied by auth plugin #22428

Closed
lblackstone opened this issue Apr 29, 2016 · 6 comments · Fixed by #22448
Closed

500 response code for commands denied by auth plugin #22428

lblackstone opened this issue Apr 29, 2016 · 6 comments · Fixed by #22448
Labels
kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed.
Milestone

Comments

@lblackstone
Copy link

lblackstone commented Apr 29, 2016

Output of docker version:

Client:
 Version:      1.11.1
 API version:  1.23
 Go version:   go1.5.4
 Git commit:   5604cbe
 Built:        Tue Apr 26 23:38:55 2016
 OS/Arch:      linux/amd64

Server:
 Version:      1.11.1
 API version:  1.23
 Go version:   go1.5.4
 Git commit:   5604cbe
 Built:        Tue Apr 26 23:38:55 2016
 OS/Arch:      linux/amd64

Output of docker info:

Containers: 0
 Running: 0
 Paused: 0
 Stopped: 0
Images: 0
Server Version: 1.11.1
Storage Driver: devicemapper
 Pool Name: docker-8:1-491769-pool
 Pool Blocksize: 65.54 kB
 Base Device Size: 10.74 GB
 Backing Filesystem: ext4
 Data file: /dev/loop0
 Metadata file: /dev/loop1
 Data Space Used: 305.7 MB
 Data Space Total: 107.4 GB
 Data Space Available: 39.55 GB
 Metadata Space Used: 729.1 kB
 Metadata Space Total: 2.147 GB
 Metadata Space Available: 2.147 GB
 Udev Sync Supported: true
 Deferred Removal Enabled: false
 Deferred Deletion Enabled: false
 Deferred Deleted Device Count: 0
 Data loop file: /var/lib/docker/devicemapper/devicemapper/data
 WARNING: Usage of loopback devices is strongly discouraged for production use. Either use `--storage-opt dm.thinpooldev` or use `--storage-opt dm.no_warn_on_loop_devices=true` to suppress this warning.
 Metadata loop file: /var/lib/docker/devicemapper/devicemapper/metadata
 Library Version: 1.02.99 (2015-06-20)
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins: 
 Volume: local
 Network: host bridge null
 Authorization: docker-auth
Kernel Version: 4.2.0-35-generic
Operating System: Ubuntu 15.10
OSType: linux
Architecture: x86_64
CPUs: 1
Total Memory: 488.9 MiB
Name: vagrant-ubuntu-wily-64
ID: RWVA:C37Z:CVFE:2BEA:U4B5:5DKD:FZ43:74KW:A5XJ:HW5K:6CDC:P5IH
Docker Root Dir: /var/lib/docker
Debug mode (client): false
Debug mode (server): true
 File Descriptors: 14
 Goroutines: 30
 System Time: 2016-04-29T21:01:09.169890289Z
 EventsListeners: 0
Registry: https://index.docker.io/v1/
WARNING: No swap limit support

Additional environment details (AWS, VirtualBox, physical, etc.):
Vagrant-created VirtualBox VM

Steps to reproduce the issue:

  1. Enable an auth plugin
  2. Perform a request that is denied by the auth plugin
  3. Check the response code

Describe the results you received:

curl -v -H "Content-Type: application/json" -X POST -d @bad-command.json --unix-socket /var/run/docker.sock http:/containers/create
*   Trying /var/run/docker.sock...
* Connected to http (/var/run/docker.sock) port 80 (#0)
> POST /containers/create HTTP/1.1
> Host: http
> User-Agent: curl/7.43.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 1645
> Expect: 100-continue
> 
< HTTP/1.1 100 Continue
* We are completely uploaded and fine
< HTTP/1.1 500 Internal Server Error
< Content-Type: text/plain; charset=utf-8
< X-Content-Type-Options: nosniff
< Date: Fri, 29 Apr 2016 20:46:31 GMT
< Content-Length: 183
< 
authorization denied by plugin docker-auth: <Error message>

Describe the results you expected:
Expected the status code to be a 403 Forbidden

Additional information you deem important (e.g. issue happens only occasionally):
According to the auth plugin docs, it looks like it should be possible to modify the response code. However, the relevant fields don't seem to actually be in the code base. (e.g., ModifiedStatusCode appears in the docs, but not the code).

@thaJeztah
Copy link
Member

ping @liron-l @dimastopel PTAL

@thaJeztah thaJeztah added the kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. label Apr 29, 2016
yongtang added a commit to yongtang/docker that referenced this issue Apr 30, 2016
This fix tried to address the issue raised in moby#22428 where
HTTP 500 status code would be returned for commands denied
by auth plugin, instead of HTTP 403 (StatusForbidden).
The reason for this issue is that the error message for
commands denied by auth plugin was not captured properly.

This fix updates the error message capturing to address
the issue.

An additional test has been added to cover the changes.

This fix fixes moby#22428.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Copy link
Member

The issue seems to have been caused by the fact that the error message:
authorization denied by plugin PLUGIN_NAME: ... was not captured properly in http error handling.

A PR #22431 has been created to try to address this issue.

@liron-l
Copy link
Contributor

liron-l commented Apr 30, 2016

Thanks @lblackstone, all response modification steps were removed as part of the authorization framework code review (and we will definitely fix the documentation).

Re response code, I'm not sure 403 (Forbidden) is the most appropriate, possibly 401 (unauthorized), @runcom, @diogomonica, @estesp WDYT?

@runcom
Copy link
Member

runcom commented Apr 30, 2016

Re response code, I'm not sure 403 (Forbidden) is the most appropriate, possibly 401 (unauthorized), @runcom, @diogomonica, @estesp WDYT?

makes sense to me

@runcom
Copy link
Member

runcom commented Apr 30, 2016

wait, I don't think it will work well. If we reply 401 during a pull operation (for instance) and the 401 comes from the auth plugin I suspect we get a login prompt (cause the cli assumes 401 is from the registry) ping @aaronlehmann

@estesp
Copy link
Contributor

estesp commented Apr 30, 2016

Given HTTP 401 will most likely interact improperly with any APIs that deal with auth (all the registry-interaction commands including push and pull), it seems like the w3c definition of 403 actually fits reasonably well with a plugin denying access to the API, given we provide the detail in the response body:

From https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html:

The server understood the request, but is refusing to fulfill it. Authorization will not help and the request SHOULD NOT be repeated. If the request method was not HEAD and the server wishes to make public why the request has not been fulfilled, it SHOULD describe the reason for the refusal in the entity.

@tiborvass tiborvass added this to the 1.11.2 milestone May 2, 2016
liron-l pushed a commit to twistlock/docker that referenced this issue May 2, 2016
…st code (403).

- Return 403 (forbidden) when request is denied in authorization flows
(including integration test)
- Fix moby#22428
- Close moby#22431

Signed-off-by: Liron Levin <liron@twistlock.com>
@thaJeztah thaJeztah modified the milestones: 1.12.0, 1.11.2 May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed.
Projects
None yet
7 participants