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

TOTP period parameter is ignored #2276

Closed
stefan2904 opened this issue Jul 18, 2022 · 2 comments · Fixed by #2278
Closed

TOTP period parameter is ignored #2276

stefan2904 opened this issue Jul 18, 2022 · 2 comments · Fixed by #2278
Assignees

Comments

@stefan2904
Copy link

stefan2904 commented Jul 18, 2022

Summary

I configure a TOTP token with a custom period (other than 30).

Unfortunately, gopass currently uses a hardcoded default value of 30:
https://github.com/gopasspw/gopass/blob/master/internal/action/otp.go#L23

Steps To Reproduce

Add a secret with a TOTP and period=60 like the following:

% gopass show testForGopassIssue 
Secret: testForGopassIssue

someSecretPassword
otpauth: //totp/demoTest?secret=bcde23456&algorithm=SHA256&digits=6&period=60

Then retrieve the current token:

% gopass otp testForGopassIssue
245371
This OTP password still lasts for:
]  3 / 29 [Goooooopass                                                                                              ]  10.34%

with a period of 30, instead of the period length of 60 given via the otpauth:// URI.

Expected behavior

Use the period-value as given via the otpauth:// URI.

Environment

  • OS: Ubuntu 21.10
  • OS version: Linux t480ssm 5.13.0-52-generic # 59-Ubuntu SMP Wed Jun 15 20:17:13 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
  • gopass Version: 1.14.0 go1.18 linux amd64
  • Installation method: deb package diretly

Additional context

@dominikschulz
Copy link
Member

It's a limitation of the OTP library we use.

We might be able to switch to https://pkg.go.dev/github.com/pquerna/otp#NewKeyFromURL that seems to export the Period length.

@dominikschulz dominikschulz self-assigned this Jul 18, 2022
@AnomalRoil
Copy link
Member

A quick look seems to indicate we might really have multiple issues here, it seems:

  • We are currently parsing the URL, if it's in the "otpauth" key-value, which should respect the period specified by the URL:
    return twofactor.FromURL(otpURL) //nolint:wrapcheck
  • The fall back is using a Google TOTP function that's assuming 30 seconds period, tho.
  • The "timer" and "progress bar" is totally not taking this into account, sadly. But running in "password only mode" should work, ignoring the progress bar and display the right tokens according to the custom period:
    gopass otp -o entry/path 
    
  • Also small bonus, we are possibly logging password here, which is something we want to never do:
    return otp, label, fmt.Errorf("invalid OTP secret %q: %w", secKey, err)

dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jul 18, 2022
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jul 18, 2022
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jul 20, 2022
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jul 20, 2022
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jul 21, 2022
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Jul 23, 2022
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit that referenced this issue Aug 2, 2022
* Use github.com/pquerna/otp to allow using the key period

RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes #2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Address review comments.

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Implement digits and algorithm parameter parsing

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Use proper formatting and add logging

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Make linters happy

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
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

Successfully merging a pull request may close this issue.

3 participants