Skip to content

Bound runtime credential fetch so prefetch can't hang on auth #2046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
tyrielv wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Bound runtime credential fetch so prefetch can't hang on auth #2046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions GVFS/GVFS.Common/GVFSConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ public static class GitConfig

public const string MaxHttpConnectionsConfig = GVFSPrefix + "max-http-connections";

/// <summary>
/// Overrides how long a runtime credential fetch may block waiting on the
/// credential manager, in seconds. 0 or negative restores the pre-bound
/// behavior of waiting indefinitely. Read once into <see cref="RetryConfig"/>.
/// </summary>
public const string CredentialTimeoutSeconds = GVFSPrefix + "credential-timeout-seconds";

public const string PrefetchUseIdx = GVFSPrefix + "prefetch-use-idx";
public const bool PrefetchUseIdxDefault = false;

Expand Down
55 changes: 39 additions & 16 deletions GVFS/GVFS.Common/Git/GitAuthentication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ public class GitAuthentication
public const int DefaultCredentialTimeoutMs = 30_000;
public const int BackgroundCredentialTimeoutMs = 120_000;

/// <summary>
/// Minimum time to wait for an in-flight credential fetch before giving up on
/// serialization. The effective wait is never shorter than the fetch timeout
/// itself, so a slow but legitimate prompt cannot cause a second prompt.
/// </summary>
private const int DefaultCredentialGateWaitMs = 60_000;

private readonly Lock gitAuthLock = new Lock();
private readonly SemaphoreSlim credentialGate = new SemaphoreSlim(1, 1);
private readonly ICredentialStore credentialStore;
Expand Down Expand Up @@ -68,7 +75,7 @@ public bool IsBackingOff

private GitSsl GitSsl { get; }

public void ApproveCredentials(ITracer tracer, string credentialString)
public void ApproveCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs)
{
lock (this.gitAuthLock)
{
Expand All @@ -86,7 +93,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString)
string password;
if (TryParseCredentialString(this.cachedCredentialString, out username, out password))
{
if (!this.credentialStore.TryStoreCredential(tracer, this.repoUrl, username, password, out string error))
if (!this.credentialStore.TryStoreCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs))
{
// Storing credentials is best effort attempt - log failure, but do not fail
tracer.RelatedWarning("Failed to store credential string: {0}", error);
Expand All @@ -107,7 +114,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString)
}
}

public void RejectCredentials(ITracer tracer, string credentialString)
public void RejectCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs)
{
lock (this.gitAuthLock)
{
Expand All @@ -118,7 +125,7 @@ public void RejectCredentials(ITracer tracer, string credentialString)
// We can't assume that the credential store's cached credential is the same as the one we have.
// Reload the credential from the store to ensure we're rejecting the correct one.
int attemptsBeforeCheckingExistingCredential = this.numberOfAttempts;
if (this.TryCallGitCredential(tracer, out string getCredentialError))
if (this.TryCallGitCredential(tracer, out string getCredentialError, out _, credentialTimeoutMs))
{
if (this.cachedCredentialString != cachedCredentialAtStartOfReject)
{
Expand All @@ -139,7 +146,7 @@ public void RejectCredentials(ITracer tracer, string credentialString)
string password;
if (TryParseCredentialString(this.cachedCredentialString, out username, out password))
{
if (!this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username, password, out string error))
if (!this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs))
{
// Deleting credentials is best effort attempt - log failure, but do not fail
tracer.RelatedWarning("Failed to delete credential string: {0}", error);
Expand All @@ -154,7 +161,7 @@ public void RejectCredentials(ITracer tracer, string credentialString)
["RepoUrl"] = this.repoUrl,
});
tracer.RelatedWarning(metadata, "Failed to parse credential string for rejection. Rejecting any credential for this repo URL.");
this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username: null, password: null, error: out string error);
this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username: null, password: null, error: out string error, timeoutMs: credentialTimeoutMs);
}

this.cachedCredentialString = null;
Expand All @@ -169,8 +176,20 @@ public void RejectCredentials(ITracer tracer, string credentialString)
}
}

public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage)
public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage, int credentialTimeoutMs = DefaultCredentialTimeoutMs)
{
return this.TryGetCredentials(tracer, out credentialString, out errorMessage, out _, credentialTimeoutMs);
}

/// <summary>
/// Fetches credentials, reporting via <paramref name="timedOut"/> whether the failure was
/// the credential manager exceeding its bound rather than a genuine auth failure. Callers
/// use this to avoid immediately retrying, which would re-prompt the user.
/// </summary>
public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage, out bool timedOut, int credentialTimeoutMs = DefaultCredentialTimeoutMs)
{
timedOut = false;

if (!this.isInitialized)
{
// Initialization may still be running in the background (mount can
Expand Down Expand Up @@ -199,7 +218,7 @@ public bool TryGetCredentials(ITracer tracer, out string credentialString, out s
return false;
}

if (!this.TryCallGitCredential(tracer, out errorMessage))
if (!this.TryCallGitCredential(tracer, out errorMessage, out timedOut, credentialTimeoutMs))
{
return false;
}
Expand Down Expand Up @@ -285,7 +304,7 @@ public bool TryInitializeAndQueryGVFSConfig(
// Server requires authentication — fetch credentials
this.IsAnonymous = false;

if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs))
if (!this.TryCallGitCredential(tracer, out errorMessage, out _, credentialTimeoutMs))
{
isAuthFailure = true;
// Mark initialized even on failure so TryGetCredentials can
Expand Down Expand Up @@ -327,7 +346,7 @@ internal bool TryInitializeAndRequireAuth(ITracer tracer, out string errorMessag
throw new InvalidOperationException("Already initialized");
}

if (this.TryCallGitCredential(tracer, out errorMessage))
if (this.TryCallGitCredential(tracer, out errorMessage, out _))
{
this.MarkInitialized();
return true;
Expand Down Expand Up @@ -419,20 +438,24 @@ private void MarkInitialized()
this.initializationComplete.Set();
}

private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int timeoutMs = -1)
private bool TryCallGitCredential(ITracer tracer, out string errorMessage, out bool timedOut, int timeoutMs = -1)
{
// Serialize credential fetches so only one git-credential-fill
// process runs at a time. Without this, a background auth task
// and a foreground object download could both spawn GCM prompts.
// Wait up to 60s for an in-flight fetch; if the gate is still
// held (e.g., background GCM prompt), fall through and let this
// caller spawn its own credential fetch.
bool acquired = this.credentialGate.Wait(60_000);
// Wait at least as long as the fetch itself may take; otherwise the
// gate would expire while the in-flight fetch is still legitimately
// waiting on the user, and we would fall through and spawn a second
// competing GCM prompt in exactly the slow-prompt case this bound
// exists to tolerate.
int gateTimeoutMs = timeoutMs < 0 ? DefaultCredentialGateWaitMs : Math.Max(DefaultCredentialGateWaitMs, timeoutMs);
bool acquired = this.credentialGate.Wait(gateTimeoutMs);
timedOut = false;
try
{
string gitUsername;
string gitPassword;
if (!this.credentialStore.TryGetCredential(tracer, this.repoUrl, out gitUsername, out gitPassword, out errorMessage, timeoutMs))
if (!this.credentialStore.TryGetCredential(tracer, this.repoUrl, out gitUsername, out gitPassword, out errorMessage, out timedOut, timeoutMs))
{
this.UpdateBackoff();
return false;
Expand Down
44 changes: 36 additions & 8 deletions GVFS/GVFS.Common/Git/GitProcess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using GVFS.Common.FileSystem;
using GVFS.Common.FileSystem;
using GVFS.Common.Tracing;
using System;
using System.Collections.Generic;
Expand Down Expand Up @@ -37,6 +37,12 @@ public class GitProcess : ICredentialStore
/// </summary>
private const int MaxCapturedStdOutChars = 128 * 1024 * 1024; // ~256 MB of UTF-16

/// <summary>
/// How long to wait for a killed process tree to actually exit before we give up
/// and read whatever the async stdout/stderr readers have captured so far.
/// </summary>
private const int ProcessKillTimeoutMs = 5_000;

private static readonly Encoding UTF8NoBOM = new UTF8Encoding(false);
private static bool failedToSetEncoding = false;
private static string expireTimeDateString;
Expand Down Expand Up @@ -192,7 +198,7 @@ public bool TryKillRunningProcess(out string processName, out int exitCode, out
}
}

public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage)
public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1)
{
StringBuilder sb = new StringBuilder();
sb.AppendFormat("url={0}\n", repoUrl);
Expand All @@ -214,7 +220,8 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u
GenerateCredentialVerbCommand("reject"),
stdin => stdin.Write(stdinConfig),
null,
usePreCommandHook: false);
usePreCommandHook: false,
timeoutMs: timeoutMs);

if (result.ExitCodeIsFailure)
{
Expand All @@ -228,7 +235,7 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u
return true;
}

public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage)
public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1)
{
StringBuilder sb = new StringBuilder();
sb.AppendFormat("url={0}\n", repoUrl);
Expand All @@ -242,7 +249,8 @@ public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string us
GenerateCredentialVerbCommand("approve"),
stdin => stdin.Write(stdinConfig),
null,
usePreCommandHook: false);
usePreCommandHook: false,
timeoutMs: timeoutMs);

if (result.ExitCodeIsFailure)
{
Expand Down Expand Up @@ -320,11 +328,13 @@ public virtual bool TryGetCredential(
out string username,
out string password,
out string errorMessage,
out bool timedOut,
int timeoutMs = -1)
{
username = null;
password = null;
errorMessage = null;
timedOut = false;

using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational))
{
Expand All @@ -343,10 +353,21 @@ public virtual bool TryGetCredential(

if (gitCredentialOutput.Errors.StartsWith("Operation timed out"))
{
timedOut = true;
errorMessage = "Credential manager did not respond within " + (timeoutMs / 1000) + " seconds";
tracer.RelatedWarning(

// Structured fields (not just message text) so the rate of this bound
// firing can be measured, and so a timeout can be correlated with a
// later successful fetch to tell "prevented a hang" apart from
// "cut off a prompt the user was about to answer".
errorData.Add("Area", nameof(GitProcess));
errorData.Add("Method", nameof(this.TryGetCredential));
errorData.Add("timeoutMs", timeoutMs);
errorData.Add("RepoUrl", repoUrl);
tracer.RelatedEvent(
EventLevel.Warning,
"CredentialFetchTimedOut",
errorData,
"Git credential fill timed out after " + timeoutMs + "ms",
Keywords.Network | Keywords.Telemetry);
}
else
Expand Down Expand Up @@ -1051,7 +1072,14 @@ protected virtual Result InvokeGitImpl(

if (!this.executingProcess.WaitForExit(timeoutMs))
{
this.executingProcess.Kill();
// Kill the entire process tree. Killing only git.exe would leave
// helper children (e.g. an interactive credential manager prompt)
// running, holding the credential store and showing orphaned UI.
this.executingProcess.Kill(entireProcessTree: true);

// Give the tree a bounded chance to actually exit so the async
// stdout/stderr readers flush before we read their buffers.
this.executingProcess.WaitForExit(ProcessKillTimeoutMs);

return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated);
}
Expand Down
6 changes: 3 additions & 3 deletions GVFS/GVFS.Common/Git/ICredentialStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ namespace GVFS.Common.Git
{
public interface ICredentialStore
{
bool TryGetCredential(ITracer tracer, string url, out string username, out string password, out string error, int timeoutMs = -1);
bool TryGetCredential(ITracer tracer, string url, out string username, out string password, out string error, out bool timedOut, int timeoutMs = -1);

bool TryStoreCredential(ITracer tracer, string url, string username, string password, out string error);
bool TryStoreCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1);

bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error);
bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1);
}
}
Loading
Loading