AzureAD/azure-activedirectory-library-for-dotnet

ADAL incorrectly forms AuthorityUri, ADFS returns 404 when authenticating KeyVaultclient

mancyc opened this issue · 7 comments

mancyc commented

If you think that your issue falls into the above categories, please fill in the form below.

Which Version of ADAL are you using ?
ADAL 4.5.1

Which platform has the issue?
net45

What authentication flow has the issue?
ADFS

Other? - please describe;
ADAL changed the way they form authority URI,
Earlier: string tenant = path.Substring(0, path.IndexOf("/", StringComparison.Ordinal));
Now: string tenant = authorityUri.Segments[authorityUri.Segments.Length - 1].TrimEnd('/');

This causes a 404 from ADFS when KeyVault client tries to authenticate. KeyVault advertises “/adfs/” in it's WWW-Authenticate header. This works using older version of ADAL.NET (~< 3.5) because it had the behavior to “ignore” additional segments of the URI provided after the first. So KV would provide an authority value of “/adfs/” but ADAL.NET would only take the first section (/adfs) and ignore the rest, thus resulting in a working solution. The later versions of ADAL.NET have changed above logic; it instead takes the “final” segment from the input authority and use that as the tenantId; so in the case of KV, it takes the input of “/adfs/” and trim it down to just “/” whereas ADFS expects /adfs

Is this a new or existing app?
a. The app is in production, and I have upgraded to a new version of ADAL

Expected behavior
string tenant = path.Substring(0, path.IndexOf("/", StringComparison.Ordinal));
in Authenticator.cs

Actual behavior
404 from ADFS since authorityUri is wrongly formed

Possible Solution
string tenant = path.Substring(0, path.IndexOf("/", StringComparison.Ordinal));

jmprieur commented

Thanks @mancyc for this heads-up.

@henrik-me @trwalke @MarkZuber @bgavrilMS @jennyf19
I'd like to take this one. See also #1410 and #1455

-- reality is stuborn ....

bgavrilMS commented

@mancyc - can you give me an example of the ADFS url that KV advertises (not necessarily your own server)

keystroke commented

I'm not sure what the right behavior is here, I'm not sure if ADAL needs to do anything TBH.

The problem is that the KeyVault services is advertising a spurious WWW-Authenticate header, indicating that the authority URI one should use when requesting a token to talk to it's vault endpoints is "/adfs/tenantId". But of course, the correct endpoint for ADFS token endpoint is just "/adfs". So when people were using older versions of ADAL (<3.5), under the hood, an authority value of "/adfs/tenantId" was being provided to the ADAL authentication context. Now, a path with two segments for authority is not valid, so in versions <3.5 of ADAL, the code "decided" to just take the first segment of the path, and ignore everything else. After version 3.5, this code changed to instead take the final segment if more than one is provided. This issue is representing that change in behavior.

ADAL could've just been throwing an exception this whole time (e.g. "invalid authority provided"), so the idea that the client code should be able to continue providing invalid authority URIs in the old way is kind of bizarre. The old behavior allowed "lazy code" to "get away" with not being precise in how the authority value is being passed. In fact, I've seen code that just always disabled authority validation as well, so work for both AAD and ADFS authority endpoints without branching in the code.

I suspect a more-correct fix would be to fix the KeyVault service itself. We are going to have this same problem in MSAL.net library as well, but it's even worse there because you have to call two separate builder methods (.WithAuthority or .WithAdfsAuthority) so any code wishing to upgrade to use MSAL.net is going to have to branch their code to handle the proper fluent API, and if they are branching their code, they can pass the correct authority value.

Above is less than ideal though... this means that when implementing an authentication callback in your code, you would have to try and parse the authority URI value provided by the discovery framework (talking about KeyVault scenarios here), and then branching based-off that. Do you think MSAL could internally try and parse the supplied authority URI, see that it starts with "/adfs" in the path, and just ignore anything else and know internally to use the "ADFS code path"? Or should clients be doing the parsing and switching?

Above applies to MSAL.net, but for ADAL, what would be the risk of changing this behavior at this point? I'm not sure how much code exists out there that is passing an authority URI that is more than one segment, and if so, how they would be relying on the final segment to be the tenantId instead of the first.

keystroke commented

Below code snippet is one I had ready to go to test some port-picking behavior in ADAL.net for ADFS systems, it can show the behavior here in this scenario as well (make sure if you run yourself you do so in fresh PowerShell process to ensure your desired version of ADAL is loaded):

$ErrorActionPreference='Stop'
try
{
    ($assembly = [System.Reflection.Assembly]::LoadFrom('D:\One\AzureUX\Katal\packages\Microsoft.IdentityModel.Clients.ActiveDirectory.5.1.1\lib\net45\Microsoft.IdentityModel.Clients.ActiveDirectory.dll'))

    Add-Type 'public static class PSLOG { public static Microsoft.IdentityModel.Clients.ActiveDirectory.LogCallback Log = (a, b, c) => System.Console.WriteLine(b); }' -ReferencedAssemblies $assembly.FullName

    [Microsoft.IdentityModel.Clients.ActiveDirectory.LoggerCallbackHandler]::LogCallback = [PSLOG]::Log

    $context = [Microsoft.IdentityModel.Clients.ActiveDirectory.AuthenticationContext]::new(
        ($authority = 'https://localhost/adfs/49b31cb2-40a5-4d7d-86b9-06931fcd7c5a'),
        ($validateAuthority = $false),
        ($tokenCache = [Microsoft.IdentityModel.Clients.ActiveDirectory.TokenCache]::new()))

    $result = [Microsoft.IdentityModel.Clients.ActiveDirectory.AuthenticationContextIntegratedAuthExtensions]::AcquireTokenAsync(
        $context,
        ($resource='https://graph.windows.net/'),
        ($clientId='1950a258-227b-4e31-a9cf-717495945fc2'),
        ($userCredential=[Microsoft.IdentityModel.Clients.ActiveDirectory.UserPasswordCredential]::new('test', 'test')))

    $result.GetAwaiter().GetResult()
}
catch
{
    throw "$_`r`n$($_.Exception)"
}

I wasn't able to spy on the traffic via fiddler, and the logging option didn't explicitly log URIs or anything, but I can cross-reference to my IIS logs on my localhost website.

Using "https[:]//localhost/adfs" and "https[:]//localhost/adfs/" both result in same behavior, an incoming call to "/adfs/oauth2/token" which is correct for a real ADFS endpoint:

  • 2019-09-25 23:08:57 127.0.0.1 POST /adfs/oauth2/token - 443 - 127.0.0.1 - - 404
  • 2019-09-25 23:10:21 127.0.0.1 POST /adfs/oauth2/token - 443 - 127.0.0.1 - - 404

Using the values from the issue, using "https[:]//localhost/adfs/49b31cb2-40a5-4d7d-86b9-06931fcd7c5a" and "https[:]//localhost/adfs/49b31cb2-40a5-4d7d-86b9-06931fcd7c5a/" both result in same behavior, an incoming call to "/tenant/oauth2/token" which is not correct for a real ADFS endpoint:

  • 2019-09-25 23:13:08 127.0.0.1 POST /49b31cb2-40a5-4d7d-86b9-06931fcd7c5a/oauth2/token - 443 - 127.0.0.1 - - 404
  • 2019-09-25 23:13:28 127.0.0.1 POST /49b31cb2-40a5-4d7d-86b9-06931fcd7c5a/oauth2/token - 443 - 127.0.0.1 - - 404

Note that my above testing is with versions 5.1.1, I think you have a newer version out but this behavior is not changed AFAIK.

The problem is that only "/adfs" is valid for authority; "/tenantId" or anything else is wrong, and KeyVault service is advertising a wrong authority URI for the purposes of acquiring a token in ADFS-backed clouds.

henrik-me commented

@keystroke : thanks a lot for the details. You raise a number of good questions. MSAL.NET should support setting just the authority in what makes the life the easiest for the developer, however because these are very different from IDP's doing validation and helping setting things up correctly we have the different helper methods. For things like this feel free to suggest a feature on MSAL .NET.
We discussed a lot throwing exceptions if the authority wasn't considered valid.
For the logging, did you set log level to Information? You should be able to get a lot of details doing so.
CC: @jmprieur @bgavrilMS : let's discuss

jennyf19 commented

should be resolved with 5.2.7 release