Revamp credential storage system to allow for smarter searching #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mjcheetham
added
bug
There was a problem hiding this comment.
Finished my first pass. Just a few random thoughts here and there.
Question: What is the user experience for someone using a dev.azure.com URL with current GCM Core and then they upgrade? Do they now need to re-auth for every organization? Or will the existing org@dev.azure.com credentials still work? It is harder to be confident in such compatibility scenarios, as the tests only check the current version of the code.
src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs
Outdated
Show resolved
Hide resolved
src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs
Outdated
Show resolved
Hide resolved
Current GCM Core does the same thing.. for |
derrickstolee
approved these changes
Sep 15, 2020
mjcheetham
force-pushed
the
newcred2
branch
3 times, most recently
from
245c05e
to
62dff0a
Compare
Update the ICredentialStore and ICredential API to allow for smarter searching of stored credentials. The new API exposes filtering by "service name" and "account" separately. Service name will typically be the URL the credential is stored against, and the account will be the username associated with the credential.
Update the macOS Keychain component implementation to match the new ICredentialStore interface. We now use the `SecItemCopyMatching` to perform a general query for items, and return a specialised credential object including all relevant attributes.
Update the Windows Credential Manager component to implement the new ICredentialStore interface, including credential enumeration and matching by account/user as well as 'service name' (target).
Recover the correct service name from the target name. Since the target name may contain a userinfo component (for example https://alice_domain.com@example.com/path), and the only place we store the service name is in the target name, we need to strip out any userinfo component. We do this by looking for the "://" and the first '@' character before the first '/', which act as the start and end of the userinfo component.
Update the SecretServiceCollection credential store (backed by libsecret) to match the new ICredentialStore interface and access model.
Update the remote URI generation from the program InputArguments to support port numbers, and special characters in usernames.
Update the HostProvider base class to use the new 'service name' abstraction rather than the simple 'unique credential key' one. With this model we can better issue credential storage queries where the username may not be specified explicitly in a get request (often the case as the username is not always included in the remote URL for many services).
Update the generic host provider to support the new credential storage model and HostProvider base class APIs.
Update the Basic authentication component to match the new ICredential interface.
Update the BitBucket provider to support and implement the new credential storage/recall model and APIs.
Update the GitHub provider to implement and follow the new credential storage/recall/matching model and ICredentialStore APIs.
Update the Azure Repos provider to support the new credential storage API/model, as well as support remote URLs with explicit port numbers.
Now that we support multiple user accounts for each host/service/remote, we have hit an interesting issue with Azure Repos. With the introduction of the dev.azure.com-style URLs for Azure Repos there was an unfortunate hack or workaround invented to add the AzDevOps org name to the userinfo part of the remote URL, for example: org@dev.azure.com/org/blah. Since GCM for Windows (and older versions of GCM Core that initially followed the same model) always uses the value "PersonalAccessToken" for the username field when storing credentials it was free to ignore the user part of the input. The problem now is that since we support multiple user accounts, and will perform an exact match against the credential (with user) if a username is specified in the remote URL, we never find the credential we now store (we now always store with the actual, real users' UPN). To workaround this workaround (yuck) we ignore the username IF AND ONLY IF the host is dev.azure.com, and return the first matching dev.azure.com/org credential. The upshot of this is that dev.azure.com-style URLs do NOT support multiple users OR full paths, however vs.com-style URLs will.
Add support for customising the namespace/prefix used to store
credentials in the OS credential store.
By default we use "git:{service}". Users can use GCM_NAMESPACE or
credential.namespace to set this to something different.
These configuration options are the same as in GCM for Windows to help
with migration.
Update the GCM Core Host Provider spec document and architecture document to reflect the changes made to the abstract HostProvider class; replacing GetCredentialKey with GetServiceName.
Fix a bug in the WindowsEnvironment implementation of the LocateExectuable method. On .NET Core the UseShellExecute property of ProcessStartInfo defaults to false, whereas on .NET Framework (the runtime that we target on Windows) defaults to true. You must set this to false if you want to redirect standard streams (which we want to do). The .NET Framework-targeting build on Windows was throwing an exception here(!)
mjcheetham
force-pushed
the
newcred2
branch
from
62dff0a
to
fd6df7c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR should hopefully address several bugs which originate from the (flawed) design of the credential storage abstraction ( #142, #160) as well as a simple port number parsing bug (#156)
The original design/assumption was that for each request we get from Git (via a
getcall) we could create a unique 'credential key' that encoded the username and URL for the hosting provider. Whilst this worked for GitHub, Azure Repos, and Bitbucket when using one user per provider (or AzDevOps organisation), it failed to work for the generic provider when the username was not specified as part of the remote URL, nor did it work for multi-user scenarios with the other providers.Following the design of the
osxkeychainandwincredGit credential helpers, we now store the username in a separate field from the URL (called a 'service' to use the macOS Keychain terminology). This means when doing a credential lookup in response to agetcall, we can search for a credential with the matching URL (service), and optionally a matching account if a username was passed, or any user if none was specified.One problem however arises from this multi-user support which is specific to the Azure Repos provider..
Background: Azure Repos supports two kinds of URL:
org.visualstudio.comanddev.azure.com/org. In the former we have all the information we need to perform authentication in the authority (protocol + hostname + orgname), but with the latter we require at least the first component of the path to get the organisation name.Git only supplies the authority information (protocol + hostname) by default, meaning we have to force the
useHttpPathGit configuration option totruemeaning we always get the path component. This also meant that for Azure Repos users using the dev.azure.com-style URL we could not differentiate between a real desire to store credentials against the full remote URL including path or not. This was already an existing limitation.However, to help 'workaround' this URL+Git design incompatibility, Azure Repos also made the decision to recommend users put the AzDevOps organisation name as the user info in the remote URL for dev.azure.com URLs. For example:
https://org@dev.azure.com/org/project/_git/repo. Previously we ignored the username (since we only supported one user), but now we have a problem with storing credentials against the real username vs the 'fake' org username value.Without a change to Git to either: a) always provide the full remote URL on each
getrequest as a separate input line, or b) provide a way to maintain state between thegetandstore/erasecalls, we cannot implement multi-user nor full-path support fordev.azure.comusers. For now, we maintain the existing behaviour of one one user and no full path storage support for credentials against thedev.azure.com-style URLs.We do however implement full mutli-user and full-path-storage support for the older
*.visualstudio.com-style URLs.For those who wish to use multiple users per Azure DevOps organisation, or store a credential against the full remote URL, they must use the
vs.com-style URLs which are still supported.