Load SSL certificate according to git config#512
Conversation
|
|
|
|
turbonaitis
force-pushed
the
ssl
branch
2 times, most recently
from
7218810 to
aa9933e
Compare
jrbriggs
left a comment
jrbriggs
left a comment
There was a problem hiding this comment.
Thanks for the contribution! A few questions.
|
|
||
| using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) | ||
| { | ||
| Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( |
There was a problem hiding this comment.
Have you tested this on Windows?
There was a problem hiding this comment.
No, sadly I do not have access to a windows machine. Do you think something in particular may behave differently here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@jrbriggs @turbonaitis, what is the status of this discussion?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@wilbaker do you agree with this? Or do you think that my changes introduce some additional risks?
There was a problem hiding this comment.
@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.
|
@jrbriggs I think I've addressed all of the comments above. Are there any other outstanding actions, that I should take, before we can merge this PR? |
jrbriggs
left a comment
jrbriggs
left a comment
There was a problem hiding this comment.
Thanks for the changes. We're almost there. Just a couple of nits and a question.
jrbriggs
left a comment
jrbriggs
left a comment
There was a problem hiding this comment.
This looks good, thanks for the changes.
|
There's a couple of stylecop warnings to fix. |
kewillford
left a comment
kewillford
left a comment
There was a problem hiding this comment.
A few questions and suggested refactoring.
kewillford
left a comment
kewillford
left a comment
There was a problem hiding this comment.
Looking good. Only outstanding question for me is handling of the results of FindBySubjectName Thanks.
|
Rebased on latest master to prevent merge-conflicts. |
wilbaker
left a comment
wilbaker
left a comment
There was a problem hiding this comment.
Questions and comments (mostly minor) after taking a first pass over the changes.
…nd server certificate verification
…e loading. Extracting interaction with GitSsl from HttpRequestor to GitAuthentication
…from file and from store. Adding filtering and ordering, when loading from store
…ing StyleCop issues
…have non-bool values. Handling InvalidRepoException in CloneVerb.
wilbaker
left a comment
wilbaker
left a comment
There was a problem hiding this comment.
Apologies for the delay in getting to this PR. I haven't looked at the testing changes yet, but I did notice that the latest code does not compile on Windows, see comments for details.
wilbaker
left a comment
wilbaker
left a comment
There was a problem hiding this comment.
Just added some comments on the latest changes, most of them are minor style comments.
|
|
||
| using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) | ||
| { | ||
| Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( |
There was a problem hiding this comment.
@jrbriggs @turbonaitis, what is the status of this discussion?
wilbaker
left a comment
wilbaker
left a comment
There was a problem hiding this comment.
Looks good, @turbonaitis thanks for all the hard work putting this together!
|
@jrbriggs we might want to cut a release branch from master before merging this PR in. |
kewillford
left a comment
kewillford
left a comment
There was a problem hiding this comment.
Looks good. Just a couple naming things.
|
Thanks for the contribution, @turbonaitis ! |
7 participants
This PR addresses #487.
It covers the scenarios, when sslCert configured in .gitconfig points either to a certificate with private key in a keychain, or to a file, containing both public and private keys.
One thing missing is sslKey support - if our .gitconfig specifies sslCert, containing just the public part, and sslKey, containing the private key, the current PR doesn't load it. Main reason being, is that dotnet currently does not support easy handling of PEM files. I'll create a separate PR addressing that, as I believe it will lead to much more discussins.