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

Fixes #333 Handle parallel calls to add with osxkeychain #334

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes #333 Handle parallel calls to add with osxkeychain #334

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion osxkeychain/osxkeychain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import (
// when the credentials are not in the keychain.
const errCredentialsNotFound = "The specified item could not be found in the keychain."

// errCredentialsAlreadyExist is the specific error message returned by OS X
// when the credentials are already in the keychain.
const errCredentialsAlreadyExist = "The specified item already exists in the keychain."

// errCredentialsNotFound is the specific error message returned by OS X
// when environment does not allow showing dialog to unlock keychain.
const errInteractionNotAllowed = "User interaction is not allowed."
Expand Down Expand Up @@ -54,7 +58,16 @@ func (h Osxkeychain) Add(creds *credentials.Credentials) error {
errMsg := C.keychain_add(s, label, username, secret)
if errMsg != nil {
defer C.free(unsafe.Pointer(errMsg))
return errors.New(C.GoString(errMsg))
switch goMsg := C.GoString(errMsg); goMsg {
case errCredentialsAlreadyExist:
// If docker login is called in parallel, we may try to
// save the same credentials twice. In that case, return
// ok, because although we lost the race, the desired
// credentials were saved by another process.
return nil
Comment on lines +62 to +67
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit on the fence if this is the right thing to do here. While the related ticket is about the same credentials being stored concurrently, it's also possible that different credentials are being stored for the same registry, in which case an error may be most appropriate.

Perhaps we could add a ErrConflict for this, and a utility to detect that, so that a client could choose to handle it;

docker-credential-helpers/credentials/error.go

Lines 100 to 111 in db1da9d

// IsCredentialsMissingServerURL returns true if the error
// was an errCredentialsMissingServerURL.
func IsCredentialsMissingServerURL(err error) bool {
var target errCredentialsMissingServerURL
return errors.As(err, &target)
}
// IsCredentialsMissingServerURLMessage checks for an
// errCredentialsMissingServerURL in the error message.
func IsCredentialsMissingServerURLMessage(err string) bool {
return strings.TrimSpace(err) == errCredentialsMissingServerURLMessage
}

@laurazard any thoughts?;

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, took me a while to work my way here. I'll TAL.

Copy link
Member

Choose a reason for hiding this comment

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

My intuition is that we should handle this a la EINTR, i.e. "things happened while your code was blocked on some call and you should handle it".

Otherwise, like @thaJeztah mentioned, we have no guarantees that the credentials saved were the desired ones by this process.

Copy link
Member

Choose a reason for hiding this comment

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

@beejeebus what do you think?

default:
return errors.New(goMsg)
}
}

return nil
Expand Down