Skip to content

Commit aeb21ce

Browse files
hickfordgitster
hickford
authored and
gitster
committed
credential: avoid erasing distinct password
Test that credential helpers do not erase a password distinct from the input. Such calls can happen when multiple credential helpers are configured. Fixes for credential-cache and credential-store. Signed-off-by: M Hickford <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent fe86abd commit aeb21ce

File tree

5 files changed

+90
-18
lines changed

5 files changed

+90
-18
lines changed

‎builtin/credential-cache--daemon.c‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,22 @@ static void cache_credential(struct credential *c, int timeout)
3333
e->expiration = time(NULL) + timeout;
3434
}
3535

36-
static struct credential_cache_entry *lookup_credential(const struct credential *c)
36+
static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password)
3737
{
3838
int i;
3939
for (i = 0; i < entries_nr; i++) {
4040
struct credential *e = &entries[i].item;
41-
if (credential_match(c, e))
41+
if (credential_match(c, e, match_password))
4242
return &entries[i];
4343
}
4444
return NULL;
4545
}
4646

47-
static void remove_credential(const struct credential *c)
47+
static void remove_credential(const struct credential *c, int match_password)
4848
{
4949
struct credential_cache_entry *e;
5050

51-
e = lookup_credential(c);
51+
e = lookup_credential(c, match_password);
5252
if (e)
5353
e->expiration = 0;
5454
}
@@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)
127127
if (read_request(in, &c, &action, &timeout) < 0)
128128
/* ignore error */ ;
129129
else if (!strcmp(action.buf, "get")) {
130-
struct credential_cache_entry *e = lookup_credential(&c);
130+
struct credential_cache_entry *e = lookup_credential(&c, 0);
131131
if (e) {
132132
fprintf(out, "username=%s\n", e->item.username);
133133
fprintf(out, "password=%s\n", e->item.password);
@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)
151151
exit(0);
152152
}
153153
else if (!strcmp(action.buf, "erase"))
154-
remove_credential(&c);
154+
remove_credential(&c, 1);
155155
else if (!strcmp(action.buf, "store")) {
156156
if (timeout < 0)
157157
warning("cache client didn't specify a timeout");
158158
else if (!c.username || !c.password)
159159
warning("cache client gave us a partial credential");
160160
else {
161-
remove_credential(&c);
161+
remove_credential(&c, 0);
162162
cache_credential(&c, timeout);
163163
}
164164
}

‎builtin/credential-store.c‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ static struct lock_file credential_lock;
1313
static int parse_credential_file(const char *fn,
1414
struct credential *c,
1515
void (*match_cb)(struct credential *),
16-
void (*other_cb)(struct strbuf *))
16+
void (*other_cb)(struct strbuf *),
17+
int match_password)
1718
{
1819
FILE *fh;
1920
struct strbuf line = STRBUF_INIT;
@@ -30,7 +31,7 @@ static int parse_credential_file(const char *fn,
3031
while (strbuf_getline_lf(&line, fh) != EOF) {
3132
if (!credential_from_url_gently(&entry, line.buf, 1) &&
3233
entry.username && entry.password &&
33-
credential_match(c, &entry)) {
34+
credential_match(c, &entry, match_password)) {
3435
found_credential = 1;
3536
if (match_cb) {
3637
match_cb(&entry);
@@ -60,7 +61,7 @@ static void print_line(struct strbuf *buf)
6061
}
6162

6263
static void rewrite_credential_file(const char *fn, struct credential *c,
63-
struct strbuf *extra)
64+
struct strbuf *extra, int match_password)
6465
{
6566
int timeout_ms = 1000;
6667

@@ -69,7 +70,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
6970
die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
7071
if (extra)
7172
print_line(extra);
72-
parse_credential_file(fn, c, NULL, print_line);
73+
parse_credential_file(fn, c, NULL, print_line, match_password);
7374
if (commit_lock_file(&credential_lock) < 0)
7475
die_errno("unable to write credential store");
7576
}
@@ -91,7 +92,7 @@ static void store_credential_file(const char *fn, struct credential *c)
9192
is_rfc3986_reserved_or_unreserved);
9293
}
9394

94-
rewrite_credential_file(fn, c, &buf);
95+
rewrite_credential_file(fn, c, &buf, 0);
9596
strbuf_release(&buf);
9697
}
9798

