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

Decimal time window #144

Closed
ghost opened this issue Jan 29, 2021 · 7 comments · Fixed by #152
Closed

Decimal time window #144

ghost opened this issue Jan 29, 2021 · 7 comments · Fixed by #152

Comments

@ghost
Copy link

ghost commented Jan 29, 2021

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? / Specification no
Library version 10.0(.1)

Currently, the OTP time window can only be an integer:
public function verify(string $otp, ?int $input = null, ?int $window = null): bool;

This parameter is a multiplier for the period set while generating the OTP;
so by using an integer it is impossible to create a time window smaller than the period itself.

Example:
Google Authenticator only accepts a period equal to 30 seconds (not less, not more).
I would like to validate the OTP in a time window which goes by timestamp - 45s to timestamp + 45s.
Currently this is impossible because I cannot set a time window equal to 0.5 (15 seconds).

@Spomky
Copy link
Member

Spomky commented Jan 29, 2021

Hi @blackhole997,

What you are describing here looks possible with the current method arguments by passing a custom timestamp.

$isVerified = $totp->verify($otp);
if (!$isVerified) {
    $isVerified = $totp->verify($otp, time()-15);
}

//Now $isVerified can be true even with an OTP that expired less than 15 seconds before

@ghost
Copy link
Author

ghost commented Jan 29, 2021

Hi @Spomky,
many thanks for the reply.

What you suggests surely works - I didn't think about doing it in that way.

So, tell me if I'm wrong:

  • The window parameter should be used for 'gross' adjustements
  • The input parameter should be used for more accurate adjustments.

I think that obtaining the same result using the same parameter could be more elegant, but this is my personal opinion.
If you don't agree I will implement the solution as you suggested.

@Spomky
Copy link
Member

Spomky commented Jan 29, 2021

The input parameter has different meaning depending on the OTP type:

  • With TOTP, it corresponds to the timestamp
  • With HOTP, it corresponds to the counter

The window parameter allows to test counters/periods before or after the current counter/timestamp.
For TOTP, you are right it can be considered as a gross adjustement.

I think that obtaining the same result using the same parameter could be more elegant, but this is my personal opinion.
If you don't agree I will implement the solution as you suggested.

I agree with you. Such leeway seems better that a hard windows which may induce security issues.
As it will change the behaviour of the parameter, I don't think it will be implement in the current major branch. But could be done in a new major one (v11).

@Spomky Spomky self-assigned this Jan 29, 2021
@Spomky Spomky added this to the V11 milestone Jan 29, 2021
@stale
Copy link

stale bot commented Mar 31, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 31, 2021
@Spomky Spomky removed the wontfix label Mar 31, 2021
@Spomky Spomky pinned this issue Mar 31, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 2, 2021
@Spomky Spomky added the pinned label Jun 8, 2021
@stale stale bot removed the wontfix label Jun 8, 2021
@Spomky Spomky mentioned this issue Feb 15, 2022
@Spomky Spomky linked a pull request Feb 15, 2022 that will close this issue
@Spomky
Copy link
Member

Spomky commented Feb 15, 2022

Hi @blackhole997,

This will be part of the v11.
The window parameter will change and this will be considered as a time drift feature.
The new behavior will fix this but also prevent misuse of the window parameter.

Can you tell me if this new v11 is what you expected here?
Please refer to the changes in #152 and in particular these lines.

Regards.

@Spomky
Copy link
Member

Spomky commented Feb 18, 2022

PR #152 is now merged

@Spomky Spomky closed this as completed Feb 18, 2022
@Spomky Spomky unpinned this issue Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant