Skip to content
This repository has been archived by the owner on Jun 30, 2023. It is now read-only.

ADAL 5.4.2 Client package is dropping /adfs/ literal from the passed authority URI #1693

Closed
1 of 7 tasks
sumantshiv opened this issue Jan 31, 2020 · 6 comments
Closed
1 of 7 tasks

Comments

@sumantshiv
Copy link

sumantshiv commented Jan 31, 2020

MSAL is the recommended auth library for use with the Microsoft identity platform

No new features will be implemented on ADAL. The team's efforts are on improving MSAL, the next-gen auth library. MSAL's wiki contains a migration guide from ADAL.

Only regressions, high severity issues and security issues will be fixed on ADAL. Other issues are likely to have already been fixed in MSAL.

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

Which Version of ADAL are you using ?
Note that to get help, you need to run the latest preview or non-preview version
For MSAL, please log issues to https://github.com/AzureAD/microsoft-authentication-library-for-dotnet

ADAL 5.2.4

Which platform has the issue?
net452

What authentication flow has the issue?

  • Desktop / Mobile
    • Interactive
    • Integrated Windows Auth
    • Username Password
    • Device code flow (browserless)
  • Web App
    • Authorization code
    • OBO
  • Web API
    • OBO

Other? - please describe;

Is this a new or existing app?

Azure Key vault -
The app is in production, and I have upgraded to a new version of ADAL from 4.19.X/4.5.1 to 5.4.2

Azure Key vault VSTS Repo

var your = (code) => here;

Expected behavior
ADAL Client library should continue to goto the specified Authority URL rather than dropping literals from it.

Actual behavior
Azure Key Vault is looking to update the ADAL client library nuget primarily from 3.19.X/4.5.1 to 5.4.2. We need this updated version to support our upcoming features. However, while testing we hit an issue with respect to the Get Token API. In case the callback Authority has the literal /adfs/, the ADAL client library for some reason is dropping it.
Hence, in case the Authority URL is: https://AuthEndpointBase/adfs/TenantId, the ADAL client is dropping this literal and actually sending the request to https://AuthEndpointBase/TenantId.

Possible Solution

Additional context/ Logs / Screenshots
Add any other context about the problem here, such as logs and screebshots. Logging is described at https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/wiki/Logging-in-ADAL.Net. Don't post logs containing PII on GitHub!

@jmprieur jmprieur added the bug label Jan 31, 2020
@jmprieur jmprieur self-assigned this Jan 31, 2020
@jmprieur jmprieur added this to the 5.2.6 milestone Feb 1, 2020
@jmprieur jmprieur removed their assignment Feb 1, 2020
@jmprieur
Copy link
Contributor

jmprieur commented Feb 1, 2020

From investigation from @sumantshiv
commit c0f5b4e has updated the logic around parsing the tenantId from the AuthorityURL. The ADAL library always formats the endpoint url as https://host/tenantID format.

If the authority URL is like: https://localhost/adfs/tenantID.

  • As per the previous parsing logic, the tenant ID was coming as “adfs”. Picking that up, the ADAL client was reaching out to the endpoint https://localhost/adfs. This was working fine.
  • In the new parsing logic (call it a fix), the tenant ID because “tenantID” and the ADAL client started to reach out for a server endpoint https://localhost/tenantID. This started to break.

@sumantshiv has cases where they need the server endpoint format as: https://localhost/adfs or https://localhost/graph.

The changed file is: https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/blob/c0f5b4edf166861db1f56d319877a212921ae055/adal/src/Microsoft.IdentityModel.Clients.ActiveDirectory/Internal/Instance/Authenticator.cs

Tenant ID parsing is at Line number 108

cc: @trwalke @henrik-me @jennyf19 @bgavrilMS

@sumantshiv
Copy link
Author

Updated the typo in Version. It should be 5.2.4.

@bgavrilMS bgavrilMS self-assigned this Feb 1, 2020
@bgavrilMS
Copy link
Member

I'll take a look at this.

@jmprieur jmprieur assigned jennyf19 and unassigned bgavrilMS Feb 6, 2020
@keystroke
Copy link

@jmprieur / @bgavrilMS this is the same thing discussed in #1653, not sure if you want to consolidate. The preference from keyvault team is that all the identity clients are updated to accommodate the spurious authority endpoint.

@jennyf19
Copy link
Contributor

@sumantshiv @keystroke I have a private build you can try out if you're interested. just send me an email -> jeferrie@microsoft.com

@jennyf19 jennyf19 added the Fixed label Feb 12, 2020
@jennyf19 jennyf19 modified the milestones: 5.2.6, 5.2.7 Feb 13, 2020
@jennyf19
Copy link
Contributor

Included in 5.2.7 release

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

No branches or pull requests

6 participants