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

Docker APIv2 build endpoint bug with X-Registry-Config #11235

Closed
matejvasek opened this issue Aug 16, 2021 · 22 comments · Fixed by #11430
Closed

Docker APIv2 build endpoint bug with X-Registry-Config #11235

matejvasek opened this issue Aug 16, 2021 · 22 comments · Fixed by #11430
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@matejvasek
Copy link
Contributor

Compat build endpoint is not working if user config file ~/.docker/config contains server key like https://index.docker.io/v1/.
It fails with message:

Error response from daemon: failed to parse query parameter 'X-Registry-Config': "n/a": error storing credentials in temporary auth file (server: "https://index.docker.io/v1/", user: "mvasek"): key https://index.docker.io/v1/ contains http[s]:// prefix

This bug was introduced in #11028.

@matejvasek
Copy link
Contributor Author

/kind bug

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 16, 2021
@matejvasek
Copy link
Contributor Author

/cc @vrothberg

@matejvasek
Copy link
Contributor Author

to reproduce run

curl -XPOST --unix-socket {YOUR SOCKET HERE} "http://d/v1.40/build?buildargs=%7B%7D&cachefrom=%5B%5D&cgroupparent=&\
cpuperiod=0&cpuquota=0&cpusetcpus=&cpusetmems=&cpushares=0&dockerfile=Dockerfile\
&labels=%7B%7D&memory=0&memswap=0&networkmode=NetworkDefault&rm=1&shmsize=0&target=&ulimits=null&version=1" \
 -H "Content-Type: application/x-tar" \
 -H "X-Registry-Config: eyAiaHR0cHM6Ly9pbmRleC5kb2NrZXIuaW8vdjEvIjogeyAidXNlcm5hbWUiOiAibXZhc2VrIiwgInBhc3N3b3JkIjogImlkZHFkIiwgInNlcnZlcmFkZHJlc3MiOiAiaHR0cHM6Ly9pbmRleC5kb2NrZXIuaW8vdjEvIiB9IH0K" \
 --data-binary '@test.tar'

Where test.tar contains some simple Dockerfile.

@vrothberg
Copy link
Member

@saschagrunert @mtrmac PTAL

@saschagrunert
Copy link
Member

@matejvasek would it be possible to get the output of debug.PrintStack() if you put it there?

return isNamespaced, errors.Errorf("key %s contains http[s]:// prefix", key)

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 17, 2021

That’s almost certainly

