Add basic framework for Git Credential Manager #1
Conversation
Add basic framework for Git Credential Manager:
- .NET Core CLI application
- common/shared library with
- minimal credential-helper command stubs (get, store, erase)
- help and version commands
- Git-style tracing
- abstracted file system and stdin/out/err for testing
- unit tests
- Use 'Secret' rather than 'Sensitive' in tracing terminology to be consistent with the environment variable name. - Always print `WriteLineSecrets` but allow the caller to pass in the secret parts as format arguments, which will now be masked if `EnableSecretTracing` is false. The rationale for this is that trace output will be more consistent, just with the secret parts masked. - git-credential treats the keys as case-sensitive; so should we. - Use `EnsureArgument` helpers in Trace.cs - Print usage information on incorrect usage.
tests/Microsoft.Git.CredentialManager.Tests/Commands/HostProviderCommandBaseTests.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public IHostProvider GetProvider(InputArguments input) | ||
| { | ||
| var provider = _hostProviders.FirstOrDefault(x => x.IsSupported(input)); |
There was a problem hiding this comment.
How will this work if multiple providers match? Will we fall back to basic?
There was a problem hiding this comment.
If multiple providers match, it will match the first one registered. Unlike the existing GCM we don't 'fall back' to the basic provider if another provider matched and failed. If a credential helper fails or returns bad credentials Git will do a basic auth prompt itself.
The model in this new GCM for selecting providers is to query each provider directly asking "do you understand the input I got from Git?", whereas in the old GCM there were two ways of selecting a provider, "Auto" and explicitly set in a configuration option. "Auto" would just run through all 'providers' actually trying to authenticate until one succeeds.
I realise that there is a potential hole with this: on-prem Azure DevOps Servers instances; they support either NTLM* or basic authentication. For basic authentication we have two options:
- continue to let 'basic' auth fall out of GCM and we tell Git we don't understand, and we let Git do it's TTY-based prompt,
- we implement a basic auth provider (I had one in the prototype which I've yet to port) which is registered last (like in the prototype) and can do a simple username/password prompt, possibly using a OS-native credential prompt window (like the old GCM does) rather than from the TTY like Git does.
*regarding NTLM, as you know the old GCM doesn't actually do anything for NTLM, it just returns an empty username and password back to Git over standard out. We could also implement such a authentication helper, but for now I'd like to get the basics (no pun intended 😛) working and in first before we try and cover every possible case.
There was a problem hiding this comment.
I should add that there is also a unit test included in this PR which covers the behaviour when multiple providers match the input 😄
There was a problem hiding this comment.
So I think the way the basic helper in current GCM is supposed to work is, that it saves whatever you type in the boxes.
I.E. I fall back to basic, enter a PAT, it gets saved and I don't get auth'ed again.
I have seen some holes in this logic but that is the way I believe it supposed to work. If we fall back to gits auth we don't have that opportunity right?
There was a problem hiding this comment.
Ah yes that's true, we're the ones expected to save them with a call to store.
Well then we probably do need a basic provider then, even if only for saving creds.. the prototype does have a basic provider which accepts all HTTP(S) inputs from Git but I've left it out so far (along with all the other providers) because I wanted to bring it back once I'd got a native OS credential popup window implemented for all platforms. I have a few more branches/PRs ready to go once this one is in which adds the providers one-by-one, so keep a look out for the one which adds basic! 😄
| } | ||
| } | ||
|
|
||
| private static string FormatText(string message, string filePath, int lineNumber, string memberName) |
There was a problem hiding this comment.
Worth adding using tests for this function? I'm also a bit confused on the formatting we're trying to achieve.
There was a problem hiding this comment.
We're trying to achieve the same formatting as Git's when GIT_TRACE is enabled. I guess this is possible to unit test, but I'd put that low on the list of things to-do at the moment, especially given the format of the tracing output is more a 'nice to have' rather than critical to the execution of the credential helper.
Plus this code is battle tested, given it came almost verbatim from the existing GCM, so I'm sure it's doing the right thing for now 😄
|
I really like the simplicity and unit testability of the new design! 👍 Just food for thought (above and beyond what we have today). Adding functional tests would give me much more confidence in PRs. Perhaps we should think about doing that for for some of the providers. Again this is a north star and above and beyond what we hve today. |
Absolutely @jeschu1! I would be looking to add these wider types of tests in due course 😁 |
| IDictionary variables = Environment.GetEnvironmentVariables(); | ||
|
|
||
| // On Windows it is technically possible to get env vars which differ only by case | ||
| // even though the assumption is that they are case insensitive on Windows. |
There was a problem hiding this comment.
the assumption is that they are case insensitive on Windows -
I wasn't sure if this was our assumption or a general Windows assumption. Do you happen to have any links to documentation that describe this behavior?
There was a problem hiding this comment.
I believe it's a general assumption in Windows. You can't use set/setx in cmd or $env: in PowerShell to get/set case-sensitive environment variables for example.
I've updated this comment to include a specific example where the .NET Process class itself makes an incorrect assumption and presented an actual bug in the current GCM that we needed to fix 😢
|
|
||
| foreach (var key in variables.Keys) | ||
| { | ||
| if (key is string name && variables[key] is string value) |
There was a problem hiding this comment.
Do you actually expect to get key / values that are not strings? or are you being defensive here?
There was a problem hiding this comment.
No I do not, and yes this is partly for defensive purposes 😀
Also since the type you get back from Environment.GetEnvironmentVariables() is the non-generic IDictionary and I want to offer the friendlier and more useful (IMO) generic IDictionary<string, string>, this syntax offers a nice way to both safely cast the object/object key-value pair to strings that are also non-null (a null value should be 'not set'/'not present').
|
|
||
| public override bool CanExecute(string[] args) | ||
| { | ||
| return args.Length != 0 && StringComparer.OrdinalIgnoreCase.Equals(args[0], Name); |
There was a problem hiding this comment.
When would you expect to have args[0] not equal to Name?
There was a problem hiding this comment.
When we're invoked with any command which doesn't match git-credential-manager <verb>?
The CanExecute method of every command could be called for any and all input (except args.Length == 0; that is just defensive), testing if the command feels it should execute given the command-line arguments.
The VerbCommandBase commands all follow the same pattern of gcm <verb> with no extra switches/flags, and are selected by their name as the first argument.
|
|
||
| protected override Task ExecuteInternalAsync(ICommandContext context, InputArguments input, IHostProvider provider, string credentialKey) | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Not implemented because this is a base class? or because it hasn't been implemented yet?
There was a problem hiding this comment.
Not implemented because it's not implemented (in this PR) yet. I'm intentionally holding back some of the code of the core credential helper commands and host providers to keep this PR more manageable; they're coming in subsequent PRs.
I expect this initial PR to be the largest.
| { | ||
| public static void Main(string[] args) | ||
| { | ||
| int exitCode = Application.RunAsync(args).ConfigureAwait(false).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
How come we are running async here? Could it be run synchronously?
There was a problem hiding this comment.
(Most) Providers are ultimately going be making network calls, or showing UI and should ideally be using *Async methods in their implementation (best practice). I've set up the IHostProvider interface to encourage and support async/await and just plumbed the awaitable Task all the way through the rest of GCM as I didn't see a reason not to(!)
I initially wanted to make the entry async Task Main too, but depending on the .NET Core or Framework version we're targeting I wasn't sure if this would be supported yet (looking at it now I think that we can; but I'll skip that for now).
|
How do you feel about adding some general comments around some of the interfaces? On my 1st readthrough, I thought it might have helped a bit if there were some high level / general comments around hat some of the interfaces represent ( |
@jamill, I've added doc-comments to all the public and abstract types/methods which should hopefully go a long way to documenting the design. I was thinking about adding a Markdown document (in a future PR) giving an overview of how all the pieces hang together, specifically around host providers and (out-of-proc) authentication helpers for future developers and provider implementors. Sound like a good idea? |
install from source: custom install location edits
Add basic framework for Git Credential Manager: