jazzband / django-oauth-toolkit

OAuth2 goodies for the Djangonauts!
https://django-oauth-toolkit.readthedocs.io
Other
3.13k stars 792 forks source link

django-oauth-toolkit and keycloack #1320

Open fijemax opened 1 year ago

fijemax commented 1 year ago

Describe the bug Token field to short to use with keycloack oidc instropsection endpoint

To Reproduce Create a client than support oidc login on keycloack and use django-oauth-toolkit to valide the query by using introspection endpoint

Version lastest

When I configure django-oauth-toolkit with keycloack and try to validate an Authorization header with a jwt token, it appear than after the introspection validation the token used can't be store in the token field of TokenAccess model because the limit is 255.

value too long for type character varying(255)

is it a normal behavior ? Did I do something wrong ?

Leelith commented 1 year ago

Hi, I faced the same problem, did you manage to solve it?

fijemax commented 1 year ago

Hello, yes to solve my problem I overided django-oauth-toolkit classes with my own classes and I changed the limit of token field in the AccessToken class by overiding it in.

oauth/models.py

from django.db.models import CharField
from oauth2_provider import models

class Application(models.AbstractApplication):
    pass

class Grant(models.AbstractGrant):
    pass

class AccessToken(models.AbstractAccessToken):
    token = CharField(
        max_length=2048,
        unique=True,
        db_index=True,
    )

class RefreshToken(models.AbstractRefreshToken):
    pass

class IDToken(models.AbstractIDToken):
    pass

settings.py


OAUTH2_PROVIDER_APPLICATION_MODEL = "oauth.Application"
OAUTH2_PROVIDER_ACCESS_TOKEN_MODEL = "oauth.AccessToken"
OAUTH2_PROVIDER_REFRESH_TOKEN_MODEL = "oauth.RefreshToken"
OAUTH2_PROVIDER_ID_TOKEN_MODEL = "oauth.IDToken"
OAUTH2_PROVIDER_GRANT_MODEL = "oauth.Grant"
Leelith commented 1 year ago

Hello, yes to solve my problem I overided django-oauth-toolkit classes with my own classes and I changed the limit of token field in the AccessToken class by overiding it in.

oauth/models.py

from django.db.models import CharField
from oauth2_provider import models

class Application(models.AbstractApplication):
    pass

class Grant(models.AbstractGrant):
    pass

class AccessToken(models.AbstractAccessToken):
    token = CharField(
        max_length=2048,
        unique=True,
        db_index=True,
    )

class RefreshToken(models.AbstractRefreshToken):
    pass

class IDToken(models.AbstractIDToken):
    pass

settings.py


OAUTH2_PROVIDER_APPLICATION_MODEL = "oauth.Application"
OAUTH2_PROVIDER_ACCESS_TOKEN_MODEL = "oauth.AccessToken"
OAUTH2_PROVIDER_REFRESH_TOKEN_MODEL = "oauth.RefreshToken"
OAUTH2_PROVIDER_ID_TOKEN_MODEL = "oauth.IDToken"
OAUTH2_PROVIDER_GRANT_MODEL = "oauth.Grant"

Thank you for your help!

dopry commented 12 months ago

This looks like a reasonable bug. We may in fact want to fully support access token JWTs in the future. We currently only do JWTs for identity tokens and we don't store the compose token for those. Would anyone be open to submitting a PR to switch the token property to a TextField? @n2ygk can you think of any reasons not to make the token a TextField? Would that impact indexing?

tonial commented 11 months ago

Hi @dopry, It may depend on the database, but in PostgreSQL, there's no performance difference : https://www.postgresql.org/docs/current/datatype-character.html

n2ygk commented 11 months ago

This looks like a reasonable bug. We may in fact want to fully support access token JWTs in the future. We currently only do JWTs for identity tokens and we don't store the compose token for those. Would anyone be open to submitting a PR to switch the token property to a TextField? @n2ygk can you think of any reasons not to make the token a TextField? Would that impact indexing?

@dopry You'd want to make sure that indexing works for the major supported Django databases. It looks like there is at least some limitation in mysql which I'm not sure is even supported via Django models. There's a documented mysql limitation on uniqueness.

It might be safer to add a separate text attribute.

dopry commented 11 months ago

@n2ygk a separate text field for the full access token?

n2ygk commented 11 months ago

@n2ygk a separate text field for the full access token?

@dopry Yeah for the JWT. I'm not sure why they would ever be needed for an Access Token but I guess that's what keycloak does. Why would one use the introspection endpoint to validate a JWT? It's cryptographically signed. I guess an AT is theoretically any opaque value so can include a JWT. But if it's too big to index then maybe store a cryptographic checksum to facilitate looking it up? mysql's max indexable length is 255.

This feels like something we discussed a while ago w.r.t. JWTs.

dopry commented 11 months ago

I'd like to be certain about spec compliance before we make any decision about whether we shouldn't. But I am inclined to agree with @n2ygk and to not make this change. I'll read up on it next time I have a few cycles for DOT if no one else gets to it first and summarizes it.

I can definitely see the utility for edge use cases where the token may be the only session asset you have... But feel that people could use the id_token in that case and maybe we should lead them in that direction.

arnaud-penet commented 3 months ago

Hi, this looks to similar to https://github.com/jazzband/django-oauth-toolkit/issues/1412, maybe a single PRs should be aimed.

I can definitely see the utility for edge use cases where the token may be the only session asset you have... But feel that people could use the id_token in that case and maybe we should lead them in that direction.

If I understand correctly, the id_token is not recommended to be used in place of an access_token . Which legitimates the need to a full support of unlimited size JWT access_token, IMHO !

dopry commented 2 months ago

passing around a full JWT in place of an access token risks exposing user details to any parties that manage to intercept it which could be used to further a targeted attack which is why access tokens are recommended to be opaque and for id tokens not to be used in their place. That said in practice I have and have seen others use a JWT as an access token. Regardless of the validity of the use case, the performance limitations for indexing need to be addressed before we can implement this.

@arnaud-penet do you have time to work on this PR. It isn't something I need so I'm probably not going to work on it. happy to review it if someone wants to work on it.