8000 issuer option can be a callback by ItalyPaleAle · Pull Request #102 · auth0/idtoken-verifier · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

issuer option can be a callback #102

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
Closed
wants to merge 4 commits into from
Closed

issuer option can be a callback #102

wants to merge 4 commits into from

Conversation

ItalyPaleAle
Copy link
Contributor
@ItalyPaleAle ItalyPaleAle commented Mar 16, 2020 •
edited
Loading

Changes

This is related to #100, even though the issue with version 2.0 was something different (see #101). This can still be a nice improvement in my opinion.

The problem: When using Azure AD in multi-tenant mode (when the authorization URL is https://login.microsoftonline.com/organizations/oauth2/v2.0/authorize, for example; note the organizations part), the returned id_token contains an iss that contains the actual tenant ID (e.g. https://login.microsoftonline.com/de366b7a-4861-4e17-b67b-8e3cdd4f2408/v2.0). There is no way to know in advance what the tenant of the user that will sign in is, and instead we need to look at what's inside the tid claim in the id_token. This is a not-really-standard behavior, but I understand why it works this way.

My previous workaround (that works with 2.0 too, save for the bug in issue #100), was to decode the JWT before passing it to idtoken-verifier, get the tid and build the expected value for the iss claim:

// Replace the tenant id in the issuer. We need to parse the jwt and look at the tid property
// We already know that the format is right
let tenant
try {
    const [, jwtPayloadB64] = jwt.split('.')
    const jwtPayloadStr = DecodeString(jwtPayloadB64)
    const jwtPayload = JSON.parse(jwtPayloadStr)
    tenant = jwtPayload && jwtPayload.tid
}
catch (e) {
    throw Error('JWT payload is invalid')
}
if (!tenant) {
    throw Error('Tenant ID missing in payload')
}

// Get the issuer
// AUTH_ISSUER looks like `https://login.microsoftonline.com/{tenant}/v2.0`
const issuer = process.env.AUTH_ISSUER.replace('{tenant}', tenant)

// Validate the token
const verifier = new IdTokenVerifier({
    jwksURI: process.env.JWKS_URL,
    issuer,
    audience: process.env.AUTH_CLI
8000
ENT_ID
})

In short, it requires me to decode the JWT twice, and do a bunch of manual work.

This PR makes it possible to pass a function as the issuer option in the IdTokenVerifier constructor. The function receives the id_token's full payload as input, and returns a string with the expected value for the iss claim. With this PR, the code above becomes much cleaner:

const verifier = new IdTokenVerifier({
    jwksURI: process.env.JWKS_URL,
    issuer: (payload) => {
        if (!payload || !payload.tid) {
            return process.env.AUTH_ISSUER
        }
        return process.env.AUTH_ISSUER.replace('{tenant}', payload.tid)
    },
    audience: process.env.AUTH_CLIENT_ID
})

PS: I haven't updated the documentation yet, but I'll do it if you show interest in this PR

Checklist

@ItalyPaleAle ItalyPaleAle requested a review from a team March 16, 2020 07:51
stevehobbsdev
stevehobbsdev requested changes Mar 16, 2020
View reviewed changes
Copy link
Contributor
@stevehobbsdev stevehobbsdev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this @ItalyPaleAle! Just left a comment below.

In addition to the documentation, we will also need appropriate unit test coverage to be added.

@ItalyPaleAle
Copy link
Contributor Author

@stevehobbsdev thanks for the quick review. I've made the changes as requested

@stevehobbsdev stevehobbsdev self-requested a review March 17, 2020 12:31
stevehobbsdev
stevehobbsdev approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor
@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

Thanks @ItalyPaleAle!

@stevehobbsdev
Copy link
Contributor

@lbalmaceda Could you also evaluate this please?

stevehobbsdev
stevehobbsdev previously approved these changes Mar 17, 2020
View reviewed changes
@lbalmaceda
Copy link
Contributor

At first glance, it doesn't feel correct, as you're letting an unverified token to tell you with that tid claim how to verify its iss claim. Let me raise this internally.

@ItalyPaleAle
Copy link
Contributor Author

@lbalmaceda I understand your point, but I don't think there's an alternative to support scenarios such as multi-tenant Azure AD. It will be the responsibility of the developer to add more checks if/when necessary.

In my case, the risk is mitigated by the fact that I am expecting an issuer formatted like https://login.microsoftonline.com/{tenant}/v2.0, and I'm happy to accept whatever tenant the token is claiming. Because the tokens are signed by the Azure AD service, I can trust that the server provided me with the correct value.

Additionally, apps that are restricted to a few tenants only can limit the range of allowed values. (Apps that are restricted to a single tenant shouldn't have this issue).

@lbalmaceda lbalmaceda dismissed stevehobbsdev’s stale review March 17, 2020 22:02

Holding off: Internal discussion ongoing

@lbalmaceda
Copy link
Contributor

@ItalyPaleAle I've raised this internally and we're thinking on the best approach in the long term, not just for "issuer" claims. Please, give us a few days to make an informed decision. I'll post an update soon.

@ItalyPaleAle
Copy link
Contributor Author

Thank you!

@lbalmaceda
Copy link
Contributor

@ItalyPaleAle Apologies for the delay. This request is still scheduled to be discussed next week for you. I'll keep you posted

@lbalmaceda
Copy link
Contributor

@ItalyPaleAle To update you, we've come to an agreement on how we want to support these "special" scenarios that go out of the spec. The idea is to accept custom claim validators in an extensible manner. How that will look in terms of the public API is still ongoing. But think of functions that are invoked from the validator and given the context they need to know to decide whether a received claim's value is correct or not.

In the meantime, please do continue to use the workaround you presented on the first comment. I'll post another update in the coming weeks. 👋

@ItalyPaleAle
Copy link
Contributor Author

Sounds good.

But please, don't over-complicate the interface too much :) That is, don't force me to write a full custom validator (for each field) if what I need is just a different validator for iss and I'm fine with the standard validator otherwise. Just a suggestion!

@ItalyPaleAle
Copy link
Contributor Author

Hey there, just wondering if you have any update on this?

@stevehobbsdev
Copy link
Contributor

Hi @ItalyPaleAle , the update is that I will be working on a plan for the API to handle this scenario this week, so I don't imagine implementation is too far off. Will keep you posted here.

@stevehobbsdev
Copy link
Contributor

Hi @ItalyPaleAle,

We've gone through some discovery internally as to what this implementation would look like and we all agree that if we bring this level of customisation, it has to be done in the right way and not done just for one property. Unfortunately, this is low priority given lots of other initiatives that we have in play at the moment and have ultimately decided against implementing it at this stage due to the effort.

Therefore, I will close this PR in the meantime and thank you for the time you've put into it. If this comes back later I will try to remember to raise visibility here.

@ItalyPaleAle
Copy link
Contributor Author

Hey @stevehobbsdev, any update on this?

@stevehobbsdev
Copy link
Contributor

@ItalyPaleAle Not as yet unfortunately. If that changes I will try and report back here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevehobbsdev stevehobbsdev stevehobbsdev left review comments

@lbalmaceda lbalmaceda Awaiting requested review from lbalmaceda

Assignees
No one assigned
Labels
CH: Changed feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ItalyPaleAle @stevehobbsdev @lbalmaceda
0