osxkeychain: avoid incorrectly skipping store operation

[KojiNakamaru]

No description provided.

[KojiNakamaru]

/preview

[dscho]

@KojiNakamaru I kicked of a re-run of the two failed osx-* jobs, which I expect to succeed, because they failed in the git p4 tests (which I have the strong sense are quite flaky).

[gitgitgadget]

Preview email sent as pull.1999.git.1763046204910.gitgitgadget@gmail.com

[KojiNakamaru]

/submit

[gitgitgadget]

Submitted as pull.1999.git.1763047599254.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1999/KojiNakamaru/fix/osxkeychain-state-seen-v1

To fetch this version to local tag pr-1999/KojiNakamaru/fix/osxkeychain-state-seen-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1999/KojiNakamaru/fix/osxkeychain-state-seen-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +/*
> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
> + * introduce significant dependencies. Therefore, we define simplified
> + * versions here to keep this code self-contained.
> + */

Sorry, but I do not quite understand this comment.  The program is
shipped as a part of Git, and using these functions and linking with
libgit.a may pull strbuf.o and some other *.o files out of libgit.a
to link with git-credential-osxkeychain.o to produce the executable,
but how can that be "significant dependencies"?  For anybody who is
building git-credential-osxkeychain, the necessary sources come for
free.

It is not like we are forcing git-credential-osxkeychain to link
with a shared object libgit.so and making git-credential-osxkeychain
depend on it, or anything like that, which may require consumers of
binary distribution of git-credential-osxkeychain to also install
another package that has libgit.so in it (which is likely to be the
"git" package).  Even if it were the case (which is not), what good
would it be to have git-credential-osxkeychain on your system
without having git on the same system?

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +/*
>> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
>> + * introduce significant dependencies. Therefore, we define simplified
>> + * versions here to keep this code self-contained.
>> + */
>
> Sorry, but I do not quite understand this comment.  The program is
> shipped as a part of Git, and using these functions and linking with
> libgit.a may pull strbuf.o and some other *.o files out of libgit.a
> to link with git-credential-osxkeychain.o to produce the executable,
> but how can that be "significant dependencies"?  For anybody who is
> building git-credential-osxkeychain, the necessary sources come for
> free.
>
> It is not like we are forcing git-credential-osxkeychain to link
> with a shared object libgit.so and making git-credential-osxkeychain
> depend on it, or anything like that, which may require consumers of
> binary distribution of git-credential-osxkeychain to also install
> another package that has libgit.so in it (which is likely to be the
> "git" package).  Even if it were the case (which is not), what good
> would it be to have git-credential-osxkeychain on your system
> without having git on the same system?

The rest of the patch, excluding the poor-man's reimplementation of
helper functions, looked like they match what the proposed log
message described.

It seems that credential material like username and password are
included in plaintext as part of the state[], but is this a safe
thing to do?  The keychain will give out the credential material in
a way the requestor with sufficient priviledges can read, and this
state[] is stored in the same place, so I am guessing that this is
not adding any extra security concerns, but I just wanted to make
sure you've considered any security implications.

Thanks.

[gitgitgadget]

On the Git mailing list, Koji Nakamaru wrote (reply to this):

On Fri, Nov 14, 2025 at 5:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> +/*
> >> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
> >> + * introduce significant dependencies. Therefore, we define simplified
> >> + * versions here to keep this code self-contained.
> >> + */
> >
> > Sorry, but I do not quite understand this comment.  The program is
> > shipped as a part of Git, and using these functions and linking with
> > libgit.a may pull strbuf.o and some other *.o files out of libgit.a
> > to link with git-credential-osxkeychain.o to produce the executable,
> > but how can that be "significant dependencies"?  For anybody who is
> > building git-credential-osxkeychain, the necessary sources come for
> > free.
> >
> > It is not like we are forcing git-credential-osxkeychain to link
> > with a shared object libgit.so and making git-credential-osxkeychain
> > depend on it, or anything like that, which may require consumers of
> > binary distribution of git-credential-osxkeychain to also install
> > another package that has libgit.so in it (which is likely to be the
> > "git" package).  Even if it were the case (which is not), what good
> > would it be to have git-credential-osxkeychain on your system
> > without having git on the same system?

I see your point. I was following the current implementation's approach
(it has its own xmalloc() and die()) and thought the comment would be
appropriate if we continued that approach. I will refactor the code to
use libgit instead.

> The rest of the patch, excluding the poor-man's reimplementation of
> helper functions, looked like they match what the proposed log
> message described.
>
> It seems that credential material like username and password are
> included in plaintext as part of the state[], but is this a safe
> thing to do?  The keychain will give out the credential material in
> a way the requestor with sufficient priviledges can read, and this
> state[] is stored in the same place, so I am guessing that this is
> not adding any extra security concerns, but I just wanted to make
> sure you've considered any security implications.

Yes, that was considered. The credential helper protocol already
passes credentials in plaintext between helpers via the "store"
operation. Since the data in state[] is handled in the same
manner, it doesn't introduce an additional security risk beyond
what the existing protocol already entails.

--
Koji Nakamaru