| Message ID | pull.1729.git.1715328467099.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fcf5b74e59c1c0d18a8e8e939475007b3b5f83ad |
| Headers | show |
| Series | osxkeychain: lock for exclusive execution | expand |
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
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 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
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.
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 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 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
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 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.
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"))