Default output of `random_base32()` is not valid base32

[tommilligan]

Relates to #109 Introduced in 9576711d5de1b0873056ab668b409473a97e3a9c

The current output of the random_base32() function is a string of base32 alphabet characters, of 26 length. This is not a valid base32 string, as it does not include padding to a length multiple of 8.

This causes problems when it is used as the secret value for a TOTP, like the output of TOTP.provisioning_uri changing depending on whether or not TOTP.verify has previously been called:

# "S46SQCPPTCNPROMHWYBDCTBZXV" is a sample output from random_base32() that exhibits buggy behaviour

In [29]: code = pyotp.totp.TOTP("S46SQCPPTCNPROMHWYBDCTBZXV")

In [30]: code.provisioning_uri()
Out[30]: 'otpauth://totp/Secret?secret=S46SQCPPTCNPROMHWYBDCTBZXV'

In [31]: code.verify("000000")
Out[31]: False

# This should give the same output, but it doesn't
In [32]: code.provisioning_uri()
Out[32]: 'otpauth://totp/Secret?secret=S46SQCPPTCNPROMHWYBDCTBZXV%3D%3D%3D%3D%3D%3D'

More importantly, it introduces undefined behaviour when interoperating with other TOTP libraries, such as node's speakeasy. The example secret below is the same in both examples, but produces different codes in each library:

In [16]: pyotp.totp.TOTP("S46SQCPPTCNPROMHWYBDCTBZXV").at(datetime.fromtimestamp(1612380872))
Out[16]: '100172'

> speakeasy.totp({"secret":"S46SQCPPTCNPROMHWYBDCTBZXV","encoding":"base32","time":1612380872})
'184825'

This is flaky behaviour, as a different base32 alphabet string of length 26 does give the same codes between libraries. I imagine how the two libraries handle invalid base32 differs in implementation detail.

In [36]: pyotp.totp.TOTP("A4QGCTHL3HNMC3NAW2OT45WWWA").at(datetime.fromtimestamp(1612380872))
Out[36]: '080982'

> speakeasy.totp({"secret":"A4QGCTHL3HNMC3NAW2OT45WWWA","encoding":"base32","time":1612380872})
'080982'

To fix this, I suggest increasing the default length of the generated string to 32, which is a multiple of 8.

[kislyuk]

Thanks for reporting and researching the problem.

While your assessment is correct in general applications of the base32 encoding, the specific implementation in PyOTP is specific to the otpauth:// key URI schema defined here: https://github.com/google/google-authenticator/wiki/Key-Uri-Format

In particular, https://github.com/google/google-authenticator/wiki/Key-Uri-Format#secret states:

REQUIRED: The secret parameter is an arbitrary key value encoded in Base32 according to RFC 3548. The padding specified in RFC 3548 section 2.2 is not required and should be omitted.

So the issue you describe appears to be a bug in the speakeasy library's base32 parsing logic as applied to the variant of base32 used in the otpauth schema.

[kislyuk]

Actually it looks like there are multiple issues here. The inconsistency in provisioning_uri output after calling verify() is definitely a bug, I've addressed it in ee827b4cc6f52317bcf6c0db1a482fc81b203fae.

[kislyuk]

Actually since the RFC recommends 160 bits, I'm going to go ahead and follow your suggestion, requiring base32 strings of minimum length 32.

[kislyuk]

Fixes released in v2.6.0, please test.

[tommilligan]

Thanks for the fast response and patches - after reading the spec I agree that the current behaviour for unpadded values is correct, both for genreation and interpretation.

I will investigate the speakeasy library further and raise an issue there if required.

I can confirm the behaviour of the new release looks good in our test suite.