git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] credential: add nocache option to the credentials API
@ 2019-07-07  5:51 Masaya Suzuki
  2019-07-09 12:56 ` Jeff King
  0 siblings, 1 reply; 2+ messages in thread
From: Masaya Suzuki @ 2019-07-07  5:51 UTC (permalink / raw)
  To: git; +Cc: Masaya Suzuki

The credentials API calls credentials helpers in order. If a
username/password pair is returned the helpers and if it's used for
authentication successfully, it's announced to the helpers and they can
store it for later use.

Some credentials are valid only for the limited time and should not be
cached. In this case, because the credential is announced to all helpers
and they can independently decide whether they will cache it or not,
those short-lived credentials can be cached.

This change adds an option that a credential helper can specify that the
credential returned by the helper should not be cached. If this is
specified, even after the credential is used successfully, it won't be
announced to other helpers for store.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 Documentation/technical/api-credentials.txt | 4 +++-
 credential.c                                | 4 +++-
 credential.h                                | 3 ++-
 t/t0300-credentials.sh                      | 9 +++++++++
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index 75368f26ca..3db5841b40 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -251,7 +251,9 @@ even no values at all if it has nothing useful to provide. Any provided
 attributes will overwrite those already known about by Git.  If a helper
 outputs a `quit` attribute with a value of `true` or `1`, no further
 helpers will be consulted, nor will the user be prompted (if no
-credential has been provided, the operation will then fail).
+credential has been provided, the operation will then fail). If a helper outputs
+a `nocache` attribute with a value of `true` or `1`, `credential_approve` will
+not be called even after the credential is used for authentication sucessfully.
 
 For a `store` or `erase` operation, the helper's output is ignored.
 If it fails to perform the requested operation, it may complain to
diff --git a/credential.c b/credential.c
index 62be651b03..db7b351447 100644
--- a/credential.c
+++ b/credential.c
@@ -179,6 +179,8 @@ int credential_read(struct credential *c, FILE *fp)
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {
 			c->quit = !!git_config_bool("quit", value);
+		} else if (!strcmp(key, "nocache")) {
+			c->no_cache= !!git_config_bool("nocache", value);
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but
@@ -296,7 +298,7 @@ void credential_approve(struct credential *c)
 {
 	int i;
 
-	if (c->approved)
+	if (c->approved || c->no_cache)
 		return;
 	if (!c->username || !c->password)
 		return;
diff --git a/credential.h b/credential.h
index 6b0cd16be2..be0f35d841 100644
--- a/credential.h
+++ b/credential.h
@@ -8,7 +8,8 @@ struct credential {
 	unsigned approved:1,
 		 configured:1,
 		 quit:1,
-		 use_http_path:1;
+		 use_http_path:1,
+		 no_cache:1;
 
 	char *username;
 	char *password;
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 82eaaea0f4..ad06f6fe11 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -118,6 +118,15 @@ test_expect_success 'do not bother storing password-less credential' '
 	EOF
 '
 
+test_expect_success 'credential_approve does not call helpers for nocache' '
+	check approve useless <<-\EOF
+	username=foo
+	password=bar
+	nocache=1
+	--
+	--
+	EOF
+'
 
 test_expect_success 'credential_reject calls all helpers' '
 	check reject useless "verbatim one two" <<-\EOF
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] credential: add nocache option to the credentials API
  2019-07-07  5:51 [PATCH] credential: add nocache option to the credentials API Masaya Suzuki
@ 2019-07-09 12:56 ` Jeff King
  0 siblings, 0 replies; 2+ messages in thread
From: Jeff King @ 2019-07-09 12:56 UTC (permalink / raw)
  To: Masaya Suzuki; +Cc: git

On Sat, Jul 06, 2019 at 10:51:32PM -0700, Masaya Suzuki wrote:

> The credentials API calls credentials helpers in order. If a
> username/password pair is returned the helpers and if it's used for
> authentication successfully, it's announced to the helpers and they can
> store it for later use.
> 
> Some credentials are valid only for the limited time and should not be
> cached. In this case, because the credential is announced to all helpers
> and they can independently decide whether they will cache it or not,
> those short-lived credentials can be cached.
> 
> This change adds an option that a credential helper can specify that the
> credential returned by the helper should not be cached. If this is
> specified, even after the credential is used successfully, it won't be
> announced to other helpers for store.

I think this makes sense to do, though note that there's an old
discussion which covers some alternatives:

  https://public-inbox.org/git/20120407033417.GA13914@sigill.intra.peff.net/

In that patch, I essentially proposed making all gathered credentials as
nocache. That's a more secure default (though in some cases less
convenient).

It did break a case Shawn had of caching the result of another helper. I
showed some options there for providing a mechanism to chain helpers
together explicitly.

We also discussed helpers passing out an explicit ttl. That's a more
general case of your nocache flag (i.e., ttl=0 covers that case, but we
could additionally pass "ttl" to the cache helper to let it be smarter).

Given the age of that discussion and the fact that nobody has really
complained much in the interim, I'm OK to go with your much simpler
approach. But I think it's worth at least thinking for a few minutes on
whether there's anything to pull from that discussion. :)

(As a side note, I've had all those patches on my "to revisit and send
upstream" queue for 7 years; if we take yours, maybe I can finally let
them go. ;) ).

>  Documentation/technical/api-credentials.txt | 4 +++-
>  credential.c                                | 4 +++-
>  credential.h                                | 3 ++-
>  t/t0300-credentials.sh                      | 9 +++++++++
>  4 files changed, 17 insertions(+), 3 deletions(-)

The patch itself looks good; two minor comments:

> @@ -296,7 +298,7 @@ void credential_approve(struct credential *c)
>  {
>  	int i;
>  
> -	if (c->approved)
> +	if (c->approved || c->no_cache)
>  		return;
>  	if (!c->username || !c->password)
>  		return;

Here we're disallowing a "nocache" credential from being passed to _any_
helper, whether it's caching or not. It could be storing permanently,
though perhaps that's semantic nitpicking (if it's not to be cached, it
probably shouldn't be stored permanently either). Other helpers could in
theory be doing something else with the data, though in practice I doubt
here are any uses beyond debugging.

I dunno. I started writing that paragraph to suggest calling it
"nostore" or something, but I think that is really no better than
"nocache".

If we did have a ttl, I'd expect to see a check for "ttl=0" here, but
then otherwise pass the ttl along to the helper (and a matching change
in credential-cache to use the minimum of the credential-specific ttl or
the global-configured one).

> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index 82eaaea0f4..ad06f6fe11 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -118,6 +118,15 @@ test_expect_success 'do not bother storing password-less credential' '
>  	EOF
>  '
>  
> +test_expect_success 'credential_approve does not call helpers for nocache' '
> +	check approve useless <<-\EOF
> +	username=foo
> +	password=bar
> +	nocache=1
> +	--
> +	--
> +	EOF
> +'

At first I thought this test was doing nothing, since we don't generally
expect a helper to produce output for an "approve" request (and there
are cases in lib-credential.sh that rely on that). But the "useless"
helper is intentionally chatty, so it produces output for all requests,
and this would fail without your patch. Good.

The cases in lib-credential also omit the "--" for empty sections, but I
see the similar "do not bother..." test above includes them. It
shouldn't matter either way.

So I think this looks good.

-Peff

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-07  5:51 [PATCH] credential: add nocache option to the credentials API Masaya Suzuki
2019-07-09 12:56 ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox