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.

Sign up for GitHub

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

Jump to bottom

Workaround MSAL.NET issue with MSA-PT account silent auth #1321

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Workaround MSAL.NET issue with MSA-PT account silent auth #1321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99dea8e
msauth: let caller select if MSA-PT behaviour is used
mjcheetham Jul 10, 2023
68bcc34
msauth: workaround MSAL.NET issue with MSA-PT accounts
mjcheetham Jul 10, 2023
725ab49
msauth: add extra tracing of MSA-PT and ATS
mjcheetham Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/shared/Core.Tests/Authentication/MicrosoftAuthenticationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public async System.Threading.Tasks.Task MicrosoftAuthentication_GetAccessTokenA
var msAuth = new MicrosoftAuthentication(context);

await Assert.ThrowsAsync<Trace2InvalidOperationException>(
() => msAuth.GetTokenAsync(authority, clientId, redirectUri, scopes, userName));
() => msAuth.GetTokenAsync(authority, clientId, redirectUri, scopes, userName, false));
}
}
}
60 changes: 47 additions & 13 deletions src/shared/Core/Authentication/MicrosoftAuthentication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using GitCredentialManager.Interop.Windows.Native;
Expand All @@ -23,7 +24,7 @@ namespace GitCredentialManager.Authentication
public interface IMicrosoftAuthentication
{
Task<IMicrosoftAuthenticationResult> GetTokenAsync(string authority, string clientId, Uri redirectUri,
string[] scopes, string userName);
string[] scopes, string userName, bool msaPt = false);
}

public interface IMicrosoftAuthenticationResult
Expand Down Expand Up @@ -59,26 +60,31 @@ public MicrosoftAuthentication(ICommandContext context)
#region IMicrosoftAuthentication

