From patchwork Wed Jun 14 21:40:37 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: M Hickford X-Patchwork-Id: 13280468 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 0930BEB64D9 for ; Wed, 14 Jun 2023 21:40:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231713AbjFNVkp (ORCPT ); Wed, 14 Jun 2023 17:40:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47210 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229554AbjFNVko (ORCPT ); Wed, 14 Jun 2023 17:40:44 -0400 Received: from mail-wr1-x435.google.com (mail-wr1-x435.google.com [IPv6:2a00:1450:4864:20::435]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EF68F268A for ; Wed, 14 Jun 2023 14:40:41 -0700 (PDT) Received: by mail-wr1-x435.google.com with SMTP id ffacd0b85a97d-30e3caa6aa7so7156317f8f.1 for ; Wed, 14 Jun 2023 14:40:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686778840; x=1689370840; 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=CBv1VcAAqTx5dof3yXNidamA5vfrduJYKrzYxppl+04=; b=Qfp0xx8Uf3us6qB4t0yt8niohVVdrXCOlj3u2B6+cbDEETQ+z1RAK6H8SUtGczuwY5 LdrBfeo6BCYWJDAEdw5PsL3EefnyLqmOH31z61JDHGd2Q4hbHV7YFmIxFm4aWorAgksp EcJFLMrprd6p933KyOU0T7gvgxnglQRlJvDJ55kfWj48JQKRMgwkdpB1LoS2W6YfmW30 FY+dtWvCD0R89Tz8EQf13gNXVFgXEZA4uMJaFxv7VTCkMqzQZMdu++zRYwAn5qsXJxGH lMtTZcK412pV718OU35W65D2b4cIqFHLxEDgQe1W8wGxjNUfyLjEwN+/7BMuvplEfdWX Kp8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686778840; x=1689370840; 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=CBv1VcAAqTx5dof3yXNidamA5vfrduJYKrzYxppl+04=; b=RlKJjtR75l5bf/vGG4mwA/K44MSc27fS3ktqQZLqPtdyVceuq6hwehF4A/vFujn6DH JollSPuyky81JGQc5IUL3fNJmQm4gHS+oeJvI2S+qw+bgJq53gBJtJZmZXzafo0jOP/n YiCd9argaSCljyuT45rP+eF4EmaxOs/jobMYOd/d930ZS3noFkYlPM7hdc2eJERDNx+a McR2J5TaCnsAPd8toWOvjLsqWEjuJkaWWRCiYt5ASmGkSbohSjN/kobOOBqBERY7fzp/ Yi2pWjIk3JDYuK9e1XmK3aKb7/txJAYJZ+YSDYdMSlTQPUVxW6POi/1GoWxjZGhw3SfB YXbg== X-Gm-Message-State: AC+VfDwTuSAbKuxig/Uaq5/w2ira2s3JoKZ/eMilHLGoZQQ9msyl/WaS Ic6JYCn/k7bNkw2g6eDQjuP1uxf+XRM= X-Google-Smtp-Source: ACHHUZ6Lqnx6LHClUY0lfxxXnhkeEy5BYPD8vVdUTOMA1DQUw0D5aHQVM5JvgQ6B5wsGVlyRwYZiVw== X-Received: by 2002:a05:6000:196e:b0:30e:5729:b5a5 with SMTP id da14-20020a056000196e00b0030e5729b5a5mr10620330wrb.38.1686778839814; Wed, 14 Jun 2023 14:40:39 -0700 (PDT) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id a8-20020a5d5088000000b0030f9c3219aasm17035583wrt.47.2023.06.14.14.40.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Jun 2023 14:40:39 -0700 (PDT) Message-Id: <35ee1795bcdb974cdb9ca0f55ddf8f8a5bc562ae.1686778838.git.gitgitgadget@gmail.com> In-Reply-To: References: Date: Wed, 14 Jun 2023 21:40:37 +0000 Subject: [PATCH v2 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 | 8 ++++---- builtin/credential-store.c | 18 ++++++++++-------- credential.c | 11 ++++++----- credential.h | 2 +- t/lib-credential.sh | 26 ++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 18 deletions(-) diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index 756c5f02aef..82f376d3351 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) +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; @@ -48,7 +48,7 @@ static void remove_credential(const struct credential *c) { struct credential_cache_entry *e; - e = lookup_credential(c); + e = lookup_credential(c, c->password != NULL); 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); diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 30c6ccf56c0..e0ae028b1c3 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -13,12 +13,14 @@ 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 *), + const char* op) { FILE *fh; struct strbuf line = STRBUF_INIT; struct credential entry = CREDENTIAL_INIT; int found_credential = 0; + const int match_password = !strcmp(op, "erase") && c->password != NULL; fh = fopen(fn, "r"); if (!fh) { @@ -29,8 +31,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 +62,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, const char *op) { int timeout_ms = 1000; @@ -69,7 +71,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, op); if (commit_lock_file(&credential_lock) < 0) die_errno("unable to write credential store"); } @@ -91,7 +93,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, "store"); strbuf_release(&buf); } @@ -138,7 +140,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, "erase"); } static void lookup_credential(const struct string_list *fns, struct credential *c) @@ -146,7 +148,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, "get")) 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..f7e4e29c5e1 100644 --- a/t/lib-credential.sh +++ b/t/lib-credential.sh @@ -44,6 +44,7 @@ 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 http path.tld user reject $1 https timeout.tld user reject $1 https sso.tld @@ -221,6 +222,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 Wed Jun 14 21:40:38 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: M Hickford X-Patchwork-Id: 13280469 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 EE7EDEB64DC for ; Wed, 14 Jun 2023 21:40:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229817AbjFNVkr (ORCPT ); Wed, 14 Jun 2023 17:40:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47212 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230487AbjFNVkp (ORCPT ); Wed, 14 Jun 2023 17:40:45 -0400 Received: from mail-wr1-x429.google.com (mail-wr1-x429.google.com [IPv6:2a00:1450:4864:20::429]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D2B1A180 for ; Wed, 14 Jun 2023 14:40:42 -0700 (PDT) Received: by mail-wr1-x429.google.com with SMTP id ffacd0b85a97d-30af56f5f52so5195590f8f.1 for ; Wed, 14 Jun 2023 14:40:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686778841; x=1689370841; 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=w3v/al3YVbEFfhDQKhnrk+4qNa0I2cPB73OttqA7p7o=; b=bJRSW8OaP493TZvop0QQ76OcaM8FrQKOY1SZO62TaMLfwocHBRyfKQ9S0obyF+WdeB 41zRCQM/rdAheTHLzio+Wbh8s2Ki2SPXOCbBAIwF7cmVb33gRgSdWRatbh8KydbBGMBR BnAiNjEiJDmAdCeFVWZ9rnW6HlrC2qh0b1LkjX6VIOP7OZpRveBbS88PMRvGPKHywnBP 8kBOVQjRW5ThqHnVz9xrZOz9MJeLXz4rDQcPom1Bix02l4lahPzGdgssYc0RS+y62h1T OhUIo2tfSt+eKmIOhOzGZjdPgeIyoc7tuvltiF5By61iA1bUhgXG68UW4dsTicy9z3A6 adWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686778841; x=1689370841; 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=w3v/al3YVbEFfhDQKhnrk+4qNa0I2cPB73OttqA7p7o=; b=K3qwXUDLx0GeKKhLCgJHKIFmLew0xa8AsU8qiIAhk0CW/btBiQPfzCWEBfZ4JfRSpR TmUqgG9jrMBmuWXDn98oSQZgbeDKPEoe2NpFBEa5njnOQWjsM26NP3zOPqUCAN4LCm2z yQg4HYeQscGZTWAKRdJSnVl6lYpUO6jPB1GTJ7q9TqpiWASRs6DL3SFqO+eGI6H/cmub /V4LGs18H5ssdH0Rn33zqZODSiNpnx05/mvvxQbNlSxYUnsA5+zGf/JTGCsNtzBeCjqK 5ndkYwefGLzJExi9Qr2RacUjyQdKWL1ztIhhm52wnU81mOKlgX00RxA5jIQi1ihpsmVc fsCA== X-Gm-Message-State: AC+VfDzYAsl1H5QBFhr8lNAt1liRO2yl0UvLDlT4/Ogg3DiH4rGERs5F WO/qK0soayLMWZbZd26P1KZw6x2//w8= X-Google-Smtp-Source: ACHHUZ6f38YdRdjcie5YRq1Y+aBNPXqBKIc3ahn4h+GrSmDRBZlUtUfTht8KXRSpaIyrfrsgQjf+aQ== X-Received: by 2002:adf:e906:0:b0:30f:be0f:fbf with SMTP id f6-20020adfe906000000b0030fbe0f0fbfmr7625724wrm.22.1686778840963; Wed, 14 Jun 2023 14:40:40 -0700 (PDT) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id z6-20020a5d4d06000000b0030fb98484f6sm12287875wrt.114.2023.06.14.14.40.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Jun 2023 14:40:40 -0700 (PDT) Message-Id: <9b12f17dc7ebbab9b1625ad0c6058133b8a53323.1686778838.git.gitgitgadget@gmail.com> In-Reply-To: References: Date: Wed, 14 Jun 2023 21:40:38 +0000 Subject: [PATCH v2 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 | 4 ++-- Documentation/gitcredentials.txt | 2 +- builtin/credential-cache--daemon.c | 15 ++++++++------ builtin/credential-store.c | 3 ++- t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index 0e6d9e85ec7..04bfb918de6 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential 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. +any configured credential helpers, which may erase stored 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 82f376d3351..5e3a766e42d 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) { struct credential_cache_entry *e; - e = lookup_credential(c, c->password != NULL); - if (e) - e->expiration = 0; + int i; + for (i = 0; i < entries_nr; i++) { + e = &entries[i]; + if (credential_match(c, &e->item, c->password != NULL)) + 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 e0ae028b1c3..85b147e460f 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn, found_credential = 1; if (match_cb) { match_cb(&entry); - break; + if (strcmp(op, "erase")) + break; } } else if (other_cb) diff --git a/t/lib-credential.sh b/t/lib-credential.sh index f7e4e29c5e1..3f4100b6ce2 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 http path.tld user reject $1 https timeout.tld user reject $1 https sso.tld @@ -298,6 +300,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))