DockerFetcher swallows non-JSON errors returned by registries

[thaJeztah]

Description

Docker Hub has introduced rate-limits, and now returns a 429 status code if those rate limits are reached, with details about the rate limits in the response body (as plain text).

However, it looks like the containerd "docker" fetcher code does not return the response body if it's not formatted as JSON.

Steps to reproduce the issue:

When using buildkit (which uses the containerd code to pull images), not details are returned;

DOCKER_BUILDKIT=1 docker build -<<EOF
FROM ratelimitalways/test
EOF
...
failed to load cache key: failed to copy: httpReaderSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/ratelimitalways/test/manifests/sha256:c2d41d2ba6d8b7b4a3ffec621578eb4d9a0909df29dfa2f6fd8a2e5fd0836aed: 429 Too Many Requests

Whereas using the "classic" builder, which uses the docker/distribution code returns the error-message returned by the registry;

DOCKER_BUILDKIT=0 docker build -<<EOF
FROM ratelimitalways/test
EOF

Sending build context to Docker daemon  2.048kB
Step 1/1 : FROM ratelimitalways/test
toomanyrequests: Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/

Same when doing a straight docker pull;

docker pull  ratelimitalways/test
Using default tag: latest
Error response from daemon: toomanyrequests: Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/

Describe the results you expected:

I expect the pull code to either;

• return details from the JSON error message (if provided), or
• return the plain-text BODY of the response

While a JSON formatted error can be returned, a plain-text body is allowed to be returned as well; looking at the distribution spec; https://github.com/opencontainers/distribution-spec/blob/v1.0.0-rc1/spec.md#error-codes

A 4XX response code from the registry MAY return a body in any format. If the response body is in JSON format, it MUST have the following format:

