Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
30f6c15
Load SSL certificate according to git config
Nov 13, 2018
2477b42
Fixing StyleCop issues
Nov 15, 2018
88de9dc
Fixing spacing
Nov 19, 2018
1e4e70c
Addressing minor code review comments
Nov 20, 2018
cac88c7
Moving SslSettings to GitAuthentication class
Nov 20, 2018
5035c30
Addressing StyleCop errors
Nov 20, 2018
68328ce
Fixing GitAuthenticationTests, adding test to verify GitSslSettings i…
Nov 20, 2018
c3cdab0
Fixing StyleCop issues
Nov 20, 2018
4c6c234
Using git config setting http.sslVerify to enable or disable client a…
Nov 21, 2018
fe9fcb0
Fixing StyleCop issues
Nov 21, 2018
08caea1
Renaming GitConfigConstants for SSL
Dec 7, 2018
c69cc57
Adding log messages, when certificate, or its password
Dec 7, 2018
5a86fb7
More StyleCop fixes
Dec 7, 2018
cd805fb
Refactoring GitSslSettings to be GitSsl and handle all the certificat…
Dec 10, 2018
2c1cb79
Renaming LoadCertificate to GetCertificate
Dec 11, 2018
9bceda1
Renaming GitSsl properties as per code review suggestions
Dec 11, 2018
4109b20
Refactoring certificate loading into different methods, when loading …
Dec 12, 2018
16fb49f
Making GitSsl non-disposable, to avoid IDisposable explosion. Address…
Dec 12, 2018
7e6a4e8
Substituting a var with explicit variable type
Dec 12, 2018
a8df303
Moving certificate password loading to GitSsl class and other minor r…
Dec 13, 2018
3915d47
Various code review fixes
Dec 14, 2018
a763194
Throwing InvalidRepoExcpetion, when ssl-related bool git config keys …
Dec 17, 2018
289aea3
Adding UnitTests to GitSsl and making it testable
Jan 15, 2019
4643c3b
Adjusting code to work on netstandard20
Jan 28, 2019
482d0fe
Ignoring GitSsl tests on non-dotnet core targets
Jan 29, 2019
9460af4
Addressing various coding style code review comments
Jan 29, 2019
113d334
Fixing spelling and naming
Feb 6, 2019
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
5 changes: 5 additions & 0 deletions GVFS/GVFS.Common/FileSystem/PhysicalFileSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ public virtual string ReadAllText(string path)
return File.ReadAllText(path);
}

public virtual byte[] ReadAllBytes(string path)
{
return File.ReadAllBytes(path);
}

