Closed redbmk closed 1 year ago
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 https://github.com/dorinclisu/fastapi-auth0/issues/27 for a similar misunderstanding about a feature that was achievable using scopes and permission management.
No valid reason to keep open.
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.
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
tojwt.decode
. This means that ifaud
is present, then it will assert it's what you pass in toaudience
, 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 inrequire_aud=True
or having an option in theAuth0
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.
By default
aud
is not a required field injwt.decode
. See:"require_aud": False,
_validate_aud
won't raise an issue if there is noaud
claimIt 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 relatedpython-jose
issue