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.

Sign up for GitHub

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

Jump to bottom

Use github.com/pquerna/otp to allow using the key period #2278

Merged
merged 5 commits into from
Aug 2, 2022
Merged

Use github.com/pquerna/otp to allow using the key period #2278

merged 5 commits into from
Aug 2, 2022

Conversation

dominikschulz
Copy link
Member

RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes #2276

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

@dominikschulz dominikschulz added the bug Defects label Jul 18, 2022
@dominikschulz dominikschulz marked this pull request as ready for review July 19, 2022 06:47
@dominikschulz dominikschulz marked this pull request as draft July 19, 2022 06:48
@dominikschulz dominikschulz requested a review from AnomalRoil July 19, 2022 19:29
@dominikschulz dominikschulz force-pushed the fix/issue-2276 branch 2 times, most recently from 8e1950f to 7bfd575 Compare July 20, 2022 20:04
@dominikschulz dominikschulz marked this pull request as ready for review July 20, 2022 20:04
@dominikschulz
Use github.com/pquerna/otp to allow using the key period
7e213e4 
RELEASE_NOTES=[BUGFIX] Use OTP key period

Fixes gopasspw#2276

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
AnomalRoil
AnomalRoil reviewed Aug 2, 2022
View reviewed changes
pkg/otp/otp.go Show resolved Hide resolved
pkg/otp/otp.go Show resolved Hide resolved
pkg/otp/otp.go Outdated Show resolved Hide resolved
internal/action/otp.go Show resolved Hide resolved
internal/action/otp.go Outdated Show resolved Hide resolved
@AnomalRoil
Copy link
Member

So, my tests show that the old binary and the new binary both work fine with TOTP and give the same value as Google Authenticator.

But the HOTP part isn't working with the new binary.

@dominikschulz
Address review comments.
2a136c8 
Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@dominikschulz
Copy link
Member Author

I'm not sure if I ever used HTOP. Do you have an example that I could use to reproduce the issue?

@AnomalRoil
Copy link
Member

I guess
otpauth://hotp/hotp@example.com?secret=N6CIHPYDGHI6QC4ZV7Q3S3FXA22BZOCY&issuer=example.com&digits=6&algorithm=SHA1&counter=1 should work.

Old binary gives 683787 with &counter=1 and 044228 with &counter=2

AnomalRoil
AnomalRoil previously approved these changes Aug 2, 2022
View reviewed changes
internal/action/otp.go Outdated Show resolved Hide resolved
internal/action/otp.go Outdated Show resolved Hide resolved
@dominikschulz
Implement digits and algorithm parameter parsing
d19479a 
Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@dominikschulz
Use proper formatting and add logging
264299e 
Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@dominikschulz
Make linters happy
d6291f3 
Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@dominikschulz dominikschulz merged commit 4cb569a into gopasspw:master Aug 2, 2022
@dominikschulz dominikschulz deleted the fix/issue-2276 branch August 2, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnomalRoil AnomalRoil AnomalRoil approved these changes

Assignees
No one assigned
Labels
bug Defects
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TOTP period parameter is ignored
2 participants
@dominikschulz @AnomalRoil