Fix error 500 on distribution endpoint #37534

thaJeztah · 2018-07-26T13:07:15Z

This PR consists of two commits

1. Return "invalid parameter" (4xx) errors for distribution

For errors related to the provided input, return an errdefs.InvalidParameter(),
so that they can be returned as a 4xx error by the API.

2. Handle correct status codes for distribution errors

This assists to address a regression where distribution errors were not properly
handled, resulting in a generic 500 (internal server error) to be returned for
/distribution/name/json if you weren't authenticated, whereas it should return
a 40x (401).

This patch attempts to extract the HTTP status-code that was returned by the
distribution code, and falls back to returning a 500 status if unable to match.

Before this change:

curl -v --unix-socket /var/run/docker.sock http://localhost/distribution/name/json
> GET /distribution/name/json HTTP/1.1
>
< HTTP/1.1 500 Internal Server Error
<
{"message":"errors:\ndenied: requested access to the resource is denied\nunauthorized: authentication required\n"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

daemon logs:

DEBU[2018-07-03T15:52:51.424950601Z] Calling GET /distribution/name/json
DEBU[2018-07-03T15:52:53.179895572Z] FIXME: Got an API for which error does not match any expected type!!!: errors:
denied: requested access to the resource is denied
unauthorized: authentication required
  error_type=errcode.Errors module=api
ERRO[2018-07-03T15:52:53.179942783Z] Handler for GET /distribution/name/json returned error: errors:
denied: requested access to the resource is denied
unauthorized: authentication required

With this patch applied:

curl -v --unix-socket /var/run/docker.sock http://localhost/distribution/name/json
> GET /distribution/name/json HTTP/1.1
>
< HTTP/1.1 403 Forbidden
<
{"message":"errors:\ndenied: requested access to the resource is denied\nunauthorized: authentication required\n"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

daemon logs:

DEBU[2018-08-03T14:58:08.018726228Z] Calling GET /distribution/name/json

distribution/errors.go

api/server/router/distribution/distribution_routes.go

codecov · 2018-08-03T15:51:37Z

Codecov Report

Merging #37534 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #37534      +/-   ##
==========================================
- Coverage   36.56%   36.54%   -0.02%     
==========================================
  Files         610      610              
  Lines       45395    45411      +16     
==========================================
- Hits        16598    16595       -3     
- Misses      26505    26523      +18     
- Partials     2292     2293       +1

thaJeztah · 2018-08-03T15:56:40Z

daemon/images/image_pull.go

Only error returned comes from

moby/registry/config.go

Lines 342 to 344 in 8aa694c

if strings.HasPrefix(val, "-") || strings.HasSuffix(val, "-") {

return "", fmt.Errorf("invalid index name (%s). Cannot begin or end with a hyphen", val)

}

Maybe this should be specified as invalid param in registry/config.go instead.
Presuming that this is an invalid param from the registry service seems a bit too strong of an assumption.

Good suggestion, I'll make that change 👍

thaJeztah · 2018-08-03T16:00:57Z

ping @dmcgowan @vdemeester @cpuguy83 PTAL

thaJeztah · 2018-08-03T16:02:12Z

api/server/httputils/errors.go

Perhaps we should have a utility in github.com/docker/distribution to work with these errors; they're a bit hard to use (imo)

cpuguy83

Minor nit.

I guess this is fine to bubble up http codes from distribution.

api/server/httputils/errors.go

thaJeztah · 2019-02-07T16:51:52Z

@cpuguy83 addressed your nit; just recalled that the last commit may have to be discussed; do we want to convert 403 to 404, or leave that part out? (We do use it in other locations)

thaJeztah · 2019-02-07T19:18:53Z

Discussing in the maintainers meeting, and I'll drop the last commit

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This assists to address a regression where distribution errors were not properly handled, resulting in a generic 500 (internal server error) to be returned for `/distribution/name/json` if you weren't authenticated, whereas it should return a 40x (401). This patch attempts to extract the HTTP status-code that was returned by the distribution code, and falls back to returning a 500 status if unable to match. Before this change: curl -v --unix-socket /var/run/docker.sock http://localhost/distribution/name/json * Trying /var/run/docker.sock... * Connected to localhost (/var/run/docker.sock) port 80 (#0) > GET /distribution/name/json HTTP/1.1 > Host: localhost > User-Agent: curl/7.52.1 > Accept: */* > < HTTP/1.1 500 Internal Server Error < Api-Version: 1.37 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/dev (linux) < Date: Tue, 03 Jul 2018 15:52:53 GMT < Content-Length: 115 < {"message":"errors:\ndenied: requested access to the resource is denied\nunauthorized: authentication required\n"} * Curl_http_done: called premature == 0 * Connection #0 to host localhost left intact daemon logs: DEBU[2018-07-03T15:52:51.424950601Z] Calling GET /distribution/name/json DEBU[2018-07-03T15:52:53.179895572Z] FIXME: Got an API for which error does not match any expected type!!!: errors: denied: requested access to the resource is denied unauthorized: authentication required error_type=errcode.Errors module=api ERRO[2018-07-03T15:52:53.179942783Z] Handler for GET /distribution/name/json returned error: errors: denied: requested access to the resource is denied unauthorized: authentication required With this patch applied: curl -v --unix-socket /var/run/docker.sock http://localhost/distribution/name/json * Trying /var/run/docker.sock... * Connected to localhost (/var/run/docker.sock) port 80 (#0) > GET /distribution/name/json HTTP/1.1 > Host: localhost > User-Agent: curl/7.52.1 > Accept: */* > < HTTP/1.1 403 Forbidden < Api-Version: 1.38 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/dev (linux) < Date: Fri, 03 Aug 2018 14:58:09 GMT < Content-Length: 115 < {"message":"errors:\ndenied: requested access to the resource is denied\nunauthorized: authentication required\n"} * Curl_http_done: called premature == 0 * Connection #0 to host localhost left intact daemon logs: DEBU[2018-08-03T14:58:08.018726228Z] Calling GET /distribution/name/json Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2019-02-08T09:38:52Z

Removed the last commit; this should be ready to go now

ping @vdemeester @yongtang PTAL

dmcgowan · 2019-03-13T00:31:31Z

LGTM

yongtang

LGTM

vdemeester

LGTM 🐯

Fix error 500 on distribution endpoint Signed-off-by: xichengliudui <1693291525@qq.com>

thaJeztah · 2019-03-13T10:11:04Z

hm, I just noticed that the API defines that this endpoint could either return a 200, a 401 (unauthorized) or a 500, but doesn't mention a 403. However, looks like the registry code uses 403, not 401 🤔 (need to double check)

I'll do some digging as to what's the expectation

thaJeztah · 2019-03-13T11:48:48Z

Ok, so looks like the registry returns two errors;

curl -v --unix-socket /var/run/docker.sock http://localhost/distribution/name/json
*   Trying /var/run/docker.sock...
* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> GET /distribution/name/json HTTP/1.1
> Host: localhost
> User-Agent: curl/7.52.1
> Accept: */*
> 
< HTTP/1.1 403 Forbidden
< Api-Version: 1.40
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Wed, 13 Mar 2019 10:29:55 GMT
< Content-Length: 115
< 
{"message":"errors:\ndenied: requested access to the resource is denied\nunauthorized: authentication required\n"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

both an 403 ("requested access to the resource is denied") and an 401 ("authentication required") are returned; https://github.com/moby/moby/blob/f2614f2/vendor/github.com/docker/distribution/registry/api/errcode/register.go#L36-L56

The change in this PR picks the "outer" error (403), so wondering which one should be picked (and if the current assumption in the API is actually incorrect), basically;

403 is correct; access to the resource is denied
401 is also correct, and could be seen as the "underlying" cause for the access to be denied

Trying what happens if an invalid authentication is sent

# create a dummy auth header
registryAuth=$(echo -n '{"username":"someusername", "password": "password", "serveraddress": "https://index.docker.io/v1/"}' | base64 -w0)

curl \
  -v \
  --unix-socket /var/run/docker.sock \
  -H "Content-Type: application/json" \
  -H "X-Registry-Auth: $registryAuth" \
  "http://localhost/distribution/name/json"

Which hm.. only returns unauthorized: incorrect username or password, but somehow this does not get handled, so a 500 is returned 😞

*   Trying /var/run/docker.sock...
* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> GET /distribution/name/json HTTP/1.1
> Host: localhost
> User-Agent: curl/7.52.1
> Accept: */*
> Content-Type: application/json
> X-Registry-Auth: eyJ1c2VybmFtZSI6InNvbWV1c2VybmFtZSIsICJwYXNzd29yZCI6ICJwYXNzd29yZCIsICJzZXJ2ZXJhZGRyZXNzIjogImh0dHBzOi8vaW5kZXguZG9ja2VyLmlvL3YxLyJ9
> 
< HTTP/1.1 500 Internal Server Error
< Api-Version: 1.40
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Wed, 13 Mar 2019 11:42:34 GMT
< Content-Length: 127
< 
{"message":"Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

Corresponding daemon logs;

DEBU[2019-03-13T11:42:34.088361967Z] Calling GET /distribution/name/json          
DEBU[2019-03-13T11:42:34.924217997Z] FIXME: Got an API for which error does not match any expected type!!!: Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password  error_type="*url.Error" module=api
ERRO[2019-03-13T11:42:34.924295114Z] Handler for GET /distribution/name/json returned error: Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T11:42:34.924308078Z] FIXME: Got an API for which error does not match any expected type!!!: Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password  error_type="*url.Error" module=api

Given that the above is using a non-existing image, I also tried using an existing private image, but that produces the same result.

thaJeztah · 2019-03-13T14:46:52Z

Adding some debugging code;

diff --git a/api/server/httputils/errors.go b/api/server/httputils/errors.go
index 2abdce2bcf..48c14a7f3c 100644
--- a/api/server/httputils/errors.go
+++ b/api/server/httputils/errors.go
@@ -142,6 +142,11 @@ func statusCodeFromDistributionError(err error) int {
                if len(errs) < 1 {
                        return http.StatusInternalServerError
                }
+               for i := range errs {
+                       if debugerr, ok := errs[i].(errcode.ErrorCoder); ok {
+                               logrus.Debugf("distribution error %d, message: %s, statusCode: %d", i, debugerr.ErrorCode().Descriptor().Message, debugerr.ErrorCode().Descriptor().HTTPStatusCode)
+                       }
+               }
                if _, ok := errs[0].(errcode.ErrorCoder); ok {
                        return statusCodeFromDistributionError(errs[0])
                }
@@ -152,5 +157,6 @@ func statusCodeFromDistributionError(err error) int {
                        return statusCodeFromDistributionError(e.Cause())
                }
        }
+       logrus.Debugf("distribution returned an unknown error %v", err)
        return http.StatusInternalServerError
 }
diff --git a/api/server/router/distribution/distribution_routes.go b/api/server/router/distribution/distribution_routes.go
index d285728382..122a7d9ad9 100644
--- a/api/server/router/distribution/distribution_routes.go
+++ b/api/server/router/distribution/distribution_routes.go
@@ -73,7 +73,7 @@ func (s *distributionRouter) getDistributionInfo(ctx context.Context, w http.Res
 
                descriptor, err := distrepo.Tags(ctx).Get(ctx, taggedRef.Tag())
                if err != nil {
-                       return err
+                       return errors.Wrap(err, "ERROR COMES FROM HERE")
                }
                distributionInspect.Descriptor = v1.Descriptor{
                        MediaType: descriptor.MediaType,
diff --git a/vendor/github.com/docker/distribution/registry/client/repository.go b/vendor/github.com/docker/distribution/registry/client/repository.go
index aa442e6540..b189d5c5af 100644
--- a/vendor/github.com/docker/distribution/registry/client/repository.go
+++ b/vendor/github.com/docker/distribution/registry/client/repository.go
@@ -320,6 +320,12 @@ func (t *tags) Get(ctx context.Context, tag string) (distribution.Descriptor, er
        }
 
        resp, err := newRequest("HEAD")
+       if resp != nil {
+               fmt.Printf("Request returned status: %d - %s", resp.StatusCode, resp.Status)
+       }
+       if err != nil {
+               fmt.Printf("Error requesting url: %q - %v\n", u, err)
+       }
        if err != nil {
                return distribution.Descriptor{}, err
        }

Produces this in the logs (for request without authentication):

curl -v --unix-socket /var/run/docker.sock http://localhost/distribution/name/json



DEBU[2019-03-13T14:43:50.857809587Z] Calling GET /distribution/name/json          
Request returned status: 401 - 401 UnauthorizedDEBU[2019-03-13T14:43:52.580540935Z] distribution error 0, message: requested access to the resource is denied, statusCode: 403 
DEBU[2019-03-13T14:43:52.580604262Z] distribution error 1, message: authentication required, statusCode: 401 
DEBU[2019-03-13T14:43:52.580618021Z] distribution error 0, message: requested access to the resource is denied, statusCode: 403 
DEBU[2019-03-13T14:43:52.580671241Z] distribution error 1, message: authentication required, statusCode: 401

In the situation with authentication, we don't seem to reach statusCodeFromDistributionError

# create a dummy auth header
registryAuth=$(echo -n '{"username":"someusername", "password": "password", "serveraddress": "https://index.docker.io/v1/"}' | base64 -w0)

curl \
  -v \
  --unix-socket /var/run/docker.sock \
  -H "Content-Type: application/json" \
  -H "X-Registry-Auth: $registryAuth" \
  "http://localhost/distribution/name/json"

DEBU[2019-03-13T14:44:20.257820739Z] Calling GET /distribution/name/json          
Error requesting url: "https://registry-1.docker.io/v2/library/name/manifests/latest" - Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password
DEBU[2019-03-13T14:44:21.095904194Z] distribution returned an unknown error Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.095928919Z] distribution returned an unknown error Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.095936386Z] distribution returned an unknown error Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.095945112Z] FIXME: Got an API for which error does not match any expected type!!!: Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password  error_type="*url.Error" module=api
ERRO[2019-03-13T14:44:21.096022786Z] Handler for GET /distribution/name/json returned error: ERROR COMES FROM HERE: Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.096066777Z] distribution returned an unknown error Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.096074798Z] distribution returned an unknown error Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.096080728Z] distribution returned an unknown error Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password 
DEBU[2019-03-13T14:44:21.096086591Z] FIXME: Got an API for which error does not match any expected type!!!: Head https://registry-1.docker.io/v2/library/name/manifests/latest: unauthorized: incorrect username or password  error_type="*url.Error" module=api

