From fcf5b74e59c1c0d18a8e8e939475007b3b5f83ad Mon Sep 17 00:00:00 2001 From: Koji Nakamaru Date: Wed, 15 May 2024 19:21:06 +0000 Subject: [PATCH 1/2] osxkeychain: exclusive lock to serialize execution of operations git passes a credential that has been used successfully to the helpers to record. If "git-credential-osxkeychain store" commands run in parallel (with fetch.parallel configuration and/or by running multiple git commands simultaneously), some of them may exit with the error "failed to store: -25299". This is because SecItemUpdate() in add_internet_password() may return errSecDuplicateItem (-25299) in this situation. Apple's documentation [1] also states as below: In macOS, some of the functions of this API block while waiting for input from the user (for example, when the user is asked to unlock a keychain or give permission to change trust settings). In general, it is safe to use this API in threads other than your main thread, but avoid calling the functions from multiple operations, work queues, or threads concurrently. Instead, serialize function calls or confine them to a single thread. The error has not been noticed before, because the former implementation ignored the error. Introduce an exclusive lock to serialize execution of operations. [1] https://developer.apple.com/documentation/security/certificate_key_and_trust_services/working_with_concurrency Signed-off-by: Koji Nakamaru Signed-off-by: Junio C Hamano --- contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 6a40917b1e..0884db48d0 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -414,6 +414,9 @@ int main(int argc, const char **argv) if (!argv[1]) die("%s", usage); + if (open(argv[0], O_RDONLY | O_EXLOCK) == -1) + die("failed to lock %s", argv[0]); + read_credential(); if (!strcmp(argv[1], "get")) From e1ab45b2dab51f94db9548666dfd7af626d2aa7e Mon Sep 17 00:00:00 2001 From: Koji Nakamaru Date: Wed, 15 May 2024 19:21:07 +0000 Subject: [PATCH 2/2] osxkeychain: state to skip unnecessary store operations git passes a credential that has been used successfully to the helpers to record. If a credential is already stored, "git-credential-osxkeychain store" just records the credential returned by "git-credential-osxkeychain get", and unnecessary (sometimes problematic) SecItemAdd() and/or SecItemUpdate() are performed. We can skip such unnecessary operations by marking a credential returned by "git-credential-osxkeychain get". This marking can be done by utilizing the "state[]" feature: - The "get" command sets the field "state[]=osxkeychain:seen=1". - The "store" command skips its actual operation if the field "state[]=osxkeychain:seen=1" exists. Introduce a new state "state[]=osxkeychain:seen=1". Suggested-by: brian m. carlson Signed-off-by: Koji Nakamaru Signed-off-by: Junio C Hamano --- .../osxkeychain/git-credential-osxkeychain.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 0884db48d0..6ce22a28ed 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -12,6 +12,7 @@ static CFStringRef username; static CFDataRef password; static CFDataRef password_expiry_utc; static CFDataRef oauth_refresh_token; +static int state_seen; static void clear_credential(void) { @@ -171,6 +172,9 @@ static OSStatus find_internet_password(void) CFRelease(item); + write_item("capability[]", "state", strlen("state")); + write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1")); + out: CFRelease(attrs); @@ -284,6 +288,9 @@ static OSStatus add_internet_password(void) CFDictionaryRef attrs; OSStatus result; + if (state_seen) + return errSecSuccess; + /* Only store complete credentials */ if (!protocol || !host || !username || !password) return -1; @@ -395,6 +402,10 @@ static void read_credential(void) oauth_refresh_token = CFDataCreate(kCFAllocatorDefault, (UInt8 *)v, strlen(v)); + else if (!strcmp(buf, "state[]")) { + if (!strcmp(v, "osxkeychain:seen=1")) + state_seen = 1; + } /* * Ignore other lines; we don't know what they mean, but * this future-proofs us when later versions of git do