scheb/2fa-google-authenticator: \OTPHP\TOTP::verify()'s $leeway argument is misused

[zerkms]

Bundle version: 6.9.0
Symfony version: 5.4
PHP version: 8.2.10
Using authenticators (enable_authenticator_manager: true): YES

Description

More than a year ago in da37027 spomky-labs/otphp dependency was bumped from ^10.0 to ^10.0 || ^11.0 with no other code changes whatsoever.

The problem is that in v11 otphp redesigned their \OTPHP\TOTP::verify() API and replaced the $window argument with a $leeway. The former was an index of a time slice, the latter is actually time in seconds. Which makes those 2 apis not interchangeable.

At the moment the totp library is used as:

2fa/src/google-authenticator/Security/TwoFactor/Provider/Google/GoogleAuthenticator.php

Line 34 in 5bd6698

return $this->totpFactory->createTotpForUser($user)->verify($code, null, $this->window);

And $this->window holds a time slice offset which is set to 1 by default.

So what happens next is - if you upgrade all dependencies and receive otphp version 11, then $this->window in fact used as a $leeway number of seconds, 1 second by default. So instead of entire +1 time slice (30 seconds) back in time, you get just 1 second back.

So I suggest to drop support of totp v10, or implement feature detection and depending on the totp version detected in runtime - call it with different arguments.

Other important point: not totp library deliberately DOES NOT support time offset larger or equal to the period duration. It means that if before you could set $this->window = 2 and have 1 minute behind, plus 30 seconds now, plus 1 minute in the future: 2.5 minutes altogether, or 5 period slices. So now with a 30 seconds duration only leeways <= 29 are allowed.

So if 2fa-google-authenticator needs to support windows larger than 1, then 2fa-google-authenticator should manually provide current time and iterate over time points with 0 $leeway.

To Reproduce

Additional Context

I can deliver the fix if nobody from the team is keen to do that, and as long as we agree on the implementation details.

[scheb]

This is known an documented behavior (https://github.com/scheb/2fa/blob/6.x/doc/configuration.rst). I made the deliberate decision to not do the effort and deal with it in the bundle, instead just passing through the value as-is. Yes, it's an inconvenience, but it's not critical to the functionality of the bundle.

We're not going to change the behavior of that configuration value, because it would break expected behavior of those people, already using it according to v11 spec.

I can deliver the fix if nobody from the team is keen to do that, and as long as we agree on the implementation details.

The "team" is me as a single person 😅. I'm not keen to do it, because I'm not considering it being important enough to invest my free time on it.

If you're up to providing a solution, I believe the most reasonable way to solve it would be to add a new configuration option that activates the classic "leeway = X * period" behavior. Then the ambiguous "window" option could be deprecated and removed with the next major release.

For simplicity, I'd be willing to drop support for v10. It's there to support PHP 8.0 environments. Since PHP 8.0 will be end of life on 26 Nov 2023, I believe it's fair to drop PHP 8.0 support from the bundle. According to Packagist it's just <2% of installs on 8.0, so imo no longer worth the hassle maintaining the support for that version.

[zerkms]

Right, if it was known it should have been a bundle major release? As it breaks literally everyone's configuration - that's how I found it.

But nonetheless, I need only "1" window back, which means I can live with new "29" seconds configuration too.

And given it's "expected" and documented - I'm not willing to provide a "solution" too.

[talkinnl]

It's their code and decision, but the behavior change should really have been mentioned in the v10 to v11 upgrade notes.

Code which doesn't break but does something completely else might be more dangerous than an actual syntax error.

If the note was there, nobody would allow ^10.0 || ^11.0 since that won't work deterministically. Dropping v10 then is easier then making up for the weird change and/or leaky abstraction.

[zerkms]

should really have been mentioned in the v10 to v11 upgrade notes.

Authors of that library appear to never specify which changes are breaking and which not. So whoever bumped a dependency here should have checked each change, since the major bump happened for a reason 🤷

[scheb]

This is addressed with v6.11.0. The window option is now deprecated and will be removed in bundle version 7.