diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 03cc27b417..b2f3612679 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -306,7 +306,8 @@ public virtual bool TryGetCertificatePassword( if (!success) { - metadata.Add("Output", gitCredentialOutput.Output); + // Never trace the raw output: it can contain the secret itself. + metadata.Add("OutputKeys", GetCredentialOutputKeys(gitCredentialOutput.Output)); } activity.Stop(metadata); @@ -370,7 +371,8 @@ public virtual bool TryGetCredential( metadata.Add("Success", success); if (!success) { - metadata.Add("Output", gitCredentialOutput.Output); + // Never trace the raw output: it can contain the secret itself. + metadata.Add("OutputKeys", GetCredentialOutputKeys(gitCredentialOutput.Output)); } activity.Stop(metadata); @@ -1075,6 +1077,32 @@ private static string GenerateCredentialVerbCommand(string verb) return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; } + /// + /// Summarizes the output of "git credential fill" for diagnostics. + /// The output is a list of "key=value" lines that can include the + /// plaintext secret, so only the key names are returned. The values + /// must never reach telemetry or the log. + /// + private static string GetCredentialOutputKeys(string credentialOutput) + { + if (string.IsNullOrEmpty(credentialOutput)) + { + return string.Empty; + } + + IEnumerable keys = credentialOutput + .Split('\n') + .Select(line => line.Trim('\r')) + .Where(line => !string.IsNullOrWhiteSpace(line)) + .Select(line => + { + int separatorIndex = line.IndexOf('='); + return separatorIndex > 0 ? line.Substring(0, separatorIndex) : ""; + }); + + return string.Join(",", keys); + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; diff --git a/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs b/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs new file mode 100644 index 0000000000..550bc6e14d --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs @@ -0,0 +1,79 @@ +using GVFS.Common.Git; +using GVFS.Common.Tracing; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using GVFS.UnitTests.Mock.Git; +using NUnit.Framework; +using System.Linq; + +namespace GVFS.UnitTests.Common.Git +{ + [TestFixture] + public class GitProcessCredentialTests + { + private const string SecretValue = "S3cretSentinelValueThatMustNotBeTraced"; + private const string AzureDevOpsUseHttpPathString = "-c credential.\"https://dev.azure.com\".useHttpPath=true"; + + [TestCase] + public void TryGetCredentialDoesNotTraceSecretWhenParseFails() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + + // The secret is on the last line and has no terminating newline, so the parse fails. + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result( + "protocol=https\nhost=example.com\nusername=someone\npassword=" + SecretValue, + string.Empty, + GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCredential(tracer, "mock://repoUrl", out _, out _, out _) + .ShouldBeFalse("Parse of the credential output must fail for this test"); + + EventMetadata metadata = GetActivityMetadata(tracer); + AssertNoSecret(metadata); + metadata["OutputKeys"].ShouldEqual("protocol,host,username,password"); + } + + [TestCase] + public void TryGetCertificatePasswordDoesNotTraceSecretWhenParseFails() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + + // The secret is on the last line and has no terminating newline, so the parse fails. + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result( + "protocol=cert\npath=mock://certificate\npassword=" + SecretValue, + string.Empty, + GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCertificatePassword(tracer, "mock://certificate", out _, out _) + .ShouldBeFalse("Parse of the credential output must fail for this test"); + + EventMetadata metadata = GetActivityMetadata(tracer); + AssertNoSecret(metadata); + metadata["OutputKeys"].ShouldEqual("protocol,path,password"); + } + + private static EventMetadata GetActivityMetadata(MockTracer tracer) + { + MockTracer activityTracer = tracer.StartActivityTracer; + activityTracer.ShouldNotBeNull("The credential call must start an activity"); + activityTracer.StoppedActivityMetadata.Count.ShouldEqual(1); + + return activityTracer.StoppedActivityMetadata.Single(); + } + + private static void AssertNoSecret(EventMetadata metadata) + { + foreach (object value in metadata.Values) + { + string text = value?.ToString() ?? string.Empty; + text.Contains(SecretValue).ShouldBeFalse("Credential output must not be traced: " + text); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index d933584e94..dbe4c947a8 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs @@ -17,6 +17,7 @@ public MockTracer() this.RelatedWarningEvents = new List(); this.RelatedErrorEvents = new List(); this.RelatedEventNames = new List(); + this.StoppedActivityMetadata = new List(); } public MockTracer StartActivityTracer { get; private set; } @@ -30,6 +31,10 @@ public MockTracer() // do not otherwise get recorded). Lets tests assert a specific diagnostic event fired. public List RelatedEventNames { get; } + // Metadata passed to Stop when an activity ends. Lets tests assert on + // what an activity reports, including that a secret is not present. + public List StoppedActivityMetadata { get; } + public void WaitForRelatedEvent() { this.waitEvent.WaitOne(); @@ -136,6 +141,11 @@ public ITracer StartActivity(string activityName, EventLevel level, Keywords sta public TimeSpan Stop(EventMetadata metadata) { + if (metadata != null) + { + this.StoppedActivityMetadata.Add(metadata); + } + return TimeSpan.Zero; }