maintenance: configure credentials to be silent #1798
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <stolee@gmail.com>
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 <stolee@gmail.com>
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 <stolee@gmail.com>
|
/submit |
|
Submitted as pull.1798.git.1726790423.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Add a new configuration value, 'credential.interactive', to specify to the
> credential helper that it should not prompt for user interaction. This
> option has been respected by Git Credential Manager since 2020 [1], so this
> is now presenting it as an official Git config value.
So, the other helpers are also supposed to check for the variable
and fail when it has to go interactive now.
> These changes were first merged into the microsoft/git fork in August 2023
> [2] but were not upstreamed immediately. The change has been a positive one
> for users of that fork, as they no longer get pop-ups and they also are not
> getting maintenance.lock file blocks when the prefetch task waits for
> credentials. This has become even more important recently as credential
> lifetimes have been restricted significantly, leading to a higher likelihood
> that this will happen during a background prefetch.
Sounds good.
|
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -9,6 +9,14 @@ credential.helper:: | |||
| Note that multiple helpers may be defined. See linkgit:gitcredentials[7] | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -501,8 +525,8 @@ void credential_fill(struct credential *c, int all_capabilities)
> c->helpers.items[i].string);
> }
>
> - credential_getpass(c);
> - if (!c->username && !c->password && !c->credential)
> + if (credential_getpass(c) ||
> + (!c->username && !c->password && !c->credential))
> die("unable to get password from user");
> }
This is a fallback mode after credential helpers have failed to fill
and return. Unless these helpers pay attention to the "interactive"
configuration, they may still get stuck. So it would be #leftoverbits
to update each credential helpers to do the right thing.
The sample credential-store backend does not have to be updated I
guess ;-)
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 7b5ab0eae16..ceb3336a5c4 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 &&
|
This patch series was integrated into seen via git@b52518a. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/20/24 5:56 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> Add a new configuration value, 'credential.interactive', to specify to the
>> credential helper that it should not prompt for user interaction. This
>> option has been respected by Git Credential Manager since 2020 [1], so this
>> is now presenting it as an official Git config value.
> > So, the other helpers are also supposed to check for the variable
> and fail when it has to go interactive now.
I would hold off from saying "supposed to" but Git is definitely hinting
towards that behavior.
Perhaps I'm just hung up on the idea that we are not adding a new wrinkle
to the "contract" but recommending a good thing that was previously not part
of the interaction.
Thanks,
-Stolee
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <stolee@gmail.com> writes:
> On 9/20/24 5:56 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> Add a new configuration value, 'credential.interactive', to specify to the
>>> credential helper that it should not prompt for user interaction. This
>>> option has been respected by Git Credential Manager since 2020 [1], so this
>>> is now presenting it as an official Git config value.
>> So, the other helpers are also supposed to check for the variable
>> and fail when it has to go interactive now.
>
> I would hold off from saying "supposed to" but Git is definitely hinting
> towards that behavior.
I would too. I didn't mean "they were behaving correctly, but we
changed the rules from under them and they need to be fixed". With
or without your patch, they would try to go interactive and make the
process get stuck, until they start to check if they should refrain
from going interactive. With your patch, they have a way to do that
check in a documented way.
> Perhaps I'm just hung up on the idea that we are not adding a new wrinkle
> to the "contract" but recommending a good thing that was previously not part
> of the interaction.
>
> Thanks,
> -Stolee |
|
This branch is now known as |
|
This patch series was integrated into seen via git@e47c2b8. |
|
There was a status update in the "Cooking" section about the branch Background tasks "git maintenance" runs may need to use credential information when going over the network, but a credential helper may work only in an interactive environment, and end up blocking a scheduled task waiting for UI. Credential helpers can now behave differently when they are not running interactively. Will merge to 'next'. source: <pull.1798.git.1726790423.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@fc54467. |
|
This patch series was integrated into next via git@379a7a1. |
|
This patch series was integrated into seen via git@c261349. |
|
There was a status update in the "Cooking" section about the branch Background tasks "git maintenance" runs may need to use credential information when going over the network, but a credential helper may work only in an interactive environment, and end up blocking a scheduled task waiting for UI. Credential helpers can now behave differently when they are not running interactively. Will merge to 'master'. source: <pull.1798.git.1726790423.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9617c17. |
|
There was a status update in the "Cooking" section about the branch Background tasks "git maintenance" runs may need to use credential information when going over the network, but a credential helper may work only in an interactive environment, and end up blocking a scheduled task waiting for UI. Credential helpers can now behave differently when they are not running interactively. Will merge to 'master'. source: <pull.1798.git.1726790423.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@a99e922. |
|
This patch series was integrated into seen via git@8999b8e. |
|
There was a status update in the "Cooking" section about the branch Background tasks "git maintenance" runs may need to use credential information when going over the network, but a credential helper may work only in an interactive environment, and end up blocking a scheduled task waiting for UI. Credential helpers can now behave differently when they are not running interactively. Will merge to 'master'. source: <pull.1798.git.1726790423.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@4251403. |
|
This patch series was integrated into master via git@4251403. |
|
This patch series was integrated into next via git@4251403. |
|
Closed via 4251403. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When background maintenance attempts to perform a prefetch to remote servers, this may trigger authentication requirements. If the credentials are expired, then the credential helper may need user input in order to get refreshed credentials. It is not a good experience for users to get credential pop-ups when not directly interacting with Git.
Add a new configuration value, 'credential.interactive', to specify to the credential helper that it should not prompt for user interaction. This option has been respected by Git Credential Manager since 2020 [1], so this is now presenting it as an official Git config value.
These changes were first merged into the microsoft/git fork in August 2023 [2] but were not upstreamed immediately. The change has been a positive one for users of that fork, as they no longer get pop-ups and they also are not getting maintenance.lock file blocks when the prefetch task waits for credentials. This has become even more important recently as credential lifetimes have been restricted significantly, leading to a higher likelihood that this will happen during a background prefetch.
I was reminded of these changes when liuzhongbo started a discussion [3] about maintenance.lock files and requesting that they are removed if they are stale. This does not address that issue directly, but is an important way to reduce the lifetime of maintenance.lock files when blocked on credential prompts.
[1] git-ecosystem/git-credential-manager#91
[2] microsoft#598
[3] https://lore.kernel.org/git/cce1d054-911e-407e-bc26-1c0bac4dd8e4@gmail.com/T/#t
Thanks, -Stolee
cc: gitster@pobox.com
cc: liuzhongbo.gg@gmail.com
cc: Johannes.Schindelin@gmx.de