public virtual IEnumerable<string> ReadLines(string path)
{
return File.ReadLines(path);
Expand Down
3 changes: 3 additions & 0 deletions GVFS/GVFS.Common/GVFS.Common.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,7 @@
</PackageReference>
<PackageReference Include="System.Security.Cryptography.Algorithms" Version="4.3.0" />
</ItemGroup>
<ItemGroup>
<Folder Include="X509Certificates\" />
</ItemGroup>
</Project>
29 changes: 29 additions & 0 deletions GVFS/GVFS.Common/Git/GitAuthentication.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
using GVFS.Common.Http;
using GVFS.Common.Tracing;
using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Http;
using System.Security.Cryptography.X509Certificates;
using System.Text;

namespace GVFS.Common.Git
Expand All @@ -25,6 +28,11 @@ public GitAuthentication(GitProcess git, string repoUrl)
{
this.git = git;
this.repoUrl = repoUrl;

if (git.TryGetConfigUrlMatch("http", this.repoUrl, out Dictionary<string, GitConfigSetting> configSettings))
{
this.GitSsl = new GitSsl(configSettings);
}
}

public bool IsBackingOff
Expand All @@ -37,6 +45,8 @@ public bool IsBackingOff

public bool IsAnonymous { get; private set; } = true;

private GitSsl GitSsl { get; }

public void ConfirmCredentialsWorked(string usedCredential)
{
lock (this.gitAuthLock)
Expand Down Expand Up @@ -152,6 +162,25 @@ public bool TryInitializeAndRequireAuth(ITracer tracer, out string errorMessage)
return false;
}

public void ConfigureHttpClientHandlerSslIfNeeded(ITracer tracer, HttpClientHandler httpClientHandler, GitProcess gitProcess)
{
X509Certificate2 cert = this.GitSsl?.GetCertificate(tracer, gitProcess);
if (cert != null)
{
if (this.GitSsl != null && !this.GitSsl.ShouldVerify)
{
httpClientHandler.ServerCertificateCustomValidationCallback =
(httpRequestMessage, c, cetChain, policyErrors) =>
{
return true;
};
}

httpClientHandler.ClientCertificateOptions = ClientCertificateOption.Manual;
httpClientHandler.ClientCertificates.Add(cert);
}
}

private bool TryAnonymousQuery(ITracer tracer, Enlistment enlistment, out bool isAnonymous)
{
bool querySucceeded;
Expand Down
10 changes: 6 additions & 4 deletions GVFS/GVFS.Common/Git/GitConfigHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,14 @@ public static Dictionary<string, GitConfigSetting> GetSettings(string[] configLi
/// section.settingName=settingValue
/// </summary>
/// <param name="input">The lines of text with the settings to parse.</param>
/// <param name="delimiter">The delimiter char, separating key from value</param>
/// <returns>A dictionary of settings, keyed off the setting name representing the settings parsed from input.</returns>
public static Dictionary<string, GitConfigSetting> ParseKeyValues(IEnumerable<string> input)
public static Dictionary<string, GitConfigSetting> ParseKeyValues(IEnumerable<string> input, char delimiter = '=')
{
Dictionary<string, GitConfigSetting> configSettings = new Dictionary<string, GitConfigSetting>(StringComparer.OrdinalIgnoreCase);
foreach (string line in input)
{
string[] fields = line.Trim().Split(new[] { '=' }, 2, StringSplitOptions.None);
Comment thread
turbonaitis marked this conversation as resolved.
string[] fields = line.Split(new[] { delimiter }, 2, StringSplitOptions.None);

if (fields.Length > 0)
{
Expand Down Expand Up @@ -128,10 +129,11 @@ public static Dictionary<string, GitConfigSetting> ParseKeyValues(IEnumerable<st
/// section.settingNameN=settingValueN
/// </summary>
/// <param name="input">The settings as text.</param>
/// <param name="delimiter">The delimiter char, separating key from value</param>
/// <returns>A dictionary of settings, keyed off the setting name representing the settings parsed from input.</returns>
public static Dictionary<string, GitConfigSetting> ParseKeyValues(string input)
public static Dictionary<string, GitConfigSetting> ParseKeyValues(string input, char delimiter = '=')
{
return ParseKeyValues(input.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries));
return ParseKeyValues(input.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries), delimiter);
}
}
}
4 changes: 4 additions & 0 deletions GVFS/GVFS.Common/Git/GitConfigSetting.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ public class GitConfigSetting
public const string CoreVirtualFileSystemName = "core.virtualfilesystem";
public const string CredentialUseHttpPath = "credential.useHttpPath";

public const string HttpSslCert = "http.sslcert";
public const string HttpSslVerify = "http.sslverify";
public const string HttpSslCertPasswordProtected = "http.sslcertpasswordprotected";

public GitConfigSetting(string name, params string[] values)
{
this.Name = name;
Expand Down
68 changes: 68 additions & 0 deletions GVFS/GVFS.Common/Git/GitProcess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,61 @@ public virtual void RevokeCredential(string repoUrl)
null);
}

/// <summary>
/// Input for certificate credentials looks like
/// <code> protocol=cert
/// path=[http.sslCert value]
/// username =</code>
/// </summary>
public virtual bool TryGetCertificatePassword(
ITracer tracer,
string certificatePath,
out string password,
out string errorMessage)
{
password = null;
errorMessage = null;

using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational))
{
Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this on Windows?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, sadly I do not have access to a windows machine. Do you think something in particular may behave differently here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prompting for a password from potentially a background process is just a flow we don't have good automation around, which is why I was asking.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrbriggs @turbonaitis, what is the status of this discussion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this doesn't differ from TryGetCredentials, a method you already have. It invokes the same git credential helper, with the same potential of being invoked from a background process. So the way I see it, I'm not introducing anything new here. And yes, when credentials change, while VFS4G is running, and when getting new credentials requires user interaction in terminal (be it for certificate or for http), bad things will happen, but that's something, that is already in the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilbaker do you agree with this? Or do you think that my changes introduce some additional risks?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turbonaitis your explanation makes sense to me. @jrbriggs is more familiar with the GCM auth flows and I'd like to hear his thoughts as well.

"credential fill",
stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"),
parseStdOutLine: null);

if (gitCredentialOutput.ExitCodeIsFailure)
{
EventMetadata errorData = new EventMetadata();
Comment thread
turbonaitis marked this conversation as resolved.
errorData.Add("CertificatePath", certificatePath);
tracer.RelatedWarning(
errorData,
"Git could not get credentials: " + gitCredentialOutput.Errors,
Keywords.Network | Keywords.Telemetry);
errorMessage = gitCredentialOutput.Errors;

return false;
}

password = ParseValue(gitCredentialOutput.Output, "password=");

bool success = password != null;
Comment thread
turbonaitis marked this conversation as resolved.

EventMetadata metadata = new EventMetadata
{
{ "Success", success },
{ "CertificatePath", certificatePath }
};

if (!success)
{
metadata.Add("Output", gitCredentialOutput.Output);
}

activity.Stop(metadata);
return success;
}
}

public virtual bool TryGetCredentials(
ITracer tracer,
string repoUrl,
Expand Down Expand Up @@ -259,6 +314,19 @@ public Result SetInFileConfig(string configFile, string settingName, string valu
value));
}

public bool TryGetConfigUrlMatch(string section, string repositoryUrl, out Dictionary<string, GitConfigSetting> configSettings)
{
Result result = this.InvokeGitAgainstDotGitFolder($"config --get-urlmatch {section} {repositoryUrl}");
if (result.ExitCodeIsFailure)
{
configSettings = null;
return false;
}

configSettings = GitConfigHelper.ParseKeyValues(result.Output, ' ');
return true;
}

public bool TryGetAllConfig(bool localOnly, out Dictionary<string, GitConfigSetting> configSettings)
{
configSettings = null;
Expand Down
Loading