Opened 14 months ago

Last modified 7 days ago

#28699 assigned Bug

Document middleware ordering requirements following CSRF change in Django 1.11.6

Reported by: stephanm Owned by: Rodrigo
Component: Documentation Version: 1.11
Severity: Normal Keywords:
Cc: Florian Apolloner, Carlton Gibson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have a problem with csrf protection starting with django 1.11.6
(django 1.11.5 has not this problem).

I am doing all time exactly what is explained in
https://docs.djangoproject.com/en/1.11/howto/auth-remote-user/

My settings:

     MIDDLEWARE = [
        "django.contrib.sessions.middleware.SessionMiddleware",
        "django.middleware.locale.LocaleMiddleware",
        "django.middleware.common.CommonMiddleware",
        "django.middleware.csrf.CsrfViewMiddleware",
        "django.contrib.auth.middleware.AuthenticationMiddleware",
        # "django.contrib.auth.middleware.RemoteUserMiddleware",
        # own middleware because behind proxy we get HTTP_REMOTE_USER 
        # instead of REMOTE_USER
        "lib.auth.middleware.RemoteUserMiddlewareProxy",
        "django.contrib.messages.middleware.MessageMiddleware",
        "django.middleware.clickjacking.XFrameOptionsMiddleware",
    ]
    
    AUTHENTICATION_BACKENDS = [
        # "django.contrib.auth.backends.RemoteUserBackend",
        "lib.auth.backends.RemoteUserBackendTooling",
        # default is:
        "django.contrib.auth.backends.ModelBackend",
    ]
# content of lib.auth.middleware.RemoteUserMiddlewareProxy
from django.contrib.auth.middleware import RemoteUserMiddleware


class RemoteUserMiddlewareProxy(RemoteUserMiddleware):
    header = "HTTP_REMOTE_USER"
# content of lib.auth.backends.RemoteUserBackendTooling
from django.contrib.auth.backends import RemoteUserBackend


class RemoteUserBackendTooling(RemoteUserBackend):

    create_unknown_user = False

    def clean_username(self, username):
        """
        Performs any cleaning on the "username" prior to using it to get or
        create the user object.  Returns the cleaned username.

        By default, returns the username unchanged.
        """
        if username.startswith("IT\\"):
            username = username[3:]
        return username
"

My C# Application does a login by using the normal
"django.contrib.auth.backends.ModelBackend", not using the REMOTE_USER !
It calls a function in my views.py:

    def auth_login_json(request):
        # code...
        # POST data with user & password

Now the csrf protection fails with an http error 403
(only with django 1.11.6, ... django 1.11.5 works)

I found two possibilities to make it work again:

  1. In MIDDLEWARE, comment out "lib.auth.middleware.RemoteUserMiddlewareProxy" but the other remote user login functionality is gone.
  2. I add @csrf_exempt to auth_login_json function like this:
    @csrf_exempt
    def auth_login_json(request):
        # code...
        # POST data with user & password

Reading the changelog https://docs.djangoproject.com/en/1.11/releases/1.11.6/
I suppose this behaviour change comes with https://code.djangoproject.com/ticket/28488

My question: Was I wrong all the years or is this a bug?

Change History (19)

comment:1 Changed 14 months ago by Tim Graham

Cc: Florian Apolloner added
Component: UncategorizedCSRF
Type: UncategorizedBug

comment:2 Changed 14 months ago by Florian Apolloner

Can your share your code/setup? I do not see anything obvious -- your C# app should always have gotten an CSRF error, or did it include a csrf token?

comment:3 Changed 14 months ago by Florian Apolloner

Actually I might have an idea, can you check if commenting out https://github.com/django/django/blob/4d60261b2a77460b4c127c3d832518b95e11a0ac/django/contrib/auth/__init__.py#L128 fixes the issue? This seems to be caused by the auth.login call from the RemoteUserMiddleware which then resets tokens :/

comment:4 Changed 14 months ago by stephanm

Hi Florian,

just commented out the "rotate_token(request)" line in login as you told me.

Now it works again.

Perhaps I am doing something wrong too, I didn't understand exactly the
csrf workflow.
I use Apache on Windows with a plugin which allows me to use NTLM as Single Sign On.
My django runs as reverse proxy and gets the remote_user from apache,
which is intended for the normal users which come with their browsers.

But my c# application does a normal login.

Is there some howto explainig how an external program c#
should login, showing when and how the csrf tokens
appears in the cookies during the HTTP conversation
and what of them should be taken?

Thanks.

comment:5 Changed 14 months ago by Florian Apolloner

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Ok, thanks -- given this I can reproduce it. This is a bug in Django (kinda), but probably a hard one to fix :(

comment:6 Changed 14 months ago by Florian Apolloner

Actually I am still not sure what and why is happening here. How does your C# app login exactly? Ie where from does it get the csrf token and is the C# app affected by the single sign on stuff?

comment:7 Changed 14 months ago by Florian Apolloner

Please restore the original Django 1.11.6 and move

       "django.contrib.auth.middleware.AuthenticationMiddleware",
        "lib.auth.middleware.RemoteUserMiddlewareProxy",

before the CSRF middleware. The issue should be gone then, which will probably mean that the fix will just be a documentation fix.

Last edited 14 months ago by Florian Apolloner (previous) (diff)

comment:8 Changed 14 months ago by stephanm

Hi,

I restored the original Django 1.11.6 and moved the lines you mentioned
*before* before the CSRF middleware and I can confirm that it works now!

Concerning my C# Application, It calls the following
function in my views.py with a GET call to
get the carf_token:

def auth_get_csrf_token_json(request):
    token = csrf(request)
    csrf_token = str(token["csrf_token"])  # ab django 1.5
    response = JsonResponse({"dataType": "csrf", "data": {"csrftoken": csrf_token}})
    # I set the cookie in the past but it seems not necessary
    ##response.set_cookie("csrftoken", csrf_token)
    return response

Note:

  • I send back the csrf token in as json data but in my C# app I use the csrf token which is in the cookie.
  • One strange thing I didn't understand: the csrf token in the returned json data and in the cookie are different

Honestly I was never sure where to get this initial csrf token
to be able to POST my login data.
So I did my experiments until I found this solution which worked for me
(some times ago).

comment:9 Changed 14 months ago by Florian Apolloner

One strange thing I didn't understand: the csrf token in the returned json data and in the cookie are different

Yes, the token changes every request to account for BREACH style attacks. you have to take the first half of it and xor it to the second one (basically) to get the constant "secret" behind it which is reused during the requests.

As for your code:

from django.middleware.csrf import get_token
get_token(request)

in your view should be enough, Django will take care of setting the cookie etc accordingly.

comment:10 in reply to:  7 Changed 14 months ago by stephanm

Replying to Florian Apolloner:

...
before the CSRF middleware. The issue should be gone then, which will probably mean that the fix will just be a documentation fix.

If the documentation fix is about to place AuthenticationMiddleware
before the CsrfViewMiddleware in the MIDDLEWARE setting then you
will have to do more than only changing the docs:

  • You will have to change: django-admin startproject so that it generates the appropriate middleware ordering.
  • You will have to tell everybody that their settings.MIDDLEWARE has to be modified, otherwise some functionality may be broken
  • modify perhaps some other places in the docs i missed ...

Is it really only a documentation fix?

comment:11 Changed 14 months ago by Florian Apolloner

It only affects the RemoteUserMiddleware, which is not enabled by default.

comment:12 Changed 14 months ago by stephanm

Aha ... so, the fix will be whats mentioned in comment:7,
the move of django.contrib.auth.middleware.AuthenticationMiddleware
and the RemoteUserMiddleware... ? (plus fixes in the docs of course)

Right? Or do you plan to do other changes?

I ask this, so I could fix my code now.

comment:13 Changed 14 months ago by Florian Apolloner

Yes, something along those lines will be the final fix. I need to think about it a bit more though, cannot gurantee if or what I missed.

comment:14 Changed 13 months ago by Tim Graham

Component: CSRFDocumentation
Severity: Release blockerNormal
Summary: Problem with CSRF in Django 1.11.6Document middleware ordering requirements following CSRF change in Django 1.11.6

comment:15 Changed 9 days ago by Rodrigo

Has patch: set
Owner: changed from nobody to Rodrigo
Status: newassigned

comment:16 Changed 8 days ago by Carlton Gibson

The PR here looks as if it implements the discussion, but I'm missing a crucial part in my understanding here: I can't see how you're meant to make a successful CSRF check in the same request as a REMOTE_USER login...

As I read it, if RemoteUserMiddleware is first then:

  • RemoteUserMiddleware.process_request() calls auth.login() which calls rotate_token():
def rotate_token(request):
    """
    Change the CSRF token in use for a request - should be done on login
    for security purposes.
    """
    request.META.update({
        "CSRF_COOKIE_USED": True,
        "CSRF_COOKIE": _get_new_csrf_token(),
    })
    request.csrf_cookie_needs_reset = True
  • But then CsrfViewMiddleware.process_request() calls _get_token() which fetches the old CSRF_COOKIE, from either the session or the cookie, and resets it on `request.META`.

Unless I missed something, this is negating the rotate_token() call. Is that correct, or have I misread it? Given the docstring in rotate_token() isn't this a no-no? If so, we can't recommend this.

On the other hand, if CsrfViewMiddleware is first then the rotate_token() call will replace whatever CSRF_COOKIE was previously set, and so the actual CSRF check in process_view() will necessarily fail. (As we must be seeing here.)

Last edited 8 days ago by Carlton Gibson (previous) (diff)

comment:17 Changed 8 days ago by Carlton Gibson

Cc: Carlton Gibson added

comment:18 in reply to:  16 Changed 7 days ago by Florian Apolloner

Replying to Carlton Gibson:

Unless I missed something, this is negating the rotate_token() call. Is that correct, or have I misread it? Given the docstring in rotate_token() isn't this a no-no? If so, we can't recommend this.

Yes, this seems correct

On the other hand, if CsrfViewMiddleware is first then the rotate_token() call will replace whatever CSRF_COOKIE was previously set, and so the actual CSRF check in process_view() will necessarily fail. (As we must be seeing here.)

Also correct. I wonder if my patch in #28488 actually made the situation worse (speaks for the complexity of the middleware :/)

comment:19 Changed 7 days ago by Carlton Gibson

Patch needs improvement: set

Okay. Thank you for following up Florian. I will dig dipper given your confirmation of my first reading.

Note: See TracTickets for help on using tickets.
Back to Top