I've had a look over the source code for the SecureClientSecret
SecureClientSecret
I'm a bit confused as to what Microsoft are trying to do with their ApplyTo method, it looks a bit like the kind of pattern you see with TryParse and out params, populating a dictionary by reference.
More explicity, populating the dictionary value at OAuthParameter.ClientSecret ('client_secret'as that translates to), with the decrypted value of the SecureString stashed away inside SecureClientSecret.
Microsoft then clear down the private SecureString member, which means any further calls to ApplyTo are going to be met with an unhandled exception when we hit the first line:
var output = new char[secureString.Length];
'secureString.Length' is going to blow, the exception you'd expect, calling null.Length... System.NullReferenceException, something that we see happening in the stack trace.
If MS really need to clear away that member field after a single use for some reason that I don't understand at the moment, maybe they should consider making the class more robust.
Add an IsApplied method/property to the interface:
public interface ISecureClientSecret
{
/// <summary>
/// Writes SecureString to the dictionary.
/// </summary>
/// <param name="parameters"></param>
void ApplyTo(IDictionary<string, string> parameters);
/// <summary>
/// Indicates whether this <see cref="ISecureClientSecret"/> has already been applied.
/// </summary>
/// <returns><c>true</c> if applied, otherwise <c>false</c></returns>
bool IsApplied { get; };
}
Which could easily be implemented in the concrete class with:
public bool IsApplied => secureString == null;
Also, the ApplyTo method could make use of locking to prevent bad things happening with lots of calls:
public void ApplyTo(IDictionary<string, string> parameters)
{
secureLock.WaitOne();
// We don't want to blow up if ApplyTo is called multiple times.
if (IsApplied)
return;
IntPtr ptr = IntPtr.Zero;
try
{
ptr = Marshal.SecureStringToCoTaskMemUnicode(secureString);
parameters[OAuthParameter.ClientSecret] = Marshal.PtrToStringUni(ptr);
}
finally
{
if (ptr != IntPtr.Zero)
Marshal.ZeroFreeCoTaskMemUnicode(ptr);
if (secureString != null && !secureString.IsReadOnly())
secureString.Clear();
secureString = null;
secureLock.Set();
}
}
Sticking a AutoResetEvent in as a member for the class.
private AutoResetEvent secureLock = new AutoResetEvent(true);
It would be great if one of the devs on this project could explain what they're trying to do as things seem a little convoluted as a newbie looking in.
UPDATE
As @Nan Yu previously said, I'd like to see the poster's calling code for the delegate that provides the auth. After playing around with my own version I'm wondering if making sure the vars related to the SecureClientSecret are scoped as low down as possible would get around the potential issues I discussed above.
I.e. Don't have any member/field references to the SecureClientSecret. Don't have any vars scoped outside the delegate itself either, as I think the underlying SecureString inside the SecureClientSecret object gets nulled and things don't work out well when you try to get another token passing in the original SecureString.
I believe this is essentially what @'Kevin ssssssss' was suggesting in their answer!
var delegateAuthProvider = new DelegateAuthenticationProvider(async (requestMessage) =>
{
var authContext = new AuthenticationContext($"https://login.microsoftonline.com/{tenant}/oauth2/token");
var result = await authContext.AcquireTokenAsync(Resource, new ClientCredential(appClientId, new SecureClientSecret(appSecretSecure)));
requestMessage.Headers.Authorization = new AuthenticationHeaderValue("bearer", result.AccessToken);
});
MS samples seem to recommend calling for a new token every time before making a Graph request, which personally seems like a waste to me, also if you end up making a lot of async calls (i.e await Task.WhenAll(someTasks);) you might end up with a race-like condition where every task wants to call for a token, and you'll get a lot of open sockets as the tasks get scheduled. Here's a snippet of some code I'd use when trying to deal with the token/delegate etc.
namespace GraphTestConsole
{
using Microsoft.Graph;
using Microsoft.IdentityModel.Clients.ActiveDirectory;
using Nito.AsyncEx;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http.Headers;
using System.Security;
using System.Threading.Tasks;
public class Program
{
private const string Resource = "https://graph.microsoft.com";
private const string AppClientId = "YOUR_CLIENT_ID";
private static readonly SecureString AppClientSecretSecure = ToSecureString("YOUR_CLIENT_SECRET");
private const string Tenant = "YOUR_TENANT.onmicrosoft.com";
private static IGraphServiceClient _graphClient;
private static string _token;
private static DateTimeOffset _tokenExpiresOn = DateTime.Now;
private static readonly AsyncLock _mutex = new AsyncLock();
public static void Main(string[] args)
{
AsyncContext.Run(() => AsyncMain(AppClientId, AppClientSecretSecure, Tenant));
}
private static async Task AsyncMain(string appId, SecureString appSecretSecure, string tenant)
{
try
{
var upn = "YOUR_USER_UPN";
_graphClient = GetGraphClient(appId, appSecretSecure, tenant);
var user = await _graphClient.Users[upn].Request().Select("id").GetAsync();
}
catch (Exception ex)
{
// Handle the exception...
Console.WriteLine(DateTime.Now.ToString());
Console.WriteLine(ex);
}
}
private static bool IsTokenInvalid => string.IsNullOrEmpty(_token) || (_tokenExpiresOn - DateTime.Now).TotalSeconds <= 0;
private static GraphServiceClient GetGraphClient(string appClientId, SecureString appSecretSecure, string tenant)
{
var delegateAuthProvider = new DelegateAuthenticationProvider(async (requestMessage) =>
{
using (await _mutex.LockAsync())
{
if (IsTokenInvalid)
{
var authContext = new AuthenticationContext($"https://login.microsoftonline.com/{tenant}/oauth2/token");
var result = await authContext.AcquireTokenAsync(Resource, new ClientCredential(appClientId, new SecureClientSecret(appSecretSecure)));
_token = result.AccessToken;
_tokenExpiresOn = result.ExpiresOn;
}
requestMessage.Headers.Authorization = new AuthenticationHeaderValue("bearer", _token);
}
});
return new GraphServiceClient(delegateAuthProvider);
}
private static SecureString ToSecureString(IEnumerable<char> value)
{
if (value == null)
throw new ArgumentNullException(nameof(value));
var secured = new SecureString();
var charArray = value.ToArray();
for (int i = 0; i < charArray.Length; i++)
{
secured.AppendChar(charArray[i]);
}
secured.MakeReadOnly();
return secured;
}
}
}
P.S. I still think the MS ADAL code is poorly written. :)
