Add secure credential storage APIs for Windows+macOS #2
Conversation
Add secure credential storage APIs for the Windows Credential Manager and the macOS Keychain.
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.Mac.cs
Outdated
Show resolved
Hide resolved
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.
|
Closing and reopening to kick off an PR build... |
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs
Show resolved
Hide resolved
tests/Microsoft.Git.CredentialManager.Tests/SecureStorage/MacOSKeychainTests.cs
Outdated
Show resolved
Hide resolved
- Add messages to the `Debug.Assert` calls in `MacOSKeychain`. - Rename the `GetDefaultCredentialStore` helper method to `CreateCredentialStore` in `CommandContext`.
| try | ||
| { | ||
| // Check if an entry already exists in the keychain | ||
| SecKeychainFindGenericPassword( |
There was a problem hiding this comment.
Do we need to check return value here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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').
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
| } | ||
| catch (KeyNotFoundException) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
| { | ||
| Type = CredentialType.Generic, | ||
| TargetName = key, | ||
| CredentialBlob = Marshal.AllocCoTaskMem(passwordBytes.Length), |
There was a problem hiding this comment.
AllocCoTaskMem or AllocHGlobal?
There was a problem hiding this comment.
This code was copied from the .NET Framework version of GCM so I didn't want to change that since we know that's working; it's been battle tested.
It might very well be wrong, but are we sure? Are the WinCredManager APIs using COM at some point?
There was a problem hiding this comment.
It might very well be wrong, but are we sure?
With the re-write, I wanted to double check some of the assumptions from the original code (at least the parts that feel wrong). I would prefer to understand the differences between the two calls and which one we should use (from 1st principles)
There was a problem hiding this comment.
I believe you might be correct here.. the CredRead, CredWrite, and CredDelete calls are all Win32 APIs which are not COM.
I've updated the code here to use the global heap allocator which is confirmed to be working fine in the tests.
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Git.CredentialManager/SecureStorage/NativeMethods.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Git.CredentialManager/SecureStorage/MacOSKeychain.cs
Outdated
Show resolved
Hide resolved
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.
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
Simplify some of the marshaling code between native and managed by using more methods from the `Marshal` type, rather than using our own helpers.
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.
mjcheetham
added
enhancement
install from source: refactor install location and fix path
Add secure credential storage APIs for:
..using the native syscalls on each platform. Should we wish to support Linux in the future, options such as GNOME Keyring could be used.