if err := imageAuth.SetAuthentication(&sys, server, config.Username, config.Password); err != nil {
.

Hum… Looking at that package, arguably containers/image#588 (turning c/image/pkg/docker/config into a broad API exposing much of the parsers/formatters, independently from c/image uses) has a point, even if the price might be not something we have been ready to pay.

At a very quick glance, is it possible that pkg/auth/auth is working way too hard, when it could instead point SystemContext directly at some files? Probably not quite…

But it does seem to me, after a somewhat slower skim, that authConfigToAuthFile could start with the user-provided JSON, instead of manually parsing it, and only adjusting it in exceptional cases.

Copy&pasting the key normalization into pkg/auth/auth, so that it always calls SetAuthentication with a valid key, should always be possible.


(I guess the first step should be to consciously decide whether those HTTP headers are expected to support the new repo-scoped credentials. Is this compatibility only, or used for new callers as well?)

@ssbarnea
Copy link
Collaborator

ssbarnea commented Sep 1, 2021

I am facing this problem with podman 3.3.0 (1629488174) on macos but not with the same version on fedora. Sounds like critical to me...

I tried pulling from both docker.io and quay.io and I get the same kind of failure from both.

@matejvasek
Copy link
Contributor Author

@ssbarnea as a workaround you try changing https://index.docker.io/v1/ -> docker.io in your configfile or keyring/credstore.

@vrothberg
Copy link
Member

@saschagrunert PTAL

@ssbarnea
Copy link
Collaborator

ssbarnea commented Sep 1, 2021

@matejvasek Can you please be a little bit more explicit? I checked and there is no registry configured inside ~/.config/containers/containers.conf and I have no container-registries.conf file on disk. I suppose there is an implicit one but I was not able to find it on https://github.com/containers/image/blob/main/docs/containers-registries.conf.5.md

@matejvasek
Copy link
Contributor Author

I think that auth.json is stored somewhere in $XDG_RUNTIME_DIR.

@matejvasek
Copy link
Contributor Author

@ssbarnea Something like /run/user/1000/containers/auth.json

@ssbarnea
Copy link
Collaborator

ssbarnea commented Sep 1, 2021

On macos there is no $XDG_RUNTIME_DIR but on remote host (using the same user has podman remote connection use), I did:

ls $XDG_RUNTIME_DIR/containers/
overlay  overlay-containers  overlay-layers  overlay-locks

So there is no auth.json either. I suppose that the auth.json file is created only when someone is doing a login. Out of curiosity I manually did an login and the file appeared after.

I also did the same on my macos box and the file was created at ~/.config/containers/auth.json with the same content. Still on macos even after that doing podman pull still fails.

My macos containers.conf file looks like:

[containers]
  log_size_max = -1
  pids_limit = 2048
  userns_size = 65536

[engine]
  num_locks = 2048
  active_service = "leno"
  stop_timeout = 10
  [engine.service_destinations]
    [engine.service_destinations.leno]
      uri = "ssh://root@leno:22/run/podman/podman.sock"

[network]

@matejvasek
Copy link
Contributor Author

Mabybe it stores you credentials in some docker credentials helper (secure store) not plaintext in json file.

@mheon
Copy link
Member

mheon commented Sep 1, 2021

podman login --verbose on 3.3.0 and up should print where we stored the credentials.

@matejvasek
Copy link
Contributor Author

@saschagrunert

goroutine 24 [running]:
runtime/debug.Stack(0x1, 0x1b188e0, 0x1)
	/usr/lib/golang/src/runtime/debug/stack.go:24 +0x9f
runtime/debug.PrintStack()
	/usr/lib/golang/src/runtime/debug/stack.go:16 +0x25
github.com/containers/image/v5/pkg/docker/config.validateKey(0xc0002ca5a0, 0x1b, 0x1c2d2fd, 0x31, 0xc00091dd38)
	/home/mvasek/go/src/github.com/containers/podman/vendor/github.com/containers/image/v5/pkg/docker/config/config.go:758 +0x3f
github.com/containers/image/v5/pkg/docker/config.SetCredentials(0xc00091df20, 0xc0002ca5a0, 0x1b, 0xc000160da8, 0x6, 0xc0007ea4e0, 0x24, 0xc0002ca5c0, 0x1c, 0x0, ...)
	/home/mvasek/go/src/github.com/containers/podman/vendor/github.com/containers/image/v5/pkg/docker/config/config.go:64 +0x50
github.com/containers/image/v5/pkg/docker/config.SetAuthentication(...)
	/home/mvasek/go/src/github.com/containers/podman/vendor/github.com/containers/image/v5/pkg/docker/config/config.go:121
github.com/containers/podman/v3/pkg/auth.authConfigsToAuthFile(0xc00066c268, 0xc000630540, 0x1bc9814, 0x8, 0x29c3e60)
	/home/mvasek/go/src/github.com/containers/podman/pkg/auth/auth.go:265 +0x256
github.com/containers/podman/v3/pkg/auth.getConfigCredentials(0xc0007c1900, 0xc0004c6cc0, 0x1bd88c3, 0x11, 0xc0006b8928, 0xc000656001)
	/home/mvasek/go/src/github.com/containers/podman/pkg/auth/auth.go:100 +0x4a6
github.com/containers/podman/v3/pkg/auth.GetCredentials(0xc0007c1900, 0xc000630420, 0x1bc2cd4, 0x4, 0x29c3e60, 0x0, 0x0, 0x1)
	/home/mvasek/go/src/github.com/containers/podman/pkg/auth/auth.go:39 +0x14c
github.com/containers/podman/v3/pkg/api/handlers/compat.BuildImage(0x1ea4070, 0xc0006140e0, 0xc0007c1900)
	/home/mvasek/go/src/github.com/containers/podman/pkg/api/handlers/compat/images_build.go:392 +0xe66
github.com/containers/podman/v3/pkg/api/server.(*APIServer).APIHandler.func1.2(0x1ea4070, 0xc0006140e0, 0xc0007c1800)
	/home/mvasek/go/src/github.com/containers/podman/pkg/api/server/handler_api.go:72 +0x103a
github.com/containers/podman/v3/pkg/api/server.(*APIServer).APIHandler.func1(0x1ea4070, 0xc0006140e0, 0xc0007c1800)
	/home/mvasek/go/src/github.com/containers/podman/pkg/api/server/handler_api.go:75 +0xa6
net/http.HandlerFunc.ServeHTTP(0xc000309dd0, 0x1ea4070, 0xc0006140e0, 0xc0007c1800)
	/usr/lib/golang/src/net/http/server.go:2049 +0x44
github.com/gorilla/mux.(*Router).ServeHTTP(0xc0007f8000, 0x1ea4070, 0xc0006140e0, 0xc0007c0900)
	/home/mvasek/go/src/github.com/containers/podman/vendor/github.com/gorilla/mux/mux.go:210 +0xd3
net/http.serverHandler.ServeHTTP(0xc00011a000, 0x1ea4070, 0xc0006140e0, 0xc0007c0900)
	/usr/lib/golang/src/net/http/server.go:2867 +0xa3
net/http.(*conn).serve(0xc000612000, 0x1eac8a8, 0xc000050080)
	/usr/lib/golang/src/net/http/server.go:1932 +0x8cd
created by net/http.(*Server).Serve
	/usr/lib/golang/src/net/http/server.go:2993 +0x39b

@ssbarnea
Copy link
Collaborator

ssbarnea commented Sep 1, 2021

I managed to sort the problem locally by altering ~/.docker/config.json file:

{
  "stackOrchestrator" : "swarm",
  "experimental" : "enabled",
  "credsStore" : "desktop",
  "auths" : {
    "trunk.registry.rdoproject.org" : {
    },
    "registry.redhat.io" : {
      "auth" : "..."
    }
  }
}

What i had to do was to remove all keys under auths that contained a full url, something like "https://index.docker.io/v1/": {} would break podman, which docker has no problem with it.

@songford
Copy link

songford commented Sep 2, 2021

I managed to sort the problem locally by altering ~/.docker/config.json file:

{
  "stackOrchestrator" : "swarm",
  "experimental" : "enabled",
  "credsStore" : "desktop",
  "auths" : {
    "trunk.registry.rdoproject.org" : {
    },
    "registry.redhat.io" : {
      "auth" : "..."
    }
  }
}

What i had to do was to remove all keys under auths that contained a full url, something like "https://index.docker.io/v1/": {} would break podman, which docker has no problem with it.

Thanks. This is exactly it.
My Podman instance on Mac didn't complain about X-Registry-Config, but X-Registry-Auth. Otherwise, everything else is exactly the same and deleting entries in auths inside ~/.docker/config.json fixes this issue.

@saschagrunert
Copy link
Member

So, we should not call validateKey() for docker config files from my point of view to keep the backwards compatibility. Do we want to mitigate this in Podman or c/image?

If just the path via authConfigsToAuthFile is affected, then we may normalize the server there:

podman/pkg/auth/auth.go

Lines 262 to 267 in 4207d95

// Note that we do not validate the credentials here. Wassume
// that all credentials are valid. They'll be used on demand
// later.
if err := imageAuth.SetAuthentication(&sys, server, config.Username, config.Password); err != nil {
return "", errors.Wrapf(err, "error storing credentials in temporary auth file (server: %q, user: %q)", server, config.Username)
}

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 2, 2021

c/image does allow URLs in config files; it rejects them in the SetAuthentication call (and it was never documented to be accepted, and it never worked for credential helpers as expected); so if pkg/auth turns config files into SetAuthentication calls, I think it’s up to pkg/auth to either transform the input to preserve semantics, or to change to use config files instead of SetAuthentication.

I don’t think c/image should just relax the SetAuthentication validation, accepting the inputs the way it used to never worked quite correctly. Some part of the fix should be in pkg/auth; it might turnout that c/image will need to be extended to support that well, I didn’t examine it that deeply.

@saschagrunert
Copy link
Member

/assign

@saschagrunert
Copy link
Member

Started to experiment with a possible fix in #11430, let's see how that works.

saschagrunert added a commit to saschagrunert/podman that referenced this issue Sep 9, 2021
Recent changes in c/image caused the `SetAuthentication` API to be more
restrictive in terms of validating the `key` (`server`) input. To ensure
that manually modified or entries in `~/.docker/config.json` still work,
we now strip the leading `http[s]://` prefix.

Fixes containers#11235

Signed-off-by: Sascha Grunert <[email protected]>
mheon pushed a commit to mheon/libpod that referenced this issue Sep 20, 2021
Recent changes in c/image caused the `SetAuthentication` API to be more
restrictive in terms of validating the `key` (`server`) input. To ensure
that manually modified or entries in `~/.docker/config.json` still work,
we now strip the leading `http[s]://` prefix.

Fixes containers#11235

Signed-off-by: Sascha Grunert <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants