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
8 changes: 4 additions & 4 deletions GVFS/GVFS.Common/Git/GitAuthentication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,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 @@ -102,7 +102,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, credentialTimeoutMs))
{
if (this.cachedCredentialString != cachedCredentialAtStartOfReject)
{
Expand Down Expand Up @@ -153,7 +153,7 @@ 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)
{
if (!this.isInitialized)
{
Expand All @@ -173,7 +173,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, credentialTimeoutMs))
{
return false;
}
Expand Down
14 changes: 12 additions & 2 deletions GVFS/GVFS.Common/Http/HttpRequestor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli

protected ITracer Tracer { get; }

// Runtime credential fetches (object/pack downloads, incl. background
// maintenance prefetch) are bounded so a missed/ignored credential prompt
// can't hang forever. We use the generous BackgroundCredentialTimeoutMs
// (120s) rather than the 30s default: this same requestor is shared by
// interactive on-demand hydration and by the user-initiated prefetch/clone
// verbs, where a human may legitimately take longer than 30s to answer a
// GCM cold-start / MFA / smartcard prompt. 120s still bounds the hang while
// being long enough that a noticed prompt is not cut off spuriously.
protected virtual int CredentialTimeoutMs => GitAuthentication.BackgroundCredentialTimeoutMs;

public static long GetNewRequestId()
{
return Interlocked.Increment(ref requestCount);
Expand All @@ -109,7 +119,7 @@ protected GitEndPointResponseData SendRequest(
string authString = null;
string errorMessage;
if (!this.authentication.IsAnonymous &&
!this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage))
!this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage, this.CredentialTimeoutMs))
{
return new GitEndPointResponseData(
HttpStatusCode.Unauthorized,
Expand Down Expand Up @@ -234,7 +244,7 @@ protected GitEndPointResponseData SendRequest(
}
else if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.BadRequest || response.StatusCode == HttpStatusCode.Redirect)
{
this.authentication.RejectCredentials(this.Tracer, authString);
this.authentication.RejectCredentials(this.Tracer, authString, this.CredentialTimeoutMs);
if (!this.authentication.IsBackingOff)
{
errorMessage = string.Format("Server returned error code {0} ({1}). Your PAT may be expired and we are asking for a new one. Original error message from server: {2}", statusInt, response.StatusCode, errorMessage);
Expand Down
42 changes: 42 additions & 0 deletions GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,48 @@ public void RejectionShouldNotBeSentIfUnderlyingTokenHasChanged()
gitProcess.CredentialRejections.ShouldBeEmpty();
}

[TestCase]
public void TryGetCredentialsTimesOutWhenCredentialManagerDoesNotRespond()
{
MockTracer tracer = new MockTracer();
MockGitProcess gitProcess = this.GetGitProcess();

GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl");
dut.TryInitializeAndRequireAuth(tracer, out _);

string authString;
string err;
dut.TryGetCredentials(tracer, out authString, out err).ShouldEqual(true, "Initial credential fetch should succeed: " + err);

// Override the fill command to simulate a credential manager timeout
gitProcess.SetExpectedCommandResult(
$"{AzureDevOpsUseHttpPathString} credential fill",
() => new GitProcess.Result(string.Empty, "Operation timed out: git credential fill", GitProcess.Result.GenericFailureCode),
matchPrefix: true);

// Reject clears the cache so the next TryGetCredentials must refetch
dut.RejectCredentials(tracer, authString);

// The re-fetch should time out
dut.TryGetCredentials(tracer, out authString, out err, credentialTimeoutMs: 1000).ShouldEqual(false, "Expected timeout to cause failure");
err.ShouldContain("did not respond");
}

[TestCase]
public void TryGetCredentialsSucceedsWithExplicitTimeout()
{
MockTracer tracer = new MockTracer();
MockGitProcess gitProcess = this.GetGitProcess();

GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl");
dut.TryInitializeAndRequireAuth(tracer, out _);

string cred;
string err;
dut.TryGetCredentials(tracer, out cred, out err, credentialTimeoutMs: 30000).ShouldEqual(true, "Expected success with explicit timeout: " + err);
cred.ShouldNotBeNull();
}

private MockGitProcess GetGitProcess()
{
MockGitProcess gitProcess = new MockGitProcess();
Expand Down
Loading