Revert "osxkeychain: state to skip unnecessary store operations"

[KojiNakamaru]

cc: "brian m. carlson" sandals@crustytoothpaste.net

[KojiNakamaru]

/preview

[gitgitgadget]

Preview email sent as pull.1998.git.1762930718283.gitgitgadget@gmail.com

[KojiNakamaru]

/submit

[gitgitgadget]

Submitted as pull.1998.git.1762930881599.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1

To fetch this version to local tag pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1

[gitgitgadget]

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>
>
> This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.

OK.  Let's make a mental note that e1ab45b2 (osxkeychain: state to
skip unnecessary store operations, 2024-05-15) appeared in v2.46 or
so.

> That commit was trying to skip to store a credential returned by
> "git-credential-osxkeychain get" by setting
> "state[]=osxkeychain:seen=1". However, this state[] is kept even if a
> credential returned by "git-credential-osxkeychain get" is invalid and
> another subsequent helper's "get" returns a valid credential. Another
> subsequent helper (such as [1]) may expect git-credential-osxkeychain to
> store the valid credential so that "store" cannot be skipped by just
> checking "state[]=osxkeychain:seen=1".
>
> In order to solve this issue, the state[] mechanism can be refined or
> "osxkeychain:seen" can encode the whole information of the last
> "get". For now, let's revert the change.

Is anybody actively working on the proper solution?

In a patch series that replaces the old commit with a more proper
solution, it could be a reasonable layout of the series to make the
first patch a revert like this patch to give the proper solution a
clean slate to work from, but this looks different.

If the problem you are trying to solve here were a regression that
happened after Git 2.51 was released, a revert is totally warranted
at this point in time, even during the pre-release freeze period.

But it does not even look like a recent regression.  Wouldn't
reverting this change at this point give existing users who are
accustomed to the current behaviour another regression, essentially
robbing Peter to pay Paul?  In such a case, I do not think "let's
revert now and then hopefully a proper solution can come later" is a
good approach.

Thanks.

> [1]: https://github.com/hickford/git-credential-oauth
>
> Reported-by: Petter Sælen <petter@saelen.eu>
> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
> ---
>     Revert "osxkeychain: state to skip unnecessary store operations"
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1998%2FKojiNakamaru%2Frevert%2Fe1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1998/KojiNakamaru/revert/e1ab45b2dab51f94db9548666dfd7af626d2aa7e-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1998
>
>  .../osxkeychain/git-credential-osxkeychain.c          | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 611c9798b3..1f49ab8548 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -12,7 +12,6 @@ static CFStringRef username;
>  static CFDataRef password;
>  static CFDataRef password_expiry_utc;
>  static CFDataRef oauth_refresh_token;
> -static int state_seen;
>  
>  static void clear_credential(void)
>  {
> @@ -172,9 +171,6 @@ static OSStatus find_internet_password(void)
>  
>  	CFRelease(item);
>  
> -	write_item("capability[]", "state", strlen("state"));
> -	write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
> -
>  out:
>  	CFRelease(attrs);
>  
> @@ -288,9 +284,6 @@ static OSStatus add_internet_password(void)
>  	CFDictionaryRef attrs;
>  	OSStatus result;
>  
> -	if (state_seen)
> -		return errSecSuccess;
> -
>  	/* Only store complete credentials */
>  	if (!protocol || !host || !username || !password)
>  		return -1;
> @@ -402,10 +395,6 @@ static void read_credential(void)
>  			oauth_refresh_token = CFDataCreate(kCFAllocatorDefault,
>  							   (UInt8 *)v,
>  							   strlen(v));
> -		else if (!strcmp(buf, "state[]")) {
> -			if (!strcmp(v, "osxkeychain:seen=1"))
> -				state_seen = 1;
> -		}
>  		/*
>  		 * Ignore other lines; we don't know what they mean, but
>  		 * this future-proofs us when later versions of git do
>
> base-commit: 4badef0c3503dc29059d678abba7fac0f042bc84

[gitgitgadget]

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

On Thu, Nov 13, 2025 at 1:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Koji Nakamaru <koji.nakamaru@gree.net>
> >
> > This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
>
> OK.  Let's make a mental note that e1ab45b2 (osxkeychain: state to
> skip unnecessary store operations, 2024-05-15) appeared in v2.46 or
> so.

I see.

> > That commit was trying to skip to store a credential returned by
> > "git-credential-osxkeychain get" by setting
> > "state[]=osxkeychain:seen=1". However, this state[] is kept even if a
> > credential returned by "git-credential-osxkeychain get" is invalid and
> > another subsequent helper's "get" returns a valid credential. Another
> > subsequent helper (such as [1]) may expect git-credential-osxkeychain to
> > store the valid credential so that "store" cannot be skipped by just
> > checking "state[]=osxkeychain:seen=1".
> >
> > In order to solve this issue, the state[] mechanism can be refined or
> > "osxkeychain:seen" can encode the whole information of the last
> > "get". For now, let's revert the change.
>
> Is anybody actively working on the proper solution?
>
> In a patch series that replaces the old commit with a more proper
> solution, it could be a reasonable layout of the series to make the
> first patch a revert like this patch to give the proper solution a
> clean slate to work from, but this looks different.
>
> If the problem you are trying to solve here were a regression that
> happened after Git 2.51 was released, a revert is totally warranted
> at this point in time, even during the pre-release freeze period.
>
> But it does not even look like a recent regression.  Wouldn't
> reverting this change at this point give existing users who are
> accustomed to the current behaviour another regression, essentially
> robbing Peter to pay Paul?  In such a case, I do not think "let's
> revert now and then hopefully a proper solution can come later" is a
> good approach.

I see. I'll work on the following approach and submit another patch.

> > "osxkeychain:seen" can encode the whole information of the last
> > "get".

[gitgitgadget]

On the Git mailing list, "brian m. carlson" wrote (reply to this):

On 2025-11-12 at 07:01:21, Koji Nakamaru via GitGitGadget wrote:
> From: Koji Nakamaru <koji.nakamaru@gree.net>
> 
> This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
> 
> That commit was trying to skip to store a credential returned by
> "git-credential-osxkeychain get" by setting
> "state[]=osxkeychain:seen=1". However, this state[] is kept even if a
> credential returned by "git-credential-osxkeychain get" is invalid and
> another subsequent helper's "get" returns a valid credential. Another
> subsequent helper (such as [1]) may expect git-credential-osxkeychain to
> store the valid credential so that "store" cannot be skipped by just
> checking "state[]=osxkeychain:seen=1".

I believe the intended approach here is that if we do a get and the
credential is invalid, we return the same state[] header to erase, but
we should not send it to subsequent gets for a new credential.  However,
we do need to send it to subsequent gets (which will not have an
intervening erase) if this is a multistage request because otherwise
multistage requests will not be able to keep state, which NTLM and
Kerberos require.  Does that make sense?

My guess is that the problem here is that we reuse the credential
structure without resetting it somewhere in the HTTP code rather than a
problem in this particular helper.  That is probably my fault, but in my
defence I would not say that the structure of the HTTP code is very easy
to follow.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[gitgitgadget]

User "brian m. carlson" <sandals@crustytoothpaste.net> has been added to the cc: list.

[gitgitgadget]

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

On Fri, Nov 14, 2025 at 8:30 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2025-11-12 at 07:01:21, Koji Nakamaru via GitGitGadget wrote:
> > From: Koji Nakamaru <koji.nakamaru@gree.net>
> >
> > This reverts commit e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
> >
> > That commit was trying to skip to store a credential returned by
> > "git-credential-osxkeychain get" by setting
> > "state[]=osxkeychain:seen=1". However, this state[] is kept even if a
> > credential returned by "git-credential-osxkeychain get" is invalid and
> > another subsequent helper's "get" returns a valid credential. Another
> > subsequent helper (such as [1]) may expect git-credential-osxkeychain to
> > store the valid credential so that "store" cannot be skipped by just
> > checking "state[]=osxkeychain:seen=1".
>
> I believe the intended approach here is that if we do a get and the
> credential is invalid, we return the same state[] header to erase, but
> we should not send it to subsequent gets for a new credential.  However,
> we do need to send it to subsequent gets (which will not have an
> intervening erase) if this is a multistage request because otherwise
> multistage requests will not be able to keep state, which NTLM and
> Kerberos require.  Does that make sense?
>
> My guess is that the problem here is that we reuse the credential
> structure without resetting it somewhere in the HTTP code rather than a
> problem in this particular helper.  That is probably my fault, but in my
> defence I would not say that the structure of the HTTP code is very easy
> to follow.

Thanks for the explanation. I misunderstood how state[] was intended to
work. The current behavior seems rather natural now that I understand
it. It might be useful if we could optionally specify that it be
discarded under predefined conditions. Also, as you and others
previously discussed in [1], this topic is delicate and interesting.

[1]: https://lore.kernel.org/git/20240510200114.GC1954863@coredump.intra.peff.net/

--
Koji Nakamaru