-
Notifications
You must be signed in to change notification settings - Fork 18.8k
[28.x backport] client: always send (empty) body on push #50471
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
[28.x backport] client: always send (empty) body on push #50471
Conversation
Before ea29dff, the image create endpoint had a [fallback for very old client versions][1] that would send authentication as body instead of through the `X-Registry-Auth` header. However, the implementation of this fallback did not handle empty bodies, resulting in an `io.EOF` error to be returned when trying to parse the body as JSON. In practice, this problem didn't happen when using the CLI, because even if no authentication was present, `registry.EncodeAuthConfig()` (used by the CLI to set the `X-Registry-Auth` header) would produce an empty JSON document (`{}`), which would be encoded in base64 (`e30=`), so we would never set an empty `X-Registry-Auth` (but other clients may have hit this situation). That behavior was unexpected, because not all registries require authentication, and omitting the `X-Registry-Auth` should be valid. We also want to have more flexibility in authentication (and being able to distinguish unauthenticated requests, so that we can fallback to alternative paths). Unfortunately, we can't change existing daemons, so must account for the faulty fallback. Currently, omitting the `X-Registry-Auth` produces an error, but we can avoid this by unconditionally sending a body, which may be an empty JSON document (`{}`). I explored possible options for this; we can either construct our own empty JSON (`json.RawMessage("{}")`) to be explicit that we're sending empty JSON, but [`encodeBody()`][2] is currently hard-coded to expect JSON requests, and unconditionally calls [`encodeData`][3], which encodes to JSON, so we may as well take advantage of `http.NoBody`, which gets marshaled to an empty JSON document; https://go.dev/play/p/QCw9dJ6LGQu package main import ( "encoding/json" "fmt" "net/http" ) func main() { body, _ := json.Marshal(http.NoBody) fmt.Println(string(body)) } Before this patch, a client omitting `X-Registry-Auth` (and no body) would produce an error; docker pull -q busybox docker tag busybox 127.0.0.1:5001/myimage:latest docker run -d --name registry -p 127.0.0.1:5001:5000 registry:3 docker push 127.0.0.1:5001/myimage:latest Error response from daemon: bad parameters and missing X-Registry-Auth: invalid X-Registry-Auth header: EOF With this patch applied, no error is produced; docker pull -q busybox docker tag busybox 127.0.0.1:5001/myimage:latest docker run -d --name registry -p 127.0.0.1:5001:5000 registry:3 docker push 127.0.0.1:5001/myimage:latest The push refers to repository [127.0.0.1:5001/myimage] 189fdd150837: Pushed latest: digest: sha256:68a0d55a75c935e1101d16ded1c748babb7f96a9af43f7533ba83b87e2508b82 size: 610 [1]: https://github.com/moby/moby/blob/63fcf7d8582bf901b912015db5a590186710b8c6/api/types/registry/authconfig_test.go#L109-L114 [2]: https://github.com/moby/moby/blob/63fcf7d8582bf901b912015db5a590186710b8c6/client/request.go#L67-L87 [3]: https://github.com/moby/moby/blob/63fcf7d8582bf901b912015db5a590186710b8c6/client/request.go#L296-L304 [4]: moby@ea29dff Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit b1ce0c8) Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
Failure is unrelated; I think we have a fix for that one as well in master;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We should probably consider including this in other maintained branches as well. |
(and the PR that removes the fallback) |
@thaJeztah, is this the fallback you are referencing? moby/api/types/registry/authconfig_test.go Lines 109 to 114 in 63fcf7d
|
@austinvazquez no it's this one on the daemon side; that's the one that was causing issues if a request was made that did not send a non-empty Related to that was this PR that tried to omit the header if no value was set (but that didn't work with the existing way we encoded the value, so it was never empty); I DONT think we need to (or should!) backport that last one, but it's good if the daemon accepts requests with an empty body without producing an obscure error (which is what #50371 should fix). |
Before ea29dff, the image create endpoint had a fallback for very old client versions that would send authentication as body instead of through the
X-Registry-Auth
header.However, the implementation of this fallback did not handle empty bodies, resulting in an
io.EOF
error to be returned when trying to parse the body as JSON.In practice, this problem didn't happen when using the CLI, because even if no authentication was present,
registry.EncodeAuthConfig()
(used by the CLI to set theX-Registry-Auth
header) would produce an empty JSON document ({}
), which would be encoded in base64 (e30=
), so we would never set an emptyX-Registry-Auth
(but other clients may have hit this situation). That behavior was unexpected, because not all registries require authentication, and omitting theX-Registry-Auth
should be valid. We also want to have more flexibility in authentication (and being able to distinguish unauthenticated requests, so that we can fallback to alternative paths).Unfortunately, we can't change existing daemons, so must account for the faulty fallback. Currently, omitting the
X-Registry-Auth
produces an error, but we can avoid this by unconditionally sending a body, which may be an empty JSON document ({}
).I explored possible options for this; we can either construct our own empty JSON (
json.RawMessage("{}")
) to be explicit that we're sending empty JSON, butencodeBody()
is currently hard-coded to expect JSON requests, and unconditionally callsencodeData
, which encodes to JSON, so we may as well take advantage ofhttp.NoBody
, which gets marshaled to an empty JSON document;https://go.dev/play/p/QCw9dJ6LGQu
Before this patch, a client omitting
X-Registry-Auth
(and no body) would produce an error;With this patch applied, no error is produced;
(cherry picked from commit b1ce0c8)
- What I did
Backports #50415 to 28.x branch
- How I did it
* manually resolved conflicts in
tryImagePush
function. i.e. patch did not apply cleanly.- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)