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

AzureADProvider does not work with default (common) endpoint #8374

Closed
georgy opened this issue Aug 21, 2023 · 26 comments
Closed

AzureADProvider does not work with default (common) endpoint #8374

georgy opened this issue Aug 21, 2023 · 26 comments
Labels
providers triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime.

Comments

@georgy
Copy link

georgy commented Aug 21, 2023

Provider type

Azure Active Directory

Environment

"next" : N/A
"react": N/A
"@auth/core": "0.12.0"
"@auth/sveltekit": "0.3.1"

Reproduction URL

N/A (it is a basic setup)

Describe the issue

I am using simple setup for AzureADProvider. My goal is to allow login with any valid Microsoft identity (I have app registration setup, etc). I am am not providing tenantId will default to common

[auth][error][SignInError]: Read more at https://errors.authjs.dev#signinerror
[auth][cause]: OperationProcessingError: "response" body "issuer" does not match "expectedIssuer"
    at Module.processDiscoveryResponse (file:///<...skipped...>/node_modules/oauth4webapi/build/index.js:232:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async getAuthorizationUrl (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/lib/oauth/authorization-url.js:19:20)
    at async Module.signin (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/lib/routes/signin.js:14:20)
    at async AuthInternal (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/lib/index.js:115:36)
    at async Proxy.Auth (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/index.js:100:30)
    at async Module.respond (/<...skipped...>/node_modules/@sveltejs/kit/src/runtime/server/respond.js:274:20)
    at async file:///<...skipped...>/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:505:22
[auth][details]: {
  "provider": "azure-ad"
}

The root cause is the way Microsoft implemented oidc.

How to reproduce

  1. Configure AzureADProvider provider following the documentation:
export const auth: Handle = SvelteKitAuth({
	trustHost: true,
	secret: AUTH_SECRET,
	providers: [
		AzureADProvider({
			clientId: AZURE_CLIENT_ID,
			clientSecret: AZURE_CLIENT_SECRET,
		}),
	],
});
  1. Try to login with any Microsoft identity

Expected behavior

User is redirected to the Microsoth login page and login works...

@georgy georgy added providers triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Aug 21, 2023
@georgy
Copy link
Author

georgy commented Aug 21, 2023

It is possible to bypass the discovery phase by specifying authorization and token urls like such:

export const auth: Handle = SvelteKitAuth({
	trustHost: true,
	secret: AUTH_SECRET,
	providers: [
		AzureADProvider({
			clientId: AZURE_CLIENT_ID,
			clientSecret: AZURE_CLIENT_SECRET,
			authorization: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
			token: 'https://login.microsoftonline.com/common/oauth2/v2.0/token',
		}),
	],
});

Then I can see the login page and can login. But JWT validation fails:

[auth][cause]: OperationProcessingError: unexpected JWT "iss" (issuer) claim value
    at validateIssuer (file:///<...skipped...>/node_modules/oauth4webapi/build/index.js:961:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async processGenericAccessTokenResponse (file:///<...skipped...>/node_modules/oauth4webapi/build/index.js:918:32)
    at async Module.processAuthorizationCodeOpenIDResponse (file:///<...skipped...>/node_modules/oauth4webapi/build/index.js:1010:20)
    at async handleOAuth (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/lib/oauth/callback.js:79:24)
    at async Module.callback (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/lib/routes/callback.js:14:41)
    at async AuthInternal (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/lib/index.js:64:38)
    at async Proxy.Auth (file:///<...skipped...>/node_modules/@auth/sveltekit/node_modules/@auth/core/index.js:100:30)
    at async Module.respond (/<...skipped...>/node_modules/@sveltejs/kit/src/runtime/server/respond.js:274:20)
    at async file:///<...skipped...>/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:505:22
[auth][details]: {
  "provider": "azure-ad"
}

The problem here is that Azure AD service returns issuer with tenantId of the organization to which user's account belongs. This means that issuer does not match the one that was built by the lib (the one with 'common' alias for tenantId)

@amplicity
Copy link

amplicity commented Aug 29, 2023

+1 I'm having the same exact issue, and have tried @georgy's explicit definition for authorization and token, get the same exact response.

@amplicity
Copy link

I'm not sure how to best implement this and test, but I believe what needs to happen is something to the effect of modifying @auth/core/src/providers/azure-ad.ts to check for token.profile.tid and using it in the issuer string if defined. I saw on successful logins from within a my tenant (when AZURE_AD_TENANT_ID is defined), that tenantId is equivalent to profile.tid.

azure-ad.ts

export default function AzureAD<P extends AzureADProfile>(
  options: OAuthUserConfig<P> & {
    /**
     * https://docs.microsoft.com/en-us/graph/api/profilephoto-get?view=graph-rest-1.0#examples
     *
     * @default 48
     */
    profilePhotoSize?: 48 | 64 | 96 | 120 | 240 | 360 | 432 | 504 | 648
    /** @default "common" */
    tenantId?: string
  }
): OAuthConfig<P> {
  const { tenantId = "common", profilePhotoSize = 48, ...rest } = options
  if (rest.profile.tid){
    tenantId = rest.profile.tid
  }
  rest.issuer ??= `https://login.microsoftonline.com/${tenantId}/v2.0`
  return {
    id: "azure-ad",
    .... // rest of code

I'm not sure how to best test this or try it out, I haven't modified a node module before I had issues running patch-package. @georgy do you have any ideas how we could accomplish this potentially from the config side, without modifying the source? I did try custom jwt encodes and decodes via the jwt option like seen below, but got the same error. Perhaps I didn't modify the the tenantId correctly. I'd love further input!

NextAuth({
  jwt{
    encode: ..
    decode: ..
  }
})

@amplicity
Copy link

@georgy were you able to resolve?

@RonB
Copy link

RonB commented Oct 15, 2023

very sorry... premature commit by accident...

@RonB
Copy link

RonB commented Oct 16, 2023

@georgy & @amplicity

I have tried something in addition to the first part of the solution.


        AzureADProvider({
            clientId: AZURE_AD_CLIENT_ID,
            clientSecret: AZURE_AD_CLIENT_SECRET,
            authorization: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
            token: 'https://login.microsoftonline.com/common/oauth2/v2.0/token',
            issuer: `https://login.microsoftonline.com/${AZURE_AD_COMMON_TENANT_ID}/v2.0`
        }),

by adding an issuer to the config with the proper issuer tenantid it works. The id itself I cannot find in the Azure portal, so I had to grab it from the network. Also, I am not sure if Microsoft keeps this id the same or if it might change....
Please let me know if you think this might be a solution...

@jaysin586
Copy link

@RonB The issue with your solution is that the issuer is different based on the domain or if its public. For examples the issuer for peter@pan.com and peter@live.com will have different values. For some reason the system is not returning common as its issuer, instead its returning the owner of the id as the issuer.

@RonB
Copy link

RonB commented Oct 27, 2023

Yes i noticed that, works for one other domain only which was the case in my situationeel. Thanx for noticing. Any idea how to solve this?

@jaysin586
Copy link

@RonB It looks like there is a standoff between the Azure team and the OAuth2 teams MicrosoftDocs/azure-docs#38427

Their issues are not my issues, but now that I know there is an issue I can dig in and let you know what I find.

@osdiab
Copy link

osdiab commented Nov 9, 2023

So is it the case that the common tenant ID (and therefore using next-auth/AuthJS with Microsoft Login for general, non-enterprise users) is fundamentally unusable?

@balazsorban44 it would be very helpful if in the documentation for Azure AD, this was plastered in big letters that AuthJS is fundamentally incompatible with Microsoft Login, and if you care about Microsoft authentication at all you should use a different library. It is very misleading to claim that next-auth is a general purpose auth library and yet refuse to support one of the biggest auth providers, regardless of their standards-noncompliant nature.

It's looking like either I need to fork next-auth and roll my own security; or stop using next-auth altogether and migrate my company away from next-auth, either of which will be a big pain 😮‍💨

@georgy
Copy link
Author

georgy commented Nov 9, 2023

@osdiab you are correct, as it stands you cannot use common unless you pin your app to a single tenant (which negates the use of common endpoint). The problem is not in next-auth directly, but in oauth4webapi. There is no simle way to fix this. Solution could be to have a callback for issuer validation (https://github.com/panva/oauth4webapi/blob/main/src/index.ts#L2826). oauth4webapi provides a number of extension points via callbacks that next-auth uses, don't see any reasons not to add one more.

@osdiab
Copy link

osdiab commented Nov 9, 2023

Welllll for now I'll do this stupid dirty hack.

  • I made a patch for oauth4webapi that just ignores the issuer check if it happens to be a specific Microsoft common tenant issuer; here is what the patch looks like:

https://github.com/panva/oauth4webapi/compare/main...osdiab:oauth4webapi:lax-for-microsoft?expand=1&w=1

  • Then I used a pnpm patch to apply this change
  • make sure oauth4webapi is explicitly declared as a dependency in your package.json

Patch file (last updated 2023-12-14 to apply @cbdabner 's suggestion):

oauth4webapi@2.3.0.patch

How to use:

  • put this file in a patches folder in the root of your repo
  • add the following to pnpm config (replace 2.3.0 in the key with whatever version is actually installed in your repo, this is the version as of now. it's ok if the patch file is not called 2.3.0 so long as it applies properly):
{
  // ...
  "pnpm": {
    "patchedDependencies": {
      "oauth4webapi@2.3.0": "patches/oauth4webapi@2.3.0.patch"
    },
  }
}

CAUTION: if you bump the oauth4webapi version, make sure you also bump it in this version here, or else the patch will no longer apply

and for completeness for anyone who is unsure how to use this, this is how I set up the Azure auth when instantiating AuthJS:

// we use the common tenant to accept any Microsoft account
const microsoftTenantId = "common";
const microsoftConfig = AzureADProvider({
  // copied from the Essentials section in the Azure Console, Microsoft Entra ID,
  // make an App Registration, get the client ID after done with that
  clientId: process.env["AZURE_AD_CLIENT_ID"], 
  // In the App Registration, click on the Client Secrets link, make a secret
  clientSecret: process.env["AZURE_AD_CLIENT_SECRET"],
  tenantId: microsoftTenantId,
  // need to override these URLs to skip the Discovery phase, to sidestep OIDC
  // validation issues
  token: {
    url: `https://login.microsoftonline.com/${microsoftTenantId}/oauth2/v2.0/token`,
  },
  userinfo: { url: "https://graph.microsoft.com/oidc/userinfo" },
  authorization: {
    url: `https://login.microsoftonline.com/${microsoftTenantId}/oauth2/v2.0/authorize`,
    params: { scope: "openid profile email User.Read" },
  },
  issuer: `https://login.microsoftonline.com/${microsoftTenantId}/v2.0`,
});

I would appreciate if anyone who actually understands OIDC security could suggest other checks that would be good to have instead of just dropping the issuer check altogether, or to advise if this is actually safe to skip. I am unaware of the negative security impact of just skipping the issuer check entirely. Thanks for the support @cbdabner and @blaine !

Aside: This doesn't check for a sts.windows.net issuer since you shouldn't get that value so long as the acceptedTokenVersion is 2 in your Microsoft auth config, which it should be if you're setting it up with defaults

@sandonl
Copy link

sandonl commented Dec 5, 2023

Following

@cbdabner
Copy link

@osdiab, re: your patch, you should check the iss claim matches the tenant returned in the custom tid claim:

function validateIssuer(expected, result) {
  if (expected === 'https://login.microsoftonline.com/common/v2.0'
    && result.claims.tid !== undefined
    && result.claims.iss === `https://login.microsoftonline.com/${result.claims.tid}/v2.0`) {
    return result;
  }
  ...

@blaine
Copy link

blaine commented Dec 13, 2023

@cbdabner's fix is the most correct one; @osdiab's pnpm patch approach works great here. Thanks both!

It's a shame that it's taken years for Microsoft to resolve this, since Microsoft has long helped to guide the OAuth and OIDC specification work. Big companies, I guess. 🙃

As a note to the next-auth authors, while I applaud your stance urging conformance to the spec, I'd encourage you to bake in the workaround (even if it's via a snarky flag like enableWorkaroundForMicrosoftNonCompliantImplementation). I'm the original creator of OAuth, and the goal was always to (1) simplify the user experience for users and (2) create secure, reusable, open-source implementations.

Ultimately, those of us who find our way to this issue are just trying to get people signed in, and most aren't concerned with the state of Microsoft's standards compliance. Resorting to one-off hacks in many codebases, versus a "supported" workaround will leave us all less secure.

Keep pressing on Microsoft, though! They should know better, but encouraging your users to leave reports on their issue tracker will have an effect. Also, thanks for your work on next-auth! 💜

@osdiab
Copy link

osdiab commented Dec 14, 2023

I updated my post to include the suggested fix!

@MrLoh
Copy link

MrLoh commented Dec 19, 2023

Thanks for the help from everyone here to figure out this OIDC issue.

When I apply this method, I do get forwarded to the Microsoft login screen, but the authentication still fails with the following, has anyone else encountered this, I can't find any further useful information about what went wrong here specifically.

[auth][debug][callback route error details] {
  method: 'GET',
  query: { code: '<some code>' },
  body: undefined
}
[auth][error][CallbackRouteError: Read more at https://errors.authjs.dev#callbackrouteerror]

@osdiab
Copy link

osdiab commented Dec 20, 2023

hard to say without more details - can you make sure that the callback/jwt implementations don't throw?

  • The signIn or jwt callback methods threw an uncaught error: Check the callback method implementations.

@MrLoh
Copy link

MrLoh commented Dec 20, 2023

Yeah I already did add logging to my callbacks, which don't throw, as well as around the image fetching in the profile, which never gets called. I'm a bit clueless where else to add logging inside next-auth to get some useful information about what went wrong. I guess I'll just wait with upgrading to next-auth@5 until this is more stable.

@MrLoh
Copy link

MrLoh commented Dec 21, 2023

Would a PR be accepted that allows switching back to the old OAuth (no OIDC) workflow on the Azure AD provider for common tenant? Not working for one of the biggest providers seems to totally defeats the purpose of this library.

@OrionSeven
Copy link

I'd very much appreciate a work around officially in Authjs, I know Microsoft isn't conforming properly, but they're one of the biggest OAuth players out there next to Google and Apple. Not having easy support for them kinda defeats the purpose of Authjs. I like @blaine's suggestion of a snarky flag, and have the Authjs documentation link to where people can put pressure on MS.

@dennishammarstrand
Copy link

@osdiab Your solution didn't fix it for me. Still get:

[auth][error] CallbackRouteError: Read more at https://errors.authjs.dev#callbackrouteerror [auth][cause]: OperationProcessingError: unexpected JWT "iss" (issuer) claim value

@osdiab
Copy link

osdiab commented Jan 25, 2024

@dennishammarstrand i can’t comment without knowing how your project is set up, but I would start by using the pnpm patch command to insert a console log or something into the function to make sure that the patch is actually applied, and maybe inspect what is in iss to see if it’s something expected.

@dennishammarstrand
Copy link

dennishammarstrand commented Jan 25, 2024

@osdiab Sorry was just me that was doing it wrong, did it the yarn way and it works great! Thanks a lot for your solution! 👍

@balazsorban44
Copy link
Member

balazsorban44 commented Jan 25, 2024

Tracking this via #9635 (comment). You can also follow #9718

closing as duplicate

@famence
Copy link

famence commented May 24, 2024

I had the same issue with "consumers" tenant and fixed it by simply replace consumers with 9188040d-6c67-4c5b-b112-36a304b66dad. Since "consumers" is actually a nickname for this tenant id.
I believe that a "common" is also a nickname so you can try to find its tenant id and use it instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
providers triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime.
Projects
None yet
Development

No branches or pull requests