Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add secure credential storage APIs for Windows+macOS #2

Merged
merged 7 commits into from
Dec 7, 2018
28 changes: 28 additions & 0 deletions src/Microsoft.Git.CredentialManager/CommandContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.IO;
using System.Linq;
using System.Text;
using Microsoft.Git.CredentialManager.SecureStorage;

namespace Microsoft.Git.CredentialManager
{
Expand Down Expand Up @@ -37,6 +38,11 @@ public interface ICommandContext
/// </summary>
IFileSystem FileSystem { get; }

/// <summary>
/// Secure credential storage.
/// </summary>
ICredentialStore CredentialStore { get; }

/// <summary>
/// Access the environment variables for the current GCM process.
/// </summary>
Expand Down Expand Up @@ -110,6 +116,8 @@ public TextWriter StdError

public IFileSystem FileSystem { get; } = new FileSystem();

public ICredentialStore CredentialStore { get; } = GetDefaultCredentialStore();

public IReadOnlyDictionary<string, string> GetEnvironmentVariables()
{
IDictionary variables = Environment.GetEnvironmentVariables();
Expand Down Expand Up @@ -140,6 +148,26 @@ public IReadOnlyDictionary<string, string> GetEnvironmentVariables()
}

#endregion

private static ICredentialStore GetDefaultCredentialStore()
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
{
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
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Git.CredentialManager/GitCredential.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Microsoft.Git.CredentialManager
/// <summary>
/// Represents a credential (username/password pair) that Git can use to authenticate to a remote repository.
/// </summary>
public class GitCredential
public class GitCredential : SecureStorage.ICredential
{
public GitCredential(string userName, string password)
{
Expand Down
23 changes: 23 additions & 0 deletions src/Microsoft.Git.CredentialManager/SecureStorage/Credential.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
namespace Microsoft.Git.CredentialManager.SecureStorage
{
/// <summary>
/// Represents a simple credential; user name and password pair.
/// </summary>
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; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
using System.Collections.Generic;

namespace Microsoft.Git.CredentialManager.SecureStorage
{
/// <summary>
/// Represents a secure storage location for <see cref="ICredential"/>s.
/// </summary>
public interface ICredentialStore
{
/// <summary>
/// Get credential from the store with the specified key.
/// </summary>
/// <param name="key">Key for credential to retrieve.</param>
/// <returns>Stored credential.</returns>
/// <exception cref="KeyNotFoundException">Thrown if no credential exists in the store with the specified key.</exception>
ICredential Get(string key);

/// <summary>
/// Add or update credential in the store with the specified key.
/// </summary>
/// <param name="key">Key for credential to add/update.</param>
/// <param name="credential">Credential to store.</param>
void AddOrUpdate(string key, ICredential credential);

/// <summary>
/// Delete credential from the store with the specified key.
/// </summary>
/// <param name="key">Key of credential to delete.</param>
/// <returns>True if the credential was deleted, false otherwise.</returns>
bool Remove(string key);
}
}
222 changes: 222 additions & 0 deletions src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Original file line number Diff line number Diff line change
@@ -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

/// <summary>
/// Open the default keychain (current user's login keychain).
/// </summary>
/// <returns>Default keychain.</returns>
public static MacOSKeychain OpenDefault()
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
{
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;
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
}
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check return value here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so since in either case (this call fails or this call succeeds) we either try and update an existing entry (itemRef != null) or create a new one.

In both of these paths we're doing ThrowOnError so any generic errors like access exceptions or 'no such keychain' will be checked & thrown as managed exceptions there.

What do you think? To add the check we'll need to filter out the 'expected error' that the key does not exist already so we can create it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... what other errors could there be? If we did hit a problem failing to find the key (that was specific to find), then it would not be surfaced and it might be harder to debug. Maybe there are no other errors that we need to be aware of.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamill I've updated the approach for error handling in other the Windows and Mac stores.

I am now always checking the return values of any P/Invoke call, and am explicitly filtering out any 'expected' error codes (such as 'not found').

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(
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do callers need to discern between "failed to remove" and "nothing to remove"? I was trying to think how callers might handle different return values...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so since there's not really much we (GCM) can do if we fail to delete the stored credential (as advised by Git).

Except for get, I don't think Git cares about the output of store and erase commands (I might be mistaken there).

}
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));
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
Marshal.Copy(new[] {(int) SecKeychainAttrType.AccountItem}, 0, tagArrayPtr, 1);
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved

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<SecKeychainAttributeList>(attrListPtr);
Debug.Assert(attrList.Count == 1);
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved

byte[] attrListArrayBytes = NativeMethods.ToByteArray(
attrList.Attributes, Marshal.SizeOf<SecKeychainAttribute>() * attrList.Count);

SecKeychainAttribute[] attributes = NativeMethods.ToStructArray<SecKeychainAttribute>(attrListArrayBytes);
mjcheetham marked this conversation as resolved.
Show resolved Hide resolved
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
}
}
Loading