oliwarner / django-multifactor

Drop-in multifactor authentication subsystem for Django.
MIT License
19 stars 11 forks source link

Redirect too many times error when using FIDO2 and development server 'localhost'. #81

Open maximbelyayev opened 6 months ago

maximbelyayev commented 6 months ago

Hello, In advance, please excuse any obvious errors as I'm relatively new to django and mfa flow.

Background: I've followed the docs and installed django-multifactor, added to INSTALLED_APPS, and I have the exact same MULTIFACTOR settings except for:

'FIDO_SERVER_ID': 'localhost', # Server ID for FIDO request

I launch via latest Chrome version and command and python manage.py runserver_plus localhost:8000 --cert-file cert.pem --key-file key.pem to access through https.

urls.py using latest decorator_include package from StevenMapes fork:

path('admin/multifactor/', include('multifactor.urls')), path('admin/', decorator_include(multifactor_protected(factors=1), admin.site.urls))

Problem: However, after logging into /admin page and setting up FIDO2. Logging out and logging back into admin results in a ERR_TOO_MANY_REDIRECTS where the redirects are between admin/multifactor/add/ and admin/multifactor/authenticate/. This doesn't occur using TOTP.

StevenMapes commented 6 months ago

So the main difference I can see between your setup and the ones I am using in all my projects is that you are decorating all of Django Admin using decorator include then have the multifactors URL as a subfolder to that URL which means it's also protected so that would then say "you need to have passed the MFA check and it'll redirect causing the endless loop until the browser gives up.

What I so is to setup the URLs like this

path('mfa/multifactor/', include('multifactor.urls')),
path('admin/', decorator_include(multifactor_protected(factors=1), admin.site.urls))

That way when you go to and admin/ page you will be redirect to the MFA page under mfa/multifactor/ which is not itself protected. That way the you can then complete the MFA steps and will be redirect back to the admin url you were trying to access.

Give that a go and let me know if it resolves the issue as I expect it will.

Also great to hear you are using my fork of decorator_include, at some-point soon I'm going to split it off to create a new repository so I can push it out to Pypi as it doesn't look like the maintainer of that project is responding to my request to take over the project. I use it heavily so I'm going to keep patching my fork to work with the latest Python and Django combinations

maximbelyayev commented 6 months ago

Thanks for the quick response! Unfortunately, it now redirects between https://localhost:8000/mfa/multifactor/add/ and https://localhost:8000/mfa/multifactor/authenticate/. Trying to also understand the root cause here...

Yes, thanks for your work on decorator_include! A standalone on Pypi would be great.

Edit: Could it have to do with the following? Printing self.available methods results in defaultdict(<class 'list'>, {})

 if not self.available_methods:
     return redirect('multifactor:add')
StevenMapes commented 6 months ago

I must confess I've only used TOTP so I'd need to setup a FIFO one in order to test myself. Hopefully @oliwarner will be able to advise what's happening

maximbelyayev commented 6 months ago

I think it as to do with the following lines under class Authenticate in django-multifactor views.py:

if domain != self.request.get_host():
    other_domains.add(domain)
    continue

Domain is set as localhost whereas self.request.get_host() equates to localhost:8000. This skips the append to self.available_methods later on.

Can't set 'FIDO_SERVER_ID': 'localhost:8000 as it results in ValueError: Invalid origin in CollectedClientData from fido2/server.py

StevenMapes commented 6 months ago

Interesting. In that case can you try upgrading the fido2 package you have installed to the latest version please to see if that resolves anything.

Looking at this PR https://github.com/Yubico/python-fido2/pull/218 it relates to localhost usage.

Update: reading further that seems to be more around allowing http for localhost so it may not actually help

StevenMapes commented 6 months ago

Some further digging

https://github.com/Yubico/python-fido2/issues/122#issuecomment-1054259584 suggests a custom verify_origin function could be used.

Then the following PR on Django-mf3 shows an example of an implementation of using the custom function. https://github.com/xi/django-mfa3/pull/17

I'm not on my laptop at the moment but based on @maximbelyayev comment this if statement could be extended to support checking for localhost

So we change

if domain != self.request.get_host():
    other_domains.add(domain)

To also check and not (domain == "localhost" and self.request.get_host().startswith("localhost:"))

If suggest forking this package, adding a check for that, seeing if that works for you then creating a PR if it does

maximbelyayev commented 6 months ago

Just created PR 82. Minor parsing change to request.get_host() statements. Thanks for the help Steven

StevenMapes commented 6 months ago

So that solved your issue without breaking the TOTP route right

maximbelyayev commented 6 months ago

Yep just tested TOTP

StevenMapes commented 6 months ago

Great! Could I ask one additional thing of you since you have a working FIDO2 implementation, would you be able to upgrade the fido2 dependency to the latest version and check that it still works please. This project is currently pinned to fido2 = '1.1.2' and that package is now on 1.1.3 so it would be useful to know it's still working

StevenMapes commented 6 months ago

I also just realised that this issue is the same as #70

StevenMapes commented 5 months ago

@oliwarner does v0.7 resolve this one, I believe it may so it can be closed if it does.