osxkeychain: lock for exclusive execution #1729
Open
+14
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @KojiNakamaru, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
@dscho could you please /allow this PR? |
|
/allow |
|
User KojiNakamaru is now allowed to use GitGitGadget. |
|
/preview |
|
Preview email sent as pull.1729.git.1715327101566.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1729.git.1715328467099.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Bo Anderson wrote (reply to this): Interesting.
SecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398
The behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us.
I wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario?
A broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes.
Bo
> On 10 May 2024, at 09:07, Koji Nakamaru via GitGitGadget <gitgitgadget@gmail.com> wrote:
>
> From: Koji Nakamaru <koji.nakamaru@gree.net>
>
> Resolves "failed to store: -25299" when "fetch.parallel 0" is configured
> and there are many submodules.
>
> The error code -25299 (errSecDuplicateItem) may be returned by
> SecItemUpdate() in add_internet_password() if multiple instances of
> git-credential-osxkeychain run in parallel. This patch introduces an
> exclusive lock to serialize execution for avoiding this and other
> potential issues.
>
> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
> ---
> osxkeychain: lock for exclusive execution
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1729
>
> 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 6a40917b1ef..0884db48d0a 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"))
>
> base-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6
> --
> gitgitgadget
|
|
On the Git mailing list, Koji Nakamaru wrote (reply to this): Thank you for detailed insights.
> SecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398
> The behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us.
> I wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario?
I tested osxkeychain with the modification at the end of this note and
got the following log for
"git fetch --all --prune --recurse-submodules". SecItemUpdate()
sometimes returns
errSecDuplicateItem but the keychain item seems okay -- its value is
correct after the command
finished -- perhaps because one of successful operations stores the
correct value. Even if every
store operation fails, perhaps the originally stored value is kept and
no damage occurs.
XXX: get
XXXX: protocol=https
XXXX: host=github.com
XXXX: wwwauth[]=Basic realm="GitHub"
XXX: store
XXXX: protocol=https
XXXX: host=github.com
XXXX: username=jenkins
XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
XXXX: -25299
XXXXX: 0
XXX: get
XXXX: protocol=https
XXXX: host=github.com
XXXX: wwwauth[]=Basic realm="GitHub"
XXX: store
XXXX: protocol=https
XXXX: host=github.com
XXXX: username=jenkins
XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
XXXX: -25299
XXXXX: 0
XXX: get
XXXX: protocol=https
XXXX: host=github.com
XXXX: wwwauth[]=Basic realm="GitHub"
XXX: store
XXXX: protocol=https
XXXX: host=github.com
XXXX: username=jenkins
XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
XXXX: -25299
XXXXX: 0
XXX: get
XXXX: protocol=https
XXXX: host=github.com
XXXX: wwwauth[]=Basic realm="GitHub"
XXX: store
XXXX: protocol=https
XXXX: host=github.com
XXXX: username=jenkins
XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
XXXX: -25299
XXXXX: -25299
failed to store: -25299
XXX: get
XXXX: protocol=https
XXXX: host=github.com
XXXX: wwwauth[]=Basic realm="GitHub"
XXX: store
XXXX: protocol=https
XXXX: host=github.com
XXXX: username=jenkins
XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
XXXX: -25299
XXXXX: 0
XXX: get
XXXX: protocol=https
XXXX: host=github.com
XXXX: wwwauth[]=Basic realm="GitHub"
XXX: store
XXXX: protocol=https
XXXX: host=github.com
XXXX: username=jenkins
XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
XXXX: -25299
XXXXX: -25299
failed to store: -25299
...
This issue however occurs only when fetch.parallel is configured. If
fetch.parallel is not
configured, we should not ignore any error (including
errSecDuplicateItem). Also, the above unstable
behaviour is essentially caused by running osxkeychain instances in
parallel where some of them
treat "get" and others treat "store". I've also considered treating
errSecDuplicateItem of
SecItemUpdate() as errSecSuccess, but ended up with the current patch
for these reasons.
> A broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes.
I agree on this point and would like to know the reason.
Koji Nakamaru
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 6a40917b1e..0373857731 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -308,10 +308,12 @@ static OSStatus add_internet_password(void)
NULL);
result = SecItemAdd(attrs, NULL);
+ fprintf(stderr, "XXXX: %d\n", result);
if (result == errSecDuplicateItem) {
CFDictionaryRef query;
query = CREATE_SEC_ATTRIBUTES(NULL);
result = SecItemUpdate(query, attrs);
+ fprintf(stderr, "XXXXX: %d\n", result);
CFRelease(query);
}
@@ -333,6 +335,7 @@ static void read_credential(void)
if (!strcmp(buf, "\n"))
break;
buf[line_len-1] = '\0';
+ fprintf(stderr, "XXXX: %s\n", buf);
v = strchr(buf, '=');
if (!v)
@@ -414,6 +417,7 @@ int main(int argc, const char **argv)
if (!argv[1])
die("%s", usage);
+ fprintf(stderr, "XXX: %s\n", argv[1]);
read_credential();
if (!strcmp(argv[1], "get"))
2024年5月10日(金) 23:58 Bo Anderson <mail@boanderson.me>:
>
> Interesting.
>
> SecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398
>
> The behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us.
>
> I wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario?
>
> A broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes.
>
> Bo
>
> On 10 May 2024, at 09:07, Koji Nakamaru via GitGitGadget <gitgitgadget@gmail.com> wrote:
>
> From: Koji Nakamaru <koji.nakamaru@gree.net>
>
> Resolves "failed to store: -25299" when "fetch.parallel 0" is configured
> and there are many submodules.
>
> The error code -25299 (errSecDuplicateItem) may be returned by
> SecItemUpdate() in add_internet_password() if multiple instances of
> git-credential-osxkeychain run in parallel. This patch introduces an
> exclusive lock to serialize execution for avoiding this and other
> potential issues.
>
> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
> ---
> osxkeychain: lock for exclusive execution
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1729
>
> 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 6a40917b1ef..0884db48d0a 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"))
>
> base-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6
> --
> gitgitgadget
>
> |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, May 10, 2024 at 04:02:03PM +0100, Bo Anderson wrote:
> A broader Git-wide question that you perhaps don’t know the answer to
> but someone else here might do is: why are we spamming updates to the
> credential helper? Every parallel fetch instance performing a store
> operation on the same host seems unexpected to me, particularly if
> there’s no actual changes.
The short answer is that Git always passes a credential which has been
used successfully to the helpers to record (if they want to). That's how
stuff gets stored in the first place. And those parallel fetches have no
knowledge of what the other ones are doing, so they all try to store.
But the more interesting question is: why do we tell helpers to store a
credential that we got from helpers in the first place? The behavior is
mostly an artifact of how the original implementation behaved, as it did
not record the source of the credential.
And I think there are several problems with that, besides inefficiency
and locking. See this old patch, which fixes it by remembering when
a credential came from a helper:
https://lore.kernel.org/git/20120407033417.GA13914@sigill.intra.peff.net/
But we didn't merge it because some people rely on the behavior of
helpers feeding back to themselves. I outlined some solutions there, but
it would definitely be a change in behavior that people would have to
adapt to.
Some possible alternatives:
- we could remember _which_ helper we got the credential from, and
avoid invoking it again.
- we could record a bit saying that the credential came from a helper,
and then feed that back to helpers when storing. So osxkeychain
could then decide not to store it.
Both of those solve the repeated stores, but still let credentials
populate across helpers (which I still think is a questionable thing to
do by default, per the discussion in that thread, but is the very thing
that some people rely on).
-Peff |
|
User |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): On 2024-05-10 at 20:01:14, Jeff King wrote:
> And I think there are several problems with that, besides inefficiency
> and locking. See this old patch, which fixes it by remembering when
> a credential came from a helper:
>
> https://lore.kernel.org/git/20120407033417.GA13914@sigill.intra.peff.net/
>
> But we didn't merge it because some people rely on the behavior of
> helpers feeding back to themselves. I outlined some solutions there, but
> it would definitely be a change in behavior that people would have to
> adapt to.
>
> Some possible alternatives:
>
> - we could remember _which_ helper we got the credential from, and
> avoid invoking it again.
This will break the new `state[]` feature, which relies on being able to
see the state after the fact to know whether the operation was
successful. As an example of the functionality the current approach
allows, authentication could use an HOTP (like TOTP, but using a counter
instead of time) value, and storing the correct used counter on success
would be important.
I agree it's not super important if we're just using a username and
password, but considering I just added support for arbitrary
authentication schemes, which can include things such as limited-use
OAuth tokens, one-time use passcodes, and certain types of HMAC-based
signing, we probably don't want to choose this approach.
> - we could record a bit saying that the credential came from a helper,
> and then feed that back to helpers when storing. So osxkeychain
> could then decide not to store it.
This is actually possible with the new `state[]` feature. `osxkeychain`
can simply set that field to something like `osxkeychain:seen=1` and
simply do nothing if it sees that field.
All the credential helper needs to do is declare support for that
functionality with the appropriate capability and emit the field if it
gets that capability on standard input.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <peff@peff.net> writes:
> - we could remember _which_ helper we got the credential from, and
> avoid invoking it again.
>
> - we could record a bit saying that the credential came from a helper,
> and then feed that back to helpers when storing. So osxkeychain
> could then decide not to store it.
>
> Both of those solve the repeated stores, but still let credentials
> populate across helpers (which I still think is a questionable thing to
> do by default, per the discussion in that thread, but is the very thing
> that some people rely on).
Would "refreshing the last-time-used record" a valid use case for
the behaviour that feeds the successful one back to where the
credential came from? Such a helper could instead log the last-time
the credential was asked for, and assume that the lack of an explicit
"reject" call signals that the use of the value it returned earlier
was auccessfully used, but it is a less obvious way to implement
such a "this hasn't been successfully used for a long time, perhaps
we should expire/ask again/do something else?" logic. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, May 10, 2024 at 08:33:08PM +0000, brian m. carlson wrote:
> > Some possible alternatives:
> >
> > - we could remember _which_ helper we got the credential from, and
> > avoid invoking it again.
>
> This will break the new `state[]` feature, which relies on being able to
> see the state after the fact to know whether the operation was
> successful. As an example of the functionality the current approach
> allows, authentication could use an HOTP (like TOTP, but using a counter
> instead of time) value, and storing the correct used counter on success
> would be important.
>
> I agree it's not super important if we're just using a username and
> password, but considering I just added support for arbitrary
> authentication schemes, which can include things such as limited-use
> OAuth tokens, one-time use passcodes, and certain types of HMAC-based
> signing, we probably don't want to choose this approach.
Yeah, I think it makes sense to keep the Git side as general as
possible. So invoking the helper but giving it extra information (so it
can decide whether to be a noop or not) seems like the better approach.
> > - we could record a bit saying that the credential came from a helper,
> > and then feed that back to helpers when storing. So osxkeychain
> > could then decide not to store it.
>
> This is actually possible with the new `state[]` feature. `osxkeychain`
> can simply set that field to something like `osxkeychain:seen=1` and
> simply do nothing if it sees that field.
Makes sense. Back in that old thread I showed a patch which would let
helpers pass arbitrary fields to each other (or back to themselves), and
this works in roughly the same way.
> All the credential helper needs to do is declare support for that
> functionality with the appropriate capability and emit the field if it
> gets that capability on standard input.
If I understand the protocol, it is just:
printf("capability[]=state\n");
printf("state[]=osxkeychain:seen=1\n");
in the helper when it returns a username/password? And I guess the
matching parse/check on "store".
Sounds like that would be easy for folks on macOS to play with.
-Peff |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, May 10, 2024 at 01:40:29PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > - we could remember _which_ helper we got the credential from, and
> > avoid invoking it again.
> >
> > - we could record a bit saying that the credential came from a helper,
> > and then feed that back to helpers when storing. So osxkeychain
> > could then decide not to store it.
> >
> > Both of those solve the repeated stores, but still let credentials
> > populate across helpers (which I still think is a questionable thing to
> > do by default, per the discussion in that thread, but is the very thing
> > that some people rely on).
>
> Would "refreshing the last-time-used record" a valid use case for
> the behaviour that feeds the successful one back to where the
> credential came from? Such a helper could instead log the last-time
> the credential was asked for, and assume that the lack of an explicit
> "reject" call signals that the use of the value it returned earlier
> was auccessfully used, but it is a less obvious way to implement
> such a "this hasn't been successfully used for a long time, perhaps
> we should expire/ask again/do something else?" logic.
There was some discussion in that old thread about whether that was
important or not. I don't have a strong opinion there. Not refreshing is
a more secure default, but possibly more annoying (and a change from the
status quo).
I do think brian's suggestion to use state[] to pass it back means that
the decision is then in the hands of the helper. So "credential-cache",
for example, could decide whether to refresh its ttl or not, or we could
even make it configurable with a command-line option for the helper.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <peff@peff.net> writes:
> I do think brian's suggestion to use state[] to pass it back means that
> the decision is then in the hands of the helper. So "credential-cache",
> for example, could decide whether to refresh its ttl or not, or we could
> even make it configurable with a command-line option for the helper.
Yeah, I read your discussion with brian, and the state[] thing all
made sense to me.
Thanks. |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): On 2024-05-10 at 22:07:15, Jeff King wrote:
> On Fri, May 10, 2024 at 08:33:08PM +0000, brian m. carlson wrote:
> > All the credential helper needs to do is declare support for that
> > functionality with the appropriate capability and emit the field if it
> > gets that capability on standard input.
>
> If I understand the protocol, it is just:
>
> printf("capability[]=state\n");
> printf("state[]=osxkeychain:seen=1\n");
>
> in the helper when it returns a username/password? And I guess the
> matching parse/check on "store".
>
> Sounds like that would be easy for folks on macOS to play with.
Yup. It may receive `state[]` fields from other helpers, so it needs to
check that the entries are its own (presumably starting with
`osxkeychain:`) when it reads them, but otherwise, that's it.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA |
|
/submit |
|
Submitted as pull.1729.v2.git.1715428542.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -414,6 +414,9 @@ int main(int argc, const char **argv) | |||
| if (!argv[1]) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Koji Nakamaru <koji.nakamaru@gree.net>
>
> Resolves "failed to store: -25299" when "fetch.parallel 0" is configured
> and there are many submodules.
Use of third-person singular without subject for the "observation"
part is highly unusual the log messages in our codebase.
The usual way to compose a log message of this project is to
- Give an observation on how the current system work in the present
tense (so no need to say "Currently X is Y", just "X is Y"), and
discuss what you perceive as a problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to the codebase to "become like so".
in this order.
> The error code -25299 (errSecDuplicateItem) may be returned by
> SecItemUpdate() in add_internet_password() if multiple instances of
> git-credential-osxkeychain run in parallel. This patch introduces an
> exclusive lock to serialize execution for avoiding this and other
> potential issues.
"This patch introduces" -> "Introduce"
Is this step still needed, though?
> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
> ---
> 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 6a40917b1ef..0884db48d0a 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"))There was a problem hiding this comment.
On the Git mailing list, Koji Nakamaru wrote (reply to this):
Thank you for the instruction about a log message. I'll follow it.
> Is this step still needed, though?
For solving the issue I originally had, just utilizing state[] to skip
"store" operations is enough.
Since the osxkeychain implementation doesn't seem to be aware that it
can run in parallel, I thought it would be better to leave this step in
case a similar problem occurs. If this reason is weak, I'll remove this
step.
Koji Nakamaru
| @@ -12,6 +12,7 @@ static CFStringRef username; | |||
| static CFDataRef password; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Koji Nakamaru <koji.nakamaru@gree.net>
>
> Records whether credentials come from get operations and skips
> unnecessary store operations by utilizing the state[] feature, as
> suggested by brian m. carlson.
This step has a problem description that is even sketchier than the
previous one. Anticipate questions by the other developers who read
this commit 6 months after it is accepted in the mainline (e.g.,
What problem is there in the current system, why it is bad and worth
solving, and how is the patch trying to solve it?) and let your
proposed log message answer the questions, as you won't be always
sitting next to these developers.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Koji Nakamaru wrote (reply to this):
Thank you. I'll clean up the whole patch and its description after
getting a final reply about exlock.
Koji Nakamarugit 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 <koji.nakamaru@gree.net>
KojiNakamaru
force-pushed
the
feature/osxkeychian_exlock
branch
from
1f57718
to
b75f70a
Compare
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 <sandals@crustytoothpaste.net> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
KojiNakamaru
force-pushed
the
feature/osxkeychian_exlock
branch
from
b75f70a
to
146b0ae
Compare
|
/submit |
|
Submitted as pull.1729.v3.git.1715800868.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Koji Nakamaru wrote (reply to this): I thought about the issue further. The approach with the "state[]"
feature reduces problematic "store" operations a lot in most cases, it
is however not perfect to avoid the original issue about parallel
execution.
For example (though quite artificial), let's suppose if two git commands
start simultaneously where the credential is input automatically by
"expect" command. Two "osxkeychain store" commands will then run
simultaneously.
I hope new descriptions are convincing.
Koji Nakamaru |
|
This branch is now known as |
|
This patch series was integrated into seen via git@25b3e21. |
|
This patch series was integrated into seen via git@c65d42e. |
|
This patch series was integrated into seen via git@81d1fb4. |
|
There was a status update in the "New Topics" section about the branch The credential helper that talks with osx keychain learned to avoid storing back the authentication material it just got received from the keychain. Comments? source: <pull.1729.v3.git.1715800868.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3c0c9b9. |
|
There was a status update in the "Cooking" section about the branch The credential helper that talks with osx keychain learned to avoid storing back the authentication material it just got received from the keychain. Comments? source: <pull.1729.v3.git.1715800868.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@555980e. |
|
This patch series was integrated into seen via git@ad8e2b9. |
|
This patch series was integrated into next via git@4d75716. |
|
This patch series was integrated into seen via git@8d10a4a. |
|
This patch series was integrated into seen via git@0776817. |
|
There was a status update in the "Cooking" section about the branch The credential helper that talks with osx keychain learned to avoid storing back the authentication material it just got received from the keychain. Will merge to 'master'. source: <pull.1729.v3.git.1715800868.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The credential helper that talks with osx keychain learned to avoid storing back the authentication material it just got received from the keychain. Will merge to 'master'. source: <pull.1729.v3.git.1715800868.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@2a5f53c. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc: Bo Anderson mail@boanderson.me
cc: Jeff King peff@peff.net
cc: "brian m. carlson" sandals@crustytoothpaste.net
cc: Junio C Hamano gitster@pobox.com