Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
maximbelyayev opened this issue May 1, 2024 · 11 comments

Comments

@maximbelyayev
Copy link

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
Copy link
Collaborator

StevenMapes commented May 1, 2024

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
Copy link
Author

maximbelyayev commented May 1, 2024

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
Copy link
Collaborator

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
Copy link
Author

maximbelyayev commented May 1, 2024

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
Copy link
Collaborator

StevenMapes commented May 1, 2024

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 Yubico/python-fido2#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
Copy link
Collaborator

Some further digging

Yubico/python-fido2#122 (comment) 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. xi/django-mfa3#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
Copy link
Author

maximbelyayev commented May 1, 2024

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

@StevenMapes
Copy link
Collaborator

So that solved your issue without breaking the TOTP route right

@maximbelyayev
Copy link
Author

Yep just tested TOTP

@StevenMapes
Copy link
Collaborator

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
Copy link
Collaborator

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

StevenMapes added a commit to StevenMapes/django-multifactor that referenced this issue May 2, 2024
Implementing the change suggested in oliwarner#70 which is the same issue reported in oliwarner#81.

This adds support for FIDO2 and using "localhost" as the ID whilst testing on the host localhost:8000. It essentially removes the port from the host if any port is specified
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants