From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH] credential: do not store credentials received from helpers
Date: Fri, 6 Apr 2012 23:34:17 -0400 [thread overview]
Message-ID: <20120407033417.GA13914@sigill.intra.peff.net> (raw)
The typical call flow for the credential API is something
like:
1. Network code (like http.c) wants a credential. It calls
credential_fill() to get it, and one of the two happens:
a. We call out to any helpers with a "get" request,
one of which provides the credential.
b. No helper provides us the credential, so we ask the
user.
2. The network code uses the credential. Let's imagine
that the request completes successfully.
3. The network code informs the credential subsystem of
success by calling credential_approve(). The credential
code then calls out to any helpers with a "store"
request, so they can optionally store it if they want.
In the case of (1b), this is a good thing. The credential
comes from the user, gets used, and then gets put into
storage for later use again. But in the case of (1a), we end
up feeding the helper with a credential that came from
itself already (or possibly from another helper).
Most of the time, this is harmless and slightly inefficient.
But it has two user-visible impacts:
1. If you have two helpers, you end up propagating data
between them. For instance, imagine you have a helper
"git-credential-wallet" which pulls data from a a
read-only password wallet. You might configure git like
this:
[credential]
helper = wallet
helper = cache
to check the wallet first, and then fall back to asking
the user and caching the result. But when we do get
something out of the wallet, git will then ask for it
to be stored in the wallet (which is a no-op), and the
cache. So your credential migrates from the wallet into
the cache, which may not be what you want (e.g.,
because the wallet implements more strict security
policies than the cache).
2. If you use a time-based storage helper like
"git-credential-cache", every time you run a git
command which uses the credential, it will also
re-insert the credential after use, freshening the
cache timestamp. So the cache will eventually expire N
time units after the last _use_, not after the time the
user actually typed the password. This is probably not
what most users expect or want (and if they do, we
should do it explicitly by providing an option to
refresh the timestamp on use).
We can solve this by marking a credential that comes from a
helper, so we don't bother feeding it back to the helpers.
The credential struct already has an "approved" flag so
that we try to store it only once, rather than for each
successful http request. We can use the same flag to
"pre-approve" a credential which comes from a helper, and
enver try to store it at all.
Signed-off-by: Jeff King <peff@peff.net>
---
Whew, that was a long explanation. Fortunately the patch text itself is
pleasantly simple.
credential.c | 4 +++-
t/t5550-http-fetch.sh | 6 +++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/credential.c b/credential.c
index 62d1c56..813e77a 100644
--- a/credential.c
+++ b/credential.c
@@ -272,8 +272,10 @@ void credential_fill(struct credential *c)
for (i = 0; i < c->helpers.nr; i++) {
credential_do(c, c->helpers.items[i].string, "get");
- if (c->username && c->password)
+ if (c->username && c->password) {
+ c->approved = 1;
return;
+ }
}
credential_getpass(c);
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index e5e6b8f..82e0d37 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -104,13 +104,17 @@ test_expect_success 'http auth can request both user and pass' '
test_expect_success 'http auth respects credential helper config' '
test_config_global credential.helper "!f() {
cat >/dev/null
+ echo helper: \$* >>helper-trace
echo username=user@host
echo password=user@host
}; f" &&
+ >helper-trace &&
>askpass-query &&
echo wrong >askpass-response &&
git clone "$HTTPD_URL/auth/repo.git" clone-auth-helper &&
- expect_askpass none
+ expect_askpass none &&
+ echo "helper: get" >helper-expect &&
+ test_cmp helper-expect helper-trace
'
test_expect_success 'http auth can get username from config' '
--
1.7.10.rc4.31.g355de
next reply other threads:[~2012-04-07 3:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-07 3:34 Jeff King [this message]
2012-04-07 4:12 ` [PATCH] credential: do not store credentials received from helpers Shawn Pearce
2012-04-07 4:56 ` Jeff King
2012-04-07 5:21 ` Jeff King
2012-04-07 4:56 ` Junio C Hamano
2012-04-07 5:09 ` Jeff King
2012-04-08 5:05 ` Junio C Hamano
2012-04-08 6:40 ` Jeff King
2012-04-08 7:07 ` Jeff King
2012-04-08 7:13 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120407033417.GA13914@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).