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.

Sign up for GitHub

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

Jump to bottom

pkg/docker: lock auth.json before reading/writing #1506

Closed
wants to merge 1 commit into from
Closed

pkg/docker: lock auth.json before reading/writing #1506

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 41 additions & 1 deletion pkg/docker/config/config.go
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/containers/image/v5/pkg/sysregistriesv2"
"github.com/containers/image/v5/types"
"github.com/containers/storage/pkg/homedir"
"github.com/containers/storage/pkg/lockfile"
helperclient "github.com/docker/docker-credential-helpers/client"
"github.com/docker/docker-credential-helpers/credentials"
"github.com/hashicorp/go-multierror"
Expand Down Expand Up @@ -541,6 +542,29 @@ func getPathToAuthWithOS(sys *types.SystemContext, goOS string) (string, bool, e
// or returns an empty dockerConfigFile data structure if auth.json does not exist
// if the file exists and is empty, readJSONFile returns an error
func readJSONFile(path string, legacyFormat bool) (dockerConfigFile, error) {
if _, err := os.Stat(path); err != nil {
if os.IsNotExist(err) {
var auths dockerConfigFile
auths.AuthConfigs = map[string]dockerAuthConfig{}
return auths, nil
}
return dockerConfigFile{}, err
}

// Make sure to read-lock the file before reading it.
lock, err := lockfile.GetLockfile(path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn’t the path parameter a path of the lock? Which contains a random value to support .Modified? How can auth.json be a JSON and a random string at the same time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Touch() and Modified() don't work in this scenario but I think that's OK in this scenario since we're controlling it here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I have not been reading that carefully enough; the contents are not actually used if we call neither Touch() nor Modified().

Still, that feels like an implementation detail that could change any time. I suppose our tests would catch it…

if err != nil {
return dockerConfigFile{}, fmt.Errorf("creating lock: %w", err)
}
lock.RLock()
defer lock.Unlock()

return readJSONFileLocked(path, legacyFormat)
}

// Implements readJSONFile but without locking the specified path.
// The caller is responsible for synchronizing access to the path.
func readJSONFileLocked(path string, legacyFormat bool) (dockerConfigFile, error) {
var auths dockerConfigFile

raw, err := ioutil.ReadFile(path)
Expand All @@ -552,6 +576,14 @@ func readJSONFile(path string, legacyFormat bool) (dockerConfigFile, error) {
return dockerConfigFile{}, err
}

// When creating a file for the first time, it may be empty at this
// point as it was created by the write lock. Hence, return early here
Copy link
Collaborator

Choose a reason for hiding this comment

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

That’s surprising to me (and incompatible with older versions of consumers, though that’s a small concern — they could easily race with WriteFile and see an empty file, and fail, for that reason).

See elsewhere about sharing the lock with JSON.

Copy link
Member Author

@vrothberg vrothberg Mar 25, 2022 •
edited

Choose a reason for hiding this comment

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

I think it's a valid concern but I do not have a good/definitive answer/idea. We could use a central one (similar to the short-names lock) but old client would still compete with newer ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about using a separate auth.json.lock file; then the concerns about the lock file object trying to own file contents would go away, and so would this situation, AFAICS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The underlying fcntl lock is unlocked when any file descriptor to that file is closed, isn’t it?

If so, ioutil.ReadFile of the JSON always releases the lock, making the modifyJSON lock ineffective — and we do need to use a different path.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about using a separate auth.json.lock file; then the concerns about the lock file object trying to own file contents would go away, and so would this situation, AFAICS.

I was considering that but didn't like the user experience. I would find it surprising/annoying if I do a --authfile file and would find a file.lock afterwards.

The underlying fcntl lock is unlocked when any file descriptor to that file is closed, isn’t it?

Holy frog, that is true and buried in the man pages. That indeed is a K.O. for this approach - we could do ref-counting on the path but that seems a bit too hacky for my taste.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be explicit,pkg/lockfile is already using a global hash table and refcounting the objects to work correctly with those locks, so that works for exclusively-used lockfiles (and as long as c/storage doesn’t get a separate /v2 with a separate hash table); but it breaks if there is any other user of that file, like here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

… the point about file.lock files on user-specified --authfile paths (outside of dot files, in user-maintained project repositories and the like) being annoying is quite valid, though; we might not even necessarily be able to create a lockfile (if the auth.json is in a read-only configuration directory).

flock(2) might be an option, but it is distinctly less portable (and the Linux man pages suggest that some implementations implement it on top of the fcntl locks. I’m sure there are good write-ups about the various locking mechanisms and their trade-offs, I haven’t now taken the time to find and read one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A possible partial alternative: use c/storage/pkg/ioutils.AtomicWriteFile. That could still lose data with concurrent writers, but at least it wouldn’t break readers concurrent with a single writer, who can now see empty files or partial contents (= invalid JSON).

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like we're between a rock and hard place. I will close this PR as I ran out of time and really do not have a good idea yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let’s at least do the AtomicWriteFile part, that should be safe and make some difference: #1515 .

// to prevent an unmarshal error; an empty file is not valid JSON.
if len(raw) == 0 {
auths.AuthConfigs = map[string]dockerAuthConfig{}
return auths, nil
}

if legacyFormat {
if err = json.Unmarshal(raw, &auths.AuthConfigs); err != nil {
return dockerConfigFile{}, errors.Wrapf(err, "unmarshaling JSON at %q", path)
Expand Down Expand Up @@ -590,7 +622,15 @@ func modifyJSON(sys *types.SystemContext, editor func(auths *dockerConfigFile) (
return "", err
}

auths, err := readJSONFile(path, false)
// Make sure to write-lock the file before writing to it.
lock, err := lockfile.GetLockfile(path)
if err != nil {
return "", fmt.Errorf("creating lock: %w", err)
}
lock.Lock()
defer lock.Unlock()

auths, err := readJSONFileLocked(path, false)
if err != nil {
return "", errors.Wrapf(err, "reading JSON file %q", path)
}
Expand Down