Various code review fixes

Author: Tomas Urbonaitis

GVFS/GVFS.Common/Git/GitAuthentication.cs

@@ -33,10 +33,6 @@ public GitAuthentication(GitProcess git, string repoUrl)
             {
                 this.GitSsl = new GitSsl(configSettings);
             }
-            else
-            {
-                this.GitSsl = new GitSsl();
-            }
         }
 
         public bool IsBackingOff
@@ -166,12 +162,12 @@ public bool TryInitializeAndRequireAuth(ITracer tracer, out string errorMessage)
             return false;
         }
 
-        public void SetupSslIfNeeded(ITracer tracer, HttpClientHandler httpClientHandler, GitProcess gitProcess)
+        public void ConfigureHttpClientHandlerSslIfNeeded(ITracer tracer, HttpClientHandler httpClientHandler, GitProcess gitProcess)
         {
-            X509Certificate2 cert = this.GitSsl.GetCertificate(tracer, gitProcess);
+            X509Certificate2 cert = this.GitSsl?.GetCertificate(tracer, gitProcess);
             if (cert != null)
             {
-                if (!this.GitSsl.ShouldVerify)
+                if (this.GitSsl != null && !this.GitSsl.ShouldVerify)
                 {
                     httpClientHandler.ServerCertificateCustomValidationCallback =
                         (httpRequestMessage, c, cetChain, policyErrors) =>

GVFS/GVFS.Common/Git/GitConfigSetting.cs

@@ -9,7 +9,6 @@ public class GitConfigSetting
         public const string CredentialUseHttpPath = "credential.useHttpPath";
 
         public const string HttpSslCert = "http.sslcert";
-        public const string HttpSslKey = "http.sslkey";
         public const string HttpSslVerify = "http.sslverify";
         public const string HttpSslCertPasswordProtected = "http.sslcertpasswordprotected";
 

GVFS/GVFS.Common/Git/GitSsl.cs

@@ -17,7 +17,11 @@ public class GitSsl
         public GitSsl()
         {
             this.certificatePathOrSubjectCommonName = null;
+
             this.isCertificatePasswordProtected = false;
+
+            // True by default, both to have good default security settings and to match git behavior.
+            // https://git-scm.com/docs/git-config#git-config-httpsslVerify
             this.ShouldVerify = true;
         }
 
@@ -27,32 +31,26 @@ public GitSsl(IDictionary<string, GitConfigSetting> configSettings) : this()
             {
                 if (configSettings.TryGetValue(GitConfigSetting.HttpSslCert, out GitConfigSetting sslCerts))
                 {
-                    this.certificatePathOrSubjectCommonName = sslCerts.Values.Single();
+                    this.certificatePathOrSubjectCommonName = sslCerts.Values.Last();
                 }
 
                 if (configSettings.TryGetValue(GitConfigSetting.HttpSslCertPasswordProtected, out GitConfigSetting isSslCertPasswordProtected))
                 {
-                    this.isCertificatePasswordProtected = isSslCertPasswordProtected.Values.Select(bool.Parse).Single();
+                    this.isCertificatePasswordProtected = isSslCertPasswordProtected.Values.Select(bool.Parse).Last();
                 }
 
                 if (configSettings.TryGetValue(GitConfigSetting.HttpSslVerify, out GitConfigSetting sslVerify))
                 {
-                    this.ShouldVerify = sslVerify.Values.Select(bool.Parse).Single();
+                    this.ShouldVerify = sslVerify.Values.Select(bool.Parse).Last();
                 }
             }
         }
-
-        public bool ShouldVerify { get; }
-
-        public string GetCertificatePassword(ITracer tracer, GitProcess git)
-        {
-            if (git.TryGetCertificatePassword(tracer, this.certificatePathOrSubjectCommonName, out string password, out string error))
-            {
-                return password;
-            }
 
-            return null;
-        }
+        /// <summary>
+        /// Gets a value indicating whether SSL certificates being loaded should be verified. Also used to determine, whether client should verify server SSL certificate. True by default.
+        /// </summary>
+        /// <value><c>true</c> if should verify SSL certificates; otherwise, <c>false</c>.</value>
+        public bool ShouldVerify { get; }
 
         public X509Certificate2 GetCertificate(ITracer tracer, GitProcess gitProcess)
         {
@@ -74,33 +72,28 @@ public X509Certificate2 GetCertificate(ITracer tracer, GitProcess gitProcess)
 
             if (result == null)
             {
-                tracer.RelatedError("Certificate {0} not found", this.certificatePathOrSubjectCommonName);
+                tracer.RelatedError(metadata, $"Certificate {this.certificatePathOrSubjectCommonName} not found");
             }
 
             return result;
         }
 
         private static void LogWithAppropriateLevel(ITracer tracer, EventMetadata metadata, IEnumerable<X509Certificate2> certificates, string logMessage)
         {
-            Action<EventMetadata, string> loggingFunction;
             int numberOfCertificates = certificates.Count();
 
             switch (numberOfCertificates)
             {
                 case 0:
-                    loggingFunction = tracer.RelatedError;
+                    tracer.RelatedError(metadata, logMessage);
                     break;
                 case 1:
-                    loggingFunction = tracer.RelatedInfo;
+                    tracer.RelatedInfo(metadata, logMessage);
                     break;
                 default:
-                    loggingFunction = tracer.RelatedWarning;
+                    tracer.RelatedWarning(metadata, logMessage);
                     break;
             }
-
-            loggingFunction(
-                metadata,
-                logMessage);
         }
 
         private static string GetSubjectNameLineForLogging(IEnumerable<X509Certificate2> certificates)
@@ -110,6 +103,16 @@ private static string GetSubjectNameLineForLogging(IEnumerable<X509Certificate2>
                         certificates.Select(x => x.Subject));
         }
 
+        private string GetCertificatePassword(ITracer tracer, GitProcess git)
+        {
+            if (git.TryGetCertificatePassword(tracer, this.certificatePathOrSubjectCommonName, out string password, out string error))
+            {
+                return password;
+            }
+
+            return null;
+        }
+
         private X509Certificate2 GetCertificateFromFile(ITracer tracer, EventMetadata metadata, GitProcess gitProcess)
         {
             string certificatePassword = null;
@@ -120,10 +123,7 @@ private X509Certificate2 GetCertificateFromFile(ITracer tracer, EventMetadata me
                 if (string.IsNullOrEmpty(certificatePassword))
                 {
                     tracer.RelatedWarning(
-                        new EventMetadata
-                        {
-                                { "SslCertificate", this.certificatePathOrSubjectCommonName }
-                        },
+                        metadata,
                         "Git config indicates, that certificate is password protected, but retrieved password was null or empty!");
                 }
 
@@ -145,7 +145,7 @@ private X509Certificate2 GetCertificateFromFile(ITracer tracer, EventMetadata me
                 }
                 catch (CryptographicException cryptEx)
                 {
-                    metadata.Add("Exception", cryptEx);
+                    metadata.Add("Exception", cryptEx.ToString());
                     tracer.RelatedError(metadata, "Error, while loading certificate from disk");
                     return null;
                 }

GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs

@@ -88,12 +88,15 @@ public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSCo
                 httpStatus = httpException.StatusCode;
             }
 
-            this.Tracer.RelatedError(
-                new EventMetadata
-                {
-                    { "Exception", output.Error }
-                },
-                "TryQueryGVFSConfig failed");
+            if (logErrors)
+            {
+                this.Tracer.RelatedError(
+                    new EventMetadata
+                    {
+                    { "Exception", output.Error.ToString() }
+                    },
+                    $"{nameof(this.TryQueryGVFSConfig)} failed");
+            }
 
             return false;
         }

GVFS/GVFS.Common/Http/HttpRequestor.cs

@@ -42,7 +42,7 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli
 
             HttpClientHandler httpClientHandler = new HttpClientHandler() { UseDefaultCredentials = true };
 
-            this.authentication.SetupSslIfNeeded(this.Tracer, httpClientHandler, enlistment.CreateGitProcess());
+            this.authentication.ConfigureHttpClientHandlerSslIfNeeded(this.Tracer, httpClientHandler, enlistment.CreateGitProcess());
 
             this.client = new HttpClient(httpClientHandler)
             {

GVFS/GVFS.UnitTests/Common/GitConfigHelperTests.cs

@@ -106,7 +106,7 @@ public void ParseKeyValuesTest()
         [TestCase]
         public void ParseSpaceSeparatedKeyValuesTest()
         {
-      string input = @"
+            string input = @"
 core.gvfs true
 gc.auto 0
 section.key value1
@@ -116,7 +116,7 @@ section.key   value4
 section.KEY value5" +
 "\nsection.empty ";
 
-      Dictionary<string, GitConfigSetting> result = GitConfigHelper.ParseKeyValues(input, ' ');
+            Dictionary<string, GitConfigSetting> result = GitConfigHelper.ParseKeyValues(input, ' ');
 
             result.Count.ShouldEqual(4);
             result["core.gvfs"].Values.Single().ShouldEqual("true");

GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs

@@ -1,16 +1,24 @@
 using GVFS.Common.Git;
+using GVFS.Tests;
 using GVFS.Tests.Should;
 using GVFS.UnitTests.Mock.Common;
 using GVFS.UnitTests.Mock.Git;
 using NUnit.Framework;
 
 namespace GVFS.UnitTests.Git
 {
-    [TestFixture]
+    [TestFixtureSource(typeof(DataSources), nameof(DataSources.AllBools))]
     public class GitAuthenticationTests
     {
         private const string CertificatePath = "certificatePath";
 
+        private readonly bool sslSettingsPresent;
+
+        public GitAuthenticationTests(bool sslSettingsPresent)
+        {
+            this.sslSettingsPresent = sslSettingsPresent;
+        }
+
         [TestCase]
         public void AuthShouldBackoffAfterFirstRetryFailure()
         {
@@ -175,7 +183,15 @@ private MockGitProcess GetGitProcess()
             MockGitProcess gitProcess = new MockGitProcess();
             gitProcess.SetExpectedCommandResult("config gvfs.FunctionalTests.UserName", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode));
             gitProcess.SetExpectedCommandResult("config gvfs.FunctionalTests.Password", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode));
-            gitProcess.SetExpectedCommandResult("config --get-urlmatch http mock://repoUrl", () => new GitProcess.Result($"http.sslCert {CertificatePath}\nhttp.sslCertPasswordProtected true\n\n", string.Empty, GitProcess.Result.SuccessCode));
+
+            if (this.sslSettingsPresent)
+            {
+                gitProcess.SetExpectedCommandResult("config --get-urlmatch http mock://repoUrl", () => new GitProcess.Result($"http.sslCert {CertificatePath}\nhttp.sslCertPasswordProtected true\n\n", string.Empty, GitProcess.Result.SuccessCode));
+            }
+            else
+            {
+                gitProcess.SetExpectedCommandResult("config --get-urlmatch http mock://repoUrl", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode));
+            }
 
             int revocations = 0;
             gitProcess.SetExpectedCommandResult(

GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs

@@ -53,7 +53,7 @@ public void RelatedInfo(string message)
         public void RelatedInfo(EventMetadata metadata, string message)
         {
             metadata[TracingConstants.MessageKey.InfoMessage] = message;
-            this.RelatedWarningEvents.Add(JsonConvert.SerializeObject(metadata));
+            this.RelatedInfoEvents.Add(JsonConvert.SerializeObject(metadata));
         }
 
         public void RelatedInfo(string format, params object[] args)