osxkeychain: avoid incorrectly skipping store operation #1999
+132
−30
Conversation
This file contains hidden or 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
Author
|
/preview |
Member
|
@KojiNakamaru I kicked of a re-run of the two failed |
|
Preview email sent as pull.1999.git.1763046204910.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.1999.git.1763047599254.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
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?
|
|
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. |
|
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 |
KojiNakamaru
force-pushed
the
fix/osxkeychain-state-seen
branch
from
b14045b to
f43ce9e
Compare
git-credential-osxkeychain skips storing a credential if its "get" action sets "state[]=osxkeychain:seen=1". This behavior was introduced in e1ab45b (osxkeychain: state to skip unnecessary store operations, 2024-05-15), which appeared in v2.46. However, this state[] persists even if a credential returned by "git-credential-osxkeychain get" is invalid and a subsequent helper's "get" operation returns a valid credential. Another subsequent helper (such as [1]) may expect git-credential-osxkeychain to store the valid credential, but the "store" operation is incorrectly skipped because it only checks "state[]=osxkeychain:seen=1". To solve this issue, "state[]=osxkeychain:seen" needs to contain enough information to identify whether the current "store" input matches the output from the previous "get" operation (and not a credential from another helper). Set "state[]=osxkeychain:seen" to a value encoding the credential output by "get", and compare it with a value encoding the credential input by "store". [1]: https://github.com/hickford/git-credential-oauth Reported-by: Petter Sælen <petter@saelen.eu> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
KojiNakamaru
force-pushed
the
fix/osxkeychain-state-seen
branch
from
f43ce9e to
dd36d29
Compare
Author
|
/preview |
|
Preview email sent as pull.1999.v2.git.1763099827672.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.1999.v2.git.1763100270949.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@fb2ff2c. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@d6b9163. |
|
This patch series was integrated into seen via git@607146b. |
|
This patch series was integrated into seen via git@a40eba6. |
|
There was a status update in the "Cooking" section about the branch An earlier check added to osx keychain credential helper to avoid storing the credential itself supplied was overeager and rejected credential material supplied by other helper backends that it would have wanted to store, which has been corrected. Will merge to 'next'. source: <pull.1999.v2.git.1763100270949.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@520bca9. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Thu, Nov 13, 2025 at 12:28:15PM -0800, Junio C Hamano wrote:
> "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.
Back when we added the contrib/credential helpers, I tried to avoid
linking with Git for two reasons:
1. The idea was that these _could_ be independent projects, and we
would not be on the hook for writing or maintaining everyone's pet
platform helper. So even though they are in our tree, the hope was
that they'd be simple enough to be totally independent programs
(and would not even have to be written in C). And avoiding any
dependencies kept us honest there.
It may be that the cost of not being able to re-use our usual code
is too high for the philosophical benefit, though.
2. If stuff in contrib/ depends on code in libgit.a, then changes in
the latter can break them. And I don't think we have a great flow
for detecting such breakage. Maybe one of the CI jobs builds
osxkeychain now? I'm not even sure.
The xmalloc and strbuf interfaces are pretty stable, so it may be
that the right rule is "you can depend on libgit.a, but only
lightly".
Mostly just offering my two cents (and a little backstory). I'm not
terribly opposed to loosening the rule, but we may expect some breakage
via (2) from time to time.
-Peff |
|
This patch series was integrated into seen via git@1221357. |
|
This patch series was integrated into next via git@52020a7. |
|
This patch series was integrated into seen via git@1f907fc. |
|
This patch series was integrated into next via git@335d6bb. |
|
There was a status update in the "Cooking" section about the branch Originally merged to 'next' on 2025-11-18 An earlier check added to osx keychain credential helper to avoid storing the credential itself supplied was overeager and rejected credential material supplied by other helper backends that it would have wanted to store, which has been corrected. Will merge to 'master'. source: <pull.1999.v2.git.1763100270949.gitgitgadget@gmail.com> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Changes since v1:
cc: Junio C Hamano gitster@pobox.com
cc: "brian m. carlson" sandals@crustytoothpaste.net
cc: Jeff King peff@peff.net