From patchwork Thu Jun 15 06:03:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: M Hickford X-Patchwork-Id: 13280724 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B75F1C0015E for ; Thu, 15 Jun 2023 06:05:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243295AbjFOGEo (ORCPT ); Thu, 15 Jun 2023 02:04:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34750 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243275AbjFOGD5 (ORCPT ); Thu, 15 Jun 2023 02:03:57 -0400 Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5E1C41FDD for ; Wed, 14 Jun 2023 23:03:28 -0700 (PDT) Received: by mail-wr1-x433.google.com with SMTP id ffacd0b85a97d-311167ba376so173137f8f.1 for ; Wed, 14 Jun 2023 23:03:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686809006; x=1689401006; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:from:to:cc:subject:date :message-id:reply-to; bh=BX6T3vDtlByHDICZ4sS6GORPBzQt50+nD3a24ouKfuk=; b=JCbQWTv+Ynj0dhKdPcdbICbNuceSmeY2D19ryyBK3rVo8zIS2PA54cVT/fFbgfMB/a ZgnbihlAb8ipzJz1NBQAFauD8E+8pGJhtBSD2zgTgGKRd+qpDE/yhHgvNFz6IuM3bJsZ oSQAo+wPtMOzu9HdSZEn8zroqjDKX7rvxM9tyUYLEGHMgKumQ6p8KwUnqxoFYP1VY43O 0AozKrUxjnOD1tcZCrraMEkKFwl2bY5iKdEKd0VPdhWm90tHS71BwBMNgxyHqKJcFxrd XyyiYpOlZLhTGbV4eGpucHIo7Pub3wCGQUeuD14qs5bgUDwTEBXc4lf3qJhjL7IzhvYY 70kg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686809006; x=1689401006; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BX6T3vDtlByHDICZ4sS6GORPBzQt50+nD3a24ouKfuk=; b=SL9zoQ/S33fcS1HiGVgQoPocpFIV2YepcsymqmaM0PlU9f3XQ5ClitqdHzBmVp2zzv hFpfIpOyzQEK3+3EgaEsxhG2Qv9Grk3iWudGoMHRGVqoW3GtGOFmLETV/valMxkfQpoT WobvISXLmQWh2xI+brgjV2KTElth0KE2e0+5JHLCnXSyNZN1qAjf2x3/KYZA7l9MXnT2 AoAW/WVwOnMWqlkyWDfvvHVgFsLeekWlBkHKsOwI2fQ54qw3n4Rm7HsaQw0mFKAAnRv0 z70V33j4eyDsSDsjltRi/Jo4duT/XjzePaIXBwrGRDmD8kAnb1HYjzWhL5FVBXSJwcGV R3xg== X-Gm-Message-State: AC+VfDyjVMDJGMebKBnovQ5THDta15kOnPjXXfejeUhu/Uw6X+FvZe0I oppuFpwdseXJAtevh+zwMFp7+lWaHQE= X-Google-Smtp-Source: ACHHUZ5OQdaVAdzt3qH9NRZ3HzA0sdJ3fI2EX7X7yJrCQ6ZZ8zXilvY13MVPZjhXoA2SbfpCgmeEjQ== X-Received: by 2002:adf:df8c:0:b0:311:1935:59fc with SMTP id z12-20020adfdf8c000000b00311193559fcmr105756wrl.16.1686809006386; Wed, 14 Jun 2023 23:03:26 -0700 (PDT) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id cs6-20020a056000088600b0030ae849c70csm20027445wrb.37.2023.06.14.23.03.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Jun 2023 23:03:26 -0700 (PDT) Message-Id: In-Reply-To: References: Date: Thu, 15 Jun 2023 06:03:23 +0000 Subject: [PATCH v3 1/2] credential: avoid erasing distinct password Fcc: Sent MIME-Version: 1.0 To: git@vger.kernel.org Cc: Jeff King , Matthew John Cheetham , M Hickford , M Hickford Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: M Hickford From: M Hickford 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 --- builtin/credential-cache--daemon.c | 14 +++--- builtin/credential-store.c | 17 ++++---- credential.c | 11 ++--- credential.h | 2 +- t/lib-credential.sh | 70 ++++++++++++++++++++++++++++++ 5 files changed, 93 insertions(+), 21 deletions(-) diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index 756c5f02aef..f64dd21d335 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -33,22 +33,22 @@ static void cache_credential(struct credential *c, int timeout) e->expiration = time(NULL) + timeout; } -static struct credential_cache_entry *lookup_credential(const struct credential *c) +static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password) { int i; for (i = 0; i < entries_nr; i++) { struct credential *e = &entries[i].item; - if (credential_match(c, e)) + if (credential_match(c, e, match_password)) return &entries[i]; } return NULL; } -static void remove_credential(const struct credential *c) +static void remove_credential(const struct credential *c, int match_password) { struct credential_cache_entry *e; - e = lookup_credential(c); + e = lookup_credential(c, match_password); if (e) e->expiration = 0; } @@ -127,7 +127,7 @@ static void serve_one_client(FILE *in, FILE *out) if (read_request(in, &c, &action, &timeout) < 0) /* ignore error */ ; else if (!strcmp(action.buf, "get")) { - struct credential_cache_entry *e = lookup_credential(&c); + struct credential_cache_entry *e = lookup_credential(&c, 0); if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); @@ -151,14 +151,14 @@ static void serve_one_client(FILE *in, FILE *out) exit(0); } else if (!strcmp(action.buf, "erase")) - remove_credential(&c); + remove_credential(&c, 1); else if (!strcmp(action.buf, "store")) { if (timeout < 0) warning("cache client didn't specify a timeout"); else if (!c.username || !c.password) warning("cache client gave us a partial credential"); else { - remove_credential(&c); + remove_credential(&c, 0); cache_credential(&c, timeout); } } diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 30c6ccf56c0..f80ff59f18a 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -13,7 +13,8 @@ static struct lock_file credential_lock; static int parse_credential_file(const char *fn, struct credential *c, void (*match_cb)(struct credential *), - void (*other_cb)(struct strbuf *)) + void (*other_cb)(struct strbuf *), + int match_password) { FILE *fh; struct strbuf line = STRBUF_INIT; @@ -29,8 +30,8 @@ static int parse_credential_file(const char *fn, while (strbuf_getline_lf(&line, fh) != EOF) { if (!credential_from_url_gently(&entry, line.buf, 1) && - entry.username && entry.password && - credential_match(c, &entry)) { + entry.username && entry.password && + credential_match(c, &entry, match_password)) { found_credential = 1; if (match_cb) { match_cb(&entry); @@ -60,7 +61,7 @@ static void print_line(struct strbuf *buf) } static void rewrite_credential_file(const char *fn, struct credential *c, - struct strbuf *extra) + struct strbuf *extra, int match_password) { int timeout_ms = 1000; @@ -69,7 +70,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c, die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms); if (extra) print_line(extra); - parse_credential_file(fn, c, NULL, print_line); + parse_credential_file(fn, c, NULL, print_line, match_password); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -91,7 +92,7 @@ static void store_credential_file(const char *fn, struct credential *c) is_rfc3986_reserved_or_unreserved); } - rewrite_credential_file(fn, c, &buf); + rewrite_credential_file(fn, c, &buf, 0); strbuf_release(&buf); } @@ -138,7 +139,7 @@ static void remove_credential(const struct string_list *fns, struct credential * return; for_each_string_list_item(fn, fns) if (!access(fn->string, F_OK)) - rewrite_credential_file(fn->string, c, NULL); + rewrite_credential_file(fn->string, c, NULL, 1); } 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 * struct string_list_item *fn; for_each_string_list_item(fn, fns) - if (parse_credential_file(fn->string, c, print_entry, NULL)) + if (parse_credential_file(fn->string, c, print_entry, NULL, 0)) return; /* Found credential */ } diff --git a/credential.c b/credential.c index 023b59d5711..9157ff0865e 100644 --- a/credential.c +++ b/credential.c @@ -33,13 +33,14 @@ void credential_clear(struct credential *c) } int credential_match(const struct credential *want, - const struct credential *have) + const struct credential *have, int match_password) { #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x))) return CHECK(protocol) && - CHECK(host) && - CHECK(path) && - CHECK(username); + CHECK(host) && + CHECK(path) && + CHECK(username) && + (!match_password || CHECK(password)); #undef CHECK } @@ -102,7 +103,7 @@ static int match_partial_url(const char *url, void *cb) warning(_("skipping credential lookup for key: credential.%s"), url); else - matches = credential_match(&want, c); + matches = credential_match(&want, c, 0); credential_clear(&want); return matches; diff --git a/credential.h b/credential.h index b8e2936d1dc..acc41adf548 100644 --- a/credential.h +++ b/credential.h @@ -211,6 +211,6 @@ void credential_from_url(struct credential *, const char *url); int credential_from_url_gently(struct credential *, const char *url, int quiet); int credential_match(const struct credential *want, - const struct credential *have); + const struct credential *have, int match_password); #endif /* CREDENTIAL_H */ diff --git a/t/lib-credential.sh b/t/lib-credential.sh index f1ab92ba35c..7b4fdd011d7 100644 --- a/t/lib-credential.sh +++ b/t/lib-credential.sh @@ -44,6 +44,8 @@ helper_test_clean() { reject $1 https example.com user1 reject $1 https example.com user2 reject $1 https example.com user4 + reject $1 https example.com user5 + reject $1 https example.com user8 reject $1 http path.tld user reject $1 https timeout.tld user reject $1 https sso.tld @@ -167,6 +169,49 @@ helper_test() { EOF ' + test_expect_success "helper ($HELPER) overwrites on store" ' + check approve $HELPER <<-\EOF && + protocol=https + host=example.com + username=user8 + password=pass1 + EOF + check approve $HELPER <<-\EOF && + protocol=https + host=example.com + username=user8 + password=pass2 + EOF + check fill $HELPER <<-\EOF && + protocol=https + host=example.com + username=user8 + -- + protocol=https + host=example.com + username=user8 + password=pass2 + EOF + check reject $HELPER <<-\EOF && + protocol=https + host=example.com + username=user8 + password=pass2 + EOF + check fill $HELPER <<-\EOF + protocol=https + host=example.com + username=user8 + -- + protocol=https + host=example.com + username=user8 + password=askpass-password + -- + askpass: Password for '\''https://user8@example.com'\'': + EOF + ' + test_expect_success "helper ($HELPER) can forget host" ' check reject $HELPER <<-\EOF && protocol=https @@ -221,6 +266,31 @@ helper_test() { EOF ' + test_expect_success "helper ($HELPER) does not erase a password distinct from input" ' + check approve $HELPER <<-\EOF && + protocol=https + host=example.com + username=user5 + password=pass1 + EOF + check reject $HELPER <<-\EOF && + protocol=https + host=example.com + username=user5 + password=pass2 + EOF + check fill $HELPER <<-\EOF + protocol=https + host=example.com + username=user5 + -- + protocol=https + host=example.com + username=user5 + password=pass1 + EOF + ' + test_expect_success "helper ($HELPER) can forget user" ' check reject $HELPER <<-\EOF && protocol=https From patchwork Thu Jun 15 06:03:24 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: M Hickford X-Patchwork-Id: 13280725 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A7F22EB64DB for ; Thu, 15 Jun 2023 06:05:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243648AbjFOGEl (ORCPT ); Thu, 15 Jun 2023 02:04:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34766 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243295AbjFOGD5 (ORCPT ); Thu, 15 Jun 2023 02:03:57 -0400 Received: from mail-lf1-x135.google.com (mail-lf1-x135.google.com [IPv6:2a00:1450:4864:20::135]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5C0A026B9 for ; Wed, 14 Jun 2023 23:03:29 -0700 (PDT) Received: by mail-lf1-x135.google.com with SMTP id 2adb3069b0e04-4f629ccb8ebso9767107e87.1 for ; Wed, 14 Jun 2023 23:03:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686809007; x=1689401007; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:from:to:cc:subject:date :message-id:reply-to; bh=ZUUI+b3f/SpIj2hu+kvoXUsTd3vD3GGQLD/VXaXAzrA=; b=LSAWny6tg5p6yV12vyAnjlmKfJhkC4NlHyOi6UqVBVzJpzEgLEGJAc3yLpKiW9ipNI EELP89ZlnyKBM6g8cNW5+cCHEBk8xr00gzN1Hy0keCUEF5rCd1bD4Pxepz8KMZV9+SzA eooBlIsksu8zaNjJBAFTz2NHVBKbdfBvY9pvHuCxpuc/Q5F9J6BOo4Ae4CRmLqBBp9ir OSYXNDh1bXisTvIa5+ShZnuhMjbNdmqCKnB65LdIXx65z+wxaZkmZlqxIqgiXVnqS/Ej ERhDFF2YEm5LXRhagwHeOaHLdSlM3Y8zewER1bMGHwkv/NZl81n04s9KBo6CVvTW2W69 xdVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686809007; x=1689401007; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZUUI+b3f/SpIj2hu+kvoXUsTd3vD3GGQLD/VXaXAzrA=; b=LCKs7XJkvlfVkZoGfYL1Mv/tfG6laHd5AdRl5+GAKkN6dL3jkHyg8tkpx2UFYADk+6 fmKfxgV2imY4t5oiAHaF07lupxRnfXwiy7e8ISRqI/bLcwMSF1pdmp3mq7I+sBJ38yvy JPyQx9Xs1EchRzcxnMJ5Yn1uXRrFlVByz4OUjZtzOyoA4e5+RzE/Eign/YCuUT2936lw QxnfBf2LtRJB+F2z68p1AkxVv73G9BmORKfpLlVIcJXzFfVjrbWCZ23p+zuMGIDpGtrx plkp8akQpFwwoXAy9WkHY5pdG6i5a3fCN42CGYORE2C9Pha46rLI9bkyNOZedFRxtLoF Qt2g== X-Gm-Message-State: AC+VfDwxJOCokKBxDcQL0hQ4Mtf022nGzBxwnbKKyzctQxGEYUiwEgi9 kAU1h9L1tcOvmeN3UlcVTkNkmnPOZqw= X-Google-Smtp-Source: ACHHUZ4IssECYVUDbrHKEUlOcz9qNLqqauyH4xrxiynLX1pgTH7p0kqguYfF3n46QtBAzinNTt3n9g== X-Received: by 2002:a19:6916:0:b0:4f4:ce78:2f17 with SMTP id e22-20020a196916000000b004f4ce782f17mr7873079lfc.13.1686809006913; Wed, 14 Jun 2023 23:03:26 -0700 (PDT) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id e10-20020a056000194a00b0030497b3224bsm20005179wry.64.2023.06.14.23.03.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Jun 2023 23:03:26 -0700 (PDT) Message-Id: In-Reply-To: References: Date: Thu, 15 Jun 2023 06:03:24 +0000 Subject: [PATCH v3 2/2] credential: erase all matching credentials Fcc: Sent MIME-Version: 1.0 To: git@vger.kernel.org Cc: Jeff King , Matthew John Cheetham , M Hickford , M Hickford Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: M Hickford From: M Hickford `credential reject` sends the erase action to each helper, but the exact behaviour of erase isn't specified in documentation or tests. Some helpers (such as credential-libsecret) delete all matching credentials, others (such as credential-cache and credential-store) delete at most one matching credential. Test that helpers erase all matching credentials. This behaviour is easiest to reason about. Users expect that `echo "url=https://example.com" | git credential reject` or `echo "url=https://example.com\nusername=tim" | git credential reject` erase all matching credentials. Fix credential-cache and credential-store. Signed-off-by: M Hickford --- Documentation/git-credential.txt | 2 +- Documentation/gitcredentials.txt | 2 +- builtin/credential-cache--daemon.c | 15 ++++++++------ builtin/credential-store.c | 1 - t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++ 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index 0e6d9e85ec7..a220afed4f3 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -39,7 +39,7 @@ for later use. If the action is `reject`, git-credential will send the description to any configured credential helpers, which may erase any stored -credential matching the description. +credentials matching the description. If the action is `approve` or `reject`, no output should be emitted. diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt index 100f045bb1a..65d652dc40e 100644 --- a/Documentation/gitcredentials.txt +++ b/Documentation/gitcredentials.txt @@ -260,7 +260,7 @@ appended to its command line, which is one of: `erase`:: - Remove a matching credential, if any, from the helper's storage. + Remove matching credentials, if any, from the helper's storage. The details of the credential will be provided on the helper's stdin stream. The exact format is the same as the input/output format of the diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f64dd21d335..dc1cf2d25fc 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout) e->expiration = time(NULL) + timeout; } -static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password) +static struct credential_cache_entry *lookup_credential(const struct credential *c) { int i; for (i = 0; i < entries_nr; i++) { struct credential *e = &entries[i].item; - if (credential_match(c, e, match_password)) + if (credential_match(c, e, 0)) return &entries[i]; } return NULL; @@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c, int match_password) { struct credential_cache_entry *e; - e = lookup_credential(c, match_password); - if (e) - e->expiration = 0; + int i; + for (i = 0; i < entries_nr; i++) { + e = &entries[i]; + if (credential_match(c, &e->item, match_password)) + e->expiration = 0; + } } static timestamp_t check_expirations(void) @@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out) if (read_request(in, &c, &action, &timeout) < 0) /* ignore error */ ; else if (!strcmp(action.buf, "get")) { - struct credential_cache_entry *e = lookup_credential(&c, 0); + struct credential_cache_entry *e = lookup_credential(&c); if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); diff --git a/builtin/credential-store.c b/builtin/credential-store.c index f80ff59f18a..06bbfa4dd03 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -35,7 +35,6 @@ static int parse_credential_file(const char *fn, found_credential = 1; if (match_cb) { match_cb(&entry); - break; } } else if (other_cb) diff --git a/t/lib-credential.sh b/t/lib-credential.sh index 7b4fdd011d7..3f21ba66929 100644 --- a/t/lib-credential.sh +++ b/t/lib-credential.sh @@ -45,6 +45,8 @@ helper_test_clean() { reject $1 https example.com user2 reject $1 https example.com user4 reject $1 https example.com user5 + reject $1 https example.com user6 + reject $1 https example.com user7 reject $1 https example.com user8 reject $1 http path.tld user reject $1 https timeout.tld user @@ -342,6 +344,37 @@ helper_test() { EOF ' + test_expect_success "helper ($HELPER) erases all matching credentials" ' + check approve $HELPER <<-\EOF && + protocol=https + host=example.com + username=user6 + password=pass1 + EOF + check approve $HELPER <<-\EOF && + protocol=https + host=example.com + username=user7 + password=pass1 + EOF + check reject $HELPER <<-\EOF && + protocol=https + host=example.com + EOF + check fill $HELPER <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://example.com'\'': + askpass: Password for '\''https://askpass-username@example.com'\'': + EOF + ' + : ${GIT_TEST_LONG_CRED_BUFFER:=1024} # 23 bytes accounts for "wwwauth[]=basic realm=" plus NUL LONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))