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

OTP incompatibility with the new AKV parser #2462

Closed
Hazegard opened this issue Dec 10, 2022 · 6 comments · Fixed by #2465
Closed

OTP incompatibility with the new AKV parser #2462

Hazegard opened this issue Dec 10, 2022 · 6 comments · Fixed by #2465
Labels
bug Defects

Comments

@Hazegard
Copy link
Contributor

Summary

Since gopass 1.5, the gopass otp XXX command returns :

Error: Failed to compute OTP token for XXXXX: Decoding of secret as base32 failed.

I investigated this issue and it seems related to the new parser introduced in 1e7a6b1.

Indeed, the separator is hardcoded to : (see https://github.com/gopasspw/gopass/blob/master/pkg/gopass/secrets/akv.go#L13)

While my OTP lines has the form:

otpauth://totp/ZZZ?secret=XXXXXX

I tried:

  • modifying the line to match the separator:
otpauth: //totp/ZZZ?secret=XXXXXX&algorithm=SHA1&digits=6&period=30 
  • modifying the kvSep variable to :

both modifications allowed gopass to properly generate the OTP code

The documentation specifies using otpauth:// (https://github.com/gopasspw/gopass/blob/d00c29a0e8e521eb32e4564998690430dcc7dbbc/docs/commands/otp.md), so I'm not sure if I should update the new format given that it breaks compatibility with other applications such as https://github.com/android-password-store/Android-Password-Store.

Steps To Reproduce

Run gopass otp XXXX with the XXX file having the following otp format:

otpauth://totp/ZZZ?secret=XXXXXX&algorithm=SHA1&digits=6&period=30 

Gopass returns the error:

Error: Failed to compute OTP token for XXXXX: Decoding of secret as base32 failed.

Expected behavior

Gopass properly generates the OTP code

Environment

  • OS: Linux
  • OS version:

Linux 6.0.11-arch1-1 #1 SMP PREEMPT_DYNAMIC Fri, 02 Dec 2022 17:25:31 +0000 x86_64 GNU/Linux

  • gopass Version:

gopass 1.15.0 go1.19.4 linux amd64
- gpg 2.2.40 - gitfs 2.38.1
Available Crypto Backends: age, gpgcli, plain
Available Storage Backends: fossilfs, fs, gitfs

  • Installation method: ArchLinux repository

Additional context

@dominikschulz
Copy link
Member

The documented format should still work and it did definitely work in our test cases. Not sure what's wrong here.

@dominikschulz dominikschulz added the bug Defects label Dec 10, 2022
@Hazegard
Copy link
Contributor Author

Thanks for your answer.

I tried with your last commit and it works now.

After digging, it seems related to the line added here:
https://github.com/gopasspw/gopass/blob/master/pkg/gopass/secrets/akv.go#L304

Without this line, the Body parsed in the Calculate function (https://github.com/gopasspw/gopass/blob/master/pkg/otp/otp.go#L25) has the form:

[OTHER_ENTRIES]otpauth://totp/ZZZ?secret=XXXXXX[OTHER_ENTRIES]

So it fails the check https://github.com/gopasspw/gopass/blob/master/pkg/otp/otp.go#L26

With this line the body is:

[OTHER_ENTRIES]
otpauth://totp/ZZZ?secret=XXXXXX
[OTHER_ENTRIES]

So now the OTP line is properly parsed and gopass generates the OTP as expected!

I close this issue as I consider it fixed 😄

@dominikschulz
Copy link
Member

Amazing. Thank you.

I guess the reason why this didn't show up in testing was that I only tested with entries that only had the otpauth entry in the body and nothing else.

Also the 1.15.0 release was missing some test coverage. I'm glad we fixed that already. Will make sure to have a new patch release out very soon.

@dominikschulz
Copy link
Member

Let me reopen this. I want to add some test-cases for this case.

@dominikschulz dominikschulz reopened this Dec 11, 2022
dominikschulz added a commit to dominikschulz/gopass that referenced this issue Dec 11, 2022
RELEASE_NOTES=n/a

Fixes gopasspw#2462

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
dominikschulz added a commit that referenced this issue Dec 11, 2022
RELEASE_NOTES=n/a

Fixes #2462

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

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
@colemickens
Copy link
Contributor

Sure is good that I have an old version in my nix store or I'd be extremely screwed right now. Thanks for the fast report and fix.

@dominikschulz
Copy link
Member

This should be fixed in 1.15.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants