django-DefectDojo icon indicating copy to clipboard operation
django-DefectDojo copied to clipboard

Added middleware to set request.user from AuthToken

Open 0x4d4e opened this issue 2 years ago • 4 comments

See https://github.com/DefectDojo/django-DefectDojo/issues/6468

PoC to set request.user in a custom middleware when TokenAuthentication is used for API access.

TODO:

  • better alternatives?
  • middleware ordering?
  • exception handling
  • tests!

0x4d4e avatar Jun 28 '22 13:06 0x4d4e

Added CSRF bypass for requests using TokenAuthentication. Now it feels even more hackish :P

By setting request.user in the new middleware, Django assumes the request is authenticated with SessionAuthentication and thus enforces verification of the CSRF Token. The underlying issue is again, that TokenAuthentication/DjangoRestFramework would only authenticate/authorize a request after the middlewares were run. By adding request.user early (to make the usual DefectDojo authorization happy), we are suddenly in uncharted territory (somewhere 'between' SessionAuthentication/non-REST and Token/REST-API) where authentication/authorization is handled by the non-REST Django functionality (i.e. assuming Session-based authentication and behavior).

I'd be happy to contribute further on this issue, but I think that someone with expertise about the authentication/authorization logic in Django/RestFramework and DefectDojo is required, at least to provide some guidance/prevent further issues down the road.

0x4d4e avatar Jul 04 '22 09:07 0x4d4e

I have run into this issue as well, but I fear the added middleware approach is a bit too hackish for me. I'll do some thinking on this.

@StefanFl I did not fully understand this PR until looking closely at the associated issue. This issue has occured in my prod env when the VM would restart do to unattended upgrades. On reboot, our pipeline would fuss and fail for a few requests, and then resolve itself. My suspicion is that that the request_cache is being dumped on restart. What are your thoughts?

Maffooch avatar Jul 14 '22 15:07 Maffooch

I also think that caching somehow fixes this issue (at least for the same user - not sure since we are currently only using the admin to access the API). However, the self-healing in repeated requests does not work reliably in all instances.

Furthermore, all API requests using Token authentication are missing the user attribute. This also affects the audit logging, which shows no user in the log output for API requests.

We are currently using the hack on top of the latest release to fix the restart issue and so far it works reliably in multiple environments.

0x4d4e avatar Jul 14 '22 16:07 0x4d4e

I agree this issue should be addressed and I agree with both of you that this approach might not be the best one. Unfortunately I don't have a better idea at the moment :-(

StefanFl avatar Jul 31 '22 06:07 StefanFl

All, I would suggest we disable the cache. I am against taking user input in this fashion which could be malicious or have other security implications. This issue can be fixed by disabling the cache.

devGregA avatar Sep 27 '22 20:09 devGregA

@0x4d4e I may have fixed this in a PR recently. Are you able to reproduce this issue when running 2.14.3?

Maffooch avatar Sep 30 '22 06:09 Maffooch

@Maffooch Seems to be fixed - updating the version on a stack without my patch worked without the previously required manual login. Thanks!

Out of interest, did removing the cache fix the issue or were some other changes required as well? I tried removing (some of) the caching annotations previously but saw no difference in the behavior.

0x4d4e avatar Oct 03 '22 07:10 0x4d4e