search

xi / django-mfa3

multi factor authentication for django
MIT License
22 stars 8 forks source link

TOTP_VALID_WINDOW setting #11

Closed tobiasmboelz closed 2 years ago

tobiasmboelz commented 2 years ago

This pull request adds a setting for the valid_window value passed to PyTOTP’s verify method.

In my tests users with a certain authenticator app couldn’t log in unless I changed the code to call verify with valid_window=1 (or higher). I added a setting instead of just changing the value as increasing the value has security implications and not everyone would want to have this change.

Please let me know if I should add a test or some additional documentation.

xi commented 2 years ago

Thanks for the contribution! The code looks good to me, and I think exposing a lower level parameter sounds reasonable.

But just to be sure: What is causing this issue? Is this a bug in the authenticator app? The spec discusses this issue:

A larger acceptable delay window would expose a larger window for attacks. We RECOMMEND that at most one time step is allowed as the network delay.

tobiasmboelz commented 2 years ago

Thanks for the contribution! The code looks good to me, and I think exposing a lower level parameter sounds reasonable.

But just to be sure: What is causing this issue? Is this a bug in the authenticator app?

The app in question is Google Authenticator for Android. I think it’s rather particular behaviour than a bug. As far as I can tell it only supports generating codes every 30 seconds and seems to expect that a code remains valid for 60 seconds.

The spec discusses this issue:

A larger acceptable delay window would expose a larger window for attacks. We RECOMMEND that at most one time step is allowed as the network delay.

Yes, you probably shouldn’t set valid_window to anything lager than 1.

The next section of the spec says:

Because of possible clock drifts between a client and a validation server, we RECOMMEND that the validator be set with a specific limit to the number of time steps a prover can be "out of synch" before being rejected.

And it suggests to do automatic resynchronisation, but PyOTP doesn’t support that.

xi commented 2 years ago

OK, sounds solid to me.

I will make a release as soon as #10 as been resolved.