From: Koji Nakamaru Date: Fri, 14 Nov 2025 03:23:12 GMT Subject: Re: [PATCH] osxkeychain: avoid incorrectly skipping store operation Message-ID: In-Reply-To: On Fri, Nov 14, 2025 at 5:35 AM Junio C Hamano wrote: > > Junio C Hamano writes: > > > "Koji Nakamaru via GitGitGadget" 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