Skip to content

Conversation

austinvazquez
Copy link
Contributor

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 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() is currently hard-coded to expect JSON requests, and unconditionally calls encodeData, 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

(cherry picked from commit b1ce0c8)

- What I did
Backports #50415 to 28.x branch

- How I did it

git cherry-pick -xsS b1ce0c89f0214cc6711c5c34e714d8bda737c65a

* 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)

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>
@thaJeztah
Copy link
Member

Failure is unrelated; I think we have a fix for that one as well in master;

=== Failed
=== FAIL: github.com/docker/docker/integration/container TestExecResize/success (0.06s)
    exec_test.go:144: assertion failed: error is not nil: Error response from daemon: NotFound: exec: '623838563a1cbf903b6f36dcb38efa5636211160ec08730d9c685c4e134d3909' in task: '479274abcace61a0c829e32786166892b8b99cb838ae5cd9f35c5a8e58cda385' not found: not found

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit bf6d688 into moby:28.x Jul 22, 2025
205 of 209 checks passed
@thaJeztah
Copy link
Member

We should probably consider including this in other maintained branches as well.

@thaJeztah
Copy link
Member

(and the PR that removes the fallback)

@austinvazquez
Copy link
Contributor Author

@thaJeztah, is this the fallback you are referencing?

{
doc: "empty",
input: AuthConfig{},
outBase64: `e30=`,
outPlain: `{}`,
},

@austinvazquez austinvazquez deleted the cherry-pick-b1ce0c89f0214cc6711c5c34e714d8bda737c65a-to-28.x branch July 22, 2025 13:47
@thaJeztah
Copy link
Member

@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 X-Registry-Auth header and did not send a body;

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants