From e066bf4c7f1907cf8443991b06f9c56b7166949f Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Tue, 20 Nov 2018 22:21:19 +0000 Subject: [PATCH 1/7] Add secure credential storage APIs for Windows+macOS Add secure credential storage APIs for the Windows Credential Manager and the macOS Keychain. --- .../CommandContext.cs | 28 +++ .../GitCredential.cs | 2 +- .../SecureStorage/Credential.cs | 23 ++ .../SecureStorage/ICredentialStore.cs | 32 +++ .../SecureStorage/MacOSKeychain.cs | 222 ++++++++++++++++++ .../SecureStorage/NativeMethods.Mac.cs | 150 ++++++++++++ .../SecureStorage/NativeMethods.Windows.cs | 95 ++++++++ .../SecureStorage/NativeMethods.cs | 34 +++ .../SecureStorage/WindowsCredentialManager.cs | 112 +++++++++ .../SecureStorage/MacOSKeychainTests.cs | 57 +++++ .../WindowsCredentialManagerTests.cs | 57 +++++ 11 files changed, 811 insertions(+), 1 deletion(-) create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/Credential.cs create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs create mode 100644 src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs create mode 100644 tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs create mode 100644 tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs diff --git a/src/Microsoft.Git.CredentialManager/CommandContext.cs b/src/Microsoft.Git.CredentialManager/CommandContext.cs index eac1d5152..6bfd0abcf 100644 --- a/src/Microsoft.Git.CredentialManager/CommandContext.cs +++ b/src/Microsoft.Git.CredentialManager/CommandContext.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Text; +using Microsoft.Git.CredentialManager.SecureStorage; namespace Microsoft.Git.CredentialManager { @@ -37,6 +38,11 @@ public interface ICommandContext /// IFileSystem FileSystem { get; } + /// + /// Secure credential storage. + /// + ICredentialStore CredentialStore { get; } + /// /// Access the environment variables for the current GCM process. /// @@ -110,6 +116,8 @@ public TextWriter StdError public IFileSystem FileSystem { get; } = new FileSystem(); + public ICredentialStore CredentialStore { get; } = GetDefaultCredentialStore(); + public IReadOnlyDictionary GetEnvironmentVariables() { IDictionary variables = Environment.GetEnvironmentVariables(); @@ -140,6 +148,26 @@ public IReadOnlyDictionary GetEnvironmentVariables() } #endregion + + private static ICredentialStore GetDefaultCredentialStore() + { + if (PlatformUtils.IsMacOS()) + { + return MacOSKeychain.OpenDefault(); + } + + if (PlatformUtils.IsWindows()) + { + return WindowsCredentialManager.OpenDefault(); + } + + if (PlatformUtils.IsLinux()) + { + throw new NotImplementedException(); + } + + throw new PlatformNotSupportedException(); + } } public static class CommandContextExtensions diff --git a/src/Microsoft.Git.CredentialManager/GitCredential.cs b/src/Microsoft.Git.CredentialManager/GitCredential.cs index 821258cad..4cf6db9bd 100644 --- a/src/Microsoft.Git.CredentialManager/GitCredential.cs +++ b/src/Microsoft.Git.CredentialManager/GitCredential.cs @@ -7,7 +7,7 @@ namespace Microsoft.Git.CredentialManager /// /// Represents a credential (username/password pair) that Git can use to authenticate to a remote repository. /// - public class GitCredential + public class GitCredential : SecureStorage.ICredential { public GitCredential(string userName, string password) { diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/Credential.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/Credential.cs new file mode 100644 index 000000000..11def9b18 --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/Credential.cs @@ -0,0 +1,23 @@ +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + /// + /// Represents a simple credential; user name and password pair. + /// + public interface ICredential + { + string UserName { get; } + string Password { get; } + } + + internal class Credential : ICredential + { + public Credential(string userName, string password) + { + UserName = userName; + Password = password; + } + + public string UserName { get; } + public string Password { get; } + } +} diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs new file mode 100644 index 000000000..4f47dd64d --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs @@ -0,0 +1,32 @@ +using System.Collections.Generic; + +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + /// + /// Represents a secure storage location for s. + /// + public interface ICredentialStore + { + /// + /// Get credential from the store with the specified key. + /// + /// Key for credential to retrieve. + /// Stored credential. + /// Thrown if no credential exists in the store with the specified key. + ICredential Get(string key); + + /// + /// 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); + + /// + /// Delete credential from the store with the specified key. + /// + /// Key of credential to delete. + /// True if the credential was deleted, false otherwise. + bool Remove(string key); + } +} diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs new file mode 100644 index 000000000..7fb99054b --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs @@ -0,0 +1,222 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Text; +using static Microsoft.Git.CredentialManager.SecureStorage.NativeMethods.MacOS; + +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + public class MacOSKeychain : ICredentialStore + { + #region Constructors + + /// + /// Open the default keychain (current user's login keychain). + /// + /// Default keychain. + public static MacOSKeychain OpenDefault() + { + return new MacOSKeychain(); + } + + private MacOSKeychain() + { + PlatformUtils.EnsureMacOS(); + } + + #endregion + + #region ICredentialStore + + public ICredential Get(string key) + { + IntPtr passwordData = IntPtr.Zero; + IntPtr itemRef = IntPtr.Zero; + + try + { + // Find the item (itemRef) and password (passwordData) in the keychain + ThrowOnError( + SecKeychainFindGenericPassword( + IntPtr.Zero, (uint) key.Length, key, 0, null, + out uint passwordLength, out passwordData, out itemRef) + ); + + // Get and decode the user name from the 'account name' attribute + byte[] userNameBytes = GetAccountNameAttributeData(itemRef); + string userName = Encoding.UTF8.GetString(userNameBytes); + + // Decode the password from the raw data + byte[] passwordBytes = NativeMethods.ToByteArray(passwordData, passwordLength); + string password = Encoding.UTF8.GetString(passwordBytes); + + return new Credential(userName, password); + } + catch (KeyNotFoundException) + { + return null; + } + finally + { + if (passwordData != IntPtr.Zero) + { + SecKeychainItemFreeContent(IntPtr.Zero, passwordData); + } + + if (itemRef != IntPtr.Zero) + { + CFRelease(itemRef); + } + } + } + + public void AddOrUpdate(string key, ICredential credential) + { + byte[] passwordBytes = Encoding.UTF8.GetBytes(credential.Password); + + IntPtr passwordData = IntPtr.Zero; + IntPtr itemRef = IntPtr.Zero; + + try + { + // Check if an entry already exists in the keychain + SecKeychainFindGenericPassword( + IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, credential.UserName, + out uint _, out passwordData, out itemRef); + + if (itemRef != IntPtr.Zero) // Update existing entry + { + ThrowOnError( + SecKeychainItemModifyAttributesAndData(itemRef, IntPtr.Zero, (uint) passwordBytes.Length, passwordBytes), + "Could not update existing item" + ); + } + else // Create new entry + { + ThrowOnError( + SecKeychainAddGenericPassword(IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, + credential.UserName, (uint) passwordBytes.Length, passwordBytes, out itemRef), + "Could not create new item" + ); + } + } + finally + { + if (passwordData != IntPtr.Zero) + { + SecKeychainItemFreeContent(IntPtr.Zero, passwordData); + } + + if (itemRef != IntPtr.Zero) + { + CFRelease(itemRef); + } + } + } + + public bool Remove(string key) + { + IntPtr passwordData = IntPtr.Zero; + IntPtr itemRef = IntPtr.Zero; + + try + { + SecKeychainFindGenericPassword( + IntPtr.Zero, (uint) key.Length, key, 0, null, + out _, out passwordData, out itemRef); + + if (itemRef != IntPtr.Zero) + { + ThrowOnError( + SecKeychainItemDelete(itemRef) + ); + + return true; + } + + return false; + } + catch (KeyNotFoundException) + { + return false; + } + finally + { + if (passwordData != IntPtr.Zero) + { + SecKeychainItemFreeContent(IntPtr.Zero, passwordData); + } + + if (itemRef != IntPtr.Zero) + { + CFRelease(itemRef); + } + } + } + + #endregion + + #region Private Methods + + private static byte[] GetAccountNameAttributeData(IntPtr itemRef) + { + IntPtr tagArrayPtr = IntPtr.Zero; + IntPtr formatArrayPtr = IntPtr.Zero; + IntPtr attrListPtr = IntPtr.Zero; // SecKeychainAttributeList + + try + { + // Extract the user name by querying for the item's 'account' attribute + tagArrayPtr = Marshal.AllocCoTaskMem(sizeof(SecKeychainAttrType)); + Marshal.Copy(new[] {(int) SecKeychainAttrType.AccountItem}, 0, tagArrayPtr, 1); + + formatArrayPtr = Marshal.AllocCoTaskMem(sizeof(CssmDbAttributeFormat)); + Marshal.Copy(new[] {(int) CssmDbAttributeFormat.String}, 0, formatArrayPtr, 1); + + var attributeInfo = new SecKeychainAttributeInfo + { + Count = 1, + Tag = tagArrayPtr, + Format = formatArrayPtr, + }; + + ThrowOnError( + SecKeychainItemCopyAttributesAndData( + itemRef, ref attributeInfo, + IntPtr.Zero, out attrListPtr, out var _, IntPtr.Zero) + ); + + SecKeychainAttributeList attrList = Marshal.PtrToStructure(attrListPtr); + Debug.Assert(attrList.Count == 1); + + byte[] attrListArrayBytes = NativeMethods.ToByteArray( + attrList.Attributes, Marshal.SizeOf() * attrList.Count); + + SecKeychainAttribute[] attributes = NativeMethods.ToStructArray(attrListArrayBytes); + Debug.Assert(attributes.Length == 1); + + return NativeMethods.ToByteArray(attributes[0].Data, attributes[0].Length); + } + finally + { + if (tagArrayPtr != IntPtr.Zero) + { + Marshal.FreeCoTaskMem(tagArrayPtr); + } + + if (formatArrayPtr != IntPtr.Zero) + { + Marshal.FreeCoTaskMem(formatArrayPtr); + } + + if (attrListPtr != IntPtr.Zero) + { + SecKeychainItemFreeAttributesAndData(attrListPtr, IntPtr.Zero); + } + } + } + + #endregion + } +} diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs new file mode 100644 index 000000000..3c1ed7ec5 --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs @@ -0,0 +1,150 @@ +using System; +using System.Collections.Generic; +using System.Runtime.InteropServices; + +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + internal static partial class NativeMethods + { + // https://developer.apple.com/documentation/security/keychain_services/keychain_items + public static class MacOS + { + private const string CoreFoundationFrameworkLib = "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation"; + private const string SecurityFrameworkLib = "/System/Library/Frameworks/Security.framework/Security"; + + private const int OK = 0; + private const int ErrorSecNoSuchKeychain = -25294; + private const int ErrorSecInvalidKeychain = -25295; + private const int ErrorSecAuthFailed = -25293; + private const int ErrorSecDuplicateItem = -25299; + private const int ErrorSecItemNotFound = -25300; + private const int ErrorSecInteractionNotAllowed = -25308; + private const int ErrorSecInteractionRequired = -25315; + private const int ErrorSecNoSuchAttr = -25303; + + public static void ThrowOnError(int error, string defaultErrorMessage = "Unknown error.") + { + switch (error) + { + case OK: + return; + case ErrorSecNoSuchKeychain: + throw new InvalidOperationException($"The keychain does not exist. ({ErrorSecNoSuchKeychain})"); + case ErrorSecInvalidKeychain: + throw new InvalidOperationException($"The keychain is not valid. ({ErrorSecInvalidKeychain})"); + case ErrorSecAuthFailed: + throw new InvalidOperationException($"Authorization/Authentication failed. ({ErrorSecAuthFailed})"); + case ErrorSecDuplicateItem: + throw new ArgumentException($"The item already exists. ({ErrorSecDuplicateItem})"); + case ErrorSecItemNotFound: + throw new KeyNotFoundException($"The item cannot be found. ({ErrorSecItemNotFound})"); + case ErrorSecInteractionNotAllowed: + throw new InvalidOperationException($"Interaction with the Security Server is not allowed. ({ErrorSecInteractionNotAllowed})"); + case ErrorSecInteractionRequired: + throw new InvalidOperationException($"User interaction is required. ({ErrorSecInteractionRequired})"); + case ErrorSecNoSuchAttr: + throw new InvalidOperationException($"The attribute does not exist. ({ErrorSecNoSuchAttr})"); + default: + throw new Exception($"{defaultErrorMessage} ({error})"); + } + } + + [StructLayout(LayoutKind.Sequential)] + public struct SecKeychainAttributeInfo + { + public uint Count; + public IntPtr Tag; // uint* (SecKeychainAttrType*) + public IntPtr Format; // uint* (CssmDbAttributeFormat*) + } + + [StructLayout(LayoutKind.Sequential)] + public struct SecKeychainAttributeList + { + public uint Count; + public IntPtr Attributes; // SecKeychainAttribute* + } + + [StructLayout(LayoutKind.Sequential)] + public struct SecKeychainAttribute + { + public SecKeychainAttrType Tag; + public uint Length; + public IntPtr Data; + } + + public enum CssmDbAttributeFormat : uint + { + String = 0, + SInt32 = 1, + UInt32 = 2, + BigNum = 3, + Real = 4, + TimeDate = 5, + Blob = 6, + MultiUInt32 = 7, + Complex = 8 + }; + + public enum SecKeychainAttrType : uint + { + AccountItem = 1633903476, + } + + [DllImport(CoreFoundationFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern void CFRelease(IntPtr cf); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainAddGenericPassword( + IntPtr keychain, + uint serviceNameLength, + string serviceName, + uint accountNameLength, + string accountName, + uint passwordLength, + byte[] passwordData, + out IntPtr itemRef); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainFindGenericPassword( + IntPtr keychainOrArray, + uint serviceNameLength, + string serviceName, + uint accountNameLength, + string accountName, + out uint passwordLength, + out IntPtr passwordData, + out IntPtr itemRef); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemCopyAttributesAndData( + IntPtr itemRef, + ref SecKeychainAttributeInfo info, + IntPtr itemClass, // SecItemClass* + out IntPtr attrList, // SecKeychainAttributeList* + out uint dataLength, + IntPtr data); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemModifyAttributesAndData( + IntPtr itemRef, + IntPtr attrList, // SecKeychainAttributeList* + uint length, + byte[] data); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemDelete( + IntPtr itemRef); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemFreeContent( + IntPtr attrList, // SecKeychainAttributeList* + IntPtr data); + + [DllImport(SecurityFrameworkLib, CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] + public static extern int SecKeychainItemFreeAttributesAndData( + IntPtr attrList, // SecKeychainAttributeList* + IntPtr data); + + } + } +} diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs new file mode 100644 index 000000000..39244619b --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs @@ -0,0 +1,95 @@ +using System; +using System.Collections.Generic; +using System.ComponentModel; +using System.Runtime.InteropServices; +using System.Runtime.InteropServices.ComTypes; +using FILETIME = System.Runtime.InteropServices.ComTypes.FILETIME; + +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + internal static partial class NativeMethods + { + // https://docs.microsoft.com/en-us/windows/desktop/api/wincred/ + public static class Windows + { + private const string Advapi32 = "advapi32.dll"; + + private const int ERROR_NO_SUCH_LOGON_SESSION = 0; + private const int ERROR_NOT_FOUND = 0x490; + + public static void ThrowOnError(bool success, string defaultErrorMessage = null) + { + int error = Marshal.GetLastWin32Error(); + if (!success) + { + switch (error) + { + case ERROR_NO_SUCH_LOGON_SESSION: + throw new InvalidOperationException( + "The logon session does not exist or there is no credential set associated with this logon session.", + new Win32Exception(error) + ); + case ERROR_NOT_FOUND: + throw new KeyNotFoundException("The item cannot be found.", new Win32Exception(error)); + default: + throw new Win32Exception(error, defaultErrorMessage); + } + } + } + + public enum CredentialType + { + Generic = 1, + DomainPassword = 2, + DomainCertificate = 3, + DomainVisiblePassword = 4, + } + + public enum CredentialPersist + { + Session = 1, + LocalMachine = 2, + Enterprise = 3, + } + + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + public struct Win32Credential + { + public int Flags; + public CredentialType Type; + [MarshalAs(UnmanagedType.LPWStr)] public string TargetName; + [MarshalAs(UnmanagedType.LPWStr)] public string Comment; + public FILETIME LastWritten; + public int CredentialBlobSize; + public IntPtr CredentialBlob; + public CredentialPersist Persist; + public int AttributeCount; + public IntPtr CredAttribute; + [MarshalAs(UnmanagedType.LPWStr)] public string TargetAlias; + [MarshalAs(UnmanagedType.LPWStr)] public string UserName; + } + + [DllImport(Advapi32, EntryPoint = "CredReadW", CharSet = CharSet.Unicode, SetLastError = true)] + public static extern bool CredRead( + string target, + CredentialType type, + int reserved, + out IntPtr credential); + + [DllImport(Advapi32, EntryPoint = "CredWriteW", CharSet = CharSet.Unicode, SetLastError = true)] + public static extern bool CredWrite( + ref Win32Credential credential, + int flags); + + [DllImport(Advapi32, EntryPoint = "CredDeleteW", CharSet = CharSet.Unicode, SetLastError = true)] + public static extern bool CredDelete( + string target, + CredentialType type, + int flags); + + [DllImport(Advapi32, EntryPoint = "CredFree", CharSet = CharSet.Unicode, SetLastError = true)] + internal static extern void CredFree( + IntPtr credential); + } + } +} diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs new file mode 100644 index 000000000..027a6bc9d --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs @@ -0,0 +1,34 @@ +using System; +using System.Runtime.InteropServices; + +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + internal static partial class NativeMethods + { + public static T[] ToStructArray(byte[] source) where T : struct + { + var destination = new T[source.Length / Marshal.SizeOf()]; + GCHandle handle = GCHandle.Alloc(destination, GCHandleType.Pinned); + try + { + IntPtr pointer = handle.AddrOfPinnedObject(); + Marshal.Copy(source, 0, pointer, source.Length); + return destination; + } + finally + { + if (handle.IsAllocated) + { + handle.Free(); + } + } + } + + public static byte[] ToByteArray(IntPtr ptr, long count) + { + var destination = new byte[count]; + Marshal.Copy(ptr, destination, 0, destination.Length); + return destination; + } + } +} diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs new file mode 100644 index 000000000..bc060fb2b --- /dev/null +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs @@ -0,0 +1,112 @@ +using System; +using System.Collections.Generic; +using System.Runtime.InteropServices; +using System.Text; +using static Microsoft.Git.CredentialManager.SecureStorage.NativeMethods.Windows; + +namespace Microsoft.Git.CredentialManager.SecureStorage +{ + public class WindowsCredentialManager : ICredentialStore + { + #region Constructors + + /// + /// Open the Windows Credential Manager vault for the current user. + /// + /// Current user's Credential Manager vault. + public static WindowsCredentialManager OpenDefault() + { + return new WindowsCredentialManager(); + } + + private WindowsCredentialManager() + { + PlatformUtils.EnsureWindows(); + } + + #endregion + + #region ICredentialStore + + public ICredential Get(string key) + { + IntPtr credPtr = IntPtr.Zero; + + try + { + ThrowOnError( + CredRead(key, CredentialType.Generic, 0, out credPtr), + "Failed to read item from store." + ); + + Win32Credential credential = Marshal.PtrToStructure(credPtr); + + var userName = credential.UserName; + + byte[] passwordBytes = NativeMethods.ToByteArray(credential.CredentialBlob, credential.CredentialBlobSize); + var password = Encoding.Unicode.GetString(passwordBytes); + + return new Credential(userName, password); + } + catch (KeyNotFoundException) + { + return null; + } + finally + { + if (credPtr != IntPtr.Zero) + { + CredFree(credPtr); + } + } + } + + public void AddOrUpdate(string key, ICredential credential) + { + byte[] passwordBytes = Encoding.Unicode.GetBytes(credential.Password); + + var w32Credential = new Win32Credential + { + Type = CredentialType.Generic, + TargetName = key, + CredentialBlob = Marshal.AllocCoTaskMem(passwordBytes.Length), + CredentialBlobSize = passwordBytes.Length, + Persist = CredentialPersist.LocalMachine, + AttributeCount = 0, + UserName = credential.UserName, + }; + + try + { + Marshal.Copy(passwordBytes, 0, w32Credential.CredentialBlob, passwordBytes.Length); + + ThrowOnError( + CredWrite(ref w32Credential, 0), + "Failed to write item to store." + ); + } + finally + { + if (w32Credential.CredentialBlob != IntPtr.Zero) + { + Marshal.FreeCoTaskMem(w32Credential.CredentialBlob); + } + } + } + + public bool Remove(string key) + { + try + { + ThrowOnError(CredDelete(key, CredentialType.Generic, 0)); + return true; + } + catch (KeyNotFoundException) + { + return false; + } + } + + #endregion + } +} diff --git a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs new file mode 100644 index 000000000..4ebc2ea56 --- /dev/null +++ b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs @@ -0,0 +1,57 @@ +using System; +using Xunit; +using Microsoft.Git.CredentialManager.SecureStorage; + +namespace Microsoft.Git.CredentialManager.Tests.SecureStorage +{ + public class MacOSKeychainTests + { + [PlatformFact(Platform.MacOS)] + public void MacOSKeychain_ReadWriteDelete() + { + MacOSKeychain keychain = MacOSKeychain.OpenDefault(); + + const string key = "secretkey"; + const string userName = "john.doe"; + const string password = "letmein123"; + var credential = new GitCredential(userName, password); + + // Write + keychain.AddOrUpdate(key, credential); + + // Read + ICredential outCredential = keychain.Get(key); + + Assert.NotNull(outCredential); + Assert.Equal(credential.UserName, outCredential.UserName); + Assert.Equal(credential.Password, outCredential.Password); + + // Delete + keychain.Remove(key); + } + + [PlatformFact(Platform.MacOS)] + public void MacOSKeychain_Get_KeyNotFound_ReturnsNull() + { + MacOSKeychain keychain = MacOSKeychain.OpenDefault(); + + // Unique key; guaranteed not to exist! + string key = Guid.NewGuid().ToString("N"); + + ICredential credential = keychain.Get(key); + Assert.Null(credential); + } + + [PlatformFact(Platform.MacOS)] + public void MacOSKeychain_Remove_KeyNotFound_ReturnsFalse() + { + MacOSKeychain keychain = MacOSKeychain.OpenDefault(); + + // Unique key; guaranteed not to exist! + string key = Guid.NewGuid().ToString("N"); + + bool result = keychain.Remove(key); + Assert.False(result); + } + } +} diff --git a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs new file mode 100644 index 000000000..a38e483e6 --- /dev/null +++ b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs @@ -0,0 +1,57 @@ +using System; +using Xunit; +using Microsoft.Git.CredentialManager.SecureStorage; + +namespace Microsoft.Git.CredentialManager.Tests.SecureStorage +{ + public class WindowsCredentialManagerTests + { + [PlatformFact(Platform.Windows)] + public void WindowsCredentialManager_ReadWriteDelete() + { + WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); + + const string key = "secretkey"; + const string userName = "john.doe"; + const string password = "letmein123"; + var credential = new GitCredential(userName, password); + + // Write + credManager.AddOrUpdate(key, credential); + + // Read + ICredential outCredential = credManager.Get(key); + + Assert.NotNull(outCredential); + Assert.Equal(credential.UserName, outCredential.UserName); + Assert.Equal(credential.Password, outCredential.Password); + + // Delete + credManager.Remove(key); + } + + [PlatformFact(Platform.Windows)] + public void WindowsCredentialManager_Get_KeyNotFound_ReturnsNull() + { + WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); + + // Unique key; guaranteed not to exist! + string key = Guid.NewGuid().ToString("N"); + + ICredential credential = credManager.Get(key); + Assert.Null(credential); + } + + [PlatformFact(Platform.Windows)] + public void WindowsCredentialManager_Remove_KeyNotFound_ReturnsFalse() + { + WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); + + // Unique key; guaranteed not to exist! + string key = Guid.NewGuid().ToString("N"); + + bool result = credManager.Remove(key); + Assert.False(result); + } + } +} From 2b335728546ad76748a38eb2f9c50859cf19adf5 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Mon, 3 Dec 2018 14:48:57 +0000 Subject: [PATCH 2/7] Fix bad ICredStore comment; add macOS native doc links Fix an incorrect documentation comment on `ICredentialStore` that an exception is thrown for a missing credential key - it should return `null` instead. Add some more links to Apple documentation showing where various constant values came from. --- .../SecureStorage/ICredentialStore.cs | 7 ++----- .../SecureStorage/NativeMethods.Mac.cs | 2 ++ 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs index 4f47dd64d..3ea71f5ed 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/ICredentialStore.cs @@ -1,6 +1,4 @@ -using System.Collections.Generic; - -namespace Microsoft.Git.CredentialManager.SecureStorage +namespace Microsoft.Git.CredentialManager.SecureStorage { /// /// Represents a secure storage location for s. @@ -11,8 +9,7 @@ public interface ICredentialStore /// Get credential from the store with the specified key. /// /// Key for credential to retrieve. - /// Stored credential. - /// Thrown if no credential exists in the store with the specified key. + /// Stored credential or null if not found. ICredential Get(string key); /// diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs index 3c1ed7ec5..39b13ed73 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs @@ -12,6 +12,7 @@ public static class MacOS private const string CoreFoundationFrameworkLib = "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation"; private const string SecurityFrameworkLib = "/System/Library/Frameworks/Security.framework/Security"; + // https://developer.apple.com/documentation/security/1542001-security_framework_result_codes private const int OK = 0; private const int ErrorSecNoSuchKeychain = -25294; private const int ErrorSecInvalidKeychain = -25295; @@ -87,6 +88,7 @@ public enum CssmDbAttributeFormat : uint public enum SecKeychainAttrType : uint { + // https://developer.apple.com/documentation/security/secitemattr/accountitemattr AccountItem = 1633903476, } From 9fa67e68fa78c4ac2610e3ee44c88ac231ff4aa6 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 5 Dec 2018 15:09:04 +0000 Subject: [PATCH 3/7] Add Debug.Assert messages; rename helper methods - Add messages to the `Debug.Assert` calls in `MacOSKeychain`. - Rename the `GetDefaultCredentialStore` helper method to `CreateCredentialStore` in `CommandContext`. --- .../CommandContext.cs | 4 +-- .../SecureStorage/MacOSKeychain.cs | 4 +-- .../SecureStorage/MacOSKeychainTests.cs | 32 +++++++++++-------- .../WindowsCredentialManagerTests.cs | 32 +++++++++++-------- 4 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/Microsoft.Git.CredentialManager/CommandContext.cs b/src/Microsoft.Git.CredentialManager/CommandContext.cs index 6bfd0abcf..e5ab419b4 100644 --- a/src/Microsoft.Git.CredentialManager/CommandContext.cs +++ b/src/Microsoft.Git.CredentialManager/CommandContext.cs @@ -116,7 +116,7 @@ public TextWriter StdError public IFileSystem FileSystem { get; } = new FileSystem(); - public ICredentialStore CredentialStore { get; } = GetDefaultCredentialStore(); + public ICredentialStore CredentialStore { get; } = CreateCredentialStore(); public IReadOnlyDictionary GetEnvironmentVariables() { @@ -149,7 +149,7 @@ public IReadOnlyDictionary GetEnvironmentVariables() #endregion - private static ICredentialStore GetDefaultCredentialStore() + private static ICredentialStore CreateCredentialStore() { if (PlatformUtils.IsMacOS()) { diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs index 7fb99054b..a2e4aa6f6 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs @@ -188,13 +188,13 @@ private static byte[] GetAccountNameAttributeData(IntPtr itemRef) ); SecKeychainAttributeList attrList = Marshal.PtrToStructure(attrListPtr); - Debug.Assert(attrList.Count == 1); + Debug.Assert(attrList.Count == 1, "Only expecting a list structure containing one attribute to be returned"); byte[] attrListArrayBytes = NativeMethods.ToByteArray( attrList.Attributes, Marshal.SizeOf() * attrList.Count); SecKeychainAttribute[] attributes = NativeMethods.ToStructArray(attrListArrayBytes); - Debug.Assert(attributes.Length == 1); + Debug.Assert(attributes.Length == 1, "Only expecting one attribute structure to returned from raw byte conversion"); return NativeMethods.ToByteArray(attributes[0].Data, attributes[0].Length); } diff --git a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs index 4ebc2ea56..2c1df3470 100644 --- a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs +++ b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs @@ -11,23 +11,29 @@ public void MacOSKeychain_ReadWriteDelete() { MacOSKeychain keychain = MacOSKeychain.OpenDefault(); - const string key = "secretkey"; + // Create a key that is guarenteed to be unique + string key = $"secretkey-{Guid.NewGuid():N}"; const string userName = "john.doe"; const string password = "letmein123"; var credential = new GitCredential(userName, password); - // Write - keychain.AddOrUpdate(key, credential); - - // Read - ICredential outCredential = keychain.Get(key); - - Assert.NotNull(outCredential); - Assert.Equal(credential.UserName, outCredential.UserName); - Assert.Equal(credential.Password, outCredential.Password); - - // Delete - keychain.Remove(key); + try + { + // Write + keychain.AddOrUpdate(key, credential); + + // Read + ICredential outCredential = keychain.Get(key); + + Assert.NotNull(outCredential); + Assert.Equal(credential.UserName, outCredential.UserName); + Assert.Equal(credential.Password, outCredential.Password); + } + finally + { + // Ensure we clean up after ourselves even in case of 'get' failures + keychain.Remove(key); + } } [PlatformFact(Platform.MacOS)] diff --git a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs index a38e483e6..369d51afa 100644 --- a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs +++ b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs @@ -11,23 +11,29 @@ public void WindowsCredentialManager_ReadWriteDelete() { WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); - const string key = "secretkey"; + // Create a key that is guarenteed to be unique + string key = $"secretkey-{Guid.NewGuid():N}"; const string userName = "john.doe"; const string password = "letmein123"; var credential = new GitCredential(userName, password); - // Write - credManager.AddOrUpdate(key, credential); - - // Read - ICredential outCredential = credManager.Get(key); - - Assert.NotNull(outCredential); - Assert.Equal(credential.UserName, outCredential.UserName); - Assert.Equal(credential.Password, outCredential.Password); - - // Delete - credManager.Remove(key); + try + { + // Write + credManager.AddOrUpdate(key, credential); + + // Read + ICredential outCredential = credManager.Get(key); + + Assert.NotNull(outCredential); + Assert.Equal(credential.UserName, outCredential.UserName); + Assert.Equal(credential.Password, outCredential.Password); + } + finally + { + // Ensure we clean up after ourselves even in case of 'get' failures + credManager.Remove(key); + } } [PlatformFact(Platform.Windows)] From 63be961a17ee01b196496801293810b21583f2de Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 6 Dec 2018 11:25:21 +0000 Subject: [PATCH 4/7] Rename cred store static `OpenDefault` ctors to `Open` Rename the `MacOSKeychain` and `WindowsCredentialManager` static contructor methods from `OpenDefault` to `Open`. In the future should we want to or need to disambiguate between _which_ store is being used we can add overloads to the that singularly named method. --- src/Microsoft.Git.CredentialManager/CommandContext.cs | 4 ++-- .../SecureStorage/MacOSKeychain.cs | 2 +- .../SecureStorage/WindowsCredentialManager.cs | 2 +- .../SecureStorage/MacOSKeychainTests.cs | 6 +++--- .../SecureStorage/WindowsCredentialManagerTests.cs | 6 +++--- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Git.CredentialManager/CommandContext.cs b/src/Microsoft.Git.CredentialManager/CommandContext.cs index e5ab419b4..d1f3df88d 100644 --- a/src/Microsoft.Git.CredentialManager/CommandContext.cs +++ b/src/Microsoft.Git.CredentialManager/CommandContext.cs @@ -153,12 +153,12 @@ private static ICredentialStore CreateCredentialStore() { if (PlatformUtils.IsMacOS()) { - return MacOSKeychain.OpenDefault(); + return MacOSKeychain.Open(); } if (PlatformUtils.IsWindows()) { - return WindowsCredentialManager.OpenDefault(); + return WindowsCredentialManager.Open(); } if (PlatformUtils.IsLinux()) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs index a2e4aa6f6..572cbc065 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs @@ -15,7 +15,7 @@ public class MacOSKeychain : ICredentialStore /// Open the default keychain (current user's login keychain). /// /// Default keychain. - public static MacOSKeychain OpenDefault() + public static MacOSKeychain Open() { return new MacOSKeychain(); } diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs index bc060fb2b..790cb0dac 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs @@ -14,7 +14,7 @@ public class WindowsCredentialManager : ICredentialStore /// Open the Windows Credential Manager vault for the current user. /// /// Current user's Credential Manager vault. - public static WindowsCredentialManager OpenDefault() + public static WindowsCredentialManager Open() { return new WindowsCredentialManager(); } diff --git a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs index 2c1df3470..134208456 100644 --- a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs +++ b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs @@ -9,7 +9,7 @@ public class MacOSKeychainTests [PlatformFact(Platform.MacOS)] public void MacOSKeychain_ReadWriteDelete() { - MacOSKeychain keychain = MacOSKeychain.OpenDefault(); + MacOSKeychain keychain = MacOSKeychain.Open(); // Create a key that is guarenteed to be unique string key = $"secretkey-{Guid.NewGuid():N}"; @@ -39,7 +39,7 @@ public void MacOSKeychain_ReadWriteDelete() [PlatformFact(Platform.MacOS)] public void MacOSKeychain_Get_KeyNotFound_ReturnsNull() { - MacOSKeychain keychain = MacOSKeychain.OpenDefault(); + MacOSKeychain keychain = MacOSKeychain.Open(); // Unique key; guaranteed not to exist! string key = Guid.NewGuid().ToString("N"); @@ -51,7 +51,7 @@ public void MacOSKeychain_Get_KeyNotFound_ReturnsNull() [PlatformFact(Platform.MacOS)] public void MacOSKeychain_Remove_KeyNotFound_ReturnsFalse() { - MacOSKeychain keychain = MacOSKeychain.OpenDefault(); + MacOSKeychain keychain = MacOSKeychain.Open(); // Unique key; guaranteed not to exist! string key = Guid.NewGuid().ToString("N"); diff --git a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs index 369d51afa..10bc704ad 100644 --- a/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs +++ b/tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/WindowsCredentialManagerTests.cs @@ -9,7 +9,7 @@ public class WindowsCredentialManagerTests [PlatformFact(Platform.Windows)] public void WindowsCredentialManager_ReadWriteDelete() { - WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); + WindowsCredentialManager credManager = WindowsCredentialManager.Open(); // Create a key that is guarenteed to be unique string key = $"secretkey-{Guid.NewGuid():N}"; @@ -39,7 +39,7 @@ public void WindowsCredentialManager_ReadWriteDelete() [PlatformFact(Platform.Windows)] public void WindowsCredentialManager_Get_KeyNotFound_ReturnsNull() { - WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); + WindowsCredentialManager credManager = WindowsCredentialManager.Open(); // Unique key; guaranteed not to exist! string key = Guid.NewGuid().ToString("N"); @@ -51,7 +51,7 @@ public void WindowsCredentialManager_Get_KeyNotFound_ReturnsNull() [PlatformFact(Platform.Windows)] public void WindowsCredentialManager_Remove_KeyNotFound_ReturnsFalse() { - WindowsCredentialManager credManager = WindowsCredentialManager.OpenDefault(); + WindowsCredentialManager credManager = WindowsCredentialManager.Open(); // Unique key; guaranteed not to exist! string key = Guid.NewGuid().ToString("N"); From c1167aa482a377e0dc8a7010677dd856c233a934 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 6 Dec 2018 11:49:08 +0000 Subject: [PATCH 5/7] Use the global alloc/free methods for native interop Replace usages of the COM allocator (`Marshal.AllocCoTaskMem` and `Marshal.FreeCoTaskMem`) in the credential stores code with the global allocator (`Marshal.AllocHGlobal` and `Marshal.FreeHGlobal`). Since there is no COM heap on Mac using this allocator was not the correct thing to do. On Windows the CredRead/Write/Delete calls are Win32 API calls which don't use COM either. Use the global allocator for WindowsCredMgr native code --- .../SecureStorage/MacOSKeychain.cs | 8 ++++---- .../SecureStorage/WindowsCredentialManager.cs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs index 572cbc065..f899b65e7 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs @@ -168,10 +168,10 @@ private static byte[] GetAccountNameAttributeData(IntPtr itemRef) try { // Extract the user name by querying for the item's 'account' attribute - tagArrayPtr = Marshal.AllocCoTaskMem(sizeof(SecKeychainAttrType)); + tagArrayPtr = Marshal.AllocHGlobal(sizeof(SecKeychainAttrType)); Marshal.Copy(new[] {(int) SecKeychainAttrType.AccountItem}, 0, tagArrayPtr, 1); - formatArrayPtr = Marshal.AllocCoTaskMem(sizeof(CssmDbAttributeFormat)); + formatArrayPtr = Marshal.AllocHGlobal(sizeof(CssmDbAttributeFormat)); Marshal.Copy(new[] {(int) CssmDbAttributeFormat.String}, 0, formatArrayPtr, 1); var attributeInfo = new SecKeychainAttributeInfo @@ -202,12 +202,12 @@ private static byte[] GetAccountNameAttributeData(IntPtr itemRef) { if (tagArrayPtr != IntPtr.Zero) { - Marshal.FreeCoTaskMem(tagArrayPtr); + Marshal.FreeHGlobal(tagArrayPtr); } if (formatArrayPtr != IntPtr.Zero) { - Marshal.FreeCoTaskMem(formatArrayPtr); + Marshal.FreeHGlobal(formatArrayPtr); } if (attrListPtr != IntPtr.Zero) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs index 790cb0dac..7c22874f7 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs @@ -69,7 +69,7 @@ public void AddOrUpdate(string key, ICredential credential) { Type = CredentialType.Generic, TargetName = key, - CredentialBlob = Marshal.AllocCoTaskMem(passwordBytes.Length), + CredentialBlob = Marshal.AllocHGlobal(passwordBytes.Length), CredentialBlobSize = passwordBytes.Length, Persist = CredentialPersist.LocalMachine, AttributeCount = 0, @@ -89,7 +89,7 @@ public void AddOrUpdate(string key, ICredential credential) { if (w32Credential.CredentialBlob != IntPtr.Zero) { - Marshal.FreeCoTaskMem(w32Credential.CredentialBlob); + Marshal.FreeHGlobal(w32Credential.CredentialBlob); } } } From 24c66a6c6b100330f507ab5b2f045ecb8177e5fd Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 6 Dec 2018 12:02:39 +0000 Subject: [PATCH 6/7] Simplify native/managed marshaling code for `MacOSKeychain` Simplify some of the marshaling code between native and managed by using more methods from the `Marshal` type, rather than using our own helpers. --- .../SecureStorage/MacOSKeychain.cs | 14 +++++--------- .../SecureStorage/NativeMethods.cs | 19 ------------------- 2 files changed, 5 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs index f899b65e7..f428dce89 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs @@ -169,10 +169,10 @@ private static byte[] GetAccountNameAttributeData(IntPtr itemRef) { // Extract the user name by querying for the item's 'account' attribute tagArrayPtr = Marshal.AllocHGlobal(sizeof(SecKeychainAttrType)); - Marshal.Copy(new[] {(int) SecKeychainAttrType.AccountItem}, 0, tagArrayPtr, 1); + Marshal.WriteInt32(tagArrayPtr, (int) SecKeychainAttrType.AccountItem); formatArrayPtr = Marshal.AllocHGlobal(sizeof(CssmDbAttributeFormat)); - Marshal.Copy(new[] {(int) CssmDbAttributeFormat.String}, 0, formatArrayPtr, 1); + Marshal.WriteInt32(formatArrayPtr, (int) CssmDbAttributeFormat.String); var attributeInfo = new SecKeychainAttributeInfo { @@ -184,19 +184,15 @@ private static byte[] GetAccountNameAttributeData(IntPtr itemRef) ThrowOnError( SecKeychainItemCopyAttributesAndData( itemRef, ref attributeInfo, - IntPtr.Zero, out attrListPtr, out var _, IntPtr.Zero) + 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"); - byte[] attrListArrayBytes = NativeMethods.ToByteArray( - attrList.Attributes, Marshal.SizeOf() * attrList.Count); + SecKeychainAttribute attribute = Marshal.PtrToStructure(attrList.Attributes); - SecKeychainAttribute[] attributes = NativeMethods.ToStructArray(attrListArrayBytes); - Debug.Assert(attributes.Length == 1, "Only expecting one attribute structure to returned from raw byte conversion"); - - return NativeMethods.ToByteArray(attributes[0].Data, attributes[0].Length); + return NativeMethods.ToByteArray(attribute.Data, attribute.Length); } finally { diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs index 027a6bc9d..902a25966 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs @@ -5,25 +5,6 @@ namespace Microsoft.Git.CredentialManager.SecureStorage { internal static partial class NativeMethods { - public static T[] ToStructArray(byte[] source) where T : struct - { - var destination = new T[source.Length / Marshal.SizeOf()]; - GCHandle handle = GCHandle.Alloc(destination, GCHandleType.Pinned); - try - { - IntPtr pointer = handle.AddrOfPinnedObject(); - Marshal.Copy(source, 0, pointer, source.Length); - return destination; - } - finally - { - if (handle.IsAllocated) - { - handle.Free(); - } - } - } - public static byte[] ToByteArray(IntPtr ptr, long count) { var destination = new byte[count]; From 83d2a3012a2510420ea5cb9e3eba0be5f2c5f172 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 6 Dec 2018 16:17:45 +0000 Subject: [PATCH 7/7] Update error handling in credstores Update the error handling performed in both `MacOSKeychain` and `WindowsCredentialManager` to be clearer about which native return codes we consider to be 'OK', and ensure we're throwing exceptions for all other types of unknown errors. --- .../SecureStorage/MacOSKeychain.cs | 103 ++++++++++-------- .../SecureStorage/NativeMethods.Mac.cs | 27 +++-- .../SecureStorage/NativeMethods.Windows.cs | 44 ++++---- .../SecureStorage/WindowsCredentialManager.cs | 59 ++++++---- 4 files changed, 132 insertions(+), 101 deletions(-) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs index f428dce89..a9c0b88b2 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Diagnostics; using System.Runtime.InteropServices; using System.Text; @@ -37,25 +36,30 @@ public ICredential Get(string key) try { // Find the item (itemRef) and password (passwordData) in the keychain - ThrowOnError( - SecKeychainFindGenericPassword( - IntPtr.Zero, (uint) key.Length, key, 0, null, - out uint passwordLength, out passwordData, out itemRef) - ); + int findResult = SecKeychainFindGenericPassword( + IntPtr.Zero, (uint) key.Length, key, 0, null, + out uint passwordLength, out passwordData, out itemRef); - // Get and decode the user name from the 'account name' attribute - byte[] userNameBytes = GetAccountNameAttributeData(itemRef); - string userName = Encoding.UTF8.GetString(userNameBytes); + switch (findResult) + { + case OK: + // Get and decode the user name from the 'account name' attribute + byte[] userNameBytes = GetAccountNameAttributeData(itemRef); + string userName = Encoding.UTF8.GetString(userNameBytes); - // Decode the password from the raw data - byte[] passwordBytes = NativeMethods.ToByteArray(passwordData, passwordLength); - string password = Encoding.UTF8.GetString(passwordBytes); + // Decode the password from the raw data + byte[] passwordBytes = NativeMethods.ToByteArray(passwordData, passwordLength); + string password = Encoding.UTF8.GetString(passwordBytes); - return new Credential(userName, password); - } - catch (KeyNotFoundException) - { - return null; + return new Credential(userName, password); + + case ErrorSecItemNotFound: + return null; + + default: + ThrowIfError(findResult); + return null; + } } finally { @@ -81,24 +85,32 @@ public void AddOrUpdate(string key, ICredential credential) try { // Check if an entry already exists in the keychain - SecKeychainFindGenericPassword( + int findResult = SecKeychainFindGenericPassword( IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, credential.UserName, out uint _, out passwordData, out itemRef); - if (itemRef != IntPtr.Zero) // Update existing entry + switch (findResult) { - ThrowOnError( - SecKeychainItemModifyAttributesAndData(itemRef, IntPtr.Zero, (uint) passwordBytes.Length, passwordBytes), - "Could not update existing item" - ); - } - else // Create new entry - { - ThrowOnError( - SecKeychainAddGenericPassword(IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, - credential.UserName, (uint) passwordBytes.Length, passwordBytes, out itemRef), - "Could not create new item" - ); + // Create new entry + case OK: + ThrowIfError( + SecKeychainItemModifyAttributesAndData(itemRef, IntPtr.Zero, (uint) passwordBytes.Length, passwordBytes), + "Could not update existing item" + ); + break; + + // Update existing entry + case ErrorSecItemNotFound: + ThrowIfError( + SecKeychainAddGenericPassword(IntPtr.Zero, (uint) key.Length, key, (uint) credential.UserName.Length, + credential.UserName, (uint) passwordBytes.Length, passwordBytes, out itemRef), + "Could not create new item" + ); + break; + + default: + ThrowIfError(findResult); + break; } } finally @@ -122,24 +134,25 @@ public bool Remove(string key) try { - SecKeychainFindGenericPassword( + int findResult = SecKeychainFindGenericPassword( IntPtr.Zero, (uint) key.Length, key, 0, null, out _, out passwordData, out itemRef); - if (itemRef != IntPtr.Zero) + switch (findResult) { - ThrowOnError( - SecKeychainItemDelete(itemRef) - ); - - return true; + case OK: + ThrowIfError( + SecKeychainItemDelete(itemRef) + ); + return true; + + case ErrorSecItemNotFound: + return false; + + default: + ThrowIfError(findResult); + return false; } - - return false; - } - catch (KeyNotFoundException) - { - return false; } finally { @@ -181,7 +194,7 @@ private static byte[] GetAccountNameAttributeData(IntPtr itemRef) Format = formatArrayPtr, }; - ThrowOnError( + ThrowIfError( SecKeychainItemCopyAttributesAndData( itemRef, ref attributeInfo, IntPtr.Zero, out attrListPtr, out _, IntPtr.Zero) diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs index 39b13ed73..878298288 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Runtime.InteropServices; namespace Microsoft.Git.CredentialManager.SecureStorage @@ -13,17 +12,17 @@ public static class MacOS private const string SecurityFrameworkLib = "/System/Library/Frameworks/Security.framework/Security"; // https://developer.apple.com/documentation/security/1542001-security_framework_result_codes - private const int OK = 0; - private const int ErrorSecNoSuchKeychain = -25294; - private const int ErrorSecInvalidKeychain = -25295; - private const int ErrorSecAuthFailed = -25293; - private const int ErrorSecDuplicateItem = -25299; - private const int ErrorSecItemNotFound = -25300; - private const int ErrorSecInteractionNotAllowed = -25308; - private const int ErrorSecInteractionRequired = -25315; - private const int ErrorSecNoSuchAttr = -25303; - - public static void ThrowOnError(int error, string defaultErrorMessage = "Unknown error.") + public const int OK = 0; + public const int ErrorSecNoSuchKeychain = -25294; + public const int ErrorSecInvalidKeychain = -25295; + public const int ErrorSecAuthFailed = -25293; + public const int ErrorSecDuplicateItem = -25299; + public const int ErrorSecItemNotFound = -25300; + public const int ErrorSecInteractionNotAllowed = -25308; + public const int ErrorSecInteractionRequired = -25315; + public const int ErrorSecNoSuchAttr = -25303; + + public static void ThrowIfError(int error, string defaultErrorMessage = "Unknown error.") { switch (error) { @@ -36,9 +35,9 @@ public static void ThrowOnError(int error, string defaultErrorMessage = "Unknown case ErrorSecAuthFailed: throw new InvalidOperationException($"Authorization/Authentication failed. ({ErrorSecAuthFailed})"); case ErrorSecDuplicateItem: - throw new ArgumentException($"The item already exists. ({ErrorSecDuplicateItem})"); + throw new InvalidOperationException($"The item already exists. ({ErrorSecDuplicateItem})"); case ErrorSecItemNotFound: - throw new KeyNotFoundException($"The item cannot be found. ({ErrorSecItemNotFound})"); + throw new InvalidOperationException($"The item cannot be found. ({ErrorSecItemNotFound})"); case ErrorSecInteractionNotAllowed: throw new InvalidOperationException($"Interaction with the Security Server is not allowed. ({ErrorSecInteractionNotAllowed})"); case ErrorSecInteractionRequired: diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs index 39244619b..7a2c1156e 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Windows.cs @@ -1,8 +1,6 @@ using System; -using System.Collections.Generic; using System.ComponentModel; using System.Runtime.InteropServices; -using System.Runtime.InteropServices.ComTypes; using FILETIME = System.Runtime.InteropServices.ComTypes.FILETIME; namespace Microsoft.Git.CredentialManager.SecureStorage @@ -14,26 +12,34 @@ public static class Windows { private const string Advapi32 = "advapi32.dll"; - private const int ERROR_NO_SUCH_LOGON_SESSION = 0; - private const int ERROR_NOT_FOUND = 0x490; + // https://docs.microsoft.com/en-gb/windows/desktop/Debug/system-error-codes + public const int OK = 0; + public const int ERROR_NO_SUCH_LOGON_SESSION = 0x520; + public const int ERROR_NOT_FOUND = 0x490; + public const int ERROR_BAD_USERNAME = 0x89A; + public const int ERROR_INVALID_FLAGS = 0x3EC; + public const int ERROR_INVALID_PARAMETER = 0x57; - public static void ThrowOnError(bool success, string defaultErrorMessage = null) + public static int GetLastError(bool success) { - int error = Marshal.GetLastWin32Error(); - if (!success) + if (success) { - switch (error) - { - case ERROR_NO_SUCH_LOGON_SESSION: - throw new InvalidOperationException( - "The logon session does not exist or there is no credential set associated with this logon session.", - new Win32Exception(error) - ); - case ERROR_NOT_FOUND: - throw new KeyNotFoundException("The item cannot be found.", new Win32Exception(error)); - default: - throw new Win32Exception(error, defaultErrorMessage); - } + return OK; + } + + return Marshal.GetLastWin32Error(); + } + + public static void ThrowIfError(int error, string defaultErrorMessage = null) + { + switch (error) + { + case OK: + return; + default: + // The Win32Exception constructor will automatically get the human-readable + // message for the error code. + throw new Win32Exception(error, defaultErrorMessage); } } diff --git a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs index 7c22874f7..17d4d179b 100644 --- a/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs +++ b/src/Microsoft.Git.CredentialManager/SecureStorage/WindowsCredentialManager.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Runtime.InteropServices; using System.Text; using static Microsoft.Git.CredentialManager.SecureStorage.NativeMethods.Windows; @@ -34,23 +33,29 @@ public ICredential Get(string key) try { - ThrowOnError( - CredRead(key, CredentialType.Generic, 0, out credPtr), - "Failed to read item from store." + int result = GetLastError( + CredRead(key, CredentialType.Generic, 0, out credPtr) ); - Win32Credential credential = Marshal.PtrToStructure(credPtr); + switch (result) + { + case OK: + Win32Credential credential = Marshal.PtrToStructure(credPtr); - var userName = credential.UserName; + var userName = credential.UserName; - byte[] passwordBytes = NativeMethods.ToByteArray(credential.CredentialBlob, credential.CredentialBlobSize); - var password = Encoding.Unicode.GetString(passwordBytes); + byte[] passwordBytes = NativeMethods.ToByteArray(credential.CredentialBlob, credential.CredentialBlobSize); + var password = Encoding.Unicode.GetString(passwordBytes); - return new Credential(userName, password); - } - catch (KeyNotFoundException) - { - return null; + return new Credential(userName, password); + + case ERROR_NOT_FOUND: + return null; + + default: + ThrowIfError(result, "Failed to read item from store."); + return null; + } } finally { @@ -80,10 +85,11 @@ public void AddOrUpdate(string key, ICredential credential) { Marshal.Copy(passwordBytes, 0, w32Credential.CredentialBlob, passwordBytes.Length); - ThrowOnError( - CredWrite(ref w32Credential, 0), - "Failed to write item to store." + int result = GetLastError( + CredWrite(ref w32Credential, 0) ); + + ThrowIfError(result, "Failed to write item to store."); } finally { @@ -96,14 +102,21 @@ public void AddOrUpdate(string key, ICredential credential) public bool Remove(string key) { - try - { - ThrowOnError(CredDelete(key, CredentialType.Generic, 0)); - return true; - } - catch (KeyNotFoundException) + int result = GetLastError( + CredDelete(key, CredentialType.Generic, 0) + ); + + switch (result) { - return false; + case OK: + return true; + + case ERROR_NOT_FOUND: + return false; + + default: + ThrowIfError(result); + return false; } }