From 1eae5bd0b06b3f349db52795828cbc10e84404f8 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Aug 2023 10:52:29 -0400 Subject: [PATCH 1/5] fixup! maintenance: delete stale lock files This change from microsoft/git#468 is causing multiple maintenance processes to get blocked on credentials instead of only one. The change did more harm than good. This reverts commit 95ed7f6a2b7b99dfdb1910e8ef7ed85207c27e61. --- builtin/gc.c | 21 --------------------- t/t7900-maintenance.sh | 17 ----------------- 2 files changed, 38 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index b35af4518400d3..28c5247e33cff0 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1309,8 +1309,6 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts) char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path); if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) { - struct stat st; - struct strbuf lock_dot_lock = STRBUF_INIT; /* * Another maintenance command is running. * @@ -1321,25 +1319,6 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts) if (!opts->auto_flag && !opts->quiet) warning(_("lock file '%s' exists, skipping maintenance"), lock_path); - - /* - * Check timestamp on .lock file to see if we should - * delete it to recover from a fail state. - */ - strbuf_addstr(&lock_dot_lock, lock_path); - strbuf_addstr(&lock_dot_lock, ".lock"); - if (lstat(lock_dot_lock.buf, &st)) - warning_errno(_("unable to stat '%s'"), lock_dot_lock.buf); - else { - if (st.st_mtime < time(NULL) - (6 * 60 * 60)) { - if (unlink(lock_dot_lock.buf)) - warning_errno(_("unable to delete stale lock file")); - else - warning(_("deleted stale lock file")); - } - } - - strbuf_release(&lock_dot_lock); free(lock_path); return 0; } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 6c483d6573ab85..e56f5980dc488e 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -54,23 +54,6 @@ test_expect_success 'run [--auto|--quiet]' ' test_subcommand git gc --no-quiet err && - grep "lock file .* exists, skipping maintenance" err && - - test-tool chmtime =-22000 .git/objects/maintenance.lock && - git maintenance run --schedule=hourly --no-quiet 2>err && - grep "deleted stale lock file" err && - test_path_is_missing .git/objects/maintenance.lock && - - git maintenance run --schedule=hourly 2>err && - test_must_be_empty err -' - test_expect_success 'maintenance.auto config option' ' GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 && test_subcommand git maintenance run --auto --quiet Date: Tue, 22 Aug 2023 11:24:10 -0400 Subject: [PATCH 2/5] credential: add new interactive config option When scripts or background maintenance wish to perform HTTP(S) requests, there is a risk that our stored credentials might be invalid. At the moment, this causes the credential helper to ping the user and block the process. Even if the credential helper does not ping the user, Git falls back to the 'askpass' method, which includes a direct ping to the user via the terminal. Even setting the 'core.askPass' config as something like 'echo' will causes Git to fallback to a terminal prompt. It uses git_terminal_prompt(), which finds the terminal from the environment and ignores whether stdin has been redirected. This can also block the process awaiting input. Create a new config option to prevent user interaction, favoring a failure to a blocked process. The chosen name, 'credential.interactive', is taken from the config option used by Git Credential Manager to already avoid user interactivity, so there is already one credential helper that integrates with this option. However, older versions of Git Credential Manager also accepted other string values, including 'auto', 'never', and 'always'. The modern use is to use a boolean value, but we should still be careful that some users could have these non-booleans. Further, we should respect 'never' the same as 'false'. This is respected by the implementation and test, but not mentioned in the documentation. The implementation for the Git interactions takes place within credential_getpass(). The method prototype is modified to return an 'int' instead of 'void'. This allows us to detect that no attempt was made to fill the given credential, changing the single caller slightly. Also, a new trace2 region is added around the interactive portion of the credential request. This provides a way to measure the amount of time spent in that region for commands that _are_ interactive. It also makes a conventient way to test that the config option works with 'test_region'. Signed-off-by: Derrick Stolee --- Documentation/config/credential.txt | 8 ++++++++ credential.c | 30 ++++++++++++++++++++++++++--- t/t5551-http-fetch-smart.sh | 22 +++++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt index 512f31876e17ed..0ed570ff979629 100644 --- a/Documentation/config/credential.txt +++ b/Documentation/config/credential.txt @@ -9,6 +9,14 @@ credential.helper:: Note that multiple helpers may be defined. See linkgit:gitcredentials[7] for details and examples. +credential.interactive:: + By default, Git and any configured credential helpers will ask for + user input when new credentials are required. Many of these helpers + will succeed based on stored credentials if those credentials are + still valid. To avoid the possibility of user interactivity from + Git, set `credential.interactive=false`. Some credential helpers + respect this option as well. + credential.useHttpPath:: When acquiring credentials, consider the "path" component of an http or https URL to be important. Defaults to false. See diff --git a/credential.c b/credential.c index 369fad17ad67dc..d9c6277fdda612 100644 --- a/credential.c +++ b/credential.c @@ -11,6 +11,8 @@ #include "strbuf.h" #include "urlmatch.h" #include "git-compat-util.h" +#include "trace2.h" +#include "repository.h" void credential_init(struct credential *c) { @@ -196,14 +198,36 @@ static char *credential_ask_one(const char *what, struct credential *c, return xstrdup(r); } -static void credential_getpass(struct credential *c) +static int credential_getpass(struct credential *c) { + int interactive; + char *value; + if (!git_config_get_maybe_bool("credential.interactive", &interactive) && + !interactive) { + trace2_data_intmax("credential", the_repository, + "interactive/skipped", 1); + return -1; + } + if (!git_config_get_string("credential.interactive", &value)) { + int same = !strcmp(value, "never"); + free(value); + if (same) { + trace2_data_intmax("credential", the_repository, + "interactive/skipped", 1); + return -1; + } + } + + trace2_region_enter("credential", "interactive", the_repository); if (!c->username) c->username = credential_ask_one("Username", c, PROMPT_ASKPASS|PROMPT_ECHO); if (!c->password) c->password = credential_ask_one("Password", c, PROMPT_ASKPASS); + trace2_region_leave("credential", "interactive", the_repository); + + return 0; } int credential_read(struct credential *c, FILE *fp) @@ -381,8 +405,8 @@ void credential_fill(struct credential *c) c->helpers.items[i].string); } - credential_getpass(c); - if (!c->username && !c->password) + if (credential_getpass(c) || + (!c->username && !c->password)) die("unable to get password from user"); } diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 21b7767cbd313b..6e70bca95a4c64 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -186,6 +186,28 @@ test_expect_success 'clone from password-protected repository' ' test_cmp expect actual ' +test_expect_success 'credential.interactive=false skips askpass' ' + set_askpass bogus nonsense && + ( + GIT_TRACE2_EVENT="$(pwd)/interactive-true" && + export GIT_TRACE2_EVENT && + test_must_fail git clone --bare "$HTTPD_URL/auth/smart/repo.git" interactive-true-dir && + test_region credential interactive interactive-true && + + GIT_TRACE2_EVENT="$(pwd)/interactive-false" && + export GIT_TRACE2_EVENT && + test_must_fail git -c credential.interactive=false \ + clone --bare "$HTTPD_URL/auth/smart/repo.git" interactive-false-dir && + test_region ! credential interactive interactive-false && + + GIT_TRACE2_EVENT="$(pwd)/interactive-never" && + export GIT_TRACE2_EVENT && + test_must_fail git -c credential.interactive=never \ + clone --bare "$HTTPD_URL/auth/smart/repo.git" interactive-never-dir && + test_region ! credential interactive interactive-never + ) +' + test_expect_success 'clone from auth-only-for-push repository' ' echo two >expect && set_askpass wrong && From 1fe12923127e61ddbbd7c080c17edd7c2f8db7fe Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 16 Aug 2023 13:32:02 -0400 Subject: [PATCH 3/5] maintenance: add custom config to background jobs At the moment, some background jobs are getting blocked on credentials during the 'prefetch' task. This leads to other tasks, such as incremental repacks, getting blocked. Further, if a user manages to fix their credentials, then they still need to cancel the background process before their background maintenance can continue working. Update the background schedules for our four scheduler integrations to include these config options via '-c' options: * 'credential.interactive=false' will stop Git and some credential helpers from prompting in the UI (assuming the '-c' parameters are carried through and respected by GCM). * 'core.askPass=true' will replace the text fallback for a username and password into the 'true' command, which will return a success in its exit code, but Git will treat the empty string returned as an invalid password and move on. We can do some testing that the credentials are passed, at least in the systemd case due to writing the service files. Signed-off-by: Derrick Stolee --- builtin/gc.c | 53 ++++++++++++++++++++++++++++++++++++------ t/t7900-maintenance.sh | 3 +++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 28c5247e33cff0..ac30c9d01a3efa 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1657,6 +1657,42 @@ static const char *get_frequency(enum schedule_priority schedule) } } +static const char *extraconfig[] = { + "credential.interactive=false", + "core.askPass=true", /* 'true' returns success, but no output. */ + NULL +}; + +static const char *get_extra_config_parameters(void) { + static const char *result = NULL; + struct strbuf builder = STRBUF_INIT; + + if (result) + return result; + + for (const char **s = extraconfig; s && *s; s++) + strbuf_addf(&builder, "-c %s ", *s); + + result = strbuf_detach(&builder, NULL); + return result; +} + +static const char *get_extra_launchctl_strings(void) { + static const char *result = NULL; + struct strbuf builder = STRBUF_INIT; + + if (result) + return result; + + for (const char **s = extraconfig; s && *s; s++) { + strbuf_addstr(&builder, "-c\n"); + strbuf_addf(&builder, "%s\n", *s); + } + + result = strbuf_detach(&builder, NULL); + return result; +} + /* * get_schedule_cmd` reads the GIT_TEST_MAINT_SCHEDULER environment variable * to mock the schedulers that `git maintenance start` rely on. @@ -1863,6 +1899,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit "\n" "%s/git\n" "--exec-path=%s\n" + "%s" /* For extra config parameters. */ "for-each-repo\n" "--config=maintenance.repo\n" "maintenance\n" @@ -1871,7 +1908,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit "\n" "StartCalendarInterval\n" "\n"; - strbuf_addf(&plist, preamble, name, exec_path, exec_path, frequency); + strbuf_addf(&plist, preamble, name, exec_path, exec_path, + get_extra_launchctl_strings(), frequency); switch (schedule) { case SCHEDULE_HOURLY: @@ -2106,11 +2144,12 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority "\n" "\n" "\"%s\\headless-git.exe\"\n" - "--exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s\n" + "--exec-path=\"%s\" %s for-each-repo --config=maintenance.repo maintenance run --schedule=%s\n" "\n" "\n" "\n"; - fprintf(tfile->fp, xml, exec_path, exec_path, frequency); + fprintf(tfile->fp, xml, exec_path, exec_path, + get_extra_config_parameters(), frequency); strvec_split(&child.args, cmd); strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", get_tempfile_path(tfile), NULL); @@ -2251,8 +2290,8 @@ static int crontab_update_schedule(int run_maintenance, int fd) "# replaced in the future by a Git command.\n\n"); strbuf_addf(&line_format, - "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n", - exec_path, exec_path); + "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" %s for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n", + exec_path, exec_path, get_extra_config_parameters()); fprintf(cron_in, line_format.buf, minute, "1-23", "*", "hourly"); fprintf(cron_in, line_format.buf, minute, "0", "1-6", "daily"); fprintf(cron_in, line_format.buf, minute, "0", "0", "weekly"); @@ -2452,7 +2491,7 @@ static int systemd_timer_write_service_template(const char *exec_path) "\n" "[Service]\n" "Type=oneshot\n" - "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" + "ExecStart=\"%s/git\" --exec-path=\"%s\" %s for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" "LockPersonality=yes\n" "MemoryDenyWriteExecute=yes\n" "NoNewPrivileges=yes\n" @@ -2462,7 +2501,7 @@ static int systemd_timer_write_service_template(const char *exec_path) "RestrictSUIDSGID=yes\n" "SystemCallArchitectures=native\n" "SystemCallFilter=@system-service\n"; - if (fprintf(file, unit, exec_path, exec_path) < 0) { + if (fprintf(file, unit, exec_path, exec_path, get_extra_config_parameters()) < 0) { error(_("failed to write to '%s'"), filename); fclose(file); goto error; diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index e56f5980dc488e..083a20a9f7ae84 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -754,6 +754,9 @@ test_expect_success 'start and stop Linux/systemd maintenance' ' test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" && test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" && + grep "core.askPass=true" "systemd/user/git-maintenance@.service" && + grep "credential.interactive=false" "systemd/user/git-maintenance@.service" && + printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && test_cmp expect args && From a18c320056e1981f2b70274550fe17fa7d010af6 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 21 Aug 2023 11:14:48 -0400 Subject: [PATCH 4/5] scalar: configure maintenance during 'reconfigure' The 'scalar reconfigure' command is intended to update registered repos with the latest settings available. However, up to now we were not reregistering the repos with background maintenance. In particular, this meant that the background maintenance schedule would not be updated if there are improvements between versions. Be sure to register repos for maintenance during the reconfigure step. Signed-off-by: Derrick Stolee --- scalar.c | 3 +++ t/t9210-scalar.sh | 7 +++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/scalar.c b/scalar.c index 142672976c6584..3030202c9794d4 100644 --- a/scalar.c +++ b/scalar.c @@ -1098,6 +1098,9 @@ static int cmd_reconfigure(int argc, const char **argv) if (set_recommended_config(1) < 0) failed = -1; + if (toggle_maintenance(1) < 0) + failed = -1; + loop_end: if (failed) { res = failed; diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index 43c4208cdc31b1..95036693e10dc2 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -176,8 +176,11 @@ test_expect_success 'scalar reconfigure' ' scalar reconfigure one && test true = "$(git -C one/src config core.preloadIndex)" && git -C one/src config core.preloadIndex false && - scalar reconfigure -a && - test true = "$(git -C one/src config core.preloadIndex)" + rm one/src/cron.txt && + GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a && + test_path_is_file one/src/cron.txt && + test true = "$(git -C one/src config core.preloadIndex)" && + test_subcommand git maintenance start Date: Tue, 22 Aug 2023 12:16:50 -0400 Subject: [PATCH 5/5] fixup! scalar: do initialize `gvfs.sharedCache` In this commit, we added the 'credential.interactive=never' option to unattended scalar options. This should be changed to 'false' to match the modern use of this config option. But also, we have a test that requires using askpass to get credentials, but the test is in unattended mode. Fix that test to include 'credential.interactive=true' to bypass this issue. --- scalar.c | 2 +- t/t9210-scalar.sh | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/scalar.c b/scalar.c index 3030202c9794d4..120daea1f6e116 100644 --- a/scalar.c +++ b/scalar.c @@ -1403,7 +1403,7 @@ int cmd_main(int argc, const char **argv) if (is_unattended()) { setenv("GIT_ASKPASS", "", 0); setenv("GIT_TERMINAL_PROMPT", "false", 0); - git_config_push_parameter("credential.interactive=never"); + git_config_push_parameter("credential.interactive=false"); } while (argc > 1 && *argv[1] == '-') { diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index 95036693e10dc2..fbbf641881ac3f 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -303,7 +303,11 @@ test_expect_success 'start GVFS-enabled server' ' test_expect_success '`scalar clone` with GVFS-enabled server' ' : the fake cache server requires fake authentication && git config --global core.askPass true && - scalar clone --single-branch -- http://$HOST_PORT/ using-gvfs && + + # We must set credential.interactive=true to bypass a setting + # in "scalar clone" that disables interactive credentials during + # an unattended command. + scalar -c credential.interactive=true clone --single-branch -- http://$HOST_PORT/ using-gvfs && : verify that the shared cache has been configured && cache_key="url_$(printf "%s" http://$HOST_PORT/ |