public async Task<IMicrosoftAuthenticationResult> GetTokenAsync(
string authority, string clientId, Uri redirectUri, string[] scopes, string userName)
string authority, string clientId, Uri redirectUri, string[] scopes, string userName, bool msaPt)
{
// Check if we can and should use OS broker authentication
bool useBroker = CanUseBroker();
Context.Trace.WriteLine(useBroker
? "OS broker is available and enabled."
: "OS broker is not available or enabled.");

if (msaPt)
{
Context.Trace.WriteLine("MSA passthrough is enabled.");
}

try
{
// Create the public client application for authentication
IPublicClientApplication app = await CreatePublicClientApplicationAsync(authority, clientId, redirectUri, useBroker);
IPublicClientApplication app = await CreatePublicClientApplicationAsync(authority, clientId, redirectUri, useBroker, msaPt);

AuthenticationResult result = null;

// Try silent authentication first if we know about an existing user
bool hasExistingUser = !string.IsNullOrWhiteSpace(userName);
if (hasExistingUser)
{
result = await GetAccessTokenSilentlyAsync(app, scopes, userName);
result = await GetAccessTokenSilentlyAsync(app, scopes, userName, msaPt);
}

//
Expand Down Expand Up @@ -116,7 +122,7 @@ public MicrosoftAuthentication(ICommandContext context)
// account then the user may become stuck in a loop of authentication failures.
if (!hasExistingUser && Context.Settings.UseMsAuthDefaultAccount)
{
result = await GetAccessTokenSilentlyAsync(app, scopes, null);
result = await GetAccessTokenSilentlyAsync(app, scopes, null, msaPt);

if (result is null || !await UseDefaultAccountAsync(result.Account.Username))
{
Expand Down Expand Up @@ -281,34 +287,62 @@ internal MicrosoftAuthenticationFlowType GetFlowType()
/// <summary>
/// Obtain an access token without showing UI or prompts.
/// </summary>
private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(IPublicClientApplication app, string[] scopes, string userName)
private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(
IPublicClientApplication app, string[] scopes, string userName, bool msaPt)
{
try
{
if (userName is null)
{
Context.Trace.WriteLine("Attempting to acquire token silently for current operating system account...");
Context.Trace.WriteLine(
"Attempting to acquire token silently for current operating system account...");

return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount).ExecuteAsync();
return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount)
.ExecuteAsync();
}
else
{
Context.Trace.WriteLine($"Attempting to acquire token silently for user '{userName}'...");

// We can either call `app.GetAccountsAsync` and filter through the IAccount objects for the instance with the correct user name,
// or we can just pass the user name string we have as the `loginHint` and let MSAL do exactly that for us instead!
return await app.AcquireTokenSilent(scopes, loginHint: userName).ExecuteAsync();
// Enumerate all accounts and find the one matching the user name
IEnumerable<IAccount> accounts = await app.GetAccountsAsync();
IAccount account = accounts.FirstOrDefault(x =>
StringComparer.OrdinalIgnoreCase.Equals(x.Username, userName));
if (account is null)
{
Context.Trace.WriteLine($"No cached account found for user '{userName}'...");
return null;
}

var atsBuilder = app.AcquireTokenSilent(scopes, account);

// Is we are operating with an MSA passthrough app we need to ensure that we target the
// special MSA 'transfer' tenant explicitly. This is a workaround for MSAL issue:
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: I'm wondering whether there's any public documentation available about the transfer tenant that we can link here for those who would like to learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately MSA passthrough is an internal (and legacy) feature of the Microsoft Identity platform, and the only docs are for Microsoft employees only: https://review.learn.microsoft.com/en-us/identity/microsoft-identity-platform/stack-choices?branch=main#msa-passthrough

// https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3077
if (msaPt && Guid.TryParse(account.HomeAccountId.TenantId, out Guid homeTenantId) &&
homeTenantId == Constants.MsaHomeTenantId)
{
atsBuilder = atsBuilder.WithTenantId(Constants.MsaTransferTenantId.ToString("D"));
}

return await atsBuilder.ExecuteAsync();
}
}
catch (MsalUiRequiredException)
{
Context.Trace.WriteLine("Failed to acquire token silently; user interaction is required.");
return null;
}
catch (Exception ex)
{
Context.Trace.WriteLine("Failed to acquire token silently.");
Context.Trace.WriteException(ex);
return null;
}
}

private async Task<IPublicClientApplication> CreatePublicClientApplicationAsync(
string authority, string clientId, Uri redirectUri, bool enableBroker)
string authority, string clientId, Uri redirectUri, bool enableBroker, bool msaPt)
{
var httpFactoryAdaptor = new MsalHttpClientFactoryAdaptor(Context.HttpClientFactory);

Expand Down Expand Up @@ -370,7 +404,7 @@ private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(IPublicClie
new BrokerOptions(BrokerOptions.OperatingSystems.Windows)
{
Title = "Git Credential Manager",
MsaPassthrough = true,
MsaPassthrough = msaPt,
}
);
#endif
Expand Down
11 changes: 11 additions & 0 deletions src/shared/Core/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,17 @@ public static class Constants

public static readonly Guid DevBoxPartnerId = new("e3171dd9-9a5f-e5be-b36c-cc7c4f3f3bcf");

/// <summary>
/// Home tenant ID for Microsoft Accounts (MSA).
/// </summary>
public static readonly Guid MsaHomeTenantId = new("9188040d-6c67-4c5b-b112-36a304b66dad");

/// <summary>
/// Special tenant ID for transferring between Microsoft Account (MSA) native tokens
/// and AAD tokens. Only required for MSA-Passthrough applications.
/// </summary>
public static readonly Guid MsaTransferTenantId = new("f8cdef31-a31e-4b4a-93e4-5f571e91255a");

public static class CredentialStoreNames
{
public const string WindowsCredentialManager = "wincredman";
Expand Down
14 changes: 7 additions & 7 deletions src/shared/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -219,7 +219,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -268,7 +268,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -315,7 +315,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
var azDevOpsMock = new Mock<IAzureDevOpsRestApi>(MockBehavior.Strict);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -363,7 +363,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
var azDevOpsMock = new Mock<IAzureDevOpsRestApi>(MockBehavior.Strict);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -413,7 +413,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_NoCachedAuthorit
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -462,7 +462,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_NoExistingPat_Ge
.ReturnsAsync(personalAccessToken);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down
6 changes: 4 additions & 2 deletions src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ private async Task<ICredential> GeneratePersonalAccessTokenAsync(InputArguments
GetClientId(),
GetRedirectUri(),
AzureDevOpsConstants.AzureDevOpsDefaultScopes,
null);
null,
msaPt: true);
_context.Trace.WriteLineSecrets(
$"Acquired Azure access token. Account='{result.AccountUpn}' Token='{{0}}'", new object[] {result.AccessToken});

Expand Down Expand Up @@ -293,7 +294,8 @@ private async Task<IMicrosoftAuthenticationResult> GetAzureAccessTokenAsync(Inpu
GetClientId(),
GetRedirectUri(),
AzureDevOpsConstants.AzureDevOpsDefaultScopes,
userName);
userName,
msaPt: true);
_context.Trace.WriteLineSecrets(
$"Acquired Azure access token. Account='{result.AccountUpn}' Token='{{0}}'", new object[] {result.AccessToken});

Expand Down