Revert "osxkeychain: state to skip unnecessary store operations"
PATCH 1/1 Revert "osxkeychain: state to skip unnecessary store operations"
From: Koji Nakamaru <[email protected]>
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".
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.
[1]: https://github.com/hickford/git-credential-oauth
---
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(-)
contrib/credential/osxkeychain/git-credential-osxkeychain.c +0 -11
@@ -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
Signature · 1 line
gitgitgadget
Quoted 3 lines · Koji Nakamaru via GitGitGadget
From: Koji Nakamaru <[email protected]>
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.
Quoted 9 lines
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".
Quoted 3 lines
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.
Quoted 59 lines
[1]: https://github.com/hickford/git-credential-oauth
Reported-by Petter Sælen <[email protected]>Signed-off-by Koji Nakamaru <[email protected]>---
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(-)contrib/credential/osxkeychain/git-credential-osxkeychain.c +0
-11@@ -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
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Koji Nakamaru via GitGitGadget
From: Koji Nakamaru <[email protected]>
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.
Quoted 24 lines
Quoted 9 lines
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".
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
Quoted 2 lines
"osxkeychain:seen" can encode the whole information of the last "get".
Quoted 4 lines · Koji Nakamaru via GitGitGadget
From: Koji Nakamaru <[email protected]>
This reverts commit
e1ab45b2dab51f94db9548666dfd7af626d2aa7e.
Quoted 8 lines
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.
Signature · 2 lines
brian m. carlson (they/them) Toronto, Ontario, CA
Quoted 23 lines · brian m. carlson
Quoted 4 lines · Koji Nakamaru via GitGitGadget
From: Koji Nakamaru <[email protected]>
This reverts commit
e1ab45b2dab51f94db9548666dfd7af626d2aa7e.Quoted 8 lines
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?
Quoted 5 lines
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/[email protected]/
--
Koji Nakamaru