Token admin page leaks access tokens into log files

[jarshwah]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• I have reduced the issue to the simplest possible case.
• I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Visit the change page for a Token in Django admin. Since the primary key is the key, the key is used to reference the token in the URL. This leaks the auth token into access logs.

The access permissions for users with access to the admin page (high) and those with permissions to view logs (medium) are different.

Expected behavior

Auth tokens should use an integer as the primary key that is used in urls and for foreign key references. The token value itself should be a non-keyed attribute with a unique index.

Actual behavior

Primary key is the secret key material.

[carltongibson]

OK. Yep. Not ideal.

The answer to token related questions has traditionally been that the supplied implementation is deliberately (over-)simple and that you should use a custom implementation if you need something more complex/better. (Bottom line here is that we've steered away from adding any more complexity here to keep the maintenance burden reasonable.)

The access permissions for users with access to the admin page (high) and those with permissions to view logs (medium) are different.

I think this isn't true for people who would/should be using the supplied implementation in production.
(In those cases they'd be one and the same person.)

Not sure what others might say...

[jarshwah]

The answer to token related questions has traditionally been that the supplied implementation is deliberately (over-)simple and that you should use a custom implementation if you need something more complex/better.

I get this position, but then the docs should strongly recommend users away from the built in implementation at a minimum, and deprecate at the extreme end of things. The 3rd party recommendation for token/signature auth is https://github.com/etoccalino/django-rest-framework-httpsignature which hasn't been updated in 3 years. I would imagine there'd be a lot more interest in 3rd party token providers if one wasn't provided out of the box.

This is a pretty serious information disclosure IMO, so I'm not sure the default position for authtoken is the right way to go in this case.

[carltongibson]

No. Which is why I didn’t close it...

[jarshwah]

Sorry if my response came off rougher than intended, that was not my goal.

[carltongibson]

No problems! 😃

(Wondering if we could just mung the ModelAdmin to use a hash of the key in the url...)

[jarshwah]

Sneaky, but that idea feels dangerous too, though I can't say why.

A migration should be able to handle the job. The tricky bit would be foreign keys in other tables, but that seems unlikely. DRF doesn't create FKs to Tokens, and I can't see many reasons that users would either.

I don't think I've tried to migrate primary key fields before, but I seem to remember there being an operation that handles it, and the corresponding foreign key updates too.

[tomchristie]

Yup. It’d need a data migration, but otherwise I guess easy enough to switch around.

[tomchristie]

Pull requests welcome. Thanks for the report!

[jarshwah]

Does the token need to necessarily be unique?

[anx-ckreuzberger]

I understand the problem, but you should consider that depending on what migration you create (to solve this issue), you might cause many others issues (e.g., introducing a new PK field will most likely break some other packages that rely on the current behaviour).

I wonder if it is possible to add an auto-increment integer field on the token table which is used as a reference within the Django Admin Page for the Auth Token, but not as the Primary Key of the table?

---

FYI, I've just verified that I also have the same issue with my extended token implementation (see django-rest-multitokenauth) - so thanks for pointing that mistake in the Admin Panel out! I'm looking forward to see the solution here, so I can adapt my python package.

[anx-ckreuzberger]

FYI, this is how I fixed it within my MultiTokenAuth package:
anexia/drf-multitokenauth@7e11ed6

I guess the same fix could be applied here, with the caveat of potentially breaking other peoples code if they have a foreign key to the token table

[raunaqss]

Hello,

I was just skimming through auth related issues before evaluating this library for an upcoming project. I have a simple question, would appreciate any help.

Can this issue be avoided by simply not registering the Token model in admin?

[tomchristie]

@raunaqss Yes, you don't need to register it.

Tho thanks for re-raising this.

[PawelMorawian]

Do we know which way we are going with this ?
Should we hash the url or migrate the pk ?

I can make a PR.

[tomchristie]

A migration will very likely be the sensible approach here.
I'm pretty wary about including a migration in the 3.11 release, since eg. it could be problematic/unexpected for folks with very large API key tables.
I think a sensible thing to do will probably be to start by releasing a migration to this seperately, so that we can make sure some folks are able to test it all out first independently of our releases.
Once we're happy with it all then we can consider including the migration directly.

[tomchristie]

Let's have a look at releasing a migration for this seperately, rather than pushing it in the 3.11 release itself. I'm too wary of implications for users with very large API key tables to push a migration on all our users in a major release.

[woodrad]

This issue has been open for a long time, and, while it seems @jarshwah's observation is addressed by django-rest-knox, we probably want to be more secure by default. Even if drf's intention is for TokenAuthentication to be simple, many developers are bound to implement an insecure authentication mechanism. I'm not sure code that leaks secret material by design should be included out of the box.

Has any progress been made in resolving this issue? Is anyone open to taking up a bounty on the issue? I would be interested in contributing code or paying a bounty if so.

[tomchristie]

I'll prfioritize it for 3.12.

[tomchristie]

I would be interested in contributing code

Very welcome to issue a PR for it, sure.

I would be interested in contributing code or paying a bounty if so.

Becoming a sponsor is a good way to contribute. Sponsors have priority support and can esclate an issue if needed. https://fund.django-rest-framework.org/topics/funding/#corporate-plans

[woodrad]

That all sounds great! I'm now a sponsor of drf and encode. I'll be sure to update this issue if/when I start work on a resolution.

[tomchristie]

Fantastic, thank you so much. 🙏

[tomchristie]

Okay, so it's really not obvious how to tackle this gracefully.

We'd really like the model to just use an auto incrementing int for the primary key, and have the "key" field be a standard unique, indexed field, but we can't migrate to that. So what are our options...

• We could introduce something like rest_framework.authtoken2 and have the docs refer to that instead, so that new projects get a better set of defaults.
• We could continue to use the existing field as the PK, and add a new field for the actual token value. It's not clear if we could introduce a data migration copying the values across, as it might? be problematic for users with large existing data sets. This looks decent... https://stackoverflow.com/questions/41500811/copy-a-database-column-into-another-in-django We'd also need to be pretty careful about still correct behaviour for unmigrated codebases. If we were working on a deployed app we'd normally want to start by introducing the new field, and ensuring that we're writing the same values to both fields while still having the codebase refer to the old field for a period of time. And only once that's all deployed & sync'ed, introduce the code change switching to using the new field.
• We could continue as we are but introduce some admin changes.

Does anyone have any preliminary thoughts on this?

Also: This is why this one has been pending for so long - there's no easy answer to it. 😬

[carltongibson]

I opened #7341 as a look at the "introduce some admin changes" option.