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

Feature: Extra enforcement of claims #29

Closed
redbmk opened this issue Jan 18, 2023 · 4 comments
Closed

Feature: Extra enforcement of claims #29

redbmk opened this issue Jan 18, 2023 · 4 comments

Comments

@redbmk
Copy link

redbmk commented Jan 18, 2023

By default aud is not a required field in jwt.decode. See:

It would be great to set those as required fields on the jwt, if not by default at least via an option.

It would also be great to be able to validate the token is issued for a given Auth0 Application (Client ID), which is set as azp. See related python-jose issue

@dorinclisu
Copy link
Owner

dorinclisu commented Jan 22, 2023

I can't comment on what makes or not sense in python-jose, but I can certainly comment that your request for fastapi-auth0 does not make sense.

Firstly, in auth0 tokens are not issued FOR an application (client id), they are issued WITHIN an application for an api (audience) on behalf of a user (the user can be the application itself if M2M).

Secondly, what is your use case and what are you trying to achieve that is not already easily and securely achieved with the current implementation? See #27 for a similar misunderstanding about a feature that was achievable using scopes and permission management.

@dorinclisu
Copy link
Owner

No valid reason to keep open.

@redbmk
Copy link
Author

redbmk commented Mar 23, 2023

Thanks for the bump - sorry I missed your first response a couple months ago.

Like you said this could be a misunderstanding on my part on how auth0 is supposed to be set up, but essentially we have a webapp with a FastAPI backend deployed in multiple environments (e.g. dev, qa, prod, etc). In auth0 we set up an application to represent this one webapp, and then we have different auth0 APIs (audiences) to represent each environment (also specific to this app).

What I've noticed in auth0 is that there's not a direct connection between an application and an API. So you can have Application A and Application B, and as long as they're in the same tenant either one can request the API X audience and they'll get a valid jwt.

So, in our API, we want to validate that a) the jwt is coming from the expected domain, b) it's issued from the expected application, and c) it's issued for the correct API/environment.

Essentially we're trying to validate the azp, aud, and iss and make them all required.

With the current implementation the audience (aud) is not actually required in order to be valid because we're not passing in require_aud=True to jwt.decode. This means that if aud is present, then it will assert it's what you pass in to audience, but if it's not present, it will still be considered valid. I don't know if that's actually even possible with auth0, but it seems prudent to double check that it's there. This could be fixed by either always passing in require_aud=True or having an option in the Auth0 constructor for it.

Also with the current implementation, there's no way to validate that the application (azp) is coming from Application A (or B or whatever). It doesn't appear that jose even has an option for this (I filed a feature request but there hasn't been any response yet), but it would be simple enough to add validation if that's a common pattern (sounds like maybe it isn't?).

Again, maybe a lot of this is just a misunderstanding of how auth0 is supposed to work and maybe we've set things up incorrectly. But based on the way we have it set up, I'd like to make sure everything we expect to be there is present and what we expect to see. Here's what we've done as a workaround to add extra validation:

from fastapi_auth0.auth import Auth0, Auth0UnauthorizedException
from fastapi_auth0.auth import Auth0User as Auth0UserBase
from pydantic import validator

from .config import auth0_settings

class Auth0User(Auth0UserBase):
    """
    Adds validation for claims that aren't currently being checked by
    fastapi_auth0/jose, or aren't completely vetted. In those libraries:

        - Client ID (azp) is not checked at all
        - Audience (aud) is checked only if it's present, but isn't required on the jwt
        - Issuer (iss) is already required and validated
    """

    azp: str
    aud: str | list[str]

    @validator("azp")
    @classmethod
    def validate_client_id(cls: type["Auth0User"], azp: str | None) -> str:
        if not azp:
            raise Auth0UnauthorizedException("Expected an azp claim")

        if azp != auth0_settings.client_id:
            raise Auth0UnauthorizedException("Invalid authorized party")

        return azp

    @validator("aud")
    @classmethod
    def require_audience(cls: type["Auth0User"], aud: str | None) -> str:
        """
        Validation is already done in fastapi_auth0 if the `aud` claim is present,
        so here we just need to make sure it's actually present. Validation is slightly
        more complicated for this one because `aud` can be an array as well as a string
        """
        if not aud:
            raise Auth0UnauthorizedException("Expected an aud claim")

        return aud


auth0 = Auth0(
    domain=auth0_settings.domain,
    api_audience=auth0_settings.api_audience
    auth0user_model=Auth0User,
)

app.include_router(
    asdf.router,
    dependencies=[Depends(auth0.implicit_scheme), Depends(auth0.get_user)]
)

Overall, this workaround wasn't too bad and maybe is the recommended way to extend validation to the base class. It just seems like it would be convenient to have a couple extra options to do the validation directly in the Auth0 constructor if this is a common enough use case.

I'd be happy to submit a PR if you think it's worth adding.

@dorinclisu
Copy link
Owner

dorinclisu commented Mar 26, 2023

So, in our API, we want to validate that a) the jwt is coming from the expected domain, b) it's issued from the expected application, and c) it's issued for the correct API/environment.

Points a) and c) are already taken care of via iss and aud claims validation. Point b) is not possible to do securely, at least not the way you are looking at it. I'll detail later.

With the current implementation the audience (aud) is not actually required in order to be valid because we're not passing in require_aud=True to jwt.decode. This means that if aud is present, then it will assert it's what you pass in to audience, but if it's not present, it will still be considered valid. I don't know if that's actually even possible with auth0, but it seems prudent to double check that it's there. This could be fixed by either always passing in require_aud=True or having an option in the Auth0 constructor for it.

Well that might be a valid concern. But all the auth0 documentation suggests that aud WILL be there.

Sure it wouldn't hurt to explicitly make it mandatory in decode() if not already, but this requires a bit more study because to me the param documentation seems a bit ambiguous. And again if you look at the code from auth0 staff above, none of it uses the options argument.

Also with the current implementation, there's no way to validate that the application (azp) is coming from Application A (or B or whatever). It doesn't appear that jose even has an option for this (I filed a feature request but there hasn't been any response yet), but it would be simple enough to add validation if that's a common pattern (sounds like maybe it isn't?).

If you look at all the JWT libraries here https://jwt.io/libraries, none of them have an azp check. To me that's a good sign it's either not needed, or relying on it may be harmful by deviating from the standards and best practices.

Now concretely to your issue, the problem I see with your approach is that, because the application is a public concept, there's nothing fundamentally preventing a user of application A (meant for API A) to also request an access token via application B (meant for API B), hence getting access to API B. You can see now that the concept of checking the azp becomes useless for restricting access.

So what's the right way to do it securely? With permissions of course, that's exactly what they're meant for. So with API A you setup for example permission app:A or env:Prod and grant these permissions only to the users meant to access application A or the prod environment. This can be facillitated by auth0 user groups, where you assign the permission set to the group and then just put the user in the group. Just remember to validate the api permissions (scopes) by injecting them in a fastapi Security context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants