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.

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

Migrate to MSAL from adal-node #2782

Closed
stevengum opened this issue Sep 16, 2020 · 12 comments · Fixed by #4548
Closed

Migrate to MSAL from adal-node #2782

stevengum opened this issue Sep 16, 2020 · 12 comments · Fixed by #4548
Assignees
Labels
Area: Authentication The issue is related to authenticating users (SSO, OAuth, etc.) Area: SDK General SDK issues that don't clearly map to other areas (e.g.: helper methods) no parity This issue applies only to this platform P0 Must Fix. Release-blocker Size: M The issue is not very complex and it is well understood, it will take 1 to 3 days to complete

Comments

@stevengum
Copy link
Member

On inspection of AzureAD/microsoft-authentication-library-for-js#2023, there is now support for using client certificates in MSAL, which means we can continue moving away from adal-node.

@Zerryth, please reach out to either @carlosscastro or myself for any questions.

Tentatively placing in R11 milestone.

@stevengum stevengum added no parity This issue applies only to this platform needs-triage The issue has just been created and it has not been reviewed by the team. Area: SDK General SDK issues that don't clearly map to other areas (e.g.: helper methods) labels Sep 16, 2020
@stevengum stevengum added this to the R11 milestone Sep 16, 2020
@stevengum stevengum added P2 Nice to have and removed needs-triage The issue has just been created and it has not been reviewed by the team. labels Sep 23, 2020
@stevengum
Copy link
Member Author

Marking as P2 since we're not blocking anyone by not performing this migration in R11.

@Zerryth, let's chat offline about the plan for this work.

@joshgummersall joshgummersall modified the milestones: R11, R12 Oct 15, 2020
@junczhu
Copy link

junczhu commented Nov 26, 2020

Hello mate, I want to find out if there is any update on this open issue? BR

@joshgummersall
Copy link
Contributor

No updates yet, this work is scheduled for the next release of the SDK which means it should be picked up relatively soon, perhaps in a few weeks.

@junczhu
Copy link

junczhu commented Dec 2, 2020

No updates yet, this work is scheduled for the next release of the SDK which means it should be picked up relatively soon, perhaps in a few weeks.

Thank you so much for the response. 🤞

@Zerryth
Copy link
Contributor

Zerryth commented Feb 1, 2021

From @joshgummersall 's comment in PR#3247

@Zerryth are you still planning on resolving #2782 as well?

Short answer is yes. 😅

I'll go ahead and start working on it, since in the MSAL migration FAQs it says that ADAL is deprecating June 30th, 2022, and that the recommended authentication library to use with Microsoft identity platform is MSAL. However I was hesitant at first (which is why I made a separate PR in 3247), because:

  • Looking at our SDK, it looks like we use:
    • client credentials grant type (regular channel <--> bot communication)
    • and authorization code grant type (for scenarios when we have a user sign in using OAuthPrompt)
  • Speaking to the MSAL.js team, it looks like they released the client credentials flow, however it's available in their msal-node package, which is still in preview (other packages in MSAL.js have GA'd, just not this one)
    • In the past Chris Mullins had rejected merging in other preview features, like QnAMaker multi-turn prompt, when it was still in preview, even though QnAMaker was something that we already had other parts of integrated in our SDK
    • I spoke to Steven G. and says I should try implementing this migration regardless, given the recommendation on migration in their docs

If anyone else feels strongly otherwise about folding in the preview MSAL package, LMK and we can hold off--seems like we do have runway time until mid-2022 anyways

@joshgummersall
Copy link
Contributor

I didn't realize it wasn't a GA package yet - seems fine to hold off for now then.

joshgummersall pushed a commit that referenced this issue Jul 2, 2021
joshgummersall pushed a commit that referenced this issue Jul 6, 2021
joshgummersall pushed a commit that referenced this issue Jul 6, 2021
joshgummersall pushed a commit that referenced this issue Jul 7, 2021
joshgummersall pushed a commit that referenced this issue Jul 9, 2021
@kinder-lab
Copy link

Is there any active work on this issue? adal-node has a dependency on xmldom which has security vulnerability CVE-2021-32796 in versions <= 0.6.0. Due to inactivity by the original maintainer, the remaining xmldom team cannot publish a patched version and have changed the dependency name to @xmldom/xmldom. See xmldom/xmldom#271 Therefore, with azal-node in archive state I am checking if the move to msal has started.

@joshgummersall
Copy link
Contributor

Unfortunately, MSAL only supports a limiter number of Typescript versions. We have aimed to support a wide matrix of Typescript versions. Moving to MSAL would effectively cut that support matrix in half at the moment.

Luckily, I have put together a sample bot with MSAL authentication enabled: https://github.com/joshgummersall/msal-bot

Take a look specifically at msalAppCredentials.ts and msalServiceClientCredentialFactory.ts.

This sample does use the new CloudAdapter, so it may not be a trivial change. However, it should be possible to do.

One other note: this bot also patches JWT validation code to support HTTP proxy. It's probably best to look at the initial commit and ignore the network client/ProxyHttpClient pieces.

@aflinchb
Copy link

aflinchb commented Sep 14, 2021

@Zerryth, @joshgummersall - In the meantime it looks like adal-node has a newer version out that resolves the xmldom vulnerability:
AzureAD/microsoft-authentication-library-for-js#4011

New version is 0.2.3

@joshgummersall
Copy link
Contributor

@aflinchb #3929 handled that! It'll be published through official channels in the coming weeks.

@mrivera-ms mrivera-ms removed this from the R15 milestone Dec 8, 2021
@gabog gabog added P0 Must Fix. Release-blocker and removed P2 Nice to have labels Jan 10, 2022
@gabog gabog added this to the R16 milestone Jan 10, 2022
@gabog gabog added Size: M The issue is not very complex and it is well understood, it will take 1 to 3 days to complete Area: Authentication The issue is related to authenticating users (SSO, OAuth, etc.) labels Jan 10, 2022
@mrivera-ms mrivera-ms modified the milestones: R16, R17 Mar 15, 2022
@ceciliaavila
Copy link
Collaborator

Hi @mrivera-ms, there is a draft PR with this migration, but it was put on hold because msal-node package is not compatible with old versions of Typescript. It supports versions over 3.8 while the SDK checks against versions from 3.3 to 4.1.
image

Should we update the list of supported TypeScript versions so this change can take place?

@SarhadSalam
Copy link

Hello @ceciliaavila @mrivera-ms I am wondering if there have been any updates to this as adal has been deprecated for a while and this might start posing security vulnerabilities going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Authentication The issue is related to authenticating users (SSO, OAuth, etc.) Area: SDK General SDK issues that don't clearly map to other areas (e.g.: helper methods) no parity This issue applies only to this platform P0 Must Fix. Release-blocker Size: M The issue is not very complex and it is well understood, it will take 1 to 3 days to complete
Projects
None yet