@@ -138,15 +139,15 @@ static void remove_credential(const struct string_list *fns, struct credential *
138139
return;
139140
for_each_string_list_item(fn, fns)
140141
if (!access(fn->string, F_OK))
141-
rewrite_credential_file(fn->string, c, NULL);
142+
rewrite_credential_file(fn->string, c, NULL, 1);
142143
}
143144

144145
static void lookup_credential(const struct string_list *fns, struct credential *c)
145146
{
146147
struct string_list_item *fn;
147148

148149
for_each_string_list_item(fn, fns)
149-
if (parse_credential_file(fn->string, c, print_entry, NULL))
150+
if (parse_credential_file(fn->string, c, print_entry, NULL, 0))
150151
return; /* Found credential */
151152
}
152153

‎credential.c‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)
3333
}
3434

3535
int credential_match(const struct credential *want,
36-
const struct credential *have)
36+
const struct credential *have, int match_password)
3737
{
3838
#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
3939
return CHECK(protocol) &&
4040
CHECK(host) &&
4141
CHECK(path) &&
42-
CHECK(username);
42+
CHECK(username) &&
43+
(!match_password || CHECK(password));
4344
#undef CHECK
4445
}
4546

@@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb)
102103
warning(_("skipping credential lookup for key: credential.%s"),
103104
url);
104105
else
105-
matches = credential_match(&want, c);
106+
matches = credential_match(&want, c, 0);
106107
credential_clear(&want);
107108

108109
return matches;

‎credential.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url);
211211
int credential_from_url_gently(struct credential *, const char *url, int quiet);
212212

213213
int credential_match(const struct credential *want,
214-
const struct credential *have);
214+
const struct credential *have, int match_password);
215215

216216
#endif /* CREDENTIAL_H */

‎t/lib-credential.sh‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ helper_test_clean() {
4444
reject $1 https example.com user1
4545
reject $1 https example.com user2
4646
reject $1 https example.com user4
47+
reject $1 https example.com user-distinct-pass
48+
reject $1 https example.com user-overwrite
4749
reject $1 http path.tld user
4850
reject $1 https timeout.tld user
4951
reject $1 https sso.tld
@@ -167,6 +169,49 @@ helper_test() {
167169
EOF
168170
'
169171

172+
test_expect_success "helper ($HELPER) overwrites on store" '
173+
check approve $HELPER <<-\EOF &&
174+
protocol=https
175+
host=example.com
176+
username=user-overwrite
177+
password=pass1
178+
EOF
179+
check approve $HELPER <<-\EOF &&
180+
protocol=https
181+
host=example.com
182+
username=user-overwrite
183+
password=pass2
184+
EOF
185+
check fill $HELPER <<-\EOF &&
186+
protocol=https
187+
host=example.com
188+
username=user-overwrite
189+
--
190+
protocol=https
191+
host=example.com
192+
username=user-overwrite
193+
password=pass2
194+
EOF
195+
check reject $HELPER <<-\EOF &&
196+
protocol=https
197+
host=example.com
198+
username=user-overwrite
199+
password=pass2
200+
EOF
201+
check fill $HELPER <<-\EOF
202+
protocol=https
203+
host=example.com
204+
username=user-overwrite
205+
--
206+
protocol=https
207+
host=example.com
208+
username=user-overwrite
209+
password=askpass-password
210+
--
211+
askpass: Password for '\''https://[email protected]'\'':
212+
EOF
213+
'
214+
170215
test_expect_success "helper ($HELPER) can forget host" '
171216
check reject $HELPER <<-\EOF &&
172217
protocol=https
@@ -221,6 +266,31 @@ helper_test() {
221266
EOF
222267
'
223268

269+
test_expect_success "helper ($HELPER) does not erase a password distinct from input" '
270+
check approve $HELPER <<-\EOF &&
271+
protocol=https
272+
host=example.com
273+
username=user-distinct-pass
274+
password=pass1
275+
EOF
276+
check reject $HELPER <<-\EOF &&
277+
protocol=https
278+
host=example.com
279+
username=user-distinct-pass
280+
password=pass2
281+
EOF
282+
check fill $HELPER <<-\EOF
283+
protocol=https
284+
host=example.com
285+
username=user-distinct-pass
286+
--
287+
protocol=https
288+
host=example.com
289+
username=user-distinct-pass
290+
password=pass1
291+
EOF
292+
'
293+
224294
test_expect_success "helper ($HELPER) can forget user" '
225295
check reject $HELPER <<-\EOF &&
226296
protocol=https

0 commit comments

Comments
 (0)