credential-cache: respect request capabilities #1842
Conversation
This file contains 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
|
There are issues in commit 945b5c5: |
hickford
force-pushed
the
cache-capability
branch
from
945b5c5 to
9197941
Compare
|
/submit |
|
Submitted as pull.1842.git.1734729534213.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
hickford
force-pushed
the
cache-capability
branch
from
9197941 to
696780d
Compare
|
/submit |
|
Submitted as pull.1842.v2.git.1736193131798.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> From: M Hickford <mirth.hickford@gmail.com>
>
> Previously, credential-cache responded with capability[]=authtype
> regardless of request.
That's the correct behaviour.
> The capabilities in a credential helper response should be a subset of
> the capabilities in the request.
No, it should not. Otherwise, it's impossible for Git to know whether
the helper does or does not support the capability. We rely on that
information to correctly pass data back when saving data.
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..692216cf83c 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
> else if (!strcmp(action.buf, "get")) {
> struct credential_cache_entry *e = lookup_credential(&c);
> if (e) {
> - e->item.capa_authtype.request_initial = 1;
> - e->item.capa_authtype.request_helper = 1;
> -
> - fprintf(out, "capability[]=authtype\n");
> + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> + fprintf(out, "capability[]=authtype\n");
> + }
This part is not correct.
> if (e->item.username)
> fprintf(out, "username=%s\n", e->item.username);
> if (e->item.password)
> fprintf(out, "password=%s\n", e->item.password);
> - if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
> fprintf(out, "authtype=%s\n", e->item.authtype);
> - if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
This part may very well be correct.
> fprintf(out, "credential=%s\n", e->item.credential);
> if (e->item.password_expiry_utc != TIME_MAX)
> fprintf(out, "password_expiry_utc=%"PRItime"\n",
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 58b9c740605..324ecc792d5 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -566,6 +566,21 @@ helper_test_authtype() {
> EOF
> '
>
> + test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
> + check fill $HELPER <<-\EOF
> + protocol=https
> + host=git.example.com
> + --
> + protocol=https
> + host=git.example.com
> + username=askpass-username
> + password=askpass-password
> + --
> + askpass: Username for '\''https://git.example.com'\'':
> + askpass: Password for '\''https://askpass-username@git.example.com'\'':
> + EOF
> + '
> +
> test_expect_success "helper ($HELPER) stores authtype and credential with username" '
> check approve $HELPER <<-\EOF
> capability[]=authtype
>
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
> --
> gitgitgadget
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA |
hickford
force-pushed
the
cache-capability
branch
from
696780d to
3129fdd
Compare
|
On the Git mailing list, M Hickford wrote (reply to this): On Mon, 6 Jan 2025 at 22:32, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > Previously, credential-cache responded with capability[]=authtype
> > regardless of request.
>
> That's the correct behaviour.
>
> > The capabilities in a credential helper response should be a subset of
> > the capabilities in the request.
>
> No, it should not. Otherwise, it's impossible for Git to know whether
> the helper does or does not support the capability. We rely on that
> information to correctly pass data back when saving data.
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index bc22f5c6d24..692216cf83c 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
> > else if (!strcmp(action.buf, "get")) {
> > struct credential_cache_entry *e = lookup_credential(&c);
> > if (e) {
> > - e->item.capa_authtype.request_initial = 1;
> > - e->item.capa_authtype.request_helper = 1;
> > -
> > - fprintf(out, "capability[]=authtype\n");
> > + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> > + fprintf(out, "capability[]=authtype\n");
> > + }
>
> This part is not correct.
Thanks for the review. I'll revert this part and amend the commit message.
>
> > if (e->item.username)
> > fprintf(out, "username=%s\n", e->item.username);
> > if (e->item.password)
> > fprintf(out, "password=%s\n", e->item.password);
> > - if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> > + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
> > fprintf(out, "authtype=%s\n", e->item.authtype);
> > - if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> > + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
>
> This part may very well be correct.
>
> > fprintf(out, "credential=%s\n", e->item.credential);
> > if (e->item.password_expiry_utc != TIME_MAX)
> > fprintf(out, "password_expiry_utc=%"PRItime"\n",
> > diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> > index 58b9c740605..324ecc792d5 100644
> > --- a/t/lib-credential.sh
> > +++ b/t/lib-credential.sh
> > @@ -566,6 +566,21 @@ helper_test_authtype() {
> > EOF
> > '
> >
> > + test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
> > + check fill $HELPER <<-\EOF
> > + protocol=https
> > + host=git.example.com
> > + --
> > + protocol=https
> > + host=git.example.com
> > + username=askpass-username
> > + password=askpass-password
> > + --
> > + askpass: Username for '\''https://git.example.com'\'':
> > + askpass: Password for '\''https://askpass-username@git.example.com'\'':
> > + EOF
> > + '
> > +
> > test_expect_success "helper ($HELPER) stores authtype and credential with username" '
> > check approve $HELPER <<-\EOF
> > capability[]=authtype
> >
> > base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
> > --
> > gitgitgadget
>
> --
> brian m. carlson (they/them or he/him)
> Toronto, Ontario, CA |
hickford
force-pushed
the
cache-capability
branch
from
3129fdd to
e9851c5
Compare
|
/submit |
|
Submitted as pull.1842.v3.git.1736204756030.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, "brian m. carlson" wrote (reply to this): On 2025-01-06 at 22:57:06, M Hickford wrote:
> On Mon, 6 Jan 2025 at 22:32, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> > > From: M Hickford <mirth.hickford@gmail.com>
> > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > > index bc22f5c6d24..692216cf83c 100644
> > > --- a/builtin/credential-cache--daemon.c
> > > +++ b/builtin/credential-cache--daemon.c
> > > @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
> > > else if (!strcmp(action.buf, "get")) {
> > > struct credential_cache_entry *e = lookup_credential(&c);
> > > if (e) {
> > > - e->item.capa_authtype.request_initial = 1;
> > > - e->item.capa_authtype.request_helper = 1;
> > > -
> > > - fprintf(out, "capability[]=authtype\n");
> > > + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> > > + fprintf(out, "capability[]=authtype\n");
> > > + }
> >
> > This part is not correct.
>
> Thanks for the review. I'll revert this part and amend the commit message.
I applied this without that change and it does still pass the test,
which I think is good and shows that can be omitted. If I have some
time, I may send a follow-up patch to add some additional tests.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA |
Previously, credential-cache populated authtype regardless of request. Signed-off-by: M Hickford <mirth.hickford@gmail.com>
hickford
force-pushed
the
cache-capability
branch
from
e9851c5 to
23942f9
Compare
|
/submit |
|
Submitted as pull.1842.v4.git.1736212760709.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): "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: M Hickford <mirth.hickford@gmail.com>
>
> Previously, credential-cache populated authtype regardless of request.
OK, that may be a correct statement of the fact, but it does not
tell readers any of the following:
- If it is a bad thing to populate authtype regardless of request,
and if so why?
- What is the (negative) consequence of doing so, if any? What
breaks because it populates authtype regardless of request?
- What is the remedy? Instead of unconditionally populating the
authtype, how does the new code decide when to populate it and
with what value?
- Can there be downsides of fixing this? Are there use cases where
this unconditional population of authtype was relied upon?
- Where did the bug come from and what is its fix? We used to look
at OP_HELPER to decide when to emit authtype, but the updated
code checks OP_RESPONSE, which readers can see in the patch.
It would be nice if the proposed log message helped them by
briefly explaining their differences, for example.
which would help future "git log" readers what this fix was about.
Will queue for now, but the log message would want to be a bit more
helpful to the readers.
Thanks.
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
> credential-cache: respect request capabilities
>
> CC: sandals@crustytoothpaste.net
>
> Patch v4 fixes test
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1842
>
> Range-diff vs v3:
>
> 1: e9851c5c4ac ! 1: 23942f9fa47 credential-cache: respect request capabilities
> @@ t/lib-credential.sh: helper_test_authtype() {
> + protocol=https
> + host=git.example.com
> + --
> -+ capability[]=authtype
> + protocol=https
> + host=git.example.com
> + username=askpass-username
>
>
> builtin/credential-cache--daemon.c | 4 ++--
> t/lib-credential.sh | 15 +++++++++++++++
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..e707618e743 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
> fprintf(out, "username=%s\n", e->item.username);
> if (e->item.password)
> fprintf(out, "password=%s\n", e->item.password);
> - if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
> fprintf(out, "authtype=%s\n", e->item.authtype);
> - if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
> fprintf(out, "credential=%s\n", e->item.credential);
> if (e->item.password_expiry_utc != TIME_MAX)
> fprintf(out, "password_expiry_utc=%"PRItime"\n",
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 58b9c740605..cc6bf9aa5f3 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -566,6 +566,21 @@ helper_test_authtype() {
> EOF
> '
>
> + test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
> + check fill $HELPER <<-\EOF
> + protocol=https
> + host=git.example.com
> + --
> + protocol=https
> + host=git.example.com
> + username=askpass-username
> + password=askpass-password
> + --
> + askpass: Username for '\''https://git.example.com'\'':
> + askpass: Password for '\''https://askpass-username@git.example.com'\'':
> + EOF
> + '
> +
> test_expect_success "helper ($HELPER) stores authtype and credential with username" '
> check approve $HELPER <<-\EOF
> capability[]=authtype
>
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f |
|
This patch series was integrated into seen via git@d970fe7. |
|
This patch series was integrated into seen via git@485116e. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CC: sandals@crustytoothpaste.net
Patch v4 fixes test