diff mbox series

[v2] credential-cache: respect request capabilities

Message ID pull.1842.v2.git.1736193131798.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] credential-cache: respect request capabilities | expand

Commit Message

M Hickford Jan. 6, 2025, 7:52 p.m. UTC 
From: M Hickford <mirth.hickford@gmail.com>

Previously, credential-cache responded with capability[]=authtype
regardless of request.

The capabilities in a credential helper response should be a subset of
the capabilities in the request.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential-cache: respect request capabilities

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1842

Range-diff vs v1:

 1:  9197941029f ! 1:  696780d4782 credential-cache: respect request capabilities
     @@ t/lib-credential.sh: helper_test_authtype() {
       		EOF
       	'
       
     -+	test_expect_success "helper ($HELPER) does not get authtype and credential without authtype capability" '
     ++	test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
      +		check fill $HELPER <<-\EOF
      +		protocol=https
      +		host=git.example.com
     @@ t/lib-credential.sh: helper_test_authtype() {
       	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
       		check approve $HELPER <<-\EOF
       		capability[]=authtype
     -
     - ## t/t0303-credential-external.sh ##
     -@@ t/t0303-credential-external.sh: helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
     - helper_test "$GIT_TEST_CREDENTIAL_HELPER"
     - helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
     - helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
     -+helper_test_authtype "$GIT_TEST_CREDENTIAL_HELPER"
     - 
     - if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
     - 	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"


 builtin/credential-cache--daemon.c | 11 +++++------
 t/lib-credential.sh                | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)


base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f

Comments

brian m. carlson Jan. 6, 2025, 10:32 p.m. UTC | #1 
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
M Hickford Jan. 6, 2025, 10:57 p.m. UTC | #2 
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
brian m. carlson Jan. 6, 2025, 11:05 p.m. UTC | #3 
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.
diff mbox series

Patch

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");
+			}
 			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)
 				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