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

Regeneration of a key not possible #83

Open
Plaenkler opened this issue Aug 10, 2023 · 6 comments
Open

Regeneration of a key not possible #83

Plaenkler opened this issue Aug 10, 2023 · 6 comments

Comments

@Plaenkler
Copy link

Plaenkler commented Aug 10, 2023

🔍 Problem Description:

I am encountering a problem where I cannot recover the QR code after restarting my application. The key.Secret() is the only information that is permanently stored. However, it seems impossible to recover the same key using only the secret.

💡 Relevant Code Snippet:

In the code section below from totp.go (lines 182-192), I initially believed that the key could be recovered only from the secret.

v := url.Values{}
if len(opts.Secret) != 0 {
    v.Set("secret", b32NoPadding.EncodeToString(opts.Secret))
} else {
    secret := make([]byte, opts.SecretSize)
    _, err := opts.Rand.Read(secret)
    if err != nil {
        return nil, err
    }
    v.Set("secret", b32NoPadding.EncodeToString(secret))
}

However, recent tests have contradicted this assumption.

🔬 Test Results:

fmt.Println(keySecret)
key, err := totp.Generate(totp.GenerateOpts{
    Issuer:      issuer,
    AccountName: accountName,
    Secret:      []byte(keySecret),
})
fmt.Println(key.Secret())

keySecret: JRKDOQRSJJBDIVCRLJJEQS22KBGQINGFQMRUGZHDGNKYINBUSRKI
key.Secret(): KKKJFUIT2RKJJUUSSCIREVMQ2SJRFEURKRKMZDES2CI5IQJFHEORSRJVJFKR22JBCEOTSLLFEU4QSVKNJEWS

🛠️ Environment:

Operating System: Windows X86@64
Golang Version: 1.21
Library version used: pqerna/otp v1.4.0

🏁 Expected Outcome:

My expectation is to get a key with the exact same secret when entering the secret.

🚫 Actual Outcome:

Because the entered secret is encoded again, another secret is created.

v.Set("secret", b32NoPadding.EncodeToString(opts.Secret))

📝 Solution:

I assume that this decision was made on purpose so I provide a wrapper that decrypts the entered secret first.

func Regenerate(opts GenerateOpts) (*otp.Key, error) {
	if opts.SecretSize == 0 {
		return nil, otp.ErrRegenerateMissingSecret
	}
	secret := make([]byte, base32.StdEncoding.DecodedLen(len(opts.Secret)))
	_, err := base32.StdEncoding.Decode(secret, opts.Secret)
	if err != nil {
		return nil, err
	}
	opts.Secret = secret
	return Generate(opts)
}
@Plaenkler Plaenkler changed the title Reproduction of the key incorrect Regeneration of a key not possible Aug 11, 2023
@pquerna
Copy link
Owner

pquerna commented Aug 21, 2023

I'm not sure this needs a new function, named Regenerate, but rather more clarity about the code flows for serialization and deserialization.

@Plaenkler
Copy link
Author

Plaenkler commented Aug 22, 2023

I'm not sure this needs a new function, named Regenerate, but rather more clarity about the code flows for serialization and deserialization.

Of course, a note in the documentation and an example could help here. However, I think the user should not be responsible for the decoding of the secret. Basically, as a user, I would assume that the set option "Secret", which is also a field of the key is used unprocessed. The function does not introduce any breaking changes, which is why I would prefer it to a correction in the documentation.

@pquerna
Copy link
Owner

pquerna commented Aug 23, 2023

Need to be clear here, its not "encrypted"; Its base32 encoded.

@Plaenkler
Copy link
Author

I had made a mistake in my choice of words. Regardless of whether it is encoded or encrypted, the library should provide a way to recover the key from the secret.

@DavesPlanet
Copy link

Plaenkler, thanks for saving my hide with your example! I would never have figured that out.
One minor tweek, the code does not seem to run without allocating space for the buffer

var secret []byte

needed to become this for me to get it to work

secret := make([]byte, base32.StdEncoding.DecodedLen(len(opts.Secret)))

@Plaenkler
Copy link
Author

Plaenkler, thanks for saving my hide with your example! I would never have figured that out. One minor tweek, the code does not seem to run without allocating space for the buffer

var secret []byte

needed to become this for me to get it to work

secret := make([]byte, base32.StdEncoding.DecodedLen(len(opts.Secret)))

Thanks for the info have adjusted it in the pull request

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