From 1425412b3f354f34e0e6cc2f1ce643e6b0056e0c Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 13:59:01 +0100 Subject: [PATCH 01/16] credstore: update credstore API to allow better search 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. --- .../Commands/GetCommand.cs | 2 +- .../Credential.cs | 10 +- .../ICredentialStore.cs | 23 ++-- .../Objects/TestCredentialStore.cs | 123 +++++++++--------- 4 files changed, 77 insertions(+), 81 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager/Commands/GetCommand.cs b/src/shared/Microsoft.Git.CredentialManager/Commands/GetCommand.cs index bfebaa03d..94b236cd4 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Commands/GetCommand.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Commands/GetCommand.cs @@ -36,7 +36,7 @@ protected override async Task ExecuteInternalAsync(ICommandContext context, Inpu } // Return the credential to Git - output["username"] = credential.UserName; + output["username"] = credential.Account; output["password"] = credential.Password; // Write the values to standard out diff --git a/src/shared/Microsoft.Git.CredentialManager/Credential.cs b/src/shared/Microsoft.Git.CredentialManager/Credential.cs index 0c59f66b9..d311a6ba0 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Credential.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Credential.cs @@ -4,14 +4,14 @@ namespace Microsoft.Git.CredentialManager { /// - /// Represents a simple credential; user name and password pair. + /// Represents a credential. /// public interface ICredential { /// - /// User name. + /// Account associated with this credential. /// - string UserName { get; } + string Account { get; } /// /// Password. @@ -26,11 +26,11 @@ public class GitCredential : ICredential { public GitCredential(string userName, string password) { - UserName = userName; + Account = userName; Password = password; } - public string UserName { get; } + public string Account { get; } public string Password { get; } } diff --git a/src/shared/Microsoft.Git.CredentialManager/ICredentialStore.cs b/src/shared/Microsoft.Git.CredentialManager/ICredentialStore.cs index c458a3836..d1459a234 100644 --- a/src/shared/Microsoft.Git.CredentialManager/ICredentialStore.cs +++ b/src/shared/Microsoft.Git.CredentialManager/ICredentialStore.cs @@ -9,24 +9,27 @@ namespace Microsoft.Git.CredentialManager public interface ICredentialStore { /// - /// Get credential from the store with the specified key. + /// Get the first credential from the store that matches the given query. /// - /// Key for credential to retrieve. - /// Stored credential or null if not found. - ICredential Get(string key); + /// Name of the service to match against. Use null to match all values. + /// Account name to match against. Use null to match all values. + /// First matching credential or null if none are found. + ICredential Get(string service, string account); /// /// Add or update credential in the store with the specified key. /// - /// Key for credential to add/update. - /// Credential to store. - void AddOrUpdate(string key, ICredential credential); + /// Name of the service this credential is for. Use null to match all values. + /// Account associated with this credential. Use null to match all values. + /// Secret value to store. + void AddOrUpdate(string service, string account, string secret); /// - /// Delete credential from the store with the specified key. + /// Delete credential from the store that matches the given query. /// - /// Key of credential to delete. + /// Name of the service to match against. Use null to match all values. + /// Account name to match against. Use null to match all values. /// True if the credential was deleted, false otherwise. - bool Remove(string key); + bool Remove(string service, string account); } } diff --git a/src/shared/TestInfrastructure/Objects/TestCredentialStore.cs b/src/shared/TestInfrastructure/Objects/TestCredentialStore.cs index bbd08c4b7..ea0afb3b7 100644 --- a/src/shared/TestInfrastructure/Objects/TestCredentialStore.cs +++ b/src/shared/TestInfrastructure/Objects/TestCredentialStore.cs @@ -1,115 +1,108 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. -using System; -using System.Collections; using System.Collections.Generic; +using System.Linq; namespace Microsoft.Git.CredentialManager.Tests.Objects { - public class TestCredentialStore : ICredentialStore, IDictionary + public class TestCredentialStore : ICredentialStore { - private readonly IDictionary _store; + private readonly IDictionary<(string service, string account), TestCredential> _store; public TestCredentialStore() { - _store = new Dictionary(StringComparer.Ordinal); + _store = new Dictionary<(string,string), TestCredential>(); } #region ICredentialStore - ICredential ICredentialStore.Get(string key) + ICredential ICredentialStore.Get(string service, string account) { - if (_store.TryGetValue(key, out var credential)) - { - return credential; - } - - return null; - } - - void ICredentialStore.AddOrUpdate(string key, ICredential credential) - { - _store[key] = credential; - } - - public void Add(string key, ICredential value) - { - _store.Add(key, value); - } - - public bool ContainsKey(string key) - { - return _store.ContainsKey(key); + return TryGet(service, account, out TestCredential credential) ? credential : null; } - public bool Remove(string key) + void ICredentialStore.AddOrUpdate(string service, string account, string secret) { - return _store.Remove(key); + Add(service, account, secret); } - public bool TryGetValue(string key, out ICredential value) + bool ICredentialStore.Remove(string service, string account) { - return _store.TryGetValue(key, out value); - } - - public ICredential this[string key] - { - get => _store[key]; - set => _store[key] = value; - } - - public ICollection Keys => _store.Keys; - - public ICollection Values => _store.Values; + foreach (var key in _store.Keys) + { + if ((service == null || key.service == service) && + (account == null || key.account == account)) + { + _store.Remove(key); + return true; + } + } - bool ICredentialStore.Remove(string key) - { - return _store.Remove(key); + return false; } #endregion - #region IDictionary + public int Count => _store.Count; - public IEnumerator> GetEnumerator() + public bool TryGet(string service, string account, out TestCredential credential) { - return _store.GetEnumerator(); + credential = Query(service, account).FirstOrDefault(); + return credential != null; } - IEnumerator IEnumerable.GetEnumerator() + public void Add(string service, TestCredential credential) { - return ((IEnumerable) _store).GetEnumerator(); + _store[(service, credential.Account)] = credential; } - public void Add(KeyValuePair item) + public TestCredential Add(string service, string account, string secret) { - _store.Add(item); + var credential = new TestCredential(service, account, secret); + _store[(service, account)] = credential; + return credential; } - public void Clear() + public bool Contains(string service, string account) { - _store.Clear(); + return TryGet(service, account, out _); } - public bool Contains(KeyValuePair item) + private IEnumerable Query(string service, string account) { - return _store.Contains(item); - } + if (string.IsNullOrWhiteSpace(account)) + { + // Find the all credentials matching service + foreach (var kvp in _store) + { + if (kvp.Key.service == service) + { + yield return kvp.Value; + } + } + } - public void CopyTo(KeyValuePair[] array, int arrayIndex) - { - _store.CopyTo(array, arrayIndex); + // Find the specific credential matching both service and credential + if (_store.TryGetValue((service, account), out var credential)) + { + yield return credential; + } } + } - public bool Remove(KeyValuePair item) + public class TestCredential : ICredential + { + public TestCredential(string service, string account, string password) { - return _store.Remove(item); + Service = service; + Account = account; + Password = password; } - public int Count => _store.Count; + public string Service { get; } - public bool IsReadOnly => _store.IsReadOnly; + public string Account { get; } - #endregion + public string Password { get; } } } From 469a453824ca9fd52a69b32689588a7e63e7908c Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 14:02:25 +0100 Subject: [PATCH 02/16] macoskeychain: update macOS Keychain to match new interface 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. --- .../Interop/MacOS/MacOSKeychainTests.cs | 41 +-- .../CommandContext.cs | 2 +- .../Constants.cs | 1 + .../Interop/MacOS/MacOSKeychain.cs | 255 +++++++++++------- .../Interop/MacOS/MacOSKeychainCredential.cs | 28 ++ .../Interop/MacOS/Native/CoreFoundation.cs | 83 ++++++ .../Interop/MacOS/Native/LibSystem.cs | 24 ++ .../Interop/MacOS/Native/SecurityFramework.cs | 66 ++++- 8 files changed, 375 insertions(+), 125 deletions(-) create mode 100644 src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychainCredential.cs create mode 100644 src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/LibSystem.cs diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/MacOS/MacOSKeychainTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/MacOS/MacOSKeychainTests.cs index 2a253b177..1d6b5dc7e 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/MacOS/MacOSKeychainTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/MacOS/MacOSKeychainTests.cs @@ -8,57 +8,58 @@ namespace Microsoft.Git.CredentialManager.Tests.Interop.MacOS { public class MacOSKeychainTests { + private const string TestNamespace = "git-test"; + [PlatformFact(Platform.MacOS)] public void MacOSKeychain_ReadWriteDelete() { - MacOSKeychain keychain = MacOSKeychain.Open(); + MacOSKeychain keychain = MacOSKeychain.Open(TestNamespace); - // Create a key that is guarenteed to be unique - string key = $"secretkey-{Guid.NewGuid():N}"; - const string userName = "john.doe"; + // Create a service that is guaranteed to be unique + string service = $"https://example.com/{Guid.NewGuid():N}"; + const string account = "john.doe"; const string password = "letmein123"; - var credential = new GitCredential(userName, password); try { // Write - keychain.AddOrUpdate(key, credential); + keychain.AddOrUpdate(service, account, password); // Read - ICredential outCredential = keychain.Get(key); + ICredential outCredential = keychain.Get(service, account); Assert.NotNull(outCredential); - Assert.Equal(credential.UserName, outCredential.UserName); - Assert.Equal(credential.Password, outCredential.Password); + Assert.Equal(account, outCredential.Account); + Assert.Equal(password, outCredential.Password); } finally { // Ensure we clean up after ourselves even in case of 'get' failures - keychain.Remove(key); + keychain.Remove(service, account); } } [PlatformFact(Platform.MacOS)] - public void MacOSKeychain_Get_KeyNotFound_ReturnsNull() + public void MacOSKeychain_Get_NotFound_ReturnsNull() { - MacOSKeychain keychain = MacOSKeychain.Open(); + MacOSKeychain keychain = MacOSKeychain.Open(TestNamespace); - // Unique key; guaranteed not to exist! - string key = Guid.NewGuid().ToString("N"); + // Unique service; guaranteed not to exist! + string service = $"https://example.com/{Guid.NewGuid():N}"; - ICredential credential = keychain.Get(key); + ICredential credential = keychain.Get(service, account: null); Assert.Null(credential); } [PlatformFact(Platform.MacOS)] - public void MacOSKeychain_Remove_KeyNotFound_ReturnsFalse() + public void MacOSKeychain_Remove_NotFound_ReturnsFalse() { - MacOSKeychain keychain = MacOSKeychain.Open(); + MacOSKeychain keychain = MacOSKeychain.Open(TestNamespace); - // Unique key; guaranteed not to exist! - string key = Guid.NewGuid().ToString("N"); + // Unique service; guaranteed not to exist! + string service = $"https://example.com/{Guid.NewGuid():N}"; - bool result = keychain.Remove(key); + bool result = keychain.Remove(service, account: null); Assert.False(result); } } diff --git a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs index 26e7f08c9..7914d2b94 100644 --- a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs +++ b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs @@ -107,7 +107,7 @@ public CommandContext() FileSystem.GetCurrentDirectory() ); Settings = new Settings(Environment, Git); - CredentialStore = MacOSKeychain.Open(); + CredentialStore = MacOSKeychain.Open(Constants.CredentialNamespace); } else if (PlatformUtils.IsLinux()) { diff --git a/src/shared/Microsoft.Git.CredentialManager/Constants.cs b/src/shared/Microsoft.Git.CredentialManager/Constants.cs index ba9ec41f0..e4ba7199f 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Constants.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Constants.cs @@ -10,6 +10,7 @@ public static class Constants public const string PersonalAccessTokenUserName = "PersonalAccessToken"; public const string OAuthTokenUserName = "OAuthToken"; public const string DefaultMsAuthHelper = "Microsoft.Authentication.Helper"; + public const string CredentialNamespace = "git"; public const string ProviderIdAuto = "auto"; public const string AuthorityIdAuto = "auto"; diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychain.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychain.cs index daebe77db..b4be185d8 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychain.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychain.cs @@ -5,108 +5,137 @@ using System.Runtime.InteropServices; using System.Text; using Microsoft.Git.CredentialManager.Interop.MacOS.Native; +using static Microsoft.Git.CredentialManager.Interop.MacOS.Native.CoreFoundation; using static Microsoft.Git.CredentialManager.Interop.MacOS.Native.SecurityFramework; namespace Microsoft.Git.CredentialManager.Interop.MacOS { public class MacOSKeychain : ICredentialStore { + private readonly string _namespace; + #region Constructors /// /// Open the default keychain (current user's login keychain). /// + /// Optional namespace to scope credential operations. /// Default keychain. - public static MacOSKeychain Open() + public static MacOSKeychain Open(string @namespace = null) { - return new MacOSKeychain(); + return new MacOSKeychain(@namespace); } - private MacOSKeychain() + private MacOSKeychain(string @namespace) { PlatformUtils.EnsureMacOS(); + _namespace = @namespace; } #endregion #region ICredentialStore - public ICredential Get(string key) + public ICredential Get(string service, string account) { - IntPtr passwordData = IntPtr.Zero; - IntPtr itemRef = IntPtr.Zero; + IntPtr query = IntPtr.Zero; + IntPtr resultPtr = IntPtr.Zero; + IntPtr servicePtr = IntPtr.Zero; + IntPtr accountPtr = IntPtr.Zero; try { - // Find the item (itemRef) and password (passwordData) in the keychain - int findResult = SecKeychainFindGenericPassword( - IntPtr.Zero, (uint) key.Length, key, 0, null, - out uint passwordLength, out passwordData, out itemRef); + query = CFDictionaryCreateMutable( + IntPtr.Zero, + 0, + IntPtr.Zero, IntPtr.Zero); - switch (findResult) + CFDictionaryAddValue(query, kSecClass, kSecClassGenericPassword); + CFDictionaryAddValue(query, kSecMatchLimit, kSecMatchLimitOne); + CFDictionaryAddValue(query, kSecReturnData, kCFBooleanTrue); + CFDictionaryAddValue(query, kSecReturnAttributes, kCFBooleanTrue); + + if (!string.IsNullOrWhiteSpace(service)) { - case OK: - // Get and decode the user name from the 'account name' attribute - byte[] userNameBytes = GetAccountNameAttributeData(itemRef); - string userName = Encoding.UTF8.GetString(userNameBytes); + string fullService = CreateServiceName(service); + servicePtr = CreateCFStringUtf8(fullService); + CFDictionaryAddValue(query, kSecAttrService, servicePtr); + } - // Decode the password from the raw data - byte[] passwordBytes = InteropUtils.ToByteArray(passwordData, passwordLength); - string password = Encoding.UTF8.GetString(passwordBytes); + if (!string.IsNullOrWhiteSpace(account)) + { + accountPtr = CreateCFStringUtf8(account); + CFDictionaryAddValue(query, kSecAttrAccount, accountPtr); + } - return new GitCredential(userName, password); + int searchResult = SecItemCopyMatching(query, out resultPtr); + + switch (searchResult) + { + case OK: + int typeId = CFGetTypeID(resultPtr); + Debug.Assert(typeId != CFArrayGetTypeID(), "Returned more than one keychain item in search"); + if (typeId == CFDictionaryGetTypeID()) + { + return CreateCredentialFromAttributes(resultPtr); + } + + throw new InteropException($"Unknown keychain search result type CFTypeID: {typeId}.", -1); case ErrorSecItemNotFound: return null; default: - ThrowIfError(findResult); + ThrowIfError(searchResult); return null; } } finally { - if (passwordData != IntPtr.Zero) - { - SecKeychainItemFreeContent(IntPtr.Zero, passwordData); - } - - if (itemRef != IntPtr.Zero) - { - CoreFoundation.CFRelease(itemRef); - } + if (query != IntPtr.Zero) CFRelease(query); + if (servicePtr != IntPtr.Zero) CFRelease(servicePtr); + if (accountPtr != IntPtr.Zero) CFRelease(accountPtr); + if (resultPtr != IntPtr.Zero) CFRelease(resultPtr); } } - public void AddOrUpdate(string key, ICredential credential) + public void AddOrUpdate(string service, string account, string secret) { - byte[] passwordBytes = Encoding.UTF8.GetBytes(credential.Password); + EnsureArgument.NotNullOrWhiteSpace(service, nameof(service)); + + byte[] secretBytes = Encoding.UTF8.GetBytes(secret); IntPtr passwordData = IntPtr.Zero; IntPtr itemRef = IntPtr.Zero; + string serviceName = CreateServiceName(service); + + + uint serviceNameLength = (uint) serviceName.Length; + uint accountLength = (uint) (account?.Length ?? 0); + try { // Check if an entry already exists in the keychain int findResult = SecKeychainFindGenericPassword( - IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, credential.UserName, + IntPtr.Zero, serviceNameLength, serviceName, accountLength, account, out uint _, out passwordData, out itemRef); switch (findResult) { - // Create new entry + // Update existing entry case OK: ThrowIfError( - SecKeychainItemModifyAttributesAndData(itemRef, IntPtr.Zero, (uint) passwordBytes.Length, passwordBytes), + SecKeychainItemModifyAttributesAndData(itemRef, IntPtr.Zero, (uint) secretBytes.Length, secretBytes), "Could not update existing item" ); break; - // Update existing entry + // Create new entry case ErrorSecItemNotFound: ThrowIfError( - SecKeychainAddGenericPassword(IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, - credential.UserName, (uint) passwordBytes.Length, passwordBytes, out itemRef), + SecKeychainAddGenericPassword(IntPtr.Zero, serviceNameLength, serviceName, accountLength, + account, (uint) secretBytes.Length, secretBytes, out itemRef), "Could not create new item" ); break; @@ -125,27 +154,50 @@ public void AddOrUpdate(string key, ICredential credential) if (itemRef != IntPtr.Zero) { - CoreFoundation.CFRelease(itemRef); + CFRelease(itemRef); } } } - public bool Remove(string key) + public bool Remove(string service, string account) { - IntPtr passwordData = IntPtr.Zero; - IntPtr itemRef = IntPtr.Zero; + IntPtr query = IntPtr.Zero; + IntPtr itemRefPtr = IntPtr.Zero; + IntPtr servicePtr = IntPtr.Zero; + IntPtr accountPtr = IntPtr.Zero; try { - int findResult = SecKeychainFindGenericPassword( - IntPtr.Zero, (uint) key.Length, key, 0, null, - out _, out passwordData, out itemRef); + query = CFDictionaryCreateMutable( + IntPtr.Zero, + 0, + IntPtr.Zero, IntPtr.Zero); - switch (findResult) + CFDictionaryAddValue(query, kSecClass, kSecClassGenericPassword); + CFDictionaryAddValue(query, kSecMatchLimit, kSecMatchLimitOne); + CFDictionaryAddValue(query, kSecReturnRef, kCFBooleanTrue); + + if (!string.IsNullOrWhiteSpace(service)) + { + string fullService = CreateServiceName(service); + servicePtr = CreateCFStringUtf8(fullService); + CFDictionaryAddValue(query, kSecAttrService, servicePtr); + } + + if (!string.IsNullOrWhiteSpace(account)) + { + accountPtr = CreateCFStringUtf8(account); + CFDictionaryAddValue(query, kSecAttrAccount, accountPtr); + } + + // Search for the credential to delete and get the SecKeychainItem ref. + int searchResult = SecItemCopyMatching(query, out itemRefPtr); + switch (searchResult) { case OK: + // Delete the item ThrowIfError( - SecKeychainItemDelete(itemRef) + SecKeychainItemDelete(itemRefPtr) ); return true; @@ -153,82 +205,89 @@ public bool Remove(string key) return false; default: - ThrowIfError(findResult); + ThrowIfError(searchResult); return false; } } finally { - if (passwordData != IntPtr.Zero) - { - SecKeychainItemFreeContent(IntPtr.Zero, passwordData); - } - - if (itemRef != IntPtr.Zero) - { - CoreFoundation.CFRelease(itemRef); - } + if (query != IntPtr.Zero) CFRelease(query); + if (itemRefPtr != IntPtr.Zero) CFRelease(itemRefPtr); + if (servicePtr != IntPtr.Zero) CFRelease(servicePtr); + if (accountPtr != IntPtr.Zero) CFRelease(accountPtr); } } #endregion - #region Private Methods + private static IntPtr CreateCFStringUtf8(string str) + { + byte[] bytes = Encoding.UTF8.GetBytes(str); + return CFStringCreateWithBytes(IntPtr.Zero, + bytes, bytes.Length, CFStringEncoding.kCFStringEncodingUTF8, false); + } - private static byte[] GetAccountNameAttributeData(IntPtr itemRef) + private static ICredential CreateCredentialFromAttributes(IntPtr attributes) { - IntPtr tagArrayPtr = IntPtr.Zero; - IntPtr formatArrayPtr = IntPtr.Zero; - IntPtr attrListPtr = IntPtr.Zero; // SecKeychainAttributeList + string service = GetStringAttribute(attributes, kSecAttrService); + string account = GetStringAttribute(attributes, kSecAttrAccount); + string password = GetStringAttribute(attributes, kSecValueData); + string label = GetStringAttribute(attributes, kSecAttrLabel); + return new MacOSKeychainCredential(service, account, password, label); + } - try + private static string GetStringAttribute(IntPtr dict, IntPtr key) + { + if (dict == IntPtr.Zero) { - // Extract the user name by querying for the item's 'account' attribute - tagArrayPtr = Marshal.AllocHGlobal(sizeof(SecKeychainAttrType)); - Marshal.WriteInt32(tagArrayPtr, (int) SecKeychainAttrType.AccountItem); - - formatArrayPtr = Marshal.AllocHGlobal(sizeof(CssmDbAttributeFormat)); - Marshal.WriteInt32(formatArrayPtr, (int) CssmDbAttributeFormat.String); + return null; + } - var attributeInfo = new SecKeychainAttributeInfo + IntPtr buffer = IntPtr.Zero; + try + { + if (CFDictionaryGetValueIfPresent(dict, key, out IntPtr value) && value != IntPtr.Zero) { - Count = 1, - Tag = tagArrayPtr, - Format = formatArrayPtr, - }; - - ThrowIfError( - SecKeychainItemCopyAttributesAndData( - itemRef, ref attributeInfo, - IntPtr.Zero, out attrListPtr, out _, IntPtr.Zero) - ); - - SecKeychainAttributeList attrList = Marshal.PtrToStructure(attrListPtr); - Debug.Assert(attrList.Count == 1, "Only expecting a list structure containing one attribute to be returned"); - - SecKeychainAttribute attribute = Marshal.PtrToStructure(attrList.Attributes); - - return InteropUtils.ToByteArray(attribute.Data, attribute.Length); + if (CFGetTypeID(value) == CFStringGetTypeID()) + { + int stringLength = (int)CFStringGetLength(value); + int bufferSize = stringLength + 1; + buffer = Marshal.AllocHGlobal(bufferSize); + if (CFStringGetCString(value, buffer, bufferSize, CFStringEncoding.kCFStringEncodingUTF8)) + { + return Marshal.PtrToStringAuto(buffer, stringLength); + } + } + + if (CFGetTypeID(value) == CFDataGetTypeID()) + { + int length = CFDataGetLength(value); + IntPtr ptr = CFDataGetBytePtr(value); + return Marshal.PtrToStringAuto(ptr, length); + } + } } finally { - if (tagArrayPtr != IntPtr.Zero) + if (buffer != IntPtr.Zero) { - Marshal.FreeHGlobal(tagArrayPtr); + Marshal.FreeHGlobal(buffer); } + } - if (formatArrayPtr != IntPtr.Zero) - { - Marshal.FreeHGlobal(formatArrayPtr); - } + return null; + } - if (attrListPtr != IntPtr.Zero) - { - SecKeychainItemFreeAttributesAndData(attrListPtr, IntPtr.Zero); - } + private string CreateServiceName(string service) + { + var sb = new StringBuilder(); + if (!string.IsNullOrWhiteSpace(_namespace)) + { + sb.AppendFormat("{0}:", _namespace); } - } - #endregion + sb.Append(service); + return sb.ToString(); + } } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychainCredential.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychainCredential.cs new file mode 100644 index 000000000..2f8a828fb --- /dev/null +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/MacOSKeychainCredential.cs @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. +using System.Diagnostics; + +namespace Microsoft.Git.CredentialManager.Interop.MacOS +{ + [DebuggerDisplay("{DebuggerDisplay}")] + public class MacOSKeychainCredential : ICredential + { + internal MacOSKeychainCredential(string service, string account, string password, string label) + { + Service = service; + Account = account; + Password = password; + Label = label; + } + + public string Service { get; } + + public string Account { get; } + + public string Label { get; } + + public string Password { get; } + + private string DebuggerDisplay => $"{Label} [Service: {Service}, Account: {Account}]"; + } +} diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/CoreFoundation.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/CoreFoundation.cs index 93cb6e8c0..2c0a16f53 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/CoreFoundation.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/CoreFoundation.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. using System; using System.Runtime.InteropServices; +using static Microsoft.Git.CredentialManager.Interop.MacOS.Native.LibSystem; namespace Microsoft.Git.CredentialManager.Interop.MacOS.Native { @@ -9,7 +10,89 @@ public static class CoreFoundation { private const string CoreFoundationFrameworkLib = "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation"; + public static readonly IntPtr Handle; + public static readonly IntPtr kCFBooleanTrue; + public static readonly IntPtr kCFBooleanFalse; + + static CoreFoundation() + { + Handle = dlopen(CoreFoundationFrameworkLib, 0); + + kCFBooleanTrue = GetGlobal(Handle, "kCFBooleanTrue"); + kCFBooleanFalse = GetGlobal(Handle, "kCFBooleanFalse"); + } + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr CFArrayCreateMutable(IntPtr allocator, long capacity, IntPtr callbacks); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern void CFArrayInsertValueAtIndex(IntPtr theArray, long idx, IntPtr value); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern long CFArrayGetCount(IntPtr theArray); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr CFArrayGetValueAtIndex(IntPtr theArray, long idx); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr CFDictionaryCreateMutable( + IntPtr allocator, + long capacity, + IntPtr keyCallBacks, + IntPtr valueCallBacks); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern void CFDictionaryAddValue( + IntPtr theDict, + IntPtr key, + IntPtr value); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr CFDictionaryGetValue(IntPtr theDict, IntPtr key); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern bool CFDictionaryGetValueIfPresent(IntPtr theDict, IntPtr key, out IntPtr value); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr CFStringCreateWithBytes(IntPtr alloc, byte[] bytes, long numBytes, + CFStringEncoding encoding, bool isExternalRepresentation); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern long CFStringGetLength(IntPtr theString); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern bool CFStringGetCString(IntPtr theString, IntPtr buffer, long bufferSize, CFStringEncoding encoding); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern void CFRetain(IntPtr cf); + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] public static extern void CFRelease(IntPtr cf); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int CFGetTypeID(IntPtr cf); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int CFStringGetTypeID(); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int CFDataGetTypeID(); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int CFDictionaryGetTypeID(); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int CFArrayGetTypeID(); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr CFDataGetBytePtr(IntPtr theData); + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int CFDataGetLength(IntPtr theData); + } + + public enum CFStringEncoding + { + kCFStringEncodingUTF8 = 0x08000100, } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/LibSystem.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/LibSystem.cs new file mode 100644 index 000000000..e4fce903f --- /dev/null +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/LibSystem.cs @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. +using System; +using System.Runtime.InteropServices; + +namespace Microsoft.Git.CredentialManager.Interop.MacOS.Native +{ + public static class LibSystem + { + private const string LibSystemLib = "/System/Library/Frameworks/System.framework/System"; + + [DllImport(LibSystemLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr dlopen(string name, int flags); + + [DllImport(LibSystemLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern IntPtr dlsym(IntPtr handle, string symbol); + + public static IntPtr GetGlobal(IntPtr handle, string symbol) + { + IntPtr ptr = dlsym(handle, symbol); + return Marshal.PtrToStructure(ptr); + } + } +} diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/SecurityFramework.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/SecurityFramework.cs index adcf7697b..e9a7a5c52 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/SecurityFramework.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/MacOS/Native/SecurityFramework.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. using System; using System.Runtime.InteropServices; +using static Microsoft.Git.CredentialManager.Interop.MacOS.Native.LibSystem; namespace Microsoft.Git.CredentialManager.Interop.MacOS.Native { @@ -10,9 +11,52 @@ public static class SecurityFramework { private const string SecurityFrameworkLib = "/System/Library/Frameworks/Security.framework/Security"; + public static readonly IntPtr Handle; + public static readonly IntPtr kSecClass; + public static readonly IntPtr kSecMatchLimit; + public static readonly IntPtr kSecReturnAttributes; + public static readonly IntPtr kSecReturnRef; + public static readonly IntPtr kSecReturnPersistentRef; + public static readonly IntPtr kSecClassGenericPassword; + public static readonly IntPtr kSecMatchLimitOne; + public static readonly IntPtr kSecMatchItemList; + public static readonly IntPtr kSecAttrLabel; + public static readonly IntPtr kSecAttrAccount; + public static readonly IntPtr kSecAttrService; + public static readonly IntPtr kSecValueRef; + public static readonly IntPtr kSecValueData; + public static readonly IntPtr kSecReturnData; + + static SecurityFramework() + { + Handle = dlopen(SecurityFrameworkLib, 0); + + kSecClass = GetGlobal(Handle, "kSecClass"); + kSecMatchLimit = GetGlobal(Handle, "kSecMatchLimit"); + kSecReturnAttributes = GetGlobal(Handle, "kSecReturnAttributes"); + kSecReturnRef = GetGlobal(Handle, "kSecReturnRef"); + kSecReturnPersistentRef = GetGlobal(Handle, "kSecReturnPersistentRef"); + kSecClassGenericPassword = GetGlobal(Handle, "kSecClassGenericPassword"); + kSecMatchLimitOne = GetGlobal(Handle, "kSecMatchLimitOne"); + kSecMatchItemList = GetGlobal(Handle, "kSecMatchItemList"); + kSecAttrLabel = GetGlobal(Handle, "kSecAttrLabel"); + kSecAttrAccount = GetGlobal(Handle, "kSecAttrAccount"); + kSecAttrService = GetGlobal(Handle, "kSecAttrService"); + kSecValueRef = GetGlobal(Handle, "kSecValueRef"); + kSecValueData = GetGlobal(Handle, "kSecValueData"); + kSecReturnData = GetGlobal(Handle, "kSecReturnData"); + } + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] public static extern int SessionGetInfo(int session, out int sessionId, out SessionAttributeBits attributes); + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecAccessCreate(IntPtr descriptor, IntPtr trustedList, out IntPtr accessRef); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemCreateFromContent(IntPtr itemClass, IntPtr attrList, uint length, + IntPtr data, IntPtr keychainRef, IntPtr initialAccess, out IntPtr itemRef); + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] public static extern int SecKeychainAddGenericPassword( IntPtr keychain, @@ -36,13 +80,13 @@ public static extern int SecKeychainFindGenericPassword( out IntPtr itemRef); [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] - public static extern int SecKeychainItemCopyAttributesAndData( + public static extern unsafe int SecKeychainItemCopyAttributesAndData( IntPtr itemRef, - ref SecKeychainAttributeInfo info, - IntPtr itemClass, // SecItemClass* - out IntPtr attrList, // SecKeychainAttributeList* - out uint dataLength, - IntPtr data); + IntPtr info, + IntPtr itemClass, + SecKeychainAttributeList** attrList, + uint* dataLength, + void** data); [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] public static extern int SecKeychainItemModifyAttributesAndData( @@ -65,6 +109,16 @@ public static extern int SecKeychainItemFreeAttributesAndData( IntPtr attrList, // SecKeychainAttributeList* IntPtr data); + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecItemCopyMatching(IntPtr query, out IntPtr result); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemCopyFromPersistentReference(IntPtr persistentItemRef, out IntPtr itemRef); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemCopyContent(IntPtr itemRef, IntPtr itemClass, IntPtr attrList, + out uint length, out IntPtr outData); + public const int CallerSecuritySession = -1; // https://developer.apple.com/documentation/security/1542001-security_framework_result_codes From bf9f51d48dbd6525e517636ff1f639c614bcd198 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 14:04:53 +0100 Subject: [PATCH 03/16] wincredmgr: update Windows CredManager to impl new interface 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). --- .../Windows/WindowsCredentialManagerTests.cs | 181 ++++++++++-- .../CommandContext.cs | 4 +- .../Interop/Windows/Native/Advapi32.cs | 19 +- .../Interop/Windows/WindowsCredential.cs | 26 ++ .../Windows/WindowsCredentialManager.cs | 272 ++++++++++++++---- 5 files changed, 429 insertions(+), 73 deletions(-) create mode 100644 src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredential.cs diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs index 0868a14ca..918ed8ea9 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs @@ -8,58 +8,203 @@ namespace Microsoft.Git.CredentialManager.Tests.Interop.Windows { public class WindowsCredentialManagerTests { + private const string TestNamespace = "git-test"; + [PlatformFact(Platform.Windows)] public void WindowsCredentialManager_ReadWriteDelete() { - WindowsCredentialManager credManager = WindowsCredentialManager.Open(); + WindowsCredentialManager credManager = WindowsCredentialManager.Open(TestNamespace); - // Create a key that is guarenteed to be unique - string key = $"secretkey-{Guid.NewGuid():N}"; + // Create a service that is guaranteed to be unique + string uniqueGuid = Guid.NewGuid().ToString("N"); + string service = $"https://example.com/{uniqueGuid}"; const string userName = "john.doe"; const string password = "letmein123"; - var credential = new GitCredential(userName, password); + + string expectedTargetName = $"{TestNamespace}:https://example.com/{uniqueGuid}"; try { // Write - credManager.AddOrUpdate(key, credential); + credManager.AddOrUpdate(service, userName, password); // Read - ICredential outCredential = credManager.Get(key); + ICredential cred = credManager.Get(service, userName); - Assert.NotNull(outCredential); - Assert.Equal(credential.UserName, outCredential.UserName); - Assert.Equal(credential.Password, outCredential.Password); + // Valdiate + var winCred = cred as WindowsCredential; + Assert.NotNull(winCred); + Assert.Equal(userName, winCred.UserName); + Assert.Equal(password, winCred.Password); + Assert.Equal(service, winCred.Service); + Assert.Equal(expectedTargetName, winCred.TargetName); } finally { // Ensure we clean up after ourselves even in case of 'get' failures - credManager.Remove(key); + credManager.Remove(service, userName); + } + } + + [PlatformFact(Platform.Windows)] + public void WindowsCredentialManager_AddOrUpdate_UsernameWithAtCharacter() + { + WindowsCredentialManager credManager = WindowsCredentialManager.Open(TestNamespace); + + // Create a service that is guaranteed to be unique + string uniqueGuid = Guid.NewGuid().ToString("N"); + string service = $"https://example.com/{uniqueGuid}"; + const string userName = "john.doe@auth.com"; + const string password = "letmein123"; + + string expectedTargetName = $"{TestNamespace}:https://example.com/{uniqueGuid}"; + + try + { + // Write + credManager.AddOrUpdate(service, userName, password); + + // Read + ICredential cred = credManager.Get(service, userName); + + // Validate + var winCred = cred as WindowsCredential; + Assert.NotNull(winCred); + Assert.Equal(userName, winCred.UserName); + Assert.Equal(password, winCred.Password); + Assert.Equal(service, winCred.Service); + Assert.Equal(expectedTargetName, winCred.TargetName); + } + finally + { + // Ensure we clean up after ourselves even in case of 'get' failures + credManager.Remove(service, userName); } } [PlatformFact(Platform.Windows)] public void WindowsCredentialManager_Get_KeyNotFound_ReturnsNull() { - WindowsCredentialManager credManager = WindowsCredentialManager.Open(); + WindowsCredentialManager credManager = WindowsCredentialManager.Open(TestNamespace); - // Unique key; guaranteed not to exist! - string key = Guid.NewGuid().ToString("N"); + // Unique service; guaranteed not to exist! + string service = Guid.NewGuid().ToString("N"); - ICredential credential = credManager.Get(key); + ICredential credential = credManager.Get(service, account: null); Assert.Null(credential); } [PlatformFact(Platform.Windows)] public void WindowsCredentialManager_Remove_KeyNotFound_ReturnsFalse() { - WindowsCredentialManager credManager = WindowsCredentialManager.Open(); + WindowsCredentialManager credManager = WindowsCredentialManager.Open(TestNamespace); - // Unique key; guaranteed not to exist! - string key = Guid.NewGuid().ToString("N"); + // Unique service; guaranteed not to exist! + string service = Guid.NewGuid().ToString("N"); - bool result = credManager.Remove(key); + bool result = credManager.Remove(service, account: null); Assert.False(result); } + + [PlatformFact(Platform.Windows)] + public void WindowsCredentialManager_AddOrUpdate_TargetNameAlreadyExists_CreatesWithUserInTargetName() + { + WindowsCredentialManager credManager = WindowsCredentialManager.Open(TestNamespace); + + // Create a service that is guaranteed to be unique + string uniqueGuid = Guid.NewGuid().ToString("N"); + string service = $"https://example.com/{uniqueGuid}"; + const string userName1 = "john.doe"; + const string userName2 = "jane.doe"; + const string password1 = "letmein123"; + const string password2 = "password123"; + + string expectedTargetName1 = $"{TestNamespace}:https://example.com/{uniqueGuid}"; + string expectedTargetName2 = $"{TestNamespace}:https://{userName2}@example.com/{uniqueGuid}"; + + try + { + // Add first credential + credManager.AddOrUpdate(service, userName1, password1); + + // Add second credential + credManager.AddOrUpdate(service, userName2, password2); + + // Validate first credential properties + ICredential cred1 = credManager.Get(service, userName1); + var winCred1 = cred1 as WindowsCredential; + Assert.NotNull(winCred1); + Assert.Equal(userName1, winCred1.UserName); + Assert.Equal(password1, winCred1.Password); + Assert.Equal(service, winCred1.Service); + Assert.Equal(expectedTargetName1, winCred1.TargetName); + + // Validate second credential properties + ICredential cred2 = credManager.Get(service, userName2); + var winCred2 = cred2 as WindowsCredential; + Assert.NotNull(winCred2); + Assert.Equal(userName2, winCred2.UserName); + Assert.Equal(password2, winCred2.Password); + Assert.Equal(service, winCred2.Service); + Assert.Equal(expectedTargetName2, winCred2.TargetName); + } + finally + { + // Ensure we clean up after ourselves in case of failures + credManager.Remove(service, userName1); + credManager.Remove(service, userName2); + } + } + + [PlatformFact(Platform.Windows)] + public void WindowsCredentialManager_AddOrUpdate_TargetNameAlreadyExistsAndUserWithAtCharacter_CreatesWithEscapedUserInTargetName() + { + WindowsCredentialManager credManager = WindowsCredentialManager.Open(TestNamespace); + + // Create a service that is guaranteed to be unique + string uniqueGuid = Guid.NewGuid().ToString("N"); + string service = $"https://example.com/{uniqueGuid}"; + const string userName1 = "john.doe@auth.com"; + const string userName2 = "jane.doe@auth.com"; + const string escapedUserName2 = "jane.doe_auth.com"; + const string password1 = "letmein123"; + const string password2 = "password123"; + + string expectedTargetName1 = $"{TestNamespace}:https://example.com/{uniqueGuid}"; + string expectedTargetName2 = $"{TestNamespace}:https://{escapedUserName2}@example.com/{uniqueGuid}"; + + try + { + // Add first credential + credManager.AddOrUpdate(service, userName1, password1); + + // Add second credential + credManager.AddOrUpdate(service, userName2, password2); + + // Validate first credential properties + ICredential cred1 = credManager.Get(service, userName1); + var winCred1 = cred1 as WindowsCredential; + Assert.NotNull(winCred1); + Assert.Equal(userName1, winCred1.UserName); + Assert.Equal(password1, winCred1.Password); + Assert.Equal(service, winCred1.Service); + Assert.Equal(expectedTargetName1, winCred1.TargetName); + + // Validate second credential properties + ICredential cred2 = credManager.Get(service, userName2); + var winCred2 = cred2 as WindowsCredential; + Assert.NotNull(winCred2); + Assert.Equal(userName2, winCred2.UserName); + Assert.Equal(password2, winCred2.Password); + Assert.Equal(service, winCred2.Service); + Assert.Equal(expectedTargetName2, winCred2.TargetName); + } + finally + { + // Ensure we clean up after ourselves in case of failures + credManager.Remove(service, userName1); + credManager.Remove(service, userName2); + } + } } } diff --git a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs index 7914d2b94..a7b0ee3a2 100644 --- a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs +++ b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs @@ -92,7 +92,7 @@ public CommandContext() FileSystem.GetCurrentDirectory() ); Settings = new Settings(Environment, Git); - CredentialStore = WindowsCredentialManager.Open(); + CredentialStore = WindowsCredentialManager.Open(Constants.CredentialNamespace); } else if (PlatformUtils.IsMacOS()) { @@ -123,7 +123,7 @@ public CommandContext() FileSystem.GetCurrentDirectory() ); Settings = new Settings(Environment, Git); - CredentialStore = new LinuxCredentialStore(Settings, Git); + CredentialStore = new LinuxCredentialStore(Settings, Git, Constants.CredentialNamespace); } else { diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/Native/Advapi32.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/Native/Advapi32.cs index 13b79b8bf..971599bcb 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/Native/Advapi32.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/Native/Advapi32.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. using System; using System.Runtime.InteropServices; -using System.Runtime.InteropServices.ComTypes; using FILETIME = System.Runtime.InteropServices.ComTypes.FILETIME; namespace Microsoft.Git.CredentialManager.Interop.Windows.Native @@ -31,8 +30,15 @@ public static extern bool CredDelete( int flags); [DllImport(LibraryName, EntryPoint = "CredFree", CallingConvention = CallingConvention.StdCall, CharSet = CharSet.Unicode, SetLastError = true)] - internal static extern void CredFree( + public static extern void CredFree( IntPtr credential); + + [DllImport(LibraryName, EntryPoint = "CredEnumerateW", CallingConvention = CallingConvention.StdCall, CharSet = CharSet.Unicode, SetLastError = true)] + public static extern bool CredEnumerate( + string filter, + CredentialEnumerateFlags flags, + out int count, + out IntPtr credentialsList); } public enum CredentialType @@ -50,6 +56,13 @@ public enum CredentialPersist Enterprise = 3, } + [Flags] + public enum CredentialEnumerateFlags + { + None = 0, + AllCredentials = 0x1 + } + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] public struct Win32Credential { @@ -64,7 +77,7 @@ public struct Win32Credential public IntPtr CredentialBlob; public CredentialPersist Persist; public int AttributeCount; - public IntPtr CredAttribute; + public IntPtr Attributes; [MarshalAs(UnmanagedType.LPWStr)] public string TargetAlias; [MarshalAs(UnmanagedType.LPWStr)] diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredential.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredential.cs new file mode 100644 index 000000000..c52f59dd9 --- /dev/null +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredential.cs @@ -0,0 +1,26 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. + +namespace Microsoft.Git.CredentialManager.Interop.Windows +{ + public class WindowsCredential : ICredential + { + public WindowsCredential(string service, string userName, string password, string targetName) + { + Service = service; + UserName = userName; + Password = password; + TargetName = targetName; + } + + public string Service { get; } + + public string UserName { get; } + + public string Password { get; } + + public string TargetName { get; } + + string ICredential.Account => UserName; + } +} diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs index 5ef8ddc74..da999ed9e 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. using System; +using System.Collections.Generic; +using System.Linq; using System.Runtime.InteropServices; using System.Text; using Microsoft.Git.CredentialManager.Interop.Windows.Native; @@ -9,119 +11,289 @@ namespace Microsoft.Git.CredentialManager.Interop.Windows { public class WindowsCredentialManager : ICredentialStore { + private const string TargetNameLegacyGenericPrefix = "LegacyGeneric:target="; + + private readonly string _namespace; + #region Constructors /// /// Open the Windows Credential Manager vault for the current user. /// + /// Optional namespace to scope credential operations. /// Current user's Credential Manager vault. - public static WindowsCredentialManager Open() + public static WindowsCredentialManager Open(string @namespace = null) { - return new WindowsCredentialManager(); + return new WindowsCredentialManager(@namespace); } - private WindowsCredentialManager() + private WindowsCredentialManager(string @namespace) { PlatformUtils.EnsureWindows(); + _namespace = @namespace; } #endregion #region ICredentialStore - public ICredential Get(string key) + public ICredential Get(string service, string account) + { + return Enumerate(service, account).FirstOrDefault(); + } + + public void AddOrUpdate(string service, string account, string secret) { - IntPtr credPtr = IntPtr.Zero; + EnsureArgument.NotNullOrWhiteSpace(service, nameof(service)); + + IntPtr existingCredPtr = IntPtr.Zero; + IntPtr credBlob = IntPtr.Zero; try { + // Determine if we need to update an existing credential, which might have + // a target name that does not include the account name. + // + // We first check for the presence of a credential with an account-less + // target name. + // + // - If such credential exists and *has the same account* then we will + // update that entry. + // - If such credential exists and does *not* have the same account then + // we must create a new entry with the account in the target name. + // - If no such credential exists then we create a new entry with the + // account-less target name. + // + string targetName = CreateTargetName(service, account: null); + if (Advapi32.CredRead(targetName, CredentialType.Generic, 0, out existingCredPtr)) + { + var existingCred = Marshal.PtrToStructure(existingCredPtr); + if (!StringComparer.Ordinal.Equals(existingCred.UserName, account)) + { + // Create new entry with the account in the target name + targetName = CreateTargetName(service, account); + } + } + + byte[] secretBytes = Encoding.Unicode.GetBytes(secret); + credBlob = Marshal.AllocHGlobal(secretBytes.Length); + Marshal.Copy(secretBytes, 0, credBlob, secretBytes.Length); + + var newCred = new Win32Credential + { + Type = CredentialType.Generic, + TargetName = targetName, + CredentialBlobSize = secretBytes.Length, + CredentialBlob = credBlob, + Persist = CredentialPersist.LocalMachine, + UserName = account, + }; + + int result = Win32Error.GetLastError( - Advapi32.CredRead(key, CredentialType.Generic, 0, out credPtr) + Advapi32.CredWrite(ref newCred, 0) + ); + + Win32Error.ThrowIfError(result, "Failed to write item to store."); + } + finally + { + if (credBlob != IntPtr.Zero) + { + Marshal.FreeHGlobal(credBlob); + } + + if (existingCredPtr != IntPtr.Zero) + { + Advapi32.CredFree(existingCredPtr); + } + } + } + + public bool Remove(string service, string account) + { + WindowsCredential credential = Enumerate(service, account).FirstOrDefault(); + + if (credential != null) + { + int result = Win32Error.GetLastError( + Advapi32.CredDelete(credential.TargetName, CredentialType.Generic, 0) ); switch (result) { case Win32Error.Success: - Win32Credential credential = Marshal.PtrToStructure(credPtr); + return true; - var userName = credential.UserName; + case Win32Error.NotFound: + return false; - byte[] passwordBytes = InteropUtils.ToByteArray(credential.CredentialBlob, credential.CredentialBlobSize); - var password = Encoding.Unicode.GetString(passwordBytes); + default: + Win32Error.ThrowIfError(result); + return false; + } + } - return new GitCredential(userName, password); + return false; + } + + #endregion + + private IEnumerable Enumerate(string service, string account) + { + IntPtr credList = IntPtr.Zero; + + try + { + int result = Win32Error.GetLastError( + Advapi32.CredEnumerate( + null, + CredentialEnumerateFlags.AllCredentials, + out int count, + out credList) + ); + + switch (result) + { + case Win32Error.Success: + int ptrSize = Marshal.SizeOf(); + for (int i = 0; i < count; i++) + { + IntPtr credPtr = Marshal.ReadIntPtr(credList, i * ptrSize); + Win32Credential credential = Marshal.PtrToStructure(credPtr); + + if (!IsMatch(service, account, credential)) + { + continue; + } + + yield return CreateCredentialFromStructure(credential); + } + break; case Win32Error.NotFound: - return null; + yield break; default: - Win32Error.ThrowIfError(result, "Failed to read item from store."); - return null; + Win32Error.ThrowIfError(result, "Failed to enumerate credentials."); + yield break; } } finally { - if (credPtr != IntPtr.Zero) + if (credList != IntPtr.Zero) { - Advapi32.CredFree(credPtr); + Advapi32.CredFree(credList); } } } - public void AddOrUpdate(string key, ICredential credential) + private WindowsCredential CreateCredentialFromStructure(Win32Credential credential) { - byte[] passwordBytes = Encoding.Unicode.GetBytes(credential.Password); + string password = null; + if (credential.CredentialBlobSize != 0 && credential.CredentialBlob != IntPtr.Zero) + { + byte[] passwordBytes = InteropUtils.ToByteArray( + credential.CredentialBlob, + credential.CredentialBlobSize); + password = Encoding.Unicode.GetString(passwordBytes); + } - var w32Credential = new Win32Credential + // Recover the service name from the target name + string service = credential.TargetName.TrimUntilIndexOf(TargetNameLegacyGenericPrefix); + if (!string.IsNullOrWhiteSpace(_namespace)) { - Type = CredentialType.Generic, - TargetName = key, - CredentialBlob = Marshal.AllocHGlobal(passwordBytes.Length), - CredentialBlobSize = passwordBytes.Length, - Persist = CredentialPersist.LocalMachine, - AttributeCount = 0, - UserName = credential.UserName, - }; + service = service.TrimUntilIndexOf($"{_namespace}:"); + } - try + return new WindowsCredential(service, credential.UserName, password, credential.TargetName); + } + + private bool IsMatch(string service, string account, Win32Credential credential) + { + // Match against the username first + if (!string.IsNullOrWhiteSpace(account) && + !StringComparer.Ordinal.Equals(account, credential.UserName)) { - Marshal.Copy(passwordBytes, 0, w32Credential.CredentialBlob, passwordBytes.Length); + return false; + } - int result = Win32Error.GetLastError( - Advapi32.CredWrite(ref w32Credential, 0) - ); + // Trim the "LegacyGeneric" prefix Windows adds and any namespace we have been filtered with + string targetName = credential.TargetName.TrimUntilIndexOf(TargetNameLegacyGenericPrefix); + if (!string.IsNullOrWhiteSpace(_namespace)) + { + targetName = targetName.TrimUntilIndexOf($"{_namespace}:"); + } - Win32Error.ThrowIfError(result, "Failed to write item to store."); + // If the target name matches the service name exactly then return 'match' + if (StringComparer.Ordinal.Equals(service, targetName)) + { + return true; } - finally + + // Try matching the target and service as URIs + if (Uri.TryCreate(service, UriKind.Absolute, out Uri serviceUri) && + Uri.TryCreate(targetName, UriKind.Absolute, out Uri targetUri)) { - if (w32Credential.CredentialBlob != IntPtr.Zero) + // Match host name + if (!StringComparer.OrdinalIgnoreCase.Equals(serviceUri.Host, targetUri.Host)) + { + return false; + } + + // Match port number + if (!serviceUri.IsDefaultPort && serviceUri.Port == targetUri.Port) + { + return false; + } + + // Match path + if (!string.IsNullOrWhiteSpace(serviceUri.AbsolutePath) && + !StringComparer.OrdinalIgnoreCase.Equals(serviceUri.AbsolutePath, targetUri.AbsolutePath)) { - Marshal.FreeHGlobal(w32Credential.CredentialBlob); + return false; } + + // URLs match + return true; } + + // Unable to match + return false; } - public bool Remove(string key) + private string CreateTargetName(string service, string account) { - int result = Win32Error.GetLastError( - Advapi32.CredDelete(key, CredentialType.Generic, 0) - ); + var serviceUri = new Uri(service, UriKind.Absolute); + var sb = new StringBuilder(); - switch (result) + if (!string.IsNullOrWhiteSpace(_namespace)) { - case Win32Error.Success: - return true; + sb.AppendFormat("{0}:", _namespace); + } - case Win32Error.NotFound: - return false; + if (!string.IsNullOrWhiteSpace(serviceUri.Scheme)) + { + sb.AppendFormat("{0}://", serviceUri.Scheme); + } - default: - Win32Error.ThrowIfError(result); - return false; + if (!string.IsNullOrWhiteSpace(account)) + { + string escapedAccount = account.Replace('@', '_'); + sb.AppendFormat("{0}@", escapedAccount); } - } - #endregion + if (!string.IsNullOrWhiteSpace(serviceUri.Host)) + { + sb.Append(serviceUri.Host); + } + + if (!string.IsNullOrWhiteSpace(serviceUri.AbsolutePath.TrimEnd('/'))) + { + sb.Append(serviceUri.AbsolutePath); + } + + return sb.ToString(); + } } } From 094ab39a4beb238f3598e0578f2ba011a8eb06a8 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2020 15:53:37 +0100 Subject: [PATCH 04/16] wincredmgr: recover the correct service name 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. --- .../Windows/WindowsCredentialManagerTests.cs | 29 +++++++++++ .../Windows/WindowsCredentialManager.cs | 48 +++++++++++++++++-- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs index 918ed8ea9..e34b54425 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Windows/WindowsCredentialManagerTests.cs @@ -206,5 +206,34 @@ public void WindowsCredentialManager_AddOrUpdate_TargetNameAlreadyExistsAndUserW credManager.Remove(service, userName2); } } + + [Theory] + [InlineData("https://example.com", "https://example.com")] + [InlineData("https://example.com/", "https://example.com/")] + [InlineData("https://example.com/@", "https://example.com/@")] + [InlineData("https://example.com/path", "https://example.com/path")] + [InlineData("https://example.com/path@", "https://example.com/path@")] + [InlineData("https://example.com:123/path@", "https://example.com:123/path@")] + [InlineData("https://example.com/path/", "https://example.com/path/")] + [InlineData("https://example.com/path@/", "https://example.com/path@/")] + [InlineData("https://example.com:123/path@/", "https://example.com:123/path@/")] + [InlineData("https://example.com/path/foo", "https://example.com/path/foo")] + [InlineData("https://example.com/path@/foo", "https://example.com/path@/foo")] + [InlineData("https://userinfo@example.com", "https://example.com")] + [InlineData("https://userinfo@example.com/", "https://example.com/")] + [InlineData("https://userinfo@example.com/@", "https://example.com/@")] + [InlineData("https://userinfo@example.com/path", "https://example.com/path")] + [InlineData("https://userinfo@example.com/path@", "https://example.com/path@")] + [InlineData("https://userinfo@example.com/path/", "https://example.com/path/")] + [InlineData("https://userinfo@example.com:123/path/", "https://example.com:123/path/")] + [InlineData("https://userinfo@example.com/path@/", "https://example.com/path@/")] + [InlineData("https://userinfo@example.com:123/path@/", "https://example.com:123/path@/")] + [InlineData("https://userinfo@example.com/path/foo", "https://example.com/path/foo")] + [InlineData("https://userinfo@example.com/path@/foo", "https://example.com/path@/foo")] + public void WindowsCredentialManager_RemoveUriUserInfo(string input, string expected) + { + string actual = WindowsCredentialManager.RemoveUriUserInfo(input); + Assert.Equal(expected, actual); + } } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs index da999ed9e..ae11befbc 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsCredentialManager.cs @@ -199,14 +199,56 @@ private WindowsCredential CreateCredentialFromStructure(Win32Credential credenti password = Encoding.Unicode.GetString(passwordBytes); } + // Recover the target name we gave from the internal (raw) target name + string targetName = credential.TargetName.TrimUntilIndexOf(TargetNameLegacyGenericPrefix); + // Recover the service name from the target name - string service = credential.TargetName.TrimUntilIndexOf(TargetNameLegacyGenericPrefix); + string serviceName = targetName; if (!string.IsNullOrWhiteSpace(_namespace)) { - service = service.TrimUntilIndexOf($"{_namespace}:"); + serviceName = serviceName.TrimUntilIndexOf($"{_namespace}:"); + } + + // Strip any userinfo component from the service name + serviceName = RemoveUriUserInfo(serviceName); + + return new WindowsCredential(serviceName, credential.UserName, password, targetName); + } + + public /* for testing */ static string RemoveUriUserInfo(string url) + { + // To remove the userinfo component we must search for the end of the :// scheme + // delimiter, and the start of the @ userinfo delimiter. We don't want to match + // any other '@' character however (such as one in the URI path). + // To ensure this we only consider an '@' character that exists before the first + // '/' character after the scheme delimiter - that is to say the authority-path + // separator. + // + // authority + // |-----------| + // scheme://userinfo@host/path + // + int schemeDelimIdx = url.IndexOf(Uri.SchemeDelimiter, StringComparison.Ordinal); + if (schemeDelimIdx > 0) + { + int authorityIdx = schemeDelimIdx + Uri.SchemeDelimiter.Length; + int slashIdx = url.IndexOf("/", authorityIdx, StringComparison.Ordinal); + int atIdx = url.IndexOf("@", StringComparison.Ordinal); + + // No path component or trailing slash; use end of string + if (slashIdx < 0) + { + slashIdx = url.Length - 1; + } + + // Only if the '@' is before the first slash is this the userinfo delimiter + if (0 < atIdx && atIdx < slashIdx) + { + return url.Substring(0, authorityIdx) + url.Substring(atIdx + 1); + } } - return new WindowsCredential(service, credential.UserName, password, credential.TargetName); + return url; } private bool IsMatch(string service, string account, Win32Credential credential) From d4a16961a31f689e1346217f7e63191c210ea21d Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Fri, 4 Sep 2020 15:07:19 +0100 Subject: [PATCH 05/16] libsecret: update libsecret interop to match new interface Update the SecretServiceCollection credential store (backed by libsecret) to match the new ICredentialStore interface and access model. --- .../Linux/SecretServiceCollectionTests.cs | 39 ++-- .../Interop/Linux/LinuxCredentialStore.cs | 21 +- .../Interop/Linux/Native/Gobject.cs | 3 + .../Interop/Linux/Native/Libsecret.cs | 3 + .../Interop/Linux/SecretServiceCollection.cs | 191 ++++++++++++------ .../Interop/Linux/SecretServiceCredential.cs | 25 +++ 6 files changed, 186 insertions(+), 96 deletions(-) create mode 100644 src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCredential.cs diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Linux/SecretServiceCollectionTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Linux/SecretServiceCollectionTests.cs index 7633f1bdc..a8ad63a86 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Linux/SecretServiceCollectionTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/Interop/Linux/SecretServiceCollectionTests.cs @@ -8,57 +8,58 @@ namespace Microsoft.Git.CredentialManager.Tests.Interop.Linux { public class SecretServiceCollectionTests { + private const string TestNamespace = "git-test"; + [PlatformFact(Platform.Linux, Skip = "Cannot run headless")] public void SecretServiceCollection_ReadWriteDelete() { - SecretServiceCollection collection = SecretServiceCollection.Open(); + SecretServiceCollection collection = SecretServiceCollection.Open(TestNamespace); - // Create a key that is guarenteed to be unique - string key = $"secretkey-{Guid.NewGuid():N}"; + // Create a service that is guaranteed to be unique + string service = $"https://example.com/{Guid.NewGuid():N}"; const string userName = "john.doe"; const string password = "letmein123"; - var credential = new GitCredential(userName, password); try { // Write - collection.AddOrUpdate(key, credential); + collection.AddOrUpdate(service, userName, password); // Read - ICredential outCredential = collection.Get(key); + ICredential outCredential = collection.Get(service, userName); Assert.NotNull(outCredential); - Assert.Equal(credential.UserName, outCredential.UserName); - Assert.Equal(credential.Password, outCredential.Password); + Assert.Equal(userName, userName); + Assert.Equal(password, outCredential.Password); } finally { // Ensure we clean up after ourselves even in case of 'get' failures - collection.Remove(key); + collection.Remove(service, userName); } } [PlatformFact(Platform.Linux, Skip = "Cannot run headless")] - public void SecretServiceCollection_Get_KeyNotFound_ReturnsNull() + public void SecretServiceCollection_Get_NotFound_ReturnsNull() { - SecretServiceCollection collection = SecretServiceCollection.Open(); + SecretServiceCollection collection = SecretServiceCollection.Open(TestNamespace); - // Unique key; guaranteed not to exist! - string key = Guid.NewGuid().ToString("N"); + // Unique service; guaranteed not to exist! + string service = $"https://example.com/{Guid.NewGuid():N}"; - ICredential credential = collection.Get(key); + ICredential credential = collection.Get(service, null); Assert.Null(credential); } [PlatformFact(Platform.Linux, Skip = "Cannot run headless")] - public void SecretServiceCollection_Remove_KeyNotFound_ReturnsFalse() + public void SecretServiceCollection_Remove_NotFound_ReturnsFalse() { - SecretServiceCollection collection = SecretServiceCollection.Open(); + SecretServiceCollection collection = SecretServiceCollection.Open(TestNamespace); - // Unique key; guaranteed not to exist! - string key = Guid.NewGuid().ToString("N"); + // Unique service; guaranteed not to exist! + string service = $"https://example.com/{Guid.NewGuid():N}"; - bool result = collection.Remove(key); + bool result = collection.Remove(service, account: null); Assert.False(result); } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/LinuxCredentialStore.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/LinuxCredentialStore.cs index df6451012..d02d09121 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/LinuxCredentialStore.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/LinuxCredentialStore.cs @@ -1,8 +1,5 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. -using System; -using System.Runtime.InteropServices; -using System.Text; namespace Microsoft.Git.CredentialManager.Interop.Linux { @@ -10,36 +7,38 @@ public class LinuxCredentialStore : ICredentialStore { private readonly ISettings _settings; private readonly IGit _git; + private readonly string _namespace; private ICredentialStore _backingStore; - public LinuxCredentialStore(ISettings settings, IGit git) + public LinuxCredentialStore(ISettings settings, IGit git, string @namespace = null) { EnsureArgument.NotNull(settings, nameof(settings)); EnsureArgument.NotNull(git, nameof(git)); _settings = settings; _git = git; + _namespace = @namespace; } #region ICredentialStore - public ICredential Get(string key) + public ICredential Get(string service, string account) { EnsureBackingStore(); - return _backingStore.Get(key); + return _backingStore.Get(service, account); } - public void AddOrUpdate(string key, ICredential credential) + public void AddOrUpdate(string service, string account, string secret) { EnsureBackingStore(); - _backingStore.AddOrUpdate(key, credential); + _backingStore.AddOrUpdate(service, account, secret); } - public bool Remove(string key) + public bool Remove(string service, string account) { EnsureBackingStore(); - return _backingStore.Remove(key); + return _backingStore.Remove(service, account); } #endregion @@ -51,7 +50,7 @@ private void EnsureBackingStore() // TODO: determine the available backing stores based on the current session // TODO: prompt for the desired backing store // TODO: store the desired backing store to ~/.gitconfig - _backingStore = SecretServiceCollection.Open(); + _backingStore = SecretServiceCollection.Open(_namespace); } } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Gobject.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Gobject.cs index b115c316c..c468c10f2 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Gobject.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Gobject.cs @@ -9,6 +9,9 @@ public static class Gobject { private const string LibraryName = "libgobject-2.0.so.0"; + [DllImport(LibraryName, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern void g_object_ref(IntPtr @object); + [DllImport(LibraryName, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] public static extern void g_object_unref(IntPtr @object); } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Libsecret.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Libsecret.cs index 3a89a6a47..2d4560f68 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Libsecret.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/Native/Libsecret.cs @@ -147,5 +147,8 @@ public static extern unsafe int secret_service_unlock_sync(SecretService* servic [DllImport(LibraryName, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] public static extern void secret_password_free(IntPtr password); + + [DllImport(LibraryName, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern unsafe bool secret_item_delete_sync(SecretItem* self, IntPtr cancellable, out Glib.GError* error); } } diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCollection.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCollection.cs index 78053e0e5..b6e8fc063 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCollection.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCollection.cs @@ -14,56 +14,51 @@ namespace Microsoft.Git.CredentialManager.Interop.Linux public class SecretServiceCollection : ICredentialStore { private const string SchemaName = "com.microsoft.GitCredentialManager"; - private const string KeyAttributeName = "key"; - private const string UserAttributeName = "user"; + private const string ServiceAttributeName = "service"; + private const string AccountAttributeName = "account"; private const string PlainTextContentType = "plain/text"; + private readonly string _namespace; + #region Constructors /// /// Open the default secret collection for the current user. /// + /// Optional namespace to scope credential operations. /// Default secret collection. - public static SecretServiceCollection Open() + public static SecretServiceCollection Open(string @namespace = null) { - return new SecretServiceCollection(); + return new SecretServiceCollection(@namespace); } - private SecretServiceCollection() + private SecretServiceCollection(string @namespace) { PlatformUtils.EnsureLinux(); + _namespace = @namespace; } #endregion #region ICredentialStore - public unsafe ICredential Get(string key) + public unsafe ICredential Get(string service, string account) { GHashTable* queryAttrs = null; - GHashTable* secretAttrs = null; - IntPtr userKeyPtr = IntPtr.Zero; - IntPtr passwordPtr = IntPtr.Zero; GList* results = null; GError* error = null; try { - SecretService* service = GetSecretService(); + SecretService* secService = GetSecretService(); - // Build search query - queryAttrs = g_hash_table_new_full( - g_str_hash, g_str_equal, - Marshal.FreeHGlobal, Marshal.FreeHGlobal); - IntPtr keyKeyPtr = Marshal.StringToHGlobalAnsi(KeyAttributeName); - IntPtr keyValuePtr = Marshal.StringToHGlobalAnsi(key); - g_hash_table_insert(queryAttrs, keyKeyPtr, keyValuePtr); + queryAttrs = CreateSearchQuery(service, account); SecretSchema schema = GetSchema(); - // Execute search query and return one result + // Execute search query and return the first result results = secret_service_search_sync( - service, + secService, ref schema, queryAttrs, SecretSearchFlags.SECRET_SEARCH_UNLOCK, @@ -94,7 +89,7 @@ public unsafe ICredential Get(string key) }; int numUnlocked = secret_service_unlock_sync( - service, + secService, &toUnlockList, IntPtr.Zero, out _, @@ -107,25 +102,7 @@ out error } } - // Extract the user attribute - secretAttrs = secret_item_get_attributes(item); - userKeyPtr = Marshal.StringToHGlobalAnsi(UserAttributeName); - IntPtr userValuePtr = g_hash_table_lookup(secretAttrs, userKeyPtr); - string userName = Marshal.PtrToStringAuto(userValuePtr); - - // Load the secret value - secret_item_load_secret_sync(item, IntPtr.Zero, out error); - SecretValue* value = secret_item_get_secret(item); - if (value == null) - { - throw new InteropException("Failed to load secret", -1); - } - - // Extract the secret/password - passwordPtr = secret_value_unref_to_password(value, out int passwordLength); - string password = Marshal.PtrToStringAuto(passwordPtr, passwordLength); - - return new GitCredential(userName, password); + return CreateCredentialFromItem(item); } return null; @@ -133,15 +110,12 @@ out error finally { if (queryAttrs != null) g_hash_table_destroy(queryAttrs); - if (secretAttrs != null) g_hash_table_unref(secretAttrs); - if (userKeyPtr != IntPtr.Zero) Marshal.FreeHGlobal(userKeyPtr); - if (passwordPtr != IntPtr.Zero) secret_password_free(passwordPtr); if (error != null) g_error_free(error); if (results != null) g_list_free_full(results, g_object_unref); } } - public unsafe void AddOrUpdate(string key, ICredential credential) + public unsafe void AddOrUpdate(string service, string account, string secret) { GHashTable* attributes = null; SecretValue* secretValue = null; @@ -149,33 +123,37 @@ public unsafe void AddOrUpdate(string key, ICredential credential) try { - SecretService* service = GetSecretService(); + SecretService* secService = GetSecretService(); // Create attributes for the key and user attributes = g_hash_table_new_full(g_str_hash, g_str_equal, Marshal.FreeHGlobal, Marshal.FreeHGlobal); - IntPtr keyKeyPtr = Marshal.StringToHGlobalAnsi(KeyAttributeName); - IntPtr keyValuePtr = Marshal.StringToHGlobalAnsi(key); - g_hash_table_insert(attributes, keyKeyPtr, keyValuePtr); + string fullServiceName = CreateServiceName(service); + IntPtr serviceKeyPtr = Marshal.StringToHGlobalAnsi(ServiceAttributeName); + IntPtr serviceValuePtr = Marshal.StringToHGlobalAnsi(fullServiceName); + g_hash_table_insert(attributes, serviceKeyPtr, serviceValuePtr); - IntPtr userKeyPtr = Marshal.StringToHGlobalAnsi(UserAttributeName); - IntPtr userValuePtr = Marshal.StringToHGlobalAnsi(credential.UserName); - g_hash_table_insert(attributes, userKeyPtr, userValuePtr); + if (!string.IsNullOrWhiteSpace(account)) + { + IntPtr accountKeyPtr = Marshal.StringToHGlobalAnsi(AccountAttributeName); + IntPtr accountValuePtr = Marshal.StringToHGlobalAnsi(account); + g_hash_table_insert(attributes, accountKeyPtr, accountValuePtr); + } - // Create the secret value from the password - byte[] passwordBytes = Encoding.UTF8.GetBytes(credential.Password); - secretValue = secret_value_new(passwordBytes, passwordBytes.Length, PlainTextContentType); + // Create the secret value object from the secret string + byte[] secretBytes = Encoding.UTF8.GetBytes(secret); + secretValue = secret_value_new(secretBytes, secretBytes.Length, PlainTextContentType); SecretSchema schema = GetSchema(); // Store the secret with the associated attributes bool result = secret_service_store_sync( - service, + secService, ref schema, attributes, null, - key, // Use the key also as the label + fullServiceName, // Use full service name as label secretValue, IntPtr.Zero, out error); @@ -200,27 +178,23 @@ public unsafe void AddOrUpdate(string key, ICredential credential) } } - public unsafe bool Remove(string key) + public unsafe bool Remove(string service, string account) { GHashTable* attributes = null; GError* error = null; try { - SecretService* service = GetSecretService(); + SecretService* secService = GetSecretService(); - // Create attributes for the key - attributes = g_hash_table_new_full(g_str_hash, g_str_equal, - Marshal.FreeHGlobal, Marshal.FreeHGlobal); - IntPtr keyKeyPtr = Marshal.StringToHGlobalAnsi(KeyAttributeName); - IntPtr keyValuePtr = Marshal.StringToHGlobalAnsi(key); - g_hash_table_insert(attributes, keyKeyPtr, keyValuePtr); + // Create search query + attributes = CreateSearchQuery(service, account); SecretSchema schema = GetSchema(); // Erase the secret with the specified key bool result = secret_service_clear_sync( - service, + secService, ref schema, attributes, IntPtr.Zero, @@ -244,6 +218,91 @@ public unsafe bool Remove(string key) #endregion + private unsafe GHashTable* CreateSearchQuery(string service, string account) + { + // Build search query + GHashTable* queryAttrs = g_hash_table_new_full( + g_str_hash, g_str_equal, + Marshal.FreeHGlobal, Marshal.FreeHGlobal); + + // If we've be given a service then filter on the service attribute + if (!string.IsNullOrWhiteSpace(service)) + { + string fullServiceName = CreateServiceName(service); + IntPtr keyPtr = Marshal.StringToHGlobalAnsi(ServiceAttributeName); + IntPtr valuePtr = Marshal.StringToHGlobalAnsi(fullServiceName); + g_hash_table_insert(queryAttrs, keyPtr, valuePtr); + } + + // If we've be given a username then filter on the account attribute + if (!string.IsNullOrWhiteSpace(account)) + { + IntPtr keyPtr = Marshal.StringToHGlobalAnsi(AccountAttributeName); + IntPtr valuePtr = Marshal.StringToHGlobalAnsi(account); + g_hash_table_insert(queryAttrs, keyPtr, valuePtr); + } + + return queryAttrs; + } + + private static unsafe ICredential CreateCredentialFromItem(SecretItem* item) + { + GHashTable* secretAttrs = null; + IntPtr serviceKeyPtr = IntPtr.Zero; + IntPtr accountKeyPtr = IntPtr.Zero; + IntPtr passwordPtr = IntPtr.Zero; + GError* error = null; + + try + { + secretAttrs = secret_item_get_attributes(item); + + // Extract the service attribute + serviceKeyPtr = Marshal.StringToHGlobalAnsi(ServiceAttributeName); + IntPtr serviceValuePtr = g_hash_table_lookup(secretAttrs, serviceKeyPtr); + string service = Marshal.PtrToStringAuto(serviceValuePtr); + + // Extract the account attribute + accountKeyPtr = Marshal.StringToHGlobalAnsi(AccountAttributeName); + IntPtr accountValuePtr = g_hash_table_lookup(secretAttrs, accountKeyPtr); + string account = Marshal.PtrToStringAuto(accountValuePtr); + + // Load the secret value + secret_item_load_secret_sync(item, IntPtr.Zero, out error); + SecretValue* value = secret_item_get_secret(item); + if (value == null) + { + throw new InteropException("Failed to load secret", -1); + } + + // Extract the secret/password + passwordPtr = secret_value_unref_to_password(value, out int passwordLength); + string password = Marshal.PtrToStringAuto(passwordPtr, passwordLength); + + return new SecretServiceCredential(service, account, password); + } + finally + { + if (secretAttrs != null) g_hash_table_unref(secretAttrs); + if (accountKeyPtr != IntPtr.Zero) Marshal.FreeHGlobal(accountKeyPtr); + if (serviceKeyPtr != IntPtr.Zero) Marshal.FreeHGlobal(serviceKeyPtr); + if (passwordPtr != IntPtr.Zero) secret_password_free(passwordPtr); + if (error != null) g_error_free(error); + } + } + + private string CreateServiceName(string service) + { + var sb = new StringBuilder(); + if (!string.IsNullOrWhiteSpace(_namespace)) + { + sb.AppendFormat("{0}:", _namespace); + } + + sb.Append(service); + return sb.ToString(); + } + private static unsafe SecretService* GetSecretService() { // Get a handle to the default secret service, open a session, @@ -274,13 +333,13 @@ private static SecretSchema GetSchema() schema.attributes[0] = new SecretSchemaAttribute { - name = KeyAttributeName, + name = ServiceAttributeName, type = SECRET_SCHEMA_ATTRIBUTE_STRING }; schema.attributes[1] = new SecretSchemaAttribute { - name = UserAttributeName, + name = AccountAttributeName, type = SECRET_SCHEMA_ATTRIBUTE_STRING }; diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCredential.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCredential.cs new file mode 100644 index 000000000..2335145c6 --- /dev/null +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Linux/SecretServiceCredential.cs @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. +using System.Diagnostics; + +namespace Microsoft.Git.CredentialManager.Interop.Linux +{ + [DebuggerDisplay("{DebuggerDisplay}")] + public class SecretServiceCredential : ICredential + { + internal SecretServiceCredential(string service, string account, string password) + { + Service = service; + Account = account; + Password = password; + } + + public string Service { get; } + + public string Account { get; } + + public string Password { get; } + + private string DebuggerDisplay => $"[Service: {Service}, Account: {Account}]"; + } +} From bac549c83c5ce1a415aea22d0b26d0c3650991cb Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:00:08 +0100 Subject: [PATCH 06/16] inputargs: update remote URI generation to support ports Update the remote URI generation from the program InputArguments to support port numbers, and special characters in usernames. --- .../InputArgumentsTests.cs | 165 ++++++++++++++++++ .../InputArguments.cs | 63 ++++++- 2 files changed, 222 insertions(+), 6 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/InputArgumentsTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/InputArgumentsTests.cs index dbd2ca035..4429b0e58 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/InputArgumentsTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/InputArgumentsTests.cs @@ -94,6 +94,71 @@ public void InputArguments_GetRemoteUri_Authority_ReturnsUriWithAuthority() Assert.Equal(expectedUri, actualUri); } + [Fact] + public void InputArguments_GetRemoteUri_IncludeUser_Authority_ReturnsUriWithAuthorityAndUser() + { + var expectedUri = new Uri("https://john.doe@example.com/"); + + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com", + + // Username should appear in the returned URI; the password should not + ["username"] = "john.doe", + ["password"] = "password123" + }; + + var inputArgs = new InputArguments(dict); + + Uri actualUri = inputArgs.GetRemoteUri(includeUser: true); + + Assert.NotNull(actualUri); + Assert.Equal(expectedUri, actualUri); + } + + [Fact] + public void InputArguments_GetRemoteUri_IncludeUserSpecialCharacters_Authority_ReturnsUriWithAuthorityAndUser() + { + var expectedUri = new Uri("https://john.doe%40domain.com@example.com/"); + + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com", + + // Username should appear in the returned URI; the password should not + ["username"] = "john.doe@domain.com", + ["password"] = "password123" + }; + + var inputArgs = new InputArguments(dict); + + Uri actualUri = inputArgs.GetRemoteUri(includeUser: true); + + Assert.NotNull(actualUri); + Assert.Equal(expectedUri, actualUri); + } + + [Fact] + public void InputArguments_GetRemoteUri_AuthorityAndPort_ReturnsUriWithAuthorityAndPort() + { + var expectedUri = new Uri("https://example.com:456/"); + + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com:456" + }; + + var inputArgs = new InputArguments(dict); + + Uri actualUri = inputArgs.GetRemoteUri(); + + Assert.NotNull(actualUri); + Assert.Equal(expectedUri, actualUri); + } + [Fact] public void InputArguments_GetRemoteUri_AuthorityPath_ReturnsUriWithAuthorityAndPath() { @@ -137,5 +202,105 @@ public void InputArguments_GetRemoteUri_AuthorityPathUserInfo_ReturnsUriWithAuth Assert.NotNull(actualUri); Assert.Equal(expectedUri, actualUri); } + + [Fact] + public void InputArguments_GetRemoteUri_IncludeUser_AuthorityPathUserInfo_ReturnsUriWithAll() + { + var expectedUri = new Uri("https://john.doe@example.com/an/example/path"); + + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com", + ["path"] = "an/example/path", + + // Username should appear in the returned URI; the password should not + ["username"] = "john.doe", + ["password"] = "password123" + }; + + var inputArgs = new InputArguments(dict); + + Uri actualUri = inputArgs.GetRemoteUri(includeUser: true); + + Assert.NotNull(actualUri); + Assert.Equal(expectedUri, actualUri); + } + + [Fact] + public void InputArguments_TryGetHostAndPort_NoPort_ReturnsHostName() + { + const string expectedHostName = "example.com"; + + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com" + }; + + var inputArgs = new InputArguments(dict); + + bool result = inputArgs.TryGetHostAndPort(out string actualHostName, out int? actualPort); + + Assert.True(result); + Assert.NotNull(actualHostName); + Assert.Equal(expectedHostName, actualHostName); + Assert.Null(actualPort); + } + + [Fact] + public void InputArguments_TryGetHostAndPort_Port_ReturnsHostNameAndPort() + { + const string expectedHostName = "example.com"; + const int expectedPort = 456; + + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com:456" + }; + + var inputArgs = new InputArguments(dict); + + bool result = inputArgs.TryGetHostAndPort(out string actualHostName, out int? actualPort); + + Assert.True(result); + Assert.NotNull(actualHostName); + Assert.Equal(expectedHostName, actualHostName); + Assert.NotNull(actualPort); + Assert.Equal(expectedPort, actualPort); + } + + [Fact] + public void InputArguments_TryGetHostAndPort_BadPort_ReturnsFalse() + { + var dict = new Dictionary + { + ["protocol"] = "https", + ["host"] = "example.com:not-a-port" + }; + + var inputArgs = new InputArguments(dict); + + bool result = inputArgs.TryGetHostAndPort(out _, out int? actualPort); + + Assert.False(result); + Assert.Null(actualPort); + } + + [Fact] + public void InputArguments_TryGetHostAndPort_NoHostNoPort_ReturnsFalse() + { + var dict = new Dictionary + { + ["protocol"] = "https", + }; + + var inputArgs = new InputArguments(dict); + + bool result = inputArgs.TryGetHostAndPort(out _, out _); + + Assert.False(result); + } } } diff --git a/src/shared/Microsoft.Git.CredentialManager/InputArguments.cs b/src/shared/Microsoft.Git.CredentialManager/InputArguments.cs index bb744025b..1a51c3e2c 100644 --- a/src/shared/Microsoft.Git.CredentialManager/InputArguments.cs +++ b/src/shared/Microsoft.Git.CredentialManager/InputArguments.cs @@ -48,22 +48,73 @@ public string this[string key] public string GetArgumentOrDefault(string key) { - return _dict.TryGetValue(key, out string value) ? value : null; + return TryGetArgument(key, out string value) ? value : null; } - public Uri GetRemoteUri() + public bool TryGetArgument(string key, out string value) + { + return _dict.TryGetValue(key, out value); + } + + public bool TryGetHostAndPort(out string host, out int? port) + { + host = null; + port = null; + + if (Host is null) + { + return false; + } + + // Split port number and hostname from host input argument + string[] hostParts = Host.Split(':'); + if (hostParts.Length > 0) + { + host = hostParts[0]; + } + + if (hostParts.Length > 1) + { + if (!int.TryParse(hostParts[1], out int portInt)) + { + return false; + } + + port = portInt; + } + + return true; + } + + public Uri GetRemoteUri(bool includeUser = false) { if (Protocol is null || Host is null) { return null; } - var ub = new UriBuilder(Protocol, Host) + string[] hostParts = Host.Split(':'); + if (hostParts.Length > 0) { - Path = Path - }; + var ub = new UriBuilder(Protocol, hostParts[0]) + { + Path = Path + }; + + if (hostParts.Length > 1 && int.TryParse(hostParts[1], out int port)) + { + ub.Port = port; + } + + if (includeUser) + { + ub.UserName = Uri.EscapeDataString(UserName); + } + + return ub.Uri; + } - return ub.Uri; + return null; } #endregion From a8e2125301e750012d76de8dd4233ab4cde1ad89 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:02:23 +0100 Subject: [PATCH 07/16] hostprovider: update host prvdr base to use service name 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). --- .../HostProviderTests.cs | 226 ++++++++---------- .../HostProvider.cs | 79 +++--- .../Objects/TestHostProvider.cs | 7 - 3 files changed, 127 insertions(+), 185 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/HostProviderTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/HostProviderTests.cs index d8cdbca2a..c35a528c4 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/HostProviderTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/HostProviderTests.cs @@ -14,23 +14,22 @@ public class HostProviderTests [Fact] public async Task HostProvider_GetCredentialAsync_CredentialExists_ReturnsExistingCredential() { - const string testUserName = "john.doe"; - const string testPassword = "letmein123"; - const string testCredentialKey = "git:test-cred-key"; + const string userName = "john.doe"; + const string password = "letmein123"; + const string service = "https://example.com"; var input = new InputArguments(new Dictionary { - ["username"] = testUserName, - ["password"] = testPassword, + ["protocol"] = "https", + ["host"] = "example.com", + ["username"] = userName, + ["password"] = password, }); - var context = new TestCommandContext - { - CredentialStore = {[testCredentialKey] = new GitCredential(testUserName, testPassword)} - }; + var context = new TestCommandContext(); + context.CredentialStore.Add(service, userName, password); var provider = new TestHostProvider(context) { IsSupportedFunc = _ => true, - CredentialKey = testCredentialKey, GenerateCredentialFunc = _ => { Assert.True(false, "Should never be called"); @@ -40,20 +39,21 @@ public async Task HostProvider_GetCredentialAsync_CredentialExists_ReturnsExisti ICredential actualCredential = await ((IHostProvider) provider).GetCredentialAsync(input); - Assert.Equal(testUserName, actualCredential.UserName); - Assert.Equal(testPassword, actualCredential.Password); + Assert.Equal(userName, actualCredential.Account); + Assert.Equal(password, actualCredential.Password); } [Fact] public async Task HostProvider_GetCredentialAsync_CredentialDoesNotExist_ReturnsNewGeneratedCredential() { - const string testUserName = "john.doe"; - const string testPassword = "letmein123"; - const string testCredentialKey = "git:test-cred-key"; + const string userName = "john.doe"; + const string password = "letmein123"; var input = new InputArguments(new Dictionary { - ["username"] = testUserName, - ["password"] = testPassword, + ["protocol"] = "https", + ["host"] = "example.com", + ["username"] = userName, + ["password"] = password, }); bool generateWasCalled = false; @@ -61,19 +61,18 @@ public async Task HostProvider_GetCredentialAsync_CredentialDoesNotExist_Returns var provider = new TestHostProvider(context) { IsSupportedFunc = _ => true, - CredentialKey = testCredentialKey, GenerateCredentialFunc = _ => { generateWasCalled = true; - return new GitCredential(testUserName, testPassword); + return new GitCredential(userName, password); }, }; ICredential actualCredential = await ((IHostProvider) provider).GetCredentialAsync(input); Assert.True(generateWasCalled); - Assert.Equal(testUserName, actualCredential.UserName); - Assert.Equal(testPassword, actualCredential.Password); + Assert.Equal(userName, actualCredential.Account); + Assert.Equal(password, actualCredential.Password); } @@ -86,42 +85,45 @@ public async Task HostProvider_StoreCredentialAsync_EmptyCredential_DoesNotStore { const string emptyUserName = ""; const string emptyPassword = ""; - const string testCredentialKey = "git:test-cred-key"; var input = new InputArguments(new Dictionary { + ["protocol"] = "https", + ["host"] = "example.com", ["username"] = emptyUserName, ["password"] = emptyPassword, }); var context = new TestCommandContext(); - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + var provider = new TestHostProvider(context); await ((IHostProvider) provider).StoreCredentialAsync(input); - Assert.Empty(context.CredentialStore); + Assert.Equal(0, context.CredentialStore.Count); } [Fact] public async Task HostProvider_StoreCredentialAsync_NonEmptyCredential_StoresCredential() { - const string testUserName = "john.doe"; - const string testPassword = "letmein123"; - const string testCredentialKey = "git:test-cred-key"; + const string userName = "john.doe"; + const string password = "letmein123"; + const string service = "https://example.com"; var input = new InputArguments(new Dictionary { - ["username"] = testUserName, - ["password"] = testPassword, + ["protocol"] = "https", + ["host"] = "example.com", + ["username"] = userName, + ["password"] = password, }); var context = new TestCommandContext(); - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + var provider = new TestHostProvider(context); await ((IHostProvider) provider).StoreCredentialAsync(input); - Assert.Single(context.CredentialStore); - Assert.True(context.CredentialStore.TryGetValue(testCredentialKey, out ICredential storedCredential)); - Assert.Equal(testUserName, storedCredential.UserName); - Assert.Equal(testPassword, storedCredential.Password); + Assert.Equal(1, context.CredentialStore.Count); + Assert.True(context.CredentialStore.TryGet(service, userName, out var storedCredential)); + Assert.Equal(userName, storedCredential.Account); + Assert.Equal(password, storedCredential.Password); } [Fact] @@ -130,24 +132,24 @@ public async Task HostProvider_StoreCredentialAsync_NonEmptyCredential_ExistingC const string testUserName = "john.doe"; const string testPasswordOld = "letmein123-old"; const string testPasswordNew = "letmein123-new"; - const string testCredentialKey = "git:test-cred-key"; + const string testService = "https://example.com"; var input = new InputArguments(new Dictionary { + ["protocol"] = "https", + ["host"] = "example.com", ["username"] = testUserName, ["password"] = testPasswordNew, }); - var context = new TestCommandContext - { - CredentialStore = {[testCredentialKey] = new GitCredential(testUserName, testPasswordOld)} - }; - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + var context = new TestCommandContext(); + context.CredentialStore.Add(testService, testUserName, testPasswordOld); + var provider = new TestHostProvider(context); await ((IHostProvider) provider).StoreCredentialAsync(input); - Assert.Single(context.CredentialStore); - Assert.True(context.CredentialStore.TryGetValue(testCredentialKey, out ICredential storedCredential)); - Assert.Equal(testUserName, storedCredential.UserName); + Assert.Equal(1, context.CredentialStore.Count); + Assert.True(context.CredentialStore.TryGet(testService, testUserName, out var storedCredential)); + Assert.Equal(testUserName, storedCredential.Account); Assert.Equal(testPasswordNew, storedCredential.Password); } @@ -156,130 +158,100 @@ public async Task HostProvider_StoreCredentialAsync_NonEmptyCredential_ExistingC #region EraseCredentialAsync [Fact] - public async Task HostProvider_EraseCredentialAsync_NoInputUserPass_CredentialExists_ErasesCredential() + public async Task HostProvider_EraseCredentialAsync_NoInputUser_CredentialExists_ErasesOneCredential() { - const string testCredentialKey = "git:test-cred-key"; - const string otherCredentialKey1 = "git:credential1"; - const string otherCredentialKey2 = "git:credential2"; - var input = new InputArguments(new Dictionary()); - - var context = new TestCommandContext + const string service = "https://example.com"; + const string userName1 = "john.doe"; + const string userName2 = "alice"; + const string userName3 = "bob"; + var input = new InputArguments(new Dictionary { - CredentialStore = - { - [testCredentialKey] = new GitCredential("john.doe", "letmein123"), - [otherCredentialKey1] = new GitCredential("this.should-1", "not.be.erased-1"), - [otherCredentialKey2] = new GitCredential("this.should-2", "not.be.erased-2") - } - }; - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + ["protocol"] = "https", + ["host"] = "example.com", + }); + + var context = new TestCommandContext(); + context.CredentialStore.Add(service, userName1, "letmein123"); + context.CredentialStore.Add(service, userName2, "do-not-erase-me"); + context.CredentialStore.Add(service, userName3, "here-forever"); + var provider = new TestHostProvider(context); await ((IHostProvider) provider).EraseCredentialAsync(input); Assert.Equal(2, context.CredentialStore.Count); - Assert.False(context.CredentialStore.ContainsKey(testCredentialKey)); - Assert.True(context.CredentialStore.ContainsKey(otherCredentialKey1)); - Assert.True(context.CredentialStore.ContainsKey(otherCredentialKey2)); } [Fact] - public async Task HostProvider_EraseCredentialAsync_InputUserPass_CredentialExists_UserNotMatch_DoesNothing() + public async Task HostProvider_EraseCredentialAsync_InputUser_CredentialExists_UserNotMatch_DoesNothing() { - const string testUserName = "john.doe"; - const string testPassword = "letmein123"; - const string testCredentialKey = "git:test-cred-key"; + const string userName1 = "john.doe"; + const string userName2 = "alice"; + const string password = "letmein123"; + const string service = "https://example.com"; var input = new InputArguments(new Dictionary { - ["username"] = testUserName, - ["password"] = testPassword, + ["protocol"] = "https", + ["host"] = "example.com", + ["username"] = userName1, + ["password"] = password, }); - var context = new TestCommandContext - { - CredentialStore = {[testCredentialKey] = new GitCredential("different-username", testPassword)} - }; - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + var context = new TestCommandContext(); + context.CredentialStore.Add(service, userName2, password); + var provider = new TestHostProvider(context); await ((IHostProvider) provider).EraseCredentialAsync(input); - Assert.Single(context.CredentialStore); - Assert.True(context.CredentialStore.ContainsKey(testCredentialKey)); + Assert.Equal(1, context.CredentialStore.Count); + Assert.True(context.CredentialStore.Contains(service, userName2)); } [Fact] - public async Task HostProvider_EraseCredentialAsync_InputUserPass_CredentialExists_PassNotMatch_DoesNothing() + public async Task HostProvider_EraseCredentialAsync_InputUser_CredentialExists_UserMatch_ErasesCredential() { - const string testUserName = "john.doe"; - const string testPassword = "letmein123"; - const string testCredentialKey = "git:test-cred-key"; + const string userName = "john.doe"; + const string password = "letmein123"; + const string service = "https://example.com"; var input = new InputArguments(new Dictionary { - ["username"] = testUserName, - ["password"] = testPassword, + ["protocol"] = "https", + ["host"] = "example.com", + ["username"] = userName, + ["password"] = password, }); - var context = new TestCommandContext - { - CredentialStore = - { - [testCredentialKey] = new GitCredential(testUserName, "different-password"), - } - }; - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + var context = new TestCommandContext(); + context.CredentialStore.Add(service, userName, password); + var provider = new TestHostProvider(context); await ((IHostProvider) provider).EraseCredentialAsync(input); - Assert.Single(context.CredentialStore); - Assert.True(context.CredentialStore.ContainsKey(testCredentialKey)); + Assert.Equal(0, context.CredentialStore.Count); + Assert.False(context.CredentialStore.Contains(service, userName)); } [Fact] - public async Task HostProvider_EraseCredentialAsync_InputUserPass_CredentialExists_UserPassMatch_ErasesCredential() + public async Task HostProvider_EraseCredentialAsync_DifferentHost_DoesNothing() { - const string testUserName = "john.doe"; - const string testPassword = "letmein123"; - const string testCredentialKey = "git:test-cred-key"; + const string service2 = "https://example2.com"; + const string service3 = "https://example3.com"; + const string userName = "john.doe"; var input = new InputArguments(new Dictionary { - ["username"] = testUserName, - ["password"] = testPassword, + ["protocol"] = "https", + ["host"] = "example1.com", }); - var context = new TestCommandContext - { - CredentialStore = {[testCredentialKey] = new GitCredential(testUserName, testPassword)} - }; - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; - - await ((IHostProvider) provider).EraseCredentialAsync(input); - - Assert.Empty(context.CredentialStore); - Assert.False(context.CredentialStore.ContainsKey(testCredentialKey)); - } - - [Fact] - public async Task HostProvider_EraseCredentialAsync_NoCredential_DoesNothing() - { - const string testCredentialKey = "git:test-cred-key"; - const string otherCredentialKey1 = "git:credential1"; - const string otherCredentialKey2 = "git:credential2"; - var input = new InputArguments(new Dictionary()); - - var context = new TestCommandContext - { - CredentialStore = - { - [otherCredentialKey1] = new GitCredential("this.should-1", "not.be.erased-1"), - [otherCredentialKey2] = new GitCredential("this.should-2", "not.be.erased-2") - } - }; - var provider = new TestHostProvider(context) {CredentialKey = testCredentialKey}; + var context = new TestCommandContext(); + context.CredentialStore.Add(service2, userName, "keep-me"); + context.CredentialStore.Add(service3, userName, "also-keep-me"); + var provider = new TestHostProvider(context); await ((IHostProvider) provider).EraseCredentialAsync(input); Assert.Equal(2, context.CredentialStore.Count); - Assert.True(context.CredentialStore.ContainsKey(otherCredentialKey1)); - Assert.True(context.CredentialStore.ContainsKey(otherCredentialKey2)); + Assert.True(context.CredentialStore.Contains(service2, userName)); + Assert.True(context.CredentialStore.Contains(service3, userName)); } #endregion diff --git a/src/shared/Microsoft.Git.CredentialManager/HostProvider.cs b/src/shared/Microsoft.Git.CredentialManager/HostProvider.cs index 6ba432977..b791ec561 100644 --- a/src/shared/Microsoft.Git.CredentialManager/HostProvider.cs +++ b/src/shared/Microsoft.Git.CredentialManager/HostProvider.cs @@ -79,16 +79,27 @@ protected HostProvider(ICommandContext context) public abstract bool IsSupported(InputArguments input); /// - /// Return a key that uniquely represents the given Git credential query arguments. + /// Return a string that uniquely identifies the service that a credential should be stored against. /// /// + /// /// This key forms part of the identifier used to retrieve and store credentials from the OS secure /// credential storage system. It is important the returned value is stable over time to avoid any /// potential re-authentication requests. + /// + /// + /// The default implementation returns the absolute URI formed by from the + /// without any userinfo component. Any trailing slashes are trimmed. + /// /// /// Input arguments of a Git credential query. - /// Stable credential key. - public abstract string GetCredentialKey(InputArguments input); + /// Credential service name. + public virtual string GetServiceName(InputArguments input) + { + // By default we assume the service name will be the absolute URI based on the + // input arguments from Git, without any userinfo part. + return input.GetRemoteUri(includeUser: false).AbsoluteUri.TrimEnd('/'); + } /// /// Create a new credential used for accessing the remote Git repository on this hosting service. @@ -99,14 +110,14 @@ protected HostProvider(ICommandContext context) public virtual async Task GetCredentialAsync(InputArguments input) { - // Try and locate an existing PAT in the OS credential store - string credentialKey = GetCredentialKey(input); - Context.Trace.WriteLine($"Looking for existing credential in store with key '{credentialKey}'..."); - ICredential credential = Context.CredentialStore.Get(credentialKey); + // Try and locate an existing credential in the OS credential store + string service = GetServiceName(input); + Context.Trace.WriteLine($"Looking for existing credential in store with service={service} account={input.UserName}..."); + ICredential credential = Context.CredentialStore.Get(service, input.UserName); if (credential == null) { - Context.Trace.WriteLine("No existing credential found."); + Context.Trace.WriteLine("No existing credentials found."); // No existing credential was found, create a new one Context.Trace.WriteLine("Creating new credential..."); @@ -123,25 +134,20 @@ public virtual async Task GetCredentialAsync(InputArguments input) public virtual Task StoreCredentialAsync(InputArguments input) { - // Create the credential based on Git's input - string userName = input.UserName; - string password = input.Password; + string service = GetServiceName(input); // WIA-authentication is signaled to Git as an empty username/password pair // and we will get called to 'store' these WIA credentials. // We avoid storing empty credentials. - if (string.IsNullOrWhiteSpace(userName) && string.IsNullOrWhiteSpace(password)) + if (string.IsNullOrWhiteSpace(input.UserName) && string.IsNullOrWhiteSpace(input.Password)) { Context.Trace.WriteLine("Not storing empty credential."); } else { - var credential = new GitCredential(userName, password); - // Add or update the credential in the store. - string credentialKey = GetCredentialKey(input); - Context.Trace.WriteLine($"Storing credential with key '{credentialKey}'..."); - Context.CredentialStore.AddOrUpdate(credentialKey, credential); + Context.Trace.WriteLine($"Storing credential with service={service} account={input.UserName}..."); + Context.CredentialStore.AddOrUpdate(service, input.UserName, input.Password); Context.Trace.WriteLine("Credential was successfully stored."); } @@ -150,46 +156,17 @@ public virtual Task StoreCredentialAsync(InputArguments input) public virtual Task EraseCredentialAsync(InputArguments input) { - // Try to locate an existing credential with the computed key - string credentialKey = GetCredentialKey(input); - Context.Trace.WriteLine($"Looking for existing credential in store with key '{credentialKey}'..."); - ICredential credential = Context.CredentialStore.Get(credentialKey); - if (credential == null) - { - Context.Trace.WriteLine("No stored credential was found."); - return Task.CompletedTask; - } - else - { - Context.Trace.WriteLine("Existing credential found."); - } - - // If we've been given a specific username and/or password we should only proceed - // to erase the stored credential if they match exactly - if (!string.IsNullOrWhiteSpace(input.UserName) && !StringComparer.Ordinal.Equals(input.UserName, credential.UserName)) - { - Context.Trace.WriteLine("Stored username does not match specified username - not erasing credential."); - Context.Trace.WriteLine($"\tInput username={input.UserName}"); - Context.Trace.WriteLine($"\tStored username={credential.UserName}"); - return Task.CompletedTask; - } - - if (!string.IsNullOrWhiteSpace(input.Password) && !StringComparer.Ordinal.Equals(input.Password, credential.Password)) - { - Context.Trace.WriteLine("Stored password does not match specified password - not erasing credential."); - Context.Trace.WriteLineSecrets("\tInput password={0}", new object[] {input.Password}); - Context.Trace.WriteLineSecrets("\tStored password={0}", new object[] {credential.Password}); - return Task.CompletedTask; - } + string service = GetServiceName(input); - Context.Trace.WriteLine("Erasing stored credential..."); - if (Context.CredentialStore.Remove(credentialKey)) + // Try to locate an existing credential + Context.Trace.WriteLine($"Erasing stored credential in store with service={service} account={input.UserName}..."); + if (Context.CredentialStore.Remove(service, input.UserName)) { Context.Trace.WriteLine("Credential was successfully erased."); } else { - Context.Trace.WriteLine("Credential erase failed."); + Context.Trace.WriteLine("No credential was erased."); } return Task.CompletedTask; diff --git a/src/shared/TestInfrastructure/Objects/TestHostProvider.cs b/src/shared/TestInfrastructure/Objects/TestHostProvider.cs index 18dc50e6b..b5caab153 100644 --- a/src/shared/TestInfrastructure/Objects/TestHostProvider.cs +++ b/src/shared/TestInfrastructure/Objects/TestHostProvider.cs @@ -12,8 +12,6 @@ public TestHostProvider(ICommandContext context) public Func IsSupportedFunc { get; set; } - public string CredentialKey { get; set; } - public string LegacyAuthorityIdValue { get; set; } public Func GenerateCredentialFunc { get; set; } @@ -28,11 +26,6 @@ public TestHostProvider(ICommandContext context) public override bool IsSupported(InputArguments input) => IsSupportedFunc(input); - public override string GetCredentialKey(InputArguments input) - { - return CredentialKey; - } - public override Task GenerateCredentialAsync(InputArguments input) { return Task.FromResult(GenerateCredentialFunc(input)); From 9ab036de07ef60dc694398c11d8803d082a1c43c Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:05:04 +0100 Subject: [PATCH 08/16] genericprovider: update the generic provider for new APIs Update the generic host provider to support the new credential storage model and HostProvider base class APIs. --- .../GenericHostProviderTests.cs | 16 ++++++------- .../GenericHostProvider.cs | 24 ++----------------- 2 files changed, 10 insertions(+), 30 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/GenericHostProviderTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/GenericHostProviderTests.cs index ebb9a8b4f..85d42a133 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/GenericHostProviderTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/GenericHostProviderTests.cs @@ -39,9 +39,9 @@ public void GenericHostProvider_IsSupported(string protocol, bool expected) } [Fact] - public void GenericHostProvider_GetCredentialKey_ReturnsCorrectKey() + public void GenericHostProvider_GetCredentialServiceUrl_ReturnsCorrectKey() { - const string expectedKey = "git:https://john.doe@example.com/foo/bar"; + const string expectedService = "https://example.com/foo/bar"; var input = new InputArguments(new Dictionary { @@ -53,9 +53,9 @@ public void GenericHostProvider_GetCredentialKey_ReturnsCorrectKey() var provider = new GenericHostProvider(new TestCommandContext()); - string actualKey = provider.GetCredentialKey(input); + string actualService = provider.GetServiceName(input); - Assert.Equal(expectedKey, actualKey); + Assert.Equal(expectedService, actualService); } [Fact] @@ -86,7 +86,7 @@ public async Task GenericHostProvider_CreateCredentialAsync_WiaNotAllowed_Return ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); wiaAuthMock.Verify(x => x.GetIsSupportedAsync(It.IsAny()), Times.Never); basicAuthMock.Verify(x => x.GetCredentials(It.IsAny(), It.IsAny()), Times.Once); @@ -121,7 +121,7 @@ public async Task GenericHostProvider_CreateCredentialAsync_LegacyAuthorityBasic ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); wiaAuthMock.Verify(x => x.GetIsSupportedAsync(It.IsAny()), Times.Never); basicAuthMock.Verify(x => x.GetCredentials(It.IsAny(), It.IsAny()), Times.Once); @@ -168,7 +168,7 @@ private static async Task TestCreateCredentialAsync_ReturnsEmptyCredential(bool ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(string.Empty, credential.UserName); + Assert.Equal(string.Empty, credential.Account); Assert.Equal(string.Empty, credential.Password); basicAuthMock.Verify(x => x.GetCredentials(It.IsAny(), It.IsAny()), Times.Never); } @@ -199,7 +199,7 @@ private static async Task TestCreateCredentialAsync_ReturnsBasicCredential(bool ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); basicAuthMock.Verify(x => x.GetCredentials(It.IsAny(), It.IsAny()), Times.Once); } diff --git a/src/shared/Microsoft.Git.CredentialManager/GenericHostProvider.cs b/src/shared/Microsoft.Git.CredentialManager/GenericHostProvider.cs index 481d192d3..421e7cc80 100644 --- a/src/shared/Microsoft.Git.CredentialManager/GenericHostProvider.cs +++ b/src/shared/Microsoft.Git.CredentialManager/GenericHostProvider.cs @@ -46,16 +46,11 @@ public override bool IsSupported(InputArguments input) StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "https")); } - public override string GetCredentialKey(InputArguments input) - { - return $"git:{GetUriFromInput(input).AbsoluteUri}"; - } - public override async Task GenerateCredentialAsync(InputArguments input) { ThrowIfDisposed(); - Uri uri = GetUriFromInput(input); + Uri uri = input.GetRemoteUri(); // Determine the if the host supports Windows Integration Authentication (WIA) if (IsWindowsAuthAllowed) @@ -89,7 +84,7 @@ public override async Task GenerateCredentialAsync(InputArguments i } Context.Trace.WriteLine("Prompting for basic credentials..."); - return _basicAuth.GetCredentials(uri.AbsoluteUri, uri.UserInfo); + return _basicAuth.GetCredentials(uri.AbsoluteUri, input.UserName); } /// @@ -124,20 +119,5 @@ protected override void ReleaseManagedResources() } #endregion - - #region Helpers - - private static Uri GetUriFromInput(InputArguments input) - { - return new UriBuilder - { - Scheme = input.Protocol, - UserName = input.UserName, - Host = input.Host, - Path = input.Path - }.Uri; - } - - #endregion } } From 860ec54ec0a118340bfef1ef31c7f8c42162111f Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:05:52 +0100 Subject: [PATCH 09/16] basic: update basic auth to match new cred interface Update the Basic authentication component to match the new ICredential interface. --- .../Authentication/BasicAuthenticationTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/BasicAuthenticationTests.cs b/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/BasicAuthenticationTests.cs index bbe3ea2a9..8221b0cdf 100644 --- a/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/BasicAuthenticationTests.cs +++ b/src/shared/Microsoft.Git.CredentialManager.Tests/Authentication/BasicAuthenticationTests.cs @@ -33,7 +33,7 @@ public void BasicAuthentication_GetCredentials_NonDesktopSession_ResourceAndUser ICredential credential = basicAuth.GetCredentials(testResource, testUserName); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); } @@ -52,7 +52,7 @@ public void BasicAuthentication_GetCredentials_NonDesktopSession_Resource_UserPa ICredential credential = basicAuth.GetCredentials(testResource); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); } @@ -99,7 +99,7 @@ public void BasicAuthentication_GetCredentials_DesktopSession_Resource_UserPassP ICredential credential = basicAuth.GetCredentials(testResource); Assert.NotNull(credential); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); } @@ -130,7 +130,7 @@ public void BasicAuthentication_GetCredentials_DesktopSession_ResourceAndUser_Pa ICredential credential = basicAuth.GetCredentials(testResource, testUserName); Assert.NotNull(credential); - Assert.Equal(testUserName, credential.UserName); + Assert.Equal(testUserName, credential.Account); Assert.Equal(testPassword, credential.Password); } @@ -162,7 +162,7 @@ public void BasicAuthentication_GetCredentials_DesktopSession_ResourceAndUser_Pa ICredential credential = basicAuth.GetCredentials(testResource, testUserName); Assert.NotNull(credential); - Assert.Equal(newUserName, credential.UserName); + Assert.Equal(newUserName, credential.Account); Assert.Equal(testPassword, credential.Password); } } From e925512477515ffb08a65390588f649354ef8c87 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:07:30 +0100 Subject: [PATCH 10/16] bitbucket: update BB provider to implement new cred APIs Update the BitBucket provider to support and implement the new credential storage/recall model and APIs. --- .../BitbucketHostProvider.cs | 176 +++++------------- 1 file changed, 50 insertions(+), 126 deletions(-) diff --git a/src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs b/src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs index c54934490..fb4da154a 100644 --- a/src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs +++ b/src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; using System.Net; -using System.Text; using System.Threading.Tasks; using Microsoft.Git.CredentialManager; using Microsoft.Git.CredentialManager.Authentication.OAuth; @@ -40,50 +39,57 @@ public BitbucketHostProvider(ICommandContext context, IBitbucketAuthentication b public bool IsSupported(InputArguments input) { + if (input is null) + { + return false; + } + + // Split port number and hostname from host input argument + input.TryGetHostAndPort(out string hostName, out _); + // We do not support unencrypted HTTP communications to Bitbucket, // but we report `true` here for HTTP so that we can show a helpful // error message for the user in `GetCredentialAsync`. - return input != null && - (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || + return (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "https")) && - input.Host.EndsWith(BitbucketConstants.BitbucketBaseUrlHost, StringComparison.OrdinalIgnoreCase); + hostName.EndsWith(BitbucketConstants.BitbucketBaseUrlHost, StringComparison.OrdinalIgnoreCase); + } public async Task GetCredentialAsync(InputArguments input) { - // Compute the target URI - Uri targetUri = GetTargetUri(input); + // Compute the remote URI + Uri targetUri = input.GetRemoteUri(); + + bool isBitbucketServer = IsBitbucketServer(input); // We should not allow unencrypted communication and should inform the user if (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") - && !IsBitbucketServer(targetUri)) + && !isBitbucketServer) { throw new Exception("Unencrypted HTTP is not supported for Bitbucket.org. Ensure the repository remote URL is using HTTPS."); } - // Try and get the username specified in the remote URL if any - string targetUriUser = targetUri.GetUserName(); - // Check for presence of refresh_token entry in credential store - string refreshKey = GetRefreshTokenKey(input); - _context.Trace.WriteLine($"Checking for refresh token with key '{refreshKey}'..."); - ICredential refreshToken = _context.CredentialStore.Get(refreshKey); + string refreshTokenService = GetRefreshTokenServiceName(input); + _context.Trace.WriteLine("Checking for refresh token..."); + ICredential refreshToken = _context.CredentialStore.Get(refreshTokenService, input.UserName); if (refreshToken is null) { // There is no refresh token either because this is a non-2FA enabled account (where OAuth is not // required), or because we previously erased the RT. // Check for the presence of a credential in the store - string credentialKey = GetCredentialKey(input); - _context.Trace.WriteLine($"Checking for credentials with key '{credentialKey}'..."); - ICredential credential = _context.CredentialStore.Get(credentialKey); + string credentialService = GetServiceName(input); + _context.Trace.WriteLine("Checking for credentials..."); + ICredential credential = _context.CredentialStore.Get(credentialService, input.UserName); if (credential is null) { // We don't have any credentials to use at all! Start with the assumption of no 2FA requirement // and capture username and password via an interactive prompt. - credential = await _bitbucketAuth.GetBasicCredentialsAsync(targetUri, targetUriUser); + credential = await _bitbucketAuth.GetBasicCredentialsAsync(targetUri, input.UserName); if (credential is null) { throw new Exception("User cancelled authentication prompt."); @@ -94,7 +100,9 @@ public async Task GetCredentialAsync(InputArguments input) // or we have a freshly captured user/pass. Regardless, we must check if these credentials // pass and two-factor requirement on the account. _context.Trace.WriteLine("Checking if two-factor requirements for stored credentials..."); - bool requires2Fa = await RequiresTwoFactorAuthenticationAsync(credential, targetUri); + + // BBS does not support 2FA out of the box so neither does GCM + bool requires2Fa = !isBitbucketServer && await RequiresTwoFactorAuthenticationAsync(credential); if (!requires2Fa) { _context.Trace.WriteLine("Two-factor requirement passed with stored credentials"); @@ -134,9 +142,8 @@ public async Task GetCredentialAsync(InputArguments input) _context.Trace.WriteLine($"Username for refreshed OAuth credential is '{refreshUserName}'"); // Store the refreshed RT - _context.Trace.WriteLine($"Storing new refresh token with key '{refreshKey}'..."); - _context.CredentialStore.AddOrUpdate(refreshKey, - new GitCredential(refreshUserName, refreshResult.RefreshToken)); + _context.Trace.WriteLine("Storing new refresh token..."); + _context.CredentialStore.AddOrUpdate(refreshTokenService, input.UserName, refreshResult.RefreshToken); // Return new access token return new GitCredential(refreshUserName, refreshResult.AccessToken); @@ -164,8 +171,8 @@ public async Task GetCredentialAsync(InputArguments input) _context.Trace.WriteLine($"Username for OAuth credential is '{newUserName}'"); // Store the new RT - _context.Trace.WriteLine($"Storing new refresh token with key '{refreshKey}'..."); - _context.CredentialStore.AddOrUpdate(refreshKey, new GitCredential(newUserName, oauthResult.RefreshToken)); + _context.Trace.WriteLine("Storing new refresh token..."); + _context.CredentialStore.AddOrUpdate(refreshTokenService, newUserName, oauthResult.RefreshToken); _context.Trace.WriteLine("Refresh token was successfully stored."); // Return the new AT as the credential @@ -177,26 +184,12 @@ public Task StoreCredentialAsync(InputArguments input) // It doesn't matter if this is an OAuth access token, or the literal username & password // because we store them the same way, against the same credential key in the store. // The OAuth refresh token is already stored on the 'get' request. - string credentialKey = GetCredentialKey(input); - ICredential credential = new GitCredential(input.UserName, input.Password); + string service = GetServiceName(input); - _context.Trace.WriteLine($"Storing credential with key '{credentialKey}'..."); - _context.CredentialStore.AddOrUpdate(credentialKey, credential); + _context.Trace.WriteLine("Storing credential..."); + _context.CredentialStore.AddOrUpdate(service, input.UserName, input.Password); _context.Trace.WriteLine("Credential was successfully stored."); - Uri targetUri = GetTargetUri(input); - if (IsBitbucketServer(targetUri)) - { - // BBS doesn't usually include the username in the urls which means they aren't included in the GET call, - // which means if we store only with the username the credentials are never found again ... - // This does have the potential to overwrite itself for different BbS accounts, - // but typically BbS doesn't encourage multiple user accounts - string bbsCredentialKey = GetBitbucketServerCredentialKey(input); - _context.Trace.WriteLine($"Storing Bitbucket Server credential with key '{bbsCredentialKey}'..."); - _context.CredentialStore.AddOrUpdate(bbsCredentialKey, credential); - _context.Trace.WriteLine("Bitbucket Server Credential was successfully stored."); - } - return Task.CompletedTask; } @@ -205,9 +198,10 @@ public Task EraseCredentialAsync(InputArguments input) // Erase the stored credential (which may be either the literal username & password, or // the OAuth access token). We don't need to erase the OAuth refresh token because on the // next 'get' request, if the RT is bad we will erase and reacquire a new one at that point. - string credentialKey = GetCredentialKey(input); - _context.Trace.WriteLine($"Erasing credential with key '{credentialKey}'..."); - if (_context.CredentialStore.Remove(credentialKey)) + string service = GetServiceName(input); + + _context.Trace.WriteLine("Erasing credential..."); + if (_context.CredentialStore.Remove(service, input.UserName)) { _context.Trace.WriteLine("Credential was successfully erased."); } @@ -234,15 +228,10 @@ private async Task ResolveOAuthUserNameAsync(string accessToken) throw new Exception($"Failed to resolve username. HTTP: {result.StatusCode}"); } - private async Task RequiresTwoFactorAuthenticationAsync(ICredential credentials, Uri targetUri) + private async Task RequiresTwoFactorAuthenticationAsync(ICredential credentials) { - if (IsBitbucketServer(targetUri)) - { - // BBS does not support 2FA out of the box so neither does GCM - return false; - } - - RestApiResult result = await _bitbucketApi.GetUserInformationAsync(credentials.UserName, credentials.Password, false); + RestApiResult result = await _bitbucketApi.GetUserInformationAsync( + credentials.Account, credentials.Password, false); switch (result.StatusCode) { // 2FA may not be required @@ -262,91 +251,26 @@ private async Task RequiresTwoFactorAuthenticationAsync(ICredential creden } } - private string GetCredentialKey(InputArguments input) - { - // The credential (user/pass or an OAuth access token) key is the full target URI. - // If the full path is included (credential.useHttpPath = true) then respect that. - string url = GetTargetUri(input).AbsoluteUri; - - // Trim trailing slash - if (url.EndsWith("/")) - { - url = url.Substring(0, url.Length - 1); - } - - return $"git:{url}"; - } - - private string GetBitbucketServerCredentialKey(InputArguments input) + private static string GetServiceName(InputArguments input) { - // The credential (user/pass or an OAuth access token) key is the full target URI. - // If the full path is included (credential.useHttpPath = true) then respect that. - string url = GetBitbucketServerTargetUri(input).AbsoluteUri; - - // Trim trailing slash - if (url.EndsWith("/")) - { - url = url.Substring(0, url.Length - 1); - } - - return $"git:{url}"; + return input.GetRemoteUri(includeUser: false).AbsoluteUri.TrimEnd('/'); } - private string GetRefreshTokenKey(InputArguments input) + private static string GetRefreshTokenServiceName(InputArguments input) { - Uri targetUri = GetTargetUri(input); + Uri baseUri = input.GetRemoteUri(includeUser: false); // The refresh token key never includes the path component. - // Starting from the full target URI, build the following: - // - // {scheme}://[{userinfo}@]{authority}/refresh_token - // - - var url = new StringBuilder(); - - url.Append(targetUri.Scheme) - .Append(Uri.SchemeDelimiter); - - if (!string.IsNullOrWhiteSpace(targetUri.UserInfo)) - { - url.Append(targetUri.UserInfo) - .Append('@'); - } - - url.Append(targetUri.Authority) - .Append("/refresh_token"); - - return $"git:{url}"; - } - - private static Uri GetTargetUri(InputArguments input) - { - Uri uri = new UriBuilder - { - Scheme = input.Protocol, - Host = input.Host, - Path = input.Path, - UserName = input.UserName - }.Uri; - - return uri; - } - - private static Uri GetBitbucketServerTargetUri(InputArguments input) - { - Uri uri = new UriBuilder - { - Scheme = input.Protocol, - Host = input.Host, - Path = input.Path - }.Uri; + // Instead we use the path component to specify this is the "refresh_token". + Uri uri = new UriBuilder(baseUri) {Path = "/refresh_token"}.Uri; - return uri; + return uri.AbsoluteUri.TrimEnd('/'); } - private bool IsBitbucketServer(Uri targetUri) + private static bool IsBitbucketServer(InputArguments input) { - return !targetUri.Host.Equals(BitbucketConstants.BitbucketBaseUrlHost); + input.TryGetHostAndPort(out string hostName, out _); + return !hostName.Equals(BitbucketConstants.BitbucketBaseUrlHost); } #endregion From f8cd76eb71168de22b9b46fdce00a2e61e0c8fe6 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:11:06 +0100 Subject: [PATCH 11/16] github: update GitHub provider to impl new cred model Update the GitHub provider to implement and follow the new credential storage/recall/matching model and ICredentialStore APIs. --- .../GitHub.Tests/GitHubHostProviderTests.cs | 41 ++++---- src/shared/GitHub.Tests/GitHubRestApiTests.cs | 8 +- src/shared/GitHub/AuthenticationResult.cs | 4 +- src/shared/GitHub/GitHubAuthentication.cs | 20 +++- src/shared/GitHub/GitHubHostProvider.cs | 93 ++++++++++--------- src/shared/GitHub/GitHubRestApi.cs | 10 +- .../AuthenticationPrompts.cs | 6 +- src/windows/GitHub.UI.Windows/Program.cs | 3 +- 8 files changed, 103 insertions(+), 82 deletions(-) diff --git a/src/shared/GitHub.Tests/GitHubHostProviderTests.cs b/src/shared/GitHub.Tests/GitHubHostProviderTests.cs index 71a93c990..b8da4393a 100644 --- a/src/shared/GitHub.Tests/GitHubHostProviderTests.cs +++ b/src/shared/GitHub.Tests/GitHubHostProviderTests.cs @@ -112,9 +112,9 @@ public void GitHubHostProvider_IsSupported_NonGitHub_ReturnsFalse() } [Fact] - public void GitHubHostProvider_GetCredentialKey_GitHubHost_ReturnsCorrectKey() + public void GitHubHostProvider_GetCredentialServiceUrl_GitHubHost_ReturnsCorrectKey() { - const string expectedKey = "git:https://github.com"; + const string expectedService = "https://github.com"; var input = new InputArguments(new Dictionary { ["protocol"] = "https", @@ -122,14 +122,14 @@ public void GitHubHostProvider_GetCredentialKey_GitHubHost_ReturnsCorrectKey() }); var provider = new GitHubHostProvider(new TestCommandContext()); - string actualKey = provider.GetCredentialKey(input); - Assert.Equal(expectedKey, actualKey); + string actualService = provider.GetServiceName(input); + Assert.Equal(expectedService, actualService); } [Fact] - public void GitHubHostProvider_GetCredentialKey_GistHost_ReturnsCorrectKey() + public void GitHubHostProvider_GetCredentialServiceUrl_GistHost_ReturnsCorrectKey() { - const string expectedKey = "git:https://github.com"; + const string expectedService = "https://github.com"; var input = new InputArguments(new Dictionary { ["protocol"] = "https", @@ -137,8 +137,8 @@ public void GitHubHostProvider_GetCredentialKey_GistHost_ReturnsCorrectKey() }); var provider = new GitHubHostProvider(new TestCommandContext()); - string actualKey = provider.GetCredentialKey(input); - Assert.Equal(expectedKey, actualKey); + string actualService = provider.GetServiceName(input); + Assert.Equal(expectedService, actualService); } [Fact] @@ -379,26 +379,29 @@ public async Task GitHubHostProvider_GenerateCredentialAsync_OAuth_ReturnsCreden GitHubConstants.OAuthScopes.Workflow, }; + var expectedUserName = "john.doe"; var tokenValue = "OAUTH-TOKEN"; var response = new OAuth2TokenResult(tokenValue, "bearer"); var context = new TestCommandContext(); var ghAuthMock = new Mock(MockBehavior.Strict); - ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, It.IsAny())) + ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, null, It.IsAny())) .ReturnsAsync(new AuthenticationPromptResult(AuthenticationModes.OAuth)); ghAuthMock.Setup(x => x.GetOAuthTokenAsync(expectedTargetUri, It.IsAny>())) .ReturnsAsync(response); var ghApiMock = new Mock(MockBehavior.Strict); + ghApiMock.Setup(x => x.GetUserInfoAsync(expectedTargetUri, tokenValue)) + .ReturnsAsync(new GitHubUserInfo{Login = expectedUserName}); var provider = new GitHubHostProvider(context, ghApiMock.Object, ghAuthMock.Object); ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(Constants.OAuthTokenUserName, credential.UserName); + Assert.Equal(expectedUserName, credential.Account); Assert.Equal(tokenValue, credential.Password); ghAuthMock.Verify( @@ -426,25 +429,26 @@ public async Task GitHubHostProvider_GenerateCredentialAsync_Basic_1FAOnly_Retur }; var patValue = "PERSONAL-ACCESS-TOKEN"; - var pat = new GitCredential(Constants.PersonalAccessTokenUserName, patValue); - var response = new AuthenticationResult(GitHubAuthenticationResultType.Success, pat); + var response = new AuthenticationResult(GitHubAuthenticationResultType.Success, patValue); var context = new TestCommandContext(); var ghAuthMock = new Mock(MockBehavior.Strict); - ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, It.IsAny())) + ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, null, It.IsAny())) .ReturnsAsync(new AuthenticationPromptResult(new GitCredential(expectedUserName, expectedPassword))); var ghApiMock = new Mock(MockBehavior.Strict); ghApiMock.Setup(x => x.CreatePersonalAccessTokenAsync(expectedTargetUri, expectedUserName, expectedPassword, null, It.IsAny>())) .ReturnsAsync(response); + ghApiMock.Setup(x => x.GetUserInfoAsync(expectedTargetUri, patValue)) + .ReturnsAsync(new GitHubUserInfo{Login = expectedUserName}); var provider = new GitHubHostProvider(context, ghApiMock.Object, ghAuthMock.Object); ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(Constants.PersonalAccessTokenUserName, credential.UserName); + Assert.Equal(expectedUserName, credential.Account); Assert.Equal(patValue, credential.Password); ghApiMock.Verify( @@ -473,14 +477,13 @@ public async Task GitHubHostProvider_GenerateCredentialAsync_Basic_2FARequired_R }; var patValue = "PERSONAL-ACCESS-TOKEN"; - var pat = new GitCredential(Constants.PersonalAccessTokenUserName, patValue); var response1 = new AuthenticationResult(GitHubAuthenticationResultType.TwoFactorApp); - var response2 = new AuthenticationResult(GitHubAuthenticationResultType.Success, pat); + var response2 = new AuthenticationResult(GitHubAuthenticationResultType.Success, patValue); var context = new TestCommandContext(); var ghAuthMock = new Mock(MockBehavior.Strict); - ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, It.IsAny())) + ghAuthMock.Setup(x => x.GetAuthenticationAsync(expectedTargetUri, null, It.IsAny())) .ReturnsAsync(new AuthenticationPromptResult(new GitCredential(expectedUserName, expectedPassword))); ghAuthMock.Setup(x => x.GetTwoFactorCodeAsync(expectedTargetUri, false)) .ReturnsAsync(expectedAuthCode); @@ -490,13 +493,15 @@ public async Task GitHubHostProvider_GenerateCredentialAsync_Basic_2FARequired_R .ReturnsAsync(response1); ghApiMock.Setup(x => x.CreatePersonalAccessTokenAsync(expectedTargetUri, expectedUserName, expectedPassword, expectedAuthCode, It.IsAny>())) .ReturnsAsync(response2); + ghApiMock.Setup(x => x.GetUserInfoAsync(expectedTargetUri, patValue)) + .ReturnsAsync(new GitHubUserInfo{Login = expectedUserName}); var provider = new GitHubHostProvider(context, ghApiMock.Object, ghAuthMock.Object); ICredential credential = await provider.GenerateCredentialAsync(input); Assert.NotNull(credential); - Assert.Equal(Constants.PersonalAccessTokenUserName, credential.UserName); + Assert.Equal(expectedUserName, credential.Account); Assert.Equal(patValue, credential.Password); ghApiMock.Verify( diff --git a/src/shared/GitHub.Tests/GitHubRestApiTests.cs b/src/shared/GitHub.Tests/GitHubRestApiTests.cs index 4f15de893..57bb4b39c 100644 --- a/src/shared/GitHub.Tests/GitHubRestApiTests.cs +++ b/src/shared/GitHub.Tests/GitHubRestApiTests.cs @@ -1,14 +1,12 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. using System; -using System.Collections.Generic; using System.Linq; using System.Net; using System.Net.Http; using System.Net.Http.Headers; using System.Text; using System.Threading.Tasks; -using Microsoft.Git.CredentialManager; using Microsoft.Git.CredentialManager.Tests.Objects; using Xunit; @@ -90,7 +88,7 @@ public async Task GitHubRestApi_AcquireTokenAsync_ValidRequestOK_ReturnsToken() Assert.Equal(GitHubAuthenticationResultType.Success, authResult.Type); Assert.NotNull(authResult.Token); - Assert.Equal(expectedTokenValue, authResult.Token.Password); + Assert.Equal(expectedTokenValue, authResult.Token); } [Fact] @@ -167,7 +165,7 @@ public async Task GitHubRestApi_AcquireTokenAsync_ValidRequestCreated_ReturnsTok Assert.Equal(GitHubAuthenticationResultType.Success, authResult.Type); Assert.NotNull(authResult.Token); - Assert.Equal(expectedTokenValue, authResult.Token.Password); + Assert.Equal(expectedTokenValue, authResult.Token); } [Fact] @@ -267,7 +265,7 @@ public async Task GitHubRestApi_AcquireTokenAsync_ValidOAuthToken_ReturnsOAuthTo Assert.Equal(GitHubAuthenticationResultType.Success, authResult.Type); Assert.NotNull(authResult.Token); - Assert.Equal(testOAuthToken, authResult.Token.Password); + Assert.Equal(testOAuthToken, authResult.Token); } [Fact] diff --git a/src/shared/GitHub/AuthenticationResult.cs b/src/shared/GitHub/AuthenticationResult.cs index cc19a706f..6797d6cdc 100644 --- a/src/shared/GitHub/AuthenticationResult.cs +++ b/src/shared/GitHub/AuthenticationResult.cs @@ -12,7 +12,7 @@ public AuthenticationResult(GitHubAuthenticationResultType type) Token = null; } - public AuthenticationResult(GitHubAuthenticationResultType type, GitCredential token) + public AuthenticationResult(GitHubAuthenticationResultType type, string token) { Type = type; Token = token; @@ -20,7 +20,7 @@ public AuthenticationResult(GitHubAuthenticationResultType type, GitCredential t public GitHubAuthenticationResultType Type { get; } - public GitCredential Token { get; } + public string Token { get; } } public enum GitHubAuthenticationResultType diff --git a/src/shared/GitHub/GitHubAuthentication.cs b/src/shared/GitHub/GitHubAuthentication.cs index 311539e06..36566265e 100644 --- a/src/shared/GitHub/GitHubAuthentication.cs +++ b/src/shared/GitHub/GitHubAuthentication.cs @@ -14,7 +14,7 @@ namespace GitHub { public interface IGitHubAuthentication : IDisposable { - Task GetAuthenticationAsync(Uri targetUri, AuthenticationModes modes); + Task GetAuthenticationAsync(Uri targetUri, string userName, AuthenticationModes modes); Task GetTwoFactorCodeAsync(Uri targetUri, bool isSms); @@ -57,7 +57,7 @@ public class GitHubAuthentication : AuthenticationBase, IGitHubAuthentication public GitHubAuthentication(ICommandContext context) : base(context) {} - public async Task GetAuthenticationAsync(Uri targetUri, AuthenticationModes modes) + public async Task GetAuthenticationAsync(Uri targetUri, string userName, AuthenticationModes modes) { ThrowIfUserInteractionDisabled(); @@ -71,7 +71,8 @@ public async Task GetAuthenticationAsync(Uri targetU var promptArgs = new StringBuilder("prompt"); if ((modes & AuthenticationModes.Basic) != 0) promptArgs.Append(" --basic"); if ((modes & AuthenticationModes.OAuth) != 0) promptArgs.Append(" --oauth"); - if (!GitHubHostProvider.IsGitHubDotCom(targetUri)) promptArgs.AppendFormat(" --enterprise-url {0}", targetUri.ToString()); + if (!GitHubHostProvider.IsGitHubDotCom(targetUri)) promptArgs.AppendFormat(" --enterprise-url {0}", targetUri); + if (!string.IsNullOrWhiteSpace(userName)) promptArgs.AppendFormat("--username {0}", userName); IDictionary resultDict = await InvokeHelperAsync(helperPath, promptArgs.ToString(), null); @@ -86,7 +87,7 @@ public async Task GetAuthenticationAsync(Uri targetU return new AuthenticationPromptResult(AuthenticationModes.OAuth); case "basic": - if (!resultDict.TryGetValue("username", out string userName)) + if (!resultDict.TryGetValue("username", out userName)) { throw new Exception("Missing 'username' in response"); } @@ -125,7 +126,16 @@ public async Task GetAuthenticationAsync(Uri targetU case AuthenticationModes.Basic: Context.Terminal.WriteLine("Enter GitHub credentials for '{0}'...", targetUri); - string userName = Context.Terminal.Prompt("Username"); + + if (string.IsNullOrWhiteSpace(userName)) + { + userName = Context.Terminal.Prompt("Username"); + } + else + { + Context.Terminal.WriteLine("Username: {0}", userName); + } + string password = Context.Terminal.PromptSecret("Password"); return new AuthenticationPromptResult(new GitCredential(userName, password)); diff --git a/src/shared/GitHub/GitHubHostProvider.cs b/src/shared/GitHub/GitHubHostProvider.cs index f155c1323..8f17b1b1f 100644 --- a/src/shared/GitHub/GitHubHostProvider.cs +++ b/src/shared/GitHub/GitHubHostProvider.cs @@ -47,27 +47,32 @@ public GitHubHostProvider(ICommandContext context, IGitHubRestApi gitHubApi, IGi public override bool IsSupported(InputArguments input) { + if (input is null) + { + return false; + } + + // Split port number and hostname from host input argument + input.TryGetHostAndPort(out string hostName, out _); + // We do not support unencrypted HTTP communications to GitHub, // but we report `true` here for HTTP so that we can show a helpful // error message for the user in `CreateCredentialAsync`. - return input != null && - (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || + return (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "https")) && - (StringComparer.OrdinalIgnoreCase.Equals(input.Host, GitHubConstants.GitHubBaseUrlHost) || - StringComparer.OrdinalIgnoreCase.Equals(input.Host, GitHubConstants.GistBaseUrlHost)); + (StringComparer.OrdinalIgnoreCase.Equals(hostName, GitHubConstants.GitHubBaseUrlHost) || + StringComparer.OrdinalIgnoreCase.Equals(hostName, GitHubConstants.GistBaseUrlHost)); } - public override string GetCredentialKey(InputArguments input) + public override string GetServiceName(InputArguments input) { - string url = GetTargetUri(input).AbsoluteUri; + var baseUri = new Uri(base.GetServiceName(input)); - // Trim trailing slash - if (url.EndsWith("/")) - { - url = url.Substring(0, url.Length - 1); - } + // Normalise the URI + string url = NormalizeUri(baseUri).AbsoluteUri; - return $"git:{url}"; + // Trim trailing slash + return url.TrimEnd('/'); } public override async Task GenerateCredentialAsync(InputArguments input) @@ -80,16 +85,19 @@ public override async Task GenerateCredentialAsync(InputArguments i throw new Exception("Unencrypted HTTP is not supported for GitHub. Ensure the repository remote URL is using HTTPS."); } - Uri targetUri = GetTargetUri(input); + Uri remoteUri = input.GetRemoteUri(); + + string service = GetServiceName(input); - AuthenticationModes authModes = await GetSupportedAuthenticationModesAsync(targetUri); + AuthenticationModes authModes = await GetSupportedAuthenticationModesAsync(remoteUri); - AuthenticationPromptResult promptResult = await _gitHubAuth.GetAuthenticationAsync(targetUri, authModes); + AuthenticationPromptResult promptResult = await _gitHubAuth.GetAuthenticationAsync(remoteUri, input.UserName, authModes); switch (promptResult.AuthenticationMode) { case AuthenticationModes.Basic: - ICredential patCredential = await GeneratePersonalAccessTokenAsync(targetUri, promptResult.BasicCredential); + GitCredential patCredential = await GeneratePersonalAccessTokenAsync(remoteUri, promptResult.BasicCredential); + // HACK: Store the PAT immediately in case this PAT is not valid for SSO. // We don't know if this PAT is valid for SAML SSO and if it's not Git will fail // with a 403 and call neither 'store' or 'erase'. The user is expected to fiddle with @@ -97,38 +105,42 @@ public override async Task GenerateCredentialAsync(InputArguments i // We must store the PAT now so they can resume/repeat the operation with the same, // now SSO authorized, PAT. // See: https://github.com/microsoft/Git-Credential-Manager-Core/issues/133 - Context.CredentialStore.AddOrUpdate(GetCredentialKey(input), patCredential); + Context.CredentialStore.AddOrUpdate(service, patCredential.Account, patCredential.Password); return patCredential; case AuthenticationModes.OAuth: - return await GenerateOAuthCredentialAsync(targetUri); + return await GenerateOAuthCredentialAsync(remoteUri); default: throw new ArgumentOutOfRangeException(nameof(promptResult)); } } - private async Task GenerateOAuthCredentialAsync(Uri targetUri) + private async Task GenerateOAuthCredentialAsync(Uri targetUri) { OAuth2TokenResult result = await _gitHubAuth.GetOAuthTokenAsync(targetUri, GitHubOAuthScopes); - return new GitCredential(Constants.OAuthTokenUserName, result.AccessToken); + // Resolve the GitHub user handle + GitHubUserInfo userInfo = await _gitHubApi.GetUserInfoAsync(targetUri, result.AccessToken); + + return new GitCredential(userInfo.Login, result.AccessToken); } - private async Task GeneratePersonalAccessTokenAsync(Uri targetUri, ICredential credentials) + private async Task GeneratePersonalAccessTokenAsync(Uri targetUri, ICredential credentials) { AuthenticationResult result = await _gitHubApi.CreatePersonalTokenAsync( targetUri, credentials, null, GitHubCredentialScopes); + string token = null; + if (result.Type == GitHubAuthenticationResultType.Success) { Context.Trace.WriteLine($"Token acquisition for '{targetUri}' succeeded"); - return result.Token; + token = result.Token; } - - if (result.Type == GitHubAuthenticationResultType.TwoFactorApp || - result.Type == GitHubAuthenticationResultType.TwoFactorSms) + else if (result.Type == GitHubAuthenticationResultType.TwoFactorApp || + result.Type == GitHubAuthenticationResultType.TwoFactorSms) { bool isSms = result.Type == GitHubAuthenticationResultType.TwoFactorSms; @@ -140,10 +152,18 @@ private async Task GeneratePersonalAccessTokenAsync(Uri targetUri, { Context.Trace.WriteLine($"Token acquisition for '{targetUri}' succeeded."); - return result.Token; + token = result.Token; } } + if (token != null) + { + // Resolve the GitHub user handle + GitHubUserInfo userInfo = await _gitHubApi.GetUserInfoAsync(targetUri, token); + + return new GitCredential(userInfo.Login, token); + } + throw new Exception($"Interactive logon for '{targetUri}' failed."); } @@ -217,32 +237,21 @@ internal static bool IsGitHubDotCom(Uri targetUri) return StringComparer.OrdinalIgnoreCase.Equals(targetUri.Host, GitHubConstants.GitHubBaseUrlHost); } - private static Uri NormalizeUri(Uri targetUri) + private static Uri NormalizeUri(Uri uri) { - if (targetUri is null) + if (uri is null) { - throw new ArgumentNullException(nameof(targetUri)); + throw new ArgumentNullException(nameof(uri)); } // Special case for gist.github.com which are git backed repositories under the hood. // Credentials for these repositories are the same as the one stored with "github.com" - if (targetUri.DnsSafeHost.Equals(GitHubConstants.GistBaseUrlHost, StringComparison.OrdinalIgnoreCase)) + if (uri.DnsSafeHost.Equals(GitHubConstants.GistBaseUrlHost, StringComparison.OrdinalIgnoreCase)) { return new Uri("https://" + GitHubConstants.GitHubBaseUrlHost); } - return targetUri; - } - - private static Uri GetTargetUri(InputArguments input) - { - Uri uri = new UriBuilder - { - Scheme = input.Protocol, - Host = input.Host, - }.Uri; - - return NormalizeUri(uri); + return uri; } #endregion diff --git a/src/shared/GitHub/GitHubRestApi.cs b/src/shared/GitHub/GitHubRestApi.cs index 7be40d3b6..9e93ec318 100644 --- a/src/shared/GitHub/GitHubRestApi.cs +++ b/src/shared/GitHub/GitHubRestApi.cs @@ -145,8 +145,7 @@ private async Task ParseForbiddenResponseAsync(Uri targetU { _context.Trace.WriteLine($"Authentication success: user supplied personal access token for '{targetUri}'."); - return new AuthenticationResult(GitHubAuthenticationResultType.Success, - new GitCredential(Constants.PersonalAccessTokenUserName, password)); + return new AuthenticationResult(GitHubAuthenticationResultType.Success, password); } _context.Trace.WriteLine($"Authentication failed for '{targetUri}'."); @@ -182,7 +181,7 @@ private AuthenticationResult ParseUnauthorizedResponse(Uri targetUri, string aut private async Task ParseSuccessResponseAsync(Uri targetUri, HttpResponseMessage response) { - GitCredential token = null; + string token = null; string responseText = await response.Content.ReadAsStringAsync(); Match tokenMatch; @@ -190,8 +189,7 @@ private async Task ParseSuccessResponseAsync(Uri targetUri RegexOptions.CultureInvariant | RegexOptions.IgnoreCase)).Success && tokenMatch.Groups.Count > 1) { - string tokenText = tokenMatch.Groups[1].Value; - token = new GitCredential(Constants.PersonalAccessTokenUserName, tokenText); + token = tokenMatch.Groups[1].Value; } if (token == null) @@ -289,7 +287,7 @@ public static Task CreatePersonalTokenAsync( { return api.CreatePersonalAccessTokenAsync( targetUri, - credentials?.UserName, + credentials?.Account, credentials?.Password, authenticationCode, scopes); diff --git a/src/windows/GitHub.UI.Windows/AuthenticationPrompts.cs b/src/windows/GitHub.UI.Windows/AuthenticationPrompts.cs index 3a5086ac0..cf8f69e74 100644 --- a/src/windows/GitHub.UI.Windows/AuthenticationPrompts.cs +++ b/src/windows/GitHub.UI.Windows/AuthenticationPrompts.cs @@ -16,14 +16,14 @@ public AuthenticationPrompts(IGui gui) private readonly IGui _gui; - public CredentialPromptResult ShowCredentialPrompt(string enterpriseUrl, bool showBasic, bool showOAuth, out string username, out string password) + public CredentialPromptResult ShowCredentialPrompt(string enterpriseUrl, bool showBasic, bool showOAuth, ref string username, out string password) { - username = null; password = null; var viewModel = new LoginCredentialsViewModel(showBasic, showOAuth) { - GitHubEnterpriseUrl = enterpriseUrl + GitHubEnterpriseUrl = enterpriseUrl, + UsernameOrEmail = username }; bool valid = _gui.ShowDialogWindow(viewModel, () => new LoginCredentialsView()); diff --git a/src/windows/GitHub.UI.Windows/Program.cs b/src/windows/GitHub.UI.Windows/Program.cs index ff1ef77a8..3bc2b91f7 100644 --- a/src/windows/GitHub.UI.Windows/Program.cs +++ b/src/windows/GitHub.UI.Windows/Program.cs @@ -31,6 +31,7 @@ public static void Main(string[] args) string enterpriseUrl = CommandLineUtils.GetParameter(args, "--enterprise-url"); bool basic = CommandLineUtils.TryGetSwitch(args, "--basic"); bool oauth = CommandLineUtils.TryGetSwitch(args, "--oauth"); + string username = CommandLineUtils.GetParameter(args, "--username"); if (!basic && !oauth) { @@ -39,7 +40,7 @@ public static void Main(string[] args) var result = prompts.ShowCredentialPrompt( enterpriseUrl, basic, oauth, - out string username, + ref username, out string password); switch (result) From fada97321bf071ca08177de0b534522ff23c4fc1 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 15:16:35 +0100 Subject: [PATCH 12/16] azrepos: update azrepos to new credential model/APIs Update the Azure Repos provider to support the new credential storage API/model, as well as support remote URLs with explicit port numbers. --- .../UriHelpersTests.cs | 30 +++++++++++++++++++ .../AzureReposHostProvider.cs | 16 +++++++--- src/shared/Microsoft.AzureRepos/UriHelpers.cs | 11 +++++-- 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs b/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs index 0b4fc42fb..c5346c814 100644 --- a/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs +++ b/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs @@ -81,6 +81,36 @@ public void UriHelpers_CreateOrganizationUri_AzureHost_ReturnsCorrectUri() Assert.Equal(expected, actual); } + [Fact] + public void UriHelpers_CreateOrganizationUri_AzureHost_WithPort_ReturnsCorrectUri() + { + var expected = new Uri("https://dev.azure.com:456/myorg"); + var input = new InputArguments(new Dictionary + { + ["protocol"] = "https", + ["host"] = "dev.azure.com:456", + ["path"] = "myorg/myproject/_git/myrepo" + }); + + Uri actual = UriHelpers.CreateOrganizationUri(input); + + Assert.Equal(expected, actual); + } + + [Fact] + public void UriHelpers_CreateOrganizationUri_AzureHost_WithBadPort_ThrowsException() + { + var expected = new Uri("https://dev.azure.com:456/myorg"); + var input = new InputArguments(new Dictionary + { + ["protocol"] = "https", + ["host"] = "dev.azure.com:not-a-port", + ["path"] = "myorg/myproject/_git/myrepo" + }); + + Assert.Throws(() => UriHelpers.CreateOrganizationUri(input)); + } + [Fact] public void UriHelpers_CreateOrganizationUri_AzureHost_OrgAlsoInUser_PrefersPathOrg() { diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs index 37590ddd4..a031e7063 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs @@ -41,17 +41,25 @@ public AzureReposHostProvider(ICommandContext context, IAzureDevOpsRestApi azDev public override bool IsSupported(InputArguments input) { + if (input is null) + { + return false; + } + + // Split port number and hostname from host input argument + input.TryGetHostAndPort(out string hostName, out _); + // We do not support unencrypted HTTP communications to Azure Repos, // but we report `true` here for HTTP so that we can show a helpful // error message for the user in `CreateCredentialAsync`. return (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "https")) && - UriHelpers.IsAzureDevOpsHost(input.Host); + UriHelpers.IsAzureDevOpsHost(hostName); } - public override string GetCredentialKey(InputArguments input) + public override string GetServiceName(InputArguments input) { - return $"git:{UriHelpers.CreateOrganizationUri(input).AbsoluteUri}"; + return UriHelpers.CreateOrganizationUri(input).AbsoluteUri.TrimEnd('/'); } public override async Task GenerateCredentialAsync(InputArguments input) @@ -97,7 +105,7 @@ public override async Task GenerateCredentialAsync(InputArguments i patScopes); Context.Trace.WriteLineSecrets("PAT created. PAT='{0}'", new object[] {pat}); - return new GitCredential(Constants.PersonalAccessTokenUserName, pat); + return new GitCredential(atUser, pat); } protected override void ReleaseManagedResources() diff --git a/src/shared/Microsoft.AzureRepos/UriHelpers.cs b/src/shared/Microsoft.AzureRepos/UriHelpers.cs index 3d74629ed..01332ffe2 100644 --- a/src/shared/Microsoft.AzureRepos/UriHelpers.cs +++ b/src/shared/Microsoft.AzureRepos/UriHelpers.cs @@ -76,7 +76,12 @@ public static Uri CreateOrganizationUri(InputArguments input) throw new InvalidOperationException("Input arguments must include host"); } - if (!IsAzureDevOpsHost(input.Host)) + if (!input.TryGetHostAndPort(out string hostName, out _)) + { + throw new InvalidOperationException("Host name and/or port is invalid"); + } + + if (!IsAzureDevOpsHost(hostName)) { throw new InvalidOperationException("Host is not Azure DevOps"); } @@ -84,12 +89,12 @@ public static Uri CreateOrganizationUri(InputArguments input) var ub = new UriBuilder { Scheme = input.Protocol, - Host = input.Host, + Host = hostName, }; // Extract the organization name for Azure ('dev.azure.com') style URLs. // The older *.visualstudio.com URLs contained the organization name in the host already. - if (StringComparer.OrdinalIgnoreCase.Equals(input.Host, AzureDevOpsConstants.AzureDevOpsHost)) + if (StringComparer.OrdinalIgnoreCase.Equals(hostName, AzureDevOpsConstants.AzureDevOpsHost)) { // dev.azure.com/{org} string[] pathParts = input.Path?.Split(new[] {'/'}, StringSplitOptions.RemoveEmptyEntries); From 05b04bae74fbf0135b792efc69ac1edf5961238d Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 2 Sep 2020 14:45:25 +0100 Subject: [PATCH 13/16] azrepos: workaround org@ user hackery 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. --- .../UriHelpersTests.cs | 36 ++++ .../AzureReposHostProvider.cs | 184 +++++++++++++++--- src/shared/Microsoft.AzureRepos/UriHelpers.cs | 97 +++++++-- 3 files changed, 276 insertions(+), 41 deletions(-) diff --git a/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs b/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs index c5346c814..7fb2839e2 100644 --- a/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs +++ b/src/shared/Microsoft.AzureRepos.Tests/UriHelpersTests.cs @@ -37,6 +37,42 @@ public void UriHelpers_IsAzureDevOpsHost(string host, bool expected) Assert.Equal(expected, UriHelpers.IsAzureDevOpsHost(host)); } + [Theory] + [InlineData("dev.azure.com", true)] + [InlineData("myorg.visualstudio.com", false)] + [InlineData("vs-ssh.myorg.visualstudio.com", false)] + [InlineData("DEV.AZURE.COM", true)] + [InlineData("MYORG.VISUALSTUDIO.COM", false)] + [InlineData(null, false)] + [InlineData("", false)] + [InlineData(" ", false)] + [InlineData("testdev.azure.com", false)] + [InlineData("test.dev.azure.com", false)] + [InlineData("visualstudio.com", false)] + [InlineData("testvisualstudio.com", false)] + public void UriHelpers_IsDevAzureComHost(string host, bool expected) + { + Assert.Equal(expected, UriHelpers.IsDevAzureComHost(host)); + } + + [Theory] + [InlineData("dev.azure.com", false)] + [InlineData("myorg.visualstudio.com", true)] + [InlineData("vs-ssh.myorg.visualstudio.com", true)] + [InlineData("DEV.AZURE.COM", false)] + [InlineData("MYORG.VISUALSTUDIO.COM", true)] + [InlineData(null, false)] + [InlineData("", false)] + [InlineData(" ", false)] + [InlineData("testdev.azure.com", false)] + [InlineData("test.dev.azure.com", false)] + [InlineData("visualstudio.com", false)] + [InlineData("testvisualstudio.com", false)] + public void UriHelpers_IsVisualStudioComHost(string host, bool expected) + { + Assert.Equal(expected, UriHelpers.IsVisualStudioComHost(host)); + } + [Fact] public void UriHelpers_CreateOrganizationUri_Null_ThrowsException() { diff --git a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs index a031e7063..bfc2ab1c3 100644 --- a/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs +++ b/src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs @@ -10,8 +10,9 @@ namespace Microsoft.AzureRepos { - public class AzureReposHostProvider : HostProvider, IConfigurableComponent + public class AzureReposHostProvider : DisposableObject, IHostProvider, IConfigurableComponent { + private readonly ICommandContext _context; private readonly IAzureDevOpsRestApi _azDevOps; private readonly IMicrosoftAuthentication _msAuth; @@ -22,47 +23,107 @@ public AzureReposHostProvider(ICommandContext context) public AzureReposHostProvider(ICommandContext context, IAzureDevOpsRestApi azDevOps, IMicrosoftAuthentication msAuth) - : base(context) { + EnsureArgument.NotNull(context, nameof(context)); EnsureArgument.NotNull(azDevOps, nameof(azDevOps)); EnsureArgument.NotNull(msAuth, nameof(msAuth)); + _context = context; _azDevOps = azDevOps; _msAuth = msAuth; } - #region HostProvider + #region IHostProvider - public override string Id => "azure-repos"; + public string Id => "azure-repos"; - public override string Name => "Azure Repos"; + public string Name => "Azure Repos"; - public override IEnumerable SupportedAuthorityIds => MicrosoftAuthentication.AuthorityIds; + public IEnumerable SupportedAuthorityIds => MicrosoftAuthentication.AuthorityIds; - public override bool IsSupported(InputArguments input) + public bool IsSupported(InputArguments input) { if (input is null) { return false; } - // Split port number and hostname from host input argument - input.TryGetHostAndPort(out string hostName, out _); - // We do not support unencrypted HTTP communications to Azure Repos, // but we report `true` here for HTTP so that we can show a helpful // error message for the user in `CreateCredentialAsync`. - return (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || - StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "https")) && + return input.TryGetHostAndPort(out string hostName, out _) + && (StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "http") || + StringComparer.OrdinalIgnoreCase.Equals(input.Protocol, "https")) && UriHelpers.IsAzureDevOpsHost(hostName); } - public override string GetServiceName(InputArguments input) + public async Task GetCredentialAsync(InputArguments input) + { + string service = GetServiceName(input); + string account = GetAccountNameForCredentialQuery(input); + + _context.Trace.WriteLine($"Looking for existing credential in store with service={service} account={account}..."); + + ICredential credential = _context.CredentialStore.Get(service, account); + if (credential == null) + { + _context.Trace.WriteLine("No existing credentials found."); + + // No existing credential was found, create a new one + _context.Trace.WriteLine("Creating new credential..."); + credential = await GenerateCredentialAsync(input); + _context.Trace.WriteLine("Credential created."); + } + else + { + _context.Trace.WriteLine("Existing credential found."); + } + + return credential; + } + + public virtual Task StoreCredentialAsync(InputArguments input) + { + string service = GetServiceName(input); + + // We always store credentials against the given username argument for + // both vs.com and dev.azure.com-style URLs. + string account = input.UserName; + + // Add or update the credential in the store. + _context.Trace.WriteLine($"Storing credential with service={service} account={account}..."); + _context.CredentialStore.AddOrUpdate(service, account, input.Password); + _context.Trace.WriteLine("Credential was successfully stored."); + + return Task.CompletedTask; + } + + public Task EraseCredentialAsync(InputArguments input) + { + string service = GetServiceName(input); + string account = GetAccountNameForCredentialQuery(input); + + // Try to locate an existing credential + _context.Trace.WriteLine($"Erasing stored credential in store with service={service} account={account}..."); + if (_context.CredentialStore.Remove(service, account)) + { + _context.Trace.WriteLine("Credential was successfully erased."); + } + else + { + _context.Trace.WriteLine("No credential was erased."); + } + + return Task.CompletedTask; + } + + protected override void ReleaseManagedResources() { - return UriHelpers.CreateOrganizationUri(input).AbsoluteUri.TrimEnd('/'); + _azDevOps.Dispose(); + base.ReleaseManagedResources(); } - public override async Task GenerateCredentialAsync(InputArguments input) + private async Task GenerateCredentialAsync(InputArguments input) { ThrowIfDisposed(); @@ -76,12 +137,12 @@ public override async Task GenerateCredentialAsync(InputArguments i Uri remoteUri = input.GetRemoteUri(); // Determine the MS authentication authority for this organization - Context.Trace.WriteLine("Determining Microsoft Authentication Authority..."); + _context.Trace.WriteLine("Determining Microsoft Authentication Authority..."); string authAuthority = await _azDevOps.GetAuthorityAsync(orgUri); - Context.Trace.WriteLine($"Authority is '{authAuthority}'."); + _context.Trace.WriteLine($"Authority is '{authAuthority}'."); // Get an AAD access token for the Azure DevOps SPS - Context.Trace.WriteLine("Getting Azure AD access token..."); + _context.Trace.WriteLine("Getting Azure AD access token..."); JsonWebToken accessToken = await _msAuth.GetAccessTokenAsync( authAuthority, AzureDevOpsConstants.AadClientId, @@ -90,7 +151,7 @@ public override async Task GenerateCredentialAsync(InputArguments i remoteUri, null); string atUser = accessToken.GetAzureUserName(); - Context.Trace.WriteLineSecrets($"Acquired Azure access token. User='{atUser}' Token='{{0}}'", new object[] {accessToken.EncodedToken}); + _context.Trace.WriteLineSecrets($"Acquired Azure access token. User='{atUser}' Token='{{0}}'", new object[] {accessToken.EncodedToken}); // Ask the Azure DevOps instance to create a new PAT var patScopes = new[] @@ -98,20 +159,89 @@ public override async Task GenerateCredentialAsync(InputArguments i AzureDevOpsConstants.PersonalAccessTokenScopes.ReposWrite, AzureDevOpsConstants.PersonalAccessTokenScopes.ArtifactsRead }; - Context.Trace.WriteLine($"Creating Azure DevOps PAT with scopes '{string.Join(", ", patScopes)}'..."); + _context.Trace.WriteLine($"Creating Azure DevOps PAT with scopes '{string.Join(", ", patScopes)}'..."); string pat = await _azDevOps.CreatePersonalAccessTokenAsync( orgUri, accessToken, patScopes); - Context.Trace.WriteLineSecrets("PAT created. PAT='{0}'", new object[] {pat}); + _context.Trace.WriteLineSecrets("PAT created. PAT='{0}'", new object[] {pat}); return new GitCredential(atUser, pat); } - protected override void ReleaseManagedResources() + /// + /// For dev.azure.com-style URLs we use the path arg to get the Azure DevOps organization name. + /// We ensure the presence of the path arg by setting credential.useHttpPath = true at install time. + /// + /// The result of this workaround is that we are now unable to determine if the user wanted to store + /// credentials with the full path or not for dev.azure.com-style URLs. + /// + /// Rather than always assume we're storing credentials against the full path, and therefore resulting + /// in an personal access token being created per remote URL/repository, we never store against + /// the full path and always store with the organization URL "dev.azure.com/org". + /// + /// For visualstudio.com-style URLs we know the AzDevOps organization name from the host arg, and + /// don't set the useHttpPath option. This means if we get the full path for a vs.com-style URL + /// we can store against the full remote path (the intended design). + /// + /// Users that need to clone a repository from Azure Repos against the full path therefore must + /// use the vs.com-style remote URL and not the dev.azure.com one. + /// + private static string GetServiceName(InputArguments input) { - _azDevOps.Dispose(); - base.ReleaseManagedResources(); + if (!input.TryGetHostAndPort(out string hostName, out _)) + { + throw new InvalidOperationException("Failed to parse host name and/or port"); + } + + // dev.azure.com + if (UriHelpers.IsDevAzureComHost(hostName)) + { + // We can never store the new dev.azure.com-style URLs against the full path because + // we have forced the useHttpPath option to true to in order to retrieve the AzDevOps + // organization name from Git. + return UriHelpers.CreateOrganizationUri(input).AbsoluteUri.TrimEnd('/'); + } + + // *.visualstudio.com + if (UriHelpers.IsVisualStudioComHost(hostName)) + { + // If we're given the full path for an older *.visualstudio.com-style URL then we should + // respect that in the service name. + return input.GetRemoteUri().AbsoluteUri.TrimEnd('/'); + } + + throw new InvalidOperationException("Host is not Azure DevOps."); + } + + private static string GetAccountNameForCredentialQuery(InputArguments input) + { + if (!input.TryGetHostAndPort(out string hostName, out _)) + { + throw new InvalidOperationException("Failed to parse host name and/or port"); + } + + // dev.azure.com + if (UriHelpers.IsDevAzureComHost(hostName)) + { + // We ignore the given username for dev.azure.com-style URLs because AzDevOps recommends + // adding the organization name as the user in the remote URL (resulting in URLs like + // https://org@dev.azure.com/org/foo/_git/bar) and we don't know if the given username + // is an actual username, or the org name. + // Use `null` as the account name so we match all possible credentials (regardless of + // the account). + return null; + } + + // *.visualstudio.com + if (UriHelpers.IsVisualStudioComHost(hostName)) + { + // If we're given a username for the vs.com-style URLs we can and should respect any + // specified username in the remote URL/input arguments. + return input.UserName; + } + + throw new InvalidOperationException("Host is not Azure DevOps."); } #endregion @@ -130,11 +260,11 @@ public Task ConfigureAsync( if (targetConfig.TryGetValue(useHttpPathKey, out string currentValue) && currentValue.IsTruthy()) { - Context.Trace.WriteLine("Git configuration 'credential.useHttpPath' is already set to 'true' for https://dev.azure.com."); + _context.Trace.WriteLine("Git configuration 'credential.useHttpPath' is already set to 'true' for https://dev.azure.com."); } else { - Context.Trace.WriteLine("Setting Git configuration 'credential.useHttpPath' to 'true' for https://dev.azure.com..."); + _context.Trace.WriteLine("Setting Git configuration 'credential.useHttpPath' to 'true' for https://dev.azure.com..."); targetConfig.SetValue(useHttpPathKey, "true"); } @@ -147,7 +277,7 @@ public Task UnconfigureAsync( { string useHttpPathKey = $"{KnownGitCfg.Credential.SectionName}.https://dev.azure.com.{KnownGitCfg.Credential.UseHttpPath}"; - Context.Trace.WriteLine("Clearing Git configuration 'credential.useHttpPath' for https://dev.azure.com..."); + _context.Trace.WriteLine("Clearing Git configuration 'credential.useHttpPath' for https://dev.azure.com..."); IGitConfiguration targetConfig = git.GetConfiguration(configurationLevel); targetConfig.Unset(useHttpPathKey); diff --git a/src/shared/Microsoft.AzureRepos/UriHelpers.cs b/src/shared/Microsoft.AzureRepos/UriHelpers.cs index 01332ffe2..453a7b49c 100644 --- a/src/shared/Microsoft.AzureRepos/UriHelpers.cs +++ b/src/shared/Microsoft.AzureRepos/UriHelpers.cs @@ -34,6 +34,62 @@ public static string CombinePath(string basePath, string path) return basePath + path; } + /// + /// Check if the hostname is the legacy Azure DevOps hostname (*.visualstudio.com). + /// + /// Git query arguments. + /// True if the hostname is the legacy Azure DevOps host, false otherwise. + public static bool IsVisualStudioComHost(InputArguments input) + { + EnsureArgument.NotNull(input, nameof(input)); + + if (!input.TryGetHostAndPort(out string hostName, out _)) + { + throw new InvalidOperationException("Host name and/or port is invalid."); + } + + return IsVisualStudioComHost(hostName); + } + + /// + /// Check if the hostname is the legacy Azure DevOps hostname (*.visualstudio.com). + /// + /// Hostname to check. + /// True if the hostname is the legacy Azure DevOps host, false otherwise. + public static bool IsVisualStudioComHost(string host) + { + return host != null && + host.EndsWith(AzureDevOpsConstants.VstsHostSuffix, StringComparison.OrdinalIgnoreCase); + } + + /// + /// Check if the hostname is the new Azure DevOps hostname (dev.azure.com). + /// + /// Git query arguments. + /// True if the hostname is the new Azure DevOps host, false otherwise. + public static bool IsDevAzureComHost(InputArguments input) + { + EnsureArgument.NotNull(input, nameof(input)); + + if (!input.TryGetHostAndPort(out string hostName, out _)) + { + throw new InvalidOperationException("Host name and/or port is invalid."); + } + + return IsDevAzureComHost(hostName); + } + + /// + /// Check if the hostname is the new Azure DevOps hostname (dev.azure.com). + /// + /// Hostname to check. + /// True if the hostname is the new Azure DevOps host, false otherwise. + public static bool IsDevAzureComHost(string host) + { + return host != null && + StringComparer.OrdinalIgnoreCase.Equals(host, AzureDevOpsConstants.AzureDevOpsHost); + } + /// /// Check if the hostname is a valid Azure DevOps hostname (dev.azure.com or *.visualstudio.com). /// @@ -41,9 +97,7 @@ public static string CombinePath(string basePath, string path) /// True if the hostname is Azure DevOps, false otherwise. public static bool IsAzureDevOpsHost(string host) { - return host != null && - (StringComparer.OrdinalIgnoreCase.Equals(host, AzureDevOpsConstants.AzureDevOpsHost) || - host.EndsWith(AzureDevOpsConstants.VstsHostSuffix, StringComparison.OrdinalIgnoreCase)); + return IsVisualStudioComHost(host) || IsDevAzureComHost(host); } /// @@ -68,22 +122,22 @@ public static Uri CreateOrganizationUri(InputArguments input) if (string.IsNullOrWhiteSpace(input.Protocol)) { - throw new InvalidOperationException("Input arguments must include protocol"); + throw new InvalidOperationException("Input arguments must include protocol."); } if (string.IsNullOrWhiteSpace(input.Host)) { - throw new InvalidOperationException("Input arguments must include host"); + throw new InvalidOperationException("Input arguments must include host."); } - if (!input.TryGetHostAndPort(out string hostName, out _)) + if (!input.TryGetHostAndPort(out string hostName, out int? port)) { - throw new InvalidOperationException("Host name and/or port is invalid"); + throw new InvalidOperationException("Host name and/or port is invalid."); } if (!IsAzureDevOpsHost(hostName)) { - throw new InvalidOperationException("Host is not Azure DevOps"); + throw new InvalidOperationException("Host is not Azure DevOps."); } var ub = new UriBuilder @@ -92,17 +146,21 @@ public static Uri CreateOrganizationUri(InputArguments input) Host = hostName, }; + if (port.HasValue) + { + ub.Port = port.Value; + } + // Extract the organization name for Azure ('dev.azure.com') style URLs. // The older *.visualstudio.com URLs contained the organization name in the host already. - if (StringComparer.OrdinalIgnoreCase.Equals(hostName, AzureDevOpsConstants.AzureDevOpsHost)) + if (IsDevAzureComHost(hostName)) { - // dev.azure.com/{org} - string[] pathParts = input.Path?.Split(new[] {'/'}, StringSplitOptions.RemoveEmptyEntries); - if (pathParts?.Length > 0) + // Prefer getting the org name from the path: dev.azure.com/{org} + if (GetFirstPathComponent(input.Path) is string orgName) { - ub.Path = pathParts[0]; + ub.Path = orgName; } - // {org}@dev.azure.com + // Failing that try using the username: {org}@dev.azure.com else if (!string.IsNullOrWhiteSpace(input.UserName)) { ub.Path = input.UserName; @@ -119,5 +177,16 @@ public static Uri CreateOrganizationUri(InputArguments input) return ub.Uri; } + + public static string GetFirstPathComponent(string path) + { + string[] parts = path?.Split(new[] {'/'}, StringSplitOptions.RemoveEmptyEntries); + if (parts?.Length > 0) + { + return parts[0]; + } + + return null; + } } } From 43e0f4d0bed7c18c9ad6b2b6462d4149834a0145 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 1 Sep 2020 18:31:26 +0100 Subject: [PATCH 14/16] credstore: add support for custom cred namespaces 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. --- docs/configuration.md | 17 ++++++++++++++ docs/environment.md | 23 +++++++++++++++++++ .../CommandContext.cs | 6 ++--- .../Constants.cs | 5 ++-- .../Settings.cs | 15 +++++++++++- .../Objects/TestSettings.cs | 4 ++++ 6 files changed, 64 insertions(+), 6 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 432ac4571..f3e5cdba6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -168,3 +168,20 @@ git config --global credential.gitHubAuthModes "oauth basic" ``` **Also see: [GCM_GITHUB_AUTHMODES](environment.md#GCM_GITHUB_AUTHMODES)** + +--- + +### credential.namespace + +Use a custom namespace prefix for credentials read and written in the OS credential store. +Credentials will be stored in the format `{namespace}:{service}`. + +Defaults to the value `git`. + +#### Example + +```shell +git config --global credential.namespace "my-namespace" +``` + +**Also see: [GCM_NAMESPACE](environment.md#GCM_NAMESPACE)** diff --git a/docs/environment.md b/docs/environment.md index 7958235cc..c0f000528 100644 --- a/docs/environment.md +++ b/docs/environment.md @@ -309,3 +309,26 @@ export GCM_GITHUB_AUTHMODES="oauth basic" ``` **Also see: [credential.gitHubAuthModes](configuration.md#credentialgitHubAuthModes)** + +--- + +### GCM_NAMESPACE + +Use a custom namespace prefix for credentials read and written in the OS credential store. +Credentials will be stored in the format `{namespace}:{service}`. + +Defaults to the value `git`. + +##### Windows + +```batch +SET GCM_NAMESPACE="my-namespace" +``` + +##### macOS/Linux + +```bash +export GCM_NAMESPACE="my-namespace" +``` + +**Also see: [credential.namespace](configuration.md#credentialnamespace)** diff --git a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs index a7b0ee3a2..b6d9c8256 100644 --- a/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs +++ b/src/shared/Microsoft.Git.CredentialManager/CommandContext.cs @@ -92,7 +92,7 @@ public CommandContext() FileSystem.GetCurrentDirectory() ); Settings = new Settings(Environment, Git); - CredentialStore = WindowsCredentialManager.Open(Constants.CredentialNamespace); + CredentialStore = WindowsCredentialManager.Open(Settings.CredentialNamespace); } else if (PlatformUtils.IsMacOS()) { @@ -107,7 +107,7 @@ public CommandContext() FileSystem.GetCurrentDirectory() ); Settings = new Settings(Environment, Git); - CredentialStore = MacOSKeychain.Open(Constants.CredentialNamespace); + CredentialStore = MacOSKeychain.Open(Settings.CredentialNamespace); } else if (PlatformUtils.IsLinux()) { @@ -123,7 +123,7 @@ public CommandContext() FileSystem.GetCurrentDirectory() ); Settings = new Settings(Environment, Git); - CredentialStore = new LinuxCredentialStore(Settings, Git, Constants.CredentialNamespace); + CredentialStore = new LinuxCredentialStore(Settings, Git, Settings.CredentialNamespace); } else { diff --git a/src/shared/Microsoft.Git.CredentialManager/Constants.cs b/src/shared/Microsoft.Git.CredentialManager/Constants.cs index e4ba7199f..2156d5dff 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Constants.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Constants.cs @@ -8,9 +8,8 @@ namespace Microsoft.Git.CredentialManager public static class Constants { public const string PersonalAccessTokenUserName = "PersonalAccessToken"; - public const string OAuthTokenUserName = "OAuthToken"; public const string DefaultMsAuthHelper = "Microsoft.Authentication.Helper"; - public const string CredentialNamespace = "git"; + public const string DefaultCredentialNamespace = "git"; public const string ProviderIdAuto = "auto"; public const string AuthorityIdAuto = "auto"; @@ -51,6 +50,7 @@ public static class EnvironmentVariables public const string GcmInteractive = "GCM_INTERACTIVE"; public const string GcmParentWindow = "GCM_MODAL_PARENTHWND"; public const string MsAuthHelper = "GCM_MSAUTH_HELPER"; + public const string GcmCredNamespace = "GCM_NAMESPACE"; } public static class Http @@ -77,6 +77,7 @@ public static class Credential public const string UseHttpPath = "useHttpPath"; public const string Interactive = "interactive"; public const string MsAuthHelper = "msauthHelper"; + public const string CredNamespace = "namespace"; } public static class Http diff --git a/src/shared/Microsoft.Git.CredentialManager/Settings.cs b/src/shared/Microsoft.Git.CredentialManager/Settings.cs index 4e911a0e1..615950c4a 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Settings.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Settings.cs @@ -109,6 +109,12 @@ public interface ISettings : IDisposable /// /// This value is platform specific. string ParentWindowId { get; } + + /// + /// Credential storage namespace prefix. + /// + /// The default value is "git" if unset. + string CredentialNamespace { get; } } public class Settings : ISettings @@ -406,7 +412,14 @@ bool TryGetUriSetting(string envarName, string section, string property, out Uri return null; } - public string ParentWindowId => _environment.Variables.TryGetValue(Constants.EnvironmentVariables.GcmParentWindow, out string parentWindowId) ? parentWindowId : null; + public string ParentWindowId => _environment.Variables.TryGetValue(KnownEnvars.GcmParentWindow, out string parentWindowId) ? parentWindowId : null; + + public string CredentialNamespace => + TryGetSetting(KnownEnvars.GcmCredNamespace, + KnownGitCfg.Credential.SectionName, KnownGitCfg.Credential.CredNamespace, + out string @namespace) + ? @namespace + : Constants.DefaultCredentialNamespace; #region IDisposable diff --git a/src/shared/TestInfrastructure/Objects/TestSettings.cs b/src/shared/TestInfrastructure/Objects/TestSettings.cs index 62b3ec36a..a195374fc 100644 --- a/src/shared/TestInfrastructure/Objects/TestSettings.cs +++ b/src/shared/TestInfrastructure/Objects/TestSettings.cs @@ -37,6 +37,8 @@ public class TestSettings : ISettings public string ParentWindowId { get; set; } + public string CredentialNamespace { get; set; } = "git-test"; + #region ISettings public bool TryGetSetting(string envarName, string section, string property, out string value) @@ -115,6 +117,8 @@ Uri ISettings.GetProxyConfiguration(out bool isDeprecatedConfiguration) string ISettings.ParentWindowId => ParentWindowId; + string ISettings.CredentialNamespace => CredentialNamespace; + #endregion #region IDisposable From 2e631dffb041af5cbefacf821debbf29b4012815 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Fri, 4 Sep 2020 11:26:32 +0100 Subject: [PATCH 15/16] docs: update documentation to reflect changes 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. --- docs/architecture.md | 20 +++++++++++++------- docs/hostprovider.md | 40 ++++++++++++++++++++++++---------------- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 9c33ac348..69da1fd4e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -196,19 +196,25 @@ directly implement the interface they can also derive from the `HostProvider` abstract class (which itself implements the `IHostProvider` interface). The `HostProvider` abstract class implements the -`Get|Store|EraseCredentialAsync` methods and instead has a -`GenerateCredentialAsync` and `GetCredentialKey` abstract methods. Calls to -`get`, `store`, or `erase` result in first a call to `GetCredentialKey` which -should return a stable and unique "key" for the request. This forms the key for -any stored credential in the credential store. During a `get` operation the -credential store is queried for an existing credential with the computed key. +`Get|Store|EraseCredentialAsync` methods and instead has the +`GenerateCredentialAsync` abstract method, and the `GetServiceName` virtual +method. Calls to `get`, `store`, or `erase` result in first a call to +`GetServiceName` which should return a stable and unique value for the provider +and request. This value forms part of the attributes associated with any stored +credential in the credential store. During a `get` operation the +credential store is queried for an existing credential with such service name. If a credential is found it is returned immediately. Similarly, calls to `store` and `erase` are handles automatically to store credentials against, and erase -credentials matching the computed key. Methods are implemented as `virtual` +credentials matching the service name. Methods are implemented as `virtual` meaning you can always override this behaviour, for example to clear other custom caches on an `erase` request, without having to reimplement the lookup/store credential logic. +The default implementation of `GetServiceName` is usually sufficient for most +providers. It returns the computed remote URL (without a trailing slash) from +the input arguments from Git - `://[/]` - no username is +included even if present. + Host providers are queried in turn (registration order) via the `IHostProvider.IsSupported` method and passed the input received from Git. If the provider recognises the request, for example by a matching known host name, diff --git a/docs/hostprovider.md b/docs/hostprovider.md index d370a4bd2..aca957c83 100644 --- a/docs/hostprovider.md +++ b/docs/hostprovider.md @@ -3,8 +3,13 @@ Property|Value -|- Author(s)|Matthew John Cheetham ([@mjcheetham](https://github.com/mjcheetham)) -Revision|1.0 -Last updated|2020-06-22 +Revision|1.1 +Last updated|2020-09-04 + +## Revision Summary + +- 1.0. Initial revision. +- 1.1. Replaced `GetCredentialKey` with `GetServiceName`. ## Abstract @@ -28,7 +33,7 @@ authentication to secured Git repositories by implementing and registering a - [2.4. Storing Credentials](#24-storing-credentials) - [2.5. Erasing Credentials](#25-erasing-credentials) - [2.6 `HostProvider` base class](#26-hostprovider-base-class) - - [2.6.1 `GetCredentialKey`](#261-getcredentialkey) + - [2.6.1 `GetServiceName`](#261-getservicename) - [2.6.2 `GenerateCredentialAsync`](#262-generatecredentialasync) - [2.7. External Metadata](#27-external-metadata) - [3. Helpers](#3-helpers) @@ -222,7 +227,7 @@ Host providers MAY store multiple credentials or tokens in the same request if it is required. One example where multiple credential storage is needed is with OAuth2 access tokens (AT) and refresh tokens (RT). Both the AT and RT SHOULD be stored in the same location using the credential store with complementary -credential keys. +credential service names. ### 2.5. Erasing Credentials @@ -256,26 +261,29 @@ most of the methods as implemented - implementors SHOULD implement the `IHostProvider` interface directly instead. Implementors that choose to derive from this base class MUST implement all -abstract methods and properties. The two primary abstract methods to implement -are `GetCredentialKey` and `GenerateCredentialAsync`. +abstract methods and properties. The primary abstract method to implement +is `GenerateCredentialAsync`. -#### 2.6.1 `GetCredentialKey` +There is also an additional `virtual` method named `GetServiceName` that is used +by the default implementations of the `Get|Store|EraseCredentialAsync` methods +to locate and store credentials. -The `GetCredentialKey` method MUST return a string that forms a key for storing -credentials for this provider and request. The key returned MUST be stable - -i.e, it MUST return the same value given the same or equivalent input arguments. +#### 2.6.1 `GetServiceName` -This key is used by the `GetCredentialAsync` method to first check for any -existing credential stored in the credential store, returning it if found. +The `GetServiceName` virtual method, if overriden, MUST return a string that +identifies the service/provider for this request, and is used for storing +credentials. The value returned MUST be stable - i.e, it MUST return the same +value given the same or equivalent input arguments. -The key is also similarly used by the `StoreCredentialAsync` and -`EraseCredentialAsync` methods to store and erase, respectively, credentials -passed as arguments by Git. +By default this method returns the full remote URI, without a trailing slash, +including protocol/scheme, hostname, and path if present in the input arguments. +Any username in the input arguments is never included in the URI. #### 2.6.2 `GenerateCredentialAsync` The `GenerateCredentialAsync` method will be called if an existing credential -with a matching credential key is not found in the credential store. +with a matching service (from `GetServiceName`) and account is not found in the +credential store. This method MUST return a freshly created/generated credential and not any existing or stored one. It MAY use existing or stored ancillary data or tokens, From fd6df7c8cfa879acac4ef1a24888af73abee7925 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 8 Sep 2020 11:11:21 +0100 Subject: [PATCH 16/16] environment: fix environment process locator 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(!) --- .../Interop/Posix/PosixEnvironment.cs | 1 + .../Interop/Windows/WindowsEnvironment.cs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Posix/PosixEnvironment.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Posix/PosixEnvironment.cs index 3251590c3..7082a098d 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Posix/PosixEnvironment.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Posix/PosixEnvironment.cs @@ -36,6 +36,7 @@ public override string LocateExecutable(string program) const string whichPath = "/usr/bin/which"; var psi = new ProcessStartInfo(whichPath, program) { + UseShellExecute = false, RedirectStandardOutput = true }; diff --git a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsEnvironment.cs b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsEnvironment.cs index 23b1cd7b4..919a48192 100644 --- a/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsEnvironment.cs +++ b/src/shared/Microsoft.Git.CredentialManager/Interop/Windows/WindowsEnvironment.cs @@ -70,6 +70,7 @@ public override string LocateExecutable(string program) string wherePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.System), "where.exe"); var psi = new ProcessStartInfo(wherePath, program) { + UseShellExecute = false, RedirectStandardOutput = true };