So t.client.Do(req) returns an err instead of a resp with a non-200 status

Doing the same with a plain http.client;

package main

import (
	"fmt"
	"net/http"
)

func main() {
	client := &http.Client{}
	method := "GET"
	u := "https://registry-1.docker.io/v2/library/name/manifests/latest"
	req, err := http.NewRequest(method, u, nil)
	if err != nil {
		fmt.Println(err)
	}
	resp, err := client.Do(req)
	if resp != nil {
		fmt.Printf("Request returned status: %d - %s", resp.StatusCode, resp.Status)
	}
	if err != nil {
		fmt.Printf("Error requesting url: %q - %v\n", u, err)
	}
	_ = resp.Body.Close()
}

Does return a response (thus a status-code);

Request returned status: 401 - 401 Unauthorized
Process finished with exit code 0

thaJeztah · 2019-03-13T14:51:12Z

That client is constructed in NewV2Repository

moby/distribution/registry.go

Lines 136 to 138 in b0e6eed

    
           repo, err = client.NewRepository(repoNameRef, endpoint.URL.String(), tr) 
        
           if err != nil { 
        
           	err = fallbackError{

moby/vendor/github.com/docker/distribution/registry/client/repository.go

Lines 135 to 152 in b0e6eed

    
           func NewRepository(name reference.Named, baseURL string, transport http.RoundTripper) (distribution.Repository, error) { 
        
           	ub, err := v2.NewURLBuilderFromString(baseURL, false) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	client := &http.Client{ 
        
           		Transport:     transport, 
        
           		CheckRedirect: checkHTTPRedirect, 
        
           		// TODO(dmcgowan): create cookie jar 
        
           	} 
        
           	return &repository{ 
        
           		client: client, 
        
           		ub:     ub, 
        
           		name:   name, 
        
           	}, nil 
        
           }

GordonTheTurtle added the status/0-triage label Jul 26, 2018

thaJeztah commented Jul 26, 2018

View reviewed changes

distribution/errors.go Outdated Show resolved Hide resolved

thaJeztah commented Jul 26, 2018

View reviewed changes

api/server/router/distribution/distribution_routes.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the fix-distribution-500 branch from 11167da to a0564b9 Compare August 3, 2018 15:51

thaJeztah commented Aug 3, 2018

View reviewed changes

thaJeztah added area/distribution Image Distribution status/2-code-review and removed status/0-triage labels Aug 3, 2018

thaJeztah commented Aug 3, 2018

View reviewed changes

thaJeztah changed the title ~~[WIP] fix error 500 on distribution endpoint~~ Fix error 500 on distribution endpoint Aug 3, 2018

GordonTheTurtle assigned runcom Aug 10, 2018

thaJeztah force-pushed the fix-distribution-500 branch from a0564b9 to 2cb2eef Compare October 25, 2018 15:51

derek bot added the rebuild/* label Dec 21, 2018

GordonTheTurtle removed the rebuild/* label Dec 21, 2018

cpuguy83 approved these changes Feb 1, 2019

View reviewed changes

api/server/httputils/errors.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the fix-distribution-500 branch from 2cb2eef to b05c3ee Compare February 7, 2019 16:50

thaJeztah added 2 commits February 8, 2019 10:35

Return "invalid parameter" (4xx) errors for distribution

d71ed3d

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the fix-distribution-500 branch from b05c3ee to cdcea6f Compare February 8, 2019 09:36

yongtang approved these changes Mar 13, 2019

View reviewed changes

vdemeester approved these changes Mar 13, 2019

View reviewed changes

vdemeester merged commit 46036c2 into moby:master Mar 13, 2019

xichengliudui pushed a commit to xichengliudui/moby that referenced this pull request Mar 13, 2019

Merge pull request moby#37534 from thaJeztah/fix-distribution-500

1fd20d0

Fix error 500 on distribution endpoint Signed-off-by: xichengliudui <1693291525@qq.com>

thaJeztah deleted the fix-distribution-500 branch March 13, 2019 09:52

thaJeztah mentioned this pull request Mar 13, 2019

[WIP][18.09 backport] Fix error 500 on distribution endpoint docker-archive/engine#172

Closed

thaJeztah mentioned this pull request Jul 13, 2021

errdefs: remove dependency on grpc, containerd, and distribution #42624

Closed

	if strings.HasPrefix(val, "-") \|\| strings.HasSuffix(val, "-") {
	return "", fmt.Errorf("invalid index name (%s). Cannot begin or end with a hyphen", val)
	}

Fix error 500 on distribution endpoint #37534

Fix error 500 on distribution endpoint #37534

Uh oh!

Conversation

thaJeztah commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Return "invalid parameter" (4xx) errors for distribution

2. Handle correct status codes for distribution errors

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 3, 2018

Uh oh!

thaJeztah Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Feb 7, 2019

Uh oh!

thaJeztah commented Feb 7, 2019

Uh oh!

thaJeztah commented Feb 8, 2019

Uh oh!

dmcgowan commented Mar 13, 2019

Uh oh!

yongtang left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thaJeztah commented Jul 26, 2018 •

edited

Loading

codecov bot commented Aug 3, 2018 •

edited

Loading

thaJeztah commented Mar 13, 2019 •

edited

Loading

thaJeztah commented Mar 13, 2019 •

edited

Loading

thaJeztah commented Mar 13, 2019 •

edited

Loading

thaJeztah commented Mar 13, 2019 •

edited

Loading