Digging a bit into the error handling, I see that de1da8b (#3173) may be the culprit. That commit was opened as a follow-up to #3109, which addressed a similar issue as reported here (#3076).

I couldn't find details about #3173. I see that commit removed the ioutil.ReadAll(resp.Body), which may have been the reason (to prevent a DOS where the body would lead to memory exhaustion). The PR review did have this comment; #3173 (comment)

The original PR was submitted to expose servers (registries) providing specific details to help users understand why a specific action failed (e.g. billing, quota, etc.)

So on the bright side, I guess we at least have a way to test server responses.

Any other relevant information:

More of a side-note (probably worth a separate "enhancement" ticket; there may be other omissions in handling of server-responses. I see that "old" docker/distribution code that docker uses is (too) "complicated", but has more specific handling for errors returned; https://github.com/docker/distribution/blob/749f6afb4572201e3c37325d0ffedb6f32be8950/registry/client/errors.go#L41-L84

And the docker code further improves handling by looking if certain request should be retried; https://github.com/moby/moby/blob/ad1b781e44fa1e44b9e654e5078929aec56aed66/distribution/errors.go#L148-L180

[thaJeztah]

@estesp @crosbymichael @dmcgowan PTAL. Happy to work on this (mostly wanted to ask for the motivations for #3173, and if the ioutil.ReadAll(resp.Body) was the reason for that PR (and thus should be avoided)

/cc @chris-crone @tonistiigi

[tonistiigi]

cc @jonjohnsonjr Discussed related aspect of this docker/build-push-action#171 (comment) (although actual error condition/request is different here)

[ingshtrom]

Well the endpoint does actually return a JSON response for the 429 status code. Here is an example that would be identical to running docker pull ratelimitalways/test:latest. We set the response header Content-Type: application/json and then the body is JSON formatted--maybe the JSON is off a bit somewhere or I missed a special character in the string that might be throwing it off?

⛵  docker-desktop in infra-allowlist on  main
❯ jwt=$(curl -sSL "https://auth.docker.io/token?service=registry.docker.io&scope=repository:ratelimitalways/test:pull" | jq -r .token)

⛵  docker-desktop in infra-allowlist on  main
❯ curl https://registry-1.docker.io/v2/ratelimitalways/test/manifests/latest -H "Authorization: Bearer ${jwt}" -v 2>&1
*   Trying 3.211.199.249...
* TCP_NODELAY set
* Connected to registry-1.docker.io (3.211.199.249) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server did not agree to a protocol
* Server certificate:
*  subject: CN=*.docker.io
*  start date: May 23 00:00:00 2020 GMT
*  expire date: Jun 23 12:00:00 2021 GMT
*  subjectAltName: host "registry-1.docker.io" matched cert's "*.docker.io"
*  issuer: C=US; O=Amazon; OU=Server CA 1B; CN=Amazon
*  SSL certificate verify ok.
> GET /v2/ratelimitalways/test/manifests/latest HTTP/1.1
> Host: registry-1.docker.io
> User-Agent: curl/7.64.1
> Accept: */*
> Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsIng1YyI6WyJNSUlDK1RDQ0FwK2dBd0lCQWdJQkFEQUtCZ2dxaGtqT1BRUURBakJHTVVRd1FnWURWUVFERXpzeVYwNVpPbFZMUzFJNlJFMUVVanBTU1U5Rk9reEhOa0U2UTFWWVZEcE5SbFZNT2tZelNFVTZOVkF5VlRwTFNqTkdPa05CTmxrNlNrbEVVVEFlRncweU1EQXhNRFl5TURVeE1UUmFGdzB5TVRBeE1qVXlNRFV4TVRSYU1FWXhSREJDQmdOVkJBTVRPMVZCVVRjNldGTk9VenBVUjFRek9rRTBXbFU2U1RWSFN6cFNOalJaT2xkRFNFTTZWMVpTU3pwTlNUTlNPa3RZVFRjNlNGZFRNenBDVmxwYU1JSUJJakFOQmdrcWhraUc5dzBCQVFFRkFBT0NBUThBTUlJQkNnS0NBUUVBcnh5Mm9uSDBTWHh4a1JCZG9wWDFWc1VuQVovOUpZR3JNSXlrelJuMTRsd1A1SkVmK1hNNUFORW1NLzBYOFJyNUlIN2VTWFV6K1lWaFVucVNKc3lPUi9xd3BTeFdLWUxxVnB1blFOWThIazdmRnlvN0l0bXgxajZ1dnRtVmFibFFPTEZJMUJNVnY2Y3IxVjV3RlZRYWc3SnhkRUFSZWtaR1M5eDlIcnM1NVdxb0lSK29GRGwxVVRjNlBFSjZVWGdwYmhXWHZoU0RPaXBPcUlYdHZkdHJoWFFpd204Y3EyczF0TEQzZzg2WmRYVFg3UDFFZkxFOG1jMEh4anBGNkdiNWxHZFhjdjU5cC9SMzEva0xlL09wRHNnVWJxMEFvd3Bsc1lLb0dlSmdVNDJaZG45SFZGUVFRcEtGTFBNK1pQN0R2ZmVGMWNIWFhGblI1TkpFU1Z1bFRRSURBUUFCbzRHeU1JR3ZNQTRHQTFVZER3RUIvd1FFQXdJSGdEQVBCZ05WSFNVRUNEQUdCZ1JWSFNVQU1FUUdBMVVkRGdROUJEdFZRVkUzT2xoVFRsTTZWRWRVTXpwQk5GcFZPa2sxUjBzNlVqWTBXVHBYUTBoRE9sZFdVa3M2VFVrelVqcExXRTAzT2toWFV6TTZRbFphV2pCR0JnTlZIU01FUHpBOWdEc3lWMDVaT2xWTFMxSTZSRTFFVWpwU1NVOUZPa3hITmtFNlExVllWRHBOUmxWTU9rWXpTRVU2TlZBeVZUcExTak5HT2tOQk5sazZTa2xFVVRBS0JnZ3Foa2pPUFFRREFnTklBREJGQWlFQXl5SEpJU1RZc1p2ZVZyNWE1YzZ4MjhrQ2U5M2w1QndQVGRUTk9SaFB0c0VDSURMR3pYdUxuekRqTCtzcWRkOU5FbkRuMnZ2UFBWVk1NLzhDQW1EaTVudnMiXX0.eyJhY2Nlc3MiOlt7InR5cGUiOiJyZXBvc2l0b3J5IiwibmFtZSI6InJhdGVsaW1pdGFsd2F5cy90ZXN0IiwiYWN0aW9ucyI6WyJwdWxsIl0sInBhcmFtZXRlcnMiOnsicHVsbF9saW1pdCI6IjEwMCIsInB1bGxfbGltaXRfaW50ZXJ2YWwiOiIyMTYwMCJ9fV0sImF1ZCI6InJlZ2lzdHJ5LmRvY2tlci5pbyIsImV4cCI6MTYwMzk5MjM1NiwiaWF0IjoxNjAzOTkyMDU2LCJpc3MiOiJhdXRoLmRvY2tlci5pbyIsImp0aSI6IlBXQTA4WXFGNXlYS1ZJbUF0RlIxIiwibmJmIjoxNjAzOTkxNzU2LCJzdWIiOiIifQ.L9iGizATKmEr-Y8BkgIYuCFtwUFiwjmKDJBzwymhupea-btEdtPWdvCbhiV0QFlpK4ZivaJbFuAK4ni4U5ijBkb9GrQVF3KLfViKc48Jle84aPFXM06h2-YqCajF5CoulHkGIylrfd2eKevw_-h4ERlpwfS_eyRvnoYh70fqJPHcpGzfKlPyEqXVY1pBd3wLrST0VRutG0GAhSfbVwqoxHmUrOa_GXHd2aCRLWvOvk5IsSFXvofZihpj9g4MA3ClFvpH1HkK1p4BMOs9QD4UxL4Y4Yly-6werTRqDK19IfjKrcqPB9mQydoJgWOMH5dNraBpTgtjYUHsGm8sBXTsHg
>
< HTTP/1.1 429 Too Many Requests
< Cache-Control: no-cache
< Connection: close
< Content-Type: application/json
< Retry-After: 60
<
{
  "details": "Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/",
  "error": {
    "details": "Too Many Requests. Please see https://docs.docker.com/docker-hub/download-rate-limit/"
  }
}
* TLSv1.2 (IN), TLS alert, close notify (256):
* Closing connection 0
* TLSv1.2 (OUT), TLS alert, close notify (256):

[cpuguy83]

Yes, the fetcher is looking for an array.

[cpuguy83]

I opened #4674 with the assumption it would just be an unformatted string, but can update fix decoding the type.

[thaJeztah]

Thanks @ingshtrom ! Let me copy from our slack chat;

Looks like the expected format is;

{
        "errors": [
            {
                "code": "<error identifier, see below>",
                "message": "<message describing condition>",
                "detail": "<unstructured>"
            }
        ]
}

So a top-level "errors" field with an array or errors, each with a code, message and detail field.

Reading the spec;

the registry MAY return a body in any format
If the response body is in JSON format, it MUST have the following format:

I'm not sure what the expectation is for "invalid formatted JSON" (fallback to print the content verbatim, as a "non-standard" JSON, or something else?)

[cpuguy83]

So is it a DockerHub bug that it's not sending an array?

[dmcgowan]

If it is common for other registries to return JSON in that format, its not a big deal to support both formats. In this case, DockerHub should probably use the standard format.

I think not returning any detail text from non-JSON responses is the right approach though.

[cpuguy83]

So the current error returns a details instead of detail.
So it still doesn't match if we try to only unmarshal a single error.

[thaJeztah]

Yes, for this case it's a bug on Docker Hub side; they're looking into that (also not sure where the format came from on their side; we'll have to check if the same incorrect format is used for other errors as well).

IIUC, this particular error message is not generated by the registry itself, but the proxy in front of it handling rate limits, so I guess other registries would not return the same format.

That said (as I mentioned above); I'm curious where this particular format originated from, and if there's other occurrences (was it a format returned by v1 registries?)

Also curious how the docker daemon was able to convert it into a proper error message (will have to dig into the code for that, but perhaps someone else knows)

[cpuguy83]

Sure enough, the dist registry client has this:

	// For backward compatibility, handle irregularly formatted
	// messages that contain a "details" field.
	var detailsErr struct {
		Details string `json:"details"`
	}

[thaJeztah]

I think not returning any detail text from non-JSON responses is the right approach though.

Not sure about that; I think servers returning a plain text error message would be more common (and described by the distribution spec as "valid"), or is that not what you meant?

[cpuguy83]

I also agree with the plain text approach. For something like 429 specifically the registry may be in a degraded state as it is and serving out plain text instead.

[cpuguy83]

Looks like there's another bug in the unmarshaller and maybe why I never see error details from containerd.

From MarshalJSON:

containerd/remotes/docker/errcode.go

Lines 219 to 221 in 5184bcc

	var tmpErrs struct {
	Errors []Error `json:"errors,omitempty"`
	}

And UnmarshalJSON:

containerd/remotes/docker/errcode.go

Lines 256 to 258 in 5184bcc

	var tmpErrs struct {
	Errors []Error
	}

The dist errcode package specifies the json as errors like MarshalJSON does above.

[cpuguy83]

Also doesn't really matter, because the error "details" are not part of the error string, only the code and the "message".

[kzys]

Docker Hub is fine. It returns the below now.

{
  "errors": [
    {
      "code": "TOOMANYREQUESTS",
      "message": "You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit"
    }
  ]
}