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 <mirth.hickford@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
M Hickford committed
Jun 15, 2023 at 19:19 UTC
aeb21ce22eec112b37975443a160cb5418c6ec22
5 files changed
+90
-18
builtin/credential-cache--daemon.c
+7
-7
@@ -33,22 +33,22 @@ static void cache_credential(struct credential *c, int timeout)
33
e->expiration = time(NULL) + timeout;
34
}
35
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)
37
{
38
int i;
39
for (i = 0; i < entries_nr; i++) {
40
struct credential *e = &entries[i].item;
41
- if (credential_match(c, e))
41
+ if (credential_match(c, e, match_password))
42
return &entries[i];
43
}
44
return NULL;
45
}
46
47
-static void remove_credential(const struct credential *c)
47
+static void remove_credential(const struct credential *c, int match_password)
48
{
49
struct credential_cache_entry *e;
50
51
- e = lookup_credential(c);
51
+ e = lookup_credential(c, match_password);
52
if (e)
53
e->expiration = 0;
54
}
@@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out)
127
if (read_request(in, &c, &action, &timeout) < 0)
128
/* ignore error */ ;
129
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);
131
if (e) {
132
fprintf(out, "username=%s\n", e->item.username);
133
fprintf(out, "password=%s\n", e->item.password);
@@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out)
151
exit(0);
152
}
153
else if (!strcmp(action.buf, "erase"))
154
- remove_credential(&c);
154
+ remove_credential(&c, 1);
155
else if (!strcmp(action.buf, "store")) {
156
if (timeout < 0)
157
warning("cache client didn't specify a timeout");
158
else if (!c.username || !c.password)
159
warning("cache client gave us a partial credential");
160
else {
161
- remove_credential(&c);
161
+ remove_credential(&c, 0);
162
cache_credential(&c, timeout);
163
}
164
}
builtin/credential-store.c
+8
-7
@@ -13,7 +13,8 @@ static struct lock_file credential_lock;
13
static int parse_credential_file(const char *fn,
14
struct credential *c,
15
void (*match_cb)(struct credential *),
16
- void (*other_cb)(struct strbuf *))
16
+ void (*other_cb)(struct strbuf *),
17
+ int match_password)
18
{
19
FILE *fh;
20
struct strbuf line = STRBUF_INIT;
@@ -30,7 +31,7 @@ static int parse_credential_file(const char *fn,
31
while (strbuf_getline_lf(&line, fh) != EOF) {
32
if (!credential_from_url_gently(&entry, line.buf, 1) &&
33
entry.username && entry.password &&
33
- credential_match(c, &entry)) {
34
+ credential_match(c, &entry, match_password)) {
35
found_credential = 1;
36
if (match_cb) {
37
match_cb(&entry);
@@ -60,7 +61,7 @@ static void print_line(struct strbuf *buf)
61
}
62
63
static void rewrite_credential_file(const char *fn, struct credential *c,
63
- struct strbuf *extra)
64
+ struct strbuf *extra, int match_password)
65
{
66
int timeout_ms = 1000;
67
@@ -69,7 +70,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
70
die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
71
if (extra)
72
print_line(extra);
72
- parse_credential_file(fn, c, NULL, print_line);
73
+ parse_credential_file(fn, c, NULL, print_line, match_password);
74
if (commit_lock_file(&credential_lock) < 0)
75
die_errno("unable to write credential store");
76
}
@@ -91,7 +92,7 @@ static void store_credential_file(const char *fn, struct credential *c)
92
is_rfc3986_reserved_or_unreserved);
93
}
94
94
- rewrite_credential_file(fn, c, &buf);
95
+ rewrite_credential_file(fn, c, &buf, 0);
96
strbuf_release(&buf);
97
}
98
@@ -138,7 +139,7 @@ static void remove_credential(const struct string_list *fns, struct credential *
139
return;
140
for_each_string_list_item(fn, fns)
141
if (!access(fn->string, F_OK))
141
- rewrite_credential_file(fn->string, c, NULL);
142
+ rewrite_credential_file(fn->string, c, NULL, 1);
143
}
144
145
static void lookup_credential(const struct string_list *fns, struct credential *c)
@@ -146,7 +147,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *
147
struct string_list_item *fn;
148
149
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))
151
return; /* Found credential */
152
}
153
credential.c
+4
-3
@@ -33,13 +33,14 @@ void credential_clear(struct credential *c)
33
}
34
35
int credential_match(const struct credential *want,
36
- const struct credential *have)
36
+ const struct credential *have, int match_password)
37
{
38
#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
39
return CHECK(protocol) &&
40
CHECK(host) &&
41
CHECK(path) &&
42
- CHECK(username);
42
+ CHECK(username) &&
43
+ (!match_password || CHECK(password));
44
#undef CHECK
45
}
46
@@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb)
103
warning(_("skipping credential lookup for key: credential.%s"),
104
url);
105
else
105
- matches = credential_match(&want, c);
106
+ matches = credential_match(&want, c, 0);
107
credential_clear(&want);
108
109
return matches;
credential.h
+1
-1
@@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url);
211
int credential_from_url_gently(struct credential *, const char *url, int quiet);
212
213
int credential_match(const struct credential *want,
214
- const struct credential *have);
214
+ const struct credential *have, int match_password);
215
216
#endif /* CREDENTIAL_H */
t/lib-credential.sh
+70
@@ -44,6 +44,8 @@ helper_test_clean() {
44
reject $1 https example.com user1
45
reject $1 https example.com user2
46
reject $1 https example.com user4
47
+ reject $1 https example.com user-distinct-pass
48
+ reject $1 https example.com user-overwrite
49
reject $1 http path.tld user
50
reject $1 https timeout.tld user
51
reject $1 https sso.tld
@@ -167,6 +169,49 @@ helper_test() {
169
EOF
170
'
171
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://user-overwrite@example.com'\'':
212
+ EOF
213
+ '
214
+
215
test_expect_success "helper ($HELPER) can forget host" '
216
check reject $HELPER <<-\EOF &&
217
protocol=https
@@ -221,6 +266,31 @@ helper_test() {
266
EOF
267
'
268
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
+
294
test_expect_success "helper ($HELPER) can forget user" '
295
check reject $HELPER <<-\EOF &&
296
protocol=https