dockerConfigFile.GetCredentialsStore.Store faulty symlink resolution behavior

apostasie commented 2 months ago

Description

When DOCKER_CONFIG points to a directory that does contain a relative, ~~dangling~~ config.json symlink, Docker credentials store will (apparently) wrongly resolve the link to the current working directory.

This can be reproduced with the following:

mkdir -p /tmp/foo
ln -s doesnotexist /tmp/foo/config.json
cd ~
DOCKER_CONFIG=/tmp/foo nerdctl login
cat ~/doesnotexist
ln -s /tmp/foo

This does suggest there is a bug in moby/docker somewhere where readlink is used to resolve against pwd instead of the parent dir (/tmp/foo).

In turn, if, for some reason, the file cannot be created in the current working directory (for example if it is readonly), this will error in a very baffling way with a very confusing message:

rename /tmp/TestBrokenCredentialsStore705218110/008/docker-config2430087685/config.json2911426040 doesnotexist: invalid cross-device link

First spotted in https://github.com/containerd/nerdctl/pull/3293#pullrequestreview-2231972713 although at the time I was unable to diagnose it.

While this is very likely a docker bug, we need to fix these tests to deal with that.

Steps to reproduce the issue

No response

Describe the results you received and expected

na

What version of nerdctl are you using?

main

Are you using a variant of nerdctl? (e.g., Rancher Desktop)

None

Host information

No response

apostasie commented 2 months ago

This ticket is both about making sure our tests work (fixed by the linked PR), but we could keep it around as "external" for now as well - or if somebody wants to, we might further diagnose it and report it to moby.

apostasie commented 2 months ago

THIS

https://github.com/docker/cli/blob/master/cli/config/configfile/file.go#L172-L174

apostasie commented 2 months ago

@thaJeztah you might want to fix this ^.

Readlink does not resolve the path for you, and by the time you write it, it will be relative to cwd.

You might consider using instead filepath.EvalSymlinks (on the full path) - but then, this will error if the target does not exist, so, you have to account for that as well... maybe just os.WriteFile("") the file to ensure it exists... will work in some cases...

But then... you do MkdirAll before resolving the links (https://github.com/docker/cli/blob/master/cli/config/configfile/file.go#L144-L147), so... you will fail mkdir-ing where the link(s) actually resolve...

I certainly appreciate that an atomic write is the right thing to do, but in that case, you just cannot get away with it with just readlink...

This is not exactly trivial...

Let me know if I am missing something and/or if there is a simpler way to deal with this in go...

thaJeztah commented 2 months ago

@apostasie thanks! Yes, looks like a tricky one. Could you open a ticket for this in docker/cli ?

apostasie commented 2 months ago

@apostasie thanks! Yes, looks like a tricky one. Could you open a ticket for this in docker/cli ?

Sure, will do eventually - it might take me some time though (busy with here), so feel free to carry it if you do have bandwidth now.

Thanks @thaJeztah !

containerd / nerdctl